Message-ID: <CAGXv+5E+D8oXr7nh67HEPermJYoRp+Xf+oqSefOiUoCpyoKYUQ@mail.gmail.com>
Date: Wed, 14 Aug 2024 19:34:00 +0800
From: Chen-Yu Tsai <wenst@...omium.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>, 
	Matthias Brugger <matthias.bgg@...il.com>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, Wolfram Sang <wsa@...nel.org>, 
	Benson Leung <bleung@...omium.org>, Tzung-Bi Shih <tzungbi@...nel.org>, 
	Mark Brown <broonie@...nel.org>, Liam Girdwood <lgirdwood@...il.com>, 
	chrome-platform@...ts.linux.dev, devicetree@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, Douglas Anderson <dianders@...omium.org>, 
	Johan Hovold <johan@...nel.org>, Jiri Kosina <jikos@...nel.org>, linux-i2c@...r.kernel.org
Subject: Re: [PATCH v4 4/6] i2c: of-prober: Add GPIO and regulator support

On Tue, Aug 13, 2024 at 7:41 PM Andy Shevchenko
<andriy.shevchenko@...ux.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.

> > Components that the prober intends to probe likely require their
> > regulator supplies be enabled, and GPIOs be toggled to enable them or
> > bring them out of reset before they will respond to probe attempts.
> >
> > Without specific knowledge of each component's resource names or
> > power sequencing requirements, the prober can only enable the
> > regulator supplies all at once, and toggle the GPIOs all at once.
> > Luckily, reset pins tend to be active low, while enable pins tend to
> > be active high, so setting the raw status of all GPIO pins to high
> > should work. The wait time before and after resources are enabled
> > are collected from existing drivers and device trees.
> >
> > The prober collects resources from all possible components and enables
> > them together, instead of enabling resources and probing each component
> > one by one. The latter approach does not provide any boot time benefits
> > over simply enabling each component and letting each driver probe
> > sequentially.
> >
> > The prober will also deduplicate the resources, since on a component
> > swap out or co-layout design, the resources are always the same.
> > While duplicate regulator supplies won't cause much issue, shared
> > GPIOs don't work reliably, especially with other drivers. For the
> > same reason, the prober will release the GPIOs before the successfully
> > probed component is actually enabled.
>
> ...
>
> > +/*
> > + * While 8 seems like a small number, especially when probing many component
> > + * options, in practice all the options will have the same resources. The
> > + * code getting the resources below does deduplication to avoid conflicts.
> > + */
> > +#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?

> ...
>
> > +#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.

> ...
>
> > +     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)

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

> > +
> > +     strscpy(con, prop->name, p - prop->name + 1);
> > +     regulator = regulator_of_get_optional(node, con);
> > +     /* DT lookup should never return -ENODEV */
> > +     if (IS_ERR(regulator))
> > +             return PTR_ERR(regulator);
>
> ...
>
> > +     for (int i = 0; i < data->regulators_num; i++)
>
> Why signed?

No real reason, other than being the same as the for loops in the
free functions.

> > +             if (regulator_is_equal(regulator, data->regulators[i])) {
> > +                     regulator_put(regulator);
> > +                     regulator = NULL;
> > +                     break;
> > +             }
>
> ...
>
> > +#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.

> ...
>
> > +     p = strstr(prop->name, GPIO_SUFFIX);
> > +     if (p) {
> > +             strscpy(con, prop->name, p - prop->name + 1);
> > +             con_id = con;
> > +     } else if (strcmp(prop->name, "gpio") && strcmp(prop->name, "gpios")) {
> > +             return 0;
>
> We have an array of these suffixes, please use it. If required make it exported
> to the others.

OK. I'll rewrite this section with that then.

> > +     }
>
> ...
>
> > +     ret = of_parse_phandle_with_args_map(node, prop->name, "gpio", 0, &phargs);
> > +     if (ret)
> > +             return ret;
>
> > +     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.

> > +     if (data->gpiods_num == ARRAY_SIZE(data->gpiods)) {
> > +             of_node_put(phargs.np);
> > +             gpiod_put(gpiod);
> > +             return -ENOMEM;
> > +     }
>
> ...
>
> > +     for (int i = data->gpiods_num - 1; i >= 0; i--)
> > +             gpiod_put(data->gpiods[i]);
>
> This sounds like reinvention of gpiod_*_array() call.

Indeed. Will switch to gpiod_put_array() with some slight tweaks.

> ...
>
> > +     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.

> ...
>
> > +     for_each_child_of_node_scoped(i2c_node, node) {
>
> Eventually _scoped(), but...
>
> > +             u32 addr;
> > +
> > +             if (!of_node_name_prefix(node, type))
> > +                     continue;
> > +             if (of_property_read_u32(node, "reg", &addr))
> > +                     continue;
> > +
> > +             dev_dbg(dev, "Requesting resources for %pOF\n", node);
> > +             ret = i2c_of_probe_get_res(dev, node, &data);
> > +             if (ret) {
>
> > +                     of_node_put(i2c_node);
>
> ...huh?!

Oops. This and the next "of_node_put(i2c_node)" should be
"i2c_put_adapter(i2c)" instead.


Thank you for the review. More stuff to try.


Regards
ChenYu

> > +                     return ret;
> > +             }
> > +     }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Powered by blists - more mailing lists