Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 01/17] thermal: add thermal_zone_device_toggle() helper
To:     Eduardo Valentin <edubezval@gmail.com>
Cc:     Zhang Rui <rui.zhang@intel.com>, Eric Anholt <eric@anholt.net>,
        Stefan Wahren <stefan.wahren@i2se.com>,
        Markus Mayer <mmayer@broadcom.com>,
        bcm-kernel-feedback-list@broadcom.com,
        Heiko Stuebner <heiko@sntech.de>,
        Thierry Reding <thierry.reding@gmail.com>,
        Jonathan Hunter <jonathanh@nvidia.com>,
        Keerthy <j-keerthy@ti.com>,
        Masahiro Yamada <yamada.masahiro@socionext.com>,
        Jun Nie <jun.nie@linaro.org>,
        Baoyou Xie <baoyou.xie@linaro.org>,
        Shawn Guo <shawnguo@kernel.org>, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org
From:   Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Date:   Fri, 14 Sep 2018 13:40:44 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
        Thunderbird/45.3.0
MIME-Version: 1.0
In-Reply-To: <20180910171605.GA4196@localhost.localdomain>
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: 7bit
Message-Id: <20180914114046eucas1p1e2d1101ec18f0ca090c9fd97c6840e78~UQV5bGFjE0451704517eucas1p17@eucas1p1.samsung.com>
CMS-TYPE: 201P
References: <1523364131-31059-1-git-send-email-b.zolnierkie@samsung.com>
        <CGME20180410124248epcas1p453f74df4e183f9d68de255847c093d4e@epcas1p4.samsung.com>
        <1523364131-31059-2-git-send-email-b.zolnierkie@samsung.com>
        <20180910171605.GA4196@localhost.localdomain>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 09/10/2018 07:16 PM, Eduardo Valentin wrote:
> On Tue, Apr 10, 2018 at 02:41:55PM +0200, Bartlomiej Zolnierkiewicz wrote:
>> Add thermal_zone_device_toggle() helper. Then update core code and
>> drivers to use it.
> 
> Cool, but I think it would be good to have some sort of rational here
> at the commit message telling why this helper is being added, 
> don't you agree?

Ok, I will enhance the patch description (the rationale is removing
code duplication and preparation for changes in the later patches from
the series).

