Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 3 May 2023 12:41:56 +0300 | Subject | Re: [PATCH v5 2/3] pinctrl: qcom: Refactor target specific pinctrl driver |
| |
On Wed, May 3, 2023 at 8:39 AM Rohit Agarwal <quic_rohiagar@quicinc.com> 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
#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.
...
> +#include <linux/pinctrl/pinctrl.h>
Keep it separate, and below the generic ones...
> #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.
...
> #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)) \ > }
-- With Best Regards, Andy Shevchenko
| |