On Wed, Aug 14, 2024 at 9:53 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> 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).
>
> ...
I ended up reworking around of_regulator_bulk_get_all(). See below.
> > > > +#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.
Ack.
> ...
>
> > > > + 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;
> > > > + }
After reworking around |struct gpio_descs|, i.e. gpio_*_array() APIs,
this ended up being the only bit that actually used the _array variant
with gpiod_put_array(). The other bits remained for-loop-based.
> ...
>
> > > > + 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.
There's of_regulator_bulk_get_all(), which has had no users since it was
merged. It gets all supplies under one device node and gives a handle for
the bulk regulator API. After reworking it to do lookups directly from
DT, all the regulator stuff can be converted to the bulk regulator API.
There's no deduplication anymore, though it's not a huge problem given
that regulators are reference counted.
I'll do one last pass over the code and send out a new version tomorrow.
Thanks
ChenYu
