Re: [PATCH v5 2/3] pinctrl: qcom: Refactor target specific pinctrl driver

[Rohit Agarwal]

On 5/3/2023 3:11 PM, Andy Shevchenko wrote:
> On Wed, May 3, 2023 at 8:39 AM Rohit Agarwal <quic_rohiagar@xxxxxxxxxxx> wrote:
> > Update the msm_function and msm_pingroup structure to reuse the generic
> structures
>
> > pinfunction and pingroup structures. Also refactor pinctrl drivers to adjust
> > the new macro and updated structure defined in pinctrl.h and pinctrl_msm.h
> > respectively.
> Thanks for this, my comments below.
>
> ...
>
> >   #define FUNCTION(fname)                                        \
> >          [APQ_MUX_##fname] = {                           \
> > -               .name = #fname,                         \
> > -               .groups = fname##_groups,               \
> > -               .ngroups = ARRAY_SIZE(fname##_groups),  \
> > -       }
> > +               .func = PINCTRL_PINFUNCTION(#fname,                     \
> > +                                       fname##_groups,                 \
> > +                                       ARRAY_SIZE(fname##_groups))             \
> > +                       }
> Does it really make sense to keep an additional wrapper data type that
> does not add any value? Can't we simply have
This was done as part of embeding the pinfunction structure in msm_function.
Will drop this in the next.
>    #define FUNCTION(fname)      [...fname] = PINCTRL_PINFUNCTION(...)
>
> ?
>
> ...
>
> > +               .grp = PINCTRL_PINGROUP("gpio"#id, gpio##id##_pins,     \
> > +                       (unsigned int)ARRAY_SIZE(gpio##id##_pins)),     \
> Why do you need this casting? Same Q to all the rest of the similar cases.
Will drop it.
> ...
>
> > +#include <linux/pinctrl/pinctrl.h>
> Keep it separate, and below the generic ones...
Sure
> >   #include <linux/pm.h>
> >   #include <linux/types.h>
> ...like here (note also a blank line).
>
> ...
>
> >   /**
> >    * struct msm_function - a pinmux function
> > - * @name:    Name of the pinmux function.
> > - * @groups:  List of pingroups for this function.
> > - * @ngroups: Number of entries in @groups.
> > + * @func: Generic data of the pin function (name and groups of pins)
> >    */
> >   struct msm_function {
> > -       const char *name;
> > -       const char * const *groups;
> > -       unsigned ngroups;
> > +       struct pinfunction func;
> >   };
> But why? Just kill the entire structure.
Got it. Can we have a typedef for pinfunction to msm_function in the msm 
header file?
> ...
>
> >   #define FUNCTION(fname)                                        \
> This definition appears in many files, instead you can make a generic
> to this drivers one and use it here
>
> #define QCOM_FUNCTION(_prefix_, _fname_)
>    [_prefix_##_fname_] = PINCTRL_PINFUNCTION(...)
>
> #define FUNCTION(fname) QCOM_FUNCTION(msm_mux, fname)
>
> (this just a pseudocode, might not even be compilable)
>
> >          [msm_mux_##fname] = {                           \
> > -               .name = #fname,                         \
> > -               .groups = fname##_groups,               \
> > -               .ngroups = ARRAY_SIZE(fname##_groups),  \
> > +               .func = PINCTRL_PINFUNCTION(#fname,                     \
> > +                                       fname##_groups,                 \
> > +                                       ARRAY_SIZE(fname##_groups))             \
> >          }
Got your point. Maybe your option 2 of using MSM_PIN_FUNCTION seems more 
generic,
as there wont be any redefinition of any macro FUNCTION altogether in 
the target specific files.

Thanks,
Rohit.