[PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp

[PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp

[Chanwoo Choi]

This patch parse 'buck[2-4]_ramp_enable and buck_ramp_delay' platform data
from dts file.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/regulator/s5m8767.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
index c24448b..cb6cdb3 100644
--- a/drivers/regulator/s5m8767.c
+++ b/drivers/regulator/s5m8767.c
@@ -640,6 +640,22 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
+	if (of_get_property(pmic_np, "s5m8767,pmic-buck2-ramp-enable", NULL))
+		pdata->buck2_ramp_enable = true;
+
+	if (of_get_property(pmic_np, "s5m8767,pmic-buck3-ramp-enable", NULL))
+		pdata->buck3_ramp_enable = true;
+
+	if (of_get_property(pmic_np, "s5m8767,pmic-buck4-ramp-enable", NULL))
+		pdata->buck4_ramp_enable = true;
+
+	if (pdata->buck2_ramp_enable || pdata->buck3_ramp_enable
+			|| pdata->buck4_ramp_enable) {
+		if (of_property_read_u32(pmic_np, "s5m8767,pmic-buck-ramp-delay",
+				&pdata->buck_ramp_delay))
+			pdata->buck_ramp_delay = 0;
+	}
+
 	return 0;
 }
 #else
-- 
1.8.0

[PATCH 2/2] regulator: s5m8767: Modify parsing method of the voltage table of buck2/3/4

[Chanwoo Choi]

The s5m8767 regulator driver parse always the voltage table of buck2/3/4.
If gpio_dvs feature isn't used and dts haven't included the voltage table
of buck2/3/4, s5m8767 regulator driver return error and file probe state.

This patch check only voltage table of buck on s5m8767_pmic_dt_parse_pdata()
if buck[2-4]_gpiodvs is true.

Signed-off-by: Chanwoo Choi <cw00.choi-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: YoungJun Cho <yj44.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/regulator/s5m8767.c | 54 +++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
index cb6cdb3..29999c0 100644
--- a/drivers/regulator/s5m8767.c
+++ b/drivers/regulator/s5m8767.c
@@ -522,7 +522,7 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
 	struct device_node *pmic_np, *regulators_np, *reg_np;
 	struct sec_regulator_data *rdata;
 	struct sec_opmode_data *rmode;
-	unsigned int i, dvs_voltage_nr = 1, ret;
+	unsigned int i, dvs_voltage_nr = 8, ret;
 
 	pmic_np = iodev->dev->of_node;
 	if (!pmic_np) {
@@ -586,15 +586,39 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
 		rmode++;
 	}
 
-	if (of_get_property(pmic_np, "s5m8767,pmic-buck2-uses-gpio-dvs", NULL))
+	if (of_get_property(pmic_np, "s5m8767,pmic-buck2-uses-gpio-dvs", NULL)) {
 		pdata->buck2_gpiodvs = true;
 
-	if (of_get_property(pmic_np, "s5m8767,pmic-buck3-uses-gpio-dvs", NULL))
+		if (of_property_read_u32_array(pmic_np,
+				"s5m8767,pmic-buck2-dvs-voltage",
+				pdata->buck2_voltage, dvs_voltage_nr)) {
+			dev_err(iodev->dev, "buck2 voltages not specified\n");
+			return -EINVAL;
+		}
+	}
+
+	if (of_get_property(pmic_np, "s5m8767,pmic-buck3-uses-gpio-dvs", NULL)) {
 		pdata->buck3_gpiodvs = true;
 
-	if (of_get_property(pmic_np, "s5m8767,pmic-buck4-uses-gpio-dvs", NULL))
+		if (of_property_read_u32_array(pmic_np,
+				"s5m8767,pmic-buck3-dvs-voltage",
+				pdata->buck3_voltage, dvs_voltage_nr)) {
+			dev_err(iodev->dev, "buck3 voltages not specified\n");
+			return -EINVAL;
+		}
+	}
+
+	if (of_get_property(pmic_np, "s5m8767,pmic-buck4-uses-gpio-dvs", NULL)) {
 		pdata->buck4_gpiodvs = true;
 
+		if (of_property_read_u32_array(pmic_np,
+				"s5m8767,pmic-buck4-dvs-voltage",
+				pdata->buck4_voltage, dvs_voltage_nr)) {
+			dev_err(iodev->dev, "buck4 voltages not specified\n");
+			return -EINVAL;
+		}
+	}
+
 	if (pdata->buck2_gpiodvs || pdata->buck3_gpiodvs ||
 						pdata->buck4_gpiodvs) {
 		ret = s5m8767_pmic_dt_parse_dvs_gpio(iodev, pdata, pmic_np);
@@ -612,34 +636,12 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
 				"invalid value for default dvs index, use 0\n");
 			}
 		}
-		dvs_voltage_nr = 8;
 	}
 
 	ret = s5m8767_pmic_dt_parse_ds_gpio(iodev, pdata, pmic_np);
 	if (ret)
 		return -EINVAL;
 
-	if (of_property_read_u32_array(pmic_np,
-				"s5m8767,pmic-buck2-dvs-voltage",
-				pdata->buck2_voltage, dvs_voltage_nr)) {
-		dev_err(iodev->dev, "buck2 voltages not specified\n");
-		return -EINVAL;
-	}
-
-	if (of_property_read_u32_array(pmic_np,
-				"s5m8767,pmic-buck3-dvs-voltage",
-				pdata->buck3_voltage, dvs_voltage_nr)) {
-		dev_err(iodev->dev, "buck3 voltages not specified\n");
-		return -EINVAL;
-	}
-
-	if (of_property_read_u32_array(pmic_np,
-				"s5m8767,pmic-buck4-dvs-voltage",
-				pdata->buck4_voltage, dvs_voltage_nr)) {
-		dev_err(iodev->dev, "buck4 voltages not specified\n");
-		return -EINVAL;
-	}
-
 	if (of_get_property(pmic_np, "s5m8767,pmic-buck2-ramp-enable", NULL))
 		pdata->buck2_ramp_enable = true;
 
-- 
1.8.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] regulator: s5m8767: Modify parsing method of the voltage table of buck2/3/4

[Mark Rutland]

