Message-ID: <b1be4c35-b452-479c-9dcb-700ae276e8b7@roeck-us.net>
Date: Thu, 27 Feb 2025 04:45:35 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: James Calligeros <jcalligeros99@...il.com>,
 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>
Cc: 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 v3 12/20] ASoC: tas2770: expose die temp to hwmon

On 2/27/25 04:07, James Calligeros wrote:
> TAS2770 includes an ADC which reports the chip's die temperature.
> As per the datasheet, the temperature in degrees Celsius is derived
> ny taking the raw value stored in the ADC's registers, dividing by 16,
> then subtracting 93.
> 
> Create and register a hwmon device to expose the chip's die temperature
> to the hwmon interface.
> 
> The ADC is shut down during software shutdown mode, and its registers
> are initialised to 0 on reset. This means that the die temperature will
> read -93 *C until the chip has been fully powered up at least once (e.g.
> the PCM its attached to is opened). Once the chip is put into software
> shutdown again, the ADC will also shut down. The last value sampled
> before this happens will persist in the ADC's registers.
> 
> Co-developed-by: Martin Povišer <povik+lin@...ebit.org>
> Signed-off-by: Martin Povišer <povik+lin@...ebit.org>
> Signed-off-by: James Calligeros <jcalligeros99@...il.com>
> ---
>   sound/soc/codecs/tas2770.c | 102 +++++++++++++++++++++++++
>   1 file changed, 102 insertions(+)
> 
> diff --git a/sound/soc/codecs/tas2770.c b/sound/soc/codecs/tas2770.c
> index 7f219df8be7046912bf3ef452f75c17b5118bcf6..9cdd3470e9a04d5a5f801c3690d40f8fd131cd94 100644
> --- a/sound/soc/codecs/tas2770.c
> +++ b/sound/soc/codecs/tas2770.c
> @@ -12,6 +12,7 @@
>   #include <linux/err.h>
>   #include <linux/init.h>
>   #include <linux/delay.h>
> +#include <linux/hwmon.h>
>   #include <linux/pm.h>
>   #include <linux/i2c.h>
>   #include <linux/gpio/consumer.h>
> @@ -491,6 +492,94 @@ static struct snd_soc_dai_driver tas2770_dai_driver[] = {
>   	},
>   };
>   
> +static int tas2770_read_die_temp(struct tas2770_priv *tas2770, long *result)
> +{
> +	int ret, reading;
> +
> +	/*
> +	 * component could be uninitialised if the codec is not
> +	 * attached to a machine driver
> +	 */
> +	if (!tas2770->component)
> +		return -EINVAL;
> +

If this is temporary, it should return -ENODATA or better only instantiate after
a component is attached. If it is permanent, the device should not instantiate
in the first place.

> +	ret = snd_soc_component_read(tas2770->component, TAS2770_TEMP_MSB);
> +	if (ret < 0)
> +		return ret;
> +	reading = ret << 4;
> +
> +	ret = snd_soc_component_read(tas2770->component, TAS2770_TEMP_LSB);
> +	if (ret < 0)
> +		return ret;
> +	reading |= ret >> 4;
> +
> +	/*
> +	 * As per datasheet: divide register by 16 and subtract 93 to get
> +	 * degrees Celsius. hwmon requires millidegrees. Let's avoid rounding
> +	 * errors by subtracting 93 * 16 then multiplying by 1000 / 16.
> +	 *
> +	 * NOTE: The ADC registers are initialised to 0 on reset. This means
> +	 * that the temperature will read -93 *C until the chip is brought out
> +	 * of software shutdown (e.g. the PCM it's attached to is opened). The
> +	 * ADC is also shut down in software shutdown/low-power mode, so the
> +	 * value read back from its registers will be the last value sampled
> +	 * before entering software shutdown.
> +	 */
> +	*result = (reading - (93 * 16)) * (1000 / 16);
> +	return 0;
> +}
> +
> +static umode_t tas2770_hwmon_is_visible(const void *data,
> +					enum hwmon_sensor_types type, u32 attr,
> +					int channel)
> +{
> +	if (type != hwmon_temp)
> +		return 0;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		return 0444;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tas2770_hwmon_read(struct device *dev,
> +			      enum hwmon_sensor_types type,
> +			      u32 attr, int channel, long *val)
> +{
> +	struct tas2770_priv *tas2770 = dev_get_drvdata(dev);
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		ret = tas2770_read_die_temp(tas2770, val);
> +		break;
> +	default:
> +		ret = -EOPNOTSUPP;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct hwmon_channel_info *const tas2770_hwmon_info[] = {
> +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> +	NULL
> +};
> +
> +static const struct hwmon_ops tas2770_hwmon_ops = {
> +	.is_visible	= tas2770_hwmon_is_visible,
> +	.read		= tas2770_hwmon_read,
> +};
> +
> +static const struct hwmon_chip_info tas2770_hwmon_chip_info = {
> +	.ops	= &tas2770_hwmon_ops,
> +	.info	= tas2770_hwmon_info,
> +};
> +
>   static const struct regmap_config tas2770_i2c_regmap;
>   
>   static int tas2770_codec_probe(struct snd_soc_component *component)
> @@ -707,6 +796,19 @@ static int tas2770_i2c_probe(struct i2c_client *client)
>   		}
>   	}
>   
> +	if (IS_REACHABLE(CONFIG_HWMON)) {
> +		struct device *hwmon;
> +
> +		hwmon = devm_hwmon_device_register_with_info(&client->dev, "tas2770",
> +							tas2770,
> +							&tas2770_hwmon_chip_info,
> +							NULL);
> +		if (IS_ERR(hwmon)) {
> +			return dev_err_probe(&client->dev, PTR_ERR(hwmon),
> +					     "Failed to register temp sensor\n");
> +		}
> +	}
> +
>   	result = tas2770_register_codec(tas2770);
>   	if (result)
>   		dev_err(tas2770->dev, "Register codec failed.\n");
> 

Powered by blists - more mailing lists