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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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.