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