| Message ID | 1241795526-15667-1-git-send-email-anuj.aggarwal@ti.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
On Fri, May 08, 2009 at 08:42:06PM +0530, Anuj Aggarwal wrote: > Added drivers\regulator\pmic.c file to the regulator framework which will I suspect you mean / there :) > have the board specific information for different regulators and will do the > regulator initialization depending on one which is available. > Signed-off-by: Anuj Aggarwal <anuj.aggarwal@ti.com> I'm really not sure what the abstraction that you're attempting to introduce here is intended to do that can't handled by the existing kernel features - could you go into more detail about the problem that this patch is intended to solve, please? The kernel already provides facilities for detecting the presence of devices. In cases where it is possible to do a generic check for a device (for example, by reading back ID registers on the device and checking the values) then the device driver can do so in its probe routine and the board can unconditionally declare that the device is there, allowing the probe to fail. In cases where something board specific needs to be done (such as checking for fit resistors or information provided by the bootloader) then that'd need to be handled in the board code anyway - it's not going to apply on generic systems. There will also be cases where it's just not possible to determine what's fitted at runtime, especially for simple devices with GPIO only control. If there is initialisation which can always be done for a chip no matter what board it's on then I'd expect the driver to just handle that when it is starting up without needing separate code - if there's something preventing that then we should definitely fix that, probably in a way that's not specific to the regulator framework since I'd expect other subsystems to be running into similar issues. Another thing here is that the code appears to assume that there will be only one regulator device in the system which isn't always true. Some designs will use a series of discrete regulators rather than integrated chips while others will use several integrated PMICs for reasons such as ease of layout or power distribution. Based on experience with the OMAP audio drivers I suspect the problem the patch is trying to solve may be that there are a lot of designs out there which are based on the reference platforms which TI have produced and are therefore have very repetitive board code. Perhaps this might be best handled by having some utility routines in the OMAP code which the machine drivers can use to say that they have the same PMIC setup as a given reference design? CCing in Liam so I've not deleted any of the text. > --- > drivers/regulator/Makefile | 2 +- > drivers/regulator/pmic.c | 103 ++++++++++++++++++++++++++++++++++++++++ > include/linux/regulator/pmic.h | 29 +++++++++++ > 3 files changed, 133 insertions(+), 1 deletions(-) > create mode 100644 drivers/regulator/pmic.c > create mode 100644 include/linux/regulator/pmic.h > > diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile > index bac133a..c0d87bf 100644 > --- a/drivers/regulator/Makefile > +++ b/drivers/regulator/Makefile > @@ -3,7 +3,7 @@ > # > > > -obj-$(CONFIG_REGULATOR) += core.o > +obj-$(CONFIG_REGULATOR) += core.o pmic.o > obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o > obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o > > diff --git a/drivers/regulator/pmic.c b/drivers/regulator/pmic.c > new file mode 100644 > index 0000000..36ed341 > --- /dev/null > +++ b/drivers/regulator/pmic.c > @@ -0,0 +1,103 @@ > +/* > + * pmic.c > + * > + * Supports run-time detection of different Power Management ICs. > + * > + * Copyright (C) 2009 Texas Instrument Incorporated - http://www.ti.com/ > + * > + * 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 version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind, > + * whether express or implied; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * General Public License for more details. > + */ > +#include <linux/regulator/driver.h> > +#include <linux/regulator/machine.h> > +#include <mach/common.h> > + > +/* > + * Definitions specific to TWL4030 > + */ > + > +/* > + * Definitions specific to TPS62350 > + */ > + > +/* > + * Definitions specific to TPS65023 > + */ > + > +static int flag_pmic_twl4030; > +static int flag_pmic_tps6235x; > +static int flag_pmic_tps65023; > + > +/* > + * Detect the current PMIC, set one of the flags > + */ > +static inline int detect_pmic(void) > +{ > + /* How? Any suggestions?? This is a temporary solution. */ > +#if defined(CONFIG_TWL4030_CORE) > + flag_pmic_twl4030 = 1; > +#endif > + > +#if defined(CONFIG_OMAP3EVM_TPS6235X) > + flag_pmic_tps6235x = 1; > +#endif > + > +#if defined(CONFIG_OMAP3EVM_TPS65023) > + flag_pmic_tps65023 = 1; > +#endif > + > + return 0; > +} > + > +/* Functions to detect which PMIC is present */ > + > +int pmic_is_twl4030(void) > +{ > + return flag_pmic_twl4030; > +} > + > +int pmic_is_tps6235x(void) > +{ > + return flag_pmic_tps6235x; > +} > + > +int pmic_is_tps65020(void) { return 0; } > + > +int pmic_is_tps65021(void) { return 0; } > + > +int pmic_is_tps65022(void) { return 0; } > + > +int pmic_is_tps65023(void) > +{ > + return flag_pmic_tps65023; > +} > + > +int pmic_is_tps65950(void) > +{ > + return flag_pmic_twl4030; > +} > + > +/* Detects the PMIC and initializes it accordingly */ > +int pmic_init(void) > +{ > +#if defined(CONFIG_TWL4030_CORE) > + /* do stuff specific to TWL4030 */ > +#endif > + > +#if defined(CONFIG_OMAP3EVM_TPS6235X) > + /* do stuff specific to TPS62350 */ > +#endif > + > +#if defined(CONFIG_OMAP3EVM_TPS65023) > + /* do stuff specific to TPS65023 */ > +#endif > + > + return 0; > +} > + > diff --git a/include/linux/regulator/pmic.h b/include/linux/regulator/pmic.h > new file mode 100644 > index 0000000..5956740 > --- /dev/null > +++ b/include/linux/regulator/pmic.h > @@ -0,0 +1,29 @@ > +/* > + * pmic.h > + * > + * Supports run-time detection of different Power Management ICs. > + * > + * Copyright (C) 2009 Texas Instrument Incorporated http://www.ti.com/ > + * > + * 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 version 2. > + * > + * This program is distributed as is WITHOUT ANY WARRANTY of any kind, whether > + * express or implied; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + */ > + > +/* Functions to detect which PMIC is present */ > +int pmic_is_twl4030(void); > +int pmic_is_tps6235x(void); > +int pmic_is_tps65020(void); > +int pmic_is_tps65021(void); > +int pmic_is_tps65022(void); > +int pmic_is_tps65023(void); > +int pmic_is_tps65950(void); > + > +/* Detects the PMIC and initializes it accordingly */ > +int pmic_init(void); > + > -- > 1.6.2.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
> -----Original Message----- > From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] > Sent: Saturday, May 09, 2009 12:37 AM > To: Aggarwal, Anuj > Cc: linux-omap@vger.kernel.org > Subject: Re: [PATCH 1/3] Regulator: Add pmic.c file to regulator framework > > On Fri, May 08, 2009 at 08:42:06PM +0530, Anuj Aggarwal wrote: > > > Added drivers\regulator\pmic.c file to the regulator framework which will > > I suspect you mean / there :) [Aggarwal, Anuj] Yeah:) > > > have the board specific information for different regulators and will do the > > regulator initialization depending on one which is available. > > > Signed-off-by: Anuj Aggarwal <anuj.aggarwal@ti.com> > > I'm really not sure what the abstraction that you're attempting to > introduce here is intended to do that can't handled by the existing > kernel features - could you go into more detail about the problem that > this patch is intended to solve, please? > > The kernel already provides facilities for detecting the presence of > devices. In cases where it is possible to do a generic check for a > device (for example, by reading back ID registers on the device and > checking the values) then the device driver can do so in its probe > routine and the board can unconditionally declare that the device is > there, allowing the probe to fail. In cases where something board > specific needs to be done (such as checking for fit resistors or > information provided by the bootloader) then that'd need to be handled > in the board code anyway - it's not going to apply on generic systems. > There will also be cases where it's just not possible to determine > what's fitted at runtime, especially for simple devices with GPIO only > control. [Aggarwal, Anuj] In my opinion, it would not be a good idea to probe for several PMICs, on all the buses present in the system. It can be simplified by probing only the ones which are relevant for this device. > > If there is initialisation which can always be done for a chip no matter > what board it's on then I'd expect the driver to just handle that when > it is starting up without needing separate code - if there's something > preventing that then we should definitely fix that, probably in a way > that's not specific to the regulator framework since I'd expect other > subsystems to be running into similar issues. > > Another thing here is that the code appears to assume that there will be > only one regulator device in the system which isn't always true. Some > designs will use a series of discrete regulators rather than integrated > chips while others will use several integrated PMICs for reasons such as > ease of layout or power distribution. [Aggarwal, Anuj] Can you explain this, I didn't get that much? > > Based on experience with the OMAP audio drivers I suspect the problem > the patch is trying to solve may be that there are a lot of designs out > there which are based on the reference platforms which TI have produced > and are therefore have very repetitive board code. Perhaps this might > be best handled by having some utility routines in the OMAP code which > the machine drivers can use to say that they have the same PMIC setup as > a given reference design? [Aggarwal, Anuj] You are right, the primary purpose of this patch is to avoid duplication of PMIC code, common to many OMAP platforms and I believe this is a simpler and cleaner approach. Instead of keeping this file in the drivers/regulator folder, we can move it to arch/arm/mach-omap2 folder. > > CCing in Liam so I've not deleted any of the text. > > > --- > > drivers/regulator/Makefile | 2 +- > > drivers/regulator/pmic.c | 103 > ++++++++++++++++++++++++++++++++++++++++ > > include/linux/regulator/pmic.h | 29 +++++++++++ > > 3 files changed, 133 insertions(+), 1 deletions(-) > > create mode 100644 drivers/regulator/pmic.c > > create mode 100644 include/linux/regulator/pmic.h > > > > diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile > > index bac133a..c0d87bf 100644 > > --- a/drivers/regulator/Makefile > > +++ b/drivers/regulator/Makefile > > @@ -3,7 +3,7 @@ > > # > > > > > > -obj-$(CONFIG_REGULATOR) += core.o > > +obj-$(CONFIG_REGULATOR) += core.o pmic.o > > obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o > > obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o > > > > diff --git a/drivers/regulator/pmic.c b/drivers/regulator/pmic.c > > new file mode 100644 > > index 0000000..36ed341 > > --- /dev/null > > +++ b/drivers/regulator/pmic.c > > @@ -0,0 +1,103 @@ > > +/* > > + * pmic.c > > + * > > + * Supports run-time detection of different Power Management ICs. > > + * > > + * Copyright (C) 2009 Texas Instrument Incorporated - http://www.ti.com/ > > + * > > + * 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 version 2. > > + * > > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > kind, > > + * whether express or implied; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > GNU > > + * General Public License for more details. > > + */ > > +#include <linux/regulator/driver.h> > > +#include <linux/regulator/machine.h> > > +#include <mach/common.h> > > + > > +/* > > + * Definitions specific to TWL4030 > > + */ > > + > > +/* > > + * Definitions specific to TPS62350 > > + */ > > + > > +/* > > + * Definitions specific to TPS65023 > > + */ > > + > > +static int flag_pmic_twl4030; > > +static int flag_pmic_tps6235x; > > +static int flag_pmic_tps65023; > > + > > +/* > > + * Detect the current PMIC, set one of the flags > > + */ > > +static inline int detect_pmic(void) > > +{ > > + /* How? Any suggestions?? This is a temporary solution. */ > > +#if defined(CONFIG_TWL4030_CORE) > > + flag_pmic_twl4030 = 1; > > +#endif > > + > > +#if defined(CONFIG_OMAP3EVM_TPS6235X) > > + flag_pmic_tps6235x = 1; > > +#endif > > + > > +#if defined(CONFIG_OMAP3EVM_TPS65023) > > + flag_pmic_tps65023 = 1; > > +#endif > > + > > + return 0; > > +} > > + > > +/* Functions to detect which PMIC is present */ > > + > > +int pmic_is_twl4030(void) > > +{ > > + return flag_pmic_twl4030; > > +} > > + > > +int pmic_is_tps6235x(void) > > +{ > > + return flag_pmic_tps6235x; > > +} > > + > > +int pmic_is_tps65020(void) { return 0; } > > + > > +int pmic_is_tps65021(void) { return 0; } > > + > > +int pmic_is_tps65022(void) { return 0; } > > + > > +int pmic_is_tps65023(void) > > +{ > > + return flag_pmic_tps65023; > > +} > > + > > +int pmic_is_tps65950(void) > > +{ > > + return flag_pmic_twl4030; > > +} > > + > > +/* Detects the PMIC and initializes it accordingly */ > > +int pmic_init(void) > > +{ > > +#if defined(CONFIG_TWL4030_CORE) > > + /* do stuff specific to TWL4030 */ > > +#endif > > + > > +#if defined(CONFIG_OMAP3EVM_TPS6235X) > > + /* do stuff specific to TPS62350 */ > > +#endif > > + > > +#if defined(CONFIG_OMAP3EVM_TPS65023) > > + /* do stuff specific to TPS65023 */ > > +#endif > > + > > + return 0; > > +} > > + > > diff --git a/include/linux/regulator/pmic.h b/include/linux/regulator/pmic.h > > new file mode 100644 > > index 0000000..5956740 > > --- /dev/null > > +++ b/include/linux/regulator/pmic.h > > @@ -0,0 +1,29 @@ > > +/* > > + * pmic.h > > + * > > + * Supports run-time detection of different Power Management ICs. > > + * > > + * Copyright (C) 2009 Texas Instrument Incorporated http://www.ti.com/ > > + * > > + * 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 version 2. > > + * > > + * This program is distributed as is WITHOUT ANY WARRANTY of any kind, > whether > > + * express or implied; without even the implied warranty of > MERCHANTABILITY or > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public > License for > > + * more details. > > + */ > > + > > +/* Functions to detect which PMIC is present */ > > +int pmic_is_twl4030(void); > > +int pmic_is_tps6235x(void); > > +int pmic_is_tps65020(void); > > +int pmic_is_tps65021(void); > > +int pmic_is_tps65022(void); > > +int pmic_is_tps65023(void); > > +int pmic_is_tps65950(void); > > + > > +/* Detects the PMIC and initializes it accordingly */ > > +int pmic_init(void); > > + > > -- > > 1.6.2.4 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > "You grabbed my hand and we fell into it, like a daydream - or a fever." -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 18, 2009 at 05:50:31PM +0530, Aggarwal, Anuj wrote: > > The kernel already provides facilities for detecting the presence of > > devices. In cases where it is possible to do a generic check for a > [Aggarwal, Anuj] In my opinion, it would not be a good idea to probe > for several PMICs, on all the buses present in the system. It can be > simplified by probing only the ones which are relevant for this device. The point here is that the kernel already provides for doing device enumeration of all kinds, both static and dynamic, and that adding this file doesn't add anything except copying of the code from arch/arm into drivers/regulator. > > Another thing here is that the code appears to assume that there will be > > only one regulator device in the system which isn't always true. Some > > designs will use a series of discrete regulators rather than integrated > > chips while others will use several integrated PMICs for reasons such as > > ease of layout or power distribution. > [Aggarwal, Anuj] Can you explain this, I didn't get that much? Instead of having a single PMIC with multiple regulator outputs a system may choose to have several regulators. One way this can happen is that some manufacturers produce simple discrete regulator chips which produce only a single output - typically several of these would be needed in a system. Another approach which cause this to come up some systems use is to have more than one PMIC, for example having one PMIC for the main application processor and a second PMIC providing supplies used by a coprocessor. This can avoid electrical issues by allowing shorter tracks for the regulated supplies and simplifying routing. > [Aggarwal, Anuj] You are right, the primary purpose of this patch is to > avoid duplication of PMIC code, common to many OMAP platforms and I believe this is a simpler and cleaner approach. > Instead of keeping this file in the drivers/regulator folder, we can move it to arch/arm/mach-omap2 folder. This sounds like a good approach. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile index bac133a..c0d87bf 100644 --- a/drivers/regulator/Makefile +++ b/drivers/regulator/Makefile @@ -3,7 +3,7 @@ # -obj-$(CONFIG_REGULATOR) += core.o +obj-$(CONFIG_REGULATOR) += core.o pmic.o obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o diff --git a/drivers/regulator/pmic.c b/drivers/regulator/pmic.c new file mode 100644 index 0000000..36ed341 --- /dev/null +++ b/drivers/regulator/pmic.c @@ -0,0 +1,103 @@ +/* + * pmic.c + * + * Supports run-time detection of different Power Management ICs. + * + * Copyright (C) 2009 Texas Instrument Incorporated - http://www.ti.com/ + * + * 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 version 2. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind, + * whether express or implied; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + */ +#include <linux/regulator/driver.h> +#include <linux/regulator/machine.h> +#include <mach/common.h> + +/* + * Definitions specific to TWL4030 + */ + +/* + * Definitions specific to TPS62350 + */ + +/* + * Definitions specific to TPS65023 + */ + +static int flag_pmic_twl4030; +static int flag_pmic_tps6235x; +static int flag_pmic_tps65023; + +/* + * Detect the current PMIC, set one of the flags + */ +static inline int detect_pmic(void) +{ + /* How? Any suggestions?? This is a temporary solution. */ +#if defined(CONFIG_TWL4030_CORE) + flag_pmic_twl4030 = 1; +#endif + +#if defined(CONFIG_OMAP3EVM_TPS6235X) + flag_pmic_tps6235x = 1; +#endif + +#if defined(CONFIG_OMAP3EVM_TPS65023) + flag_pmic_tps65023 = 1; +#endif + + return 0; +} + +/* Functions to detect which PMIC is present */ + +int pmic_is_twl4030(void) +{ + return flag_pmic_twl4030; +} + +int pmic_is_tps6235x(void) +{ + return flag_pmic_tps6235x; +} + +int pmic_is_tps65020(void) { return 0; } + +int pmic_is_tps65021(void) { return 0; } + +int pmic_is_tps65022(void) { return 0; } + +int pmic_is_tps65023(void) +{ + return flag_pmic_tps65023; +} + +int pmic_is_tps65950(void) +{ + return flag_pmic_twl4030; +} + +/* Detects the PMIC and initializes it accordingly */ +int pmic_init(void) +{ +#if defined(CONFIG_TWL4030_CORE) + /* do stuff specific to TWL4030 */ +#endif + +#if defined(CONFIG_OMAP3EVM_TPS6235X) + /* do stuff specific to TPS62350 */ +#endif + +#if defined(CONFIG_OMAP3EVM_TPS65023) + /* do stuff specific to TPS65023 */ +#endif + + return 0; +} + diff --git a/include/linux/regulator/pmic.h b/include/linux/regulator/pmic.h new file mode 100644 index 0000000..5956740 --- /dev/null +++ b/include/linux/regulator/pmic.h @@ -0,0 +1,29 @@ +/* + * pmic.h + * + * Supports run-time detection of different Power Management ICs. + * + * Copyright (C) 2009 Texas Instrument Incorporated http://www.ti.com/ + * + * 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 version 2. + * + * This program is distributed as is WITHOUT ANY WARRANTY of any kind, whether + * express or implied; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + */ + +/* Functions to detect which PMIC is present */ +int pmic_is_twl4030(void); +int pmic_is_tps6235x(void); +int pmic_is_tps65020(void); +int pmic_is_tps65021(void); +int pmic_is_tps65022(void); +int pmic_is_tps65023(void); +int pmic_is_tps65950(void); + +/* Detects the PMIC and initializes it accordingly */ +int pmic_init(void); +
Added drivers\regulator\pmic.c file to the regulator framework which will have the board specific information for different regulators and will do the regulator initialization depending on one which is available. Signed-off-by: Anuj Aggarwal <anuj.aggarwal@ti.com> --- drivers/regulator/Makefile | 2 +- drivers/regulator/pmic.c | 103 ++++++++++++++++++++++++++++++++++++++++ include/linux/regulator/pmic.h | 29 +++++++++++ 3 files changed, 133 insertions(+), 1 deletions(-) create mode 100644 drivers/regulator/pmic.c create mode 100644 include/linux/regulator/pmic.h -- 1.6.2.4 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html