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

From: Guenter Roeck
Date: Tue Jul 08 2025 - 12:21:34 EST

On Tue, Jul 08, 2025 at 06:13:04PM +0200, Dimitri Fedrau via B4 Relay wrote:
> From: Dimitri Fedrau <dimitri.fedrau@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("PWM_MC33XS2410");
>
> --
> 2.39.5
>
>
>