lkml.org 
[lkml]   [2018]   [Feb]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v8 3/9] mfd: madera: Add common support for Cirrus Logic Madera codecs
From
Date
On 26/02/18 17:19, Andy Shevchenko wrote:
> On Mon, Feb 26, 2018 at 7:06 PM, Richard Fitzgerald
> <rf@opensource.cirrus.com> wrote:
>> On 26/02/18 14:11, Andy Shevchenko wrote:
>>> On Mon, Feb 26, 2018 at 3:05 PM, Richard Fitzgerald
>>> <rf@opensource.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)

\
 
 \ /
  Last update: 2018-02-26 18:37    [W:0.409 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site