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

[Lee Jones]

On Tue, 05 Jun 2018, Marek Vasut wrote:

> On 06/05/2018 09:53 AM, Lee Jones wrote:
> > On Sat, 02 Jun 2018, Marek Vasut wrote:
> > 
> >> The DA9063L does not contain RTC block, unlike the full DA9063.
> >> Split the RTC block into separate mfd cell and register it only
> >> on DA9063.
> >>
> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxx>
> >> Cc: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> >> Cc: Lee Jones <lee.jones@xxxxxxxxxx>
> >> Cc: Mark Brown <broonie@xxxxxxxxxx>
> >> Cc: Steve Twiss <stwiss.opensource@xxxxxxxxxxx>
> >> Cc: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> >> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx
> >> ---
> >> V2: No change
> >> V3: Rework of mfd: da9063: Disallow RTC on DA9063L
> >> ---
> >>  drivers/mfd/da9063-core.c | 30 +++++++++++++++++++++++-------
> >>  1 file changed, 23 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> >> index eebca3442cf3..b05910c797af 100644
> >> --- a/drivers/mfd/da9063-core.c
> >> +++ b/drivers/mfd/da9063-core.c
> >> @@ -76,7 +76,7 @@ static struct resource da9063_hwmon_resources[] = {
> >>  };
> >>  
> >>  
> >> -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().  Due to the current obfuscation, I currently have no idea
what this device's name is.  This technique is not allowed for new
drivers - unfortunately I didn't not review this driver in the first
instance.

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!) :)

[...]

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

-- 
Lee Jones [æçæ]
Linaro Services Technical Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog