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
#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
