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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 17/06/2025 12:45, Quentin Schulz wrote:
> 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@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.
>>
> 
> 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.

Drop the header, it's not an ABI. You just use register values. This is
not a Linux ABI. The values are coming from the hardware.

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

Yes

> 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)?

No, because it is not a binding.

Best regards,
Krzysztof