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
[off-list ref] wrote:

quoted

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

...

quoted

quoted

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

quoted

Can this be two patches?

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

Yes.

...

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

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

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

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

...

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.

...

quoted

quoted

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

(1)

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

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

...

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.

-- 
With Best Regards,
Andy Shevchenko