lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20a45e1e-6e62-9940-33d8-af7bad02b68d@quicinc.com> Date: Wed, 3 May 2023 16:44:43 +0530 From: Rohit Agarwal <quic_rohiagar@...cinc.com> To: Andy Shevchenko <andy.shevchenko@...il.com> CC: <agross@...nel.org>, <andersson@...nel.org>, <konrad.dybcio@...aro.org>, <linus.walleij@...aro.org>, <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>, <richardcochran@...il.com>, <manivannan.sadhasivam@...aro.org>, <linux-arm-msm@...r.kernel.org>, <linux-gpio@...r.kernel.org>, <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org> Subject: Re: [PATCH v5 2/3] pinctrl: qcom: Refactor target specific pinctrl driver On 5/3/2023 3:11 PM, Andy Shevchenko wrote: > On Wed, May 3, 2023 at 8:39 AM Rohit Agarwal <quic_rohiagar@...cinc.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.
Powered by blists - more mailing lists