| Message ID | 1345619928-15446-5-git-send-email-b29396@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 22, 2012 at 03:18:45PM +0800, Dong Aisheng wrote: > From: Dong Aisheng <dong.aisheng@linaro.org> > > Using standard imx syscon API to access anatop register. Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com> With the conversion to regmap it'd also be good to convert the driver to use the regmap helper functions for enable and voltage operations if possible.
On 08/22/2012 01:18 AM, Dong Aisheng wrote: > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org> > diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c > @@ -109,7 +110,11 @@ static int __devinit anatop_regulator_probe(struct platform_device *pdev) > rdesc->ops = &anatop_rops; > rdesc->type = REGULATOR_VOLTAGE; > rdesc->owner = THIS_MODULE; > - sreg->mfd = anatopmfd; > + > + sreg->anatop = of_parse_phandle(np, "fsl,anatop", 0); > + if (!sreg->anatop) > + return -ENODEV; In fact, that imx_syscon_lookup function I proposed could even do the of_parse_phandle() internally, so perhaps: foo->syscon_dev = imx_syscon_lookup(np, "fsl,anatop", 0); if (IS_ERR(foo->syscon_dev)) return PTR_ERR(foo->syscon_dev); with imx_syscon_lookup() internally knowing when to return EPROBE_DEFER rather than any other permanent error code (e.g. for missing property, bad phandle, etc.)
On Wed, Aug 22, 2012 at 11:21:03PM -0600, Stephen Warren wrote: > On 08/22/2012 01:18 AM, Dong Aisheng wrote: > > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org> > > > diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c > > > @@ -109,7 +110,11 @@ static int __devinit anatop_regulator_probe(struct platform_device *pdev) > > rdesc->ops = &anatop_rops; > > rdesc->type = REGULATOR_VOLTAGE; > > rdesc->owner = THIS_MODULE; > > - sreg->mfd = anatopmfd; > > + > > + sreg->anatop = of_parse_phandle(np, "fsl,anatop", 0); > > + if (!sreg->anatop) > > + return -ENODEV; > > In fact, that imx_syscon_lookup function I proposed could even do the > of_parse_phandle() internally, so perhaps: > > foo->syscon_dev = imx_syscon_lookup(np, "fsl,anatop", 0); > if (IS_ERR(foo->syscon_dev)) > return PTR_ERR(foo->syscon_dev); > > with imx_syscon_lookup() internally knowing when to return EPROBE_DEFER > rather than any other permanent error code (e.g. for missing property, > bad phandle, etc.) In some case that we access register in machine code, we don't have any phandler. The node is got by find compatible or by path. Thanks Richard
On Wed, Aug 22, 2012 at 11:59:53PM +0800, Mark Brown wrote: > On Wed, Aug 22, 2012 at 03:18:45PM +0800, Dong Aisheng wrote: > > From: Dong Aisheng <dong.aisheng@linaro.org> > > > > Using standard imx syscon API to access anatop register. > > Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com> > Thanks > With the conversion to regmap it'd also be good to convert the driver to > use the regmap helper functions for enable and voltage operations if > possible. The anatop-regulator driver only implements set_voltage_sel/get_voltage_sel which i have already converted, what do you mean others like 'for enable and voltage operations' i should also convert? Regards Dong Aisheng
On Thu, Aug 23, 2012 at 01:21:03PM +0800, Stephen Warren wrote: > On 08/22/2012 01:18 AM, Dong Aisheng wrote: > > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org> > > > diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c > > > @@ -109,7 +110,11 @@ static int __devinit anatop_regulator_probe(struct platform_device *pdev) > > rdesc->ops = &anatop_rops; > > rdesc->type = REGULATOR_VOLTAGE; > > rdesc->owner = THIS_MODULE; > > - sreg->mfd = anatopmfd; > > + > > + sreg->anatop = of_parse_phandle(np, "fsl,anatop", 0); > > + if (!sreg->anatop) > > + return -ENODEV; > > In fact, that imx_syscon_lookup function I proposed could even do the > of_parse_phandle() internally, so perhaps: > > foo->syscon_dev = imx_syscon_lookup(np, "fsl,anatop", 0); > if (IS_ERR(foo->syscon_dev)) > return PTR_ERR(foo->syscon_dev); > > with imx_syscon_lookup() internally knowing when to return EPROBE_DEFER > rather than any other permanent error code (e.g. for missing property, > bad phandle, etc.) > This also looks reasonable to me. btw, see my last reply to mark in another mail. I'm not sure but Mark may want something slightly different as this one, imx_syscon_lookup directly return regmap rather than dev, then we do not need implement any register read/write API in imx-syscon driver, just using the exist regmap r/w API is ok. Regards Dong Aisheng
On Thu, Aug 23, 2012 at 03:15:04PM +0800, Dong Aisheng wrote: > On Wed, Aug 22, 2012 at 11:59:53PM +0800, Mark Brown wrote: > > With the conversion to regmap it'd also be good to convert the driver to > > use the regmap helper functions for enable and voltage operations if > > possible. > The anatop-regulator driver only implements set_voltage_sel/get_voltage_sel > which i have already converted, what do you mean others like > 'for enable and voltage operations' i should also convert? Those operations should ideally be converted to use the generic regmap implementation now the device uses regmap - regmap_get_voltage_sel_regmap and so on.
On 08/23/2012 12:12 AM, Richard Zhao wrote: > On Wed, Aug 22, 2012 at 11:21:03PM -0600, Stephen Warren wrote: >> On 08/22/2012 01:18 AM, Dong Aisheng wrote: >>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org> >> >>> diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c >> >>> @@ -109,7 +110,11 @@ static int __devinit anatop_regulator_probe(struct platform_device *pdev) >>> rdesc->ops = &anatop_rops; >>> rdesc->type = REGULATOR_VOLTAGE; >>> rdesc->owner = THIS_MODULE; >>> - sreg->mfd = anatopmfd; >>> + >>> + sreg->anatop = of_parse_phandle(np, "fsl,anatop", 0); >>> + if (!sreg->anatop) >>> + return -ENODEV; >> >> In fact, that imx_syscon_lookup function I proposed could even do the >> of_parse_phandle() internally, so perhaps: >> >> foo->syscon_dev = imx_syscon_lookup(np, "fsl,anatop", 0); >> if (IS_ERR(foo->syscon_dev)) >> return PTR_ERR(foo->syscon_dev); >> >> with imx_syscon_lookup() internally knowing when to return EPROBE_DEFER >> rather than any other permanent error code (e.g. for missing property, >> bad phandle, etc.) > > In some case that we access register in machine code, we don't have any > phandle. The node is got by find compatible or by path. That sounds a little odd; why not just use a phandle consistently everywhere? Either way though, I could imagine still putting all the lookup code into the syscon driver; just have different functions for the different lookup methods: imx_syscon_lookup_by_phandle(np, char *property_name) imx_syscon_lookup_by_compatible(char *compatible imx_syscon_lookup_by_path(char *node_path)
On Thu, Aug 23, 2012 at 07:17:41PM +0800, Mark Brown wrote: > On Thu, Aug 23, 2012 at 03:15:04PM +0800, Dong Aisheng wrote: > > On Wed, Aug 22, 2012 at 11:59:53PM +0800, Mark Brown wrote: > > > > With the conversion to regmap it'd also be good to convert the driver to > > > use the regmap helper functions for enable and voltage operations if > > > possible. > > > The anatop-regulator driver only implements set_voltage_sel/get_voltage_sel > > which i have already converted, what do you mean others like > > 'for enable and voltage operations' i should also convert? > > Those operations should ideally be converted to use the generic regmap > implementation now the device uses regmap - regmap_get_voltage_sel_regmap > and so on. Okay, got it. Regards Dong Aisheng
On Fri, Aug 24, 2012 at 01:56:58AM +0800, Stephen Warren wrote: > On 08/23/2012 12:12 AM, Richard Zhao wrote: > > On Wed, Aug 22, 2012 at 11:21:03PM -0600, Stephen Warren wrote: > >> On 08/22/2012 01:18 AM, Dong Aisheng wrote: > >>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org> > >> > >>> diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c > >> > >>> @@ -109,7 +110,11 @@ static int __devinit anatop_regulator_probe(struct platform_device *pdev) > >>> rdesc->ops = &anatop_rops; > >>> rdesc->type = REGULATOR_VOLTAGE; > >>> rdesc->owner = THIS_MODULE; > >>> - sreg->mfd = anatopmfd; > >>> + > >>> + sreg->anatop = of_parse_phandle(np, "fsl,anatop", 0); > >>> + if (!sreg->anatop) > >>> + return -ENODEV; > >> > >> In fact, that imx_syscon_lookup function I proposed could even do the > >> of_parse_phandle() internally, so perhaps: > >> > >> foo->syscon_dev = imx_syscon_lookup(np, "fsl,anatop", 0); > >> if (IS_ERR(foo->syscon_dev)) > >> return PTR_ERR(foo->syscon_dev); > >> > >> with imx_syscon_lookup() internally knowing when to return EPROBE_DEFER > >> rather than any other permanent error code (e.g. for missing property, > >> bad phandle, etc.) > > > > In some case that we access register in machine code, we don't have any > > phandle. The node is got by find compatible or by path. > > That sounds a little odd; why not just use a phandle consistently > everywhere? Maybe for some places we do not have that device node, e.g: arch/arm/mach-imx/mach-imx6q.c > > Either way though, I could imagine still putting all the lookup code > into the syscon driver; just have different functions for the different > lookup methods: > > imx_syscon_lookup_by_phandle(np, char *property_name) Probably we do not need the left two lookup, below seems also ok if needed: np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-anatop"); regmap = imx_syscon_lookup_by_phandle(np, property_name) Then we do not need to handle how to find the compatible node. > imx_syscon_lookup_by_compatible(char *compatible > imx_syscon_lookup_by_path(char *node_path) Regards Dong Aisheng
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi index 7732b70..7076be0 100644 --- a/arch/arm/boot/dts/imx6q.dtsi +++ b/arch/arm/boot/dts/imx6q.dtsi @@ -395,6 +395,7 @@ anatop-min-bit-val = <4>; anatop-min-voltage = <800000>; anatop-max-voltage = <1375000>; + fsl,anatop = <&anatop>; }; regulator-3p0@120 { @@ -409,6 +410,7 @@ anatop-min-bit-val = <0>; anatop-min-voltage = <2625000>; anatop-max-voltage = <3400000>; + fsl,anatop = <&anatop>; }; regulator-2p5@130 { @@ -423,6 +425,7 @@ anatop-min-bit-val = <0>; anatop-min-voltage = <2000000>; anatop-max-voltage = <2750000>; + fsl,anatop = <&anatop>; }; regulator-vddcore@140 { @@ -437,6 +440,7 @@ anatop-min-bit-val = <1>; anatop-min-voltage = <725000>; anatop-max-voltage = <1450000>; + fsl,anatop = <&anatop>; }; regulator-vddpu@140 { @@ -451,6 +455,7 @@ anatop-min-bit-val = <1>; anatop-min-voltage = <725000>; anatop-max-voltage = <1450000>; + fsl,anatop = <&anatop>; }; regulator-vddsoc@140 { @@ -465,6 +470,7 @@ anatop-min-bit-val = <1>; anatop-min-voltage = <725000>; anatop-max-voltage = <1450000>; + fsl,anatop = <&anatop>; }; }; diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig index 4e932cc..7ceea1a 100644 --- a/drivers/regulator/Kconfig +++ b/drivers/regulator/Kconfig @@ -112,7 +112,7 @@ config REGULATOR_DA9052 config REGULATOR_ANATOP tristate "Freescale i.MX on-chip ANATOP LDO regulators" - depends on MFD_ANATOP + depends on MFD_IMX_SYSCON help Say y here to support Freescale i.MX on-chip ANATOP LDOs regulators. It is recommended that this option be diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c index ce0fe72..e0b9ef1 100644 --- a/drivers/regulator/anatop-regulator.c +++ b/drivers/regulator/anatop-regulator.c @@ -21,19 +21,20 @@ #include <linux/slab.h> #include <linux/device.h> #include <linux/module.h> +#include <linux/mfd/imx-syscon.h> #include <linux/err.h> #include <linux/io.h> #include <linux/platform_device.h> #include <linux/of.h> #include <linux/of_address.h> -#include <linux/mfd/anatop.h> +#include <linux/regmap.h> #include <linux/regulator/driver.h> #include <linux/regulator/of_regulator.h> struct anatop_regulator { const char *name; u32 control_reg; - struct anatop *mfd; + struct device_node *anatop; int vol_bit_shift; int vol_bit_width; int min_bit_val; @@ -56,7 +57,8 @@ static int anatop_set_voltage_sel(struct regulator_dev *reg, unsigned selector) mask = ((1 << anatop_reg->vol_bit_width) - 1) << anatop_reg->vol_bit_shift; val <<= anatop_reg->vol_bit_shift; - anatop_write_reg(anatop_reg->mfd, anatop_reg->control_reg, val, mask); + imx_syscon_update_bits(anatop_reg->anatop, anatop_reg->control_reg, + mask, val); return 0; } @@ -69,7 +71,7 @@ static int anatop_get_voltage_sel(struct regulator_dev *reg) if (!anatop_reg->control_reg) return -ENOTSUPP; - val = anatop_read_reg(anatop_reg->mfd, anatop_reg->control_reg); + imx_syscon_read(anatop_reg->anatop, anatop_reg->control_reg, &val); mask = ((1 << anatop_reg->vol_bit_width) - 1) << anatop_reg->vol_bit_shift; val = (val & mask) >> anatop_reg->vol_bit_shift; @@ -92,7 +94,6 @@ static int __devinit anatop_regulator_probe(struct platform_device *pdev) struct regulator_dev *rdev; struct anatop_regulator *sreg; struct regulator_init_data *initdata; - struct anatop *anatopmfd = dev_get_drvdata(pdev->dev.parent); struct regulator_config config = { }; int ret = 0; @@ -109,7 +110,11 @@ static int __devinit anatop_regulator_probe(struct platform_device *pdev) rdesc->ops = &anatop_rops; rdesc->type = REGULATOR_VOLTAGE; rdesc->owner = THIS_MODULE; - sreg->mfd = anatopmfd; + + sreg->anatop = of_parse_phandle(np, "fsl,anatop", 0); + if (!sreg->anatop) + return -ENODEV; + ret = of_property_read_u32(np, "anatop-reg-offset", &sreg->control_reg); if (ret) {