Re: [PATCH v2 06/16] thermal: Don't use 'device' internal thermal zone structure field

From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
	Balsam CHIHI <bchihi@baylibre.com>,
	Amit Kucheria <amitk@kernel.org>, Zhang Rui <rui.zhang@intel.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>,
	Eduardo Valentin <edubezval@gmail.com>,
	Keerthy <j-keerthy@ti.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	AngeloGioacchino Del Regno 
	<angelogioacchino.delregno@collabora.com>,
	Ido Schimmel <idosch@nvidia.com>, Mark Brown <broonie@kernel.org>,
	"moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
	<linux-arm-kernel@lists.infradead.org>,
	"open list:TI BANDGAP AND THERMAL DRIVER" 
	<linux-omap@vger.kernel.org>,
	"moderated list:ARM/Mediatek SoC support" 
	<linux-mediatek@lists.infradead.org>
Subject: Re: [PATCH v2 06/16] thermal: Don't use 'device' internal thermal zone structure field
Date: Thu, 23 Feb 2023 10:56:02 +0100	[thread overview]
Message-ID: <2b843912-cc9b-f1ac-1f81-28e3ecf8daba@linaro.org> (raw)
In-Reply-To: <CAJZ5v0h=UCOyJ88UgEdqch4NZK+DaT8jWnDaguQGNx-KOMhPPA@mail.gmail.com>

On 22/02/2023 21:06, Rafael J. Wysocki wrote:
> On Wed, Feb 22, 2023 at 9:00 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 22/02/2023 20:43, Rafael J. Wysocki wrote:
>>> On Tue, Feb 21, 2023 at 7:07 PM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> Some drivers are directly using the thermal zone's 'device' structure
>>>> field.
>>>>
>>>> Use the driver device pointer instead of the thermal zone device when
>>>> it is available.
>>>>
>>>> Remove the traces when they are duplicate with the traces in the core
>>>> code.
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>> Reviewed-by: Balsam CHIHI <bchihi@baylibre.com> #Mediatek LVTS
>>>> ---
>>
>> [ ... ]
>>
>>>>           thermal_zone_device_update(data->ti_thermal, THERMAL_EVENT_UNSPECIFIED);
>>>>
>>>> -       dev_dbg(&data->ti_thermal->device, "updated thermal zone %s\n",
>>>> +       dev_dbg(data->bgp->dev, "updated thermal zone %s\n",
>>>>                   data->ti_thermal->type);
>>>
>>> The code before the change is more consistent, because it refers to
>>> the same object in both instances.
>>>
>>> It looks like a type field accessor is needed, eg. thermal_zone_device_type()?
>>>
>>> Or move the debug message to thermal_zone_device_update()?
>>
>> Actually it is done on purpose because the patch 9 replaces the accesses
>> to 'type' by 'id', the thermal_zone_device_type() accessor won't be needed.
> 
> Cool.
> 
> However, this is a change in behavior (albeit small) which doesn't
> appear to be necessary.
> 
> What would be wrong with having a tz->type accessor too?

I can add the 'type' accessor but from my point of view it is not 
correct because the information belongs to the thermal framework and it 
is used to export the information in the sysfs which is along with the 
directory name giving the id of the thermal zone.

Actually, the useful information is the id of the thermal zone, not the 
type. This one can be duplicate, for instance:

cat /sys/class/thermal/thermal_zone*/type
acpitz
acpitz

Given there are few places where 'type' is used in the drivers, I prefer 
to directly change that to 'id' in the next patch instead of creating 
the accessor for 'type', then send another series removing it.

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog