mbox series

[v2,00/17] Self-encapsulate the thermal zone device structure

Message ID 20230221180710.2781027-1-daniel.lezcano@linaro.org
Headers show
Series Self-encapsulate the thermal zone device structure | expand

Message

Daniel Lezcano Feb. 21, 2023, 6:06 p.m. UTC
The exported thermal headers expose the thermal core structure while those
should be private to the framework. The initial idea was the thermal sensor
drivers use the thermal zone device structure pointer to pass it around from
the ops to the thermal framework API like a handler.

Unfortunately, different drivers are using and abusing the internals of this
structure to hook the associated struct device, read the internals values, take
the lock, etc ...

rn order to fix this situation, let's encapsulate the structure leaking the
more in the different drivers: the thermal_zone_device structure.

This series revisit the existing drivers using the thermal zone private
structure internals to change the access to something else. For instance, the
get_temp() ops is using the tz->dev to write a debug trace. Despite the trace
is not helpful, we can check the return value for the get_temp() ops in the
call site and show the message in this place.

With this set of changes, the thermal_zone_device is almost self-encapsulated.
As usual, the acpi driver needs a more complex changes, so that will come in a
separate series along with the structure moved the private core headers.

Changelog:
	- V2:
	   - Collected tags
	   - Add mising change for ->devdata for the tsens driver
	   - Renamed thermal_zone_device_get_data() to thermal_zone_priv()
	   - Added stubs when CONFIG_THERMAL is not set
	   - Dropped hwmon change where we remove the tz->lock usage

Thank you all for your comments


Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Len Brown <lenb@kernel.org>
Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: Jean Delvare <jdelvare@suse.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Chen-Yu Tsai <wens@csie.org>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Samuel Holland <samuel@sholland.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Ido Schimmel <idosch@nvidia.com>
Cc: Petr Machata <petrm@nvidia.com>
Cc: Gregory Greenman <gregory.greenman@intel.com>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: Sebastian Reichel <sre@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Amit Kucheria <amitk@kernel.org>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
Cc: Ray Jui <rjui@broadcom.com>
Cc: Scott Branden <sbranden@broadcom.com>
Cc: Markus Mayer <mmayer@broadcom.com>
Cc: Support Opensource <support.opensource@diasemi.com>
Cc: Andy Gross <agross@kernel.org>
Cc: Bjorn Andersson <andersson@kernel.org>
Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
Cc: Thara Gopinath <thara.gopinath@gmail.com>
Cc: "Niklas Söderlund" <niklas.soderlund@ragnatech.se>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Orson Zhai <orsonzhai@gmail.com>
Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: Chunyan Zhang <zhang.lyra@gmail.com>
Cc: Vasily Khoruzhick <anarsoul@gmail.com>
Cc: Yangtao Li <tiny.windzz@gmail.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Jonathan Hunter <jonathanh@nvidia.com>
Cc: Talel Shenhar <talel@amazon.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Cc: Keerthy <j-keerthy@ti.com>
Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Zheng Yongjun <zhengyongjun3@huawei.com>
Cc: Yang Li <yang.lee@linux.alibaba.com>
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Daniel Golle <daniel@makrotopia.org>
Cc: Balsam CHIHI <bchihi@baylibre.com>
Cc: Mikko Perttunen <mperttunen@nvidia.com>
Cc: linux-acpi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-ide@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-hwmon@vger.kernel.org
Cc: linux-iio@vger.kernel.org
Cc: linux-sunxi@lists.linux.dev
Cc: linux-input@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: linux-wireless@vger.kernel.org
Cc: linux-pm@vger.kernel.org
Cc: linux-rpi-kernel@lists.infradead.org
Cc: linux-arm-msm@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org
Cc: linux-rockchip@lists.infradead.org
Cc: linux-samsung-soc@vger.kernel.org
Cc: linux-tegra@vger.kernel.org
Cc: linux-omap@vger.kernel.org
Cc: linux-mediatek@lists.infradead.org

