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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

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.

> 	{ }
> };

...

> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/gpio/consumer.h>

It seems you are using much more that these ones.

...

> +	char prop_name[32]; /* 32 is max size of property name */

Why is it not defined then?

...

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

...

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

...

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

...

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

...

> +	/*
> +	 * 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;
	}

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

> +	return count ? count : -ENOENT;

Elvis would work as well.

	return count ?: -ENOENT;


...

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

> +	 */

> +	if (is_software_node(fwnode->secondary)) {

With the previous comments it would become

	if (fwnode && is_...)

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

-- 
With Best Regards,
Andy Shevchenko