lkml.org 
[lkml]   [2018]   [Jun]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 08/10] mfd: da9063: Register RTC only on DA9063L
From
Date
On 06/06/2018 08:16 AM, Lee Jones wrote:
> On Tue, 05 Jun 2018, Marek Vasut wrote:

[...]

>>>> -static const struct mfd_cell da9063_devs[] = {
>>>> +static const struct mfd_cell da9063_common_devs[] = {
>>>> {
>>>> .name = DA9063_DRVNAME_REGULATORS,
>>>
>>> Appreciate that these are historical, but these device name defines
>>> make me shudder. They only serve to act as an obfuscation layer when
>>> debugging at platform level. Please consider getting rid of them.
>>
>> The macro can be shared between the core and the drivers, so the names
>> never run out of sync.
>
> Platform driver name changes are vary rare. Even if they are changed,
> even light testing would uncover the fact that child drivers do not
> .probe().

Sure, while if the macro is used, this problem is avoided altogether.

> Due to the current obfuscation, I currently have no idea
> what this device's name is.

I'm sure ctags or git grep can easily help.

> This technique is not allowed for new
> drivers - unfortunately I didn't not review this driver in the first
> instance.

Why not ? This looks like a step back to me.

> It doesn't bother me enough to go and change it myself and I'm not
> going to have a baby over patches not being submitted to fix it.
>
>>>> .num_resources = ARRAY_SIZE(da9063_regulators_resources),
>>>> @@ -100,15 +100,19 @@ static const struct mfd_cell da9063_devs[] = {
>>>> .resources = da9063_onkey_resources,
>>>> .of_compatible = "dlg,da9063-onkey",
>>>> },
>>>> + {
>>>> + .name = DA9063_DRVNAME_VIBRATION,
>>>> + },
>>>
>>> Place this on a single line please.
>>
>> This would only make the style inconsistent with the ie. LEDs entry.
>>
>>> { .name = DA9063_DRVNAME_VIBRATION },
>
> If that is a one line entry spaced over multiple lines, then that
> should also be changed.
>
> Maybe I will go through and stylise this driver a bit after all (but
> as time is short at the moment, maybe not!) :)

You'd end up with two entries which look different then the rest, which
triggers my OCD.

> [...]
>
>>>> +err_mfd_cleanup:
>>>> + mfd_remove_devices(da9063->dev);
>>>
>>> Any reason why you can't use devm_*?
>>
>> Because we need to undo the MFD setup before the IRQ setup.
>
> Sounds like a good enough reason.

Or the da9063_irq_init() could use devm_regmap_add_irq_chip().

--
Best regards,
Marek Vasut

\
 
 \ /
  Last update: 2018-06-06 11:52    [W:0.046 / U:20.160 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site