| Message ID | 20220329152926.50958-1-andriy.shevchenko@linux.intel.com |
|---|---|
| Headers | show |
| Series | gpiolib: Two new helpers and way toward fwnode | expand |
On 29/03/2022 17:29, Andy Shevchenko wrote: > Rename REG_* to MREG_* as a prerequisite for enabling COMPILE_TEST. What error do you hit ? MREG_ is rather ugly, something like PINCONF_REG_ or more simpler MESON_REG_ would be more appropriate. Neil > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/pinctrl/meson/pinctrl-meson.c | 24 ++++++++++++------------ > drivers/pinctrl/meson/pinctrl-meson.h | 24 ++++++++++++------------ > 2 files changed, 24 insertions(+), 24 deletions(-) > > diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c > index 49851444a6e3..64da61ba2bb9 100644 > --- a/drivers/pinctrl/meson/pinctrl-meson.c > +++ b/drivers/pinctrl/meson/pinctrl-meson.c > @@ -218,13 +218,13 @@ static int meson_pinconf_set_output(struct meson_pinctrl *pc, > unsigned int pin, > bool out) > { > - return meson_pinconf_set_gpio_bit(pc, pin, REG_DIR, !out); > + return meson_pinconf_set_gpio_bit(pc, pin, MREG_DIR, !out); > } > > static int meson_pinconf_get_output(struct meson_pinctrl *pc, > unsigned int pin) > { > - int ret = meson_pinconf_get_gpio_bit(pc, pin, REG_DIR); > + int ret = meson_pinconf_get_gpio_bit(pc, pin, MREG_DIR); > > if (ret < 0) > return ret; > @@ -236,13 +236,13 @@ static int meson_pinconf_set_drive(struct meson_pinctrl *pc, > unsigned int pin, > bool high) > { > - return meson_pinconf_set_gpio_bit(pc, pin, REG_OUT, high); > + return meson_pinconf_set_gpio_bit(pc, pin, MREG_OUT, high); > } > > static int meson_pinconf_get_drive(struct meson_pinctrl *pc, > unsigned int pin) > { > - return meson_pinconf_get_gpio_bit(pc, pin, REG_OUT); > + return meson_pinconf_get_gpio_bit(pc, pin, MREG_OUT); > } > > static int meson_pinconf_set_output_drive(struct meson_pinctrl *pc, > @@ -269,7 +269,7 @@ static int meson_pinconf_disable_bias(struct meson_pinctrl *pc, > if (ret) > return ret; > > - meson_calc_reg_and_bit(bank, pin, REG_PULLEN, ®, &bit); > + meson_calc_reg_and_bit(bank, pin, MREG_PULLEN, ®, &bit); > ret = regmap_update_bits(pc->reg_pullen, reg, BIT(bit), 0); > if (ret) > return ret; > @@ -288,7 +288,7 @@ static int meson_pinconf_enable_bias(struct meson_pinctrl *pc, unsigned int pin, > if (ret) > return ret; > > - meson_calc_reg_and_bit(bank, pin, REG_PULL, ®, &bit); > + meson_calc_reg_and_bit(bank, pin, MREG_PULL, ®, &bit); > if (pull_up) > val = BIT(bit); > > @@ -296,7 +296,7 @@ static int meson_pinconf_enable_bias(struct meson_pinctrl *pc, unsigned int pin, > if (ret) > return ret; > > - meson_calc_reg_and_bit(bank, pin, REG_PULLEN, ®, &bit); > + meson_calc_reg_and_bit(bank, pin, MREG_PULLEN, ®, &bit); > ret = regmap_update_bits(pc->reg_pullen, reg, BIT(bit), BIT(bit)); > if (ret) > return ret; > @@ -321,7 +321,7 @@ static int meson_pinconf_set_drive_strength(struct meson_pinctrl *pc, > if (ret) > return ret; > > - meson_calc_reg_and_bit(bank, pin, REG_DS, ®, &bit); > + meson_calc_reg_and_bit(bank, pin, MREG_DS, ®, &bit); > > if (drive_strength_ua <= 500) { > ds_val = MESON_PINCONF_DRV_500UA; > @@ -407,7 +407,7 @@ static int meson_pinconf_get_pull(struct meson_pinctrl *pc, unsigned int pin) > if (ret) > return ret; > > - meson_calc_reg_and_bit(bank, pin, REG_PULLEN, ®, &bit); > + meson_calc_reg_and_bit(bank, pin, MREG_PULLEN, ®, &bit); > > ret = regmap_read(pc->reg_pullen, reg, &val); > if (ret) > @@ -416,7 +416,7 @@ static int meson_pinconf_get_pull(struct meson_pinctrl *pc, unsigned int pin) > if (!(val & BIT(bit))) { > conf = PIN_CONFIG_BIAS_DISABLE; > } else { > - meson_calc_reg_and_bit(bank, pin, REG_PULL, ®, &bit); > + meson_calc_reg_and_bit(bank, pin, MREG_PULL, ®, &bit); > > ret = regmap_read(pc->reg_pull, reg, &val); > if (ret) > @@ -447,7 +447,7 @@ static int meson_pinconf_get_drive_strength(struct meson_pinctrl *pc, > if (ret) > return ret; > > - meson_calc_reg_and_bit(bank, pin, REG_DS, ®, &bit); > + meson_calc_reg_and_bit(bank, pin, MREG_DS, ®, &bit); > > ret = regmap_read(pc->reg_ds, reg, &val); > if (ret) > @@ -595,7 +595,7 @@ static int meson_gpio_get(struct gpio_chip *chip, unsigned gpio) > if (ret) > return ret; > > - meson_calc_reg_and_bit(bank, gpio, REG_IN, ®, &bit); > + meson_calc_reg_and_bit(bank, gpio, MREG_IN, ®, &bit); > regmap_read(pc->reg_gpio, reg, &val); > > return !!(val & BIT(bit)); > diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h > index ff5372e0a475..c00d9ad27843 100644 > --- a/drivers/pinctrl/meson/pinctrl-meson.h > +++ b/drivers/pinctrl/meson/pinctrl-meson.h > @@ -63,12 +63,12 @@ struct meson_reg_desc { > * enum meson_reg_type - type of registers encoded in @meson_reg_desc > */ > enum meson_reg_type { > - REG_PULLEN, > - REG_PULL, > - REG_DIR, > - REG_OUT, > - REG_IN, > - REG_DS, > + MREG_PULLEN, > + MREG_PULL, > + MREG_DIR, > + MREG_OUT, > + MREG_IN, > + MREG_DS, > NUM_REG, > }; > > @@ -150,12 +150,12 @@ struct meson_pinctrl { > .irq_first = fi, \ > .irq_last = li, \ > .regs = { \ > - [REG_PULLEN] = { per, peb }, \ > - [REG_PULL] = { pr, pb }, \ > - [REG_DIR] = { dr, db }, \ > - [REG_OUT] = { or, ob }, \ > - [REG_IN] = { ir, ib }, \ > - [REG_DS] = { dsr, dsb }, \ > + [MREG_PULLEN] = { per, peb }, \ > + [MREG_PULL] = { pr, pb }, \ > + [MREG_DIR] = { dr, db }, \ > + [MREG_OUT] = { or, ob }, \ > + [MREG_IN] = { ir, ib }, \ > + [MREG_DS] = { dsr, dsb }, \ > }, \ > } >
On Tue, Mar 29, 2022 at 06:13:19PM +0200, Neil Armstrong wrote: > On 29/03/2022 17:29, Andy Shevchenko wrote: > > Rename REG_* to * as a prerequisite for enabling COMPILE_TEST. > > What error do you hit ? arch/x86/include/asm/arch_hweight.h:9:17: error: expected identifier before string constant 9 | #define REG_OUT "a" | ^~~ drivers/pinctrl/meson/pinctrl-meson.h:69:9: note: in expansion of macro ‘REG_OUT’ 69 | REG_OUT, And some more. > MREG_ is rather ugly, something like PINCONF_REG_ or more simpler MESON_REG_ would be more appropriate. Fine with me.
Hi Andy, On Tue, Mar 29, 2022 at 6:47 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Tue, Mar 29, 2022 at 06:13:19PM +0200, Neil Armstrong wrote: > > On 29/03/2022 17:29, Andy Shevchenko wrote: > > > Rename REG_* to * as a prerequisite for enabling COMPILE_TEST. > > > > What error do you hit ? > > arch/x86/include/asm/arch_hweight.h:9:17: error: expected identifier before string constant > 9 | #define REG_OUT "a" > | ^~~ Perhaps REG_{OUT,IN} in arch/x86/include/asm/arch_hweight.h should be renamed instead, as this is a generic header file that can be included anywhere, while the REG_{OUT,IN} definitions are only used locally, in the header file? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 30/03/2022 10:54, Geert Uytterhoeven wrote: > Hi Andy, > > On Tue, Mar 29, 2022 at 6:47 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: >> On Tue, Mar 29, 2022 at 06:13:19PM +0200, Neil Armstrong wrote: >>> On 29/03/2022 17:29, Andy Shevchenko wrote: >>>> Rename REG_* to * as a prerequisite for enabling COMPILE_TEST. >>> >>> What error do you hit ? >> >> arch/x86/include/asm/arch_hweight.h:9:17: error: expected identifier before string constant >> 9 | #define REG_OUT "a" >> | ^~~ > > Perhaps REG_{OUT,IN} in arch/x86/include/asm/arch_hweight.h should be > renamed instead, as this is a generic header file that can be included > anywhere, while the REG_{OUT,IN} definitions are only used locally, > in the header file? Even better, those REG_OUT/REG_IN should be undefined at the end of the header since only used in the headers inline functions: ==============><================================== diff --git a/arch/x86/include/asm/arch_hweight.h b/arch/x86/include/asm/arch_hweight.h index ba88edd0d58b..139a4b0a2a14 100644 --- a/arch/x86/include/asm/arch_hweight.h +++ b/arch/x86/include/asm/arch_hweight.h @@ -52,4 +52,7 @@ static __always_inline unsigned long __arch_hweight64(__u64 w) } #endif /* CONFIG_X86_32 */ +#undef REG_IN +#undef REG_OUT + #endif ==============><================================== Neil > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
On Wed, Mar 30, 2022 at 11:09:11AM +0200, Neil Armstrong wrote: > On 30/03/2022 10:54, Geert Uytterhoeven wrote: > > On Tue, Mar 29, 2022 at 6:47 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Tue, Mar 29, 2022 at 06:13:19PM +0200, Neil Armstrong wrote: > > > > On 29/03/2022 17:29, Andy Shevchenko wrote: ... > > > > What error do you hit ? > > > > > > arch/x86/include/asm/arch_hweight.h:9:17: error: expected identifier before string constant > > > 9 | #define REG_OUT "a" > > > | ^~~ > > > > Perhaps REG_{OUT,IN} in arch/x86/include/asm/arch_hweight.h should be > > renamed instead, as this is a generic header file that can be included > > anywhere, while the REG_{OUT,IN} definitions are only used locally, > > in the header file? > > Even better, those REG_OUT/REG_IN should be undefined at the end of the header since only > used in the headers inline functions: > ==============><================================== > diff --git a/arch/x86/include/asm/arch_hweight.h b/arch/x86/include/asm/arch_hweight.h > index ba88edd0d58b..139a4b0a2a14 100644 > --- a/arch/x86/include/asm/arch_hweight.h > +++ b/arch/x86/include/asm/arch_hweight.h > @@ -52,4 +52,7 @@ static __always_inline unsigned long __arch_hweight64(__u64 w) > } > #endif /* CONFIG_X86_32 */ > > +#undef REG_IN > +#undef REG_OUT > + > #endif > ==============><================================== Can you submit a formal patch, please? And I think it would be good to have my patch as well, so we do not depend on the fate of the other one.
On Wed, Mar 30, 2022 at 12:02:07PM +0200, Geert Uytterhoeven wrote: > On Tue, Mar 29, 2022 at 5:29 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: ... > > + unsigned int count; > > Preinitialize to zero? Whatever for what consensus will be achieved. Initially I have that way, then I changed. > > + count = 0; > > + for_each_gpiochip_node(dev, child) > > + count++; > > Regardless: > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> Thanks!
Hi Andy, On Wed, Mar 30, 2022 at 2:17 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, Mar 30, 2022 at 12:00:27PM +0200, Geert Uytterhoeven wrote: > > On Tue, Mar 29, 2022 at 5:29 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > ... > > > > + struct fwnode_reference_args of_args; > > > > fw_args? > > Perhaps just args as other drivers do? Fine for me. > > > - chip->label = devm_kasprintf(rza1_pctl->dev, GFP_KERNEL, "%pOFn", > > > - np); > > > + chip->label = devm_kasprintf(rza1_pctl->dev, GFP_KERNEL, "%pfw", fwnode); > > > > This changes the label from e.g. "/soc/pinctrl@fcfe3000/gpio-11" to "gpio-11". > > Hmm... It seems other way around, i.e. it changes from the name to full name. Oops, sorry about that. (I accidentally included the change from testing "%pfwP" ;-) > > > %pfwP? > > But conclusion here is correct. Good catch! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Wed, Mar 30, 2022 at 02:32:36PM +0200, Fabien DESSENNE wrote: > Hi Andy > > > Thank you for the patch. > > Fabien > > On 29/03/2022 17:29, Andy Shevchenko wrote: > > Switch the code to use for_each_gpiochip_node() helper. > > > > While at it, in order to avoid additional churn in the future, > > switch to fwnode APIs where it makes sense. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Reviewed-by: Fabien Dessenne <fabien.dessenne@foss.st.com> Thank you for the prompt review!
On 30/03/2022 11:18, Andy Shevchenko wrote: > On Wed, Mar 30, 2022 at 11:09:11AM +0200, Neil Armstrong wrote: >> On 30/03/2022 10:54, Geert Uytterhoeven wrote: >>> On Tue, Mar 29, 2022 at 6:47 PM Andy Shevchenko >>> <andriy.shevchenko@linux.intel.com> wrote: >>>> On Tue, Mar 29, 2022 at 06:13:19PM +0200, Neil Armstrong wrote: >>>>> On 29/03/2022 17:29, Andy Shevchenko wrote: > > ... > >>>>> What error do you hit ? >>>> >>>> arch/x86/include/asm/arch_hweight.h:9:17: error: expected identifier before string constant >>>> 9 | #define REG_OUT "a" >>>> | ^~~ >>> >>> Perhaps REG_{OUT,IN} in arch/x86/include/asm/arch_hweight.h should be >>> renamed instead, as this is a generic header file that can be included >>> anywhere, while the REG_{OUT,IN} definitions are only used locally, >>> in the header file? >> >> Even better, those REG_OUT/REG_IN should be undefined at the end of the header since only >> used in the headers inline functions: >> ==============><================================== >> diff --git a/arch/x86/include/asm/arch_hweight.h b/arch/x86/include/asm/arch_hweight.h >> index ba88edd0d58b..139a4b0a2a14 100644 >> --- a/arch/x86/include/asm/arch_hweight.h >> +++ b/arch/x86/include/asm/arch_hweight.h >> @@ -52,4 +52,7 @@ static __always_inline unsigned long __arch_hweight64(__u64 w) >> } >> #endif /* CONFIG_X86_32 */ >> >> +#undef REG_IN >> +#undef REG_OUT >> + >> #endif >> ==============><================================== > > Can you submit a formal patch, please? I'll submit it separately > > > And I think it would be good to have my patch as well, so we do not depend on > the fate of the other one. > Yes sure Thanks, Neil
On Wed, Mar 30, 2022 at 05:22:56PM +0200, Neil Armstrong wrote: > On 30/03/2022 11:18, Andy Shevchenko wrote: ... > > > > > > What error do you hit ? > > > > > > > > > > arch/x86/include/asm/arch_hweight.h:9:17: error: expected identifier before string constant > > > > > 9 | #define REG_OUT "a" > > > > > | ^~~ > > > > > > > > Perhaps REG_{OUT,IN} in arch/x86/include/asm/arch_hweight.h should be > > > > renamed instead, as this is a generic header file that can be included > > > > anywhere, while the REG_{OUT,IN} definitions are only used locally, > > > > in the header file? > > > > > > Even better, those REG_OUT/REG_IN should be undefined at the end of the header since only > > > used in the headers inline functions: > > > ==============><================================== > > > diff --git a/arch/x86/include/asm/arch_hweight.h b/arch/x86/include/asm/arch_hweight.h > > > index ba88edd0d58b..139a4b0a2a14 100644 > > > --- a/arch/x86/include/asm/arch_hweight.h > > > +++ b/arch/x86/include/asm/arch_hweight.h > > > @@ -52,4 +52,7 @@ static __always_inline unsigned long __arch_hweight64(__u64 w) > > > } > > > #endif /* CONFIG_X86_32 */ > > > > > > +#undef REG_IN > > > +#undef REG_OUT > > > + > > > #endif > > > ==============><================================== > > > > Can you submit a formal patch, please? > > I'll submit it separately Sure! > > And I think it would be good to have my patch as well, so we do not depend on > > the fate of the other one. > > Yes sure Thanks for acknowledging and review!
This is a spin-off of the previous work of switching GPIO library to use fwnode instead of of_node. Here we introduce a couple of a new macro helpers, which allows to switch some of the drivers to use fwnode and partially fwnode APIs. Bart, Linus, I can take it thru my tree with an immutable branch if it's the way you prefer, otherwise please suggest on how to proceed. Changelog v2: - properly based, so kbuild bot may test it (LKP) - fixed typo in the macro (Geert) - split to two macro helpers and rename the gpiochip_count() - tagged one of stm32 and one of meson patches (Fabien, Neil) - unified previously standalone armada patch - due to above rewrote the armada patch from v1 completely (Sergey) - added a lot of a new patches - compile tested all of them on x86 Andy Shevchenko (13): gpiolib: Introduce for_each_gpiochip_node() loop helper gpiolib: Introduce gpiochip_node_count() helper pinctrl: stm32: Replace custom code by gpiochip_node_count() call pinctrl: stm32: Switch to use for_each_gpiochip_node() helper pinctrl: samsung: Switch to use for_each_gpiochip_node() helper pinctrl: renesas: rza1: Replace custom code by gpiochip_node_count() call pinctrl: renesas: rza1: Switch to use for_each_gpiochip_node() helper pinctrl: npcm7xx: Switch to use for_each_gpiochip_node() helper pinctrl: meson: Rename REG_* to MREG_* pinctrl: meson: Enable COMPILE_TEST pinctrl: meson: Replace custom code by gpiochip_node_count() call pinctrl: armada-37xx: Switch to use fwnode instead of of_node pinctrl: armada-37xx: Reuse GPIO fwnode in armada_37xx_irqchip_register() drivers/pinctrl/meson/Kconfig | 2 +- drivers/pinctrl/meson/pinctrl-meson.c | 52 ++++---- drivers/pinctrl/meson/pinctrl-meson.h | 24 ++-- drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 34 ++--- drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c | 141 +++++++++----------- drivers/pinctrl/renesas/pinctrl-rza1.c | 39 ++---- drivers/pinctrl/samsung/pinctrl-samsung.c | 30 ++--- drivers/pinctrl/samsung/pinctrl-samsung.h | 2 +- drivers/pinctrl/stm32/pinctrl-stm32.c | 80 +++++------ include/linux/gpio/driver.h | 19 ++- 10 files changed, 187 insertions(+), 236 deletions(-)