Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Subject: Re: [PATCH v4 08/11] thermal: Explicitly enable non-changing
 thermal zone devices
Cc:     linux-pm@vger.kernel.org, linux-acpi@vger.kernel.org,
        netdev@vger.kernel.org, linux-wireless@vger.kernel.org,
        platform-driver-x86@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-renesas-soc@vger.kernel.org,
        linux-rockchip@lists.infradead.org,
        "Rafael J . Wysocki" <rjw@rjwysocki.net>,
        Len Brown <lenb@kernel.org>,
        Vishal Kulkarni <vishal@chelsio.com>,
        "David S . Miller" <davem@davemloft.net>,
        Jiri Pirko <jiri@mellanox.com>,
        Ido Schimmel <idosch@mellanox.com>,
        Johannes Berg <johannes.berg@intel.com>,
        Emmanuel Grumbach <emmanuel.grumbach@intel.com>,
        Luca Coelho <luciano.coelho@intel.com>,
        Intel Linux Wireless <linuxwifi@intel.com>,
        Kalle Valo <kvalo@codeaurora.org>,
        Peter Kaestle <peter@piie.net>,
        Darren Hart <dvhart@infradead.org>,
        Andy Shevchenko <andy@infradead.org>,
        Sebastian Reichel <sre@kernel.org>,
        Miquel Raynal <miquel.raynal@bootlin.com>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Amit Kucheria <amit.kucheria@verdurent.com>,
        Support Opensource <support.opensource@diasemi.com>,
        Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Fabio Estevam <festevam@gmail.com>,
        NXP Linux Team <linux-imx@nxp.com>,
        =?UTF-8?Q?Niklas_S=c3=b6derlund?= <niklas.soderlund@ragnatech.se>,
        Heiko Stuebner <heiko@sntech.de>,
        Orson Zhai <orsonzhai@gmail.com>,
        Baolin Wang <baolin.wang7@gmail.com>,
        Chunyan Zhang <zhang.lyra@gmail.com>,
        Zhang Rui <rui.zhang@intel.com>,
        Allison Randal <allison@lohutok.net>,
        Enrico Weigelt <info@metux.net>,
        Gayatri Kammela <gayatri.kammela@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>, kernel@collabora.com
To:     Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Message-ID: <0dc0796a-1559-d5e9-5297-c95281792a06@samsung.com>
Date:   Wed, 24 Jun 2020 12:00:59 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
        Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20200528192051.28034-9-andrzej.p@collabora.com>
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <Message-ID: <4493c0e4-51aa-3907-810c-74949ff27ca4@samsung.com>
        <20200528192051.28034-1-andrzej.p@collabora.com>
        <CGME20200528192214eucas1p183eafb73cad6465e71e779fb68679f03@eucas1p1.samsung.com>
        <20200528192051.28034-9-andrzej.p@collabora.com>
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk


On 5/28/20 9:20 PM, Andrzej Pietrasiewicz wrote:
> Some thermal zone devices never change their state, so they should be
> always enabled.
> 
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> ---
>  drivers/net/ethernet/chelsio/cxgb4/cxgb4_thermal.c     |  8 ++++++++
>  drivers/net/wireless/intel/iwlwifi/mvm/tt.c            |  9 ++++++++-
>  drivers/platform/x86/intel_mid_thermal.c               |  6 ++++++
>  drivers/power/supply/power_supply_core.c               |  9 +++++++--
>  drivers/thermal/armada_thermal.c                       |  6 ++++++
>  drivers/thermal/dove_thermal.c                         |  6 ++++++
>  .../thermal/intel/int340x_thermal/int3400_thermal.c    |  5 +++++
>  .../intel/int340x_thermal/int340x_thermal_zone.c       |  5 +++++
>  drivers/thermal/intel/intel_pch_thermal.c              |  5 +++++
>  drivers/thermal/intel/intel_soc_dts_iosf.c             |  3 +++
>  drivers/thermal/intel/x86_pkg_temp_thermal.c           |  6 ++++++
>  drivers/thermal/kirkwood_thermal.c                     |  7 +++++++
>  drivers/thermal/rcar_thermal.c                         |  9 ++++++++-
>  drivers/thermal/spear_thermal.c                        |  7 +++++++
>  drivers/thermal/st/st_thermal.c                        |  5 +++++
>  drivers/thermal/thermal_of.c                           | 10 +++++++++-
>  16 files changed, 101 insertions(+), 5 deletions(-)

[...]

> diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> index 3c0397a29b8c..8e8c9af7e5f4 100644
> --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> @@ -485,6 +485,10 @@ static int int3400_thermal_probe(struct platform_device *pdev)
>  		goto free_art_trt;
>  	}
>  
> +	result = thermal_zone_device_enable(priv->thermal);

I'm not sure about correctness of this addition.

This driver contains ->set_mode but doesn't call it on initialization
(in v3 it was using THERMAL_DEVICE_DISABLED as .initial_mode parameter).

> +	if (result)
> +		goto free_tzd;
> +
>  	priv->rel_misc_dev_res = acpi_thermal_rel_misc_device_add(
>  							priv->adev->handle);
>  
> @@ -518,6 +522,7 @@ static int int3400_thermal_probe(struct platform_device *pdev)
>  free_rel_misc:
>  	if (!priv->rel_misc_dev_res)
>  		acpi_thermal_rel_misc_device_remove(priv->adev->handle);
> +free_tzd:
>  	thermal_zone_device_unregister(priv->thermal);
>  free_art_trt:
>  	kfree(priv->trts);

[...]

> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index 43a516a35d64..011fd7f0a01e 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -1066,7 +1066,7 @@ int __init of_parse_thermal_zones(void)
>  	for_each_available_child_of_node(np, child) {
>  		struct thermal_zone_device *zone;
>  		struct thermal_zone_params *tzp;
> -		int i, mask = 0;
> +		int i, ret, mask = 0;
>  		u32 prop;
>  
>  		tz = thermal_of_build_thermal_zone(child);
> @@ -1113,6 +1113,14 @@ int __init of_parse_thermal_zones(void)
>  			of_thermal_free_zone(tz);
>  			/* attempting to build remaining zones still */
>  		}
> +		ret = thermal_zone_device_enable(zone);

This doesn't seem correct as it is done too early and
there is already proper thermal_zone_device_enable() call
in thermal_zone_of_sensor_register().

> +		if (ret) {
> +			thermal_zone_device_unregister(zone);
> +			pr_err("Failed to enable thermal zone\n");
> +			kfree(tzp);
> +			kfree(ops);
> +			of_thermal_free_zone(tz);
> +		}
>  	}
>  	of_node_put(np);
>  

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