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

From: Lee Jones <lee.jones@linaro.org>
To: Marek Vasut <marek.vasut@gmail.com>
Cc: linux-kernel@vger.kernel.org,
	Marek Vasut <marek.vasut+renesas@gmail.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Mark Brown <broonie@kernel.org>,
	Steve Twiss <stwiss.opensource@diasemi.com>,
	Wolfram Sang <wsa+renesas@sang-engineering.com>,
	linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH v3 08/10] mfd: da9063: Register RTC only on DA9063L
Date: Thu, 7 Jun 2018 06:04:11 +0100	[thread overview]
Message-ID: <20180607050411.GB21163@dell> (raw)
In-Reply-To: <69893894-899f-54cd-2610-96653ec246c0@gmail.com>

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.

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.

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