diff mbox series

[v8,08/12] regulator: bd718x7: Split driver to common and bd718x7 specific parts

Message ID d247d71e183b388dd7f211aee1235965cff979b4.1577694311.git.matti.vaittinen@fi.rohmeurope.com
State New
Headers show
Series Support ROHM BD71828 PMIC | expand

Commit Message

Matti Vaittinen Dec. 30, 2019, 9:39 a.m. UTC
Few ROHM PMICs allow setting the voltage states for different system states
like RUN, IDLE, SUSPEND and LPSR. States are then changed via SoC specific
mechanisms. bd718x7 driver implemented device-tree parsing functions for
these state specific voltages. The parsing functions can be re-used by
other ROHM chip drivers like bd71828. Split the generic functions from
bd718x7-regulator.c to rohm-regulator.c and export them for other modules
to use.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Acked-by: Mark Brown <broonie@kernel.org>
---

Changes from v7 - no changes

 drivers/regulator/Kconfig             |   4 +
 drivers/regulator/Makefile            |   1 +
 drivers/regulator/bd718x7-regulator.c | 183 ++++++++------------------
 drivers/regulator/rohm-regulator.c    |  95 +++++++++++++
 include/linux/mfd/rohm-generic.h      |  44 +++++++
 5 files changed, 199 insertions(+), 128 deletions(-)
 create mode 100644 drivers/regulator/rohm-regulator.c

Comments

Lee Jones Jan. 7, 2020, 12:41 p.m. UTC | #1
On Mon, 30 Dec 2019, Matti Vaittinen wrote:

> Few ROHM PMICs allow setting the voltage states for different system states
> like RUN, IDLE, SUSPEND and LPSR. States are then changed via SoC specific
> mechanisms. bd718x7 driver implemented device-tree parsing functions for
> these state specific voltages. The parsing functions can be re-used by
> other ROHM chip drivers like bd71828. Split the generic functions from
> bd718x7-regulator.c to rohm-regulator.c and export them for other modules
> to use.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> Acked-by: Mark Brown <broonie@kernel.org>
> ---
> 
> Changes from v7 - no changes
> 
>  drivers/regulator/Kconfig             |   4 +
>  drivers/regulator/Makefile            |   1 +
>  drivers/regulator/bd718x7-regulator.c | 183 ++++++++------------------
>  drivers/regulator/rohm-regulator.c    |  95 +++++++++++++
>  include/linux/mfd/rohm-generic.h      |  44 +++++++
>  5 files changed, 199 insertions(+), 128 deletions(-)
>  create mode 100644 drivers/regulator/rohm-regulator.c

[...]

> diff --git a/include/linux/mfd/rohm-generic.h b/include/linux/mfd/rohm-generic.h
> index ff3dd7578fd3..8037421cc6a1 100644
> --- a/include/linux/mfd/rohm-generic.h
> +++ b/include/linux/mfd/rohm-generic.h
> @@ -4,6 +4,9 @@
>  #ifndef __LINUX_MFD_ROHM_H__
>  #define __LINUX_MFD_ROHM_H__
>  
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +
>  enum rohm_chip_type {
>  	ROHM_CHIP_TYPE_BD71837 = 0,
>  	ROHM_CHIP_TYPE_BD71847,
> @@ -17,4 +20,45 @@ struct rohm_regmap_dev {
>  	struct regmap *regmap;
>  };
>  
> +enum {
> +	ROHM_DVS_LEVEL_UNKNOWN,
> +	ROHM_DVS_LEVEL_RUN,
> +	ROHM_DVS_LEVEL_IDLE,
> +	ROHM_DVS_LEVEL_SUSPEND,
> +	ROHM_DVS_LEVEL_LPSR,
> +#define ROHM_DVS_LEVEL_MAX ROHM_DVS_LEVEL_LPSR
> +};
> +
> +struct rohm_dvs_config {
> +	uint64_t level_map;
> +	unsigned int run_reg;
> +	unsigned int run_mask;
> +	unsigned int run_on_mask;
> +	unsigned int idle_reg;
> +	unsigned int idle_mask;
> +	unsigned int idle_on_mask;
> +	unsigned int suspend_reg;
> +	unsigned int suspend_mask;
> +	unsigned int suspend_on_mask;
> +	unsigned int lpsr_reg;
> +	unsigned int lpsr_mask;
> +	unsigned int lpsr_on_mask;
> +};

I think this deserves a kernel-doc header.

> +#if IS_ENABLED(CONFIG_REGULATOR_ROHM)
> +int rohm_regulator_set_dvs_levels(const struct rohm_dvs_config *dvs,
> +				  struct device_node *np,
> +				  const struct regulator_desc *desc,
> +				  struct regmap *regmap);

Does these really need to live in the parent's header file?

What other call-sites are there?

> +#else
> +static inline int rohm_regulator_set_dvs_levels(const struct rohm_dvs_config *dvs,
> +						struct device_node *np,
> +						const struct regulator_desc *desc,
> +						struct regmap *regmap)
> +{
> +	return 0;
> +}
> +#endif //IS_ENABLED(CONFIG_REGULATOR_ROHM)

a) This comment is not really required
b) You shouldn't be using C++ comments
Matti Vaittinen Jan. 8, 2020, 8:34 a.m. UTC | #2
Hello Lee,

