[PATCH] regulator: tps6586x: add SMx slew rate setting

[PATCH] regulator: tps6586x: add SMx slew rate setting

[dahuang]

From: Danny Huang <dahuang@nvidia.com>

Add output vlotage slew rate setting for SM0/SM1

From: Xin Xie <xxie@nvidia.com>

Signed-off-by: Xin Xie <xxie@nvidia.com>
Signed-off-by: Danny Huang <dahuang@nvidia.com>
---
 drivers/regulator/tps6586x-regulator.c |   27 ++++++++++++++++++++++++++-
 include/linux/mfd/tps6586x.h           |   15 +++++++++++++++
 2 files changed, 41 insertions(+), 1 deletions(-)

diff --git a/drivers/regulator/tps6586x-regulator.c b/drivers/regulator/tps6586x-regulator.c
index bb04a75..9e5b459 100644
--- a/drivers/regulator/tps6586x-regulator.c
+++ b/drivers/regulator/tps6586x-regulator.c
@@ -332,6 +332,31 @@ static inline int tps6586x_regulator_preinit(struct device *parent,
 				 1 << ri->enable_bit[1]);
 }
 
+static int tps6586x_regulator_set_slew_rate(struct platform_device *pdev)
+{
+	struct device *parent = pdev->dev.parent;
+	struct regulator_init_data *p = pdev->dev.platform_data;
+	struct tps6586x_settings *setting = p->driver_data;
+	uint8_t reg;
+
+	if (setting == NULL)
+		return 0;
+
+	/* only SM0 and SM1 can have the slew rate settings */
+	switch (pdev->id) {
+	case TPS6586X_ID_SM_0:
+		reg = TPS6586X_SM0SL;
+		break;
+	case TPS6586X_ID_SM_1:
+		reg = TPS6586X_SM1SL;
+		break;
+	default:
+		dev_err(&pdev->dev, "invalid regulator ID\n");
+		return -EINVAL;
+	}
+	return tps6586x_write(parent, reg, setting->slew_rate);
+}
+
 static inline struct tps6586x_regulator *find_regulator_info(int id)
 {
 	struct tps6586x_regulator *ri;
@@ -374,7 +399,7 @@ static int __devinit tps6586x_regulator_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, rdev);
 
-	return 0;
+	return tps6586x_regulator_set_slew_rate(pdev);
 }
 
 static int __devexit tps6586x_regulator_remove(struct platform_device *pdev)
diff --git a/include/linux/mfd/tps6586x.h b/include/linux/mfd/tps6586x.h
index b6bab1b..9dfb2bb 100644
--- a/include/linux/mfd/tps6586x.h
+++ b/include/linux/mfd/tps6586x.h
@@ -48,6 +48,21 @@ enum {
 	TPS6586X_INT_RTC_ALM2,
 };
 
