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

[Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>]

On Wed, Nov 09, 2022 at 01:20:46PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 08, 2022 at 04:26:51PM -0800, 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)
> > - a new PROPERTY_ENTRY_GPIO() macro is supplied to ensure the above
> > - name of the software node representing gpiochip must match label of
> >   the gpiochip, as we use it to locate gpiochip structure at runtime
> > 
> > The following illustrates use of software nodes to describe a "System"
> > button that is currently specified via use of gpio_keys_platform_data
> > in arch/mips/alchemy/board-mtx1.c. It follows bindings specified in
> > Documentation/devicetree/bindings/input/gpio-keys.yaml.
> > 
> > static const struct software_node mxt1_gpiochip2_node = {
> > 	.name = "alchemy-gpio2",
> > };
> > 
> > static const struct property_entry mtx1_gpio_button_props[] = {
> > 	PROPERTY_ENTRY_U32("linux,code", BTN_0),
> > 	PROPERTY_ENTRY_STRING("label", "System button"),
> > 	PROPERTY_ENTRY_GPIO("gpios", &mxt1_gpiochip2_node, 7, GPIO_ACTIVE_LOW),
> > 	{ }
> > };
> > 
> > Similarly, arch/arm/mach-tegra/board-paz00.c can be converted to:
> > 
> > static const struct software_node tegra_gpiochip_node = {
> > 	.name = "tegra-gpio",
> > };
> > 
> > static struct property_entry wifi_rfkill_prop[] __initdata = {
> > 	PROPERTY_ENTRY_STRING("name", "wifi_rfkill"),
> > 	PROPERTY_ENTRY_STRING("type", "wlan"),
> > 	PROPERTY_ENTRY_GPIO("reset-gpios",
> > 			    &tegra_gpiochip_node, 25, GPIO_ACTIVE_HIGH);
> > 	PROPERTY_ENTRY_GPIO("shutdown-gpios",
> > 			    &tegra_gpiochip_node, 85, GPIO_ACTIVE_HIGH);
> > 	{ },
> > };
> > 
> > static struct platform_device wifi_rfkill_device = {
> > 	.name	= "rfkill_gpio",
> > 	.id	= -1,
> > };
> > 
> > ...
> > 
> > 	software_node_register(&tegra_gpiochip_node);
> > 	device_create_managed_software_node(&wifi_rfkill_device.dev,
> > 					    wifi_rfkill_prop, NULL);
> 
> ...
> 
> > +static struct gpio_chip *swnode_get_chip(struct fwnode_handle *fwnode)
> > +{
> > +	const struct software_node *chip_node;
> > +	struct gpio_chip *chip;
> > +
> > +	chip_node = to_software_node(fwnode);
> > +	if (!chip_node || !chip_node->name)
> > +		return ERR_PTR(-EINVAL);
> 
> > +	chip = gpiochip_find((void *)chip_node->name,
> > +			     swnode_gpiochip_match_name);
> 
> One line?

OK.

> 
> > +	if (!chip)
> > +		return ERR_PTR(-EPROBE_DEFER);
> > +
> > +	return chip;
> 
> As below you can use Elvis here as well, up to you.
> 
> 	return chip ?: ERR_PTR(...);

OK.

> 
> > +}
> 
> ...
> 
> > +	desc = gpiochip_get_desc(chip, args.args[0]);
> > +	*flags = args.args[1]; /* We expect native GPIO flags */
> > +
> > +	pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' - status (%d)\n",
> > +		 __func__, propname, fwnode, idx, PTR_ERR_OR_ZERO(desc));
> 
> %pe ?

"/* %pe with a non-ERR_PTR gets treated as plain %p */".

I do not think users are interested in the address on success.


> 
> > +	return desc;
> 
> ...
> 
> > +	while (fwnode_property_get_reference_args(fwnode, propname, NULL,
> > +						  0, count, &args) == 0) {
> 
> I would move 0 to the previous line.

OK.

> 
> > +		fwnode_handle_put(args.fwnode);
> > +		count++;
> > +	}
> 
> ...
> 
> >  int gpiod_count(struct device *dev, const char *con_id)
> >  {
> > -	const struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
> > -	int count = -ENOENT;
> > +	struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
> 
> Why dropping const?

Restored.

> 
> > +	int count;
> 
> Why this change is needed?

Restored.

> 
> >  	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;
> 
> ...
> 
> > +#include <dt-bindings/gpio/gpio.h>
> 
> Not sure why we have this here.

For convenience - so that users have access to GPIO_ACTIVE_HIGH/
GPIO_ACTIVE_LOW and other flags.

> 
> > +#include <linux/property.h>
> > +
> > +#define PROPERTY_ENTRY_GPIO(_name_, _chip_node_, _idx_, _flags_) \
> > +	PROPERTY_ENTRY_REF(_name_, _chip_node_, _idx_, _flags_)
> 

Thanks.

-- 
Dmitry