On Tue, 2020-01-07 at 12:41 +0000, Lee Jones wrote:
> On Mon, 30 Dec 2019, Matti Vaittinen wrote:
> 
> > Few ROHM PMICs allow setting the voltage states for different
> > system states
> > like RUN, IDLE, SUSPEND and LPSR. States are then changed via SoC
> > specific
> > mechanisms. bd718x7 driver implemented device-tree parsing
> > functions for
> > these state specific voltages. The parsing functions can be re-used 
> > by
> > other ROHM chip drivers like bd71828. Split the generic functions
> > from
> > bd718x7-regulator.c to rohm-regulator.c and export them for other
> > modules
> > to use.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > Acked-by: Mark Brown <broonie@kernel.org>
> > ---
> > 
> > +
> > +struct rohm_dvs_config {
> > +	uint64_t level_map;
> > +	unsigned int run_reg;
> > +	unsigned int run_mask;
> > +	unsigned int run_on_mask;
> > +	unsigned int idle_reg;
> > +	unsigned int idle_mask;
> > +	unsigned int idle_on_mask;
> > +	unsigned int suspend_reg;
> > +	unsigned int suspend_mask;
> > +	unsigned int suspend_on_mask;
> > +	unsigned int lpsr_reg;
> > +	unsigned int lpsr_mask;
> > +	unsigned int lpsr_on_mask;
> > +};
> 
> I think this deserves a kernel-doc header.
Oh, the struct here? Hmm. You might be correct. I can add some.

> 
> > +#if IS_ENABLED(CONFIG_REGULATOR_ROHM)
> > +int rohm_regulator_set_dvs_levels(const struct rohm_dvs_config
> > *dvs,
> > +				  struct device_node *np,
> > +				  const struct regulator_desc *desc,
> > +				  struct regmap *regmap);
> 
> Does these really need to live in the parent's header file?

I don't know what would be a better place?

> What other call-sites are there?

After this series the bd718x7-regulator.c and bd71828-regulator.c are
the in-tree drivers using these. rohm-regulator.c is implementing them.
And I hope we see yet another driver landing in later this year. 

Anyways, I will investigate if I can switch this to some common (not
rohm specific) DT bindings at some point (I've scheduled this study to
March) - If I can then they should live in regulator core headers.

But changing the existing properties should again be own set of patches
and I'd prefer doing that work independently of this series and not
delaying the BD71828 due to not-yet-evaluated bd718x7 property changes.

> 
> > +#else
> > +static inline int rohm_regulator_set_dvs_levels(const struct
> > rohm_dvs_config *dvs,
> > +						struct device_node *np,
> > +						const struct
> > regulator_desc *desc,
> > +						struct regmap *regmap)
> > +{
> > +	return 0;
> > +}
> > +#endif //IS_ENABLED(CONFIG_REGULATOR_ROHM)
> 
> a) This comment is not really required
> b) You shouldn't be using C++ comments

Thanks, I'll remove it.

Best Regards
	Matti
Lee Jones Jan. 13, 2020, 10:53 a.m. UTC | #3
On Wed, 08 Jan 2020, Vaittinen, Matti wrote:

> Hello Lee,
> 
> On Tue, 2020-01-07 at 12:41 +0000, Lee Jones wrote:
> > On Mon, 30 Dec 2019, Matti Vaittinen wrote:
> > 
> > > Few ROHM PMICs allow setting the voltage states for different
> > > system states
> > > like RUN, IDLE, SUSPEND and LPSR. States are then changed via SoC
> > > specific
> > > mechanisms. bd718x7 driver implemented device-tree parsing
> > > functions for
> > > these state specific voltages. The parsing functions can be re-used 
> > > by
> > > other ROHM chip drivers like bd71828. Split the generic functions
> > > from
> > > bd718x7-regulator.c to rohm-regulator.c and export them for other
> > > modules
> > > to use.
> > > 
> > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > Acked-by: Mark Brown <broonie@kernel.org>
> > > ---

[...]

> > > +#if IS_ENABLED(CONFIG_REGULATOR_ROHM)
> > > +int rohm_regulator_set_dvs_levels(const struct rohm_dvs_config
> > > *dvs,
> > > +				  struct device_node *np,
> > > +				  const struct regulator_desc *desc,
> > > +				  struct regmap *regmap);
> > 
> > Does these really need to live in the parent's header file?
> 
> I don't know what would be a better place?

You don't have a regulator header file?

It seems over-kill to create one for this, so leave it as is.

> > What other call-sites are there?
> 
> After this series the bd718x7-regulator.c and bd71828-regulator.c are
> the in-tree drivers using these. rohm-regulator.c is implementing them.
> And I hope we see yet another driver landing in later this year. 
> 
> Anyways, I will investigate if I can switch this to some common (not
> rohm specific) DT bindings at some point (I've scheduled this study to
> March) - If I can then they should live in regulator core headers.
> 
> But changing the existing properties should again be own set of patches
> and I'd prefer doing that work independently of this series and not
> delaying the BD71828 due to not-yet-evaluated bd718x7 property changes.

That's fine.
Matti Vaittinen Jan. 13, 2020, 11:49 a.m. UTC | #4
Hello Lee,

On Mon, 2020-01-13 at 10:53 +0000, Lee Jones wrote:
> On Wed, 08 Jan 2020, Vaittinen, Matti wrote:
> 
> > Hello Lee,
> > 
> > On Tue, 2020-01-07 at 12:41 +0000, Lee Jones wrote:
> > > On Mon, 30 Dec 2019, Matti Vaittinen wrote:
> > > 
> > > > Few ROHM PMICs allow setting the voltage states for different
> > > > system states
> > > > like RUN, IDLE, SUSPEND and LPSR. States are then changed via
> > > > SoC
> > > > specific
> > > > mechanisms. bd718x7 driver implemented device-tree parsing
> > > > functions for
> > > > these state specific voltages. The parsing functions can be re-
> > > > used 
> > > > by
> > > > other ROHM chip drivers like bd71828. Split the generic
> > > > functions
> > > > from
> > > > bd718x7-regulator.c to rohm-regulator.c and export them for
> > > > other
> > > > modules
> > > > to use.
> > > > 
> > > > Signed-off-by: Matti Vaittinen <
> > > > matti.vaittinen@fi.rohmeurope.com>
> > > > Acked-by: Mark Brown <broonie@kernel.org>
> > > > ---
> 
> [...]
> 
> > > > +#if IS_ENABLED(CONFIG_REGULATOR_ROHM)
> > > > +int rohm_regulator_set_dvs_levels(const struct rohm_dvs_config
> > > > *dvs,
> > > > +				  struct device_node *np,
> > > > +				  const struct regulator_desc
> > > > *desc,
> > > > +				  struct regmap *regmap);
> > > 
> > > Does these really need to live in the parent's header file?
> > 
> > I don't know what would be a better place?
> 
> You don't have a regulator header file?
> 
> It seems over-kill to create one for this, so leave it as is.
> 
> > > What other call-sites are there?
> > 
> > After this series the bd718x7-regulator.c and bd71828-regulator.c
> > are
> > the in-tree drivers using these. rohm-regulator.c is implementing
> > them.
> > And I hope we see yet another driver landing in later this year. 
> > 
> > Anyways, I will investigate if I can switch this to some common
> > (not
> > rohm specific) DT bindings at some point (I've scheduled this study
> > to
> > March) - If I can then they should live in regulator core headers.
> > 
> > But changing the existing properties should again be own set of
> > patches
> > and I'd prefer doing that work independently of this series and not
> > delaying the BD71828 due to not-yet-evaluated bd718x7 property
> > changes.
> 
> That's fine.


Glad to hear :) By the way, I already sent the v9 ;)

Br,
	Matti
Lee Jones Jan. 13, 2020, 12:11 p.m. UTC | #5
On Mon, 13 Jan 2020, Vaittinen, Matti wrote:

> Hello Lee,
> 
> On Mon, 2020-01-13 at 10:53 +0000, Lee Jones wrote:
> > On Wed, 08 Jan 2020, Vaittinen, Matti wrote:
> > 
> > > Hello Lee,
> > > 
> > > On Tue, 2020-01-07 at 12:41 +0000, Lee Jones wrote:
> > > > On Mon, 30 Dec 2019, Matti Vaittinen wrote:
> > > > 
> > > > > Few ROHM PMICs allow setting the voltage states for different
> > > > > system states
> > > > > like RUN, IDLE, SUSPEND and LPSR. States are then changed via
> > > > > SoC
> > > > > specific
> > > > > mechanisms. bd718x7 driver implemented device-tree parsing
> > > > > functions for
> > > > > these state specific voltages. The parsing functions can be re-
> > > > > used 
> > > > > by
> > > > > other ROHM chip drivers like bd71828. Split the generic
> > > > > functions
> > > > > from
> > > > > bd718x7-regulator.c to rohm-regulator.c and export them for
> > > > > other
> > > > > modules
> > > > > to use.
> > > > > 
> > > > > Signed-off-by: Matti Vaittinen <
> > > > > matti.vaittinen@fi.rohmeurope.com>
> > > > > Acked-by: Mark Brown <broonie@kernel.org>
> > > > > ---
> > 
> > [...]
> > 
> > > > > +#if IS_ENABLED(CONFIG_REGULATOR_ROHM)
> > > > > +int rohm_regulator_set_dvs_levels(const struct rohm_dvs_config
> > > > > *dvs,
> > > > > +				  struct device_node *np,
> > > > > +				  const struct regulator_desc
> > > > > *desc,
> > > > > +				  struct regmap *regmap);
> > > > 
> > > > Does these really need to live in the parent's header file?
> > > 
> > > I don't know what would be a better place?
> > 
> > You don't have a regulator header file?
> > 
> > It seems over-kill to create one for this, so leave it as is.
> > 
> > > > What other call-sites are there?
> > > 
> > > After this series the bd718x7-regulator.c and bd71828-regulator.c
> > > are
> > > the in-tree drivers using these. rohm-regulator.c is implementing
> > > them.
> > > And I hope we see yet another driver landing in later this year. 
> > > 
> > > Anyways, I will investigate if I can switch this to some common
> > > (not
> > > rohm specific) DT bindings at some point (I've scheduled this study
> > > to
> > > March) - If I can then they should live in regulator core headers.
> > > 
> > > But changing the existing properties should again be own set of
> > > patches
> > > and I'd prefer doing that work independently of this series and not
> > > delaying the BD71828 due to not-yet-evaluated bd718x7 property
> > > changes.
> > 
> > That's fine.
> 
> Glad to hear :) By the way, I already sent the v9 ;)

