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
