Message ID | 20220524074007.2792445-2-Basavaraj.Natikar@amd.com |
---|---|
State | New |
Headers | show |
Series | Enhancements to AMD pinctrl and implementation of AMD pinmux | expand |
Hi Basavaraj, thanks for your patch! On Tue, May 24, 2022 at 9:40 AM Basavaraj Natikar <Basavaraj.Natikar@amd.com> wrote: > +#define PINCTRL_GRP(_name, _pins, _npins) \ > + { .name = _name, .pins = _pins, .npins = _npins} Please prefix this macro with AMD_ so it doesn't run the risk of namespace collisions with generic macros, AMD_PINCTRL_GRP(...). Yours, Linus Walleij
On Tue, May 24, 2022 at 10:13 AM Basavaraj Natikar <Basavaraj.Natikar@amd.com> wrote: > > AMD pingroup can be extended to support multi-function pins. > Hence define and use PINCTRL_GRP to manage and represent > larger number of pingroups inline. > > Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> > --- > drivers/pinctrl/pinctrl-amd.h | 39 ++++++++--------------------------- > 1 file changed, 9 insertions(+), 30 deletions(-) > > diff --git a/drivers/pinctrl/pinctrl-amd.h b/drivers/pinctrl/pinctrl-amd.h > index 1d4317073654..de2bc9dddc9c 100644 > --- a/drivers/pinctrl/pinctrl-amd.h > +++ b/drivers/pinctrl/pinctrl-amd.h > @@ -296,37 +296,16 @@ static const unsigned i2c3_pins[] = {19, 20}; > static const unsigned uart0_pins[] = {135, 136, 137, 138, 139}; > static const unsigned uart1_pins[] = {140, 141, 142, 143, 144}; > > +#define PINCTRL_GRP(_name, _pins, _npins) \ > + { .name = _name, .pins = _pins, .npins = _npins} > + > static const struct amd_pingroup kerncz_groups[] = { > - { > - .name = "i2c0", > - .pins = i2c0_pins, > - .npins = 2, > - }, > - { > - .name = "i2c1", > - .pins = i2c1_pins, > - .npins = 2, > - }, > - { > - .name = "i2c2", > - .pins = i2c2_pins, > - .npins = 2, > - }, > - { > - .name = "i2c3", > - .pins = i2c3_pins, > - .npins = 2, > - }, > - { > - .name = "uart0", > - .pins = uart0_pins, > - .npins = 5, > - }, > - { > - .name = "uart1", > - .pins = uart1_pins, > - .npins = 5, > - }, > + PINCTRL_GRP("i2c0", i2c0_pins, 2), > + PINCTRL_GRP("i2c1", i2c1_pins, 2), > + PINCTRL_GRP("i2c2", i2c2_pins, 2), > + PINCTRL_GRP("i2c3", i2c3_pins, 2), > + PINCTRL_GRP("uart0", uart0_pins, 5), > + PINCTRL_GRP("uart1", uart1_pins, 5), > }; > > #endif > -- > 2.25.1 >
On Tue, May 24, 2022 at 10:13 AM Basavaraj Natikar <Basavaraj.Natikar@amd.com> wrote: > > AMD pingroup can be extended to support multi-function pins. > Hence define and use PINCTRL_GRP to manage and represent > larger number of pingroups inline. This is good idea, but please make it available for all pin control drivers, since the data structure like pingroup { *name; *pins; npins; } is used in many drivers. That said, I think the AMD_ prefix will be misleading in this case. ... > +#define PINCTRL_GRP(_name, _pins, _npins) \ > + { .name = _name, .pins = _pins, .npins = _npins} Please, don't forget spaces and commas, also it would be better to 1) split {} on the separate lines; 2) make it compound literal, so one can use it in the code.
On Tue, May 24, 2022 at 4:39 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, May 24, 2022 at 10:13 AM Basavaraj Natikar > <Basavaraj.Natikar@amd.com> wrote: > > > > AMD pingroup can be extended to support multi-function pins. > > Hence define and use PINCTRL_GRP to manage and represent > > larger number of pingroups inline. > > This is good idea, but please make it available for all pin control > drivers, since the data structure like > > pingroup { > *name; > *pins; > npins; > } > > is used in many drivers. > > That said, I think the AMD_ prefix will be misleading in this case. Aha you mean like a utility macro? That's useful, don't know where to put it though, include/linux/pinctrl/pinmux.h? Yours, Linus Walleij
On 5/25/2022 11:35 AM, Linus Walleij wrote: > On Tue, May 24, 2022 at 4:39 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Tue, May 24, 2022 at 10:13 AM Basavaraj Natikar >> <Basavaraj.Natikar@amd.com> wrote: >>> AMD pingroup can be extended to support multi-function pins. >>> Hence define and use PINCTRL_GRP to manage and represent >>> larger number of pingroups inline. >> This is good idea, but please make it available for all pin control >> drivers, since the data structure like >> >> pingroup { >> *name; >> *pins; >> npins; >> } >> >> is used in many drivers. >> >> That said, I think the AMD_ prefix will be misleading in this case. > Aha you mean like a utility macro? That's useful, don't know where to put it > though, include/linux/pinctrl/pinmux.h? For me looks like we need to put this in include/linux/pinctrl/pinctrl.h for a generic usage. is this fine? > Yours, > Linus Walleij
diff --git a/drivers/pinctrl/pinctrl-amd.h b/drivers/pinctrl/pinctrl-amd.h index 1d4317073654..de2bc9dddc9c 100644 --- a/drivers/pinctrl/pinctrl-amd.h +++ b/drivers/pinctrl/pinctrl-amd.h @@ -296,37 +296,16 @@ static const unsigned i2c3_pins[] = {19, 20}; static const unsigned uart0_pins[] = {135, 136, 137, 138, 139}; static const unsigned uart1_pins[] = {140, 141, 142, 143, 144}; +#define PINCTRL_GRP(_name, _pins, _npins) \ + { .name = _name, .pins = _pins, .npins = _npins} + static const struct amd_pingroup kerncz_groups[] = { - { - .name = "i2c0", - .pins = i2c0_pins, - .npins = 2, - }, - { - .name = "i2c1", - .pins = i2c1_pins, - .npins = 2, - }, - { - .name = "i2c2", - .pins = i2c2_pins, - .npins = 2, - }, - { - .name = "i2c3", - .pins = i2c3_pins, - .npins = 2, - }, - { - .name = "uart0", - .pins = uart0_pins, - .npins = 5, - }, - { - .name = "uart1", - .pins = uart1_pins, - .npins = 5, - }, + PINCTRL_GRP("i2c0", i2c0_pins, 2), + PINCTRL_GRP("i2c1", i2c1_pins, 2), + PINCTRL_GRP("i2c2", i2c2_pins, 2), + PINCTRL_GRP("i2c3", i2c3_pins, 2), + PINCTRL_GRP("uart0", uart0_pins, 5), + PINCTRL_GRP("uart1", uart1_pins, 5), }; #endif
AMD pingroup can be extended to support multi-function pins. Hence define and use PINCTRL_GRP to manage and represent larger number of pingroups inline. Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> --- drivers/pinctrl/pinctrl-amd.h | 39 ++++++++--------------------------- 1 file changed, 9 insertions(+), 30 deletions(-)