* [PATCH 10/10] mmp: append device support in jasper @ 2010-04-28 12:23 Haojian Zhuang 2010-04-28 13:11 ` Eric Miao 2010-04-28 13:52 ` Mark Brown 0 siblings, 2 replies; 7+ messages in thread From: Haojian Zhuang @ 2010-04-28 12:23 UTC (permalink / raw) To: linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 10/10] mmp: append device support in jasper 2010-04-28 12:23 [PATCH 10/10] mmp: append device support in jasper Haojian Zhuang @ 2010-04-28 13:11 ` Eric Miao 2010-04-28 13:41 ` Haojian Zhuang 2010-04-28 13:52 ` Mark Brown 1 sibling, 1 reply; 7+ messages in thread From: Eric Miao @ 2010-04-28 13:11 UTC (permalink / raw) To: linux-arm-kernel On Wed, Apr 28, 2010 at 8:23 PM, Haojian Zhuang <haojian.zhuang@gmail.com> wrote: > From cf52f98eec9a1fb6042ba800b92cc999108b463a Mon Sep 17 00:00:00 2001 > From: Haojian Zhuang <haojian.zhuang@marvell.com> > Date: Wed, 28 Apr 2010 15:43:21 -0400 > Subject: [PATCH] [ARM] mmp: append device support in jasper > > Support regulator MAX8649, PMIC MAX8925 into the Jasper. > Backlight, regulator & rtc components of MAX8925 are enabled in Jasper. > > Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com> Applied. Haojian, Please check my 'mmp2' branch and let me if there are any problems. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 10/10] mmp: append device support in jasper 2010-04-28 13:11 ` Eric Miao @ 2010-04-28 13:41 ` Haojian Zhuang 0 siblings, 0 replies; 7+ messages in thread From: Haojian Zhuang @ 2010-04-28 13:41 UTC (permalink / raw) To: linux-arm-kernel On Wed, Apr 28, 2010 at 9:11 AM, Eric Miao <eric.y.miao@gmail.com> wrote: > On Wed, Apr 28, 2010 at 8:23 PM, Haojian Zhuang > <haojian.zhuang@gmail.com> wrote: >> From cf52f98eec9a1fb6042ba800b92cc999108b463a Mon Sep 17 00:00:00 2001 >> From: Haojian Zhuang <haojian.zhuang@marvell.com> >> Date: Wed, 28 Apr 2010 15:43:21 -0400 >> Subject: [PATCH] [ARM] mmp: append device support in jasper >> >> Support regulator MAX8649, PMIC MAX8925 into the Jasper. >> Backlight, regulator & rtc components of MAX8925 are enabled in Jasper. >> >> Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com> > > Applied. > > Haojian, > > Please check my 'mmp2' branch and let me if there are any problems. > Everything is OK. It works well. Thanks Haojian ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 10/10] mmp: append device support in jasper 2010-04-28 12:23 [PATCH 10/10] mmp: append device support in jasper Haojian Zhuang 2010-04-28 13:11 ` Eric Miao @ 2010-04-28 13:52 ` Mark Brown 2010-04-29 3:05 ` Haojian Zhuang 1 sibling, 1 reply; 7+ messages in thread From: Mark Brown @ 2010-04-28 13:52 UTC (permalink / raw) To: linux-arm-kernel On Wed, Apr 28, 2010 at 08:23:24AM -0400, Haojian Zhuang wrote: > +static struct regulator_consumer_supply regulator_supply[] = { You shouldn't have all your consumer supplies in one big array, each regulator should have its own set of supplies for the devices connected to it. > + [MAX8925_ID_SD1] = REGULATOR_SUPPLY("v_sd1", NULL), > + [MAX8925_ID_SD2] = REGULATOR_SUPPLY("v_sd2", NULL), > + [MAX8925_ID_SD3] = REGULATOR_SUPPLY("v_sd3", NULL), > + [MAX8925_ID_LDO1] = REGULATOR_SUPPLY("v_ldo1", NULL), > + [MAX8925_ID_LDO2] = REGULATOR_SUPPLY("v_ldo2", NULL), None of these supplies should be being defined at all - the supplies from regulators are for hooking up individual device supplies to the regulators, you should only have supplies with null devices in exceptional cases like CPUfreq where no usable struct device exists. I'm fairly sure this has been pointed out with regard to previous machines you have submitted regulator support for, it is disappointing to see the same issue coming up again. > +#define REG_INIT(_name, _min, _max, _always, _boot) \ > +{ \ > + .constraints = { \ > + .name = __stringify(_name), \ > + .min_uV = _min, \ > + .max_uV = _max, \ > + .always_on = _always, \ > + .boot_on = _boot, \ > + .valid_ops_mask = REGULATOR_CHANGE_VOLTAGE, \ > + }, \ > + .num_consumer_supplies = 1, \ > + .consumer_supplies = ®ulator_supply[MAX8925_ID_##_name], \ This macro shouldn't be assuming that there are devices being supplied, and definitely needs to be able to cope with more than one regulator using the supply. > +static struct regulator_init_data regulator_data[] = { > + [MAX8925_ID_SD1] = REG_INIT(SD1, 637500, 1425000, 0, 0), > + [MAX8925_ID_SD2] = REG_INIT(SD2, 650000, 2225000, 1, 1), > + [MAX8925_ID_SD3] = REG_INIT(SD3, 750000, 3900000, 1, 1), > + [MAX8925_ID_LDO1] = REG_INIT(LDO1, 750000, 3900000, 1, 1), > + [MAX8925_ID_LDO2] = REG_INIT(LDO2, 650000, 2250000, 1, 1), Have all the voltage ranges you're specifying here been audited against the board design? It would be very unusual for every single supply on the board have such wide voltage ranges - it looks awfully like the ranges here are the supported ranges for the regulators themselves. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 10/10] mmp: append device support in jasper 2010-04-28 13:52 ` Mark Brown @ 2010-04-29 3:05 ` Haojian Zhuang 2010-04-29 4:01 ` Eric Miao 2010-04-29 9:17 ` Mark Brown 0 siblings, 2 replies; 7+ messages in thread From: Haojian Zhuang @ 2010-04-29 3:05 UTC (permalink / raw) To: linux-arm-kernel On Wed, Apr 28, 2010 at 9:52 AM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Wed, Apr 28, 2010 at 08:23:24AM -0400, Haojian Zhuang wrote: > >> +static struct regulator_consumer_supply regulator_supply[] = { > > You shouldn't have all your consumer supplies in one big array, each > regulator should have its own set of supplies for the devices connected > to it. > >> + ? ? [MAX8925_ID_SD1] ? ? ? ?= REGULATOR_SUPPLY("v_sd1", NULL), >> + ? ? [MAX8925_ID_SD2] ? ? ? ?= REGULATOR_SUPPLY("v_sd2", NULL), >> + ? ? [MAX8925_ID_SD3] ? ? ? ?= REGULATOR_SUPPLY("v_sd3", NULL), >> + ? ? [MAX8925_ID_LDO1] ? ? ? = REGULATOR_SUPPLY("v_ldo1", NULL), >> + ? ? [MAX8925_ID_LDO2] ? ? ? = REGULATOR_SUPPLY("v_ldo2", NULL), > > None of these supplies should be being defined at all - the supplies > from regulators are for hooking up individual device supplies to the > regulators, you should only have supplies with null devices in > exceptional cases like CPUfreq where no usable struct device exists. > > I'm fairly sure this has been pointed out with regard to previous > machines you have submitted regulator support for, it is disappointing > to see the same issue coming up again. > >> +#define REG_INIT(_name, _min, _max, _always, _boot) ? ? ? ? ?\ >> +{ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\ >> + ? ? .constraints = { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\ >> + ? ? ? ? ? ? .name ? ? ? ? ? = __stringify(_name), ? ? ? ? ? \ >> + ? ? ? ? ? ? .min_uV ? ? ? ? = _min, ? ? ? ? ? ? ? ? ? ? ? ? \ >> + ? ? ? ? ? ? .max_uV ? ? ? ? = _max, ? ? ? ? ? ? ? ? ? ? ? ? \ >> + ? ? ? ? ? ? .always_on ? ? ?= _always, ? ? ? ? ? ? ? ? ? ? ?\ >> + ? ? ? ? ? ? .boot_on ? ? ? ?= _boot, ? ? ? ? ? ? ? ? ? ? ? ?\ >> + ? ? ? ? ? ? .valid_ops_mask = REGULATOR_CHANGE_VOLTAGE, ? ? \ >> + ? ? }, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\ >> + ? ? .num_consumer_supplies ?= 1, ? ? ? ? ? ? ? ? ? ? ? ? ? ?\ >> + ? ? .consumer_supplies ? ? ?= ®ulator_supply[MAX8925_ID_##_name], \ > > This macro shouldn't be assuming that there are devices being supplied, > and definitely needs to be able to cope with more than one regulator > using the supply. > >> +static struct regulator_init_data regulator_data[] = { >> + ? ? [MAX8925_ID_SD1] = REG_INIT(SD1, 637500, 1425000, 0, 0), >> + ? ? [MAX8925_ID_SD2] = REG_INIT(SD2, 650000, 2225000, 1, 1), >> + ? ? [MAX8925_ID_SD3] = REG_INIT(SD3, 750000, 3900000, 1, 1), >> + ? ? [MAX8925_ID_LDO1] = REG_INIT(LDO1, 750000, 3900000, 1, 1), >> + ? ? [MAX8925_ID_LDO2] = REG_INIT(LDO2, 650000, 2250000, 1, 1), > > Have all the voltage ranges you're specifying here been audited against > the board design? ?It would be very unusual for every single supply on > the board have such wide voltage ranges - it looks awfully like the > ranges here are the supported ranges for the regulators themselves. > OK. Update this patch by removing regulators in max8925. They'll be contained in later patches. Whatever I think a big array is suitable for unused regulators. Is it right? Thanks Haojian -------------- next part -------------- A non-text attachment was scrubbed... Name: 0010--ARM-mmp-append-device-support-in-jasper.patch Type: text/x-patch Size: 3053 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100428/7006a778/attachment-0001.bin> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 10/10] mmp: append device support in jasper 2010-04-29 3:05 ` Haojian Zhuang @ 2010-04-29 4:01 ` Eric Miao 2010-04-29 9:17 ` Mark Brown 1 sibling, 0 replies; 7+ messages in thread From: Eric Miao @ 2010-04-29 4:01 UTC (permalink / raw) To: linux-arm-kernel On Thu, Apr 29, 2010 at 11:05 AM, Haojian Zhuang <haojian.zhuang@gmail.com> wrote: > On Wed, Apr 28, 2010 at 9:52 AM, Mark Brown > <broonie@opensource.wolfsonmicro.com> wrote: >> On Wed, Apr 28, 2010 at 08:23:24AM -0400, Haojian Zhuang wrote: >> >>> +static struct regulator_consumer_supply regulator_supply[] = { >> >> You shouldn't have all your consumer supplies in one big array, each >> regulator should have its own set of supplies for the devices connected >> to it. >> >>> + ? ? [MAX8925_ID_SD1] ? ? ? ?= REGULATOR_SUPPLY("v_sd1", NULL), >>> + ? ? [MAX8925_ID_SD2] ? ? ? ?= REGULATOR_SUPPLY("v_sd2", NULL), >>> + ? ? [MAX8925_ID_SD3] ? ? ? ?= REGULATOR_SUPPLY("v_sd3", NULL), >>> + ? ? [MAX8925_ID_LDO1] ? ? ? = REGULATOR_SUPPLY("v_ldo1", NULL), >>> + ? ? [MAX8925_ID_LDO2] ? ? ? = REGULATOR_SUPPLY("v_ldo2", NULL), >> >> None of these supplies should be being defined at all - the supplies >> from regulators are for hooking up individual device supplies to the >> regulators, you should only have supplies with null devices in >> exceptional cases like CPUfreq where no usable struct device exists. >> >> I'm fairly sure this has been pointed out with regard to previous >> machines you have submitted regulator support for, it is disappointing >> to see the same issue coming up again. >> >>> +#define REG_INIT(_name, _min, _max, _always, _boot) ? ? ? ? ?\ >>> +{ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\ >>> + ? ? .constraints = { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\ >>> + ? ? ? ? ? ? .name ? ? ? ? ? = __stringify(_name), ? ? ? ? ? \ >>> + ? ? ? ? ? ? .min_uV ? ? ? ? = _min, ? ? ? ? ? ? ? ? ? ? ? ? \ >>> + ? ? ? ? ? ? .max_uV ? ? ? ? = _max, ? ? ? ? ? ? ? ? ? ? ? ? \ >>> + ? ? ? ? ? ? .always_on ? ? ?= _always, ? ? ? ? ? ? ? ? ? ? ?\ >>> + ? ? ? ? ? ? .boot_on ? ? ? ?= _boot, ? ? ? ? ? ? ? ? ? ? ? ?\ >>> + ? ? ? ? ? ? .valid_ops_mask = REGULATOR_CHANGE_VOLTAGE, ? ? \ >>> + ? ? }, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\ >>> + ? ? .num_consumer_supplies ?= 1, ? ? ? ? ? ? ? ? ? ? ? ? ? ?\ >>> + ? ? .consumer_supplies ? ? ?= ®ulator_supply[MAX8925_ID_##_name], \ >> >> This macro shouldn't be assuming that there are devices being supplied, >> and definitely needs to be able to cope with more than one regulator >> using the supply. >> >>> +static struct regulator_init_data regulator_data[] = { >>> + ? ? [MAX8925_ID_SD1] = REG_INIT(SD1, 637500, 1425000, 0, 0), >>> + ? ? [MAX8925_ID_SD2] = REG_INIT(SD2, 650000, 2225000, 1, 1), >>> + ? ? [MAX8925_ID_SD3] = REG_INIT(SD3, 750000, 3900000, 1, 1), >>> + ? ? [MAX8925_ID_LDO1] = REG_INIT(LDO1, 750000, 3900000, 1, 1), >>> + ? ? [MAX8925_ID_LDO2] = REG_INIT(LDO2, 650000, 2250000, 1, 1), >> >> Have all the voltage ranges you're specifying here been audited against >> the board design? ?It would be very unusual for every single supply on >> the board have such wide voltage ranges - it looks awfully like the >> ranges here are the supported ranges for the regulators themselves. >> > > OK. Update this patch by removing regulators in max8925. They'll be > contained in later patches. Whatever I think a big array is suitable > for unused regulators. Is it right? > Mark, I need your Ack on this. Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 10/10] mmp: append device support in jasper 2010-04-29 3:05 ` Haojian Zhuang 2010-04-29 4:01 ` Eric Miao @ 2010-04-29 9:17 ` Mark Brown 1 sibling, 0 replies; 7+ messages in thread From: Mark Brown @ 2010-04-29 9:17 UTC (permalink / raw) To: linux-arm-kernel On Wed, Apr 28, 2010 at 11:05:13PM -0400, Haojian Zhuang wrote: > OK. Update this patch by removing regulators in max8925. They'll be Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com> > contained in later patches. Whatever I think a big array is suitable > for unused regulators. Is it right? No, not at all. If the regulator has no consumers then there should not be any supplies set up from it. The sole purpose of supplies is to connect the consumers and regulators to each other so if the regulator is unused there should be no supplies defined at all. This is what I was saying about supplies with NULL devices being very suspicious. > +static struct regulator_consumer_supply max8649_supply[] = { > + REGULATOR_SUPPLY("vcc_core", NULL), > +}; > + > +static struct regulator_init_data max8649_init_data = { > + .constraints = { > + .name = "vcc_core range", You probably just want to call this vcc_core (the name is used when logging about the supply and provided to userspace so you want something that would make sense in a log message). It doesn't really matter, though. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-04-29 9:17 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-04-28 12:23 [PATCH 10/10] mmp: append device support in jasper Haojian Zhuang 2010-04-28 13:11 ` Eric Miao 2010-04-28 13:41 ` Haojian Zhuang 2010-04-28 13:52 ` Mark Brown 2010-04-29 3:05 ` Haojian Zhuang 2010-04-29 4:01 ` Eric Miao 2010-04-29 9:17 ` Mark Brown
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).