From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Daniel Scally <djrscally@gmail.com>
Cc: linux-kernel@vger.kernel.org,
platform-driver-x86@vger.kernel.org, hdegoede@redhat.com,
mgross@linux.intel.com, luzmaximilian@gmail.com,
lgirdwood@gmail.com, broonie@kernel.org,
andy.shevchenko@gmail.com, kieran.bingham@ideasonboard.com
Subject: Re: [RFC PATCH 0/2] Add software node support to regulator framework
Date: Mon, 12 Jul 2021 14:50:40 +0300 [thread overview]
Message-ID: <YOwskGqqOZnwZWy5@pendragon.ideasonboard.com> (raw)
In-Reply-To: <1944291d-1486-fe7f-376b-fe3250ee6b7d@gmail.com>
Hi Dan,
On Mon, Jul 12, 2021 at 09:13:00AM +0100, Daniel Scally wrote:
> On 11/07/2021 17:55, Laurent Pinchart wrote:
> > On Sat, Jul 10, 2021 at 11:54:26PM +0100, Daniel Scally wrote:
> >> On 10/07/2021 23:28, Laurent Pinchart wrote:
> >>> 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.
> >
> > If technically feasible, gathering all the data in a single place would
> > be my preference. Whether that should take the form of software nodes in
> > all cases, or be modelled as custom data that the int3472 driver would
> > interpret to create the regulators and clocks is a different (but
> > related) question.
>
> I'll have to think on that one then; the problem there is that the
> cio2-bridge is just given ACPI HIDs for the sensors as "ok to parse
> this", and of course the INT347A that is being dealt with here should
> already be supported on most Surface platforms via the intel-skl-int3472
> stuff, so once the ov8865 edits are (posted and) accepted and that
> driver is supported my plan would be to add it into the bridge. So we'd
> need a way to exclude Go2 from that if we wanted to define all the
> software nodes parts in a single board file instead.
>
> > The very important part is to ensure that the correct board data will be
> > used, as otherwise we could damage the hardware.
>
> Not sure how this is usually guarded against; we could do a DMI match at
> the start of the init function to confirm it's running on a Go2 and exit
> if not?
Unless the information is available in ACPI properties that the CIO2
bridge driver can access (and I wouldn't be surprised if it was the case
in some way, there's are different IDs that we're not sure how to use,
the Windows driver may very well map them to a set of reference
designs), then a DMI match is likely the best option.
> >>>> [1] https://lore.kernel.org/lkml/20210603224007.120560-1-djrscally@gmail.com/
> >>>> [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
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2021-07-12 11:51 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-08 22:42 [RFC PATCH 0/2] Add software node support to regulator framework Daniel Scally
2021-07-08 22:42 ` [RFC PATCH 1/2] regulator: Add support for software node connections Daniel Scally
2021-07-09 17:26 ` Mark Brown
2021-07-08 22:42 ` [RFC PATCH 2/2] platform/surface: Add Surface Go 2 board file Daniel Scally
2021-07-09 17:40 ` Mark Brown
2021-07-09 17:04 ` [RFC PATCH 0/2] Add software node support to regulator framework Mark Brown
2021-07-10 22:48 ` Daniel Scally
2021-07-12 14:15 ` Mark Brown
2021-07-12 16:55 ` Laurent Pinchart
2021-07-12 17:32 ` Mark Brown
2021-07-11 9:37 ` Andy Shevchenko
2021-07-12 12:42 ` Mark Brown
2021-07-12 13:01 ` Andy Shevchenko
2021-07-12 13:34 ` Mark Brown
2021-07-12 16:08 ` Andy Shevchenko
2021-07-12 17:01 ` Mark Brown
2021-07-12 23:32 ` Daniel Scally
2021-07-13 15:24 ` Mark Brown
2021-07-13 15:42 ` Laurent Pinchart
2021-07-13 16:02 ` Mark Brown
2021-07-13 16:06 ` Laurent Pinchart
2021-07-13 18:24 ` Mark Brown
2021-07-13 15:55 ` Andy Shevchenko
2021-07-13 18:18 ` Mark Brown
2021-07-13 19:46 ` Andy Shevchenko
2021-07-14 16:05 ` Mark Brown
2021-07-14 7:25 ` Laurent Pinchart
2021-07-14 16:59 ` Mark Brown
2021-07-14 17:18 ` Laurent Pinchart
2021-07-14 17:28 ` Mark Brown
2021-07-14 17:41 ` Laurent Pinchart
2021-07-14 19:18 ` Mark Brown
2021-07-14 21:53 ` Laurent Pinchart
2021-07-13 22:06 ` Daniel Scally
2021-07-10 22:28 ` Laurent Pinchart
2021-07-10 22:54 ` Daniel Scally
2021-07-11 16:55 ` Laurent Pinchart
2021-07-12 8:13 ` Daniel Scally
2021-07-12 11:50 ` Laurent Pinchart [this message]
2021-07-12 13:23 ` Mark Brown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YOwskGqqOZnwZWy5@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=andy.shevchenko@gmail.com \
--cc=broonie@kernel.org \
--cc=djrscally@gmail.com \
--cc=hdegoede@redhat.com \
--cc=kieran.bingham@ideasonboard.com \
--cc=lgirdwood@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=luzmaximilian@gmail.com \
--cc=mgross@linux.intel.com \
--cc=platform-driver-x86@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).