On Wed, Feb 19, 2025 at 1:20 AM Guenter Roeck <linux@xxxxxxxxxxxx> 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 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