linux-kernel - Re: [PATCH v4 2/2] hwmon: add support for MC33XS2410 hardware monitoring

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] [day] [month] [year] [list]
Message-ID: <20250711054541.GA3276@legfed1>
Date: Fri, 11 Jul 2025 07:45:41 +0200
From: Dimitri Fedrau <dima.fedrau@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: dimitri.fedrau@...bherr.com,
	Uwe Kleine-König <ukleinek@...nel.org>,
	Jean Delvare <jdelvare@...e.com>, linux-pwm@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org
Subject: Re: [PATCH v4 2/2] hwmon: add support for MC33XS2410 hardware
 monitoring

Hi Guenter,

thanks for the quicke review, will add the documentation in v5.

Best regards,
Dimitri Fedrau

Am Tue, Jul 08, 2025 at 09:21:19AM -0700 schrieb Guenter Roeck:
> On Tue, Jul 08, 2025 at 06:13:04PM +0200, Dimitri Fedrau via B4 Relay wrote:
> > From: Dimitri Fedrau <dimitri.fedrau@...bherr.com>
> > 
> > The device is able to monitor temperature, voltage and current of each of
> > the four outputs. Add basic support for monitoring the temperature of the
> > four outputs and the die temperature.
> > 
> > Signed-off-by: Dimitri Fedrau <dimitri.fedrau@...bherr.com>
> > ---
> >  drivers/hwmon/Kconfig            |  10 +++
> >  drivers/hwmon/Makefile           |   1 +
> >  drivers/hwmon/mc33xs2410_hwmon.c | 179 +++++++++++++++++++++++++++++++++++++++
> 
> Code looks good, except the documentation is missing.
> Please add to Documentation/hwmon/.
> 
> Thanks,
> Guenter
> 
> >  3 files changed, 190 insertions(+)
> > 
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 079620dd42862ef5e026697e9e1b1fcd5b8be298..9d28fcf7cd2a6f9e2f54694a717bd85ff4047b46 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -700,6 +700,16 @@ config SENSORS_MC13783_ADC
> >          help
> >            Support for the A/D converter on MC13783 and MC13892 PMIC.
> >  
> > +config SENSORS_MC33XS2410
> > +	tristate "MC33XS2410 HWMON support"
> > +	depends on PWM_MC33XS2410
> > +	help
> > +	  If you say yes here you get hardware monitoring support for
> > +	  MC33XS2410.
> > +
> > +	  This driver can also be built as a module. If so, the module
> > +	  will be called mc33xs2410_hwmon.
> > +
> >  config SENSORS_FSCHMD
> >  	tristate "Fujitsu Siemens Computers sensor chips"
> >  	depends on (X86 || COMPILE_TEST) && I2C
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 48e5866c0c9a7677089d1001a9c5ae4adebff5d5..cd8bc4752b4dbf015c6eb46157626f4e8f87dfae 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -165,6 +165,7 @@ obj-$(CONFIG_SENSORS_MAX31790)	+= max31790.o
> >  obj-$(CONFIG_MAX31827) += max31827.o
> >  obj-$(CONFIG_SENSORS_MAX77705) += max77705-hwmon.o
> >  obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
> > +obj-$(CONFIG_SENSORS_MC33XS2410) += mc33xs2410_hwmon.o
> >  obj-$(CONFIG_SENSORS_MC34VR500)	+= mc34vr500.o
> >  obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
> >  obj-$(CONFIG_SENSORS_TC654)	+= tc654.o
> > diff --git a/drivers/hwmon/mc33xs2410_hwmon.c b/drivers/hwmon/mc33xs2410_hwmon.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..71a14932c6ccaf8d15b045b55093238d924160ea
> > --- /dev/null
> > +++ b/drivers/hwmon/mc33xs2410_hwmon.c
> > @@ -0,0 +1,179 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2025 Liebherr-Electronics and Drives GmbH
> > + */
> > +
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/mc33xs2410.h>
> > +#include <linux/module.h>
> > +
> > +/* ctrl registers */
> > +
> > +#define MC33XS2410_TEMP_WT			0x29
> > +#define MC33XS2410_TEMP_WT_MASK			GENMASK(7, 0)
> > +
> > +/* diag registers */
> > +
> > +/* chan in { 1 ... 4 } */
> > +#define MC33XS2410_OUT_STA(chan)		(0x02 + (chan) - 1)
> > +#define MC33XS2410_OUT_STA_OTW			BIT(8)
> > +
> > +#define MC33XS2410_TS_TEMP_DIE			0x26
> > +#define MC33XS2410_TS_TEMP_MASK			GENMASK(9, 0)
> > +
> > +/* chan in { 1 ... 4 } */
> > +#define MC33XS2410_TS_TEMP(chan)		(0x2f + (chan) - 1)
> > +
> > +static const struct hwmon_channel_info * const mc33xs2410_hwmon_info[] = {
> > +	HWMON_CHANNEL_INFO(temp,
> > +			   HWMON_T_LABEL | HWMON_T_INPUT,
> > +			   HWMON_T_LABEL | HWMON_T_INPUT | HWMON_T_MAX |
> > +			   HWMON_T_ALARM,
> > +			   HWMON_T_LABEL | HWMON_T_INPUT | HWMON_T_MAX |
> > +			   HWMON_T_ALARM,
> > +			   HWMON_T_LABEL | HWMON_T_INPUT | HWMON_T_MAX |
> > +			   HWMON_T_ALARM,
> > +			   HWMON_T_LABEL | HWMON_T_INPUT | HWMON_T_MAX |
> > +			   HWMON_T_ALARM),
> > +	NULL,
> > +};
> > +
> > +static umode_t mc33xs2410_hwmon_is_visible(const void *data,
> > +					   enum hwmon_sensor_types type,
> > +					   u32 attr, int channel)
> > +{
> > +	switch (attr) {
> > +	case hwmon_temp_input:
> > +	case hwmon_temp_alarm:
> > +	case hwmon_temp_label:
> > +		return 0444;
> > +	case hwmon_temp_max:
> > +		return 0644;
> > +	default:
> > +		return 0;
> > +	}
> > +}
> > +
> > +static int mc33xs2410_hwmon_read(struct device *dev,
> > +				 enum hwmon_sensor_types type,
> > +				 u32 attr, int channel, long *val)
> > +{
> > +	struct spi_device *spi = dev_get_drvdata(dev);
> > +	u16 reg_val;
> > +	int ret;
> > +	u8 reg;
> > +
> > +	switch (attr) {
> > +	case hwmon_temp_input:
> > +		reg = (channel == 0) ? MC33XS2410_TS_TEMP_DIE :
> > +				       MC33XS2410_TS_TEMP(channel);
> > +		ret = mc33xs2410_read_reg_diag(spi, reg, &reg_val);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		/* LSB is 0.25 degree celsius */
> > +		*val = FIELD_GET(MC33XS2410_TS_TEMP_MASK, reg_val) * 250 - 40000;
> > +		return 0;
> > +	case hwmon_temp_alarm:
> > +		ret = mc33xs2410_read_reg_diag(spi, MC33XS2410_OUT_STA(channel),
> > +					       &reg_val);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*val = FIELD_GET(MC33XS2410_OUT_STA_OTW, reg_val);
> > +		return 0;
> > +	case hwmon_temp_max:
> > +		ret = mc33xs2410_read_reg_ctrl(spi, MC33XS2410_TEMP_WT, &reg_val);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		/* LSB is 1 degree celsius */
> > +		*val = FIELD_GET(MC33XS2410_TEMP_WT_MASK, reg_val) * 1000 - 40000;
> > +		return 0;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +}
> > +
> > +static int mc33xs2410_hwmon_write(struct device *dev,
> > +				  enum hwmon_sensor_types type, u32 attr,
> > +				  int channel, long val)
> > +{
> > +	struct spi_device *spi = dev_get_drvdata(dev);
> > +
> > +	switch (attr) {
> > +	case hwmon_temp_max:
> > +		val = clamp_val(val, -40000, 215000);
> > +
> > +		/* LSB is 1 degree celsius */
> > +		val = (val / 1000) + 40;
> > +		return mc33xs2410_modify_reg(spi, MC33XS2410_TEMP_WT,
> > +					     MC33XS2410_TEMP_WT_MASK, val);
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +}
> > +
> > +static const char *const mc33xs2410_temp_label[] = {
> > +	"Central die temperature",
> > +	"Channel 1 temperature",
> > +	"Channel 2 temperature",
> > +	"Channel 3 temperature",
> > +	"Channel 4 temperature",
> > +};
> > +
> > +static int mc33xs2410_read_string(struct device *dev,
> > +				  enum hwmon_sensor_types type,
> > +				  u32 attr, int channel, const char **str)
> > +{
> > +	*str = mc33xs2410_temp_label[channel];
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct hwmon_ops mc33xs2410_hwmon_hwmon_ops = {
> > +	.is_visible = mc33xs2410_hwmon_is_visible,
> > +	.read = mc33xs2410_hwmon_read,
> > +	.read_string = mc33xs2410_read_string,
> > +	.write = mc33xs2410_hwmon_write,
> > +};
> > +
> > +static const struct hwmon_chip_info mc33xs2410_hwmon_chip_info = {
> > +	.ops = &mc33xs2410_hwmon_hwmon_ops,
> > +	.info = mc33xs2410_hwmon_info,
> > +};
> > +
> > +static int mc33xs2410_hwmon_probe(struct auxiliary_device *adev,
> > +				  const struct auxiliary_device_id *id)
> > +{
> > +	struct device *dev = &adev->dev;
> > +	struct spi_device *spi = container_of(dev->parent, struct spi_device, dev);
> > +	struct device *hwmon;
> > +
> > +	hwmon = devm_hwmon_device_register_with_info(dev, NULL, spi,
> > +						     &mc33xs2410_hwmon_chip_info,
> > +						     NULL);
> > +	return PTR_ERR_OR_ZERO(hwmon);
> > +}
> > +
> > +static const struct auxiliary_device_id mc33xs2410_hwmon_ids[] = {
> > +	{
> > +		.name = "pwm_mc33xs2410.hwmon",
> > +	},
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(auxiliary, mc33xs2410_hwmon_ids);
> > +
> > +static struct auxiliary_driver mc33xs2410_hwmon_driver = {
> > +	.probe = mc33xs2410_hwmon_probe,
> > +	.id_table = mc33xs2410_hwmon_ids,
> > +};
> > +module_auxiliary_driver(mc33xs2410_hwmon_driver);
> > +
> > +MODULE_DESCRIPTION("NXP MC33XS2410 hwmon driver");
> > +MODULE_AUTHOR("Dimitri Fedrau <dimitri.fedrau@...bherr.com>");
> > +MODULE_LICENSE("GPL");
> > +MODULE_IMPORT_NS("PWM_MC33XS2410");
> > 
> > -- 
> > 2.39.5
> > 
> > 
> >