Re: [PATCH 1/6] gpio: make the gpiochip a real device

[Johan Hovold <johan@kernel.org>]

On Thu, Oct 22, 2015 at 10:32:25AM +0200, Linus Walleij wrote:
> GPIO chips have been around for years, but were never real devices,
> instead they were piggy-backing on a parent device (such as a
> platform_device or amba_device) but this was always optional.
> GPIO chips could also exist without any device at all, with its
> struct device *dev pointer being set to null.
> 
> When sysfs was in use, a mock device would be created, with the
> optional parent assigned, or just floating orphaned with NULL
> as parent.
> 
> For a proper userspace ABI we need gpiochips to *always have a
> populated struct device, so add this in the gpio_chip struct.
> The name "dev" is unfortunately already take so we use "device"
> to name it.
> 
> If sysfs is active, it will use this device as parent, and the
> former parent device "dev" will be set as parent of the new
> "device" struct member.
> 
> From this point on, gpiochips are devices.
> 
> Cc: Johan Hovold <johan@kernel.org>
> Cc: Michael Welling <mwelling@ieee.org>
> Cc: Markus Pargmann <mpa@pengutronix.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/gpiolib-sysfs.c |  2 +-
>  drivers/gpio/gpiolib.c       | 44 +++++++++++++++++++++++++++++++++++++++-----
>  include/linux/gpio/driver.h  |  9 +++++++--
>  3 files changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index b57ed8e55ab5..7e5bc5736e47 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -605,7 +605,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>  	if (chip->names && chip->names[offset])
>  		ioname = chip->names[offset];
>  
> -	dev = device_create_with_groups(&gpio_class, chip->dev,
> +	dev = device_create_with_groups(&gpio_class, &chip->device,
>  					MKDEV(0, 0), data, gpio_groups,
>  					ioname ? ioname : "gpio%u",
>  					desc_to_gpio(desc));
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 6798355c61c6..0ab4f75b7f8e 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -16,6 +16,7 @@
>  #include <linux/gpio/driver.h>
>  #include <linux/gpio/machine.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/idr.h>
>  
>  #include "gpiolib.h"
>  
> @@ -42,6 +43,9 @@
>  #define	extra_checks	0
>  #endif
>  
> +/* Device and char device-related information */
> +static DEFINE_IDA(gpio_ida);
> +
>  /* gpio_lock prevents conflicts during gpio_desc[] table updates.
>   * While any GPIO is requested, its gpio_chip is not removable;
>   * each GPIO's "requested" flag serves as a lock and refcount.
> @@ -52,7 +56,6 @@ static DEFINE_MUTEX(gpio_lookup_lock);
>  static LIST_HEAD(gpio_lookup_list);
>  LIST_HEAD(gpio_chips);
>  
> -
>  static void gpiochip_free_hogs(struct gpio_chip *chip);
>  static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip);
>  
> @@ -300,7 +303,7 @@ int gpiochip_add(struct gpio_chip *chip)
>  {
>  	unsigned long	flags;
>  	int		status = 0;
> -	unsigned	id;
> +	unsigned	i;
>  	int		base = chip->base;
>  	struct gpio_desc *descs;
>  
> @@ -308,6 +311,28 @@ int gpiochip_add(struct gpio_chip *chip)
>  	if (!descs)
>  		return -ENOMEM;
>  
> +	/*
> +	 * The "dev" member of gpiochip is the parent, and the actual
> +	 * device is named "device" for historical reasons.
> +	 *
> +	 * We memset the struct to zero to avoid reentrance issues.
> +	 */
> +	memset(&chip->device, 0, sizeof(chip->device));

This is an indication of a larger problem.

First of all, you must never register the same device structure twice.

And the larger problem is: With the current interface where a struct
gpio_chip is passed and registered, how would you prevent the device
from going away while in use?

You grab a reference to the chip->device when opening the node (in a
later patch), but it is not used to manage the life time of struct
gpio_chip.

> +	if (chip->dev) {
> +		chip->device.parent = chip->dev;
> +		chip->device.of_node = chip->dev->of_node;
> +	} else {
> +#ifdef CONFIG_OF_GPIO
> +	/* If the gpiochip has an assigned OF node this takes precedence */
> +	if (chip->of_node)
> +		chip->device.of_node = chip->of_node;
> +#endif
> +	}
> +	chip->id = ida_simple_get(&gpio_ida, 0, 0, GFP_KERNEL);

Missing error handling.

> +	dev_set_name(&chip->device, "gpiochip%d", chip->id);
> +	device_initialize(&chip->device);
> +	dev_set_drvdata(&chip->device, chip);
> +
>  	spin_lock_irqsave(&gpio_lock, flags);
>  
>  	if (base < 0) {

Johan