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

[Daniel Lezcano]

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