Date:   Mon, 26 Feb 2018 17:37:18 +0000
From:   Richard Fitzgerald <rf@...nsource.cirrus.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Lee Jones <lee.jones@...aro.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        <patches@...nsource.cirrus.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v8 3/9] mfd: madera: Add common support for Cirrus Logic
 Madera codecs

On 26/02/18 17:19, Andy Shevchenko wrote:
> On Mon, Feb 26, 2018 at 7:06 PM, Richard Fitzgerald
> <rf@...nsource.cirrus.com> wrote:
>> On 26/02/18 14:11, Andy Shevchenko wrote:
>>> On Mon, Feb 26, 2018 at 3:05 PM, Richard Fitzgerald
>>> <rf@...nsource.cirrus.com> wrote:
> 
>>>> +static void madera_enable_hard_reset(struct madera *madera)
>>>> +{
>>>> +       if (madera->reset_gpio)
>>>
>>>
>>> if (!...)
>>>    return
>>>
>>
>> Could do but why bother? For such a trivial function, in my opinion
>>
>> static void madera_enable_hard_reset(struct madera *madera)
>> {
>>          if (madera->reset_gpio)
>>                  gpiod_set_value_cansleep(madera->reset_gpio, 0);
>> }
>>
>> is simpler and more readable than
>>
>> static void madera_enable_hard_reset(struct madera *madera)
>> {
>>          if (!madera->reset_gpio)
>>                  return;
>>
>>          gpiod_set_value_cansleep(madera->reset_gpio, 0);
>> }
> 
> The rationale is that if someone wants to add more code you will not
> need to take care of deeper indentation and potentially split lines.
> 

Yes, true. It's probably unlikely here and I'm inclined to leave it
as it is because Lee already acked it.


>>
>>>> +               gpiod_set_value_cansleep(madera->reset_gpio, 0);
>>>> +}
>>>> +
>>>> +static void madera_disable_hard_reset(struct madera *madera)
>>>> +{
>>>> +       if (madera->reset_gpio) {
>>>
>>>
>>> Ditto.
>>>
>>
>> As above, yes it would work the other way but I think for such a simple
>> implementation the way I have written it is more readable.
> 
> I have different opinion, but yes. It's more matter of taste with
> rationale above (perhaps never happen to this code).
> 
>>>> +               gpiod_set_value_cansleep(madera->reset_gpio, 1);
>>>> +               usleep_range(1000, 2000);
>>>> +       }
>>>> +}
> 
>>>> +const struct of_device_id madera_of_match[] = {
>>>> +       { .compatible = "cirrus,cs47l35", .data = (void *)CS47L35 },
>>>> +       { .compatible = "cirrus,cs47l85", .data = (void *)CS47L85 },
>>>> +       { .compatible = "cirrus,cs47l90", .data = (void *)CS47L90 },
>>>> +       { .compatible = "cirrus,cs47l91", .data = (void *)CS47L91 },
>>>> +       { .compatible = "cirrus,wm1840", .data = (void *)WM1840 },
>>>
>>>
>>>> +       {},
>>>
>>>
>>> No comma.
>>>
>>
>> Seems to be personal preference. Both ways are used in the kernel and
>> we've always used this style so I'll leave it to Lee to decide.
> 
> This is not.
> The rationale is pretty obvious, terminator must terminate. With cheap
> price (no comma), we just prevent some potential weird cases (bad
> patch application for example, or not very careful contributor) where
> entry goes after. Compiler will fail.
> 

Yes, ok. I see a lot of people don't do that (I searched). But if I
do a new version of this patch I'll change it.

>>
>>>> +};
> 
>>>> +               ret = devm_gpio_request_one(madera->dev,
>>>> +                                           madera->pdata.reset,
>>>> +                                           GPIOF_DIR_OUT |
>>>> GPIOF_INIT_LOW,
>>>> +                                           "madera reset");
>>>> +               if (!ret)
>>>> +                       madera->reset_gpio =
>>>> gpio_to_desc(madera->pdata.reset);
>>>
>>>
>>> Why? What's wrong with descriptors?
> 
>> This is what I mean by code going stale when it's acked but then never
>> gets merged. Some time ago there was a reason (which I forget).
> 
> So, can we switch to descriptors?
> 

Yes, however as this patch has been in review for nearly 1 year now, and
acked for several months, I'd really hoped we could get it merged now
and update it later.

>>>> +       dev_set_drvdata(madera->dev, madera);
>>>> +       if (dev_get_platdata(madera->dev))
>>>
>>>
>>> What this dance for?
>>>
>>
>> Are you perhaps thinking the second line is dev_get_drvdata()?
>> dev_get_platdata() gets a pointer to any pdata, so not related
>> to dev_set_drvdata().
> 
> Indeed.
> 
>>>> +       ret = mfd_add_devices(madera->dev, PLATFORM_DEVID_NONE,
>>>> +                             mfd_devs, n_devs,
>>>> +                             NULL, 0, NULL);
>>>
>>>
>>> devm_?
>>>
>>
>> I can try it and see. It's scary because we can depend on our
>> children but maybe devm_mfd_add_devices() is safe.
> 
> It will fail in the same way. It does nothing more, than keeping a
> pointer to release function and its data.
> 
>>>> +struct madera_irqchip_pdata;
>>>> +struct madera_codec_pdata;
> 
>>> Why do you need platform data in new code?
> 
>> Answered in a comment in another patch. We care about allowing people
>> to use our chips with systems that don't use devicetree/acpi. There
>> are also many out-of-tree systems.
> 
> a) we don't care about out of tree much;

You might not, but as a commercial company we have to.

> b) there are other means to provide date w/o using platform data:
>   - unified device property API (including built-in device properties)
>   - bunch of lookup tables GPIO, regulator, PWM, etc
>   - fwnode graph for more complex cases with device dependencies
> 

Basically same answer as (a)

Powered by blists - more mailing lists