From: Guenter Roeck <linux@roeck-us.net>
To: Daniel Lezcano <daniel.lezcano@linaro.org>, rui.zhang@intel.com
Cc: amit.kucheria@verdurent.com, linux-kernel@vger.kernel.org,
Kamil Debski <kamil@wypas.org>,
Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
Jean Delvare <jdelvare@suse.com>,
"open list:PWM FAN DRIVER" <linux-hwmon@vger.kernel.org>
Subject: Re: [PATCH 1/6] thermal: hwmon: Replace the call the thermal_cdev_update()
Date: Sat, 11 Apr 2020 10:26:59 -0700 [thread overview]
Message-ID: <bf81a4db-9687-b9b2-4976-64bdd364b101@roeck-us.net> (raw)
In-Reply-To: <907914e7-7f5a-e66d-bf38-be110aa1f6f0@linaro.org>
On 4/11/20 9:45 AM, Daniel Lezcano wrote:
> On 11/04/2020 03:32, Guenter Roeck wrote:
>> On 4/10/20 3:12 PM, Daniel Lezcano wrote:
>>> The function thermal_cdev_upadte is called from the throttling
>>
>> misspelled
>>
>>> functions in the governors not from the cooling device itself.
>>>
>>> The cooling device is set to its maximum state and then updated. Even
>>> if I don't get the purpose of probing the pwm-fan to its maximum
>>> cooling state, we can replace the thermal_cdev_update() call to the
>>> internal set_cur_state() function directly.
>>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> ---
>>> drivers/hwmon/pwm-fan.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
>>> index 30b7b3ea8836..a654ecdf21ab 100644
>>> --- a/drivers/hwmon/pwm-fan.c
>>> +++ b/drivers/hwmon/pwm-fan.c
>>> @@ -372,7 +372,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
>>> if (ret)
>>> return ret;
>>>
>>> - ctx->pwm_fan_state = ctx->pwm_fan_max_state;
>>> if (IS_ENABLED(CONFIG_THERMAL)) {
>>> cdev = devm_thermal_of_cooling_device_register(dev,
>>> dev->of_node, "pwm-fan", ctx, &pwm_fan_cooling_ops);
>>> @@ -384,7 +383,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
>>> return ret;
>>> }
>>> ctx->cdev = cdev;
>>> - thermal_cdev_update(cdev);
>>> + pwm_fan_set_cur_state(cdev, ctx->pwm_fan_max_state);
>>
>> So far the function would only change the state if the new
>> state is not equal to the old state. This was the case because
>> pwm_fan_state was set to pwm_fan_max_state, and the call to
>> thermal_cdev_update() and thus pwm_fan_set_cur_state() would
>> do nothing except update statistics. The old code _assumed_
>> that the current state is pwm_fan_max_state. The new code
>> enforces it. That is a substantial semantic change, and it
>> is not really reflected in the commit message. Is that really
>> what you want ? If so, the commit message needs to state that
>> and explain the rationale.
>
> Well, to be honest I'm not getting the rational of calling
> thermal_cdev_update(cdev) right after
> devm_thermal_of_cooling_device_register() neither setting pwm_fan_state
> to pwm_fan_max_state.
>
Good question. The author might know/recall. Maybe the idea was that
thermal would update the state to a lower state shortly thereafter.
> Do we have the guarantee there is at this point a thermal instance
> making the target state working when thermal_cdev_update is called?
>
> Are we sure a thermal_cdev_update(cdev) is actually right here?
>
I don't know. I am not exactly familiar with thermal subsystem
particulars. I do recall seeing similar code in other drivers, though.
Either case, your patch does change functionality, and we should not
do that without understanding its impact.
Thanks
Guenter
next prev parent reply other threads:[~2020-04-11 17:27 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20200410221236.6484-1-daniel.lezcano@linaro.org>
2020-04-10 22:12 ` [PATCH 1/6] thermal: hwmon: Replace the call the thermal_cdev_update() Daniel Lezcano
2020-04-11 1:32 ` Guenter Roeck
2020-04-11 16:45 ` Daniel Lezcano
2020-04-11 17:26 ` Guenter Roeck [this message]
2020-04-11 21:07 ` Daniel Lezcano
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=bf81a4db-9687-b9b2-4976-64bdd364b101@roeck-us.net \
--to=linux@roeck-us.net \
--cc=amit.kucheria@verdurent.com \
--cc=b.zolnierkie@samsung.com \
--cc=daniel.lezcano@linaro.org \
--cc=jdelvare@suse.com \
--cc=kamil@wypas.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rui.zhang@intel.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 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).