| Message ID | 20220531084322.1310250-2-Basavaraj.Natikar@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enhancements to AMD pinctrl and implementation of AMD pinmux | expand |
On Tue, May 31, 2022 at 02:13:17PM +0530, Basavaraj Natikar wrote: > Add 'struct pingroup' to represent pingroup and 'PINCTRL_GRP' macro for > inline use. Both are used to manage and represent larger number > of pingroups. Thanks! My comments below. ... > +/** > + * struct pingroups - provides information on pingroup Try `make W=1` against each of your patches from the series. Here is the kernel doc issue. > + * @name: a name for pingroup > + * @pins: an array of pins in the pingroup > + * @npins: number of pins in the pingroup > + */ > +struct pingroup { > + const char *name; > + const unsigned int *pins; > + unsigned int npins; size_t probably would be better. > +}; > + > +/* Convenience macro to define a single named or anonymous pingroup */ > +#define PINCTRL_GRP(_name, _pins, _npins) \ I think PINCTRL_PINGROUP would be more precise. > +((struct pingroup) { \ No need to have space before { and compound literal means that it's not a GCC expression, i.e. drop outer parentheses (). > + .name = _name, \ > + .pins = _pins, \ > + .npins = _npins, \ > +})
On 5/31/2022 2:49 PM, Andy Shevchenko wrote: > On Tue, May 31, 2022 at 02:13:17PM +0530, Basavaraj Natikar wrote: >> Add 'struct pingroup' to represent pingroup and 'PINCTRL_GRP' macro for >> inline use. Both are used to manage and represent larger number >> of pingroups. > > Thanks! My comments below. > > ... > >> +/** >> + * struct pingroups - provides information on pingroup > Try `make W=1` against each of your patches from the series. Here is the kernel > doc issue. shall address your comments in my next revision, I tried 'make W=1' could not hit the kernel doc issue. Can you please elaborate a bit. >> + * @name: a name for pingroup >> + * @pins: an array of pins in the pingroup >> + * @npins: number of pins in the pingroup >> + */ >> +struct pingroup { >> + const char *name; >> + const unsigned int *pins; >> + unsigned int npins; > size_t probably would be better. > >> +}; >> + >> +/* Convenience macro to define a single named or anonymous pingroup */ >> +#define PINCTRL_GRP(_name, _pins, _npins) \ > I think PINCTRL_PINGROUP would be more precise. > >> +((struct pingroup) { \ > No need to have space before { and compound literal means that it's not a GCC > expression, i.e. drop outer parentheses (). > >> + .name = _name, \ >> + .pins = _pins, \ >> + .npins = _npins, \ >> +}) yes, or else I will hit the checkpatch error as below ERROR: Macros with complex values should be enclosed in parentheses #36: FILE: include/linux/pinctrl/pinctrl.h:42: +#define PINCTRL_GRP(_name, _pins, _npins) \ +(struct pingroup){ \ + .name = _name, \ + .pins = _pins, \ + .npins = _npins, \ +} Thanks, Basavaraj
On Tue, May 31, 2022 at 07:35:44PM +0530, Basavaraj Natikar wrote: > On 5/31/2022 2:49 PM, Andy Shevchenko wrote: > > On Tue, May 31, 2022 at 02:13:17PM +0530, Basavaraj Natikar wrote: ... > >> +/** > >> + * struct pingroups - provides information on pingroup > > Try `make W=1` against each of your patches from the series. Here is the kernel > > doc issue. > > shall address your comments in my next revision, I tried 'make W=1' could not hit the > kernel doc issue. Can you please elaborate a bit. Hmm... $ scripts/kernel-doc -none -v include/linux/pinctrl/pinctrl.h ... include/linux/pinctrl/pinctrl.h:39: warning: expecting prototype for struct pingroups. Prototype was for struct pingroup instead > >> + * @name: a name for pingroup > >> + * @pins: an array of pins in the pingroup > >> + * @npins: number of pins in the pingroup > >> + */ > >> +struct pingroup { > >> + const char *name; > >> + const unsigned int *pins; > >> + unsigned int npins; > > size_t probably would be better. > > > >> +}; ... > >> +/* Convenience macro to define a single named or anonymous pingroup */ > >> +#define PINCTRL_GRP(_name, _pins, _npins) \ > > I think PINCTRL_PINGROUP would be more precise. > > > >> +((struct pingroup) { \ > > No need to have space before { and compound literal means that it's not a GCC > > expression, i.e. drop outer parentheses (). > > > >> + .name = _name, \ > >> + .pins = _pins, \ > >> + .npins = _npins, \ > >> +}) > > > yes, or else I will hit the checkpatch error as below Does it compile? Does it work? If so, fix checkpatch. > ERROR: Macros with complex values should be enclosed in parentheses > > #36: FILE: include/linux/pinctrl/pinctrl.h:42: > > +#define PINCTRL_GRP(_name, _pins, _npins) \ > +(struct pingroup){ \ > + .name = _name, \ > + .pins = _pins, \ > + .npins = _npins, \ > +}
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h index 70b45d28e7a9..1bdffac6e7aa 100644 --- a/include/linux/pinctrl/pinctrl.h +++ b/include/linux/pinctrl/pinctrl.h @@ -26,6 +26,26 @@ struct pin_config_item; struct gpio_chip; struct device_node; +/** + * struct pingroups - provides information on pingroup + * @name: a name for pingroup + * @pins: an array of pins in the pingroup + * @npins: number of pins in the pingroup + */ +struct pingroup { + const char *name; + const unsigned int *pins; + unsigned int npins; +}; + +/* Convenience macro to define a single named or anonymous pingroup */ +#define PINCTRL_GRP(_name, _pins, _npins) \ +((struct pingroup) { \ + .name = _name, \ + .pins = _pins, \ + .npins = _npins, \ +}) + /** * struct pinctrl_pin_desc - boards/machines provide information on their * pins, pads or other muxable units in this struct
Add 'struct pingroup' to represent pingroup and 'PINCTRL_GRP' macro for inline use. Both are used to manage and represent larger number of pingroups. Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> --- include/linux/pinctrl/pinctrl.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)