It's in my queue.
Lee Jones Jan. 15, 2020, 8:29 a.m. UTC | #6
On Mon, 13 Jan 2020, Lee Jones wrote:
> On Mon, 13 Jan 2020, Vaittinen, Matti wrote:
> > On Mon, 2020-01-13 at 10:53 +0000, Lee Jones wrote:
> > > On Wed, 08 Jan 2020, Vaittinen, Matti wrote:
> > > > On Tue, 2020-01-07 at 12:41 +0000, Lee Jones wrote:
> > > > > On Mon, 30 Dec 2019, Matti Vaittinen wrote:
> > > > > 
> > > > > > Few ROHM PMICs allow setting the voltage states for different
> > > > > > system states
> > > > > > like RUN, IDLE, SUSPEND and LPSR. States are then changed via
> > > > > > SoC
> > > > > > specific
> > > > > > mechanisms. bd718x7 driver implemented device-tree parsing
> > > > > > functions for
> > > > > > these state specific voltages. The parsing functions can be re-
> > > > > > used 
> > > > > > by
> > > > > > other ROHM chip drivers like bd71828. Split the generic
> > > > > > functions
> > > > > > from
> > > > > > bd718x7-regulator.c to rohm-regulator.c and export them for
> > > > > > other
> > > > > > modules
> > > > > > to use.
> > > > > > 
> > > > > > Signed-off-by: Matti Vaittinen <
> > > > > > matti.vaittinen@fi.rohmeurope.com>
> > > > > > Acked-by: Mark Brown <broonie@kernel.org>
> > > > > > ---
> > > 
> > > [...]
> > > 
> > > > > > +#if IS_ENABLED(CONFIG_REGULATOR_ROHM)
> > > > > > +int rohm_regulator_set_dvs_levels(const struct rohm_dvs_config
> > > > > > *dvs,
> > > > > > +				  struct device_node *np,
> > > > > > +				  const struct regulator_desc
> > > > > > *desc,
> > > > > > +				  struct regmap *regmap);
> > > > > 
> > > > > Does these really need to live in the parent's header file?
> > > > 
> > > > I don't know what would be a better place?
> > > 
> > > You don't have a regulator header file?
> > > 
> > > It seems over-kill to create one for this, so leave it as is.
> > > 
> > > > > What other call-sites are there?
> > > > 
> > > > After this series the bd718x7-regulator.c and bd71828-regulator.c
> > > > are
> > > > the in-tree drivers using these. rohm-regulator.c is implementing
> > > > them.
> > > > And I hope we see yet another driver landing in later this year. 
> > > > 
> > > > Anyways, I will investigate if I can switch this to some common
> > > > (not
> > > > rohm specific) DT bindings at some point (I've scheduled this study
> > > > to
> > > > March) - If I can then they should live in regulator core headers.
> > > > 
> > > > But changing the existing properties should again be own set of
> > > > patches
> > > > and I'd prefer doing that work independently of this series and not
> > > > delaying the BD71828 due to not-yet-evaluated bd718x7 property
> > > > changes.
> > > 
> > > That's fine.
> > 
> > Glad to hear :) By the way, I already sent the v9 ;)
> 
> It's in my queue.

