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

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

[Andy Shevchenko]

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@nvidia.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> 
> 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(-)
> 
> 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;
>  
>  	/*
>  	 * First: allocate and populate the internal stat container, and
> @@ -672,14 +674,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>  	gdev->chip = gc;
>  	gc->gpiodev = gdev;
>  
> -	of_gpio_dev_init(gc, gdev);
> -	acpi_gpio_dev_init(gc, gdev);
> -
> -	/*
> -	 * Assign fwnode depending on the result of the previous calls,
> -	 * if none of them succeed, assign it to the parent's one.
> -	 */
> -	gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
> +	device_set_node(&gdev->dev, gc->fwnode);
>  
>  	gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
>  	if (gdev->id < 0) {
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index 2a44600b01f7..ee382712f066 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -503,13 +503,6 @@ struct gpio_chip {
>  	 * the device tree automatically may have an OF translation
>  	 */
>  
> -	/**
> -	 * @of_node:
> -	 *
> -	 * Pointer to a device tree node representing this GPIO controller.
> -	 */
> -	struct device_node *of_node;
> -
>  	/**
>  	 * @of_gpio_n_cells:
>  	 *
> 
> base-commit: 8dab99c9eab3162bfb4326c35579a3388dbf68f2

Uhpffhh, sorry for this, the base is today's Linux Next.

-- 
With Best Regards,
Andy Shevchenko

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 7440 bytes --]

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@nvidia.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> 
> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Andy Shevchenko]

On Wed, Nov 16, 2022 at 02:47:07PM +0100, Thierry Reding wrote:
> 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.

...

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

I prefer that we explicitly use GPIO device firmware node.
Independently on this message I came up with another patch
(I'm just about to sent it right away) which I think it
the best to have in current case.

Ideally I would like to see const struct gpio_chip *gc to be a parameter
to the GPIO chip add(). But it may happen in distant future.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Nov 16, 2022 at 04:09:44PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 16, 2022 at 02:47:07PM +0100, Thierry Reding wrote:
> > On Wed, Nov 16, 2022 at 11:18:59AM +0200, Andy Shevchenko wrote:

...

> > > +	/* 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.
> 
> I prefer that we explicitly use GPIO device firmware node.
> Independently on this message I came up with another patch
> (I'm just about to sent it right away) which I think it
> the best to have in current case.
> 
> Ideally I would like to see const struct gpio_chip *gc to be a parameter
> to the GPIO chip add(). But it may happen in distant future.

I have updated this patch locally to use dev_of_node() instead of
to_of_node(chip->fwnode), and also relying on the patch I just sent.

Nevertheless, for of_gpiochip_add()/of_gpiochip_remove() and
of_mm_gpiochip_add_data() I still left use of fwnode, because it feels
the right thing to do: we are taking reference on the input data in
such cases.

-- 
With Best Regards,
Andy Shevchenko