Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Daniel Scally]

Hi Laurent

On 10/07/2021 23:28, Laurent Pinchart wrote:
> Hi Dan,
>
> On Thu, Jul 08, 2021 at 11:42:24PM +0100, Daniel Scally wrote:
>> Hello all
>>
>> See previous series for some background context [1]
>>
>> Some x86 laptops with ACPI tables designed for Windows have a TPS68470
>> PMIC providing regulators and clocks to camera modules. The DSDT tables for
>> those cameras lack any power control methods, declaring only a
>> dependency on the ACPI device representing the TPS68470. This leaves the
>> regulator framework with no means of determining appropriate voltages for the
>> regulators provided by the PMIC, or of determining which regulators relate to
>> which of the sensor's requested supplies. 
>>
>> This series is a prototype of an emulation of the device tree regulator
>> initialisation and lookup functions, using software nodes. Software nodes
>> relating to each regulator are registered as children of the TPS68470's ACPI
>> firmware node. Those regulators have properties describing their constraints
>> (for example "regulator-min-microvolt"). Similarly, software nodes are
>> registered and assigned as secondary to the Camera's firmware node - these
>> software nodes have reference properties named after the supply in the same
>> way as device tree's phandles, for example "avdd-supply", and linking to the
>> software node assigned to the appropriate regulator. We can then use those
>> constraints to specify the appropriate voltages and the references to allow the
>> camera drivers to look up the correct regulator device. 
>>
>> Although not included in this series, I would plan to use a similar method for
>> linking the clocks provided by the TPS68470 to the sensor so that it can be
>> discovered too.
>>
>> I'm posting this to see if people agree it's a good approach for tackling the 
>> problem; I may be overthinking this and there's a much easier way that I should
>> be looking at instead. It will have knock-ons in the cio2-bridge code [2], as
>> that is adding software nodes to the same sensors to connect them to the media
>> graph. Similarly, is the board file an acceptable solution, or should we just
>> define the configuration for these devices (there's three orf our laptop models
>> in scope) in int3472-tps68470 instead?
> I may have missed something, but if you load the SGo2 board file, won't
> it create the regulator software nodes if it finds an INT3472,
> regardless of whether the device is an SGo2 ? If you happen to do so on
> a machine that requires different voltages, that sounds dangerous.


Ah, yes - hadn't thought of that. If a driver registered regulators with
those names, it would try to apply those voltages during registration.
Good point.

> Given that INT3472 models the virtual "Intel Skylake and Kabylake camera
> PMIC", I think moving device-specific information to the int3472 driver
> may make sense. I'm unsure what option is best though, having all the
> data (regulators, clocks, but also data currently stored in the
> cio2-bridge driver) in a single file (or a single file per machine) is
> tempting.


It is tempting, particularly because (assuming we do end up using this
approach) setting the references to the supplies in a board file like
this complicated the cio2-bridge code quite a bit, since it then needs
to extend the properties array against an already-existing software node
rather than registering a new one. But then, I don't particularly want
to handle that aspect of the problem in two separate places.

>> [1] https://lore.kernel.org/lkml/20210603224007.120560-1-djrscally@xxxxxxxxx/
>> [2] https://elixir.bootlin.com/linux/latest/source/drivers/media/pci/intel/ipu3/cio2-bridge.c#L166
>>
>>
>> Daniel Scally (2):
>>   regulator: Add support for software node connections
>>   platform/surface: Add Surface Go 2 board file
>>
>>  MAINTAINERS                                |   6 +
>>  drivers/platform/surface/Kconfig           |  10 ++
>>  drivers/platform/surface/Makefile          |   1 +
>>  drivers/platform/surface/surface_go_2.c    | 135 +++++++++++++++++++++
>>  drivers/regulator/Kconfig                  |   6 +
>>  drivers/regulator/Makefile                 |   1 +
>>  drivers/regulator/core.c                   |  23 ++++
>>  drivers/regulator/swnode_regulator.c       | 111 +++++++++++++++++
>>  include/linux/regulator/swnode_regulator.h |  33 +++++
>>  9 files changed, 326 insertions(+)
>>  create mode 100644 drivers/platform/surface/surface_go_2.c
>>  create mode 100644 drivers/regulator/swnode_regulator.c
>>  create mode 100644 include/linux/regulator/swnode_regulator.h