Daniel Lezcano (16):
  thermal/core: Add a thermal zone 'devdata' accessor
  thermal/core: Show a debug message when get_temp() fails
  thermal: Remove debug or error messages in get_temp() ops
  thermal/hwmon: Do not set no_hwmon before calling
    thermal_add_hwmon_sysfs()
  thermal/hwmon: Use the right device for devm_thermal_add_hwmon_sysfs()
  thermal: Don't use 'device' internal thermal zone structure field
  thermal/drivers/spear: Don't use tz->device but pdev->dev
  thermal: Add a thermal zone id accessor
  thermal: Do not access 'type' field, use the tz id instead
  thermal/drivers/da9062: Don't access the thermal zone device fields
  thermal/hwmon: Use the thermal_core.h header
  thermal/drivers/tegra: Remove unneeded lock when setting a trip point
  thermal/tegra: Do not enable the thermal zone, it is already enabled
  thermal/drivers/acerhdf: Make interval setting only at module load
    time
  thermal/drivers/acerhdf: Remove pointless governor test
  thermal/traces: Replace the thermal zone structure parameter with the
    field value

 drivers/acpi/thermal.c                        | 18 +++----
 drivers/ata/ahci_imx.c                        |  2 +-
 drivers/hwmon/hwmon.c                         |  4 +-
 drivers/hwmon/pmbus/pmbus_core.c              |  2 +-
 drivers/hwmon/scmi-hwmon.c                    |  4 +-
 drivers/hwmon/scpi-hwmon.c                    |  2 +-
 drivers/iio/adc/sun4i-gpadc-iio.c             |  2 +-
 drivers/input/touchscreen/sun4i-ts.c          |  2 +-
 .../ethernet/chelsio/cxgb4/cxgb4_thermal.c    |  2 +-
 .../ethernet/mellanox/mlxsw/core_thermal.c    | 18 +++----
 drivers/net/wireless/intel/iwlwifi/mvm/tt.c   |  4 +-
 drivers/platform/x86/acerhdf.c                | 19 ++------
 drivers/power/supply/power_supply_core.c      |  2 +-
 drivers/regulator/max8973-regulator.c         |  2 +-
 drivers/thermal/amlogic_thermal.c             |  2 +-
 drivers/thermal/armada_thermal.c              | 14 ++----
 drivers/thermal/broadcom/bcm2711_thermal.c    |  3 +-
 drivers/thermal/broadcom/bcm2835_thermal.c    |  3 +-
 drivers/thermal/broadcom/brcmstb_thermal.c    |  8 ++--
 drivers/thermal/broadcom/ns-thermal.c         |  2 +-
 drivers/thermal/broadcom/sr-thermal.c         |  2 +-
 drivers/thermal/da9062-thermal.c              | 13 +++--
 drivers/thermal/dove_thermal.c                |  7 +--
 drivers/thermal/gov_fair_share.c              |  2 +-
 drivers/thermal/gov_power_allocator.c         |  4 +-
 drivers/thermal/gov_step_wise.c               |  2 +-
 drivers/thermal/hisi_thermal.c                |  5 +-
 drivers/thermal/imx8mm_thermal.c              |  4 +-
 drivers/thermal/imx_sc_thermal.c              |  9 ++--
 drivers/thermal/imx_thermal.c                 | 47 +++++--------------
 drivers/thermal/intel/intel_pch_thermal.c     |  2 +-
 drivers/thermal/intel/intel_soc_dts_iosf.c    | 13 ++---
 drivers/thermal/intel/x86_pkg_temp_thermal.c  |  4 +-
 drivers/thermal/k3_bandgap.c                  |  4 +-
 drivers/thermal/k3_j72xx_bandgap.c            |  2 +-
 drivers/thermal/kirkwood_thermal.c            |  7 +--
 drivers/thermal/max77620_thermal.c            |  6 +--
 drivers/thermal/mediatek/auxadc_thermal.c     |  4 +-
 drivers/thermal/mediatek/lvts_thermal.c       |  9 ++--
 drivers/thermal/qcom/qcom-spmi-adc-tm5.c      |  6 +--
 drivers/thermal/qcom/qcom-spmi-temp-alarm.c   |  6 +--
 drivers/thermal/qcom/tsens.c                  |  2 +-
 drivers/thermal/qoriq_thermal.c               |  4 +-
 drivers/thermal/rcar_gen3_thermal.c           |  5 +-
 drivers/thermal/rcar_thermal.c                |  8 +---
 drivers/thermal/rockchip_thermal.c            |  8 +---
 drivers/thermal/rzg2l_thermal.c               |  3 +-
 drivers/thermal/samsung/exynos_tmu.c          |  4 +-
 drivers/thermal/spear_thermal.c               | 10 ++--
 drivers/thermal/sprd_thermal.c                |  2 +-
 drivers/thermal/st/st_thermal.c               |  2 -
 drivers/thermal/sun8i_thermal.c               |  4 +-
 drivers/thermal/tegra/tegra-bpmp-thermal.c    |  6 ++-
 drivers/thermal/tegra/tegra30-tsensor.c       | 31 ++++++------
 drivers/thermal/thermal-generic-adc.c         |  7 ++-
 drivers/thermal/thermal_core.c                | 17 ++++++-
 drivers/thermal/thermal_helpers.c             |  3 ++
 drivers/thermal/thermal_hwmon.c               |  9 ++--
 drivers/thermal/thermal_hwmon.h               |  4 +-
 drivers/thermal/thermal_mmio.c                |  2 +-
 .../ti-soc-thermal/ti-thermal-common.c        | 10 ++--
 drivers/thermal/uniphier_thermal.c            |  2 +-
 include/linux/thermal.h                       |  9 ++++
 include/trace/events/thermal.h                | 24 +++++-----
 .../trace/events/thermal_power_allocator.h    | 12 ++---
 65 files changed, 206 insertions(+), 255 deletions(-)

