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

[James Calligeros]

On Saturday, 22 February 2025 1:03:38 am Australian Eastern Standard Time 
Guenter Roeck wrote:
> 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

My mistake. We return an intermediate value that is then manipulated in
die_temp_show() to yield degrees. I will clean this up for the next submission
since we will no longer require the sysfs interface at all. Apologies for the
confusion.

Regards,
James