On Fri, Oct 15, 2021 at 10:14:30PM +0200, Hans de Goede wrote: > On 10/15/21 9:59 PM, Mark Brown wrote: > > No, it's not. What normally happens is that whatever registers the > > device will when registering the device supply platform data that the > > device later picks up from the struct device during probe. What you're > > saying is that the idea here is that driver unconditionally declares > > platform data and then other code scribbles over that before the driver > > instantiates. This is cleaner in that it keeps the platform Actually, correction - there's no export of tps68470_init[] so I guess that's just the data that gets used :/ > > configuration together and safer since the device can't exist before > > it's configuration is provided. > Right, this is the standard device-tree model. Unfortunately the > information about which supplies are needed and the constraints > for those supplies is missing from the ACPI description for the > devices which we are dealing with here. That's not just the standard device tree model, that's how systems with board files work too. > During that discussion you said that instead we should sinmply > directly pass the regulator_init_data, rather then first > encoding this in device-properties in a swnode and then > decoding those again in the regulator core. > And passing the regulator_init_data is exactly what we are doing > now; and now again this is not what we should be doing ? No, it is not what the driver doing now. The driver will *optionally* check for platform data, but if none is provided or if it doesn't configure some of the regulators then the driver will provide some hard coded regulator_init_data as a default. These might be OK on the system you're looking at but will also be used on any other system that happens to instantiate the driver without platform data where there's no guarantee that the information provided will be safe. These defaults that are being provided may use the same structure that gets used for platform data but they aren't really platform data. Yes, someone could use this on a system that does things in the standard fashion where the platform data is getting passed in but if it's ever run on any other system then it's going to assume this default platform data with these constraints that have been embedded directly into the driver without anything to ensure that they make sense on that system. Indeed, now I go back and dig out the rest of the series it seems that there's some drivers/platforms/x86 code added later which does in fact do the right thing for some but not all of the regulators, it supplies some platform data which overrides some but not all of this default regulator_init_data using platform_data having identified the system using DMI information (with configurations quite different to and much more restricted than the defaults provided, exactly why defaults are an issue). I'm now even more confused about what the information that's there is doing in the driver. Either it's all unneeded even for your system and should just be deleted, or if any of it is needed then it should be moved to being initialised in the same place everything else is so that it's only used on your system and not on any other system that happens to end up running the driver. In any case given that your platform does actually have code for identifying it and supplying appropriate platform data the driver itself can be fixed by just deleting the else case of if (pdata && pdata->reg_init_data[i]) config.init_data = pdata->reg_init_data[i]; else config.init_data = &tps68470_init[i]; and the data structure/macro it uses. If no configuration is provided by the platform then none should be provided to the core, this in turn means that the regulator framework won't reconfigure the hardware if it doesn't know it's safe to do so. > Also note that the current solution is exactly what I suggested > we should do during the discussion with Daniel and I even provided > example code and you said absolutely nothing about this! I had been under the impression that the platform data would look like normal platform data and come along with the device registration, providing default regulator_init_data hadn't really occurred to me. > And please keep in mind that we *cannot* change the ACPI firmware interfaces > and that whether we like it or not things simply work different in the ACPI > world. > Frankly I'm quit unhappy, angry even about how you are blocking progress > here. You don't like APCI we get it, can we get over that now please? ACPI is fine, we have a bunch of perfectly good ways to handle things that need quirking on it safely - both platform_data and DMI quirks can and do work well here. The issue is that we should be using those things rather than inventing new things unless those new things solve a problem. > So far all you seem to be doing is shooting down solutions, then first > being quiet about suggested alternative solutions and then once the > alternative solutions are implemented shoot them down again... > And all that without adding anything constructive. All you have > told us is how things should not be done (according to you). > So fine everything we come up is wrong, please tell us how we should > solve this instead then! The important thing is to get rid of the hard coded defaults for the regulator_init_data in the driver itself, if there is regulator_init_data in the driver itself then it should be guarded with a DMI or similar quirk. Like I say above I think now I've gone back and dug through the rest of the series once the default init_data is gone it's probably OK.
Attachment:
signature.asc
Description: PGP signature
