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