Although you didn't submit the whole set to me, so I only have a few
of the patches.  Why did you choose to do that this time?
Matti Vaittinen Jan. 15, 2020, 8:34 a.m. UTC | #7
On Wed, 2020-01-15 at 08:29 +0000, Lee Jones wrote:
> On Mon, 13 Jan 2020, Lee Jones wrote:
> > On Mon, 13 Jan 2020, Vaittinen, Matti wrote:
> > > On Mon, 2020-01-13 at 10:53 +0000, Lee Jones wrote:
> > > > On Wed, 08 Jan 2020, Vaittinen, Matti wrote:
> > > > > On Tue, 2020-01-07 at 12:41 +0000, Lee Jones wrote:
> > > > > > On Mon, 30 Dec 2019, Matti Vaittinen wrote:
> > > > > > 
> > > > > > > Few ROHM PMICs allow setting the voltage states for
> > > > > > > different
> > > > > > > system states
> > > > > > > like RUN, IDLE, SUSPEND and LPSR. States are then changed
> > > > > > > via
> > > > > > > SoC
> > > > > > > specific
> > > > > > > mechanisms. bd718x7 driver implemented device-tree
> > > > > > > parsing
> > > > > > > functions for
> > > > > > > these state specific voltages. The parsing functions can
> > > > > > > be re-
> > > > > > > used 
> > > > > > > by
> > > > > > > other ROHM chip drivers like bd71828. Split the generic
> > > > > > > functions
> > > > > > > from
> > > > > > > bd718x7-regulator.c to rohm-regulator.c and export them
> > > > > > > for
> > > > > > > other
> > > > > > > modules
> > > > > > > to use.
> > > > > > > 
> > > > > > > Signed-off-by: Matti Vaittinen <
> > > > > > > matti.vaittinen@fi.rohmeurope.com>
> > > > > > > Acked-by: Mark Brown <broonie@kernel.org>
> > > > > > > ---
> > > > 
> > > > [...]
> > > > 
> > > > > > > +#if IS_ENABLED(CONFIG_REGULATOR_ROHM)
> > > > > > > +int rohm_regulator_set_dvs_levels(const struct
> > > > > > > rohm_dvs_config
> > > > > > > *dvs,
> > > > > > > +				  struct device_node *np,
> > > > > > > +				  const struct regulator_desc
> > > > > > > *desc,
> > > > > > > +				  struct regmap *regmap);
> > > > > > 
> > > > > > Does these really need to live in the parent's header file?
> > > > > 
> > > > > I don't know what would be a better place?
> > > > 
> > > > You don't have a regulator header file?
> > > > 
> > > > It seems over-kill to create one for this, so leave it as is.
> > > > 
> > > > > > What other call-sites are there?
> > > > > 
> > > > > After this series the bd718x7-regulator.c and bd71828-
> > > > > regulator.c
> > > > > are
> > > > > the in-tree drivers using these. rohm-regulator.c is
> > > > > implementing
> > > > > them.
> > > > > And I hope we see yet another driver landing in later this
> > > > > year. 
> > > > > 
> > > > > Anyways, I will investigate if I can switch this to some
> > > > > common
> > > > > (not
> > > > > rohm specific) DT bindings at some point (I've scheduled this
> > > > > study
> > > > > to
> > > > > March) - If I can then they should live in regulator core
> > > > > headers.
> > > > > 
> > > > > But changing the existing properties should again be own set
> > > > > of
> > > > > patches
> > > > > and I'd prefer doing that work independently of this series
> > > > > and not
> > > > > delaying the BD71828 due to not-yet-evaluated bd718x7
> > > > > property
> > > > > changes.
> > > > 
> > > > That's fine.
> > > 
> > > Glad to hear :) By the way, I already sent the v9 ;)
> > 
> > It's in my queue.
> 
> Although you didn't submit the whole set to me, so I only have a few
> of the patches.  Why did you choose to do that this time?

Sorry Lee. I have not dropped you intentionally! I will resend the
series.

Br,
	Matti Vaittinen
diff mbox series

Patch

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 74eb5af7295f..a4897ae52f14 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -197,6 +197,7 @@  config REGULATOR_BD70528
 config REGULATOR_BD718XX
 	tristate "ROHM BD71837 Power Regulator"
 	depends on MFD_ROHM_BD718XX
+	select REGULATOR_ROHM
 	help
 	  This driver supports voltage regulators on ROHM BD71837 PMIC.
 	  This will enable support for the software controllable buck
@@ -790,6 +791,9 @@  config REGULATOR_RN5T618
 	  Say y here to support the regulators found on Ricoh RN5T567,
 	  RN5T618 or RC5T619 PMIC.
 
+config REGULATOR_ROHM
+	tristate
+
 config REGULATOR_RT5033
 	tristate "Richtek RT5033 Regulators"
 	depends on MFD_RT5033
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 2210ba56f9bd..6bcab72c1fc7 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -99,6 +99,7 @@  obj-$(CONFIG_REGULATOR_PCF50633) += pcf50633-regulator.o
 obj-$(CONFIG_REGULATOR_RC5T583)  += rc5t583-regulator.o
 obj-$(CONFIG_REGULATOR_RK808)   += rk808-regulator.o
 obj-$(CONFIG_REGULATOR_RN5T618) += rn5t618-regulator.o
+obj-$(CONFIG_REGULATOR_ROHM)	+= rohm-regulator.o
 obj-$(CONFIG_REGULATOR_RT5033)	+= rt5033-regulator.o
 obj-$(CONFIG_REGULATOR_S2MPA01) += s2mpa01.o
 obj-$(CONFIG_REGULATOR_S2MPS11) += s2mps11.o
diff --git a/drivers/regulator/bd718x7-regulator.c b/drivers/regulator/bd718x7-regulator.c
index 6beaf867d9cb..55decb58c777 100644
--- a/drivers/regulator/bd718x7-regulator.c
+++ b/drivers/regulator/bd718x7-regulator.c
@@ -318,6 +318,7 @@  struct reg_init {
 };
 struct bd718xx_regulator_data {
 	struct regulator_desc desc;
+	const struct rohm_dvs_config dvs;
 	const struct reg_init init;
 	const struct reg_init *additional_inits;
 	int additional_init_amnt;
@@ -349,133 +350,15 @@  static const struct reg_init bd71837_ldo6_inits[] = {
 	},
 };
 
