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

[Krzysztof Kozlowski]

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 at cherry.de>
>>> + */
>>> +
>>> +#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.

> 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.

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]
Therefore since I object against unused binding headers in general
(there is no user here technically), I need to object here as well. :(


https://lore.kernel.org/all/0d381ad0-85d4-43de-a050-3b9ed03bf5d8@kernel.org/

Best regards,
Krzysztof