Re: [PATCH v3 08/10] mfd: da9063: Register RTC only on DA9063L

[Marek Vasut]

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