Hi Mark, On 10/16/21 12:29 AM, Mark Brown wrote: > On Fri, Oct 15, 2021 at 10:14:30PM +0200, Hans de Goede wrote: >> On 10/15/21 9:59 PM, Mark Brown wrote: <snip> >> 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. Ah, ok. The default regulator_init_data in the tps68470_init[] array was already there in the out of tree version of this driver Daniel and I started with: https://github.com/intel/linux-intel-lts/blob/4.14/base/drivers/regulator/tps68470-regulator.c Now that you have pointed this out I would be more then happy to delete it and I agree that providing this bogus data is not a good idea. <snip> > 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. Ok, for the next version of this patch-set I will: 1. Remove the default init_data 2. Change the toplevel comment to be all C++ style matching the SPDX line 3. Add a || COMPILE_TEST to the Kconfig so that this can be compile-tested without selecting the INTEL_SKL_INT3472 option Thank you for taking the time to dive a bit deeper into the patch-set and make it clear that your objection is the presence of the default regulator_init_data; and sorry for loosing my cool a bit in my previous email. Regards, Hans
