| Message ID | 1489356665-3175-3-git-send-email-stefan.wahren@i2se.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Eduardo Valentin |
| Headers | show |
On 03/12/17 15:11, Stefan Wahren wrote: > Use the new function of_property_read_s32_array() to prepare > of-thermal for negative coefficients. These are used by > the upcoming bcm2835_thermal driver. > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> > --- > drivers/thermal/of-thermal.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c > index d04ec3b..491d58a 100644 > --- a/drivers/thermal/of-thermal.c > +++ b/drivers/thermal/of-thermal.c > @@ -821,7 +821,8 @@ __init *thermal_of_build_thermal_zone(struct device_node *np) > struct device_node *child = NULL, *gchild; > struct __thermal_zone *tz; > int ret, i; > - u32 prop, coef[2]; > + u32 prop; > + s32 coef[2]; > > if (!np) { > pr_err("no thermal zone np\n"); > @@ -851,7 +852,7 @@ __init *thermal_of_build_thermal_zone(struct device_node *np) > * one sensor per thermal zone. Thus, we are considering > * only the first two values as slope and offset. > */ > - ret = of_property_read_u32_array(np, "coefficients", coef, 2); > + ret = of_property_read_s32_array(np, "coefficients", coef, 2); > if (ret == 0) { > tz->slope = coef[0]; > tz->offset = coef[1]; > Since you are doing so much work to fix reading the array of s32 property, you might also want to do the same for the s32 properties, "polling-delay-passive" and "polling-delay". Just change of_property_read_u32() to of_property_read_s32() and change the type of prop to match. drivers/thermal/of-thermal.c: In function 'thermal_of_build_thermal_zone': drivers/thermal/of-thermal.c:841:2: warning: conversion to 'int' from 'u32' may change the sign of the result [-Wsign-conversion] drivers/thermal/of-thermal.c:848:2: warning: conversion to 'int' from 'u32' may change the sign of the result [-Wsign-conversion] 836 ret = of_property_read_u32(np, "polling-delay-passive", &prop); 837 if (ret < 0) { 838 pr_err("missing polling-delay-passive property\n"); 839 goto free_tz; 840 } 841 tz->passive_delay = prop; 842 843 ret = of_property_read_u32(np, "polling-delay", &prop); 844 if (ret < 0) { 845 pr_err("missing polling-delay property\n"); 846 goto free_tz; 847 } 848 tz->polling_delay = prop; -Frank
> Frank Rowand <frowand.list@gmail.com> hat am 24. März 2017 um 00:32 geschrieben: > > > On 03/12/17 15:11, Stefan Wahren wrote: > > Use the new function of_property_read_s32_array() to prepare > > of-thermal for negative coefficients. These are used by > > the upcoming bcm2835_thermal driver. > > > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> > > --- > > drivers/thermal/of-thermal.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c > > index d04ec3b..491d58a 100644 > > --- a/drivers/thermal/of-thermal.c > > +++ b/drivers/thermal/of-thermal.c > > @@ -821,7 +821,8 @@ __init *thermal_of_build_thermal_zone(struct device_node *np) > > struct device_node *child = NULL, *gchild; > > struct __thermal_zone *tz; > > int ret, i; > > - u32 prop, coef[2]; > > + u32 prop; > > + s32 coef[2]; > > > > if (!np) { > > pr_err("no thermal zone np\n"); > > @@ -851,7 +852,7 @@ __init *thermal_of_build_thermal_zone(struct device_node *np) > > * one sensor per thermal zone. Thus, we are considering > > * only the first two values as slope and offset. > > */ > > - ret = of_property_read_u32_array(np, "coefficients", coef, 2); > > + ret = of_property_read_s32_array(np, "coefficients", coef, 2); > > if (ret == 0) { > > tz->slope = coef[0]; > > tz->offset = coef[1]; > > > > Since you are doing so much work to fix reading the array of s32 property, you > might also want to do the same for the s32 properties, "polling-delay-passive" > and "polling-delay". Just change of_property_read_u32() to of_property_read_s32() > and change the type of prop to match. > The intension behind this patch series is adding a new thermal driver not fixing of-thermal. Since the initial version of this series was posted by Martin in May 2016 i do not want to wait much longer. Btw changing polling-delay-passive and polling-delay into a signed doesn't make any sense to me. Why do we need negative delays? I suggest to send a separate patch for this issue. Thanks for the review. > > drivers/thermal/of-thermal.c: In function 'thermal_of_build_thermal_zone': > drivers/thermal/of-thermal.c:841:2: warning: conversion to 'int' from 'u32' may change the sign of the result [-Wsign-conversion] > drivers/thermal/of-thermal.c:848:2: warning: conversion to 'int' from 'u32' may change the sign of the result [-Wsign-conversion] > > > 836 ret = of_property_read_u32(np, "polling-delay-passive", &prop); > 837 if (ret < 0) { > 838 pr_err("missing polling-delay-passive property\n"); > 839 goto free_tz; > 840 } > 841 tz->passive_delay = prop; > 842 > 843 ret = of_property_read_u32(np, "polling-delay", &prop); > 844 if (ret < 0) { > 845 pr_err("missing polling-delay property\n"); > 846 goto free_tz; > 847 } > 848 tz->polling_delay = prop; > > > -Frank
On 03/24/17 00:27, Stefan Wahren wrote: > >> Frank Rowand <frowand.list@gmail.com> hat am 24. März 2017 um 00:32 geschrieben: >> >> >> On 03/12/17 15:11, Stefan Wahren wrote: >>> Use the new function of_property_read_s32_array() to prepare >>> of-thermal for negative coefficients. These are used by >>> the upcoming bcm2835_thermal driver. >>> >>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> >>> --- >>> drivers/thermal/of-thermal.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c >>> index d04ec3b..491d58a 100644 >>> --- a/drivers/thermal/of-thermal.c >>> +++ b/drivers/thermal/of-thermal.c >>> @@ -821,7 +821,8 @@ __init *thermal_of_build_thermal_zone(struct device_node *np) >>> struct device_node *child = NULL, *gchild; >>> struct __thermal_zone *tz; >>> int ret, i; >>> - u32 prop, coef[2]; >>> + u32 prop; >>> + s32 coef[2]; >>> >>> if (!np) { >>> pr_err("no thermal zone np\n"); >>> @@ -851,7 +852,7 @@ __init *thermal_of_build_thermal_zone(struct device_node *np) >>> * one sensor per thermal zone. Thus, we are considering >>> * only the first two values as slope and offset. >>> */ >>> - ret = of_property_read_u32_array(np, "coefficients", coef, 2); >>> + ret = of_property_read_s32_array(np, "coefficients", coef, 2); >>> if (ret == 0) { >>> tz->slope = coef[0]; >>> tz->offset = coef[1]; >>> >> >> Since you are doing so much work to fix reading the array of s32 property, you >> might also want to do the same for the s32 properties, "polling-delay-passive" >> and "polling-delay". Just change of_property_read_u32() to of_property_read_s32() >> and change the type of prop to match. >> > > The intension behind this patch series is adding a new thermal driver > not fixing of-thermal. Since the initial version of this series was > posted by Martin in May 2016 i do not want to wait much longer. Not my driver, your choice, I was just pointing out some compile warnings (without thinking enough about the context of the warnings - see below). > > Btw changing polling-delay-passive and polling-delay into a signed > doesn't make any sense to me. Why do we need negative delays? Good point. I did not actually look at what the meaning of the two properties is, and whether it makes sense for them to have a negative value. I also did not check the binding description (doing so now, it clearly states that these property values are unsigned), I just noted that there is a mismatch between the type of the properties (u32) and the variables they are assigned to. As the compile warnings below indicate, if the property value is large enough, it will be a negative value after assignment to the int variable. The proper answer is probably to change the variables to unsigned. > > I suggest to send a separate patch for this issue. It would be good if someone did. Not a critical issue, just a trap waiting to catch someone who puts a very large value for one of those properties in a device tree source file and does not realize it will be a negative number in the driver. > > Thanks for the review. > >> >> drivers/thermal/of-thermal.c: In function 'thermal_of_build_thermal_zone': >> drivers/thermal/of-thermal.c:841:2: warning: conversion to 'int' from 'u32' may change the sign of the result [-Wsign-conversion] >> drivers/thermal/of-thermal.c:848:2: warning: conversion to 'int' from 'u32' may change the sign of the result [-Wsign-conversion] >> >> >> 836 ret = of_property_read_u32(np, "polling-delay-passive", &prop); >> 837 if (ret < 0) { >> 838 pr_err("missing polling-delay-passive property\n"); >> 839 goto free_tz; >> 840 } >> 841 tz->passive_delay = prop; >> 842 >> 843 ret = of_property_read_u32(np, "polling-delay", &prop); >> 844 if (ret < 0) { >> 845 pr_err("missing polling-delay property\n"); >> 846 goto free_tz; >> 847 } >> 848 tz->polling_delay = prop; >> >> >> -Frank >
Stefen, On Sun, Mar 12, 2017 at 10:11:01PM +0000, Stefan Wahren wrote: > Use the new function of_property_read_s32_array() to prepare > of-thermal for negative coefficients. These are used by > the upcoming bcm2835_thermal driver. > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> Once done with the comment pointed by Frank, you can add my Reviewed-by: Eduardo Valentin <edubezval@gmail.com> > --- > drivers/thermal/of-thermal.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c > index d04ec3b..491d58a 100644 > --- a/drivers/thermal/of-thermal.c > +++ b/drivers/thermal/of-thermal.c > @@ -821,7 +821,8 @@ __init *thermal_of_build_thermal_zone(struct device_node *np) > struct device_node *child = NULL, *gchild; > struct __thermal_zone *tz; > int ret, i; > - u32 prop, coef[2]; > + u32 prop; > + s32 coef[2]; > > if (!np) { > pr_err("no thermal zone np\n"); > @@ -851,7 +852,7 @@ __init *thermal_of_build_thermal_zone(struct device_node *np) > * one sensor per thermal zone. Thus, we are considering > * only the first two values as slope and offset. > */ > - ret = of_property_read_u32_array(np, "coefficients", coef, 2); > + ret = of_property_read_s32_array(np, "coefficients", coef, 2); > if (ret == 0) { > tz->slope = coef[0]; > tz->offset = coef[1]; > -- > 1.7.9.5 >
On Sun, Mar 12, 2017 at 10:11:01PM +0000, Stefan Wahren wrote: > Use the new function of_property_read_s32_array() to prepare > of-thermal for negative coefficients. These are used by > the upcoming bcm2835_thermal driver. > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> > --- > drivers/thermal/of-thermal.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c > index d04ec3b..491d58a 100644 > --- a/drivers/thermal/of-thermal.c > +++ b/drivers/thermal/of-thermal.c > @@ -821,7 +821,8 @@ __init *thermal_of_build_thermal_zone(struct device_node *np) > struct device_node *child = NULL, *gchild; > struct __thermal_zone *tz; > int ret, i; > - u32 prop, coef[2]; > + u32 prop; > + s32 coef[2]; > > if (!np) { > pr_err("no thermal zone np\n"); > @@ -851,7 +852,7 @@ __init *thermal_of_build_thermal_zone(struct device_node *np) > * one sensor per thermal zone. Thus, we are considering > * only the first two values as slope and offset. > */ > - ret = of_property_read_u32_array(np, "coefficients", coef, 2); > + ret = of_property_read_s32_array(np, "coefficients", coef, 2); For the scope of the proposed change I see not problem with this patch. However, this needs to go in after the of core change gets accepted. Alternatively, I could take the full series if you get an Ack from OF maintainers. > if (ret == 0) { > tz->slope = coef[0]; > tz->offset = coef[1]; > -- > 1.7.9.5 >
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c index d04ec3b..491d58a 100644 --- a/drivers/thermal/of-thermal.c +++ b/drivers/thermal/of-thermal.c @@ -821,7 +821,8 @@ __init *thermal_of_build_thermal_zone(struct device_node *np) struct device_node *child = NULL, *gchild; struct __thermal_zone *tz; int ret, i; - u32 prop, coef[2]; + u32 prop; + s32 coef[2]; if (!np) { pr_err("no thermal zone np\n"); @@ -851,7 +852,7 @@ __init *thermal_of_build_thermal_zone(struct device_node *np) * one sensor per thermal zone. Thus, we are considering * only the first two values as slope and offset. */ - ret = of_property_read_u32_array(np, "coefficients", coef, 2); + ret = of_property_read_s32_array(np, "coefficients", coef, 2); if (ret == 0) { tz->slope = coef[0]; tz->offset = coef[1];
Use the new function of_property_read_s32_array() to prepare of-thermal for negative coefficients. These are used by the upcoming bcm2835_thermal driver. Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> --- drivers/thermal/of-thermal.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)