Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
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. signature.asc Description: Digital signature
Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
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. signature.asc Description: Digital signature
Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
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. signature.asc Description: Digital signature
Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
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
> >> Signed-off-by: Kyungmin Park
> >> ---
> >> 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",
> >> + >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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
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
>> Signed-off-by: Kyungmin Park
>> ---
>> 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",
>> +>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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
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
> Signed-off-by: Kyungmin Park
> ---
> 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",
> + >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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
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.c...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
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.c...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
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.c...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
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. signature.asc Description: Digital signature
[PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
This patch parse 'buck[2-4]_ramp_enable and buck_ramp_delay' platform data
from dts file.
Signed-off-by: Chanwoo Choi
Signed-off-by: Kyungmin Park
---
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",
+ >buck_ramp_delay))
+ pdata->buck_ramp_delay = 0;
+ }
+
return 0;
}
#else
--
1.8.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] regulator: s5m8767: Modify parse_dt function to parse data related to ramp
This patch parse 'buck[2-4]_ramp_enable and buck_ramp_delay' platform data
from dts file.
Signed-off-by: Chanwoo Choi cw00.c...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
