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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4e0e2ae0-c53d-444c-9d8a-d465be690232@roeck-us.net>
Date: Tue, 18 Feb 2025 07:20: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 v2 14/29] ASoC: tas2770: expose die temp to hwmon

On 2/18/25 00:35, James Calligeros wrote:
> Create and register a hwmon device to export the die temperature
> to the hwmon interface
> 
> Signed-off-by: James Calligeros <jcalligeros99@...il.com>
> ---
>   sound/soc/codecs/tas2770.c | 69 +++++++++++++++++++++++++
>   1 file changed, 69 insertions(+)
> 
> diff --git a/sound/soc/codecs/tas2770.c b/sound/soc/codecs/tas2770.c
> index 84066884d36be8d41d83bca5680e9f683c420d78..fee99db904a5885d740c1cfe8ce2645a963c6e1d 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>
> @@ -537,6 +538,61 @@ static struct attribute *tas2770_sysfs_attrs[] = {
>   };
>   ATTRIBUTE_GROUPS(tas2770_sysfs);
>   
> +#if defined(CONFIG_HWMON)

If this works with CONFIG_TAS2770=y and CONFIG_HWMON=m, the ifdef is unnececessary
because IS_REACHABLE() is used below and the code will be optimized away if
it is not reachable.

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

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

Guenter

> +		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,
> +};
> +#endif
> +
>   static const struct regmap_config tas2770_i2c_regmap;
>   
>   static int tas2770_codec_probe(struct snd_soc_component *component)
> @@ -768,6 +824,19 @@ static int tas2770_i2c_probe(struct i2c_client *client)
>   	if (result)
>   		dev_err(tas2770->dev, "Register codec failed.\n");
>   
> +	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");
> +		}
> +	}
> +
>   	return result;
>   }
>   
>