On Thu, Oct 10, 2013 at 02:41:36AM +0100, Chanwoo Choi wrote:
> The s5m8767 regulator driver parse always the voltage table of buck2/3/4.
> If gpio_dvs feature isn't used and dts haven't included the voltage table
> of buck2/3/4, s5m8767 regulator driver return error and file probe state.
> 
> This patch check only voltage table of buck on s5m8767_pmic_dt_parse_pdata()
> if buck[2-4]_gpiodvs is true.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: YoungJun Cho <yj44.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/regulator/s5m8767.c | 54 +++++++++++++++++++++++----------------------
>  1 file changed, 28 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
> index cb6cdb3..29999c0 100644
> --- a/drivers/regulator/s5m8767.c
> +++ b/drivers/regulator/s5m8767.c
> @@ -522,7 +522,7 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
>  	struct device_node *pmic_np, *regulators_np, *reg_np;
>  	struct sec_regulator_data *rdata;
>  	struct sec_opmode_data *rmode;
> -	unsigned int i, dvs_voltage_nr = 1, ret;
> +	unsigned int i, dvs_voltage_nr = 8, ret;
>  
>  	pmic_np = iodev->dev->of_node;
>  	if (!pmic_np) {
> @@ -586,15 +586,39 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
>  		rmode++;
>  	}
>  
> -	if (of_get_property(pmic_np, "s5m8767,pmic-buck2-uses-gpio-dvs", NULL))
> +	if (of_get_property(pmic_np, "s5m8767,pmic-buck2-uses-gpio-dvs", NULL)) {
>  		pdata->buck2_gpiodvs = true;

As this line is being changed, please switch to of_property_read_bool.

>  
> -	if (of_get_property(pmic_np, "s5m8767,pmic-buck3-uses-gpio-dvs", NULL))
> +		if (of_property_read_u32_array(pmic_np,
> +				"s5m8767,pmic-buck2-dvs-voltage",
> +				pdata->buck2_voltage, dvs_voltage_nr)) {

Won't this break existing (not conforming to the binding) dts?

$(git grep s5m8767,pmic-buck2-dvs-voltage) shows me
arch/arm/boot/dts/exynos5250-arndale.dts is only has one entry in its
s5m8767,pmic-buck3-uses-gpio-dvs property's list.

> +			dev_err(iodev->dev, "buck2 voltages not specified\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (of_get_property(pmic_np, "s5m8767,pmic-buck3-uses-gpio-dvs", NULL)) {
>  		pdata->buck3_gpiodvs = true;

Another of_proeprty_read_bool candidate.

>  
> -	if (of_get_property(pmic_np, "s5m8767,pmic-buck4-uses-gpio-dvs", NULL))
> +		if (of_property_read_u32_array(pmic_np,
> +				"s5m8767,pmic-buck3-dvs-voltage",
> +				pdata->buck3_voltage, dvs_voltage_nr)) {
> +			dev_err(iodev->dev, "buck3 voltages not specified\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (of_get_property(pmic_np, "s5m8767,pmic-buck4-uses-gpio-dvs", NULL)) {
>  		pdata->buck4_gpiodvs = true;

Similarly.

In general I'm confused by the need for the *-uses-gpio-dvs properties.
Are they not implied by the presence of *-dvs-voltage properties?

Cheers,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] regulator: s5m8767: Modify parsing method of the voltage table of buck2/3/4

[Chanwoo Choi]

On 10/10/2013 07:36 PM, Mark Rutland wrote:
> On Thu, Oct 10, 2013 at 02:41:36AM +0100, Chanwoo Choi wrote:
>> The s5m8767 regulator driver parse always the voltage table of buck2/3/4.
>> If gpio_dvs feature isn't used and dts haven't included the voltage table
>> of buck2/3/4, s5m8767 regulator driver return error and file probe state.
>>
>> This patch check only voltage table of buck on s5m8767_pmic_dt_parse_pdata()
>> if buck[2-4]_gpiodvs is true.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  drivers/regulator/s5m8767.c | 54 +++++++++++++++++++++++----------------------
>>  1 file changed, 28 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
>> index cb6cdb3..29999c0 100644
>> --- a/drivers/regulator/s5m8767.c
>> +++ b/drivers/regulator/s5m8767.c
>> @@ -522,7 +522,7 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
>>  	struct device_node *pmic_np, *regulators_np, *reg_np;
>>  	struct sec_regulator_data *rdata;
>>  	struct sec_opmode_data *rmode;
>> -	unsigned int i, dvs_voltage_nr = 1, ret;
>> +	unsigned int i, dvs_voltage_nr = 8, ret;
>>  
>>  	pmic_np = iodev->dev->of_node;
>>  	if (!pmic_np) {
>> @@ -586,15 +586,39 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
>>  		rmode++;
>>  	}
>>  
>> -	if (of_get_property(pmic_np, "s5m8767,pmic-buck2-uses-gpio-dvs", NULL))
>> +	if (of_get_property(pmic_np, "s5m8767,pmic-buck2-uses-gpio-dvs", NULL)) {
>>  		pdata->buck2_gpiodvs = true;
> 
> As this line is being changed, please switch to of_property_read_bool.

OK.

> 
>>  
>> -	if (of_get_property(pmic_np, "s5m8767,pmic-buck3-uses-gpio-dvs", NULL))
>> +		if (of_property_read_u32_array(pmic_np,
>> +				"s5m8767,pmic-buck2-dvs-voltage",
>> +				pdata->buck2_voltage, dvs_voltage_nr)) {
> 
> Won't this break existing (not conforming to the binding) dts?
> 
> $(git grep s5m8767,pmic-buck2-dvs-voltage) shows me
> arch/arm/boot/dts/exynos5250-arndale.dts is only has one entry in its
> s5m8767,pmic-buck3-uses-gpio-dvs property's list.

I checked it but exynos5250-arndale.dts haven't included pmic-buck3-uses-gpio-dvs property.
What is reference code?

> 
>> +			dev_err(iodev->dev, "buck2 voltages not specified\n");
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	if (of_get_property(pmic_np, "s5m8767,pmic-buck3-uses-gpio-dvs", NULL)) {
>>  		pdata->buck3_gpiodvs = true;
> 
> Another of_proeprty_read_bool candidate.

OK.

> 
>>  
>> -	if (of_get_property(pmic_np, "s5m8767,pmic-buck4-uses-gpio-dvs", NULL))
>> +		if (of_property_read_u32_array(pmic_np,
>> +				"s5m8767,pmic-buck3-dvs-voltage",
>> +				pdata->buck3_voltage, dvs_voltage_nr)) {
>> +			dev_err(iodev->dev, "buck3 voltages not specified\n");
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	if (of_get_property(pmic_np, "s5m8767,pmic-buck4-uses-gpio-dvs", NULL)) {
>>  		pdata->buck4_gpiodvs = true;
> 
> Similarly.
> 
> In general I'm confused by the need for the *-uses-gpio-dvs properties.
> Are they not implied by the presence of *-dvs-voltage properties?
> 

The s5m8767 support the power control using DVS(Dynamic Voltage scaling) by using
GPIO or I2C interface between PMIC and AP. 

When controlling DVS by using GPIO interface, *-uses-gpio-dvs/*-dvs-voltage properties is used.
The s5m8767 pmic control DVS of one only buck using GPIO. If specific target use GPIO interface
to control buck2 DVS, must set buck2-uses-gpio-dvs as true and buck3/4-uses-gpio-dvs is false.
Also, GPIO interface include three gpio pin which select total 8 step voltage according to
three gpio state. *-dvs-voltage properties include the voltage table of 8 step.

Also, you can check the aim of *-uses-gpio-dvs/*-dvs-voltage properties
on Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt.

Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp

[Mark Rutland]

On Thu, Oct 10, 2013 at 02:41:35AM +0100, Chanwoo Choi wrote:
> This patch parse 'buck[2-4]_ramp_enable and buck_ramp_delay' platform data
> from dts file.

Why?

What do these describe? Why do we need them?

> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/regulator/s5m8767.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
> index c24448b..cb6cdb3 100644
> --- a/drivers/regulator/s5m8767.c
> +++ b/drivers/regulator/s5m8767.c
> @@ -640,6 +640,22 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
>  		return -EINVAL;
>  	}
>  
> +	if (of_get_property(pmic_np, "s5m8767,pmic-buck2-ramp-enable", NULL))
> +		pdata->buck2_ramp_enable = true;

Please describe what this is and why we need it.

For reading boolean proeprties, use of_property_read_bool.

> +
> +	if (of_get_property(pmic_np, "s5m8767,pmic-buck3-ramp-enable", NULL))
> +		pdata->buck3_ramp_enable = true;
> +
> +	if (of_get_property(pmic_np, "s5m8767,pmic-buck4-ramp-enable", NULL))
> +		pdata->buck4_ramp_enable = true;
> +
> +	if (pdata->buck2_ramp_enable || pdata->buck3_ramp_enable
> +			|| pdata->buck4_ramp_enable) {
> +		if (of_property_read_u32(pmic_np, "s5m8767,pmic-buck-ramp-delay",
> +				&pdata->buck_ramp_delay))
> +			pdata->buck_ramp_delay = 0;

Why not initialise pdata->buck_ramp_delay to 0 beforehand?

That way you don't need to check the return value of
of_property_read_u32 here.

I note that pdata->buck_ramp_delay is an int, not a u32. Please use a
u32 temporary variable.

> +	}
> +

NAK. None of these seem to be documented in mainline's
Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt, nor
does documentation seem to be added here. Until there's some description
of what these are and why they're needed, I don't think they're
suitable.

Thanks,
Mark.

Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp

[Chanwoo Choi]

On 10/10/2013 07:28 PM, Mark Rutland wrote:
> On Thu, Oct 10, 2013 at 02:41:35AM +0100, Chanwoo Choi wrote:
>> This patch parse 'buck[2-4]_ramp_enable and buck_ramp_delay' platform data
>> from dts file.
> 
> Why?
> 
> What do these describe? Why do we need them?

Turn on ramp control of buck2/3/4.

> 
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> Signed-off-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
>>  drivers/regulator/s5m8767.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
>> index c24448b..cb6cdb3 100644
>> --- a/drivers/regulator/s5m8767.c
>> +++ b/drivers/regulator/s5m8767.c
>> @@ -640,6 +640,22 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
>>  		return -EINVAL;
>>  	}
>>  
>> +	if (of_get_property(pmic_np, "s5m8767,pmic-buck2-ramp-enable", NULL))
>> +		pdata->buck2_ramp_enable = true;
> 
> Please describe what this is and why we need it.

buck2/3/4_ramp_enable is used to protect the chip from the in-rush current.

> 
> For reading boolean proeprties, use of_property_read_bool.

I'll fix it.

> 
>> +
>> +	if (of_get_property(pmic_np, "s5m8767,pmic-buck3-ramp-enable", NULL))
>> +		pdata->buck3_ramp_enable = true;
>> +
>> +	if (of_get_property(pmic_np, "s5m8767,pmic-buck4-ramp-enable", NULL))
>> +		pdata->buck4_ramp_enable = true;
>> +
>> +	if (pdata->buck2_ramp_enable || pdata->buck3_ramp_enable
>> +			|| pdata->buck4_ramp_enable) {
>> +		if (of_property_read_u32(pmic_np, "s5m8767,pmic-buck-ramp-delay",
>> +				&pdata->buck_ramp_delay))
>> +			pdata->buck_ramp_delay = 0;
> 
> Why not initialise pdata->buck_ramp_delay to 0 beforehand?
> 
> That way you don't need to check the return value of
> of_property_read_u32 here.
> 
> I note that pdata->buck_ramp_delay is an int, not a u32. Please use a
> u32 temporary variable.

I'll fix it

> 
>> +	}
>> +
> 
> NAK. None of these seem to be documented in mainline's
> Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt, nor
> does documentation seem to be added here. Until there's some description
> of what these are and why they're needed, I don't think they're
> suitable.

I'll add description about this patch.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp

[Mark Rutland]

On Thu, Oct 10, 2013 at 12:10:53PM +0100, Chanwoo Choi wrote:
> On 10/10/2013 07:28 PM, Mark Rutland wrote:
> > On Thu, Oct 10, 2013 at 02:41:35AM +0100, Chanwoo Choi wrote:
> >> This patch parse 'buck[2-4]_ramp_enable and buck_ramp_delay' platform data
> >> from dts file.
> > 
> > Why?
> > 
> > What do these describe? Why do we need them?
> 
> Turn on ramp control of buck2/3/4.

Ok, but why does this need to be in the dt?

> 
> > 
> >>
> >> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> ---
> >>  drivers/regulator/s5m8767.c | 16 ++++++++++++++++
> >>  1 file changed, 16 insertions(+)
> >>
> >> diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
> >> index c24448b..cb6cdb3 100644
> >> --- a/drivers/regulator/s5m8767.c
> >> +++ b/drivers/regulator/s5m8767.c
> >> @@ -640,6 +640,22 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
> >>  		return -EINVAL;
> >>  	}
> >>  
> >> +	if (of_get_property(pmic_np, "s5m8767,pmic-buck2-ramp-enable", NULL))
> >> +		pdata->buck2_ramp_enable = true;
> > 
> > Please describe what this is and why we need it.
> 
> buck2/3/4_ramp_enable is used to protect the chip from the in-rush current.

Ok. Could we not always set these in the driver? Under what
circumstances do we need this?

> 
> > 
> > For reading boolean proeprties, use of_property_read_bool.
> 
> I'll fix it.
> 
> > 
> >> +
> >> +	if (of_get_property(pmic_np, "s5m8767,pmic-buck3-ramp-enable", NULL))
> >> +		pdata->buck3_ramp_enable = true;
> >> +
> >> +	if (of_get_property(pmic_np, "s5m8767,pmic-buck4-ramp-enable", NULL))
> >> +		pdata->buck4_ramp_enable = true;
> >> +
> >> +	if (pdata->buck2_ramp_enable || pdata->buck3_ramp_enable
> >> +			|| pdata->buck4_ramp_enable) {
> >> +		if (of_property_read_u32(pmic_np, "s5m8767,pmic-buck-ramp-delay",
> >> +				&pdata->buck_ramp_delay))
> >> +			pdata->buck_ramp_delay = 0;
> > 
> > Why not initialise pdata->buck_ramp_delay to 0 beforehand?
> > 
> > That way you don't need to check the return value of
> > of_property_read_u32 here.
> > 
> > I note that pdata->buck_ramp_delay is an int, not a u32. Please use a
> > u32 temporary variable.
> 
> I'll fix it
> 
> > 
> >> +	}
> >> +
> > 
> > NAK. None of these seem to be documented in mainline's
> > Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt, nor
> > does documentation seem to be added here. Until there's some description
> > of what these are and why they're needed, I don't think they're
> > suitable.
> 
> I'll add description about this patch.

Cheers.

Mark.

Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 543 bytes --]

On Thu, Oct 10, 2013 at 05:51:00PM +0100, Mark Rutland wrote:
> On Thu, Oct 10, 2013 at 12:10:53PM +0100, Chanwoo Choi wrote:

> > Turn on ramp control of buck2/3/4.

> Ok, but why does this need to be in the dt?

Ramp control is a hardware system integration decision which people
working with regulators should be familiar with - you're trading off
speed of power on or voltage change for maximum current draw from the
regulator supply.  The term should be sufficiently clear to an engineer
who can make an informed decision about using it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 260 bytes --]

On Thu, Oct 10, 2013 at 10:41:35AM +0900, Chanwoo Choi wrote:
> This patch parse 'buck[2-4]_ramp_enable and buck_ramp_delay' platform data
> from dts file.

Applied both, thanks.  Please submit a followup patch to use
of_property_read_bool() as noted by Mark.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp

[Chanwoo Choi]

On 10/24/2013 07:15 PM, Mark Brown wrote:
> On Thu, Oct 10, 2013 at 10:41:35AM +0900, Chanwoo Choi wrote:
>> This patch parse 'buck[2-4]_ramp_enable and buck_ramp_delay' platform data
>> from dts file.
> 
> Applied both, thanks.  Please submit a followup patch to use
> of_property_read_bool() as noted by Mark.
> 

OK, Thanks,

Best regards,
Chanwoo Choi