Comments

Jernej Škrabec Feb. 21, 2023, 6:20 p.m. UTC | #1
Dne torek, 21. februar 2023 ob 19:06:55 CET je Daniel Lezcano napisal(a):
> The thermal zone device structure is exposed to the different drivers
> and obviously they access the internals while that should be
> restricted to the core thermal code.
> 
> In order to self-encapsulate the thermal core code, we need to prevent
> the drivers accessing directly the thermal zone structure and provide
> accessor functions to deal with.
> 
> Provide an accessor to the 'devdata' structure and make use of it in
> the different drivers.
> 
> No functional changes intended.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Acked-by: Guenter Roeck <linux@roeck-us.net> #hwmon
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> #R-Car
> Acked-by: Mark Brown <broonie@kernel.org>
> Reviewed-by: Ido Schimmel <idosch@nvidia.com> #mlxsw
> Reviewed-by: AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> #MediaTek auxadc and lvts
> Reviewed-by: Balsam CHIHI <bchihi@baylibre.com> #Mediatek lvts
> Acked-by: Gregory Greenman <gregory.greenman@intel.com> #iwlwifi
> Reviewed-by: Adam Ward <DLG-Adam.Ward.opensource@dm.renesas.com> #da9062
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>  #spread
> Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com> #power_supply
> ---
>  drivers/acpi/thermal.c                           | 16 ++++++++--------
>  drivers/ata/ahci_imx.c                           |  2 +-
>  drivers/hwmon/hwmon.c                            |  4 ++--
>  drivers/hwmon/pmbus/pmbus_core.c                 |  2 +-
>  drivers/hwmon/scmi-hwmon.c                       |  2 +-
>  drivers/hwmon/scpi-hwmon.c                       |  2 +-
>  drivers/iio/adc/sun4i-gpadc-iio.c                |  2 +-
>  drivers/input/touchscreen/sun4i-ts.c             |  2 +-
>  .../net/ethernet/chelsio/cxgb4/cxgb4_thermal.c   |  2 +-
>  .../net/ethernet/mellanox/mlxsw/core_thermal.c   | 14 +++++++-------
>  drivers/net/wireless/intel/iwlwifi/mvm/tt.c      |  4 ++--
>  drivers/power/supply/power_supply_core.c         |  2 +-
>  drivers/regulator/max8973-regulator.c            |  2 +-
>  drivers/thermal/armada_thermal.c                 |  4 ++--
>  drivers/thermal/broadcom/bcm2711_thermal.c       |  2 +-
>  drivers/thermal/broadcom/bcm2835_thermal.c       |  2 +-
>  drivers/thermal/broadcom/brcmstb_thermal.c       |  4 ++--
>  drivers/thermal/broadcom/ns-thermal.c            |  2 +-
>  drivers/thermal/broadcom/sr-thermal.c            |  2 +-
>  drivers/thermal/da9062-thermal.c                 |  2 +-
>  drivers/thermal/dove_thermal.c                   |  2 +-
>  drivers/thermal/hisi_thermal.c                   |  2 +-
>  drivers/thermal/imx8mm_thermal.c                 |  2 +-
>  drivers/thermal/imx_sc_thermal.c                 |  2 +-
>  drivers/thermal/imx_thermal.c                    |  6 +++---
>  drivers/thermal/intel/intel_pch_thermal.c        |  2 +-
>  drivers/thermal/intel/intel_soc_dts_iosf.c       | 13 +++++--------
>  drivers/thermal/intel/x86_pkg_temp_thermal.c     |  4 ++--
>  drivers/thermal/k3_bandgap.c                     |  2 +-
>  drivers/thermal/k3_j72xx_bandgap.c               |  2 +-
>  drivers/thermal/kirkwood_thermal.c               |  2 +-
>  drivers/thermal/max77620_thermal.c               |  2 +-
>  drivers/thermal/mediatek/auxadc_thermal.c        |  2 +-
>  drivers/thermal/mediatek/lvts_thermal.c          |  4 ++--
>  drivers/thermal/qcom/qcom-spmi-adc-tm5.c         |  4 ++--
>  drivers/thermal/qcom/qcom-spmi-temp-alarm.c      |  4 ++--
>  drivers/thermal/qoriq_thermal.c                  |  2 +-
>  drivers/thermal/rcar_gen3_thermal.c              |  4 ++--
>  drivers/thermal/rcar_thermal.c                   |  3 +--
>  drivers/thermal/rockchip_thermal.c               |  4 ++--
>  drivers/thermal/rzg2l_thermal.c                  |  2 +-
>  drivers/thermal/samsung/exynos_tmu.c             |  4 ++--
>  drivers/thermal/spear_thermal.c                  |  8 ++++----
>  drivers/thermal/sprd_thermal.c                   |  2 +-
>  drivers/thermal/sun8i_thermal.c                  |  2 +-