+enum {
+	TPS6586x_SLEW_RATE_INSTANTLY,
+	TPS6586x_SLEW_RATE_110UV,
+	TPS6586x_SLEW_RATE_220UV,
+	TPS6586x_SLEW_RATE_440UV,
+	TPS6586x_SLEW_RATE_880UV,
+	TPS6586x_SLEW_RATE_1760UV,
+	TPS6586x_SLEW_RATE_3520UV,
+	TPS6586x_SLEW_RATE_7040UV,
+};
+
+struct tps6586x_settings {
+	int slew_rate;
+};
+
 struct tps6586x_subdev_info {
 	int		id;
 	const char	*name;
-- 
1.7.0.4

Re: [PATCH] regulator: tps6586x: add SMx slew rate setting

[Mark Brown]

On Thu, Aug 04, 2011 at 07:34:22PM +0800, dahuang@nvidia.com wrote:
> From: Danny Huang <dahuang@nvidia.com>
> 
> Add output vlotage slew rate setting for SM0/SM1
> 
> From: Xin Xie <xxie@nvidia.com>

Looks like you messed up here, I rather suspect Xin Xie rather than you
should be the author?

> +static int tps6586x_regulator_set_slew_rate(struct platform_device *pdev)
> +{
> +	struct device *parent = pdev->dev.parent;
> +	struct regulator_init_data *p = pdev->dev.platform_data;
> +	struct tps6586x_settings *setting = p->driver_data;

If this is system configured data (which is what one would expect for
this) it should be coming in as platform data not driver data - what's
happened here?

> +	default:
> +		dev_err(&pdev->dev, "invalid regulator ID\n");
> +		return -EINVAL;
> +	}

Should say what data is invalid here, otherwise it's not going to be at
all obvious what's invalid.

> +enum {
> +	TPS6586x_SLEW_RATE_INSTANTLY,
> +	TPS6586x_SLEW_RATE_110UV,
> +	TPS6586x_SLEW_RATE_220UV,
> +	TPS6586x_SLEW_RATE_440UV,
> +	TPS6586x_SLEW_RATE_880UV,
> +	TPS6586x_SLEW_RATE_1760UV,
> +	TPS6586x_SLEW_RATE_3520UV,
> +	TPS6586x_SLEW_RATE_7040UV,
> +};

If the values are being written directly to the chip you should probably
explicitly specify the values that are being set.

Re: [PATCH] regulator: tps6586x: add SMx slew rate setting

[Mark Brown]

On Thu, Aug 04, 2011 at 07:34:22PM +0800, dahuang@nvidia.com wrote:
> From: Danny Huang <dahuang@nvidia.com>
> 
> Add output vlotage slew rate setting for SM0/SM1

Oh, and you should also be implementing set_voltage_time_sel() if you
know what the slew rate for the device is.

RE: [PATCH] regulator: tps6586x: add SMx slew rate setting

[Danny Huang]

Thanks for the feedback.
The reason for using driver_data is that I can't find a proper field in regulator_init_data for the slew rate setting.
I'll do some correction based on the feedback and try to add a new field for the slew rate setting.

-----Original Message-----
From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] 
Sent: Thursday, August 04, 2011 20:13
To: Danny Huang
Cc: lrg@ti.com; mike@compulab.co.il; sameo@linux.intel.com; Xin Xie; gking@nvidia.com; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] regulator: tps6586x: add SMx slew rate setting

On Thu, Aug 04, 2011 at 07:34:22PM +0800, dahuang@nvidia.com wrote:
> From: Danny Huang <dahuang@nvidia.com>
> 
> Add output vlotage slew rate setting for SM0/SM1
> 
> From: Xin Xie <xxie@nvidia.com>

Looks like you messed up here, I rather suspect Xin Xie rather than you should be the author?

> +static int tps6586x_regulator_set_slew_rate(struct platform_device 
> +*pdev) {
> +	struct device *parent = pdev->dev.parent;
> +	struct regulator_init_data *p = pdev->dev.platform_data;
> +	struct tps6586x_settings *setting = p->driver_data;

If this is system configured data (which is what one would expect for
this) it should be coming in as platform data not driver data - what's happened here?

> +	default:
> +		dev_err(&pdev->dev, "invalid regulator ID\n");
> +		return -EINVAL;
> +	}

Should say what data is invalid here, otherwise it's not going to be at all obvious what's invalid.

> +enum {
> +	TPS6586x_SLEW_RATE_INSTANTLY,
> +	TPS6586x_SLEW_RATE_110UV,
> +	TPS6586x_SLEW_RATE_220UV,
> +	TPS6586x_SLEW_RATE_440UV,
> +	TPS6586x_SLEW_RATE_880UV,
> +	TPS6586x_SLEW_RATE_1760UV,
> +	TPS6586x_SLEW_RATE_3520UV,
> +	TPS6586x_SLEW_RATE_7040UV,
> +};

If the values are being written directly to the chip you should probably explicitly specify the values that are being set.

RE: [PATCH] regulator: tps6586x: add SMx slew rate setting

[Danny Huang]

Thanks. I'll implement this in a separate patch.

-----Original Message-----
From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] 
Sent: Thursday, August 04, 2011 20:42
To: Danny Huang
Cc: lrg@ti.com; mike@compulab.co.il; sameo@linux.intel.com; Xin Xie; gking@nvidia.com; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] regulator: tps6586x: add SMx slew rate setting