>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>> ---
>>  drivers/thermal/hisi_thermal.c     | 14 ++------------
>>  drivers/thermal/of-thermal.c       |  2 +-
>>  drivers/thermal/rockchip_thermal.c | 22 +++++-----------------
>>  drivers/thermal/thermal_helpers.c  | 15 +++++++++++++++
>>  drivers/thermal/thermal_sysfs.c    |  8 +++++---
>>  include/linux/thermal.h            |  1 +
>>  6 files changed, 29 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
>> index 761d055..9428499 100644
>> --- a/drivers/thermal/hisi_thermal.c
>> +++ b/drivers/thermal/hisi_thermal.c
>> @@ -515,15 +515,6 @@ static int hisi_thermal_register_sensor(struct platform_device *pdev,
>>  };
>>  MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
>>  
>> -static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
>> -				       bool on)
>> -{
>> -	struct thermal_zone_device *tzd = sensor->tzd;
>> -
>> -	tzd->ops->set_mode(tzd,
>> -		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
>> -}
>> -
>>  static int hisi_thermal_probe(struct platform_device *pdev)
>>  {
>>  	struct hisi_thermal_data *data;
>> @@ -571,7 +562,7 @@ static int hisi_thermal_probe(struct platform_device *pdev)
>>  		}
>>  	}
>>  
>> -	hisi_thermal_toggle_sensor(&data->sensor, true);
>> +	thermal_zone_device_toggle((&data->sensor)->tzd, true);
>>  
>>  	return 0;
>>  }
>> @@ -579,9 +570,8 @@ static int hisi_thermal_probe(struct platform_device *pdev)
>>  static int hisi_thermal_remove(struct platform_device *pdev)
>>  {
>>  	struct hisi_thermal_data *data = platform_get_drvdata(pdev);
>> -	struct hisi_thermal_sensor *sensor = &data->sensor;
>>  
>> -	hisi_thermal_toggle_sensor(sensor, false);
>> +	thermal_zone_device_toggle((&data->sensor)->tzd, false);
>>  
>>  	data->disable_sensor(data);
>>  
>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>> index e09f035..f138b78 100644
>> --- a/drivers/thermal/of-thermal.c
>> +++ b/drivers/thermal/of-thermal.c
>> @@ -510,7 +510,7 @@ struct thermal_zone_device *
>>  			tzd = thermal_zone_of_add_sensor(child, sensor_np,
>>  							 data, ops);
>>  			if (!IS_ERR(tzd))
>> -				tzd->ops->set_mode(tzd, THERMAL_DEVICE_ENABLED);
>> +				thermal_zone_device_toggle(tzd, true);
>>  
>>  			of_node_put(sensor_specs.np);
>>  			of_node_put(child);
>> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
>> index f36375d..c191e41 100644
>> --- a/drivers/thermal/rockchip_thermal.c
>> +++ b/drivers/thermal/rockchip_thermal.c
>> @@ -1022,15 +1022,6 @@ static void rk_tsadcv2_tshut_mode(int chn, void __iomem *regs,
>>  };
>>  MODULE_DEVICE_TABLE(of, of_rockchip_thermal_match);
>>  
>> -static void
>> -rockchip_thermal_toggle_sensor(struct rockchip_thermal_sensor *sensor, bool on)
>> -{
>> -	struct thermal_zone_device *tzd = sensor->tzd;
>> -
>> -	tzd->ops->set_mode(tzd,
>> -		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
>> -}
>> -
>>  static irqreturn_t rockchip_thermal_alarm_irq_thread(int irq, void *dev)
>>  {
>>  	struct rockchip_thermal_data *thermal = dev;
>> @@ -1292,7 +1283,7 @@ static int rockchip_thermal_probe(struct platform_device *pdev)
>>  	thermal->chip->control(thermal->regs, true);
>>  
>>  	for (i = 0; i < thermal->chip->chn_num; i++)
>> -		rockchip_thermal_toggle_sensor(&thermal->sensors[i], true);
>> +		thermal_zone_device_toggle((&thermal->sensors[i])->tzd, true);
>>  
>>  	platform_set_drvdata(pdev, thermal);
>>  
>> @@ -1311,11 +1302,8 @@ static int rockchip_thermal_remove(struct platform_device *pdev)
>>  	struct rockchip_thermal_data *thermal = platform_get_drvdata(pdev);
>>  	int i;
>>  
>> -	for (i = 0; i < thermal->chip->chn_num; i++) {
>> -		struct rockchip_thermal_sensor *sensor = &thermal->sensors[i];
>> -
>> -		rockchip_thermal_toggle_sensor(sensor, false);
>> -	}
>> +	for (i = 0; i < thermal->chip->chn_num; i++)
>> +		thermal_zone_device_toggle((&thermal->sensors[i])->tzd, false);
>>  
>>  	thermal->chip->control(thermal->regs, false);
>>  
>> @@ -1332,7 +1320,7 @@ static int __maybe_unused rockchip_thermal_suspend(struct device *dev)
>>  	int i;
>>  
>>  	for (i = 0; i < thermal->chip->chn_num; i++)
>> -		rockchip_thermal_toggle_sensor(&thermal->sensors[i], false);
>> +		thermal_zone_device_toggle((&thermal->sensors[i])->tzd, false);
>>  
>>  	thermal->chip->control(thermal->regs, false);
>>  
>> @@ -1383,7 +1371,7 @@ static int __maybe_unused rockchip_thermal_resume(struct device *dev)
>>  	thermal->chip->control(thermal->regs, true);
>>  
>>  	for (i = 0; i < thermal->chip->chn_num; i++)
>> -		rockchip_thermal_toggle_sensor(&thermal->sensors[i], true);
>> +		thermal_zone_device_toggle((&thermal->sensors[i])->tzd, true);
>>  
>>  	pinctrl_pm_select_default_state(dev);
>>  
>> diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
>> index eb03d7e..d5db101 100644
>> --- a/drivers/thermal/thermal_helpers.c
>> +++ b/drivers/thermal/thermal_helpers.c
>> @@ -227,3 +227,18 @@ int thermal_zone_get_offset(struct thermal_zone_device *tz)
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(thermal_zone_get_offset);
>> +
>> +/**
>> + * thermal_zone_device_toggle() - enables/disables thermal zone device
>> + * @tz: a valid pointer to a struct thermal_zone_device
>> + *
>> + * Enables/Disables thermal zone device.
>> + *
>> + * Return: On success returns 0, an error code otherwise.
>> + */
>> +int thermal_zone_device_toggle(struct thermal_zone_device *tz, bool on)
>> +{
>> +	return tz->ops->set_mode(tz,
>> +		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
> 
> why not simply getting a mode type (enum thermal_device_mode)?

Well, none of open-coded instances that thermal_zone_device_toggle()
replaces used enum thermal_device_mode. Using bool is simpler and
this enum looks a bit like over-engineering, though I can use it if
you insist.

>> +}
>> +EXPORT_SYMBOL_GPL(thermal_zone_device_toggle);
> 
> I know naming is always the hardest part :-), but to me, this function
> is more a thermal_zone_set_mode() than device_toggle(). At least, based
> on what I see in the code, we are not really toggling device per si,
> just setting a mode.

Ok, I will change it (the naming was based on existing helpers from
hisi_thermal.c and rockchip_thermal.c).

>> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
>> index 23b5e0a..343f52b 100644
>> --- a/drivers/thermal/thermal_sysfs.c
>> +++ b/drivers/thermal/thermal_sysfs.c
>> @@ -72,17 +72,19 @@
>>  {
>>  	struct thermal_zone_device *tz = to_thermal_zone(dev);
>>  	int result;
>> +	bool on;
>>  
>>  	if (!tz->ops->set_mode)
>>  		return -EPERM;
>>  
>>  	if (!strncmp(buf, "enabled", sizeof("enabled") - 1))
>> -		result = tz->ops->set_mode(tz, THERMAL_DEVICE_ENABLED);
>> +		on = true;
>>  	else if (!strncmp(buf, "disabled", sizeof("disabled") - 1))
>> -		result = tz->ops->set_mode(tz, THERMAL_DEVICE_DISABLED);
>> +		on = false;
>>  	else
>> -		result = -EINVAL;
>> +		return -EINVAL;
>>  
>> +	result = thermal_zone_device_toggle(tz, on);
>>  	if (result)
>>  		return result;
>>  
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index 7834be6..4fbbabe 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -467,6 +467,7 @@ struct thermal_cooling_device *
>>  int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
>>  int thermal_zone_get_slope(struct thermal_zone_device *tz);
>>  int thermal_zone_get_offset(struct thermal_zone_device *tz);
>> +int thermal_zone_device_toggle(struct thermal_zone_device *tz, bool on);
>>  
> 
> You missed the #else section of the #if IS_ENABLED(CONFIG_THERMAL),
> didn't you?

No, this function shouldn't be used outside of CONFIG_THERMAL.

> We want to have a stub when config thermal is not set.

Actually it seems that a lot of these existing stubs don't make sense
outside of CONFIG_THERMAL and should be cleaned up.

>>  int get_tz_trend(struct thermal_zone_device *, int);
>>  struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics