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