Re: [PATCH v8 3/9] mfd: madera: Add common support for Cirrus Logic Madera codecs

[Andy Shevchenko]

On Mon, Feb 26, 2018 at 7:06 PM, Richard Fitzgerald
<rf@xxxxxxxxxxxxxxxxxxxxx> wrote:
> On 26/02/18 14:11, Andy Shevchenko wrote:
>> On Mon, Feb 26, 2018 at 3:05 PM, Richard Fitzgerald
>> <rf@xxxxxxxxxxxxxxxxxxxxx> 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.

>
>>> +               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.

>
>>> +};

>>> +               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?

>>> +       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;
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

-- 
With Best Regards,
Andy Shevchenko