Message-ID: <d262b45a-c0ed-4eff-86c6-e8bcfc005ddb@cherry.de>
Date: Tue, 17 Jun 2025 12:45:00 +0200
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Krzysztof Kozlowski <krzk@...nel.org>,
 Quentin Schulz <foss+kernel@...il.net>
Cc: Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Sebastian Reichel <sebastian.reichel@...labora.com>,
 Lukasz Czechowski <lukasz.czechowski@...umatec.com>,
 Daniel Semkowicz <dse@...umatec.com>,
 Nicolas Frattaroli <nicolas.frattaroli@...labora.com>,
 devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/4] dt-bindings: mfd: rk806: allow to customize PMIC
 reset mode

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

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

Powered by blists - more mailing lists