| Message ID | 20250512-mc33xs2410-hwmon-v1-1-addba77c78f9@liebherr.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: mc33xs2410: add support for temperature sensors | expand |
On 5/12/25 04:26, Dimitri Fedrau via B4 Relay wrote: > 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. > > Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com> > --- > + > +static int mc33xs2410_hwmon_read_out_status(struct spi_device *spi, > + int channel, u16 *val) > +{ > + int ret; > + > + ret = mc33xs2410_read_reg_diag(spi, MC33XS2410_OUT_STA(channel), val); > + if (ret < 0) > + return ret; > + > + /* Bits latches high */ > + return mc33xs2410_read_reg_diag(spi, MC33XS2410_OUT_STA(channel), val); Is that double read of the same register needed ? If so, you'll probably need a lock to prevent it from being executed from multiple threads at the same time. The comment "Bit latches high" doesn't really mean anything to me and doesn't explain why the register needs to be read twice. Thanks, Guenter
Hi Guenter, Am Mon, May 12, 2025 at 06:04:33AM -0700 schrieb Guenter Roeck: > On 5/12/25 04:26, Dimitri Fedrau via B4 Relay wrote: > > 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. > > > > Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com> > > --- > > > + > > +static int mc33xs2410_hwmon_read_out_status(struct spi_device *spi, > > + int channel, u16 *val) > > +{ > > + int ret; > > + > > + ret = mc33xs2410_read_reg_diag(spi, MC33XS2410_OUT_STA(channel), val); > > + if (ret < 0) > > + return ret; > > + > > + /* Bits latches high */ > > + return mc33xs2410_read_reg_diag(spi, MC33XS2410_OUT_STA(channel), val); > > Is that double read of the same register needed ? If so, you'll probably > need a lock to prevent it from being executed from multiple threads at the > same time. > > The comment "Bit latches high" doesn't really mean anything to me and doesn't > explain why the register needs to be read twice. > > All bits of the output status registers are latched high. In case there was overtemperature detected, the bit stays set until read once and cleared afterwards. So I need a second read to get the "realtime" status. Otherwise I might end up returning an false positive overtemperature warning. I don't think a lock is really necessary, since I'm only interested in the "realtime" status but not if there was a warning in the past. What do you think ? Will improve the comment. Best regards Dimitri Fedrau
On 5/12/25 06:31, Dimitri Fedrau wrote: > Hi Guenter, > > Am Mon, May 12, 2025 at 06:04:33AM -0700 schrieb Guenter Roeck: >> On 5/12/25 04:26, Dimitri Fedrau via B4 Relay wrote: >>> 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. >>> >>> Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com> >>> --- >> >>> + >>> +static int mc33xs2410_hwmon_read_out_status(struct spi_device *spi, >>> + int channel, u16 *val) >>> +{ >>> + int ret; >>> + >>> + ret = mc33xs2410_read_reg_diag(spi, MC33XS2410_OUT_STA(channel), val); >>> + if (ret < 0) >>> + return ret; >>> + >>> + /* Bits latches high */ >>> + return mc33xs2410_read_reg_diag(spi, MC33XS2410_OUT_STA(channel), val); >> >> Is that double read of the same register needed ? If so, you'll probably >> need a lock to prevent it from being executed from multiple threads at the >> same time. >> >> The comment "Bit latches high" doesn't really mean anything to me and doesn't >> explain why the register needs to be read twice. >> >> > > All bits of the output status registers are latched high. In case there > was overtemperature detected, the bit stays set until read once and cleared > afterwards. So I need a second read to get the "realtime" status. > Otherwise I might end up returning an false positive overtemperature > warning. I don't think a lock is really necessary, since I'm only > interested in the "realtime" status but not if there was a warning in > the past. What do you think ? > Hardware monitoring is _expected_ to report the last latched status and clear it afterwards, to ensure that historic alarms are reported at least once. This isn't about "false positive", it is about "report at least once if possible". Given that, the second read is unnecessary from hwmon ABI perspective. If you don't want to do that, you should explicitly document that latched (historic) over-temperature alarms are not reported. Guenter
Am Mon, May 12, 2025 at 06:53:21AM -0700 schrieb Guenter Roeck: > On 5/12/25 06:31, Dimitri Fedrau wrote: > > Hi Guenter, > > > > Am Mon, May 12, 2025 at 06:04:33AM -0700 schrieb Guenter Roeck: > > > On 5/12/25 04:26, Dimitri Fedrau via B4 Relay wrote: > > > > 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. > > > > > > > > Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com> > > > > --- > > > > > > > + > > > > +static int mc33xs2410_hwmon_read_out_status(struct spi_device *spi, > > > > + int channel, u16 *val) > > > > +{ > > > > + int ret; > > > > + > > > > + ret = mc33xs2410_read_reg_diag(spi, MC33XS2410_OUT_STA(channel), val); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + /* Bits latches high */ > > > > + return mc33xs2410_read_reg_diag(spi, MC33XS2410_OUT_STA(channel), val); > > > > > > Is that double read of the same register needed ? If so, you'll probably > > > need a lock to prevent it from being executed from multiple threads at the > > > same time. > > > > > > The comment "Bit latches high" doesn't really mean anything to me and doesn't > > > explain why the register needs to be read twice. > > > > > > > > > > All bits of the output status registers are latched high. In case there > > was overtemperature detected, the bit stays set until read once and cleared > > afterwards. So I need a second read to get the "realtime" status. > > Otherwise I might end up returning an false positive overtemperature > > warning. I don't think a lock is really necessary, since I'm only > > interested in the "realtime" status but not if there was a warning in > > the past. What do you think ? > > > > Hardware monitoring is _expected_ to report the last latched status and clear > it afterwards, to ensure that historic alarms are reported at least once. > This isn't about "false positive", it is about "report at least once if > possible". > Didn't know that, thanks for the explanation. > Given that, the second read is unnecessary from hwmon ABI perspective. If you > don't want to do that, you should explicitly document that latched (historic) > over-temperature alarms are not reported. > I would stick to hwmon ABI, just didn't know better. Best regards, Dimitri Fedrau
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 6faa8b2ec0a4844f667a84335f30bde44d52378e..0deaf8447f4302e7cfd3b4cb35c7d46ef19e006c 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -425,6 +425,7 @@ config PWM_LPSS_PLATFORM config PWM_MC33XS2410 tristate "MC33XS2410 PWM support" + depends on HWMON || HWMON=n depends on OF depends on SPI help diff --git a/drivers/pwm/pwm-mc33xs2410.c b/drivers/pwm/pwm-mc33xs2410.c index a1ac3445ccdb4709d92e0075d424a8abc1416eee..c3302b58b3acb60e5985c1c14746c380271eb6d6 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,157 @@ static int mc33xs2410_modify_reg(struct spi_device *spi, u8 reg, u8 mask, u8 val return mc33xs2410_write_reg(spi, reg, tmp); } +#if IS_ENABLED(CONFIG_HWMON) +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_out_status(struct spi_device *spi, + int channel, u16 *val) +{ + int ret; + + ret = mc33xs2410_read_reg_diag(spi, MC33XS2410_OUT_STA(channel), val); + if (ret < 0) + return ret; + + /* Bits latches high */ + return mc33xs2410_read_reg_diag(spi, MC33XS2410_OUT_STA(channel), val); +} + +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, ®_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_hwmon_read_out_status(spi, channel, ®_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, ®_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; + struct device *hwmon; + + hwmon = devm_hwmon_device_register_with_info(dev, NULL, spi, + &mc33xs2410_hwmon_chip_info, + NULL); + + return PTR_ERR_OR_ZERO(hwmon); +} + +#else +static int mc33xs2410_hwmon_probe(struct spi_device *spi) +{ + return 0; +} +#endif + static u8 mc33xs2410_pwm_get_freq(u64 period) { u8 step, count; @@ -361,7 +535,7 @@ static int mc33xs2410_probe(struct spi_device *spi) if (ret < 0) return dev_err_probe(dev, ret, "Failed to add pwm chip\n"); - return 0; + return mc33xs2410_hwmon_probe(spi); } static const struct spi_device_id mc33xs2410_spi_id[] = {