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

[Hans de Goede]

Hi,

On 10/15/21 9:59 PM, Mark Brown wrote:
> On Fri, Oct 15, 2021 at 09:48:24PM +0200, Hans de Goede wrote:
>> On 10/15/21 9:40 PM, Mark Brown wrote:
> 
>>> I can't see how the quirking gets propagated through into the driver and
>>> I'd really expect that in a situation like this the platform data would
>>> be passed through as platform data from the code doing the quirks,
> 
>> That is exactly what is happening here. The platform_data in this
>> case is just an array of regulator_init_data pointers (one per
>> regulator in the PMIC):
> 
> 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
> 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.

Daniel Scally tried to solve this by allowing specifying this
in software-firmware-nodes. Which you nacked because those need
separate parsing code in the regulator core.

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 ?

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!

>> So we have the code doing the quirks determining the regulator_init_data
>> and passing this through platform_data, which AFAICT is exactly what
>> you want?
> 
> No.  There should be no sign of the platform data getting allocated or
> initialised in the driver consuming the platform data.  It should purely
> be reading the data it gets passed by the platform initialisation code.
> 
> Please make the use of platform data look like normal platform data use
> rather than going and inventing some new scheme.

I'm sorry but I no longer have any clue what it is what you want us to do /
how you want us to solve this.

AFAIK what the current code is doing is exactly what you requested during
the discussion with Daniel.

If this is not to your looking please provide some pseudo code explaining
how you want us to solve this problem instead of the current solution.

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?

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!

Regards,

Hans