[PATCH v3] pwm: mc33xs2410: add support for temperature sensors

[PATCH v3] pwm: mc33xs2410: add support for temperature sensors

[Dimitri Fedrau via B4 Relay]

From: Dimitri Fedrau <dimitri.fedrau@liebherr.com>

The MC33XS2410 provides temperature sensors for the central die temperature
and the four outputs. Additionally a common temperature warning threshold
can be configured for the outputs. Add hwmon support for the sensors.

Acked-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
---
Changes in v3:
- Add changes suggested by Uwe Kleine-König.
  Remove "#if IS_ENABLED(CONFIG_HWMON)" and add
  "if (IS_REACHABLE(CONFIG_HWMON))" in mc33xs2410_hwmon_probe.
- Link to v2: https://lore.kernel.org/r/20250515-mc33xs2410-hwmon-v2-1-8d2e78f7e30d@liebherr.com

Changes in v2:
- Remove helper mc33xs2410_hwmon_read_out_status and report the last
  latched status.
- Link to v1: https://lore.kernel.org/r/20250512-mc33xs2410-hwmon-v1-1-addba77c78f9@liebherr.com
---
 drivers/pwm/pwm-mc33xs2410.c | 164 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 164 insertions(+)

diff --git a/drivers/pwm/pwm-mc33xs2410.c b/drivers/pwm/pwm-mc33xs2410.c
index a1ac3445ccdb4709d92e0075d424a8abc1416eee..6a0a1e50d9584dbca751748143669ee1ebefb7e3 100644
--- a/drivers/pwm/pwm-mc33xs2410.c
+++ b/drivers/pwm/pwm-mc33xs2410.c
@@ -21,6 +21,7 @@
 #include <linux/bitfield.h>
 #include <linux/delay.h>
 #include <linux/err.h>
+#include <linux/hwmon.h>
 #include <linux/math64.h>
 #include <linux/minmax.h>
 #include <linux/module.h>
@@ -29,6 +30,8 @@
 
 #include <linux/spi/spi.h>
 
+/* ctrl registers */
+
 #define MC33XS2410_GLB_CTRL			0x00
 #define MC33XS2410_GLB_CTRL_MODE		GENMASK(7, 6)
 #define MC33XS2410_GLB_CTRL_MODE_NORMAL		FIELD_PREP(MC33XS2410_GLB_CTRL_MODE, 1)
@@ -51,6 +54,21 @@
 
 #define MC33XS2410_WDT				0x14
 
+#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)
+
 #define MC33XS2410_PWM_MIN_PERIOD		488282
 /* step in { 0 ... 3 } */
 #define MC33XS2410_PWM_MAX_PERIOD(step)		(2000000000 >> (2 * (step)))
@@ -125,6 +143,11 @@ static int mc33xs2410_read_reg_ctrl(struct spi_device *spi, u8 reg, u16 *val)
 	return mc33xs2410_read_reg(spi, reg, val, MC33XS2410_FRAME_IN_DATA_RD);
 }
 
+static int mc33xs2410_read_reg_diag(struct spi_device *spi, u8 reg, u16 *val)
+{
+	return mc33xs2410_read_reg(spi, reg, val, 0);
+}
+
 static int mc33xs2410_modify_reg(struct spi_device *spi, u8 reg, u8 mask, u8 val)
 {
 	u16 tmp;
@@ -140,6 +163,143 @@ static int mc33xs2410_modify_reg(struct spi_device *spi, u8 reg, u8 mask, u8 val
 	return mc33xs2410_write_reg(spi, reg, tmp);
 }
 
+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 spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+
+	if (IS_REACHABLE(CONFIG_HWMON)) {
+		struct device *hwmon;
+
+		hwmon = devm_hwmon_device_register_with_info(dev, NULL, spi,
+							     &mc33xs2410_hwmon_chip_info,
+							     NULL);
+		return PTR_ERR_OR_ZERO(hwmon);
+	}
+
+	dev_dbg(dev, "Not registering hwmon sensors\n");
+
+	return 0;
+}
+
 static u8 mc33xs2410_pwm_get_freq(u64 period)
 {
 	u8 step, count;
@@ -361,6 +521,10 @@ static int mc33xs2410_probe(struct spi_device *spi)
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to add pwm chip\n");
 
+	ret = mc33xs2410_hwmon_probe(spi);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to register hwmon sensors\n");
+
 	return 0;
 }
 

