Re: [PATCH 6/6] gpiolib: add support for software nodes

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Fri, Nov 04, 2022 at 12:33:06PM -0700, Dmitry Torokhov wrote:
> On Fri, Nov 04, 2022 at 08:08:03PM +0200, Andy Shevchenko wrote:
> > On Thu, Nov 03, 2022 at 11:10:16PM -0700, Dmitry Torokhov wrote:

...

> > > const struct property_entry simone_key_enter_props[] __initconst = {
> > > 	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
> > 
> > > 	PROPERTY_ENTRY_STRING("label", "enter"),
> > > 	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
> > 
> > Okay, can we have an example for something like reset-gpios? Because from
> > the above I can't easily get what label is and how in the `gpioinfo` tool
> > the requested line will look like.
> 
> The label is something unrelated to gpio. The example was supposed to
> match gpio-keys binding found in
> Documentation/devicetree/bindings/input/gpio-keys.yaml

Yes, but what would be output of `gpioinfo` for the above  example and
if GPIO is named properly (with con_id)?

> > > 	{ }
> > > };

...

> > > +	/*
> > > +	 * We expect all swnode-described GPIOs have GPIO number and
> > > +	 * polarity arguments, hence nargs is set to 2.
> > > +	 */
> > 
> > Maybe instead you can provide a custom macro wrapper that will check the number
> > of arguments at compile time?
> 
> We could have PROPERTY_ENTRY_GPIO() built on top of PROPERTY_ENTRY_REF()
> that enforces needed arguments.

Yes, that's what I meant.

...

> > > +		pr_debug("%s: can't parse '%s' property of node '%pfwP[%d]'\n",
> > > +			__func__, prop_name, fwnode, idx);
> > 
> > __func__ is not needed. Dynamic Debug can automatically add it.
> > Since you have an fwnode, use that as a marker.
> 
> I was mimicking gpiolib-of.c::of_get_named_gpiod_flags(). I guess we can
> guess the function from other log messages we emit, but does it hurt
> having it?

I think it's redundant. You can modify message itself to improve its
uniqueness.

...

> > > +	/*
> > > +	 * This is not very efficient, but GPIO lists usually have only
> > > +	 * 1 or 2 entries.
> > > +	 */
> > > +	count = 0;
> > > +	while (fwnode_property_get_reference_args(fwnode, prop_name, NULL,
> > > +						  0, count, &args) == 0)
> > 
> > I would put it into for loop (and looking into property.h I think propname
> > is fine variable name):
> > 
> > 	for (count = 0; ; count++) {
> > 		if (fwnode_property_get_reference_args(fwnode, propname, NULL, 0, count, &args))
> > 			break;
> > 	}
> 
> OK on name, but I like explicit counting with the "while" loop as it
> shows the purpose of the code.

OK, let's see how it will look like with the proper dropped reference.

> > Btw, what about reference counting? Do we need to care about it?
> 
> Yes, indeed, we need to drop the reference, thank you for noticing!

...

> > > +	/*
> > > +	 * First look up GPIO in the secondary software node in case
> > > +	 * it was used to store updated properties.
> > 
> > Why this is done first? We don't try secondary before we have checked primary.
> 
> I believe we should check secondary first, so that secondaries can be
> used not only to add missing properties, but also to override existing
> ones in case they are incorrect.

It contradicts all code we have in the kernel regarding the use of software
nodes, you need very strong argument to justify that.

Personally I think this must be fixed.

> > > +	 */

-- 
With Best Regards,
Andy Shevchenko