* [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
@ 2013-10-10 1:41 Chanwoo Choi
2013-10-10 1:41 ` [PATCH 2/2] regulator: s5m8767: Modify parsing method of the voltage table of buck2/3/4 Chanwoo Choi
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Chanwoo Choi @ 2013-10-10 1:41 UTC (permalink / raw)
To: broonie, lgirdwood, sbkim73
Cc: linux-kernel, devicetree, Chanwoo Choi, Kyungmin Park
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] regulator: s5m8767: Modify parsing method of the voltage table of buck2/3/4
2013-10-10 1:41 [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp Chanwoo Choi
@ 2013-10-10 1:41 ` Chanwoo Choi
2013-10-10 10:36 ` Mark Rutland
2013-10-10 10:28 ` [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp Mark Rutland
2013-10-24 10:15 ` Mark Brown
2 siblings, 1 reply; 10+ messages in thread
From: Chanwoo Choi @ 2013-10-10 1:41 UTC (permalink / raw)
To: broonie, lgirdwood, sbkim73
Cc: linux-kernel, devicetree, Chanwoo Choi, YoungJun Cho, Kyungmin Park
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;
- 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
2013-10-10 1:41 [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp Chanwoo Choi
2013-10-10 1:41 ` [PATCH 2/2] regulator: s5m8767: Modify parsing method of the voltage table of buck2/3/4 Chanwoo Choi
@ 2013-10-10 10:28 ` Mark Rutland
2013-10-10 11:10 ` Chanwoo Choi
2013-10-24 10:15 ` Mark Brown
2 siblings, 1 reply; 10+ messages in thread
From: Mark Rutland @ 2013-10-10 10:28 UTC (permalink / raw)
To: Chanwoo Choi
Cc: broonie, lgirdwood, sbkim73, linux-kernel, devicetree, Kyungmin Park
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] regulator: s5m8767: Modify parsing method of the voltage table of buck2/3/4
2013-10-10 1:41 ` [PATCH 2/2] regulator: s5m8767: Modify parsing method of the voltage table of buck2/3/4 Chanwoo Choi
@ 2013-10-10 10:36 ` Mark Rutland
2013-10-10 11:46 ` Chanwoo Choi
0 siblings, 1 reply; 10+ messages in thread
From: Mark Rutland @ 2013-10-10 10:36 UTC (permalink / raw)
To: Chanwoo Choi
Cc: broonie, lgirdwood, sbkim73, linux-kernel, devicetree,
YoungJun Cho, Kyungmin Park
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.
>
> - 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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
2013-10-10 10:28 ` [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp Mark Rutland
@ 2013-10-10 11:10 ` Chanwoo Choi
2013-10-10 16:51 ` Mark Rutland
0 siblings, 1 reply; 10+ messages in thread
From: Chanwoo Choi @ 2013-10-10 11:10 UTC (permalink / raw)
To: Mark Rutland
Cc: broonie, lgirdwood, sbkim73, linux-kernel, devicetree, Kyungmin Park
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@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.
>
> 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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] regulator: s5m8767: Modify parsing method of the voltage table of buck2/3/4
2013-10-10 10:36 ` Mark Rutland
@ 2013-10-10 11:46 ` Chanwoo Choi
0 siblings, 0 replies; 10+ messages in thread
From: Chanwoo Choi @ 2013-10-10 11:46 UTC (permalink / raw)
To: Mark Rutland
Cc: broonie, lgirdwood, sbkim73, linux-kernel, devicetree,
YoungJun Cho, Kyungmin Park
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
2013-10-10 11:10 ` Chanwoo Choi
@ 2013-10-10 16:51 ` Mark Rutland
2013-10-10 17:36 ` Mark Brown
0 siblings, 1 reply; 10+ messages in thread
From: Mark Rutland @ 2013-10-10 16:51 UTC (permalink / raw)
To: Chanwoo Choi
Cc: broonie, lgirdwood, sbkim73, linux-kernel, devicetree, Kyungmin Park
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
2013-10-10 16:51 ` Mark Rutland
@ 2013-10-10 17:36 ` Mark Brown
0 siblings, 0 replies; 10+ messages in thread
From: Mark Brown @ 2013-10-10 17:36 UTC (permalink / raw)
To: Mark Rutland
Cc: Chanwoo Choi, lgirdwood, sbkim73, linux-kernel, devicetree,
Kyungmin Park
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
2013-10-10 1:41 [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp Chanwoo Choi
2013-10-10 1:41 ` [PATCH 2/2] regulator: s5m8767: Modify parsing method of the voltage table of buck2/3/4 Chanwoo Choi
2013-10-10 10:28 ` [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp Mark Rutland
@ 2013-10-24 10:15 ` Mark Brown
2013-10-25 9:08 ` Chanwoo Choi
2 siblings, 1 reply; 10+ messages in thread
From: Mark Brown @ 2013-10-24 10:15 UTC (permalink / raw)
To: Chanwoo Choi; +Cc: lgirdwood, sbkim73, linux-kernel, devicetree, Kyungmin Park
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
2013-10-24 10:15 ` Mark Brown
@ 2013-10-25 9:08 ` Chanwoo Choi
0 siblings, 0 replies; 10+ messages in thread
From: Chanwoo Choi @ 2013-10-25 9:08 UTC (permalink / raw)
To: Mark Brown; +Cc: lgirdwood, sbkim73, linux-kernel, devicetree, Kyungmin Park
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-10-25 9:08 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-10 1:41 [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp Chanwoo Choi
2013-10-10 1:41 ` [PATCH 2/2] regulator: s5m8767: Modify parsing method of the voltage table of buck2/3/4 Chanwoo Choi
2013-10-10 10:36 ` Mark Rutland
2013-10-10 11:46 ` Chanwoo Choi
2013-10-10 10:28 ` [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp Mark Rutland
2013-10-10 11:10 ` Chanwoo Choi
2013-10-10 16:51 ` Mark Rutland
2013-10-10 17:36 ` Mark Brown
2013-10-24 10:15 ` Mark Brown
2013-10-25 9:08 ` Chanwoo Choi
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).