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

[Lee Jones <lee.jones@linaro.org>]

On Thu, 07 Jun 2018, Marek Vasut wrote:

> 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.

Hmm ... yes, that's the same! :D

> > 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.

It is consistent.

 - Multi-line entries go on multiple lines.
 - Single line entries go on single lines.

See drivers/mfd/max77620.c for how it should look.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog