Re: [PATCH v2 1/4] dt-bindings: mfd: rk806: allow to customize PMIC reset mode

From: Quentin Schulz
Date: Tue Jun 17 2025 - 06:45:19 EST

On 6/17/25 12:21 PM, Krzysztof Kozlowski wrote:
On 17/06/2025 11:38, Quentin Schulz wrote:
Hi Krzysztof,

On 6/17/25 10:08 AM, Krzysztof Kozlowski wrote:
On Thu, Jun 05, 2025 at 05:41:06PM GMT, Quentin Schulz wrote:
+ rockchip,reset-mode:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [0, 1, 2]
+ description:
+ Mode to use when a reset of the PMIC is triggered.
+
+ The reset can be triggered either programmatically, via one of
+ the PWRCTRL pins (provided additional configuration) or
+ asserting RESETB pin low.
+
+ The following modes are supported (see also
+ include/dt-bindings/mfd/rockchip,rk8xx.h)
+
+ - 0 (RK806_RESTART) restart PMU,
+ - 1 (RK806_RESET) reset all power off reset registers and force
+ state to switch to ACTIVE mode,
+ - 2 (RK806_RESET_NOTIFY) same as RK806_RESET and also pull
+ RESETB pin down for 5ms,
+
+ For example, some hardware may require a full restart
+ (RK806_RESTART mode) in order to function properly as regulators
+ are shortly interrupted in this mode.
+

This is fine, although now points to missing restart-handler schema and
maybe this should be once made common property. But that's just
digression, nothing needed here.

vcc1-supply:
description:
The input supply for dcdc-reg1.
diff --git a/include/dt-bindings/mfd/rockchip,rk8xx.h b/include/dt-bindings/mfd/rockchip,rk8xx.h
new file mode 100644
index 0000000000000000000000000000000000000000..f058ed1ca661185f79738a358aa2d4f04539c590
--- /dev/null
+++ b/include/dt-bindings/mfd/rockchip,rk8xx.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+/*
+ * Device Tree defines for Rockchip RK8xx PMICs
+ *
+ * Copyright 2025 Cherry Embedded Solutions GmbH
+ *
+ * Author: Quentin Schulz <quentin.schulz@xxxxxxxxx>
+ */
+
+#ifndef _DT_BINDINGS_MFD_ROCKCHIP_RK8XX_H
+#define _DT_BINDINGS_MFD_ROCKCHIP_RK8XX_H
+
+#define RK806_RESTART 0
+#define RK806_RESET 1
+#define RK806_RESET_NOTIFY 2

I do not see how this is a binding. Where do you use this in the driver
(to be a binding because otherwise you just add unused ABI)?


Explained in the commit log of the driver patch:

"""
This adds the appropriate logic in the driver to parse the new
rockchip,reset-mode DT property to pass this information. It just
happens that the values in the binding match the values to write in the
bitfield so no mapping is necessary.
"""

I can add useless mapping in the driver if it's preferred. I had the

No, I comment and raise questions when you add ABI which is neither ABI
or should not be ABI.


Not sure what would make something part of the ABI or not. I would assume the value in the DT property to be ABI anyway so this is just another name for the same value no? Trying to understand this from your perspective.

impression that simply using a hardcoded value in the DT binding and
then writing it to the register was not desired, so the constant is now
here to make this less obscure from DT perspective though I'm still
writing the value directly in the register. If hardcoded values are ok
in the binding, then I can remove that header file.

If you want something user readable, make it an enum string or keep the
header within DTS.


Just to be sure I understood correctly, moving that file to e.g. arch/arm64/boot/dts/rockchip/rk806.h (or rk8xx.h or whatever) with the file content unchanged from this v2 would be fine with you? Would I be able to point at this file from the DT binding (I assume not)?
Of course, Heiko may have a different opinion on the location of this file as I don't see header files in arch/arm64/boot/dts/rockchip yet :)

If I review it like that, it will be brought to me next time for some
other patch saying that commit was reviewed so I can do the same. [1]

Fair :)

Therefore since I object against unused binding headers in general
(there is no user here technically), I need to object here as well. :(


Laws do evolve too over time, same as how society view things. Something done decades ago could simply not be acceptable today, and vice-versa. It can be good, it can be bad :) Not here to judge, if there are new rules to contribute, there are new rules to follow :) (or discussed so they evolve to better work for maintainers, the community or project :) ).

It's easier to follow rules if they are made explicit somewhere. Is there a public documentation on those "new" rules maybe we can read ahead of time to make this easier on you? For example I really like https://www.kernel.org/doc/html/latest/devicetree/bindings/dts-coding-style.html and point to it often (internally, sometimes on the ML too). Maybe something to add to https://www.kernel.org/doc/html/latest/devicetree/bindings/writing-bindings.html or https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html?

Cheers,
Quentin