Mayuresh, I will correct the sentence. Thanks. -----Original Message----- From: Mayuresh Janorkar [mailto:mayureshjanorkar@xxxxxxxxx] Sent: Thursday, April 14, 2011 3:59 PM To: Yen.Lin@xxxxxxxxxx Cc: broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx; olof@xxxxxxxxx; mike@xxxxxxxxxxxxxx; lrg@xxxxxxxxxxxxxxx; Stephen Warren; linux-tegra@xxxxxxxxxxxxxxx; Xin Xie; Yen Lin Subject: Re: [PATCH] ARM: regulator: tps6586x: add PFM/PWM options on SMs On Fri, Apr 15, 2011 at 3:29 AM, <Yen.Lin@xxxxxxxxxx> wrote: > > From: Yen Lin <yelin@xxxxxxxxxx> > > TPS6586x SM0, SM1 and SM2 port have 2 power switching modes: > - PWM only, or > - PMW-PFM auto mode > > Some of TPS6586x have voltage spike in PFM-to-FWM transition which can > lockup the CPU if choose PWM-PFM auto mode. Looks like some problem with grammar. If I understand it correctly you mean to say: "It can lockup CPU if PWM-PF auto mode is chosen" > > This patch enables such mode selection on SMs ports from the board > level power configuration file. > > Signed-off-by: Yen Lin <yelin@xxxxxxxxxx> > --- > drivers/regulator/tps6586x-regulator.c | 46 > ++++++++++++++++++++++++++++++++ > include/linux/mfd/tps6586x.h | 15 ++++++++++ > 2 files changed, 61 insertions(+), 0 deletions(-) > > diff --git a/drivers/regulator/tps6586x-regulator.c > b/drivers/regulator/tps6586x-regulator.c > index 6d20b04..c6b945d 100644 > --- a/drivers/regulator/tps6586x-regulator.c > +++ b/drivers/regulator/tps6586x-regulator.c > @@ -327,6 +327,45 @@ static inline int > tps6586x_regulator_preinit(struct device *parent, > 1 << ri->enable_bit[1]); > } > > +static inline int tps6586x_regulator_set_pwm_mode(struct > +platform_device *pdev) { > + struct regulator_init_data *p = pdev->dev.platform_data; > + struct tps6586x_settings *setting; > + struct device *parent; > + int ret = 0; > + uint8_t mask; > + > + if (p == NULL) > + return 0; You can use if (!p) > + > + setting = p->driver_data; > + if (setting == NULL) > + return 0; Same here. > + > + switch (pdev->id) { > + case TPS6586X_ID_SM_0: > + mask = 1 << SM0_PWM_BIT; > + break; > + case TPS6586X_ID_SM_1: > + mask = 1 << SM1_PWM_BIT; > + break; > + case TPS6586X_ID_SM_2: > + mask = 1 << SM2_PWM_BIT; > + break; > + default: > + /* not all regulators have PWM/PFM option */ > + return 0; > + } > + > + parent = pdev->dev.parent; Can parent be NULL?, if it can be then you can check it here. > + if (setting->sm_pwm_mode == PWM_ONLY) > + ret = tps6586x_set_bits(parent, TPS6586X_SMODE1, > + mask); > + else if (setting->sm_pwm_mode == AUTO_PWM_PFM) > + ret = tps6586x_clr_bits(parent, TPS6586X_SMODE1, > + mask); > + When pwm_mode is not PWM_ONLY or AUTO_PWM_PFM 0 (success) is returned, is it intended? Do you wish to add an else assigning ret = error_code in such a case? > + return ret; > +} > + > static inline struct tps6586x_regulator *find_regulator_info(int id) > { > struct tps6586x_regulator *ri; > @@ -367,6 +406,13 @@ static int __devinit > tps6586x_regulator_probe(struct platform_device *pdev) > return PTR_ERR(rdev); > } > > + err = tps6586x_regulator_set_pwm_mode(pdev); > + if (err) { > + dev_err(&pdev->dev, "failed to set pwm mode\n"); > + regulator_unregister(rdev); > + return err; > + } > + > platform_set_drvdata(pdev, rdev); > > return 0; > diff --git a/include/linux/mfd/tps6586x.h > b/include/linux/mfd/tps6586x.h index d96fb3d..74e92c0 100644 > --- a/include/linux/mfd/tps6586x.h > +++ b/include/linux/mfd/tps6586x.h > @@ -1,6 +1,10 @@ > #ifndef __LINUX_MFD_TPS6586X_H > #define __LINUX_MFD_TPS6586X_H > > +#define SM0_PWM_BIT 0 > +#define SM1_PWM_BIT 1 > +#define SM2_PWM_BIT 2 > + > enum { > TPS6586X_ID_SM_0, > TPS6586X_ID_SM_1, > @@ -48,6 +52,17 @@ enum { > TPS6586X_INT_RTC_ALM2, > }; > > +enum pwm_pfm_mode { > + PWM_ONLY, > + AUTO_PWM_PFM, > + NOT_CONFIGURABLE > +}; > + > +struct tps6586x_settings { > + /* SM0, SM1 and SM2 have PWM-only and auto PWM/PFM mode */ > + enum pwm_pfm_mode sm_pwm_mode; }; > + > struct tps6586x_subdev_info { > int id; > const char *name; > -- > 1.7.2.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-tegra" > in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo > info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
