| Message ID | 1683092380-29551-1-git-send-email-quic_rohiagar@quicinc.com |
|---|---|
| Headers | show |
| Series | Add pinctrl support for SDX75 | expand |
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)) \ > }
On 5/3/2023 3:11 PM, Andy Shevchenko wrote: > 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 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.
On 5/3/2023 7:23 PM, Andy Shevchenko wrote: > On Wed, May 3, 2023 at 2:14 PM Rohit Agarwal <quic_rohiagar@quicinc.com> wrote: >> On 5/3/2023 3:11 PM, Andy Shevchenko wrote: >>> On Wed, May 3, 2023 at 8:39 AM Rohit Agarwal <quic_rohiagar@quicinc.com> wrote: > ... > >>>> /** >>>> * 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? > But why? You can replace the type everywhere it needs to be replaced. > I can't expect many lines to change. > > Also consider splitting struct pingroup change out of this. We will > focus only on the struct pinfunction change and less code to review. Ok Will update all of this. Thanks, Rohit.
Hi, Changes in v5: - Refactor the pinctrl target files based on the new macro and structure defined as suggested by Andy. Changes in v4: - Fixed the bindings check and rebased on linux-next. Changes in v3: - Rebased the bindings on linux-next as suggested by Krzysztof. Changes in v2: - Updated the bindings to clear the bindings check. This patch series adds pinctrl bindings and tlmm support for SDX75. Thanks, Rohit. Rohit Agarwal (3): dt-bindings: pinctrl: qcom: Add SDX75 pinctrl devicetree compatible pinctrl: qcom: Refactor target specific pinctrl driver pinctrl: qcom: Add SDX75 pincontrol driver .../bindings/pinctrl/qcom,sdx75-tlmm.yaml | 169 +++ drivers/pinctrl/qcom/Kconfig | 30 +- drivers/pinctrl/qcom/Makefile | 3 +- drivers/pinctrl/qcom/pinctrl-apq8064.c | 19 +- drivers/pinctrl/qcom/pinctrl-apq8084.c | 17 +- drivers/pinctrl/qcom/pinctrl-ipq4019.c | 12 +- drivers/pinctrl/qcom/pinctrl-ipq5332.c | 12 +- drivers/pinctrl/qcom/pinctrl-ipq6018.c | 12 +- drivers/pinctrl/qcom/pinctrl-ipq8064.c | 17 +- drivers/pinctrl/qcom/pinctrl-ipq8074.c | 12 +- drivers/pinctrl/qcom/pinctrl-mdm9607.c | 17 +- drivers/pinctrl/qcom/pinctrl-mdm9615.c | 12 +- drivers/pinctrl/qcom/pinctrl-msm.c | 18 +- drivers/pinctrl/qcom/pinctrl-msm.h | 17 +- drivers/pinctrl/qcom/pinctrl-msm8226.c | 17 +- drivers/pinctrl/qcom/pinctrl-msm8660.c | 17 +- drivers/pinctrl/qcom/pinctrl-msm8909.c | 17 +- drivers/pinctrl/qcom/pinctrl-msm8916.c | 17 +- drivers/pinctrl/qcom/pinctrl-msm8953.c | 17 +- drivers/pinctrl/qcom/pinctrl-msm8960.c | 17 +- drivers/pinctrl/qcom/pinctrl-msm8976.c | 17 +- drivers/pinctrl/qcom/pinctrl-msm8994.c | 17 +- drivers/pinctrl/qcom/pinctrl-msm8996.c | 17 +- drivers/pinctrl/qcom/pinctrl-msm8998.c | 21 +- drivers/pinctrl/qcom/pinctrl-msm8x74.c | 22 +- drivers/pinctrl/qcom/pinctrl-qcm2290.c | 22 +- drivers/pinctrl/qcom/pinctrl-qcs404.c | 17 +- drivers/pinctrl/qcom/pinctrl-qdf2xxx.c | 6 +- drivers/pinctrl/qcom/pinctrl-qdu1000.c | 22 +- drivers/pinctrl/qcom/pinctrl-sa8775p.c | 22 +- drivers/pinctrl/qcom/pinctrl-sc7180.c | 22 +- drivers/pinctrl/qcom/pinctrl-sc7280.c | 22 +- drivers/pinctrl/qcom/pinctrl-sc8180x.c | 22 +- drivers/pinctrl/qcom/pinctrl-sc8280xp.c | 22 +- drivers/pinctrl/qcom/pinctrl-sdm660.c | 17 +- drivers/pinctrl/qcom/pinctrl-sdm670.c | 27 +- drivers/pinctrl/qcom/pinctrl-sdm845.c | 22 +- drivers/pinctrl/qcom/pinctrl-sdx55.c | 17 +- drivers/pinctrl/qcom/pinctrl-sdx65.c | 22 +- drivers/pinctrl/qcom/pinctrl-sdx75.c | 1601 ++++++++++++++++++++ drivers/pinctrl/qcom/pinctrl-sm6115.c | 22 +- drivers/pinctrl/qcom/pinctrl-sm6125.c | 22 +- drivers/pinctrl/qcom/pinctrl-sm6350.c | 22 +- drivers/pinctrl/qcom/pinctrl-sm6375.c | 21 +- drivers/pinctrl/qcom/pinctrl-sm8150.c | 22 +- drivers/pinctrl/qcom/pinctrl-sm8250.c | 22 +- drivers/pinctrl/qcom/pinctrl-sm8350.c | 22 +- drivers/pinctrl/qcom/pinctrl-sm8450.c | 22 +- drivers/pinctrl/qcom/pinctrl-sm8550.c | 22 +- 49 files changed, 2138 insertions(+), 505 deletions(-) create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,sdx75-tlmm.yaml create mode 100644 drivers/pinctrl/qcom/pinctrl-sdx75.c