diff mbox series

[v6,02/15] gpio: regmap: set gpio_chip of_node

Message ID 20210310125504.31886-3-noltari@gmail.com (mailing list archive)
State New, archived
Headers show
Series pinctrl: add BCM63XX pincontrol support | expand

Commit Message

Álvaro Fernández Rojas March 10, 2021, 12:54 p.m. UTC
This is needed for properly registering GPIO regmap as a child of a regmap
pin controller.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Reviewed-by: Michael Walle <michael@walle.cc>
---
 v6: add comment and simplify of_node assignment
 v5: switch to fwnode
 v4: fix documentation
 v3: introduce patch needed for properly parsing gpio-range

 drivers/gpio/gpio-regmap.c  | 1 +
 include/linux/gpio/regmap.h | 4 ++++
 2 files changed, 5 insertions(+)

Comments

Andy Shevchenko March 10, 2021, 2:01 p.m. UTC | #1
On Wed, Mar 10, 2021 at 2:55 PM Álvaro Fernández Rojas
<noltari@gmail.com> wrote:
>
> This is needed for properly registering GPIO regmap as a child of a regmap
> pin controller.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Thanks!

> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> Reviewed-by: Michael Walle <michael@walle.cc>
> ---
>  v6: add comment and simplify of_node assignment
>  v5: switch to fwnode
>  v4: fix documentation
>  v3: introduce patch needed for properly parsing gpio-range
>
>  drivers/gpio/gpio-regmap.c  | 1 +
>  include/linux/gpio/regmap.h | 4 ++++
>  2 files changed, 5 insertions(+)
>
> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> index 5412cb3b0b2a..d4fc656e70b0 100644
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
> @@ -249,6 +249,7 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
>
>         chip = &gpio->gpio_chip;
>         chip->parent = config->parent;
> +       chip->of_node = to_of_node(config->fwnode);
>         chip->base = -1;
>         chip->ngpio = config->ngpio;
>         chip->names = config->names;
> diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
> index ad76f3d0a6ba..334dd928042b 100644
> --- a/include/linux/gpio/regmap.h
> +++ b/include/linux/gpio/regmap.h
> @@ -4,6 +4,7 @@
>  #define _LINUX_GPIO_REGMAP_H
>
>  struct device;
> +struct fwnode_handle;
>  struct gpio_regmap;
>  struct irq_domain;
>  struct regmap;
> @@ -16,6 +17,8 @@ struct regmap;
>   * @parent:            The parent device
>   * @regmap:            The regmap used to access the registers
>   *                     given, the name of the device is used
> + * @fwnode:            (Optional) The firmware node.
> + *                     If not given, the fwnode of the parent is used.
>   * @label:             (Optional) Descriptive name for GPIO controller.
>   *                     If not given, the name of the device is used.
>   * @ngpio:             Number of GPIOs
> @@ -57,6 +60,7 @@ struct regmap;
>  struct gpio_regmap_config {
>         struct device *parent;
>         struct regmap *regmap;
> +       struct fwnode_handle *fwnode;
>
>         const char *label;
>         int ngpio;
> --
> 2.20.1
>
Michael Walle March 10, 2021, 6:27 p.m. UTC | #2
Am 2021-03-10 13:54, schrieb Álvaro Fernández Rojas:
> This is needed for properly registering GPIO regmap as a child of a 
> regmap
> pin controller.
> 
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> Reviewed-by: Michael Walle <michael@walle.cc>
> ---
>  v6: add comment and simplify of_node assignment

Ah, I see you add the comment for the documentation. Nice. But I'd
like to see it in the code, too. See below.

>  v5: switch to fwnode
>  v4: fix documentation
>  v3: introduce patch needed for properly parsing gpio-range
> 
>  drivers/gpio/gpio-regmap.c  | 1 +
>  include/linux/gpio/regmap.h | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> index 5412cb3b0b2a..d4fc656e70b0 100644
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
> @@ -249,6 +249,7 @@ struct gpio_regmap *gpio_regmap_register(const
> struct gpio_regmap_config *config
> 
>  	chip = &gpio->gpio_chip;
>  	chip->parent = config->parent;

If there will be a new version, please add the following comment:

/* gpiolib will use of_node of the parent if chip->of_node is NULL */

>> +       chip->of_node = to_of_node(config->fwnode);

Otherwise, it is not obvious that config->fwnode is optional.

-michael

> +	chip->of_node = to_of_node(config->fwnode);
>  	chip->base = -1;
>  	chip->ngpio = config->ngpio;
>  	chip->names = config->names;
> diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
> index ad76f3d0a6ba..334dd928042b 100644
> --- a/include/linux/gpio/regmap.h
> +++ b/include/linux/gpio/regmap.h
> @@ -4,6 +4,7 @@
>  #define _LINUX_GPIO_REGMAP_H
> 
>  struct device;
> +struct fwnode_handle;
>  struct gpio_regmap;
>  struct irq_domain;
>  struct regmap;
> @@ -16,6 +17,8 @@ struct regmap;
>   * @parent:		The parent device
>   * @regmap:		The regmap used to access the registers
>   *			given, the name of the device is used
> + * @fwnode:		(Optional) The firmware node.
> + *			If not given, the fwnode of the parent is used.
>   * @label:		(Optional) Descriptive name for GPIO controller.
>   *			If not given, the name of the device is used.
>   * @ngpio:		Number of GPIOs
> @@ -57,6 +60,7 @@ struct regmap;
>  struct gpio_regmap_config {
>  	struct device *parent;
>  	struct regmap *regmap;
> +	struct fwnode_handle *fwnode;
> 
>  	const char *label;
>  	int ngpio;
Álvaro Fernández Rojas March 10, 2021, 7:12 p.m. UTC | #3
Hi Michael,

> El 10 mar 2021, a las 19:27, Michael Walle <michael@walle.cc> escribió:
> 
> Am 2021-03-10 13:54, schrieb Álvaro Fernández Rojas:
>> This is needed for properly registering GPIO regmap as a child of a regmap
>> pin controller.
>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>> Reviewed-by: Michael Walle <michael@walle.cc>
>> ---
>> v6: add comment and simplify of_node assignment
> 
> Ah, I see you add the comment for the documentation. Nice. But I'd
> like to see it in the code, too. See below.

Ah, sorry for that, I thought you wanted it on the header.
Excuse me for that...

> 
>> v5: switch to fwnode
>> v4: fix documentation
>> v3: introduce patch needed for properly parsing gpio-range
>> drivers/gpio/gpio-regmap.c  | 1 +
>> include/linux/gpio/regmap.h | 4 ++++
>> 2 files changed, 5 insertions(+)
>> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
>> index 5412cb3b0b2a..d4fc656e70b0 100644
>> --- a/drivers/gpio/gpio-regmap.c
>> +++ b/drivers/gpio/gpio-regmap.c
>> @@ -249,6 +249,7 @@ struct gpio_regmap *gpio_regmap_register(const
>> struct gpio_regmap_config *config
>> 	chip = &gpio->gpio_chip;
>> 	chip->parent = config->parent;
> 
> If there will be a new version, please add the following comment:

Right now I don’t know that either, because I’m honestly getting tired of this…

> 
> /* gpiolib will use of_node of the parent if chip->of_node is NULL */
> 
>>> +       chip->of_node = to_of_node(config->fwnode);
> 
> Otherwise, it is not obvious that config->fwnode is optional.

Yes, you’re right.

> 
> -michael
> 
>> +	chip->of_node = to_of_node(config->fwnode);
>> 	chip->base = -1;
>> 	chip->ngpio = config->ngpio;
>> 	chip->names = config->names;
>> diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
>> index ad76f3d0a6ba..334dd928042b 100644
>> --- a/include/linux/gpio/regmap.h
>> +++ b/include/linux/gpio/regmap.h
>> @@ -4,6 +4,7 @@
>> #define _LINUX_GPIO_REGMAP_H
>> struct device;
>> +struct fwnode_handle;
>> struct gpio_regmap;
>> struct irq_domain;
>> struct regmap;
>> @@ -16,6 +17,8 @@ struct regmap;
>>  * @parent:		The parent device
>>  * @regmap:		The regmap used to access the registers
>>  *			given, the name of the device is used
>> + * @fwnode:		(Optional) The firmware node.
>> + *			If not given, the fwnode of the parent is used.
>>  * @label:		(Optional) Descriptive name for GPIO controller.
>>  *			If not given, the name of the device is used.
>>  * @ngpio:		Number of GPIOs
>> @@ -57,6 +60,7 @@ struct regmap;
>> struct gpio_regmap_config {
>> 	struct device *parent;
>> 	struct regmap *regmap;
>> +	struct fwnode_handle *fwnode;
>> 	const char *label;
>> 	int ngpio;
Linus Walleij March 11, 2021, 1:16 a.m. UTC | #4
On Wed, Mar 10, 2021 at 8:12 PM Álvaro Fernández Rojas
<noltari@gmail.com> wrote:

> > If there will be a new version, please add the following comment:
>
> Right now I don’t know that either, because I’m honestly getting tired of this…

IMO there is indeed such a thing as over-review when it comes
to migrating legacy platforms: as subsystem maintainer I ask the bigger
question: does the kernel look better after than before this patch? If the
author is stressed by too much review I tend to just apply it and say that
comments can be addressed by additional patches.

DT bindings are different because they are written in stone. We just need
to settle the DT bindings. Give the patch set some rest and come back and
poke me to apply it when the chatter stops.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index 5412cb3b0b2a..d4fc656e70b0 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -249,6 +249,7 @@  struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 
 	chip = &gpio->gpio_chip;
 	chip->parent = config->parent;
+	chip->of_node = to_of_node(config->fwnode);
 	chip->base = -1;
 	chip->ngpio = config->ngpio;
 	chip->names = config->names;
diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
index ad76f3d0a6ba..334dd928042b 100644
--- a/include/linux/gpio/regmap.h
+++ b/include/linux/gpio/regmap.h
@@ -4,6 +4,7 @@ 
 #define _LINUX_GPIO_REGMAP_H
 
 struct device;
+struct fwnode_handle;
 struct gpio_regmap;
 struct irq_domain;
 struct regmap;
@@ -16,6 +17,8 @@  struct regmap;
  * @parent:		The parent device
  * @regmap:		The regmap used to access the registers
  *			given, the name of the device is used
+ * @fwnode:		(Optional) The firmware node.
+ *			If not given, the fwnode of the parent is used.
  * @label:		(Optional) Descriptive name for GPIO controller.
  *			If not given, the name of the device is used.
  * @ngpio:		Number of GPIOs
@@ -57,6 +60,7 @@  struct regmap;
 struct gpio_regmap_config {
 	struct device *parent;
 	struct regmap *regmap;
+	struct fwnode_handle *fwnode;
 
 	const char *label;
 	int ngpio;