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.s...@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.s...@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.s...@linaro.org>
> * Copyright 2009-2010 Pengutronix
> * Uwe Kleine-Koenig <u.kleine-koe...@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.s...@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 |
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
