Message-ID: <9932fe61-9a33-79f4-8680-8f504357d514@linaro.org>
Date:   Tue, 21 Feb 2023 14:31:30 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH RFC] thermal/drivers/intel_menlow: Remove
 add_one_attribute
Content-Language: en-US
To:     "Zhang, Rui" <rui.zhang@intel.com>,
        "rafael@kernel.org" <rafael@kernel.org>
Cc:     "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Thomas, Sujith" <sujith.thomas@intel.com>,
        "amitk@kernel.org" <amitk@kernel.org>
References: <20230220162419.1276952-1-daniel.lezcano@linaro.org>
 <27488f102c917ce1f6d24d30f801b1e1745674e1.camel@intel.com>
 <b66fca52-d1f9-2761-4117-819c5820308d@linaro.org>
 <6b63db0832fadffa56ff85a1c4ca2b98d165df50.camel@intel.com>
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
In-Reply-To: <6b63db0832fadffa56ff85a1c4ca2b98d165df50.camel@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@linaro.org>
>>>
>>> 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