| Message ID | 20250218-apple-codec-changes-v2-14-932760fd7e07@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: tas27{64,70}: improve support for Apple codec variants | expand |
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@gmail.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; > } > >
On Tue, Feb 18, 2025 at 06:35:48PM +1000, James Calligeros wrote: > Create and register a hwmon device to export the die temperature > to the hwmon interface Oh, so there is actualy a hwmon device added (which was why I thought you were ignoring my review comments on the last patch...). The question then becomes why also have the custom ABI for this as well? sysfs files are also supposed to be documented in Documention/ABI, though actal enforcement of that is a bit patchy.
On Wed, Feb 19, 2025 at 1:20 AM Guenter Roeck <linux@roeck-us.net> 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
On 2/21/25 03:31, James Calligeros wrote: > On Wed, Feb 19, 2025 at 1:20 AM Guenter Roeck <linux@roeck-us.net> 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
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) +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); + if (!ret) + *val *= 1000; + 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; }
Create and register a hwmon device to export the die temperature to the hwmon interface Signed-off-by: James Calligeros <jcalligeros99@gmail.com> --- sound/soc/codecs/tas2770.c | 69 +++++++++++++++++++++++++ 1 file changed, 69 insertions(+)