---
base-commit: 446d36aa71b56e8a926e6ff066d83b12130fda59
change-id: 20250507-mc33xs2410-hwmon-a5ff9efec005

Best regards,
-- 
Dimitri Fedrau <dimitri.fedrau@liebherr.com>

Re: [PATCH v3] pwm: mc33xs2410: add support for temperature sensors

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]

hello Dimitri,

On Thu, Jun 19, 2025 at 07:32:42PM +0200, Dimitri Fedrau via B4 Relay wrote:
> @@ -29,6 +30,8 @@
>  
>  #include <linux/spi/spi.h>
>  
> +/* ctrl registers */
> +
>  #define MC33XS2410_GLB_CTRL			0x00
>  #define MC33XS2410_GLB_CTRL_MODE		GENMASK(7, 6)
>  #define MC33XS2410_GLB_CTRL_MODE_NORMAL		FIELD_PREP(MC33XS2410_GLB_CTRL_MODE, 1)
> @@ -51,6 +54,21 @@
>  
>  #define MC33XS2410_WDT				0x14
>  
> +#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)

Keep the registers in address order please

> +/* chan in { 1 ... 4 } */
> +#define MC33XS2410_TS_TEMP(chan)		(0x2f + (chan) - 1)

I wonder if it would be cleaner if this was abstracted using mfd. Then
the hwmon driver could live in drivers/hwmon

Liebe Grüße
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3] pwm: mc33xs2410: add support for temperature sensors

[Guenter Roeck]

On Mon, Jun 23, 2025 at 11:07:24AM +0200, Uwe Kleine-König wrote:
> hello Dimitri,
> 
> On Thu, Jun 19, 2025 at 07:32:42PM +0200, Dimitri Fedrau via B4 Relay wrote:
> > @@ -29,6 +30,8 @@
> >  
> >  #include <linux/spi/spi.h>
> >  
> > +/* ctrl registers */
> > +
> >  #define MC33XS2410_GLB_CTRL			0x00
> >  #define MC33XS2410_GLB_CTRL_MODE		GENMASK(7, 6)
> >  #define MC33XS2410_GLB_CTRL_MODE_NORMAL		FIELD_PREP(MC33XS2410_GLB_CTRL_MODE, 1)
> > @@ -51,6 +54,21 @@
> >  
> >  #define MC33XS2410_WDT				0x14
> >  
> > +#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)
> 
> Keep the registers in address order please
> 
> > +/* chan in { 1 ... 4 } */
> > +#define MC33XS2410_TS_TEMP(chan)		(0x2f + (chan) - 1)
> 
> I wonder if it would be cleaner if this was abstracted using mfd. Then
> the hwmon driver could live in drivers/hwmon
> 

For some reason the recent tendency is to move code out of hwmon, not into
it. Maybe my review feedback is too strong or something. Either case,
consider using auxiliary devices if you really want to move the code. That
seems to be a better fit, and it would be more lightweight since it doesn't
require an interconnect driver.

Guenter

Re: [PATCH v3] pwm: mc33xs2410: add support for temperature sensors

[Dimitri Fedrau]

Hi Uwe,

Am Mon, Jun 23, 2025 at 11:07:24AM +0200 schrieb Uwe Kleine-König:
> hello Dimitri,
> 
> On Thu, Jun 19, 2025 at 07:32:42PM +0200, Dimitri Fedrau via B4 Relay wrote:
> > @@ -29,6 +30,8 @@
> >  
> >  #include <linux/spi/spi.h>
> >  
> > +/* ctrl registers */
> > +
> >  #define MC33XS2410_GLB_CTRL			0x00
> >  #define MC33XS2410_GLB_CTRL_MODE		GENMASK(7, 6)
> >  #define MC33XS2410_GLB_CTRL_MODE_NORMAL		FIELD_PREP(MC33XS2410_GLB_CTRL_MODE, 1)
> > @@ -51,6 +54,21 @@
> >  
> >  #define MC33XS2410_WDT				0x14
> >  
> > +#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)
> 
> Keep the registers in address order please
> 
Aren't they ? There are "ctrl" and "diag" registers and I didn't mix up
adresses. There is a comment for each of them when defines start.

