Re: [PATCH RFC] thermal/drivers/intel_menlow: Remove add_one_attribute

From: Daniel Lezcano
Date: Tue Feb 21 2023 - 08:31:38 EST


On 21/02/2023 13:58, Zhang, Rui wrote:
Hi, Daniel,

On Tue, 2023-02-21 at 12:30 +0100, Daniel Lezcano wrote:
Hi Rui,


On 21/02/2023 07:22, Zhang, Rui wrote:
On Mon, 2023-02-20 at 17:24 +0100, Daniel Lezcano wrote:
The driver hooks the thermal framework sysfs to add some driver
specific information. A debatable approach as that may belong the
device sysfs directory, not the thermal zone directory.

As the driver is accessing the thermal internals, we should
provide
at
least an API to the thermal framework to add an attribute to the
existing sysfs thermal zone entry.

Before doing that and given the age of the driver (2008) may be
it is
worth to double check if these attributes are really needed. So
my
first proposal is to remove them if that does not hurt.

Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>

I don't have any device that uses this driver.
Let's see what Sujith says.

Thanks for your answer.

I take the opportunity to ask you for the ACPI thermal additional
sysfs
entries.

The ACPI thermal driver adds a link:

/sys/class/thermal/thermal_zone0/device

which points to:

../../../LNXSYSTM:00/LNXSYBUS:01/LNXTHERM:00


And in this directory there is:

/sys/devices/LNXSYSTM:00/LNXSYBUS:01/LNXTHERM:00/thermal_zone

pointing to /sys/class/thermal/thermal_zone0


I was wondering if we have to keep it also? It is a cyclic
description
and we can have the several thermal zones having a device link
pointing
to the same location.

I don't think so. So far, ACPI Thermal object and the generic thermal
zone device are 1:1 match.

So I'm not sure this is correct.

I can understand adding a link in the thermal zone pointing to the
device could make sense, and that could be generalized to all the
thermal zone creation, but the back pointer link seems strange.

Would it make sense to remove this second link ?

That was required by some userpsace tool running on menlow, similar to
this one. But TBH, I don't recall the userspace details.

If there is a 1:1 relationship, how can the userspace require the kernel to describe an information it already has?

eg.

/sys/class/thermal/thermal_zone0/device points to ../../../LNXSYSTM:00/LNXSYBUS:01/LNXTHERM:00

So, userspace has already the opposite relationship as the 1:1 tells thermal_zone0 -> LNXTHERM:00, then LNXTHERM:00 is associated to thermal_zone0. It is a duplicate information in sysfs.

Anyway, I guess that now it is in sysfs, the removal will be very hard to achieve *sigh*

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