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

[Marek Vasut]

On 06/07/2018 07:04 AM, Lee Jones wrote:
> On Wed, 06 Jun 2018, Marek Vasut wrote:
>> 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.
> 
> I'm aware how to get around the 'issue', but it's an additional step
> which is avoidable.  For me personally it comes from doing *a lot* of
> platform level work and being irritated by the extra grepping.  Macros
> for driver names does not sit right with me at all.  There are even
> worse examples of people defining the MACROs *inside* the driver,
> which doesn't even benefit from the small redeeming feature you
> mention above.

If we follow this line of thinking, we could just run cpp and expand all
macros. Then there's no need for grepping at all. That probably won't be
the result anyone would like though.

> Anyway, I'm happy with you not wanting to change it.  Just leave them
> as they are for now.
> 
>>>>>> +	{
>>>>>> +		.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.
> 
> OCD or not, it's never okay to waste lines.  If ordering it not
> important (which it should not be -- it's fragile to rely on device
> ordering in MFD cells), the multi-line entries go at the top, followed
> by the single line entries.  If done right, it looks the opposite of
> bad/out of place.

My point is, the style should at least be consistent. But anyway.

-- 
Best regards,
Marek Vasut