On Thu, Aug 04, 2011 at 07:34:22PM +0800, dahuang@nvidia.com wrote:
> From: Danny Huang <dahuang@nvidia.com>
> 
> Add output vlotage slew rate setting for SM0/SM1

Oh, and you should also be implementing set_voltage_time_sel() if you know what the slew rate for the device is.

Re: [PATCH] regulator: tps6586x: add SMx slew rate setting

[Mark Brown]

On Fri, Aug 05, 2011 at 07:51:15PM +0800, Danny Huang wrote:
> Thanks for the feedback.
> The reason for using driver_data is that I can't find a proper field in regulator_init_data for the slew rate setting.
> I'll do some correction based on the feedback and try to add a new field for the slew rate setting.

No, this is OK - we should really rename the driver_data field, it's
very misleading.  However:

> +       return tps6586x_write(parent, reg, setting->slew_rate);

what happens if the user sets zero for the slew rate (eg, if another
field is added to the platform data that they want to set)?  Is this a
sane setting (like the chip default) or might it break something?

RE: [PATCH] regulator: tps6586x: add SMx slew rate setting

[Danny Huang]

From: Mark Brown, Sent: Monday, August 08, 2011 17:37
> On Fri, Aug 05, 2011 at 07:51:15PM +0800, Danny Huang wrote:
> > Thanks for the feedback.
> > The reason for using driver_data is that I can't find a proper field in
> regulator_init_data for the slew rate setting.
> > I'll do some correction based on the feedback and try to add a new field for
> the slew rate setting.
> 
> No, this is OK - we should really rename the driver_data field, it's
> very misleading.  However:
> 
> > +       return tps6586x_write(parent, reg, setting->slew_rate);
> 
> what happens if the user sets zero for the slew rate (eg, if another
> field is added to the platform data that they want to set)?  Is this a
> sane setting (like the chip default) or might it break something?

Zero is a valid setting for the regulator but not the default setting.
The default setting is 0x07.
This might affect the system stability under some particular condition.
I thought that users should pick a proper setting before init.

Re: [PATCH] regulator: tps6586x: add SMx slew rate setting

[Mark Brown]

On Mon, Aug 08, 2011 at 08:28:30PM +0800, Danny Huang wrote:

> Zero is a valid setting for the regulator but not the default setting.
> The default setting is 0x07.
> This might affect the system stability under some particular condition.
> I thought that users should pick a proper setting before init.

It'd be good to do something like make the variable wider than the
register then require that users set an out of band bit to program the
value to zero.  This would improve robustness in hte case where someone
relies on a zero initialized platform data by default.

Re: [PATCH] regulator: tps6586x: add SMx slew rate setting

[Liam Girdwood]

