From: Frank Rowand <frowand.list@gmail.com>
To: Stefan Wahren <stefan.wahren@i2se.com>,
kernel@martin.sperl.org, Zhang Rui <rui.zhang@intel.com>,
Eduardo Valentin <edubezval@gmail.com>
Cc: Eric Anholt <eric@anholt.net>, Rob Herring <robh+dt@kernel.org>,
Florian Fainelli <f.fainelli@gmail.com>,
linux-rpi-kernel@lists.infradead.org, linux-pm@vger.kernel.org,
devicetree@vger.kernel.org
Subject: Re: [PATCH V11 2/6] thermal: of-thermal: Implement signed coefficient support
Date: Fri, 24 Mar 2017 10:23:24 -0700 [thread overview]
Message-ID: <58D5560C.7040503@gmail.com> (raw)
In-Reply-To: <872776681.149555.1490340443231@email.1und1.de>
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
>
next prev parent reply other threads:[~2017-03-24 17:23 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-12 22:10 [PATCH V11 0/6] thermal: bcm2835: add thermal driver for bcm2835 SoC Stefan Wahren
[not found] ` <1489356665-3175-1-git-send-email-stefan.wahren-eS4NqCHxEME@public.gmane.org>
2017-03-12 22:11 ` [PATCH V11 1/6] of: base: Implement read function for s32 array Stefan Wahren
2017-03-23 23:26 ` Frank Rowand
2017-03-12 22:11 ` [PATCH V11 2/6] thermal: of-thermal: Implement signed coefficient support Stefan Wahren
[not found] ` <1489356665-3175-3-git-send-email-stefan.wahren-eS4NqCHxEME@public.gmane.org>
2017-03-23 23:32 ` Frank Rowand
2017-03-24 7:27 ` Stefan Wahren
2017-03-24 17:23 ` Frank Rowand [this message]
2017-03-29 4:52 ` Eduardo Valentin
2017-03-29 4:54 ` Eduardo Valentin
2017-03-12 22:11 ` [PATCH V11 3/6] dt-bindings: Add thermal zone to bcm2835-thermal example Stefan Wahren
2017-03-12 22:11 ` [PATCH V11 4/6] ARM: dts: bcm283x: Add CPU thermal zone with 1 trip point Stefan Wahren
2017-03-12 22:11 ` [PATCH V11 5/6] ARM64: dts: bcm2837: Define CPU thermal coefficients Stefan Wahren
2017-03-12 22:11 ` [PATCH V11 6/6] thermal: bcm2835: add thermal driver for bcm2835 SoC Stefan Wahren
2017-03-23 16:53 ` Nobuhiro Iwamatsu
[not found] ` <CABMQnVJx10e2qm0N=C_vxU9WAQGUJ3ZyAUgrK7WQZoxm9+P0wg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-03-23 18:52 ` Stefan Wahren
[not found] ` <1489356665-3175-7-git-send-email-stefan.wahren-eS4NqCHxEME@public.gmane.org>
2017-03-29 4:58 ` Eduardo Valentin
2017-03-30 4:57 ` Eduardo Valentin
[not found] ` <20170330045725.GA12995-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
2017-03-30 6:30 ` Stefan Wahren
[not found] ` <1205844664.21224.1490855429497-7tX72C7vayboQLBSYMtkGA@public.gmane.org>
2017-03-31 1:08 ` Eduardo Valentin
2017-03-30 19:11 ` Stefan Wahren
[not found] ` <1620097357.332334.1490901102005-7tX72C7vayboQLBSYMtkGA@public.gmane.org>
2017-03-31 1:06 ` Eduardo Valentin
2017-03-22 19:30 ` [PATCH V11 0/6] " Stefan Wahren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=58D5560C.7040503@gmail.com \
--to=frowand.list@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=edubezval@gmail.com \
--cc=eric@anholt.net \
--cc=f.fainelli@gmail.com \
--cc=kernel@martin.sperl.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-rpi-kernel@lists.infradead.org \
--cc=robh+dt@kernel.org \
--cc=rui.zhang@intel.com \
--cc=stefan.wahren@i2se.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.