Re: [PATCH 2/8] dt-bindings: mfd: ROHM BD73800 PMIC

[Matti Vaittinen]

pe 3.7.2026 klo 23.46 Linus Walleij (linusw@xxxxxxxxxx) kirjoitti:
>
> Hi Matti,
>
> thanks for your patch!

Thank you for the review!

> I have some inevitable pin control nitpicks!

Great! That's what the v1 is sent for, right? ;)

>
> On Wed, Jul 1, 2026 at 2:41 PM Matti Vaittinen
> <matti.vaittinen@xxxxxxxxx> wrote:
>
> > +  # The GPIO1, CLKOUT (GPIO2), FAULT_B and EXTEN_OUT pins can be
> > +  # configured to interrupt pins by OTP.
>
> Maybe move this helpful comment into the top description: instead?
> It's kind of generic helpful info.

Sure. I believe Krzk asked for that too.

>
> > +# The GPIO1, CLKOUT, FAULT_B and EXTEN_OUT pins may be configured for a
> > +# specific purpose (like ADC input, 32.768 clk output, fault indicator or
> > +# delivering power sequence to a companion PMIC when multiple PMICs are
> > +# used) - but also to be either a GPO or GPI. (When used as a GPI the pin
> > +# can also be used as an IRQ source). The pin purpose is determined by
> > +# OTP (One Time Programmable memory), typically during device manufacturing.
> > +# The OTP can't be read at runtime so device-tree should describe the pins.
> > +  rohm,pin-gpio1:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description:
> > +      Indicate if the GPIO1 pin has been set to GPI or GPO at manufacturing.
> > +    enum: [gpi, gpo]
> > +
> > +  rohm,pin-clkout:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description:
> > +      Indicate if the CLKOUT pin has been set to GPI or GPO at manufacturing.
> > +    enum: [gpi, gpo]
> > +
> > +  rohm,pin-fault-b:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description:
> > +      Indicate if the FAULT_B pin has been set to GPI or GPO at manufacturing.
> > +    enum: [gpi, gpo]
> > +
> > +  rohm,pin-exten:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description:
> > +      Indicate if the EXTEN_OUT pin has been set to GPI or GPO at
> > +      manufacturing.
> > +    enum: [gpi, gpo]
>
> Can we explain what "GPI" and "GPO" means in this context?
>
> I read it as "general purpose input" and "general purpose output", but...
> you just describe the exact purpose? So what is "general purpose"
> about them in that case?

I see I could improve the description. These properties aren't really
configuring the pins, but telling the SW how they have been set at the
manufacturing. For example the CLKOUT pin's "usual" purpose is to be
the RTC CLK output. If this is what has been set to OTP, then the
GP(IO) registers have no relation to the pin and they shouldn't be
used. However, it is possible to configure the OTP so that the CLKOUT
pin is not outputting the RTC CLK, but is a GPO. (Eg, is an output
which HIGH/LOW state can be set by the software). In this case it
truly is a general purpose output. With another OTP config the pin can
be an input, and state can be read from a register.

So, these properties are intended to describe the hardware for the GPIO driver.

>
> I would re-use "input-enable" and "output-enable" from:
> Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> (I mean don't $rf that, just use these strings).
>
> I suppose:
> enum: [input-enable, output-enable]

These sound like control properties to me. To me this sounds like a
request to enable input/output - and this is something we can't do
with this IC. But I think you still have a valid point regarding
re-use. I will check this when I rework the series, but I believe the
BD72720 does already use these same properties. I might be able to do
a common binding for these and $ref them from this and BD72720
bindings. (But I will check this when I am back at the business).

>
> > +  rohm,clkout-open-drain:
> > +    description: clk32kout mode. Set to 1 for "open-drain" or 0 for "cmos".
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 0
> > +    maximum: 1
>
> Here I would also reuse the generic pinconf properties,
> something like;
>
> rohm,clkout-drive-type:
>   enum: [drive-push-pull, drive-open-drain]
>
> (Push-pull is what is colloquially referred to as "cmos".)
>
> > +            rohm,pin-gpio1 = "gpo";
> > +            rohm,pin-exten = "gpi";
>
> If you instead use nodes with properties you can do this:
>
> rohm,pin-clkout {
>     output-enable;
>     drive-push-pull;
> };
>
> This collects the clkout config in one place and make
> it obvious what is going on. But I don't know what the DT
> maintainers think about this idea.

I will take a better look at this when I am back. Still, the
"rohm,clkout-open-drain" is also already used by
rohm,bd71815-pmic.yaml, rohm,bd71828-pmic.yaml and
rohm,bd72720-pmic.yaml so I am a bit reluctant to changing it as it'd
require me to either ask people to change their dt-entries (asking for
a nose bleed) or support both old and new properties in the same
driver (asking for a nose bleed).  :)

Yours,
    -- Matti

-- 

Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

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

Discuss - Estimate - Plan - Report and finally accomplish this:
void do_work(int time) __attribute__ ((const));