diff mbox series

[05/12] regulator: Introduce tps68470-regulator driver

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

Commit Message

Hans de Goede Oct. 8, 2021, 4:21 p.m. UTC
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

Comments

Mark Brown Oct. 11, 2021, 10:42 a.m. UTC | #1
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?
Hans de Goede Oct. 11, 2021, 11:43 a.m. UTC | #2
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
Mark Brown Oct. 15, 2021, 4:46 p.m. UTC | #3
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.
Hans de Goede Oct. 15, 2021, 6:50 p.m. UTC | #4
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
Mark Brown Oct. 15, 2021, 6:58 p.m. UTC | #5
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.
Hans de Goede Oct. 15, 2021, 7:27 p.m. UTC | #6
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
Mark Brown Oct. 15, 2021, 7:40 p.m. UTC | #7
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.
Hans de Goede Oct. 15, 2021, 7:48 p.m. UTC | #8
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, &regulators[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
Mark Brown Oct. 15, 2021, 7:59 p.m. UTC | #9
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.
Hans de Goede Oct. 15, 2021, 8:14 p.m. UTC | #10
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
Mark Brown Oct. 15, 2021, 10:29 p.m. UTC | #11
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.
Hans de Goede Oct. 16, 2021, 10:18 a.m. UTC | #12
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 mbox series

Patch

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, &regulators[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");