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

On Wed, Feb 19, 2025 at 1:20 AM Guenter Roeck [off-list ref] wrote:

On 2/18/25 00:35, James Calligeros wrote:

quoted

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

quoted

+             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
when dropping the die temp sysfs interface so that things are a little
more logical.

Regards,
James

[1] https://www.kernel.org/doc/Documentation/hwmon/sysfs-interface