| Message ID | 20211008162121.6628-6-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Add support for X86/ACPI camera sensor/PMIC setup with clk and regulator platform data | expand |
On Fri, Oct 08, 2021 at 06:21:14PM +0200, Hans de Goede wrote: > +++ b/drivers/regulator/tps68470-regulator.c > @@ -0,0 +1,194 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Regulator driver for TPS68470 PMIC > + * Please make the entire comment a C++ one so things look more intentional. > + > +/* > + * The ACPI tps68470 probe-ordering depends on the clk/gpio/regulator drivers > + * being registered before the MFD cells are created (the MFD driver calls > + * acpi_dev_clear_dependencies() after the cell creation). > + * subsys_initcall() ensures this when the drivers are builtin. > + */ > +static int __init tps68470_regulator_init(void) > +{ > + return platform_driver_register(&tps68470_regulator_driver); > +} > +subsys_initcall(tps68470_regulator_init); If this is actually required then the driver is broken for modular use which frankly is just generally broken. I don't understand why this driver would require this when other drivers don't, or what the actual requirement is here - what does the call do and why is the ordering important?
Hi, On 10/11/21 12:42 PM, Mark Brown wrote: > On Fri, Oct 08, 2021 at 06:21:14PM +0200, Hans de Goede wrote: > >> +++ b/drivers/regulator/tps68470-regulator.c >> @@ -0,0 +1,194 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Regulator driver for TPS68470 PMIC >> + * > > Please make the entire comment a C++ one so things look more > intentional. Ok, will do so for the next version. >> + >> +/* >> + * The ACPI tps68470 probe-ordering depends on the clk/gpio/regulator drivers >> + * being registered before the MFD cells are created (the MFD driver calls >> + * acpi_dev_clear_dependencies() after the cell creation). >> + * subsys_initcall() ensures this when the drivers are builtin. >> + */ >> +static int __init tps68470_regulator_init(void) >> +{ >> + return platform_driver_register(&tps68470_regulator_driver); >> +} >> +subsys_initcall(tps68470_regulator_init); > > If this is actually required then the driver is broken for modular use > which frankly is just generally broken. I don't understand why this > driver would require this when other drivers don't, or what the actual > requirement is here - what does the call do and why is the ordering > important? For the camera-sensor which is a consumer of this devices to be able to get the regulators (and not end up with a dummy regulator) the consumer info added through the constraints passed as platform data must be available to the regulator framework before the sensor-driver's probe() method tries to get the regulators. The ACPI fwnode describing the sensor has an ACPI _DEP dependency on the ACPI fwnode describing the PMIC. To ensure that the PMIC driver binds first patches 1 + 2 of this series make the ACPI code use this dependency to not instantiate the i2c-client for the sensor until the PMIC driver has bound. The PMIC driver is a MFD driver creating GPIO, clk and regulator MFD cells. So in order for the ACPI code delaying the instantiation to help, the regulator constraints / consumer info must be registered when the MFD driver is done binding. This means that the regulator driver for the regulator MFD cells must be registered before the platform_dev-s for the cell is instantiated, so that the driver binds immediately (during instantiation) and thus the regulator consumer info is available before the PMIC-MFD-driver's probe() method is done. The use of a subsys_initcall() here ensures that when builtin the regulator driver is registered before the PMIC-MFD-driver is registered (the PMIC driver uses a normal device_initcall). To make this work when everything is build as a module patch 12/12 adds the following to the PMIC-MFD-driver: MODULE_SOFTDEP("pre: clk-tps68470 tps68470-regulator"); This will make modprobe load the clk and regulator drivers before it loads the main/MFD tps68470 driver. I've tested this with everything built as module (the typical setup for standard x86 setups) and without the MODULE_SOFTDEP the sensor driver ends up with a dummy regulator (illustrating the problem) and with the SOFTDEP in place everything works as it should. I hope this helps explain things. Regards, Hans
On Mon, Oct 11, 2021 at 01:43:40PM +0200, Hans de Goede wrote: > To make this work when everything is build as a module patch 12/12 > adds the following to the PMIC-MFD-driver: > MODULE_SOFTDEP("pre: clk-tps68470 tps68470-regulator"); > This will make modprobe load the clk and regulator drivers > before it loads the main/MFD tps68470 driver. I feel nervous about this being reliable with all userspaces - IIRC there was an alternative implementation of the modules stuff in userspace and someone could always be doing insmod. OTOH without better in kernel dependency management and/or more standards based firmware interfaces I guess we're stuck with this.
Hi, On 10/15/21 6:46 PM, Mark Brown wrote: > On Mon, Oct 11, 2021 at 01:43:40PM +0200, Hans de Goede wrote: > >> To make this work when everything is build as a module patch 12/12 >> adds the following to the PMIC-MFD-driver: > >> MODULE_SOFTDEP("pre: clk-tps68470 tps68470-regulator"); > >> This will make modprobe load the clk and regulator drivers >> before it loads the main/MFD tps68470 driver. > > I feel nervous about this being reliable with all userspaces - IIRC > there was an alternative implementation of the modules stuff in > userspace and someone could always be doing insmod. OTOH without better > in kernel dependency management and/or more standards based firmware > interfaces I guess we're stuck with this. Right, this is all less then ideak, but I believe that this is the best we can do for now. Are you happy with the platform_data for this driver as defined in patch 4/12 ? : https://lore.kernel.org/platform-driver-x86/20211008162121.6628-1-hdegoede@redhat.com/T/#m745cc1191f531a57ae7998f5c8817ba9a46f0fed And are you ok with me doing an immutable-branch based on 5.15-rc1 with just the patch adding the platform_data in there ? The platform_data is used/shared by most patches in this series. So the idea is to have an immutable branch which can be shared/merged by all subsystems which have patches in this patch series. Regards, Hans
On Fri, Oct 15, 2021 at 08:50:13PM +0200, Hans de Goede wrote: > Are you happy with the platform_data for this driver as defined in > patch 4/12 ? : Some of the other review comments lead me to believe that you'd be sending out a new version at some point? > https://lore.kernel.org/platform-driver-x86/20211008162121.6628-1-hdegoede@redhat.com/T/#m745cc1191f531a57ae7998f5c8817ba9a46f0fed I am very confused about why it's in the driver without a DMI quirk and/or clear comments about why and saying that this is a terrible example to copy. I'd also expect to get compile test coverage for the driver.
Hi, On 10/15/21 8:58 PM, Mark Brown wrote: > On Fri, Oct 15, 2021 at 08:50:13PM +0200, Hans de Goede wrote: > >> Are you happy with the platform_data for this driver as defined in >> patch 4/12 ? : > > Some of the other review comments lead me to believe that you'd be > sending out a new version at some point? That is correct. > >> https://lore.kernel.org/platform-driver-x86/20211008162121.6628-1-hdegoede@redhat.com/T/#m745cc1191f531a57ae7998f5c8817ba9a46f0fed > > I am very confused about why it's in the driver without a DMI quirk > and/or clear comments about why and saying that this is a terrible > example to copy. The DMI quirks live in the ACPI glue code under drivers/platform/x86, that code instantiates the MFD cell and sets the platform-data as part of the cell. > I'd also expect to get compile test coverage for the driver. Ack, Stephen made the same remark for the clk driver. I'll fix this for the next version. Regards, Hans
On Fri, Oct 15, 2021 at 09:27:50PM +0200, Hans de Goede wrote: > On 10/15/21 8:58 PM, Mark Brown wrote: > > I am very confused about why it's in the driver without a DMI quirk > > and/or clear comments about why and saying that this is a terrible > > example to copy. > The DMI quirks live in the ACPI glue code under drivers/platform/x86, > that code instantiates the MFD cell and sets the platform-data > as part of the cell. I can't see how the quirking gets propagated through into the driver and I'd really expect that in a situation like this the platform data would be passed through as platform data from the code doing the quirks, this just looks wrong and it'll go badly wrong if the same part shows up in some other machine.
Hi, On 10/15/21 9:40 PM, Mark Brown wrote: > On Fri, Oct 15, 2021 at 09:27:50PM +0200, Hans de Goede wrote: >> On 10/15/21 8:58 PM, Mark Brown wrote: > >>> I am very confused about why it's in the driver without a DMI quirk >>> and/or clear comments about why and saying that this is a terrible >>> example to copy. > >> The DMI quirks live in the ACPI glue code under drivers/platform/x86, >> that code instantiates the MFD cell and sets the platform-data >> as part of the cell. > > I can't see how the quirking gets propagated through into the driver and > I'd really expect that in a situation like this the platform data would > be passed through as platform data from the code doing the quirks, That is exactly what is happening here. The platform_data in this case is just an array of regulator_init_data pointers (one per regulator in the PMIC): struct tps68470_regulator_platform_data { const struct regulator_init_data *reg_init_data[TPS68470_NUM_REGULATORS]; }; This struct gets filled by platform specific code under drivers/platform/x86 (in later patches in the series). And the regulator code in this patch consumes this like this: if (pdata && pdata->reg_init_data[i]) config.init_data = pdata->reg_init_data[i]; else config.init_data = &tps68470_init[i]; rdev = devm_regulator_register(&pdev->dev, ®ulators[i], &config); So we have the code doing the quirks determining the regulator_init_data and passing this through platform_data, which AFAICT is exactly what you want? Regards, Hans
On Fri, Oct 15, 2021 at 09:48:24PM +0200, Hans de Goede wrote: > On 10/15/21 9:40 PM, Mark Brown wrote: > > I can't see how the quirking gets propagated through into the driver and > > I'd really expect that in a situation like this the platform data would > > be passed through as platform data from the code doing the quirks, > That is exactly what is happening here. The platform_data in this > case is just an array of regulator_init_data pointers (one per > regulator in the PMIC): 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 configuration together and safer since the device can't exist before it's configuration is provided. > So we have the code doing the quirks determining the regulator_init_data > and passing this through platform_data, which AFAICT is exactly what > you want? No. There should be no sign of the platform data getting allocated or initialised in the driver consuming the platform data. It should purely be reading the data it gets passed by the platform initialisation code. Please make the use of platform data look like normal platform data use rather than going and inventing some new scheme.
Hi, On 10/15/21 9:59 PM, Mark Brown wrote: > On Fri, Oct 15, 2021 at 09:48:24PM +0200, Hans de Goede wrote: >> On 10/15/21 9:40 PM, Mark Brown wrote: > >>> I can't see how the quirking gets propagated through into the driver and >>> I'd really expect that in a situation like this the platform data would >>> be passed through as platform data from the code doing the quirks, > >> That is exactly what is happening here. The platform_data in this >> case is just an array of regulator_init_data pointers (one per >> regulator in the PMIC): > > 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 > 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. Daniel Scally tried to solve this by allowing specifying this in software-firmware-nodes. Which you nacked because those need separate parsing code in the regulator core. 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 ? 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! >> So we have the code doing the quirks determining the regulator_init_data >> and passing this through platform_data, which AFAICT is exactly what >> you want? > > No. There should be no sign of the platform data getting allocated or > initialised in the driver consuming the platform data. It should purely > be reading the data it gets passed by the platform initialisation code. > > Please make the use of platform data look like normal platform data use > rather than going and inventing some new scheme. I'm sorry but I no longer have any clue what it is what you want us to do / how you want us to solve this. AFAIK what the current code is doing is exactly what you requested during the discussion with Daniel. If this is not to your looking please provide some pseudo code explaining how you want us to solve this problem instead of the current solution. 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? 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! Regards, Hans
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.
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
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig index 4fd13b06231f..d107af5bff6c 100644 --- a/drivers/regulator/Kconfig +++ b/drivers/regulator/Kconfig @@ -1339,6 +1339,15 @@ config REGULATOR_TPS65912 help This driver supports TPS65912 voltage regulator chip. +config REGULATOR_TPS68470 + tristate "TI TPS68370 PMIC Regulators Driver" + depends on INTEL_SKL_INT3472 + help + This driver adds support for the TPS68470 PMIC to register + regulators against the usual framework. + + The module will be called "tps68470-regulator" + config REGULATOR_TPS80031 tristate "TI TPS80031/TPS80032 power regulator driver" depends on MFD_TPS80031 diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile index 9e382b50a5ef..03c318110986 100644 --- a/drivers/regulator/Makefile +++ b/drivers/regulator/Makefile @@ -158,6 +158,7 @@ obj-$(CONFIG_REGULATOR_TPS6524X) += tps6524x-regulator.o obj-$(CONFIG_REGULATOR_TPS6586X) += tps6586x-regulator.o obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o obj-$(CONFIG_REGULATOR_TPS65912) += tps65912-regulator.o +obj-$(CONFIG_REGULATOR_TPS68470) += tps68470-regulator.o obj-$(CONFIG_REGULATOR_TPS80031) += tps80031-regulator.o obj-$(CONFIG_REGULATOR_TPS65132) += tps65132-regulator.o obj-$(CONFIG_REGULATOR_TWL4030) += twl-regulator.o twl6030-regulator.o diff --git a/drivers/regulator/tps68470-regulator.c b/drivers/regulator/tps68470-regulator.c new file mode 100644 index 000000000000..82c4b1211c6f --- /dev/null +++ b/drivers/regulator/tps68470-regulator.c @@ -0,0 +1,194 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Regulator driver for TPS68470 PMIC + * + * Copyright (C) 2018 Intel Corporation + * + * Authors: + * Zaikuo Wang <zaikuo.wang@intel.com> + * Tianshu Qiu <tian.shu.qiu@intel.com> + * Jian Xu Zheng <jian.xu.zheng@intel.com> + * Yuning Pu <yuning.pu@intel.com> + * Rajmohan Mani <rajmohan.mani@intel.com> + */ + +#include <linux/device.h> +#include <linux/err.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/mfd/tps68470.h> +#include <linux/module.h> +#include <linux/platform_data/tps68470.h> +#include <linux/platform_device.h> +#include <linux/regulator/driver.h> +#include <linux/regulator/machine.h> + +#define TPS68470_REGULATOR(_name, _id, _ops, _n, _vr, \ + _vm, _er, _em, _t, _lr, _nlr) \ + [TPS68470_ ## _name] = { \ + .name = # _name, \ + .id = _id, \ + .ops = &_ops, \ + .n_voltages = _n, \ + .type = REGULATOR_VOLTAGE, \ + .owner = THIS_MODULE, \ + .vsel_reg = _vr, \ + .vsel_mask = _vm, \ + .enable_reg = _er, \ + .enable_mask = _em, \ + .volt_table = _t, \ + .linear_ranges = _lr, \ + .n_linear_ranges = _nlr, \ + } + +static const struct linear_range tps68470_ldo_ranges[] = { + REGULATOR_LINEAR_RANGE(875000, 0, 125, 17800), +}; + +static const struct linear_range tps68470_core_ranges[] = { + REGULATOR_LINEAR_RANGE(900000, 0, 42, 25000), +}; + +/* Operations permitted on DCDCx, LDO2, LDO3 and LDO4 */ +static struct regulator_ops tps68470_regulator_ops = { + .is_enabled = regulator_is_enabled_regmap, + .enable = regulator_enable_regmap, + .disable = regulator_disable_regmap, + .get_voltage_sel = regulator_get_voltage_sel_regmap, + .set_voltage_sel = regulator_set_voltage_sel_regmap, + .list_voltage = regulator_list_voltage_linear_range, + .map_voltage = regulator_map_voltage_linear_range, +}; + +static const struct regulator_desc regulators[] = { + TPS68470_REGULATOR(CORE, TPS68470_CORE, + tps68470_regulator_ops, 43, TPS68470_REG_VDVAL, + TPS68470_VDVAL_DVOLT_MASK, TPS68470_REG_VDCTL, + TPS68470_VDCTL_EN_MASK, + NULL, tps68470_core_ranges, + ARRAY_SIZE(tps68470_core_ranges)), + TPS68470_REGULATOR(ANA, TPS68470_ANA, + tps68470_regulator_ops, 126, TPS68470_REG_VAVAL, + TPS68470_VAVAL_AVOLT_MASK, TPS68470_REG_VACTL, + TPS68470_VACTL_EN_MASK, + NULL, tps68470_ldo_ranges, + ARRAY_SIZE(tps68470_ldo_ranges)), + TPS68470_REGULATOR(VCM, TPS68470_VCM, + tps68470_regulator_ops, 126, TPS68470_REG_VCMVAL, + TPS68470_VCMVAL_VCVOLT_MASK, TPS68470_REG_VCMCTL, + TPS68470_VCMCTL_EN_MASK, + NULL, tps68470_ldo_ranges, + ARRAY_SIZE(tps68470_ldo_ranges)), + TPS68470_REGULATOR(VIO, TPS68470_VIO, + tps68470_regulator_ops, 126, TPS68470_REG_VIOVAL, + TPS68470_VIOVAL_IOVOLT_MASK, TPS68470_REG_S_I2C_CTL, + TPS68470_S_I2C_CTL_EN_MASK, + NULL, tps68470_ldo_ranges, + ARRAY_SIZE(tps68470_ldo_ranges)), + +/* + * (1) This register must have same setting as VIOVAL if S_IO LDO is used to + * power daisy chained IOs in the receive side. + * (2) If there is no I2C daisy chain it can be set freely. + * + */ + TPS68470_REGULATOR(VSIO, TPS68470_VSIO, + tps68470_regulator_ops, 126, TPS68470_REG_VSIOVAL, + TPS68470_VSIOVAL_IOVOLT_MASK, TPS68470_REG_S_I2C_CTL, + TPS68470_S_I2C_CTL_EN_MASK, + NULL, tps68470_ldo_ranges, + ARRAY_SIZE(tps68470_ldo_ranges)), + TPS68470_REGULATOR(AUX1, TPS68470_AUX1, + tps68470_regulator_ops, 126, TPS68470_REG_VAUX1VAL, + TPS68470_VAUX1VAL_AUX1VOLT_MASK, + TPS68470_REG_VAUX1CTL, + TPS68470_VAUX1CTL_EN_MASK, + NULL, tps68470_ldo_ranges, + ARRAY_SIZE(tps68470_ldo_ranges)), + TPS68470_REGULATOR(AUX2, TPS68470_AUX2, + tps68470_regulator_ops, 126, TPS68470_REG_VAUX2VAL, + TPS68470_VAUX2VAL_AUX2VOLT_MASK, + TPS68470_REG_VAUX2CTL, + TPS68470_VAUX2CTL_EN_MASK, + NULL, tps68470_ldo_ranges, + ARRAY_SIZE(tps68470_ldo_ranges)), +}; + +#define TPS68470_REG_INIT_DATA(_name, _min_uV, _max_uV) \ + [TPS68470_ ## _name] = { \ + .constraints = { \ + .name = # _name, \ + .valid_ops_mask = REGULATOR_CHANGE_VOLTAGE | \ + REGULATOR_CHANGE_STATUS, \ + .min_uV = _min_uV, \ + .max_uV = _max_uV, \ + }, \ + } + +struct regulator_init_data tps68470_init[] = { + TPS68470_REG_INIT_DATA(CORE, 900000, 1950000), + TPS68470_REG_INIT_DATA(ANA, 875000, 3100000), + TPS68470_REG_INIT_DATA(VCM, 875000, 3100000), + TPS68470_REG_INIT_DATA(VIO, 875000, 3100000), + TPS68470_REG_INIT_DATA(VSIO, 875000, 3100000), + TPS68470_REG_INIT_DATA(AUX1, 875000, 3100000), + TPS68470_REG_INIT_DATA(AUX2, 875000, 3100000), +}; + +static int tps68470_regulator_probe(struct platform_device *pdev) +{ + struct tps68470_regulator_platform_data *pdata = pdev->dev.platform_data; + struct regulator_config config = { }; + struct regmap *tps68470_regmap; + struct regulator_dev *rdev; + int i; + + tps68470_regmap = dev_get_drvdata(pdev->dev.parent); + + for (i = 0; i < TPS68470_NUM_REGULATORS; i++) { + config.dev = pdev->dev.parent; + config.regmap = tps68470_regmap; + if (pdata && pdata->reg_init_data[i]) + config.init_data = pdata->reg_init_data[i]; + else + config.init_data = &tps68470_init[i]; + + rdev = devm_regulator_register(&pdev->dev, ®ulators[i], &config); + if (IS_ERR(rdev)) { + dev_err(&pdev->dev, "failed to register %s regulator\n", + regulators[i].name); + return PTR_ERR(rdev); + } + } + + return 0; +} + +static struct platform_driver tps68470_regulator_driver = { + .driver = { + .name = "tps68470-regulator", + }, + .probe = tps68470_regulator_probe, +}; + +/* + * The ACPI tps68470 probe-ordering depends on the clk/gpio/regulator drivers + * being registered before the MFD cells are created (the MFD driver calls + * acpi_dev_clear_dependencies() after the cell creation). + * subsys_initcall() ensures this when the drivers are builtin. + */ +static int __init tps68470_regulator_init(void) +{ + return platform_driver_register(&tps68470_regulator_driver); +} +subsys_initcall(tps68470_regulator_init); + +static void __exit tps68470_regulator_exit(void) +{ + platform_driver_unregister(&tps68470_regulator_driver); +} +module_exit(tps68470_regulator_exit); + +MODULE_ALIAS("platform:tps68470-regulator"); +MODULE_DESCRIPTION("TPS68470 voltage regulator driver"); +MODULE_LICENSE("GPL v2");
The TPS68470 PMIC provides Clocks, GPIOs and Regulators. At present in the kernel the Regulators and Clocks are controlled by an OpRegion driver designed to work with power control methods defined in ACPI, but some platforms lack those methods, meaning drivers need to be able to consume the resources of these chips through the usual frameworks. This commit adds a driver for the regulators provided by the tps68470, and is designed to bind to the platform_device registered by the intel_skl_int3472 module. This is based on this out of tree driver written by Intel: https://github.com/intel/linux-intel-lts/blob/4.14/base/drivers/regulator/tps68470-regulator.c with various cleanups added. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/regulator/Kconfig | 9 ++ drivers/regulator/Makefile | 1 + drivers/regulator/tps68470-regulator.c | 194 +++++++++++++++++++++++++ 3 files changed, 204 insertions(+) create mode 100644 drivers/regulator/tps68470-regulator.c