Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 05/12] regulator: Introduce tps68470-regulator driver
To:     Mark Brown <broonie@kernel.org>
Cc:     "Rafael J . Wysocki" <rjw@rjwysocki.net>,
        Mark Gross <markgross@kernel.org>,
        Andy Shevchenko <andy@infradead.org>,
        Daniel Scally <djrscally@gmail.com>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>, Len Brown <lenb@kernel.org>,
        linux-acpi@vger.kernel.org, platform-driver-x86@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Kate Hsuan <hpa@redhat.com>, linux-media@vger.kernel.org,
        linux-clk@vger.kernel.org
References: <YWQU/SYTT5Vk24XH@sirena.org.uk>
 <f6f2d7e8-fdb8-ed64-0cdd-65aded9fc42c@redhat.com>
 <YWmwZJvDYjPWJdb4@sirena.org.uk>
 <d0d1dc05-4cc6-2f47-88a9-700cfc356d86@redhat.com>
 <YWnPaI/ZECdfYre9@sirena.org.uk>
 <843f939a-7e43-bc12-e9fc-582e01129b63@redhat.com>
 <YWnZIZTPiuAIazV+@sirena.org.uk>
 <c595b143-d7ed-e76b-7734-e03d14e0f76e@redhat.com>
 <YWndpGgBtLEAEaNj@sirena.org.uk>
 <1805d16e-87ab-c291-6a73-adabf41344ca@redhat.com>
 <YWoAtCdikYfMpUnD@sirena.org.uk>
From:   Hans de Goede <hdegoede@redhat.com>
Message-ID: <26a1b636-1b82-2ca6-4f78-b1d22fa556d6@redhat.com>
Date:   Sat, 16 Oct 2021 12:18:17 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <YWoAtCdikYfMpUnD@sirena.org.uk>
Content-Type: text/plain; charset=windows-1252
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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