Re: [PATCH v2 01/10] dt-bindings: mfd: common ROHM PMIC properties

On 07/08/2026 23:23, Linus Walleij wrote:
> Hi Matti,
> 
> thanks for your patch (and the other comments in reply to the
> old review).

Nope. Thank You for taking the time to do the review - and read my replies!

> 
> On Tue, Aug 4, 2026 at 12:20 PM Matti Vaittinen
> <[email protected]> wrote:
> 
>> Sometimes the existing properties aren't 100% what reviewers would
>> prefer. When issues are minor or just cosmetic, changing the existing
>> properties is not feasible. Reviewers can't be expected to know which
>> properties are new, and which are existing - and this can lead to
>> unnecessary review discussion wasting time and energy from everyone.
>>
>> Adding a common file for re-used ROHM PMIC properties should make it
>> clearly visible that a property is re-used, when a new PMIC binding
>> refers to this file instead of (re-)describing all the existing
>> bindings. This will also help keeping the common properties identical
>> across the variants.
>>
>> Add new file listing commonly used ROHM PMIC properties.
> 
> I see the upside of this, the DT binding maintainers will decide
> on it. It has the upside you describe, but it has the downside that
> new components will always (ab-)use the old properties maybe
> even when there would be a good opportunity to modernize
> the syntax, and then the reviewers will not catch it.

I see your point :) I am not intending to "completely hide" these 
properties from the reviewers. Please, see below.

> But well, maybe that is not a big deal in the universe.
> 
> What about patching those old bindings:
> Documentation/devicetree/bindings/mfd/rohm,bd71815-pmic.yaml:
> rohm,clkout-open-drain:
> Documentation/devicetree/bindings/mfd/rohm,bd71828-pmic.yaml:
> rohm,clkout-open-drain:
> Documentation/devicetree/bindings/mfd/rohm,bd72720-pmic.yaml:
> rohm,clkout-open-drain:
> ...to include this file as part of this patch, and delete the old defines from
> those three files?

I am patching them. I, however, am not planning to delete the properties 
completely (the comment above about not hiding properties from 
reviewers). For example, the bd71815 and bd71828, do not support (at 
least all) of the OTP configured pins which are also declared here. 
Thus, I don't want to allow those unsupported pin properties for them.

My plan is to keep the "additionalProperties: false", and require the 
supported properties to be explicitly listed in the 
rohm,bd71828-pmic.yaml and rohm,bd72720-pmic.yaml. And, when the types, 
descriptions, and allowed values (when common for all PMICs) are in the 
referenced file, the rohm,bdXXXX-pmic.yaml can become quite simple:

rohm,clkout-open-drain: true;

Enough to remind reviewers that there is legacy - and allow reviewers as 
well as authors to re-evaluate the properties - while also pointing out 
that this is existing stuff. (And allow validator scripts to catch use 
of unsupported common properties - at least if this works as I think it 
does).

> Then it is clear what is going on.
> 
> Maybe this happens in later patches, but anyway it should be part
> of this patch I think.

I somehow thought it'd be clearer to split the changes in own patches - 
but I am fully Ok with squashing these if it is preferred way.

>> +properties:
>> +  rohm,clkout-open-drain:
>> +    description: clk32kout mode. Set to 1 for "open-drain" or 0 for "cmos"
>> +      a.k.a "push-pull".
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    maximum: 1
> 
> Yeah I see now that this is already in use in three other bindings...
> 
> Today we would for a new component use something like
> rohm,clkout-drive-mode = "drive-push-pull";
> rohm,clkout-drive-mode = "drive-open-drain";
> reflecting pin control.
> 
> I do understand the idea to keep using the same bindings
> for all of Rohm MFDs. But when a completely new design
> arrives with many new properties etc, maybe we can think
> of something new?

In case of the "rohm,clkout-open-drain" - AFAIR it is not used from new 
drivers, but the new PMICs which are added, are supported by the same 
MFD driver. I am not too keen on changing the property values in this 
driver (because it'd require every user to update their dtses - which 
feels like a source of problems). Nor am I too excited about supporting 
old and new values in this driver as it makes the driver (in my opinion 
unnecessarily) more complex.

What comes to using something else for completely new designs - I'd say 
(a bit cautiously) that changing the properties there is fine. 
Cautiously because it is still dancing between "what users have used to 
see" Vs. "what feels like best course of action - Today" ;) Cautiously 
also because we don't probably want to support multiple vendor specific 
properties for same purpose - at least not in a long run.

Yours,
	-- Matti

Ps, planning to join ELCE/Plumbers this year? It'd be about a time to 
meet you in person :)

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~