Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <20a45e1e-6e62-9940-33d8-af7bad02b68d@quicinc.com>
Date:   Wed, 3 May 2023 16:44:43 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.0
Subject: Re: [PATCH v5 2/3] pinctrl: qcom: Refactor target specific pinctrl
 driver
Content-Language: en-US
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
CC:     <agross@kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <linus.walleij@linaro.org>,
        <robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <richardcochran@gmail.com>, <manivannan.sadhasivam@linaro.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-gpio@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <netdev@vger.kernel.org>
References: <1683092380-29551-1-git-send-email-quic_rohiagar@quicinc.com>
 <1683092380-29551-3-git-send-email-quic_rohiagar@quicinc.com>
 <CAHp75VegxMgAamS3ORiJ2=D4MH7asD9PiWrM+3JAm-QOuEgcrg@mail.gmail.com>
From:   Rohit Agarwal <quic_rohiagar@quicinc.com>
In-Reply-To: <CAHp75VegxMgAamS3ORiJ2=D4MH7asD9PiWrM+3JAm-QOuEgcrg@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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.