-#define NUM_DVS_BUCKS 4
-
-struct of_dvs_setting {
-	const char *prop;
-	unsigned int reg;
-};
-
-static int set_dvs_levels(const struct of_dvs_setting *dvs,
-			  struct device_node *np,
-			  const struct regulator_desc *desc,
-			  struct regmap *regmap)
-{
-	int ret, i;
-	unsigned int uv;
-
-	ret = of_property_read_u32(np, dvs->prop, &uv);
-	if (ret) {
-		if (ret != -EINVAL)
-			return ret;
-		return 0;
-	}
-
-	for (i = 0; i < desc->n_voltages; i++) {
-		ret = regulator_desc_list_voltage_linear_range(desc, i);
-		if (ret < 0)
-			continue;
-		if (ret == uv) {
-			i <<= ffs(desc->vsel_mask) - 1;
-			ret = regmap_update_bits(regmap, dvs->reg,
-						 DVS_BUCK_RUN_MASK, i);
-			break;
-		}
-	}
-	return ret;
-}
-
-static int buck4_set_hw_dvs_levels(struct device_node *np,
+static int buck_set_hw_dvs_levels(struct device_node *np,
 			    const struct regulator_desc *desc,
 			    struct regulator_config *cfg)
 {
-	int ret, i;
-	const struct of_dvs_setting dvs[] = {
-		{
-			.prop = "rohm,dvs-run-voltage",
-			.reg = BD71837_REG_BUCK4_VOLT_RUN,
-		},
-	};
+	struct bd718xx_regulator_data *data;
 
-	for (i = 0; i < ARRAY_SIZE(dvs); i++) {
-		ret = set_dvs_levels(&dvs[i], np, desc, cfg->regmap);
-		if (ret)
-			break;
-	}
-	return ret;
-}
-static int buck3_set_hw_dvs_levels(struct device_node *np,
-			    const struct regulator_desc *desc,
-			    struct regulator_config *cfg)
-{
-	int ret, i;
-	const struct of_dvs_setting dvs[] = {
-		{
-			.prop = "rohm,dvs-run-voltage",
-			.reg = BD71837_REG_BUCK3_VOLT_RUN,
-		},
-	};
+	data = container_of(desc, struct bd718xx_regulator_data, desc);
 
-	for (i = 0; i < ARRAY_SIZE(dvs); i++) {
-		ret = set_dvs_levels(&dvs[i], np, desc, cfg->regmap);
-		if (ret)
-			break;
-	}
-	return ret;
-}
-
-static int buck2_set_hw_dvs_levels(struct device_node *np,
-			    const struct regulator_desc *desc,
-			    struct regulator_config *cfg)
-{
-	int ret, i;
-	const struct of_dvs_setting dvs[] = {
-		{
-			.prop = "rohm,dvs-run-voltage",
-			.reg = BD718XX_REG_BUCK2_VOLT_RUN,
-		},
-		{
-			.prop = "rohm,dvs-idle-voltage",
-			.reg = BD718XX_REG_BUCK2_VOLT_IDLE,
-		},
-	};
-
-
-
-	for (i = 0; i < ARRAY_SIZE(dvs); i++) {
-		ret = set_dvs_levels(&dvs[i], np, desc, cfg->regmap);
-		if (ret)
-			break;
-	}
-	return ret;
-}
-
-static int buck1_set_hw_dvs_levels(struct device_node *np,
-			    const struct regulator_desc *desc,
-			    struct regulator_config *cfg)
-{
-	int ret, i;
-	const struct of_dvs_setting dvs[] = {
-		{
-			.prop = "rohm,dvs-run-voltage",
-			.reg = BD718XX_REG_BUCK1_VOLT_RUN,
-		},
-		{
-			.prop = "rohm,dvs-idle-voltage",
-			.reg = BD718XX_REG_BUCK1_VOLT_IDLE,
-		},
-		{
-			.prop = "rohm,dvs-suspend-voltage",
-			.reg = BD718XX_REG_BUCK1_VOLT_SUSP,
-		},
-	};
-
-	for (i = 0; i < ARRAY_SIZE(dvs); i++) {
-		ret = set_dvs_levels(&dvs[i], np, desc, cfg->regmap);
-		if (ret)
-			break;
-	}
-	return ret;
+	return rohm_regulator_set_dvs_levels(&data->dvs, np, desc, cfg->regmap);
 }
 
 static const struct bd718xx_regulator_data bd71847_regulators[] = {
@@ -496,7 +379,17 @@  static const struct bd718xx_regulator_data bd71847_regulators[] = {
 			.enable_reg = BD718XX_REG_BUCK1_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
 			.owner = THIS_MODULE,
-			.of_parse_cb = buck1_set_hw_dvs_levels,
+			.of_parse_cb = buck_set_hw_dvs_levels,
+		},
+		.dvs = {
+			.level_map = ROHM_DVS_LEVEL_RUN | ROHM_DVS_LEVEL_IDLE |
+				     ROHM_DVS_LEVEL_SUSPEND,
+			.run_reg = BD718XX_REG_BUCK1_VOLT_RUN,
+			.run_mask = DVS_BUCK_RUN_MASK,
+			.idle_reg = BD718XX_REG_BUCK1_VOLT_IDLE,
+			.idle_mask = DVS_BUCK_RUN_MASK,
+			.suspend_reg = BD718XX_REG_BUCK1_VOLT_SUSP,
+			.suspend_mask = DVS_BUCK_RUN_MASK,
 		},
 		.init = {
 			.reg = BD718XX_REG_BUCK1_CTRL,
@@ -520,7 +413,14 @@  static const struct bd718xx_regulator_data bd71847_regulators[] = {
 			.enable_reg = BD718XX_REG_BUCK2_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
 			.owner = THIS_MODULE,
-			.of_parse_cb = buck2_set_hw_dvs_levels,
+			.of_parse_cb = buck_set_hw_dvs_levels,
+		},
+		.dvs = {
+			.level_map = ROHM_DVS_LEVEL_RUN | ROHM_DVS_LEVEL_IDLE,
+			.run_reg = BD718XX_REG_BUCK2_VOLT_RUN,
+			.run_mask = DVS_BUCK_RUN_MASK,
+			.idle_reg = BD718XX_REG_BUCK2_VOLT_IDLE,
+			.idle_mask = DVS_BUCK_RUN_MASK,
 		},
 		.init = {
 			.reg = BD718XX_REG_BUCK2_CTRL,
@@ -792,7 +692,17 @@  static const struct bd718xx_regulator_data bd71837_regulators[] = {
 			.enable_reg = BD718XX_REG_BUCK1_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
 			.owner = THIS_MODULE,
-			.of_parse_cb = buck1_set_hw_dvs_levels,
+			.of_parse_cb = buck_set_hw_dvs_levels,
+		},
+		.dvs = {
+			.level_map = ROHM_DVS_LEVEL_RUN | ROHM_DVS_LEVEL_IDLE |
+				     ROHM_DVS_LEVEL_SUSPEND,
+			.run_reg = BD718XX_REG_BUCK1_VOLT_RUN,
+			.run_mask = DVS_BUCK_RUN_MASK,
+			.idle_reg = BD718XX_REG_BUCK1_VOLT_IDLE,
+			.idle_mask = DVS_BUCK_RUN_MASK,
+			.suspend_reg = BD718XX_REG_BUCK1_VOLT_SUSP,
+			.suspend_mask = DVS_BUCK_RUN_MASK,
 		},
 		.init = {
 			.reg = BD718XX_REG_BUCK1_CTRL,
@@ -816,7 +726,14 @@  static const struct bd718xx_regulator_data bd71837_regulators[] = {
 			.enable_reg = BD718XX_REG_BUCK2_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
 			.owner = THIS_MODULE,
-			.of_parse_cb = buck2_set_hw_dvs_levels,
+			.of_parse_cb = buck_set_hw_dvs_levels,
+		},
+		.dvs = {
+			.level_map = ROHM_DVS_LEVEL_RUN | ROHM_DVS_LEVEL_IDLE,
+			.run_reg = BD718XX_REG_BUCK2_VOLT_RUN,
+			.run_mask = DVS_BUCK_RUN_MASK,
+			.idle_reg = BD718XX_REG_BUCK2_VOLT_IDLE,
+			.idle_mask = DVS_BUCK_RUN_MASK,
 		},
 		.init = {
 			.reg = BD718XX_REG_BUCK2_CTRL,
@@ -840,7 +757,12 @@  static const struct bd718xx_regulator_data bd71837_regulators[] = {
 			.enable_reg = BD71837_REG_BUCK3_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
 			.owner = THIS_MODULE,
-			.of_parse_cb = buck3_set_hw_dvs_levels,
+			.of_parse_cb = buck_set_hw_dvs_levels,
+		},
+		.dvs = {
+			.level_map = ROHM_DVS_LEVEL_RUN,
+			.run_reg = BD71837_REG_BUCK3_VOLT_RUN,
+			.run_mask = DVS_BUCK_RUN_MASK,
 		},
 		.init = {
 			.reg = BD71837_REG_BUCK3_CTRL,
@@ -864,7 +786,12 @@  static const struct bd718xx_regulator_data bd71837_regulators[] = {
 			.enable_reg = BD71837_REG_BUCK4_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
 			.owner = THIS_MODULE,
-			.of_parse_cb = buck4_set_hw_dvs_levels,
+			.of_parse_cb = buck_set_hw_dvs_levels,
+		},
+		.dvs = {
+			.level_map = ROHM_DVS_LEVEL_RUN,
+			.run_reg = BD71837_REG_BUCK4_VOLT_RUN,
+			.run_mask = DVS_BUCK_RUN_MASK,
 		},
 		.init = {
 			.reg = BD71837_REG_BUCK4_CTRL,
diff --git a/drivers/regulator/rohm-regulator.c b/drivers/regulator/rohm-regulator.c
new file mode 100644
index 000000000000..ca368ada53c6
--- /dev/null
+++ b/drivers/regulator/rohm-regulator.c
@@ -0,0 +1,95 @@ 
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 ROHM Semiconductors
+
+#include <linux/errno.h>
+#include <linux/mfd/rohm-generic.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+
+static int set_dvs_level(const struct regulator_desc *desc,
+			 struct device_node *np, struct regmap *regmap,
+			 char *prop, unsigned int reg, unsigned int mask,
+			 unsigned int omask, unsigned int oreg)
+{
+	int ret, i;
+	uint32_t uv;
+
+	ret = of_property_read_u32(np, prop, &uv);
+	if (ret) {
+		if (ret != -EINVAL)
+			return ret;
+		return 0;
+	}
+
+	if (uv == 0) {
+		if (omask)
+			return regmap_update_bits(regmap, oreg, omask, 0);
+	}
+	for (i = 0; i < desc->n_voltages; i++) {
+		ret = regulator_desc_list_voltage_linear_range(desc, i);
+		if (ret < 0)
+			continue;
+		if (ret == uv) {
+			i <<= ffs(desc->vsel_mask) - 1;
+			ret = regmap_update_bits(regmap, reg, mask, i);
+			if (omask && !ret)
+				ret = regmap_update_bits(regmap, oreg, omask,
+							 omask);
+			break;
+		}
+	}
+	return ret;
+}
+
+int rohm_regulator_set_dvs_levels(const struct rohm_dvs_config *dvs,
+			  struct device_node *np,
+			  const struct regulator_desc *desc,
+			  struct regmap *regmap)
+{
+	int i, ret = 0;
+	char *prop;
+	unsigned int reg, mask, omask, oreg = desc->enable_reg;
+
+	for (i = 0; i < ROHM_DVS_LEVEL_MAX && !ret; i++) {
+		if (dvs->level_map & (1 << i)) {
+			switch (i + 1) {
+			case ROHM_DVS_LEVEL_RUN:
+				prop = "rohm,dvs-run-voltage";
+				reg = dvs->run_reg;
+				mask = dvs->run_mask;
+				omask = dvs->run_on_mask;
+				break;
+			case ROHM_DVS_LEVEL_IDLE:
+				prop = "rohm,dvs-idle-voltage";
+				reg = dvs->idle_reg;
+				mask = dvs->idle_mask;
+				omask = dvs->idle_on_mask;
+				break;
+			case ROHM_DVS_LEVEL_SUSPEND:
+				prop = "rohm,dvs-suspend-voltage";
+				reg = dvs->suspend_reg;
+				mask = dvs->suspend_mask;
+				omask = dvs->suspend_on_mask;
+				break;
+			case ROHM_DVS_LEVEL_LPSR:
+				prop = "rohm,dvs-lpsr-voltage";
+				reg = dvs->lpsr_reg;
+				mask = dvs->lpsr_mask;
+				omask = dvs->lpsr_on_mask;
+				break;
+			default:
+				return -EINVAL;
+			}
+			ret = set_dvs_level(desc, np, regmap, prop, reg, mask,
+					    omask, oreg);
+		}
+	}
+	return ret;
+}
+EXPORT_SYMBOL(rohm_regulator_set_dvs_levels);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_DESCRIPTION("Generic helpers for ROHM PMIC regulator drivers");
diff --git a/include/linux/mfd/rohm-generic.h b/include/linux/mfd/rohm-generic.h
index ff3dd7578fd3..8037421cc6a1 100644
--- a/include/linux/mfd/rohm-generic.h
+++ b/include/linux/mfd/rohm-generic.h
@@ -4,6 +4,9 @@ 
 #ifndef __LINUX_MFD_ROHM_H__
 #define __LINUX_MFD_ROHM_H__
 
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+
 enum rohm_chip_type {
 	ROHM_CHIP_TYPE_BD71837 = 0,
 	ROHM_CHIP_TYPE_BD71847,
@@ -17,4 +20,45 @@  struct rohm_regmap_dev {
 	struct regmap *regmap;
 };
 
+enum {
+	ROHM_DVS_LEVEL_UNKNOWN,
+	ROHM_DVS_LEVEL_RUN,
+	ROHM_DVS_LEVEL_IDLE,
+	ROHM_DVS_LEVEL_SUSPEND,
+	ROHM_DVS_LEVEL_LPSR,
+#define ROHM_DVS_LEVEL_MAX ROHM_DVS_LEVEL_LPSR
+};
+
+struct rohm_dvs_config {
+	uint64_t level_map;
+	unsigned int run_reg;
+	unsigned int run_mask;
+	unsigned int run_on_mask;
+	unsigned int idle_reg;
+	unsigned int idle_mask;
+	unsigned int idle_on_mask;
+	unsigned int suspend_reg;
+	unsigned int suspend_mask;
+	unsigned int suspend_on_mask;
+	unsigned int lpsr_reg;
+	unsigned int lpsr_mask;
+	unsigned int lpsr_on_mask;
+};
+
+#if IS_ENABLED(CONFIG_REGULATOR_ROHM)
+int rohm_regulator_set_dvs_levels(const struct rohm_dvs_config *dvs,
+				  struct device_node *np,
+				  const struct regulator_desc *desc,
+				  struct regmap *regmap);
+
+#else
+static inline int rohm_regulator_set_dvs_levels(const struct rohm_dvs_config *dvs,
+						struct device_node *np,
+						const struct regulator_desc *desc,
+						struct regmap *regmap)
+{
+	return 0;
+}
+#endif //IS_ENABLED(CONFIG_REGULATOR_ROHM)
+
 #endif