* Re: [PATCH v4 02/11] thermal: Store thermal mode in a dedicated enum
2020-05-28 19:20 ` [PATCH v4 02/11] thermal: Store thermal mode in a dedicated enum Andrzej Pietrasiewicz
@ 2020-05-29 14:48 ` Guenter Roeck
2020-05-29 15:13 ` Andrzej Pietrasiewicz
2020-06-01 11:36 ` Peter Kästle
2020-06-24 9:38 ` Bartlomiej Zolnierkiewicz
2 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2020-05-29 14:48 UTC (permalink / raw)
To: Andrzej Pietrasiewicz
Cc: linux-pm, linux-acpi, netdev, linux-wireless, platform-driver-x86,
linux-arm-kernel, linux-renesas-soc, linux-rockchip,
Emmanuel Grumbach, Heiko Stuebner, Rafael J . Wysocki,
Vishal Kulkarni, Luca Coelho, Miquel Raynal, kernel,
Fabio Estevam, Amit Kucheria, Chunyan Zhang, Daniel Lezcano,
Allison Randal, NXP Linux Team
On Thu, May 28, 2020 at 09:20:42PM +0200, Andrzej Pietrasiewicz wrote:
> Prepare for storing mode in struct thermal_zone_device.
>
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> ---
> drivers/acpi/thermal.c | 27 +++++++++----------
> drivers/platform/x86/acerhdf.c | 8 ++++--
> .../intel/int340x_thermal/int3400_thermal.c | 18 +++++--------
> 3 files changed, 25 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index 6de8066ca1e7..fb46070c66d8 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -172,7 +172,7 @@ struct acpi_thermal {
> struct acpi_thermal_trips trips;
> struct acpi_handle_list devices;
> struct thermal_zone_device *thermal_zone;
> - int tz_enabled;
> + enum thermal_device_mode mode;
> int kelvin_offset; /* in millidegrees */
> struct work_struct thermal_check_work;
> };
> @@ -500,7 +500,7 @@ static void acpi_thermal_check(void *data)
> {
> struct acpi_thermal *tz = data;
>
> - if (!tz->tz_enabled)
> + if (tz->mode != THERMAL_DEVICE_ENABLED)
> return;
>
> thermal_zone_device_update(tz->thermal_zone,
> @@ -534,8 +534,7 @@ static int thermal_get_mode(struct thermal_zone_device *thermal,
> if (!tz)
> return -EINVAL;
>
> - *mode = tz->tz_enabled ? THERMAL_DEVICE_ENABLED :
> - THERMAL_DEVICE_DISABLED;
> + *mode = tz->mode;
>
> return 0;
> }
> @@ -544,27 +543,25 @@ static int thermal_set_mode(struct thermal_zone_device *thermal,
> enum thermal_device_mode mode)
> {
> struct acpi_thermal *tz = thermal->devdata;
> - int enable;
>
> if (!tz)
> return -EINVAL;
>
> + if (mode != THERMAL_DEVICE_DISABLED &&
> + mode != THERMAL_DEVICE_ENABLED)
> + return -EINVAL;
Personally I find this check unnecessary: The enum has no other values,
and it is verifyable that the callers provide the enum and not some other
value.
> /*
> * enable/disable thermal management from ACPI thermal driver
> */
> - if (mode == THERMAL_DEVICE_ENABLED)
> - enable = 1;
> - else if (mode == THERMAL_DEVICE_DISABLED) {
> - enable = 0;
> + if (mode == THERMAL_DEVICE_DISABLED)
> pr_warn("thermal zone will be disabled\n");
> - } else
> - return -EINVAL;
>
> - if (enable != tz->tz_enabled) {
> - tz->tz_enabled = enable;
> + if (mode != tz->mode) {
> + tz->mode = mode;
> ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> "%s kernel ACPI thermal control\n",
> - tz->tz_enabled ? "Enable" : "Disable"));
> + tz->mode == THERMAL_DEVICE_ENABLED ?
> + "Enable" : "Disable"));
> acpi_thermal_check(tz);
> }
> return 0;
> @@ -915,7 +912,7 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
> goto remove_dev_link;
> }
>
> - tz->tz_enabled = 1;
> + tz->mode = THERMAL_DEVICE_ENABLED;
>
> dev_info(&tz->device->dev, "registered as thermal_zone%d\n",
> tz->thermal_zone->id);
> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> index 8cc86f4e3ac1..830a8b060e74 100644
> --- a/drivers/platform/x86/acerhdf.c
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -68,6 +68,7 @@ static int kernelmode = 1;
> #else
> static int kernelmode;
> #endif
> +static enum thermal_device_mode thermal_mode;
>
> static unsigned int interval = 10;
> static unsigned int fanon = 60000;
> @@ -397,6 +398,7 @@ static inline void acerhdf_revert_to_bios_mode(void)
> {
> acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
> kernelmode = 0;
> + thermal_mode = THERMAL_DEVICE_DISABLED;
> if (thz_dev)
> thz_dev->polling_delay = 0;
> pr_notice("kernel mode fan control OFF\n");
> @@ -404,6 +406,7 @@ static inline void acerhdf_revert_to_bios_mode(void)
> static inline void acerhdf_enable_kernelmode(void)
> {
> kernelmode = 1;
> + thermal_mode = THERMAL_DEVICE_ENABLED;
>
> thz_dev->polling_delay = interval*1000;
> thermal_zone_device_update(thz_dev, THERMAL_EVENT_UNSPECIFIED);
> @@ -416,8 +419,7 @@ static int acerhdf_get_mode(struct thermal_zone_device *thermal,
> if (verbose)
> pr_notice("kernel mode fan control %d\n", kernelmode);
>
> - *mode = (kernelmode) ? THERMAL_DEVICE_ENABLED
> - : THERMAL_DEVICE_DISABLED;
> + *mode = thermal_mode;
>
> return 0;
> }
> @@ -739,6 +741,8 @@ static int __init acerhdf_register_thermal(void)
> if (IS_ERR(cl_dev))
> return -EINVAL;
>
> + thermal_mode = kernelmode ?
> + THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED;
> thz_dev = thermal_zone_device_register("acerhdf", 2, 0, NULL,
> &acerhdf_dev_ops,
> &acerhdf_zone_params, 0,
> diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> index 0b3a62655843..e84faaadff87 100644
> --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> @@ -48,7 +48,7 @@ struct int3400_thermal_priv {
> struct acpi_device *adev;
> struct platform_device *pdev;
> struct thermal_zone_device *thermal;
> - int mode;
> + enum thermal_device_mode mode;
> int art_count;
> struct art *arts;
> int trt_count;
> @@ -395,24 +395,20 @@ static int int3400_thermal_set_mode(struct thermal_zone_device *thermal,
> enum thermal_device_mode mode)
> {
> struct int3400_thermal_priv *priv = thermal->devdata;
> - bool enable;
> int result = 0;
>
> if (!priv)
> return -EINVAL;
>
> - if (mode == THERMAL_DEVICE_ENABLED)
> - enable = true;
> - else if (mode == THERMAL_DEVICE_DISABLED)
> - enable = false;
> - else
> + if (mode != THERMAL_DEVICE_ENABLED &&
> + mode != THERMAL_DEVICE_DISABLED)
> return -EINVAL;
Same as above.
>
> - if (enable != priv->mode) {
> - priv->mode = enable;
> + if (mode != priv->mode) {
> + priv->mode = mode;
> result = int3400_thermal_run_osc(priv->adev->handle,
> - priv->current_uuid_index,
> - enable);
> + priv->current_uuid_index,
> + mode == THERMAL_DEVICE_ENABLED);
> }
>
> evaluate_odvp(priv);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v4 02/11] thermal: Store thermal mode in a dedicated enum
2020-05-29 14:48 ` Guenter Roeck
@ 2020-05-29 15:13 ` Andrzej Pietrasiewicz
2020-05-29 15:34 ` Guenter Roeck
0 siblings, 1 reply; 9+ messages in thread
From: Andrzej Pietrasiewicz @ 2020-05-29 15:13 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-pm, linux-acpi, netdev, linux-wireless, platform-driver-x86,
linux-arm-kernel, linux-renesas-soc, linux-rockchip,
Emmanuel Grumbach, Heiko Stuebner, Rafael J . Wysocki,
Vishal Kulkarni, Luca Coelho, Miquel Raynal, kernel,
Fabio Estevam, Amit Kucheria, Chunyan Zhang, Daniel Lezcano,
Allison Randal, NXP Linux Team
Hi Guenter,
W dniu 29.05.2020 o 16:48, Guenter Roeck pisze:
> On Thu, May 28, 2020 at 09:20:42PM +0200, Andrzej Pietrasiewicz wrote:
>> Prepare for storing mode in struct thermal_zone_device.
>>
>> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
>> ---
>> drivers/acpi/thermal.c | 27 +++++++++----------
>> drivers/platform/x86/acerhdf.c | 8 ++++--
>> .../intel/int340x_thermal/int3400_thermal.c | 18 +++++--------
>> 3 files changed, 25 insertions(+), 28 deletions(-)
<snip>
>> @@ -544,27 +543,25 @@ static int thermal_set_mode(struct thermal_zone_device *thermal,
>> enum thermal_device_mode mode)
>> {
>> struct acpi_thermal *tz = thermal->devdata;
>> - int enable;
>>
>> if (!tz)
>> return -EINVAL;
>>
>> + if (mode != THERMAL_DEVICE_DISABLED &&
>> + mode != THERMAL_DEVICE_ENABLED)
>> + return -EINVAL;
>
> Personally I find this check unnecessary: The enum has no other values,
> and it is verifyable that the callers provide the enum and not some other
> value.
It is getting removed in PATCH 10/11.
>> + if (mode != THERMAL_DEVICE_ENABLED &&
>> + mode != THERMAL_DEVICE_DISABLED)
>> return -EINVAL;
>
> Same as above.
ditto.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v4 02/11] thermal: Store thermal mode in a dedicated enum
2020-05-29 15:13 ` Andrzej Pietrasiewicz
@ 2020-05-29 15:34 ` Guenter Roeck
0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2020-05-29 15:34 UTC (permalink / raw)
To: Andrzej Pietrasiewicz
Cc: linux-pm, linux-acpi, netdev, linux-wireless, platform-driver-x86,
linux-arm-kernel, linux-renesas-soc, linux-rockchip,
Emmanuel Grumbach, Heiko Stuebner, Rafael J . Wysocki,
Vishal Kulkarni, Luca Coelho, Miquel Raynal, kernel,
Fabio Estevam, Amit Kucheria, Chunyan Zhang, Daniel Lezcano,
Allison Randal, NXP Linux Team
On 5/29/20 8:13 AM, Andrzej Pietrasiewicz wrote:
> Hi Guenter,
>
> W dniu 29.05.2020 o 16:48, Guenter Roeck pisze:
>> On Thu, May 28, 2020 at 09:20:42PM +0200, Andrzej Pietrasiewicz wrote:
>>> Prepare for storing mode in struct thermal_zone_device.
>>>
>>> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
>>> ---
>>> drivers/acpi/thermal.c | 27 +++++++++----------
>>> drivers/platform/x86/acerhdf.c | 8 ++++--
>>> .../intel/int340x_thermal/int3400_thermal.c | 18 +++++--------
>>> 3 files changed, 25 insertions(+), 28 deletions(-)
>
> <snip>
>
>>> @@ -544,27 +543,25 @@ static int thermal_set_mode(struct thermal_zone_device *thermal,
>>> enum thermal_device_mode mode)
>>> {
>>> struct acpi_thermal *tz = thermal->devdata;
>>> - int enable;
>>> if (!tz)
>>> return -EINVAL;
>>> + if (mode != THERMAL_DEVICE_DISABLED &&
>>> + mode != THERMAL_DEVICE_ENABLED)
>>> + return -EINVAL;
>>
>> Personally I find this check unnecessary: The enum has no other values,
>> and it is verifyable that the callers provide the enum and not some other
>> value.
>
> It is getting removed in PATCH 10/11.
>
>
>>> + if (mode != THERMAL_DEVICE_ENABLED &&
>>> + mode != THERMAL_DEVICE_DISABLED)
>>> return -EINVAL;
>>
>> Same as above.
>
> ditto.
Hmm, I think that would be better done with this patch. But I guess
that is a bit of PoV, so
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
since I don't have any other objections/observations.
Guenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 02/11] thermal: Store thermal mode in a dedicated enum
2020-05-28 19:20 ` [PATCH v4 02/11] thermal: Store thermal mode in a dedicated enum Andrzej Pietrasiewicz
2020-05-29 14:48 ` Guenter Roeck
@ 2020-06-01 11:36 ` Peter Kästle
2020-06-24 9:38 ` Bartlomiej Zolnierkiewicz
2 siblings, 0 replies; 9+ messages in thread
From: Peter Kästle @ 2020-06-01 11:36 UTC (permalink / raw)
To: Andrzej Pietrasiewicz, linux-pm, linux-acpi, netdev,
linux-wireless, platform-driver-x86, linux-arm-kernel,
linux-renesas-soc, linux-rockchip
Cc: Rafael J . Wysocki, Len Brown, Vishal Kulkarni, David S . Miller,
Jiri Pirko, Ido Schimmel, Johannes Berg, Emmanuel Grumbach,
Luca Coelho, Intel Linux Wireless, Kalle Valo, Darren Hart,
Andy Shevchenko, Sebastian Reichel, Miquel Raynal, Daniel Lezcano,
Amit Kucheria, Support Opensource
28. Mai 2020 21:21, "Andrzej Pietrasiewicz" <andrzej.p@collabora.com> schrieb:
> Prepare for storing mode in struct thermal_zone_device.
>
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> ---
[...]
> drivers/platform/x86/acerhdf.c | 8 ++++--
Acked-by: Peter Kaestle <peter@piie.net>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 02/11] thermal: Store thermal mode in a dedicated enum
2020-05-28 19:20 ` [PATCH v4 02/11] thermal: Store thermal mode in a dedicated enum Andrzej Pietrasiewicz
2020-05-29 14:48 ` Guenter Roeck
2020-06-01 11:36 ` Peter Kästle
@ 2020-06-24 9:38 ` Bartlomiej Zolnierkiewicz
2 siblings, 0 replies; 9+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2020-06-24 9:38 UTC (permalink / raw)
To: Andrzej Pietrasiewicz
Cc: linux-pm, linux-acpi, netdev, linux-wireless, platform-driver-x86,
linux-arm-kernel, linux-renesas-soc, linux-rockchip,
Rafael J . Wysocki, Len Brown, Vishal Kulkarni, David S . Miller,
Jiri Pirko, Ido Schimmel, Johannes Berg, Emmanuel Grumbach,
Luca Coelho, Intel Linux Wireless, Kalle Valo, Peter Kaestle,
Darren Hart
On 5/28/20 9:20 PM, Andrzej Pietrasiewicz wrote:
> Prepare for storing mode in struct thermal_zone_device.
>
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
> ---
> drivers/acpi/thermal.c | 27 +++++++++----------
> drivers/platform/x86/acerhdf.c | 8 ++++--
> .../intel/int340x_thermal/int3400_thermal.c | 18 +++++--------
> 3 files changed, 25 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index 6de8066ca1e7..fb46070c66d8 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -172,7 +172,7 @@ struct acpi_thermal {
> struct acpi_thermal_trips trips;
> struct acpi_handle_list devices;
> struct thermal_zone_device *thermal_zone;
> - int tz_enabled;
> + enum thermal_device_mode mode;
> int kelvin_offset; /* in millidegrees */
> struct work_struct thermal_check_work;
> };
> @@ -500,7 +500,7 @@ static void acpi_thermal_check(void *data)
> {
> struct acpi_thermal *tz = data;
>
> - if (!tz->tz_enabled)
> + if (tz->mode != THERMAL_DEVICE_ENABLED)
> return;
>
> thermal_zone_device_update(tz->thermal_zone,
> @@ -534,8 +534,7 @@ static int thermal_get_mode(struct thermal_zone_device *thermal,
> if (!tz)
> return -EINVAL;
>
> - *mode = tz->tz_enabled ? THERMAL_DEVICE_ENABLED :
> - THERMAL_DEVICE_DISABLED;
> + *mode = tz->mode;
>
> return 0;
> }
> @@ -544,27 +543,25 @@ static int thermal_set_mode(struct thermal_zone_device *thermal,
> enum thermal_device_mode mode)
> {
> struct acpi_thermal *tz = thermal->devdata;
> - int enable;
>
> if (!tz)
> return -EINVAL;
>
> + if (mode != THERMAL_DEVICE_DISABLED &&
> + mode != THERMAL_DEVICE_ENABLED)
> + return -EINVAL;
> /*
> * enable/disable thermal management from ACPI thermal driver
> */
> - if (mode == THERMAL_DEVICE_ENABLED)
> - enable = 1;
> - else if (mode == THERMAL_DEVICE_DISABLED) {
> - enable = 0;
> + if (mode == THERMAL_DEVICE_DISABLED)
> pr_warn("thermal zone will be disabled\n");
> - } else
> - return -EINVAL;
>
> - if (enable != tz->tz_enabled) {
> - tz->tz_enabled = enable;
> + if (mode != tz->mode) {
> + tz->mode = mode;
> ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> "%s kernel ACPI thermal control\n",
> - tz->tz_enabled ? "Enable" : "Disable"));
> + tz->mode == THERMAL_DEVICE_ENABLED ?
> + "Enable" : "Disable"));
> acpi_thermal_check(tz);
> }
> return 0;
> @@ -915,7 +912,7 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
> goto remove_dev_link;
> }
>
> - tz->tz_enabled = 1;
> + tz->mode = THERMAL_DEVICE_ENABLED;
>
> dev_info(&tz->device->dev, "registered as thermal_zone%d\n",
> tz->thermal_zone->id);
> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> index 8cc86f4e3ac1..830a8b060e74 100644
> --- a/drivers/platform/x86/acerhdf.c
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -68,6 +68,7 @@ static int kernelmode = 1;
> #else
> static int kernelmode;
> #endif
> +static enum thermal_device_mode thermal_mode;
>
> static unsigned int interval = 10;
> static unsigned int fanon = 60000;
> @@ -397,6 +398,7 @@ static inline void acerhdf_revert_to_bios_mode(void)
> {
> acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
> kernelmode = 0;
> + thermal_mode = THERMAL_DEVICE_DISABLED;
> if (thz_dev)
> thz_dev->polling_delay = 0;
> pr_notice("kernel mode fan control OFF\n");
> @@ -404,6 +406,7 @@ static inline void acerhdf_revert_to_bios_mode(void)
> static inline void acerhdf_enable_kernelmode(void)
> {
> kernelmode = 1;
> + thermal_mode = THERMAL_DEVICE_ENABLED;
>
> thz_dev->polling_delay = interval*1000;
> thermal_zone_device_update(thz_dev, THERMAL_EVENT_UNSPECIFIED);
> @@ -416,8 +419,7 @@ static int acerhdf_get_mode(struct thermal_zone_device *thermal,
> if (verbose)
> pr_notice("kernel mode fan control %d\n", kernelmode);
>
> - *mode = (kernelmode) ? THERMAL_DEVICE_ENABLED
> - : THERMAL_DEVICE_DISABLED;
> + *mode = thermal_mode;
>
> return 0;
> }
> @@ -739,6 +741,8 @@ static int __init acerhdf_register_thermal(void)
> if (IS_ERR(cl_dev))
> return -EINVAL;
>
> + thermal_mode = kernelmode ?
> + THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED;
> thz_dev = thermal_zone_device_register("acerhdf", 2, 0, NULL,
> &acerhdf_dev_ops,
> &acerhdf_zone_params, 0,
> diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> index 0b3a62655843..e84faaadff87 100644
> --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> @@ -48,7 +48,7 @@ struct int3400_thermal_priv {
> struct acpi_device *adev;
> struct platform_device *pdev;
> struct thermal_zone_device *thermal;
> - int mode;
> + enum thermal_device_mode mode;
> int art_count;
> struct art *arts;
> int trt_count;
> @@ -395,24 +395,20 @@ static int int3400_thermal_set_mode(struct thermal_zone_device *thermal,
> enum thermal_device_mode mode)
> {
> struct int3400_thermal_priv *priv = thermal->devdata;
> - bool enable;
> int result = 0;
>
> if (!priv)
> return -EINVAL;
>
> - if (mode == THERMAL_DEVICE_ENABLED)
> - enable = true;
> - else if (mode == THERMAL_DEVICE_DISABLED)
> - enable = false;
> - else
> + if (mode != THERMAL_DEVICE_ENABLED &&
> + mode != THERMAL_DEVICE_DISABLED)
> return -EINVAL;
>
> - if (enable != priv->mode) {
> - priv->mode = enable;
> + if (mode != priv->mode) {
> + priv->mode = mode;
> result = int3400_thermal_run_osc(priv->adev->handle,
> - priv->current_uuid_index,
> - enable);
> + priv->current_uuid_index,
> + mode == THERMAL_DEVICE_ENABLED);
> }
>
> evaluate_odvp(priv);
>
^ permalink raw reply [flat|nested] 9+ messages in thread