Message-ID: <c95da883-581b-d1f4-4c8a-2162b8b58b64@gmail.com>
Date:   Sat, 10 Jul 2021 23:48:33 +0100
From:   Daniel Scally <djrscally@...il.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        hdegoede@...hat.com, mgross@...ux.intel.com,
        luzmaximilian@...il.com, lgirdwood@...il.com,
        andy.shevchenko@...il.com, laurent.pinchart@...asonboard.com,
        kieran.bingham@...asonboard.com
Subject: Re: [RFC PATCH 0/2] Add software node support to regulator framework

Hi Mark, thanks for the feedback - much appreciated.

On 09/07/2021 18:04, Mark Brown wrote:
> On Thu, Jul 08, 2021 at 11:42:24PM +0100, Daniel Scally wrote:
>
>> See previous series for some background context [1]
> That's a link to a series "[PATCH v5 0/6] Introduce intel_skl_int3472
> module" which doesn't have any explanatory text as to what it's doing in
> the cover letter (just an inter version changelog) nor any obvious
> relevance to this series, are you sure that's the right link?  In
> general it's best if your patch series contains enough explanatory
> information to allow someone to have a reasonable idea what the series
> does without having to follow links like this.


It is, but I certainly could have done a better job of explaining
context there rather than throwing it in (particularly given the
uselessness of the cover letter there, I forgot about that - sorry).
That series contains an i2c driver that binds to the TPS68470 when
enumerated via ACPI. That i2c driver is effectively an MFD driver, it
registers a regmap plus platform devices for the GPIO, clk, regulator
and OpRegion parts of the TPS68470, each of which has their own platform
driver to control them (though only the GPIO and OpRegion ones are
currently upstream - I need to post the clk and regulator ones), all
sharing the same regmap. This series is a follow on from that,
attempting to remedy problems with the ACPI that prevent those resources
from being discoverable by consumers on some devices.

>> This series is a prototype of an emulation of the device tree regulator
>> initialisation and lookup functions, using software nodes. Software nodes
> What is a software node and why would we want to use one here?


Like of_nodes, but defined entirely within kernel code rather than
parsed out of devicetree or from ACPI. I think we need to use them to
make up for the deficiencies in ACPI on these platforms.

>> 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. 
> So these systems lack an enumerable description of the system provided
> by hardware or firmware (or given that these are ACPI systems I guess
> the firmware description is just broken) so we need to use board files.
> Why are we not just using board files, what does this new abstraction
> solve?


I went with this approach because the ACPI isn't entirely lacking, it
enumerates the TPS68470 as an i2c device for its driver to bind to
without a problem which results in the regulator driver registering the
regulator devices (7 of them for this chip), so I was thinking along the
lines of repairing the problems with ACPI to give those rdevs the right
init_data rather than sidestepping it altogether. I could register the
platform devices for the regulator driver to bind to in a board file
instead if that's the preferred option...usually this would involve
using i2c_board_info I think but as ACPI will enumerate the i2c device
for the chip independently we'd need to handle that somehow to stop them
racing each other I guess.


I'll take a look and see if I can make it work that way.


>> 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
> I don't think I understand what the problem you are trying to solve is
> so it's hard to say if this is a good approach to solving it.


Hope this is a bit clearer now?

Powered by blists - more mailing lists