* [PATCHv2] make mc13783 regulator code generic
@ 2010-12-01 7:15 Yong Shen
2010-12-01 7:50 ` Sascha Hauer
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Yong Shen @ 2010-12-01 7:15 UTC (permalink / raw)
To: linux-arm-kernel
Hi there,
This is the v2 with some changes according to comments from v1. There
will be few patches coming out after this one, for mc13892 regulator
to share some code with mc13783.
Still, cause the firewall problem, I send out patch this way. Please
give comments inline and use attached patch for testing.
Yong
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCHv2] make mc13783 regulator code generic 2010-12-01 7:15 [PATCHv2] make mc13783 regulator code generic Yong Shen @ 2010-12-01 7:50 ` Sascha Hauer 2010-12-02 2:36 ` Yong Shen 2010-12-01 11:25 ` Mark Brown 2010-12-02 10:45 ` [PATCH 2/2] " Yong Shen 2 siblings, 1 reply; 8+ messages in thread From: Sascha Hauer @ 2010-12-01 7:50 UTC (permalink / raw) To: linux-arm-kernel Hi Yong, On Wed, Dec 01, 2010 at 03:15:55PM +0800, Yong Shen wrote: > Hi there, > > This is the v2 with some changes according to comments from v1. There > will be few patches coming out after this one, for mc13892 regulator > to share some code with mc13783. > > Still, cause the firewall problem, I send out patch this way. Please > give comments inline and use attached patch for testing. This patch definitely goes into the right direction. Some comments inline. > > Yong > > From b3451070f4665c90f11e600a8722f86b68437ded Mon Sep 17 00:00:00 2001 > From: Yong Shen <yong.shen@linaro.org> > Date: Tue, 30 Nov 2010 14:11:34 +0800 > Subject: [PATCH] make mc13783 regulator code generic > > move some common functions and micros of mc13783 regulaor driver to > a seperate file, which makes it possible for mc13892 to share code. > > Signed-off-by: Yong Shen <yong.shen@linaro.org> > --- > drivers/regulator/Kconfig | 4 + > drivers/regulator/Makefile | 1 + > drivers/regulator/mc13783-regulator.c | 325 ++++------------------------ > drivers/regulator/mc13xxx-regulator-core.c | 239 ++++++++++++++++++++ > include/linux/mfd/mc13783.h | 67 +++--- > include/linux/regulator/mc13xxx.h | 101 +++++++++ > 6 files changed, 425 insertions(+), 312 deletions(-) > create mode 100644 drivers/regulator/mc13xxx-regulator-core.c > create mode 100644 include/linux/regulator/mc13xxx.h > > diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig > index dd30e88..63c2bdd 100644 > --- a/drivers/regulator/Kconfig > +++ b/drivers/regulator/Kconfig > @@ -186,9 +186,13 @@ config REGULATOR_PCAP > This driver provides support for the voltage regulators of the > PCAP2 PMIC. > > +config REGULATOR_MC13XXX_CORE > + tristate "Support regulators on Freescale MC13xxx PMIC" > + This does not need a prompt. Selecting only this option does not make sense and it will be selected by the mc13783/mc13892 code anyway. > config REGULATOR_MC13783 > tristate "Support regulators on Freescale MC13783 PMIC" > depends on MFD_MC13783 > + select REGULATOR_MC13XXX_CORE > help > Say y here to support the regulators found on the Freescale MC13783 > PMIC. > diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile > index bff8157..11876be 100644 > --- a/drivers/regulator/Makefile > +++ b/drivers/regulator/Makefile > @@ -30,6 +30,7 @@ obj-$(CONFIG_REGULATOR_DA903X) += da903x.o > obj-$(CONFIG_REGULATOR_PCF50633) += pcf50633-regulator.o > obj-$(CONFIG_REGULATOR_PCAP) += pcap-regulator.o > obj-$(CONFIG_REGULATOR_MC13783) += mc13783-regulator.o > +obj-$(CONFIG_REGULATOR_MC13XXX_CORE) += mc13xxx-regulator-core.o > obj-$(CONFIG_REGULATOR_AB3100) += ab3100.o > > obj-$(CONFIG_REGULATOR_TPS65023) += tps65023-regulator.o [snip] > diff --git a/include/linux/mfd/mc13783.h b/include/linux/mfd/mc13783.h > index b4c741e..7d0f3d6 100644 > --- a/include/linux/mfd/mc13783.h > +++ b/include/linux/mfd/mc13783.h > @@ -1,4 +1,5 @@ > /* > + * Copyright 2010 Yong Shen <yong.shen@linaro.org> > * Copyright 2009-2010 Pengutronix > * Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de> > * > @@ -122,39 +123,39 @@ int mc13783_adc_do_conversion(struct mc13783 > *mc13783, unsigned int mode, > unsigned int channel, unsigned int *sample); > > > -#define MC13783_SW_SW1A 0 > -#define MC13783_SW_SW1B 1 > -#define MC13783_SW_SW2A 2 > -#define MC13783_SW_SW2B 3 > -#define MC13783_SW_SW3 4 > -#define MC13783_SW_PLL 5 > -#define MC13783_REGU_VAUDIO 6 > -#define MC13783_REGU_VIOHI 7 > -#define MC13783_REGU_VIOLO 8 > -#define MC13783_REGU_VDIG 9 > -#define MC13783_REGU_VGEN 10 > -#define MC13783_REGU_VRFDIG 11 > -#define MC13783_REGU_VRFREF 12 > -#define MC13783_REGU_VRFCP 13 > -#define MC13783_REGU_VSIM 14 > -#define MC13783_REGU_VESIM 15 > -#define MC13783_REGU_VCAM 16 > -#define MC13783_REGU_VRFBG 17 > -#define MC13783_REGU_VVIB 18 > -#define MC13783_REGU_VRF1 19 > -#define MC13783_REGU_VRF2 20 > -#define MC13783_REGU_VMMC1 21 > -#define MC13783_REGU_VMMC2 22 > -#define MC13783_REGU_GPO1 23 > -#define MC13783_REGU_GPO2 24 > -#define MC13783_REGU_GPO3 25 > -#define MC13783_REGU_GPO4 26 > -#define MC13783_REGU_V1 27 > -#define MC13783_REGU_V2 28 > -#define MC13783_REGU_V3 29 > -#define MC13783_REGU_V4 30 > -#define MC13783_REGU_PWGT1SPI 31 > -#define MC13783_REGU_PWGT2SPI 32 These have users. If you really want to change them you must change the users aswell. Also, I would suggest an additional patch for this. Remember, one topic per patch. Renaming things is a topic that can easily be split out. > +#define MC13783_REG_SW1A 0 > +#define MC13783_REG_SW1B 1 > +#define MC13783_REG_SW2A 2 > +#define MC13783_REG_SW2B 3 > +#define MC13783_REG_SW3 4 > +#define MC13783_REG_PLL 5 > +#define MC13783_REG_VAUDIO 6 > +#define MC13783_REG_VIOHI 7 > +#define MC13783_REG_VIOLO 8 > +#define MC13783_REG_VDIG 9 > +#define MC13783_REG_VGEN 10 > +#define MC13783_REG_VRFDIG 11 > +#define MC13783_REG_VRFREF 12 > +#define MC13783_REG_VRFCP 13 > +#define MC13783_REG_VSIM 14 > +#define MC13783_REG_VESIM 15 > +#define MC13783_REG_VCAM 16 > +#define MC13783_REG_VRFBG 17 > +#define MC13783_REG_VVIB 18 > +#define MC13783_REG_VRF1 19 > +#define MC13783_REG_VRF2 20 > +#define MC13783_REG_VMMC1 21 > +#define MC13783_REG_VMMC2 22 > +#define MC13783_REG_GPO1 23 > +#define MC13783_REG_GPO2 24 > +#define MC13783_REG_GPO3 25 > +#define MC13783_REG_GPO4 26 > +#define MC13783_REG_V1 27 > +#define MC13783_REG_V2 28 > +#define MC13783_REG_V3 29 > +#define MC13783_REG_V4 30 > +#define MC13783_REG_PWGT1SPI 31 > +#define MC13783_REG_PWGT2SPI 32 > > #define MC13783_IRQ_ADCDONE MC13XXX_IRQ_ADCDONE > #define MC13783_IRQ_ADCBISDONE MC13XXX_IRQ_ADCBISDONE > diff --git a/include/linux/regulator/mc13xxx.h > b/include/linux/regulator/mc13xxx.h > new file mode 100644 > index 0000000..a60c9be > --- /dev/null > +++ b/include/linux/regulator/mc13xxx.h The things in this file are private to the driver. IMO this should go to drivers/regulator/mc13xxx.h. > @@ -0,0 +1,101 @@ > +/* > + * mc13xxx.h - regulators for the Freescale mc13xxx PMIC > + * > + * Copyright (C) 2010 Yong Shen <yong.shen@linaro.org> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#ifndef __LINUX_REGULATOR_MC13XXX_H > +#define __LINUX_REGULATOR_MC13XXX_H > + > +#include <linux/regulator/driver.h> > + > +struct mc13xxx_regulator { > + struct regulator_desc desc; > + int reg; > + int enable_bit; > + int vsel_reg; > + int vsel_shift; > + int vsel_mask; > + int hi_bit; > + int const *voltages; > +}; > + > +struct mc13xxx_regulator_priv { > + struct mc13xxx *mc13xxx; > + u32 powermisc_pwgt_state; > + struct mc13xxx_regulator *mc13xxx_regulators; > + struct regulator_dev *regulators[]; > +}; > + > +extern int mc13xxx_sw_regulator(struct regulator_dev *rdev); > +extern int mc13xxx_sw_regulator_is_enabled(struct regulator_dev *rdev); > +extern int mc13xxx_get_best_voltage_index(struct regulator_dev *rdev, > + int min_uV, int max_uV); > +extern int mc13xxx_regulator_list_voltage(struct regulator_dev *rdev, > + unsigned selector); > +extern int mc13xxx_fixed_regulator_set_voltage(struct regulator_dev *rdev, > + int min_uV, int max_uV); > +extern int mc13xxx_fixed_regulator_get_voltage(struct regulator_dev *rdev); > + > +extern struct regulator_ops mc13xxx_regulator_ops; > +extern struct regulator_ops mc13xxx_fixed_regulator_ops; > + > +#define MC13xxx_DEFINE(prefix, _name, _reg, _vsel_reg, _voltages, _ops) \ > + [prefix ## _name] = { \ > + .desc = { \ > + .name = #prefix "_" #_name, \ > + .n_voltages = ARRAY_SIZE(_voltages), \ > + .ops = &_ops, \ > + .type = REGULATOR_VOLTAGE, \ > + .id = prefix ## _name, \ > + .owner = THIS_MODULE, \ > + }, \ > + .reg = prefix ## _reg, \ > + .enable_bit = prefix ## _reg ## _ ## _name ## EN, \ > + .vsel_reg = prefix ## _vsel_reg, \ > + .vsel_shift = prefix ## _vsel_reg ## _ ## _name ## VSEL,\ > + .vsel_mask = prefix ## _vsel_reg ## _ ## _name ## VSEL_M,\ > + .voltages = _voltages, \ > + } > + > +#define MC13xxx_FIXED_DEFINE(prefix, _name, _reg, _voltages, _ops) \ > + [prefix ## _name] = { \ > + .desc = { \ > + .name = #prefix "_" #_name, \ > + .n_voltages = ARRAY_SIZE(_voltages), \ > + .ops = &_ops, \ > + .type = REGULATOR_VOLTAGE, \ > + .id = prefix ## _name, \ > + .owner = THIS_MODULE, \ > + }, \ > + .reg = prefix ## _reg, \ > + .enable_bit = prefix ## _reg ## _ ## _name ## EN, \ > + .voltages = _voltages, \ > + } > + > +#define MC13xxx_GPO_DEFINE(prefix, _name, _reg, _voltages, _ops) \ > + [prefix ## _name] = { \ > + .desc = { \ > + .name = #prefix "_" #_name, \ > + .n_voltages = ARRAY_SIZE(_voltages), \ > + .ops = &_ops, \ > + .type = REGULATOR_VOLTAGE, \ > + .id = prefix ## _name, \ > + .owner = THIS_MODULE, \ > + }, \ > + .reg = prefix ## _reg, \ > + .enable_bit = prefix ## _reg ## _ ## _name ## EN, \ > + .voltages = _voltages, \ > + } > + > +#define MC13xxx_DEFINE_SW(_name, _reg, _vsel_reg, _voltages, ops) \ > + MC13xxx_DEFINE(SW, _name, _reg, _vsel_reg, _voltages, ops) > +#define MC13xxx_DEFINE_REGU(_name, _reg, _vsel_reg, _voltages, ops) \ > + MC13xxx_DEFINE(REGU, _name, _reg, _vsel_reg, _voltages, ops) > + > +#endif > -- > 1.7.0.4 > > Cheers > Yong -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCHv2] make mc13783 regulator code generic 2010-12-01 7:50 ` Sascha Hauer @ 2010-12-02 2:36 ` Yong Shen 0 siblings, 0 replies; 8+ messages in thread From: Yong Shen @ 2010-12-02 2:36 UTC (permalink / raw) To: linux-arm-kernel On Wed, Dec 1, 2010 at 3:50 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > Hi Yong, > > On Wed, Dec 01, 2010 at 03:15:55PM +0800, Yong Shen wrote: >> Hi there, >> >> This is the v2 with some changes according to comments from v1. There >> will be few patches coming out after this one, for mc13892 regulator >> to share some code with mc13783. >> >> Still, cause the firewall problem, I send out patch this way. Please >> give comments inline and use attached patch for testing. > > This patch definitely goes into the right direction. Some comments > inline. > >> >> Yong >> >> From b3451070f4665c90f11e600a8722f86b68437ded Mon Sep 17 00:00:00 2001 >> From: Yong Shen <yong.shen@linaro.org> >> Date: Tue, 30 Nov 2010 14:11:34 +0800 >> Subject: [PATCH] make mc13783 regulator code generic >> >> move some common functions and micros of mc13783 regulaor driver to >> a seperate file, which makes it possible for mc13892 to share code. >> >> Signed-off-by: Yong Shen <yong.shen@linaro.org> >> --- >> ?drivers/regulator/Kconfig ? ? ? ? ? ? ? ? ?| ? ?4 + >> ?drivers/regulator/Makefile ? ? ? ? ? ? ? ? | ? ?1 + >> ?drivers/regulator/mc13783-regulator.c ? ? ?| ?325 ++++------------------------ >> ?drivers/regulator/mc13xxx-regulator-core.c | ?239 ++++++++++++++++++++ >> ?include/linux/mfd/mc13783.h ? ? ? ? ? ? ? ?| ? 67 +++--- >> ?include/linux/regulator/mc13xxx.h ? ? ? ? ?| ?101 +++++++++ >> ?6 files changed, 425 insertions(+), 312 deletions(-) >> ?create mode 100644 drivers/regulator/mc13xxx-regulator-core.c >> ?create mode 100644 include/linux/regulator/mc13xxx.h >> >> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig >> index dd30e88..63c2bdd 100644 >> --- a/drivers/regulator/Kconfig >> +++ b/drivers/regulator/Kconfig >> @@ -186,9 +186,13 @@ config REGULATOR_PCAP >> ? ? ? ?This driver provides support for the voltage regulators of the >> ? ? ? ?PCAP2 PMIC. >> >> +config REGULATOR_MC13XXX_CORE >> + ? ? tristate "Support regulators on Freescale MC13xxx PMIC" >> + > > This does not need a prompt. Selecting only this option does not make > sense and it will be selected by the mc13783/mc13892 code anyway. > >> ?config REGULATOR_MC13783 >> ? ? ? tristate "Support regulators on Freescale MC13783 PMIC" >> ? ? ? depends on MFD_MC13783 >> + ? ? select REGULATOR_MC13XXX_CORE >> ? ? ? help >> ? ? ? ? Say y here to support the regulators found on the Freescale MC13783 >> ? ? ? ? PMIC. >> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile >> index bff8157..11876be 100644 >> --- a/drivers/regulator/Makefile >> +++ b/drivers/regulator/Makefile >> @@ -30,6 +30,7 @@ obj-$(CONFIG_REGULATOR_DA903X) ? ? ?+= da903x.o >> ?obj-$(CONFIG_REGULATOR_PCF50633) += pcf50633-regulator.o >> ?obj-$(CONFIG_REGULATOR_PCAP) += pcap-regulator.o >> ?obj-$(CONFIG_REGULATOR_MC13783) += mc13783-regulator.o >> +obj-$(CONFIG_REGULATOR_MC13XXX_CORE) += ?mc13xxx-regulator-core.o >> ?obj-$(CONFIG_REGULATOR_AB3100) += ab3100.o >> >> ?obj-$(CONFIG_REGULATOR_TPS65023) += tps65023-regulator.o > > [snip] > >> diff --git a/include/linux/mfd/mc13783.h b/include/linux/mfd/mc13783.h >> index b4c741e..7d0f3d6 100644 >> --- a/include/linux/mfd/mc13783.h >> +++ b/include/linux/mfd/mc13783.h >> @@ -1,4 +1,5 @@ >> ?/* >> + * Copyright 2010 Yong Shen <yong.shen@linaro.org> >> ? * Copyright 2009-2010 Pengutronix >> ? * Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de> >> ? * >> @@ -122,39 +123,39 @@ int mc13783_adc_do_conversion(struct mc13783 >> *mc13783, unsigned int mode, >> ? ? ? ? ? ? ? unsigned int channel, unsigned int *sample); >> >> >> -#define ? ? ?MC13783_SW_SW1A ? ? ? ? 0 >> -#define ? ? ?MC13783_SW_SW1B ? ? ? ? 1 >> -#define ? ? ?MC13783_SW_SW2A ? ? ? ? 2 >> -#define ? ? ?MC13783_SW_SW2B ? ? ? ? 3 >> -#define ? ? ?MC13783_SW_SW3 ? ? ? ? ?4 >> -#define ? ? ?MC13783_SW_PLL ? ? ? ? ?5 >> -#define ? ? ?MC13783_REGU_VAUDIO ? ? 6 >> -#define ? ? ?MC13783_REGU_VIOHI ? ? ?7 >> -#define ? ? ?MC13783_REGU_VIOLO ? ? ?8 >> -#define ? ? ?MC13783_REGU_VDIG ? ? ? 9 >> -#define ? ? ?MC13783_REGU_VGEN ? ? ? 10 >> -#define ? ? ?MC13783_REGU_VRFDIG ? ? 11 >> -#define ? ? ?MC13783_REGU_VRFREF ? ? 12 >> -#define ? ? ?MC13783_REGU_VRFCP ? ? ?13 >> -#define ? ? ?MC13783_REGU_VSIM ? ? ? 14 >> -#define ? ? ?MC13783_REGU_VESIM ? ? ?15 >> -#define ? ? ?MC13783_REGU_VCAM ? ? ? 16 >> -#define ? ? ?MC13783_REGU_VRFBG ? ? ?17 >> -#define ? ? ?MC13783_REGU_VVIB ? ? ? 18 >> -#define ? ? ?MC13783_REGU_VRF1 ? ? ? 19 >> -#define ? ? ?MC13783_REGU_VRF2 ? ? ? 20 >> -#define ? ? ?MC13783_REGU_VMMC1 ? ? ?21 >> -#define ? ? ?MC13783_REGU_VMMC2 ? ? ?22 >> -#define ? ? ?MC13783_REGU_GPO1 ? ? ? 23 >> -#define ? ? ?MC13783_REGU_GPO2 ? ? ? 24 >> -#define ? ? ?MC13783_REGU_GPO3 ? ? ? 25 >> -#define ? ? ?MC13783_REGU_GPO4 ? ? ? 26 >> -#define ? ? ?MC13783_REGU_V1 ? ? ? ? 27 >> -#define ? ? ?MC13783_REGU_V2 ? ? ? ? 28 >> -#define ? ? ?MC13783_REGU_V3 ? ? ? ? 29 >> -#define ? ? ?MC13783_REGU_V4 ? ? ? ? 30 >> -#define ? ? ?MC13783_REGU_PWGT1SPI ? 31 >> -#define ? ? ?MC13783_REGU_PWGT2SPI ? 32 > > These have users. If you really want to change them you must change > the users aswell. Also, I would suggest an additional patch for this. > Remember, one topic per patch. Renaming things is a topic that can > easily be split out. My bad, I had not noticed that. In this case, I will split it into two. The renaming patch goes first followed with code restructuring patch. > >> +#define ? ? ?MC13783_REG_SW1A ? ? ? ? ? ? ? ?0 >> +#define ? ? ?MC13783_REG_SW1B ? ? ? ? ? ? ? ?1 >> +#define ? ? ?MC13783_REG_SW2A ? ? ? ? ? ? ? ?2 >> +#define ? ? ?MC13783_REG_SW2B ? ? ? ? ? ? ? ?3 >> +#define ? ? ?MC13783_REG_SW3 ? ? ? ? 4 >> +#define ? ? ?MC13783_REG_PLL ? ? ? ? 5 >> +#define ? ? ?MC13783_REG_VAUDIO ? ? ?6 >> +#define ? ? ?MC13783_REG_VIOHI ? ? ? 7 >> +#define ? ? ?MC13783_REG_VIOLO ? ? ? 8 >> +#define ? ? ?MC13783_REG_VDIG ? ? ? ?9 >> +#define ? ? ?MC13783_REG_VGEN ? ? ? ?10 >> +#define ? ? ?MC13783_REG_VRFDIG ? ? ?11 >> +#define ? ? ?MC13783_REG_VRFREF ? ? ?12 >> +#define ? ? ?MC13783_REG_VRFCP ? ? ? 13 >> +#define ? ? ?MC13783_REG_VSIM ? ? ? ?14 >> +#define ? ? ?MC13783_REG_VESIM ? ? ? 15 >> +#define ? ? ?MC13783_REG_VCAM ? ? ? ?16 >> +#define ? ? ?MC13783_REG_VRFBG ? ? ? 17 >> +#define ? ? ?MC13783_REG_VVIB ? ? ? ?18 >> +#define ? ? ?MC13783_REG_VRF1 ? ? ? ?19 >> +#define ? ? ?MC13783_REG_VRF2 ? ? ? ?20 >> +#define ? ? ?MC13783_REG_VMMC1 ? ? ? 21 >> +#define ? ? ?MC13783_REG_VMMC2 ? ? ? 22 >> +#define ? ? ?MC13783_REG_GPO1 ? ? ? ?23 >> +#define ? ? ?MC13783_REG_GPO2 ? ? ? ?24 >> +#define ? ? ?MC13783_REG_GPO3 ? ? ? ?25 >> +#define ? ? ?MC13783_REG_GPO4 ? ? ? ?26 >> +#define ? ? ?MC13783_REG_V1 ? ? ? ? ?27 >> +#define ? ? ?MC13783_REG_V2 ? ? ? ? ?28 >> +#define ? ? ?MC13783_REG_V3 ? ? ? ? ?29 >> +#define ? ? ?MC13783_REG_V4 ? ? ? ? ?30 >> +#define ? ? ?MC13783_REG_PWGT1SPI ? ?31 >> +#define ? ? ?MC13783_REG_PWGT2SPI ? ?32 >> >> ?#define MC13783_IRQ_ADCDONE ?MC13XXX_IRQ_ADCDONE >> ?#define MC13783_IRQ_ADCBISDONE ? ? ? MC13XXX_IRQ_ADCBISDONE >> diff --git a/include/linux/regulator/mc13xxx.h >> b/include/linux/regulator/mc13xxx.h >> new file mode 100644 >> index 0000000..a60c9be >> --- /dev/null >> +++ b/include/linux/regulator/mc13xxx.h > > The things in this file are private to the driver. IMO this should go to > drivers/regulator/mc13xxx.h. > >> @@ -0,0 +1,101 @@ >> +/* >> + * mc13xxx.h - regulators for the Freescale mc13xxx PMIC >> + * >> + * ?Copyright (C) 2010 Yong Shen <yong.shen@linaro.org> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + */ >> + >> +#ifndef __LINUX_REGULATOR_MC13XXX_H >> +#define __LINUX_REGULATOR_MC13XXX_H >> + >> +#include <linux/regulator/driver.h> >> + >> +struct mc13xxx_regulator { >> + ? ? struct regulator_desc desc; >> + ? ? int reg; >> + ? ? int enable_bit; >> + ? ? int vsel_reg; >> + ? ? int vsel_shift; >> + ? ? int vsel_mask; >> + ? ? int hi_bit; >> + ? ? int const *voltages; >> +}; >> + >> +struct mc13xxx_regulator_priv { >> + ? ? struct mc13xxx *mc13xxx; >> + ? ? u32 powermisc_pwgt_state; >> + ? ? struct mc13xxx_regulator *mc13xxx_regulators; >> + ? ? struct regulator_dev *regulators[]; >> +}; >> + >> +extern int mc13xxx_sw_regulator(struct regulator_dev *rdev); >> +extern int mc13xxx_sw_regulator_is_enabled(struct regulator_dev *rdev); >> +extern int mc13xxx_get_best_voltage_index(struct regulator_dev *rdev, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int min_uV, int max_uV); >> +extern int mc13xxx_regulator_list_voltage(struct regulator_dev *rdev, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned selector); >> +extern int mc13xxx_fixed_regulator_set_voltage(struct regulator_dev *rdev, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int min_uV, int max_uV); >> +extern int mc13xxx_fixed_regulator_get_voltage(struct regulator_dev *rdev); >> + >> +extern struct regulator_ops mc13xxx_regulator_ops; >> +extern struct regulator_ops mc13xxx_fixed_regulator_ops; >> + >> +#define MC13xxx_DEFINE(prefix, _name, _reg, _vsel_reg, _voltages, _ops) ? ? ?\ >> + ? ? [prefix ## _name] = { ? ? ? ? ? ? ? ? ? ? ? ? ? \ >> + ? ? ? ? ? ? .desc = { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \ >> + ? ? ? ? ? ? ? ? ? ? .name = #prefix "_" #_name, ? ? ? ? ? ? ? ? ? ? \ >> + ? ? ? ? ? ? ? ? ? ? .n_voltages = ARRAY_SIZE(_voltages), ? ? ? ? ? ?\ >> + ? ? ? ? ? ? ? ? ? ? .ops = &_ops, ? ? ? ? ? ? ? ? ? \ >> + ? ? ? ? ? ? ? ? ? ? .type = REGULATOR_VOLTAGE, ? ? ? ? ? ? ? ? ? ? ?\ >> + ? ? ? ? ? ? ? ? ? ? .id = prefix ## _name, ? ? ? ? ?\ >> + ? ? ? ? ? ? ? ? ? ? .owner = THIS_MODULE, ? ? ? ? ? ? ? ? ? ? ? ? ? \ >> + ? ? ? ? ? ? }, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\ >> + ? ? ? ? ? ? .reg = prefix ## _reg, ? ? ? ? ? ? ? ? ? ? ? ? ?\ >> + ? ? ? ? ? ? .enable_bit = prefix ## _reg ## _ ## _name ## EN, ? ? ? \ >> + ? ? ? ? ? ? .vsel_reg = prefix ## _vsel_reg, ? ? ? ? ? ? ? ? ? ? ? ?\ >> + ? ? ? ? ? ? .vsel_shift = prefix ## _vsel_reg ## _ ## _name ## VSEL,\ >> + ? ? ? ? ? ? .vsel_mask = prefix ## _vsel_reg ## _ ## _name ## VSEL_M,\ >> + ? ? ? ? ? ? .voltages = ?_voltages, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \ >> + ? ? } >> + >> +#define MC13xxx_FIXED_DEFINE(prefix, _name, _reg, _voltages, _ops) ? \ >> + ? ? [prefix ## _name] = { ? ? ? ? ? ? ? ? ? ? ? ? ? \ >> + ? ? ? ? ? ? .desc = { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \ >> + ? ? ? ? ? ? ? ? ? ? .name = #prefix "_" #_name, ? ? ? ? ? ? ? ? ? ? \ >> + ? ? ? ? ? ? ? ? ? ? .n_voltages = ARRAY_SIZE(_voltages), ? ? ? ? ? ?\ >> + ? ? ? ? ? ? ? ? ? ? .ops = &_ops, ? ? ? ? ? \ >> + ? ? ? ? ? ? ? ? ? ? .type = REGULATOR_VOLTAGE, ? ? ? ? ? ? ? ? ? ? ?\ >> + ? ? ? ? ? ? ? ? ? ? .id = prefix ## _name, ? ? ? ? ?\ >> + ? ? ? ? ? ? ? ? ? ? .owner = THIS_MODULE, ? ? ? ? ? ? ? ? ? ? ? ? ? \ >> + ? ? ? ? ? ? }, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\ >> + ? ? ? ? ? ? .reg = prefix ## _reg, ? ? ? ? ? ? ? ? ? ? ? ? ?\ >> + ? ? ? ? ? ? .enable_bit = prefix ## _reg ## _ ## _name ## EN, ? ? ? \ >> + ? ? ? ? ? ? .voltages = ?_voltages, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \ >> + ? ? } >> + >> +#define MC13xxx_GPO_DEFINE(prefix, _name, _reg, ?_voltages, _ops) ? ?\ >> + ? ? [prefix ## _name] = { ? ? ? ? ? ? ? ? ? ? ? ? ? \ >> + ? ? ? ? ? ? .desc = { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \ >> + ? ? ? ? ? ? ? ? ? ? .name = #prefix "_" #_name, ? ? ? ? ? ? ? ? ? ? \ >> + ? ? ? ? ? ? ? ? ? ? .n_voltages = ARRAY_SIZE(_voltages), ? ? ? ? ? ?\ >> + ? ? ? ? ? ? ? ? ? ? .ops = &_ops, ? ? ? ? ? \ >> + ? ? ? ? ? ? ? ? ? ? .type = REGULATOR_VOLTAGE, ? ? ? ? ? ? ? ? ? ? ?\ >> + ? ? ? ? ? ? ? ? ? ? .id = prefix ## _name, ? ? ? ? ?\ >> + ? ? ? ? ? ? ? ? ? ? .owner = THIS_MODULE, ? ? ? ? ? ? ? ? ? ? ? ? ? \ >> + ? ? ? ? ? ? }, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\ >> + ? ? ? ? ? ? .reg = prefix ## _reg, ? ? ? ? ? ? ? ? ? ? ? ? ?\ >> + ? ? ? ? ? ? .enable_bit = prefix ## _reg ## _ ## _name ## EN, ? ? ? \ >> + ? ? ? ? ? ? .voltages = ?_voltages, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \ >> + ? ? } >> + >> +#define MC13xxx_DEFINE_SW(_name, _reg, _vsel_reg, _voltages, ops) ? ?\ >> + ? ? MC13xxx_DEFINE(SW, _name, _reg, _vsel_reg, _voltages, ops) >> +#define MC13xxx_DEFINE_REGU(_name, _reg, _vsel_reg, _voltages, ops) ?\ >> + ? ? MC13xxx_DEFINE(REGU, _name, _reg, _vsel_reg, _voltages, ops) >> + >> +#endif >> -- >> 1.7.0.4 >> >> Cheers >> Yong > > > > -- > Pengutronix e.K. ? ? ? ? ? ? ? ? ? ? ? ? ? | ? ? ? ? ? ? ? ? ? ? ? ? ? ? | > Industrial Linux Solutions ? ? ? ? ? ? ? ? | http://www.pengutronix.de/ ?| > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 ? ?| > Amtsgericht Hildesheim, HRA 2686 ? ? ? ? ? | Fax: ? +49-5121-206917-5555 | > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCHv2] make mc13783 regulator code generic 2010-12-01 7:15 [PATCHv2] make mc13783 regulator code generic Yong Shen 2010-12-01 7:50 ` Sascha Hauer @ 2010-12-01 11:25 ` Mark Brown 2010-12-02 2:41 ` Yong Shen 2010-12-02 10:45 ` [PATCH 2/2] " Yong Shen 2 siblings, 1 reply; 8+ messages in thread From: Mark Brown @ 2010-12-01 11:25 UTC (permalink / raw) To: linux-arm-kernel On Wed, Dec 01, 2010 at 03:15:55PM +0800, Yong Shen wrote: > move some common functions and micros of mc13783 regulaor driver to > a seperate file, which makes it possible for mc13892 to share code. You've done way more than this in the patch - you've also renamed a lot of things and done other restructurings. I'd suggest splitting out the big mechanical changes for easier review. For example, a patch moving code from one place to another, another patch renaming things and so on. > +config REGULATOR_MC13XXX_CORE > + tristate "Support regulators on Freescale MC13xxx PMIC" > + This doesn't need to be user visible - the user only cares about the individual regulator drivers. > /* Power Gate enable value is 0 */ > - if (id == MC13783_REGU_PWGT1SPI || > - id == MC13783_REGU_PWGT2SPI) > + if (id == MC13783_REG_PWGT1SPI || > + id == MC13783_REG_PWGT2SPI) > en_val = 0; I can't tell what the actual change is here? > +int mc13xxx_sw_regulator(struct regulator_dev *rdev) > +{ > + return 0; > +} > + Eh? > +MODULE_ALIAS("platform:mc13xxx-regulator-core"); Is there really going to be a platform device for this? > +++ b/include/linux/regulator/mc13xxx.h Pretty much everything in this file is internal to the driver and shouldn't be in include/linux. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCHv2] make mc13783 regulator code generic 2010-12-01 11:25 ` Mark Brown @ 2010-12-02 2:41 ` Yong Shen 0 siblings, 0 replies; 8+ messages in thread From: Yong Shen @ 2010-12-02 2:41 UTC (permalink / raw) To: linux-arm-kernel Hi, please see inline feedback. On Wed, Dec 1, 2010 at 7:25 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Wed, Dec 01, 2010 at 03:15:55PM +0800, Yong Shen wrote: > >> ?move some common functions and micros of mc13783 regulaor driver to >> a seperate file, which makes it possible for mc13892 to share code. > > You've done way more than this in the patch - you've also renamed a lot > of things and done other restructurings. ?I'd suggest splitting out the > big mechanical changes for easier review. ?For example, a patch moving > code from one place to another, another patch renaming things and so on. > >> +config REGULATOR_MC13XXX_CORE >> + ? ? tristate "Support regulators on Freescale MC13xxx PMIC" >> + > > This doesn't need to be user visible - the user only cares about the > individual regulator drivers. > >> ? ? ? /* Power Gate enable value is 0 */ >> - ? ? if (id == MC13783_REGU_PWGT1SPI || >> - ? ? ? ? id == MC13783_REGU_PWGT2SPI) >> + ? ? if (id == MC13783_REG_PWGT1SPI || >> + ? ? ? ? id == MC13783_REG_PWGT2SPI) >> ? ? ? ? ? ? ? en_val = 0; > > I can't tell what the actual change is here? The macro names changed. And this will go into a separate patch. > >> +int mc13xxx_sw_regulator(struct regulator_dev *rdev) >> +{ >> + ? ? return 0; >> +} >> + > > Eh? Confused naming and it is not necessary. it will be removed. > >> +MODULE_ALIAS("platform:mc13xxx-regulator-core"); > > Is there really going to be a platform device for this? ACKed. > >> +++ b/include/linux/regulator/mc13xxx.h > > Pretty much everything in this file is internal to the driver and > shouldn't be in include/linux. > ACKed. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] make mc13783 regulator code generic 2010-12-01 7:15 [PATCHv2] make mc13783 regulator code generic Yong Shen 2010-12-01 7:50 ` Sascha Hauer 2010-12-01 11:25 ` Mark Brown @ 2010-12-02 10:45 ` Yong Shen 2010-12-02 10:57 ` Uwe Kleine-König 2 siblings, 1 reply; 8+ messages in thread From: Yong Shen @ 2010-12-02 10:45 UTC (permalink / raw) To: linux-arm-kernel Hi there, Please give comments inline and use attached patch for testing. Thanks Yong ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] make mc13783 regulator code generic 2010-12-02 10:45 ` [PATCH 2/2] " Yong Shen @ 2010-12-02 10:57 ` Uwe Kleine-König 2010-12-02 11:15 ` Yong Shen 0 siblings, 1 reply; 8+ messages in thread From: Uwe Kleine-König @ 2010-12-02 10:57 UTC (permalink / raw) To: linux-arm-kernel On Thu, Dec 02, 2010 at 06:45:32PM +0800, Yong Shen wrote: > Hi there, > > Please give comments inline and use attached patch for testing. > > Thanks > Yong > > > From 377c680c35089aea7697397c42349189843eeaff Mon Sep 17 00:00:00 2001 > From: Yong Shen <yong.shen@linaro.org> > Date: Thu, 2 Dec 2010 14:59:00 +0800 > Subject: [PATCH 2/2] make mc13783 regulator code generic > > move some common functions and micros of mc13783 regulaor driver to s/micros/macros/ s/regulaor/regulator/ > a seperate file, which makes it possible for mc13892 to share code. s/seperate/separate/ (Note I havn't looked at the patch) Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] make mc13783 regulator code generic 2010-12-02 10:57 ` Uwe Kleine-König @ 2010-12-02 11:15 ` Yong Shen 0 siblings, 0 replies; 8+ messages in thread From: Yong Shen @ 2010-12-02 11:15 UTC (permalink / raw) To: linux-arm-kernel Sorry, I will resend them. 2010/12/2 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>: > On Thu, Dec 02, 2010 at 06:45:32PM +0800, Yong Shen wrote: >> Hi there, >> >> Please give comments inline and use attached patch for testing. >> >> Thanks >> Yong >> >> >> From 377c680c35089aea7697397c42349189843eeaff Mon Sep 17 00:00:00 2001 >> From: Yong Shen <yong.shen@linaro.org> >> Date: Thu, 2 Dec 2010 14:59:00 +0800 >> Subject: [PATCH 2/2] make mc13783 regulator code generic >> >> move some common functions and micros of mc13783 regulaor driver to > s/micros/macros/ s/regulaor/regulator/ > >> a seperate file, which makes it possible for mc13892 to share code. > s/seperate/separate/ > > (Note I havn't looked at the patch) > > Best regards > Uwe > > -- > Pengutronix e.K. ? ? ? ? ? ? ? ? ? ? ? ? ? | Uwe Kleine-K?nig ? ? ? ? ? ?| > Industrial Linux Solutions ? ? ? ? ? ? ? ? | http://www.pengutronix.de/ ?| > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2010-12-02 11:15 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-12-01 7:15 [PATCHv2] make mc13783 regulator code generic Yong Shen 2010-12-01 7:50 ` Sascha Hauer 2010-12-02 2:36 ` Yong Shen 2010-12-01 11:25 ` Mark Brown 2010-12-02 2:41 ` Yong Shen 2010-12-02 10:45 ` [PATCH 2/2] " Yong Shen 2010-12-02 10:57 ` Uwe Kleine-König 2010-12-02 11:15 ` Yong Shen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).