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

[Guenter Roeck <linux@xxxxxxxxxxxx>]

On 2/21/25 03:31, James Calligeros wrote:
> 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

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