Hi Mark On 13/07/2021 16:24, Mark Brown wrote: > On Tue, Jul 13, 2021 at 12:32:26AM +0100, Daniel Scally wrote: > >> I do think it can simplify driver code too; a lot of them aren't written >> to parse platform data to get the init data, as they're just relying on >> reading it from devicetree so in the event that we get more cases like >> this, we need to modify those drivers to look for platform data too. On >> the other hand, even the drivers that don't directly call >> of_get_regulator_init_data() still do that lookup during the >> regulator_of_get_init_data() call in regulator_register(), so the ones >> that do parse platform data for init_data structs will check DT as part >> of regulator_register() anyway. Imitating that seems simpler to me. > The driver code is trivial boilerplate, assuming someone doesn't go and > implement a helper to register stuff separately like I suggested. The > proposed swnode stuff would involve duplicating the DT parsing code. > This seems like a whole lot of effort for something that provides a > worse result than either of the existing things. Alright - let me look at adding a helper to register them instead then. >> It also creates some problems to suppress the enumeration of the i2c >> device via ACPI (which we'll have to do in a machine specific fashion, >> because some laptops have this chip with properly configured ACPI and > To be clear I think that's a terrible idea. Me too. I thought you were suggesting that I do that - sorry to have misunderstood there. The problem we're trying to resolve here is kinda exacerbated by a lot non-standard stuff within the ACPI for these devices, so for example, on top of not having any power management AML (which is what I'm trying to fix up here), the ACPI device can actually represent a bunch of different things that might be a TPS68470 PMIC, a different PMIC entirely or even not a physical PMIC at all, but rather just a convenient dummy device to collect a bunch of GPIOs under. Which one it is is revealed by parsing a buffer out of the device's ACPI, so we need the ACPI enumeration to be able to use that properly. Part of the reasons I went with addressing this with software nodes is that we've used them to correct other deficiencies in the ACPI on the same devices, like the references between the cameras and the image signal processor ought to be described in _DSD packages [1], but are again just hidden inside a buffer that we need to parse to figure out the right way to make the connection, and then we used software nodes to represent that. The difference there is that that's implementing something that should have been there in the first place, whereas the regulators wouldn't ordinarily be described in this way. >>> down to being another data table, I imagine you could write a helper for >>> it, or probably even come up with some generic thing that let you >>> register a platform data/DMI combo independently of the driver to get it >>> out of the driver code (looking more like the existing GPIO code which >>> is already being used in another bit of this quirking). >> The advantage of the GPIO lookups is there's no need to have the pointer >> to the registered devices to register the lookup table; we could imitate >> that, by adding entries to a list with the lookup values being device >> and regulator name (with the init data as the thing that's "looked up") >> and check for those during regulator_register() maybe? > Like I keep saying I think that's a much better approach than trying to > use swnodes, they just seem like a terrible fit for the problem. Okedokey; I'm happy to take another look at it from this angle then - thanks for the feedback. [1] https://www.kernel.org/doc/html/latest/firmware-guide/acpi/dsd/graph.html
