Date:   Fri, 4 Nov 2022 12:33:06 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        linux-acpi@...r.kernel.org, linux-gpio@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/6] gpiolib: add support for software nodes

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:
> > Now that static device properties understand notion of child nodes and
> > references, let's teach gpiolib to handle them:
> > 
> > - GPIOs are represented as a references to software nodes representing
> >   gpiochip
> > - references must have 2 arguments - GPIO number within the chip and
> >   GPIO flags (GPIO_ACTIVE_LOW/GPIO_ACTIVE_HIGH, etc).
> > - name of the software node representing gpiochip must match label of
> >   the gpiochip, as we use it to locate gpiochip structure at runtime.
> > 
> > const struct software_node gpio_bank_b_node = {
> > 	.name = "B",
> > };
> > 
> > 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

> 
> > 	{ }
> > };
> 
> ...
> 
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/gpio/consumer.h>
> 
> It seems you are using much more that these ones.

Yeah, you are right.

> 
> ...
> 
> > +	char prop_name[32]; /* 32 is max size of property name */
> 
> Why is it not defined then?

Not sure. 32 is the limit used throughout gpiolib (see the main
gpiolib.c, gpiolib-acpi.c and gpiolib-of.c). We could add a private
gpiolib define. Or we could dynamically allocate strings if we belive
this is an issue.

I'd like to do it as a followup if we decide this needs changing.


> 
> ...
> 
> > +	/*
> > +	 * Note we do not need to try both -gpios and -gpio suffixes,
> > +	 * as, unlike OF and ACPI, we can fix software nodes to conform
> > +	 * to the proper binding.
> > +	 */
> 
> True, but for the sake of consistency between providers perhaps it makes sense
> to check that as well. Dunno, up to Linus and Bart to decide.

I hear you, however we had to have this fallback for OF and ACPI because
of concerns of separate DT/firmware and keeping compatibility. Here we
do not have this problem, so I think we should require properly named
properties.

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


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

> 
> ...
> 
> > +	chip = gpiochip_find((void *)chip_node->name,
> > +			     swnode_gpiochip_match_name);
> 
> One line?
> 
> ...
> 
> > +	pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' - status (%d)\n",
> > +		 __func__, prop_name, fwnode, idx, PTR_ERR_OR_ZERO(desc));
> 
> Same as above.
> 
> ...
> 
> > +	char prop_name[32];
> 
> > +	if (con_id)
> > +		snprintf(prop_name, sizeof(prop_name), "%s-gpios", con_id);
> > +	else
> > +		strscpy(prop_name, "gpios", sizeof(prop_name));
> 
> I saw this code, please deduplicate.

OK.

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

> 
> Btw, what about reference counting? Do we need to care about it?

Yes, indeed, we need to drop the reference, thank you for noticing!
> 
> > +	return count ? count : -ENOENT;
> 
> Elvis would work as well.
> 
> 	return count ?: -ENOENT;

OK, I like Elvis.

> 
> 
> ...
> 
> > +struct fwnode_handle;
> 
> struct gpio_desc;
> 
> > +
> > +struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
> > +				   const char *con_id, unsigned int idx,
> > +				   unsigned long *flags);
> > +int swnode_gpio_count(struct fwnode_handle *fwnode, const char *con_id);
> 
> ...
> 
> > +	/*
> > +	 * 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.

> 
> > +	 */
> 
> > +	if (is_software_node(fwnode->secondary)) {
> 
> With the previous comments it would become
> 
> 	if (fwnode && is_...)

Right, I prefer if we could trust that fwnode passed here is not NULL.

> 
> > +		dev_dbg(consumer,
> > +			"using secondary software node for GPIO lookup\n");
> > +		desc = swnode_find_gpio(fwnode->secondary,
> > +					con_id, idx, lookupflags);
> > +		if (!gpiod_not_found(desc))
> > +			return desc;
> > +	}
> 
> ...
> 
> >  int gpiod_count(struct device *dev, const char *con_id)
> >  {
> > +	struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
> > +	int count;
> > +
> > +	/*
> > +	 * First look up GPIO in the secondary software node in case
> > +	 * it was used to store updated properties.
> > +	 */
> 
> Same question as above.
> 
> > +	if (!IS_ERR_OR_NULL(fwnode) && is_software_node(fwnode->secondary)) {
> > +		count = swnode_gpio_count(fwnode->secondary, con_id);
> > +		if (count > 0)
> > +			return count;
> > +	}
> >  
> >  	if (is_of_node(fwnode))
> >  		count = of_gpio_get_count(dev, con_id);
> >  	else if (is_acpi_node(fwnode))
> >  		count = acpi_gpio_count(dev, con_id);
> > +	else if (is_software_node(fwnode))
> > +		count = swnode_gpio_count(fwnode, con_id);
> > +	else
> > +		count = -ENOENT;
> >  
> >  	if (count < 0)
> >  		count = platform_gpio_count(dev, con_id);
> 

Thanks for the review!

-- 
Dmitry

Powered by blists - more mailing lists