Re: [PATCH 7/8] gpio: bd73800: Support ROHM BD73800 PMIC GPIOs

[Matti Vaittinen <mazziesaccount@gmail.com>]

pe 3.7.2026 klo 16.11 Bartosz Golaszewski (brgl@kernel.org) kirjoitti:
>
> On Wed, 1 Jul 2026 14:43:25 +0200, Matti Vaittinen
> <matti.vaittinen@linux.dev> said:
> > From: Matti Vaittinen <mazziesaccount@gmail.com>
> >
> > The ROHM BD73800 PMIC has 4 pins (named GPIO1, CLKOUT, FAULT_B and
> > EXTEN_OUT) which might have been set to operate as a GPI or GPO when OTP
> > (One Time Programmable memory) is written at device manufacturing.
> > Support the GPI/GPO use-case via GPIO framework.
> >
> > The default OTP for these pins is to not use any of them as GPI or GPO.
> > (The GPIO1 defaults as an ADC input regardless the naming). Hence the
> > driver assumes none of these pins is a GPI/GPO unless explicitly pointed
> > as GPI or GPO via device tree.
> >
> > Furthermore, pin's direction can't be changed after OTP configuration is
> > done. Also the default drive type for a GPO (CMOS / Open Drain) is set
> > by the OTP configuration. The BD73800 has a set of undocumented test
> > registers which should allow changing the drive type. Access to the test
> > register area or the test registers aren't documented and so this driver
> > does not support configuring the drive type even though it might be
> > doable.
> >
> > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> > ---

Thanks for the review! I appreciate it!

> > +static int bd73800_gpio_get_pins(struct bd73800_gpio *g)
> > +{
> > +     static const char * const properties[] = {"rohm,pin-gpio1",
> > +             "rohm,pin-clkout", "rohm,pin-fault-b", "rohm,pin-exten"};
> > +     const char *val;
> > +     int i, ret;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(properties); i++) {
> > +             ret = fwnode_property_read_string(dev_fwnode(g->dev->parent),
>
> It would be cleaner with device_property_read_string(g->dev->parent, ...)

True.


> > +
> > +static int gpo_bd73800_probe(struct platform_device *pdev)
> > +{
> > +     struct bd73800_gpio *g;
>
> May I suggest a slightly more descriptive name for driver data? Maybe at the
> very least... "data"? :)

Ok.

> > +     struct device *parent, *dev;
> > +     int ret;
> > +
> > +     /*
> > +      * Bind devm lifetime to this platform device => use dev for devm.
> > +      * also the prints should originate from this device.
> > +      */
>
> Why would you need to comment on it though? It's very much the standard
> approach, isn't it?

Sure. I might have had this comment combined with the one below. Idea
has probably been to explain why we use both this device and the
parent. Can't bind lifetime to parent while needing it for regmap and
dt. Well, I agree that this device's comment is really not required.
I'll keep the comment about parent while removing this. Thanks.

> > +     dev = &pdev->dev;
> > +     /* The device-tree and regmap come from MFD => use parent for that */
> > +     parent = dev->parent;
> > +
> > +     g = devm_kzalloc(dev, sizeof(*g), GFP_KERNEL);
> > +     if (!g)
> > +             return -ENOMEM;
> > +
> > +     g->chip = bd73800gpio_chip;
> > +     g->chip.base = -1;
> > +     g->chip.parent = parent;
> > +     g->regmap = dev_get_regmap(parent, NULL);
> > +     g->dev = dev;
> > +
> > +     ret = bd73800_gpio_get_pins(g);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (!g->num_pins) {
> > +             /*
> > +              * The BD73800 may or may not have pins allocated for GPIO
> > +              * depending on the OTP used at manufacturing. Free the memory
> > +              * and go out if there is no pins as then we have nothing to do
> > +              */
> > +             dev_dbg(dev, "no GPIO pins\n");
> > +             devm_kfree(dev, g);
>
> No need for that, because...
>
> > +             return 0;
>
> ... this will still leave a bound device in the system, I'm not sure we want
> it? The correct error code in this case is -ENODEV.

I am a bit uncertain. This isn't really an error. It should be very
much Ok to configure the support for PMICs with GPIOs to be compiled -
and also very much Ok to have a PMIC with no GPIOs. Returning an error
doesn't feel right. Furthermore, I am afraid that returning an error
might hurt the system start-up? Do you see some really problematic
consequences if we return Ok without registering the GPIOs. I'd assume
that wastes some bytes but I don't see other really severe
consequences.

As noted when sending - I am mostly offline during July so my replies
may arrive a bit late and next version is unlikely to be sent before
August.

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