Date:   Sat, 16 Oct 2021 12:18:17 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Mark Gross <markgross@...nel.org>,
        Andy Shevchenko <andy@...radead.org>,
        Daniel Scally <djrscally@...il.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, Len Brown <lenb@...nel.org>,
        linux-acpi@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Kate Hsuan <hpa@...hat.com>, linux-media@...r.kernel.org,
        linux-clk@...r.kernel.org
Subject: Re: [PATCH 05/12] regulator: Introduce tps68470-regulator driver

Hi Mark,

On 10/16/21 12:29 AM, Mark Brown wrote:
> On Fri, Oct 15, 2021 at 10:14:30PM +0200, Hans de Goede wrote:
>> On 10/15/21 9:59 PM, Mark Brown wrote:

<snip>

>> During that discussion you said that instead we should sinmply
>> directly pass the regulator_init_data, rather then first
>> encoding this in device-properties in a swnode and then
>> decoding those again in the regulator core.
> 
>> And passing the regulator_init_data is exactly what we are doing
>> now; and now again this is not what we should be doing ?
> 
> No, it is not what the driver doing now.  The driver will *optionally*
> check for platform data, but if none is provided or if it doesn't
> configure some of the regulators then the driver will provide some hard
> coded regulator_init_data as a default.  These might be OK on the system
> you're looking at but will also be used on any other system that happens
> to instantiate the driver without platform data where there's no
> guarantee that the information provided will be safe.  These defaults
> that are being provided may use the same structure that gets used for
> platform data but they aren't really platform data.
> 
> Yes, someone could use this on a system that does things in the standard
> fashion where the platform data is getting passed in but if it's ever
> run on any other system then it's going to assume this default platform
> data with these constraints that have been embedded directly into the
> driver without anything to ensure that they make sense on that system.
> 
> Indeed, now I go back and dig out the rest of the series it seems that
> there's some drivers/platforms/x86 code added later which does in fact
> do the right thing for some but not all of the regulators, it supplies
> some platform data which overrides some but not all of this default
> regulator_init_data using platform_data having identified the system
> using DMI information (with configurations quite different to and much
> more restricted than the defaults provided, exactly why defaults are an
> issue).  I'm now even more confused about what the information that's
> there is doing in the driver.  Either it's all unneeded even for your
> system and should just be deleted, or if any of it is needed then it
> should be moved to being initialised in the same place everything else
> is so that it's only used on your system and not on any other system
> that happens to end up running the driver.
> 
> In any case given that your platform does actually have code for
> identifying it and supplying appropriate platform data the driver itself
> can be fixed by just deleting the else case of
> 
> 	if (pdata && pdata->reg_init_data[i])
> 		config.init_data = pdata->reg_init_data[i];
> 	else
> 		config.init_data = &tps68470_init[i];
> 
> and the data structure/macro it uses.  If no configuration is provided
> by the platform then none should be provided to the core, this in turn
> means that the regulator framework won't reconfigure the hardware if it
> doesn't know it's safe to do so.

Ah, ok. The default regulator_init_data in the tps68470_init[]
array was already there in the out of tree version of this driver
Daniel and I started with:

https://github.com/intel/linux-intel-lts/blob/4.14/base/drivers/regulator/tps68470-regulator.c

Now that you have pointed this out I would be more then happy to
delete it and I agree that providing this bogus data is not a
good idea.

<snip>

> The important thing is to get rid of the hard coded defaults for the
> regulator_init_data in the driver itself, if there is regulator_init_data
> in the driver itself then it should be guarded with a DMI or similar
> quirk.  Like I say above I think now I've gone back and dug through the
> rest of the series once the default init_data is gone it's probably OK.

Ok, for the next version of this patch-set I will:

1. Remove the default init_data
2. Change the toplevel comment to be all C++ style matching the SPDX line
3. Add a || COMPILE_TEST to the Kconfig so that this can be compile-tested
   without selecting the INTEL_SKL_INT3472 option

Thank you for taking the time to dive a bit deeper into the patch-set
and make it clear that your objection is the presence of the default
regulator_init_data; and sorry for loosing my cool a bit in my previous
email.

Regards,

Hans

Powered by blists - more mailing lists