Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Daniel Scally]

Hi Hans

On 25/08/2021 15:48, Hans de Goede wrote:
> Hi all,
> 
> On 8/25/21 12:33 PM, Mark Brown wrote:
>> On Wed, Aug 25, 2021 at 12:06:18AM +0100, Daniel Scally wrote:
>>> In some situations regulator devices can be enumerated via either
>>> devicetree or ACPI and bound to regulator drivers but without any
>>> init data being provided in firmware. This leaves their consumers
>>> unable to acquire them via regulator_get().
>>>
>>> To fix the issue, add the ability to register a lookup table to a
>>> list within regulator core, which will allow board files to provide
>>> init data via matching against the regulator name and device name in
>>> the same fashion as the gpiod lookup table.
>>
>> This is the wrong level to do this I think, this is a generic problem
>> that affects all kinds of platform data so if we're not going to scatter
>> DMI quirks throughout the drivers like we currently do then we should
>> have a way for boards to just store generic platform data for a device
>> and then have that platform data joined up with the device later.  This
>> could for example also be used by all the laptop audio subsystems which
>> need DMI quirk tables in drivers for their components to figure out how
>> they're wired up and avoids the need to go through subsystems adding new
>> APIs.
> 
> Daniel, I believe that what Mark wants here is something similar to what
> we already do for the 5v boost converter regulator in the TI bq24190 charger
> chip used on some Cherry Trail devices.
> 
> There the entire i2c-client is instantiated by platform code here:
> drivers/i2c/busses/i2c-cht-wc.c
> 
> This attaches a struct bq24190_platform_data as platform data to
> the i2c-client, this struct contains a single 
> 
> const struct regulator_init_data *regulator_init_data
> 
> member which then gets consumed (if there is platform data set) by
> the regulator code in:
> 
> drivers/power/supply/bq24190_charger.c
> 
> For the tps68470 regulator code the platform_data then would need to
> have 3 const struct regulator_init_data * members one for each of the
> 3 regulators.
> 
> This platform_data could then be directly set (based on a DMI match table)
> from intel_skl_int3472_tps68470.c avoiding probe-ordering issues (1) with
> the lookup solution and will allow the code containing the DMI and
> regulator_init_data tables to be build as a module.
> 
> So all in all I think that this will be a better solution.

So assign an array of the init_data to the i2c-INT3472:nn device as
platform data before registering the MFD cells in
intel_skl_int3472_tps68470.c and parse that out when the regulators
register. OK - that's fine by me.

> 
> Regards,
> 
> Hans
> 
> 
> 1) You are forcing the DMI matching driver adding the lookups to be builtin
> but what if the tps68740-MFD + regulatorcode is also builtin, then there
> still is no guarantee the lookups will be added before the regulator drivers'
> probe function runs

Err, excellent point - I hadn't thought of that if I'm honest.

> 
> p.s.
> 
> I see that you mention something similar later in the thread referring to
> the tps65023-regulator driver. I did not check, but assuming that uses what
> I describe above; then yes the idea would be to do something similar for
> the tps68740-code, setting the platform_data when (just before) the MFD-cells
> are instantiated from intel_skl_int3472_tps68470.c

The tps65023-regulator driver parses init data out of platform data yes.
I think that that's fine, but personally I'd prefer that done in core.c
rather than in the regulator driver itself if possible.