Re: [PATCH 05/12] regulator: Introduce tps68470-regulator driver

[Mark Brown]

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

> > No, it's not.  What normally happens is that whatever registers the
> > device will when registering the device supply platform data that the
> > device later picks up from the struct device during probe.  What you're
> > saying is that the idea here is that driver unconditionally declares
> > platform data and then other code scribbles over that before the driver
> > instantiates.  This is cleaner in that it keeps the platform

Actually, correction - there's no export of tps68470_init[] so I guess
that's just the data that gets used :/

> > configuration together and safer since the device can't exist before
> > it's configuration is provided.

> Right, this is the standard device-tree model. Unfortunately the
> information about which supplies are needed and the constraints
> for those supplies is missing from the ACPI description for the
> devices which we are dealing with here.

That's not just the standard device tree model, that's how systems with
board files work too.

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

> Also note that the current solution is exactly what I suggested
> we should do during the discussion with Daniel and I even provided
> example code and you said absolutely nothing about this!

I had been under the impression that the platform data would look like
normal platform data and come along with the device registration,
providing default regulator_init_data hadn't really occurred to me.

> And please keep in mind that we *cannot* change the ACPI firmware interfaces
> and that whether we like it or not things simply work different in the ACPI
> world.

> Frankly I'm quit unhappy, angry even about how you are blocking progress
> here. You don't like APCI we get it, can we get over that now please?

ACPI is fine, we have a bunch of perfectly good ways to handle things
that need quirking on it safely - both platform_data and DMI quirks can
and do work well here.  The issue is that we should be using those
things rather than inventing new things unless those new things solve a
problem.

> So far all you seem to be doing is shooting down solutions, then first
> being quiet about suggested alternative solutions and then once the
> alternative solutions are implemented shoot them down again...

> And all that without adding anything constructive. All you have
> told us is how things should not be done (according to you).

> So fine everything we come up is wrong, please tell us how we should
> solve this instead then!

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.

Attachment: signature.asc
Description: PGP signature