For sun8i_thermal:
Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

>  drivers/thermal/tegra/tegra-bpmp-thermal.c       |  6 ++++--
>  drivers/thermal/tegra/tegra30-tsensor.c          |  4 ++--
>  drivers/thermal/thermal-generic-adc.c            |  2 +-
>  drivers/thermal/thermal_core.c                   |  6 ++++++
>  drivers/thermal/thermal_mmio.c                   |  2 +-
>  .../thermal/ti-soc-thermal/ti-thermal-common.c   |  4 ++--
>  drivers/thermal/uniphier_thermal.c               |  2 +-
>  include/linux/thermal.h                          |  7 +++++++
>  53 files changed, 102 insertions(+), 91 deletions(-)
AngeloGioacchino Del Regno Feb. 22, 2023, 9:31 a.m. UTC | #2
Il 21/02/23 19:07, Daniel Lezcano ha scritto:
> 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

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> 
#MediaTek LVTS
Florian Fainelli Feb. 22, 2023, 7:20 p.m. UTC | #3
On 2/21/23 10:06, Daniel Lezcano wrote:
> The thermal zone device structure is exposed to the different drivers
> and obviously they access the internals while that should be
> restricted to the core thermal code.
> 
> In order to self-encapsulate the thermal core code, we need to prevent
> the drivers accessing directly the thermal zone structure and provide
> accessor functions to deal with.
> 
> Provide an accessor to the 'devdata' structure and make use of it in
> the different drivers.
> 
> No functional changes intended.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Acked-by: Guenter Roeck <linux@roeck-us.net> #hwmon
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> #R-Car
> Acked-by: Mark Brown <broonie@kernel.org>
> Reviewed-by: Ido Schimmel <idosch@nvidia.com> #mlxsw
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> #MediaTek auxadc and lvts
> Reviewed-by: Balsam CHIHI <bchihi@baylibre.com> #Mediatek lvts
> Acked-by: Gregory Greenman <gregory.greenman@intel.com> #iwlwifi
> Reviewed-by: Adam Ward <DLG-Adam.Ward.opensource@dm.renesas.com> #da9062
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>  #spread
> Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com> #power_supply

Acked-by: Florian Fainelli <f.fainelli@gmail.com> #Broadcom
Rafael J. Wysocki Feb. 22, 2023, 8:06 p.m. UTC | #4
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?
Daniel Lezcano Feb. 23, 2023, 9:56 a.m. UTC | #5
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.
Rafael J. Wysocki Feb. 23, 2023, 11:43 a.m. UTC | #6
On Thu, Feb 23, 2023 at 10:56 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> 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.

I'm not sure what you mean here.

Surely, the 'type' is provided by whoever registers the thermal zone,
so I'm not sure in what way it "belongs" to the framework.

> 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

That's correct, but in the particular case of DT-based systems it
comes from the DT (AFAICT) and so it allows to connect the kernel
message with the DT contents.  The id could be used for that, but that
involves an extra sysfs lookup.

> 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.

So you are going to change the behavior of those few places with the
only reason being aesthetics AFAICS.  Is this really a good enough
reason to do that?
Daniel Lezcano Feb. 23, 2023, 2:35 p.m. UTC | #7
On 23/02/2023 12:43, Rafael J. Wysocki wrote:
> On Thu, Feb 23, 2023 at 10:56 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> 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.
> 
> I'm not sure what you mean here.
> 
> Surely, the 'type' is provided by whoever registers the thermal zone,
> so I'm not sure in what way it "belongs" to the framework.

I meant the goal of 'type' is to be exported to sysfs, nothing else.

That is the reason why I used the word 'belongs', because it was 
introduced to stay in the scope of the thermal framework, but then its 
usage has been diverted to a name.

Anyway, from my POV having traces in the ops is not a good thing, so 
I'll propose later to remove them and add a single message in the call 
sites.

Meanwhile, I'll provide the accessor for 'type' and hopefully we do not 
end up with a plethora of accessors to be used in the core code.