On Tue, 2011-08-09 at 18:47 +0800, dahuang@nvidia.com wrote:
> From: Xin Xie <xxie@nvidia.com>
> 
> Add output vlotage slew rate setting for SM0/SM1
> 
> Signed-off-by: Xin Xie <xxie@nvidia.com>
> Signed-off-by: Danny Huang <dahuang@nvidia.com>
> ---
>  drivers/regulator/tps6586x-regulator.c |   32 +++++++++++++++++++++++++++++++-
>  include/linux/mfd/tps6586x.h           |   16 ++++++++++++++++
>  2 files changed, 47 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/regulator/tps6586x-regulator.c b/drivers/regulator/tps6586x-regulator.c
> index bb04a75..dbcf09d 100644
> --- a/drivers/regulator/tps6586x-regulator.c
> +++ b/drivers/regulator/tps6586x-regulator.c
> @@ -332,6 +332,36 @@ static inline int tps6586x_regulator_preinit(struct device *parent,
>  				 1 << ri->enable_bit[1]);
>  }
>  
> +static int tps6586x_regulator_set_slew_rate(struct platform_device *pdev)
> +{
> +	struct device *parent = pdev->dev.parent;
> +	struct regulator_init_data *p = pdev->dev.platform_data;
> +	struct tps6586x_settings *setting = p->driver_data;
> +	uint8_t reg;
> +
> +	if (setting == NULL)
> +		return 0;
> +
> +	if (!(setting->slew_rate & TPS6586X_SLEW_RATE_SET))
> +		return 0;
> +
> +	/* only SM0 and SM1 can have the slew rate settings */
> +	switch (pdev->id) {
> +	case TPS6586X_ID_SM_0:
> +		reg = TPS6586X_SM0SL;
> +		break;
> +	case TPS6586X_ID_SM_1:
> +		reg = TPS6586X_SM1SL;
> +		break;
> +	default:
> +		dev_warn(&pdev->dev, "Only SM0/SM1 can set slew rate\n");
> +		return -EINVAL;
> +	}
> +
> +	return tps6586x_write(parent, reg,
> +			setting->slew_rate & TPS6586X_SLEW_RATE_MASK);
> +}
> +
>  static inline struct tps6586x_regulator *find_regulator_info(int id)
>  {
>  	struct tps6586x_regulator *ri;
> @@ -374,7 +404,7 @@ static int __devinit tps6586x_regulator_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, rdev);
>  
> -	return 0;
> +	return tps6586x_regulator_set_slew_rate(pdev);
>  }
>  
>  static int __devexit tps6586x_regulator_remove(struct platform_device *pdev)
> diff --git a/include/linux/mfd/tps6586x.h b/include/linux/mfd/tps6586x.h
> index b6bab1b..b19176e 100644
> --- a/include/linux/mfd/tps6586x.h
> +++ b/include/linux/mfd/tps6586x.h
> @@ -1,6 +1,18 @@
>  #ifndef __LINUX_MFD_TPS6586X_H
>  #define __LINUX_MFD_TPS6586X_H
>  
> +#define TPS6586X_SLEW_RATE_INSTANTLY	0x00
> +#define TPS6586X_SLEW_RATE_110UV	0x01
> +#define TPS6586X_SLEW_RATE_220UV	0x02
> +#define TPS6586X_SLEW_RATE_440UV	0x03
> +#define TPS6586X_SLEW_RATE_880UV	0x04
> +#define TPS6586X_SLEW_RATE_1760UV	0x05
> +#define TPS6586X_SLEW_RATE_3520UV	0x06
> +#define TPS6586X_SLEW_RATE_7040UV	0x07
> +
> +#define TPS6586X_SLEW_RATE_SET		0x08
> +#define TPS6586X_SLEW_RATE_MASK         0x07
> +
>  enum {
>  	TPS6586X_ID_SM_0,
>  	TPS6586X_ID_SM_1,
> @@ -48,6 +60,10 @@ enum {
>  	TPS6586X_INT_RTC_ALM2,
>  };
>  
> +struct tps6586x_settings {
> +	int slew_rate;
> +};
> +
>  struct tps6586x_subdev_info {
>  	int		id;
>  	const char	*name;

Applied.

Thanks

Liam

Re: [PATCH] regulator: tps6586x: add SMx slew rate setting

[Liam Girdwood]

On 22/08/11 10:35, Samuel Ortiz wrote:
> Hi Xin,
> 
> On Tue, Aug 09, 2011 at 06:47:50PM +0800, dahuang@nvidia.com wrote:
>> From: Xin Xie <xxie@nvidia.com>
>>
>> Add output vlotage slew rate setting for SM0/SM1
>>
>> Signed-off-by: Xin Xie <xxie@nvidia.com>
>> Signed-off-by: Danny Huang <dahuang@nvidia.com>
> Acked-by: Samuel Ortiz <sameo@linux.intel.com>
> 
> Liam, are you taking this one ?
> 

I will do in the next few days. I've had an ADSL fault for the last two weeks that is keeping me off line atm. 

Liam

Re: [PATCH] regulator: tps6586x: add SMx slew rate setting

[Samuel Ortiz]

Hi Xin,

On Tue, Aug 09, 2011 at 06:47:50PM +0800, dahuang@nvidia.com wrote:
> From: Xin Xie <xxie@nvidia.com>
> 
> Add output vlotage slew rate setting for SM0/SM1
> 
> Signed-off-by: Xin Xie <xxie@nvidia.com>
> Signed-off-by: Danny Huang <dahuang@nvidia.com>
Acked-by: Samuel Ortiz <sameo@linux.intel.com>

Liam, are you taking this one ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH] regulator: tps6586x: add SMx slew rate setting

[Mark Brown]

On Tue, Aug 09, 2011 at 06:47:50PM +0800, dahuang@nvidia.com wrote:
> From: Xin Xie <xxie@nvidia.com>
> 
> Add output vlotage slew rate setting for SM0/SM1

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

[PATCH] regulator: tps6586x: add SMx slew rate setting

[dahuang]

From: Xin Xie <xxie@nvidia.com>

Add output vlotage slew rate setting for SM0/SM1

Signed-off-by: Xin Xie <xxie@nvidia.com>
Signed-off-by: Danny Huang <dahuang@nvidia.com>
---
 drivers/regulator/tps6586x-regulator.c |   32 +++++++++++++++++++++++++++++++-
 include/linux/mfd/tps6586x.h           |   16 ++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/drivers/regulator/tps6586x-regulator.c b/drivers/regulator/tps6586x-regulator.c
index bb04a75..dbcf09d 100644
--- a/drivers/regulator/tps6586x-regulator.c
+++ b/drivers/regulator/tps6586x-regulator.c
@@ -332,6 +332,36 @@ static inline int tps6586x_regulator_preinit(struct device *parent,
 				 1 << ri->enable_bit[1]);
 }
 
+static int tps6586x_regulator_set_slew_rate(struct platform_device *pdev)
+{
+	struct device *parent = pdev->dev.parent;
+	struct regulator_init_data *p = pdev->dev.platform_data;
+	struct tps6586x_settings *setting = p->driver_data;
+	uint8_t reg;
+
+	if (setting == NULL)
+		return 0;
+
+	if (!(setting->slew_rate & TPS6586X_SLEW_RATE_SET))
+		return 0;
+
+	/* only SM0 and SM1 can have the slew rate settings */
+	switch (pdev->id) {
+	case TPS6586X_ID_SM_0:
+		reg = TPS6586X_SM0SL;
+		break;
+	case TPS6586X_ID_SM_1:
+		reg = TPS6586X_SM1SL;
+		break;
+	default:
+		dev_warn(&pdev->dev, "Only SM0/SM1 can set slew rate\n");
+		return -EINVAL;
+	}
+
+	return tps6586x_write(parent, reg,
+			setting->slew_rate & TPS6586X_SLEW_RATE_MASK);
+}
+
 static inline struct tps6586x_regulator *find_regulator_info(int id)
 {
 	struct tps6586x_regulator *ri;
@@ -374,7 +404,7 @@ static int __devinit tps6586x_regulator_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, rdev);
 
-	return 0;
+	return tps6586x_regulator_set_slew_rate(pdev);
 }
 
 static int __devexit tps6586x_regulator_remove(struct platform_device *pdev)
diff --git a/include/linux/mfd/tps6586x.h b/include/linux/mfd/tps6586x.h
index b6bab1b..b19176e 100644
--- a/include/linux/mfd/tps6586x.h
+++ b/include/linux/mfd/tps6586x.h
@@ -1,6 +1,18 @@
 #ifndef __LINUX_MFD_TPS6586X_H
 #define __LINUX_MFD_TPS6586X_H
 
+#define TPS6586X_SLEW_RATE_INSTANTLY	0x00
+#define TPS6586X_SLEW_RATE_110UV	0x01
+#define TPS6586X_SLEW_RATE_220UV	0x02
+#define TPS6586X_SLEW_RATE_440UV	0x03
+#define TPS6586X_SLEW_RATE_880UV	0x04
+#define TPS6586X_SLEW_RATE_1760UV	0x05
+#define TPS6586X_SLEW_RATE_3520UV	0x06
+#define TPS6586X_SLEW_RATE_7040UV	0x07
+
+#define TPS6586X_SLEW_RATE_SET		0x08
+#define TPS6586X_SLEW_RATE_MASK         0x07
+
 enum {
 	TPS6586X_ID_SM_0,
 	TPS6586X_ID_SM_1,
@@ -48,6 +60,10 @@ enum {
 	TPS6586X_INT_RTC_ALM2,
 };
 
+struct tps6586x_settings {
+	int slew_rate;
+};
+
 struct tps6586x_subdev_info {
 	int		id;
 	const char	*name;
-- 
1.7.6