linux-kernel - Re: [PATCH v2 14/29] ASoC: tas2770: expose die temp to hwmon

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <409e8873-5ebd-41af-8162-30668271be6a@roeck-us.net>
Date: Fri, 21 Feb 2025 07:03:38 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: James Calligeros <jcalligeros99@...il.com>
Cc: Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
 Shenghao Ding <shenghao-ding@...com>, Kevin Lu <kevin-lu@...com>,
 Baojun Xu <baojun.xu@...com>, Dan Murphy <dmurphy@...com>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Shi Fu <shifu0704@...ndersoft.com>,
 Jean Delvare <jdelvare@...e.com>, Alyssa Rosenzweig <alyssa@...enzweig.io>,
 Martin Povišer <povik+lin@...ebit.org>,
 Hector Martin <marcan@...can.st>, linux-sound@...r.kernel.org,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
 asahi@...ts.linux.dev, linux-hwmon@...r.kernel.org
Subject: Re: [PATCH v2 14/29] ASoC: tas2770: expose die temp to hwmon

On 2/21/25 03:31, James Calligeros wrote:
> On Wed, Feb 19, 2025 at 1:20 AM Guenter Roeck <linux@...ck-us.net> wrote:
>>
>> On 2/18/25 00:35, James Calligeros wrote:
>>> +static int tas2770_hwmon_read(struct device *dev,
>>> +                           enum hwmon_sensor_types type,
>>> +                           u32 attr, int channel, long *val)
>>> +{
>>> +     struct tas2770_priv *tas2770 = i2c_get_clientdata(to_i2c_client(dev));
>>> +     int ret;
>>> +
>>> +     switch (attr) {
>>> +     case hwmon_temp_input:
>>> +             ret = tas2770_read_die_temp(tas2770, (int *)val);
>>
>> Type casting a pointer like this is never a good idea. This only works
>> if sizeof(int) == sizeof(long).
> 
> I will rework this when dropping the die temp sysfs interface. This
> was mostly so that
> I didn't have to change any of the code there, but since we're going
> to drop that
> anyway it's redundant.
> 
>>> +             if (!ret)
>>> +                     *val *= 1000;
>>
>> The calculations in the previous patch suggest that this is wrong.
>>
>> Either case, this is redundant. The temperature is already displayed
>> as device specific sysfs attribute. Displaying it twice does not make sense.
>> I would suggest to either drop the sysfs attribute in the previous patch
>> or to drop this patch.
> 
> The calculation in the datasheet yields the temperature in degrees Celsius.
> hwmon consumers expect temperatures in "millidegrees" Celsius as per the
> sysfs interface documentation[1]. Regardless, as above I will likely rework this

Yes, I am well aware of that.

> when dropping the die temp sysfs interface so that things are a little
> more logical.
> 

Unless I really misread the code, tas2770_read_die_temp() doesn't return
the temperature in degrees C.

Guenter