On Wed, Aug 14, 2024 at 9:53 PM Andy Shevchenko
[off-list ref] wrote:

On Wed, Aug 14, 2024 at 07:34:00PM +0800, Chen-Yu Tsai wrote:

quoted

On Tue, Aug 13, 2024 at 7:41 PM Andy Shevchenko
[off-list ref] wrote:

quoted

On Thu, Aug 08, 2024 at 05:59:27PM +0800, Chen-Yu Tsai wrote:

...

quoted

quoted

quoted

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

quoted

quoted

Can this be two patches?

You mean one for GPIOs and one for regulators right? Sure.

Yes.

...

quoted

quoted

quoted

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

...

quoted

quoted

quoted

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

...

quoted

quoted

quoted

+     p = strstr(prop->name, REGULATOR_SUFFIX);

strstr()?! Are you sure it will have no side effects on some interesting names?

quoted

+     if (!p)
+             return 0;

quoted

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

quoted

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.

quoted

quoted

quoted

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

...

quoted

quoted

quoted

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

(1)

quoted

quoted

quoted

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

quoted

quoted

quoted

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

...

quoted

quoted

quoted

+     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