Re: [rft, PATCH v3 1/1] gpiolib: Get rid of not used of_node member

[Thierry Reding]

On Wed, Nov 16, 2022 at 11:18:59AM +0200, Andy Shevchenko wrote:
> All new drivers should use fwnode and / or parent to provide the
> necessary information to the GPIO library.
> 
> Cc: Thierry Reding <treding@xxxxxxxxxx>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
> 
> v3: rebased against latest Linux Next: expected not to fail now
>     (Also keeping in mind Thierry's report, so reworked a bit)
> v2: resent against latest Linux Next: expected not to fail now
>     (Linux Next has no more users of of_node member of gpio_chip)
> v1: to test for now (using CIs and build bots) what is left unconverted
>     (Expected to fail in some configurations!)
> 
> The idea is to send this after v6.2-rc1, so we will have a full cycle
> to test. Currently one patch is in pin control tree prevents us doing
> this during v6.1 cycle.
> 
>  drivers/gpio/gpiolib-acpi.c | 10 ----------
>  drivers/gpio/gpiolib-acpi.h |  4 ----
>  drivers/gpio/gpiolib-of.c   | 24 ++++--------------------
>  drivers/gpio/gpiolib-of.h   |  5 -----
>  drivers/gpio/gpiolib.c      | 11 +++--------
>  include/linux/gpio/driver.h |  7 -------
>  6 files changed, 7 insertions(+), 54 deletions(-)

Seems to work fine on Tegra210 (gpio-tegra) and Tegra194
(gpio-tegra186). One thing I noticed, though, see below.

> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index bed0380c5136..466622a8fb36 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -1387,16 +1387,6 @@ void acpi_gpiochip_remove(struct gpio_chip *chip)
>  	kfree(acpi_gpio);
>  }
>  
> -void acpi_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev)
> -{
> -	/* Set default fwnode to parent's one if present */
> -	if (gc->parent)
> -		ACPI_COMPANION_SET(&gdev->dev, ACPI_COMPANION(gc->parent));
> -
> -	if (gc->fwnode)
> -		device_set_node(&gdev->dev, gc->fwnode);
> -}
> -
>  static int acpi_gpio_package_count(const union acpi_object *obj)
>  {
>  	const union acpi_object *element = obj->package.elements;
> diff --git a/drivers/gpio/gpiolib-acpi.h b/drivers/gpio/gpiolib-acpi.h
> index 9475f99a9694..5fa315b3c912 100644
> --- a/drivers/gpio/gpiolib-acpi.h
> +++ b/drivers/gpio/gpiolib-acpi.h
> @@ -26,8 +26,6 @@ struct gpio_device;
>  void acpi_gpiochip_add(struct gpio_chip *chip);
>  void acpi_gpiochip_remove(struct gpio_chip *chip);
>  
> -void acpi_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev);
> -
>  void acpi_gpiochip_request_interrupts(struct gpio_chip *chip);
>  void acpi_gpiochip_free_interrupts(struct gpio_chip *chip);
>  
> @@ -42,8 +40,6 @@ int acpi_gpio_count(struct device *dev, const char *con_id);
>  static inline void acpi_gpiochip_add(struct gpio_chip *chip) { }
>  static inline void acpi_gpiochip_remove(struct gpio_chip *chip) { }
>  
> -static inline void acpi_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev) { }
> -
>  static inline void
>  acpi_gpiochip_request_interrupts(struct gpio_chip *chip) { }
>  
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 4fff7258ee41..00b55dbe0323 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -668,7 +668,7 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
>  	u32 tmp;
>  	int ret;
>  
> -	chip_np = chip->of_node;
> +	chip_np = to_of_node(chip->fwnode);
>  	if (!chip_np)
>  		return ERR_PTR(-EINVAL);
>  
> @@ -760,7 +760,7 @@ static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
>  	struct device_node *np;
>  	int ret;
>  
> -	for_each_available_child_of_node(chip->of_node, np) {
> +	for_each_available_child_of_node(to_of_node(chip->fwnode), np) {
>  		if (!of_property_read_bool(np, "gpio-hog"))
>  			continue;
>  
> @@ -970,7 +970,7 @@ EXPORT_SYMBOL_GPL(of_mm_gpiochip_remove);
>  #ifdef CONFIG_PINCTRL
>  static int of_gpiochip_add_pin_range(struct gpio_chip *chip)
>  {
> -	struct device_node *np = chip->of_node;
> +	struct device_node *np = to_of_node(chip->fwnode);
>  	struct of_phandle_args pinspec;
>  	struct pinctrl_dev *pctldev;
>  	int index = 0, ret;
> @@ -1063,7 +1063,7 @@ int of_gpiochip_add(struct gpio_chip *chip)
>  	struct device_node *np;
>  	int ret;
>  
> -	np = to_of_node(dev_fwnode(&chip->gpiodev->dev));
> +	np = to_of_node(chip->fwnode);
>  	if (!np)
>  		return 0;
>  
> @@ -1092,19 +1092,3 @@ void of_gpiochip_remove(struct gpio_chip *chip)
>  {
>  	fwnode_handle_put(chip->fwnode);
>  }
> -
> -void of_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev)
> -{
> -	/* Set default OF node to parent's one if present */
> -	if (gc->parent)
> -		gdev->dev.of_node = gc->parent->of_node;
> -
> -	if (gc->fwnode)
> -		gc->of_node = to_of_node(gc->fwnode);
> -
> -	/* If the gpiochip has an assigned OF node this takes precedence */
> -	if (gc->of_node)
> -		gdev->dev.of_node = gc->of_node;
> -	else
> -		gc->of_node = gdev->dev.of_node;
> -}
> diff --git a/drivers/gpio/gpiolib-of.h b/drivers/gpio/gpiolib-of.h
> index a6c593e6766c..e5bb065d82ef 100644
> --- a/drivers/gpio/gpiolib-of.h
> +++ b/drivers/gpio/gpiolib-of.h
> @@ -23,7 +23,6 @@ struct gpio_desc *of_find_gpio(struct device_node *np,
>  int of_gpiochip_add(struct gpio_chip *gc);
>  void of_gpiochip_remove(struct gpio_chip *gc);
>  int of_gpio_get_count(struct device *dev, const char *con_id);
> -void of_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev);
>  #else
>  static inline struct gpio_desc *of_find_gpio(struct device_node *np,
>  					     const char *con_id,
> @@ -38,10 +37,6 @@ static inline int of_gpio_get_count(struct device *dev, const char *con_id)
>  {
>  	return 0;
>  }
> -static inline void of_gpio_dev_init(struct gpio_chip *gc,
> -				    struct gpio_device *gdev)
> -{
> -}
>  #endif /* CONFIG_OF_GPIO */
>  
>  extern struct notifier_block gpio_of_notifier;
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 7936d54a2e30..3df482d36366 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -655,10 +655,12 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>  	int ret = 0;
>  	u32 ngpios;
>  
> +	/* If the calling driver did not initialize firmware node, do it here */
>  	if (gc->fwnode)
>  		fwnode = gc->fwnode;
>  	else if (gc->parent)
>  		fwnode = dev_fwnode(gc->parent);
> +	gc->fwnode = fwnode;

I'm not sure we want to set this one. We recently discussed this in
another thread and my reading is that gc->fwnode is supposed to be used
only to explicitly override which fwnode to use if the default isn't
appropriate. Right now the standard way to access the device's fwnode
seems to be dev_fwnode(&gdev->dev), with only very few exceptions, so
it'd be great if we could settle on that, rather than introduce a second
field that contains the same value and use them interchangeably.

One way we could enforce this is by setting gc->fwnode to NULL here
instead of fwnode. That should cause a crash anywhere it's used after
this, so we should be able to easily weed out any abuses.

Of course if people prefer to use gc->fwnode instead, then we may want
to remove all uses of gdev->dev.fwnode. There's simply no point in
keeping the same value in two different place, it's just going to lead
to a big mess.

Thierry

Attachment: signature.asc
Description: PGP signature