Re: [PATCH v4 4/6] i2c: of-prober: Add GPIO and regulator support

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Aug 14, 2024 at 07:34:00PM +0800, Chen-Yu Tsai wrote:
> On Tue, Aug 13, 2024 at 7:41 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Aug 08, 2024 at 05:59:27PM +0800, Chen-Yu Tsai wrote:

...

> > > This adds GPIO and regulator management to the I2C OF component prober.

> > Can this be two patches?
> 
> You mean one for GPIOs and one for regulators right? Sure.

Yes.

...

> > > +#define RESOURCE_MAX 8
> >
> > Badly (broadly) named constant. Is it not the same that defines arguments in
> > the OF phandle lookup? Can you use that instead?
> 
> I'm not sure what you are referring to. This is how many unique instances
> of a given resource (GPIOs or regulators) the prober will track.
> 
> MAX_TRACKED_RESOURCES maybe?

Better, but still ambiguous. We have a namespace approach, something like
I2C_PROBER_... I have checked the existing constant and it's not what you
want, so forget about that part, only name of the definition is questionable.

...

> > > +#define REGULATOR_SUFFIX "-supply"
> >
> > Name is bad, also move '-' to the code, it's not part of the suffix, it's a
> > separator AFAICT.
> 
> OF_REGULATOR_SUPPLY_SUFFIX then?
> 
> Also, "supply" is not a valid property. It is always "X-supply".
> Having the '-' directly in the string makes things simpler, albeit
> making the name slightly off.

Let's use proper SUFFIX and separator separately.

#define I2C_PROBER_..._SUFFIX "supply"

(or alike)

...

> > > +     p = strstr(prop->name, REGULATOR_SUFFIX);
> >
> > strstr()?! Are you sure it will have no side effects on some interesting names?
> >
> > > +     if (!p)
> > > +             return 0;
> >
> > > +     if (strcmp(p, REGULATOR_SUFFIX))
> > > +             return 0;
> >
> > Ah, you do it this way...
> >
> > What about
> 
> About? (feels like an unfinished comment)

Yes, sorry for that. Since you found a better alternative, no need to finish
this part :-)

> Looking around, it seems I could just rename and export "is_supply_name()"
> from drivers/regulator/of_regulator.c .

Even better!

Something similar most likely can be done with GPIO (if not, we are always open
to the ideas how to deduplicate the code).

...

> > > +#define GPIO_SUFFIX "-gpio"
> >
> > Bad define name, and why not "gpios"?
> 
> "-gpio" in strstr() would match against both "foo-gpio" and "foo-gpios".
> More like laziness.

And opens can of worms with whatever ending of the property name.
Again, let's have something that GPIO library provides for everybody.

...

> > > +     ret = of_parse_phandle_with_args_map(node, prop->name, "gpio", 0, &phargs);
> > > +     if (ret)
> > > +             return ret;

(1)

> > > +     gpiod = fwnode_gpiod_get_index(fwnode, con_id, 0, GPIOD_ASIS, "i2c-of-prober");
> > > +     if (IS_ERR(gpiod)) {
> > > +             of_node_put(phargs.np);
> > > +             return PTR_ERR(gpiod);
> > > +     }
> >
> > Try not to mix fwnode and OF specifics. You may rely on fwnode for GPIO completely.
> 
> Well, fwnode doesn't give a way to identify GPIOs without requesting them.
> 
> Instead I think I could first request GPIOs exclusively, and if that fails
> try non-exclusive and see if that GPIO descriptor matches any known one.
> If not then something in the DT is broken and it should error out. Then
> the phandle parsing code could be dropped.

What I meant, the, e.g., (1) can be rewritten using fwnode API, but if you know
better way of doing things, then go for it.

> > > +     if (data->gpiods_num == ARRAY_SIZE(data->gpiods)) {
> > > +             of_node_put(phargs.np);
> > > +             gpiod_put(gpiod);
> > > +             return -ENOMEM;
> > > +     }

...

> > > +     for (int i = data->regulators_num; i >= 0; i--)
> > > +             regulator_put(data->regulators[i]);
> >
> > Bulk regulators?
> 
> Bulk regulators API uses its own data structure which is not just an
> array. So unlike gpiod_*_array() it can't be used in this case.

But it sounds like a bulk regulator case...
Whatever, it's Mark's area and he might suggest something better.

-- 
With Best Regards,
Andy Shevchenko