Re: [PATCH v3 1/6] pinctrl: Add pingroup and define PINCTRL_GRP

[Basavaraj Natikar <bnatikar@xxxxxxx>]

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