> > +/* chan in { 1 ... 4 } */
> > +#define MC33XS2410_TS_TEMP(chan)		(0x2f + (chan) - 1)
> 
> I wonder if it would be cleaner if this was abstracted using mfd. Then
> the hwmon driver could live in drivers/hwmon
> 
I will have a look into auxiliary devices as Guenter proposed in a
following mail.

> Liebe Grüße
> Uwe

Viele Grüße
Dimitri

Re: [PATCH v3] pwm: mc33xs2410: add support for temperature sensors

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1753 bytes --]

On Tue, Jun 24, 2025 at 09:04:42AM +0200, Dimitri Fedrau wrote:
> Hi Uwe,
> 
> Am Mon, Jun 23, 2025 at 11:07:24AM +0200 schrieb Uwe Kleine-König:
> > hello Dimitri,
> > 
> > On Thu, Jun 19, 2025 at 07:32:42PM +0200, Dimitri Fedrau via B4 Relay wrote:
> > > @@ -29,6 +30,8 @@
> > >  
> > >  #include <linux/spi/spi.h>
> > >  
> > > +/* ctrl registers */
> > > +
> > >  #define MC33XS2410_GLB_CTRL			0x00
> > >  #define MC33XS2410_GLB_CTRL_MODE		GENMASK(7, 6)
> > >  #define MC33XS2410_GLB_CTRL_MODE_NORMAL		FIELD_PREP(MC33XS2410_GLB_CTRL_MODE, 1)
> > > @@ -51,6 +54,21 @@
> > >  
> > >  #define MC33XS2410_WDT				0x14
> > >  
> > > +#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)
> > 
> > Keep the registers in address order please
>
> Aren't they ? There are "ctrl" and "diag" registers and I didn't mix up
> adresses. There is a comment for each of them when defines start.

+#define MC33XS2410_TEMP_WT                     0x29
...
+#define MC33XS2410_TS_TEMP_DIE                 0x26

> > > +/* chan in { 1 ... 4 } */
> > > +#define MC33XS2410_TS_TEMP(chan)		(0x2f + (chan) - 1)
> > 
> > I wonder if it would be cleaner if this was abstracted using mfd. Then
> > the hwmon driver could live in drivers/hwmon
> > 
> I will have a look into auxiliary devices as Guenter proposed in a
> following mail.

FTR: auxiliary device is also fine for me.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3] pwm: mc33xs2410: add support for temperature sensors

[Dimitri Fedrau]

Am Tue, Jun 24, 2025 at 09:29:25AM +0200 schrieb Uwe Kleine-König:
> On Tue, Jun 24, 2025 at 09:04:42AM +0200, Dimitri Fedrau wrote:
> > Hi Uwe,
> > 
> > Am Mon, Jun 23, 2025 at 11:07:24AM +0200 schrieb Uwe Kleine-König:
> > > hello Dimitri,
> > > 
> > > On Thu, Jun 19, 2025 at 07:32:42PM +0200, Dimitri Fedrau via B4 Relay wrote:
> > > > @@ -29,6 +30,8 @@
> > > >  
> > > >  #include <linux/spi/spi.h>
> > > >  
> > > > +/* ctrl registers */
> > > > +
> > > >  #define MC33XS2410_GLB_CTRL			0x00
> > > >  #define MC33XS2410_GLB_CTRL_MODE		GENMASK(7, 6)
> > > >  #define MC33XS2410_GLB_CTRL_MODE_NORMAL		FIELD_PREP(MC33XS2410_GLB_CTRL_MODE, 1)
> > > > @@ -51,6 +54,21 @@
> > > >  
> > > >  #define MC33XS2410_WDT				0x14
> > > >  
> > > > +#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)
> > > 
> > > Keep the registers in address order please
> >
> > Aren't they ? There are "ctrl" and "diag" registers and I didn't mix up
> > adresses. There is a comment for each of them when defines start.
> 
> +#define MC33XS2410_TEMP_WT                     0x29
> ...
> +#define MC33XS2410_TS_TEMP_DIE                 0x26
>

MC33XS2410_TEMP_WT is a "ctrl" register and MC33XS2410_TS_TEMP_DIE is a
"diag" register. Both register sets start with adress 0. "ctrl" and
"diag" defines are separated because I didn't want to mix them up.

[...]

Best regards,
Dimitri Fedrau