pwm: add support for NXPs high-side switch MC33XS2410

drivers/pwm/pwm-mc33xs2410.c new file +49 -0

new file mode 100644
index 000000000000..f9a334a5e69b
@@ -0,0 +1,422 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2024 Liebherr-Electronics and Drives GmbH
+ *
+ * Reference Manual : https://www.nxp.com/docs/en/data-sheet/MC33XS2410.pdf
+ *
+ * Limitations:
+ * - Supports frequencies between 0.5Hz and 2048Hz with following steps:
+ *   - 0.5 Hz steps from 0.5 Hz to 32 Hz
+ *   - 2 Hz steps from 2 Hz to 128 Hz
+ *   - 8 Hz steps from 8 Hz to 512 Hz
+ *   - 32 Hz steps from 32 Hz to 2048 Hz
+ * - Cannot generate a 0 % duty cycle.
+ * - Always produces low output if disabled.
+ * - Configuration isn't atomic. When changing polarity, duty cycle or period
+ *   the data is taken immediately, counters not being affected, resulting in a
+ *   behavior of the output pin that is neither the old nor the new state,
+ *   rather something in between.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/math64.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pwm.h>
+
+#include <asm/unaligned.h>
+
+#include <linux/spi/spi.h>
+
+#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)
+#define MC33XS2410_PWM_CTRL1		0x05
+#define MC33XS2410_PWM_CTRL1_POL_INV(x)	BIT(x)
+#define MC33XS2410_PWM_CTRL3		0x07
+/* x in { 0 ... 3 } */
+#define MC33XS2410_PWM_CTRL3_EN(x)	BIT(4 + (x))
+#define MC33XS2410_PWM_FREQ1		0x08
+/* x in { 1 ... 4 } */
+#define MC33XS2410_PWM_FREQ(x)		(MC33XS2410_PWM_FREQ1 + (x - 1))
+#define MC33XS2410_PWM_FREQ_STEP_MASK	GENMASK(7, 6)
+#define MC33XS2410_PWM_FREQ_COUNT_MASK	GENMASK(5, 0)
+#define MC33XS2410_PWM_DC1		0x0c
+/* x in { 1 ... 4 } */
+#define MC33XS2410_PWM_DC(x)		(MC33XS2410_PWM_DC1 + (x - 1))

drivers/pwm/pwm-mc33xs2410.c new file +4 -0

+#define MC33XS2410_MAX_TRANSFERS	5
+#define MC33XS2410_WORD_LEN		2
+
+struct mc33xs2410_pwm {

drivers/pwm/pwm-mc33xs2410.c new file +25 -0

+{
+	struct spi_transfer t[MC33XS2410_MAX_TRANSFERS] = { { 0 } };
+	u8 tx[MC33XS2410_MAX_TRANSFERS * MC33XS2410_WORD_LEN];
+	u8 rx[MC33XS2410_MAX_TRANSFERS * MC33XS2410_WORD_LEN];
+	int i, ret, reg_i, val_i;
+
+	if (!len)
+		return 0;
+
+	if (read)
+		len++;
+
+	if (len > MC33XS2410_MAX_TRANSFERS)
+		return -EINVAL;
+
+	for (i = 0; i < len; i++) {
+		reg_i = i * MC33XS2410_WORD_LEN;
+		val_i = reg_i + 1;
+		if (read) {
+			if (i < len - 1) {
+				tx[reg_i] = reg[i];
+				tx[val_i] = ctrl[i] ? MC33XS2410_RD_CTRL : 0;
+				t[i].tx_buf = &tx[reg_i];
+			}
+

drivers/pwm/pwm-mc33xs2410.c new file +11 -0

+	}
+
+	t[len - 1].cs_change = 0;
+
+	ret = spi_sync_transfer(spi, &t[0], len);
+	if (ret < 0)
+		return ret;
+
+	if (read) {
+		for (i = 0; i < len - 1; i++) {
+			reg_i = i * MC33XS2410_WORD_LEN;

drivers/pwm/pwm-mc33xs2410.c new file +4 -0

+}
[...]
+static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+				const struct pwm_state *state)
+{

drivers/pwm/pwm-mc33xs2410.c new file +13 -0

+	/* polarity */
+	mask = MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm);
+	val[2] = (state->polarity == PWM_POLARITY_INVERSED) ?
+		 (val[2] | mask) : (val[2] & ~mask);
+
+	/* enable output */
+	mask = MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm);
+	val[3] = (state->enabled && rel_dc >= 0) ? (val[3] | mask) :
+						   (val[3] & ~mask);
+
+	return mc33xs2410_write_regs(spi, reg, val, 4);
+}
+

drivers/pwm/pwm-mc33xs2410.c new file +3 -0

+	return 0;
+}
+
[...]

drivers/pwm/pwm-mc33xs2410.c line 59

Quoted 5 lines

drivers/pwm/pwm-mc33xs2410.c new file +5 -0

+
+#define MC33XS2410_MIN_PERIOD		488282
+#define MC33XS2410_MAX_PERIOD_STEP0	2000000000
+/* x in { 0 ... 3 } */
+#define MC33XS2410_MAX_PERIOD_STEP(x)	(MC33XS2410_MAX_PERIOD_STEP0 >> (2 * x))

Nitpick: These register definition become easier to parse for a human if you indent the RHS of register fields one tab further and add an empty line between the definitions for different registers.

MC33XS2410_PWM_DC1 is only used once, I'd hard-code it into the definition of MC33XS2410_PWM_DC.

The register fields [7:4] in MC33XS2410_PWM_CTRL3 are called PWM_ON4 .. PWM_ON1. So your x in { 0 ... 3 } is wrong. (Luckily, having some x range over { 0 ... 3 } and others orver { 1 ... 4 } is prone to error and confusion.)

Also I'd drop all _MASK suffixes.

For MC33XS2410_MAX_PERIOD_STEP maybe use a different variable name than for the others. For the register definitions the range is over hwpwm (which might be a good name there?), for MC33XS2410_MAX_PERIOD_STEP it's about MC33XS2410_PWM_FREQ_STEP.

Quoted 8 lines

drivers/pwm/pwm-mc33xs2410.c new file +8 -0

+#define MC33XS2410_MAX_TRANSFERS	5
+#define MC33XS2410_WORD_LEN		2
+
+struct mc33xs2410_pwm {
+	struct spi_device *spi;
+};
+
+static inline struct mc33xs2410_pwm *mc33xs2410_from_chip(struct pwm_chip *chip)

drivers/pwm/pwm-mc33xs2410.c line 74

Quoted 6 lines

drivers/pwm/pwm-mc33xs2410.c new file +6 -0

+{
+	return pwmchip_get_drvdata(chip);
+}
+
+static int mc33xs2410_xfer_regs(struct spi_device *spi, bool read, u8 *reg,
+				u16 *val, bool *ctrl, int len)

Should len better be unsigned?

Unless I missed something all ctrl[x] are always identical. If so represent that by a single bool.

Quoted 29 lines

drivers/pwm/pwm-mc33xs2410.c new file +29 -0

+{
+	struct spi_transfer t[MC33XS2410_MAX_TRANSFERS] = { { 0 } };
+	u8 tx[MC33XS2410_MAX_TRANSFERS * MC33XS2410_WORD_LEN];
+	u8 rx[MC33XS2410_MAX_TRANSFERS * MC33XS2410_WORD_LEN];
+	int i, ret, reg_i, val_i;
+
+	if (!len)
+		return 0;
+
+	if (read)
+		len++;
+
+	if (len > MC33XS2410_MAX_TRANSFERS)
+		return -EINVAL;
+
+	for (i = 0; i < len; i++) {
+		reg_i = i * MC33XS2410_WORD_LEN;
+		val_i = reg_i + 1;
+		if (read) {
+			if (i < len - 1) {
+				tx[reg_i] = reg[i];
+				tx[val_i] = ctrl[i] ? MC33XS2410_RD_CTRL : 0;
+				t[i].tx_buf = &tx[reg_i];
+			}
+
+			if (i > 0)
+				t[i].rx_buf = &rx[reg_i - MC33XS2410_WORD_LEN];
+		} else {
+			tx[reg_i] = reg[i] | MC33XS2410_WR;

drivers/pwm/pwm-mc33xs2410.c line 109

Quoted 6 lines

drivers/pwm/pwm-mc33xs2410.c new file +6 -0

+			tx[val_i] = val[i];
+			t[i].tx_buf = &tx[reg_i];
+		}
+
+		t[i].len = MC33XS2410_WORD_LEN;
+		t[i].cs_change = 1;

Not sure if MC33XS2410_WORD_LEN really improves readability here.

Why is this done using $len transfers, wouldn't a single one do (and maybe be more performant and not rely on a spi controller that supports cs_change)?

Quoted 16 lines

drivers/pwm/pwm-mc33xs2410.c new file +16 -0

+	}
+
+	t[len - 1].cs_change = 0;
+
+	ret = spi_sync_transfer(spi, &t[0], len);
+	if (ret < 0)
+		return ret;
+
+	if (read) {
+		for (i = 0; i < len - 1; i++) {
+			reg_i = i * MC33XS2410_WORD_LEN;
+			val[i] = FIELD_GET(MC33XS2410_RD_DATA_MASK,
+					   get_unaligned_be16(&rx[reg_i]));
+		}
+	}
+

drivers/pwm/pwm-mc33xs2410.c line 149

Quoted 5 lines

drivers/pwm/pwm-mc33xs2410.c new file +4 -0

+	return 0;
+}
[...]
+static
+int mc33xs2410_read_reg(struct spi_device *spi, u8 reg, u16 *val, bool ctrl)

My personal opinion: Better break the line in the argument list or not at all. Having a "static" on its own line looks ugly.

drivers/pwm/pwm-mc33xs2410.c line 232

Quoted 9 lines

drivers/pwm/pwm-mc33xs2410.c new file +7 -0

+{
+	return mc33xs2410_read_regs(spi, &reg, &ctrl, val, 1);
+}
[...]
+static u64 mc33xs2410_pwm_get_period(u8 reg)
+{
[...]
+	/* Convert frequency to period, considering the doubled frequency. */
+	return DIV_ROUND_UP((u32)(2 * NSEC_PER_SEC), freq);

That u32 cast isn't needed.

Quoted 13 lines

drivers/pwm/pwm-mc33xs2410.c new file +11 -0

+}
[...]
+static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+				const struct pwm_state *state)
+{
[...]
+	/* frequency */
+	val[0] = mc33xs2410_pwm_get_freq(period);
+	/* Continue calculations with the possibly truncated period */
+	period = mc33xs2410_pwm_get_period(val[0]);
+
+	/* duty cycle */
+	duty_cycle = min(period, state->duty_cycle);

drivers/pwm/pwm-mc33xs2410.c line 289

Quoted 2 lines

drivers/pwm/pwm-mc33xs2410.c new file +2 -0

+	rel_dc = mc33xs2410_pwm_get_relative_duty_cycle(period, duty_cycle);
+	val[1] = rel_dc < 0 ? 0 : rel_dc;

Handling of the duty cycle is correct here, but misleading. I already added a comment here that using val[1] = 0 if rel_dc < 0 is wrong and just deleted it again after I saw (rel_dc >= 0) being used determining the value for MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm). An explicit if block would make this more obvious.

mc33xs2410_pwm_get_relative_duty_cycle() is simple enough and only used once that I'd unroll it here.

Quoted 18 lines

drivers/pwm/pwm-mc33xs2410.c new file +17 -0

+	/* polarity */
+	mask = MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm);
+	val[2] = (state->polarity == PWM_POLARITY_INVERSED) ?
+		 (val[2] | mask) : (val[2] & ~mask);
+
+	/* enable output */
+	mask = MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm);
+	val[3] = (state->enabled && rel_dc >= 0) ? (val[3] | mask) :
+						   (val[3] & ~mask);
+
+	return mc33xs2410_write_regs(spi, reg, val, 4);
+}
+
+static int mc33xs2410_pwm_get_state(struct pwm_chip *chip,
+				    struct pwm_device *pwm,
+				    struct pwm_state *state)
+{
[...]

drivers/pwm/pwm-mc33xs2410.c line 328

Quoted 5 lines

drivers/pwm/pwm-mc33xs2410.c new file +5 -0

+	state->period = mc33xs2410_pwm_get_period(val[0]);
+	state->polarity = (val[2] & MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm)) ?
+			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+	state->enabled = !!(val[3] & MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm));
+	mc33xs2410_pwm_set_relative_duty_cycle(state, val[1]);

No need to set state->duty_cycle = 0 if state->enabled is false. This is another function I suggest to unroll as it hides more than it abstracts.

Quoted 9 lines

drivers/pwm/pwm-mc33xs2410.c new file +7 -0

+	return 0;
+}
+
[...]
+static int mc33xs2410_probe(struct spi_device *spi)
+{
[...]
+	/* Transition to normal mode */
+	ret = mc33xs2410_modify_reg(spi, MC33XS2410_GLB_CTRL,

drivers/pwm/pwm-mc33xs2410.c line 389

Quoted 5 lines

drivers/pwm/pwm-mc33xs2410.c new file +5 -0

+				    MC33XS2410_GLB_CTRL_MODE,
+				    MC33XS2410_GLB_CTRL_MODE_NORMAL);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Failed to transition to normal mode\n");

What is the effect of this register write if the PWM was already setup by the bootloader?

drivers/pwm/pwm-mc33xs2410.c line 396

Quoted 7 lines

drivers/pwm/pwm-mc33xs2410.c new file +7 -0

+
+	ret = devm_pwmchip_add(dev, chip);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to add pwm chip\n");
+
+	return 0;
+}

Best regards
Uwe

Am Tue, Oct 22, 2024 at 09:54:50AM +0200 schrieb Uwe Kleine-König:

[...]

Quoted 9 lines

drivers/pwm/pwm-mc33xs2410.c line 59

Quoted 5 lines

drivers/pwm/pwm-mc33xs2410.c new file +5 -0

+
+#define MC33XS2410_MIN_PERIOD		488282
+#define MC33XS2410_MAX_PERIOD_STEP0	2000000000
+/* x in { 0 ... 3 } */
+#define MC33XS2410_MAX_PERIOD_STEP(x)	(MC33XS2410_MAX_PERIOD_STEP0 >> (2 * x))

Nitpick: These register definition become easier to parse for a human if you indent the RHS of register fields one tab further and add an empty line between the definitions for different registers.

Adding an empty line seems reasonable to me but the additional tab doesn't help me to improve readability.

Quoted 2 lines

MC33XS2410_PWM_DC1 is only used once, I'd hard-code it into the definition of MC33XS2410_PWM_DC.

Ok. Should I do the same for MC33XS2410_PWM_FREQ1 and MC33XS2410_MAX_PERIOD_STEP0 ?

Quoted 4 lines

The register fields [7:4] in MC33XS2410_PWM_CTRL3 are called PWM_ON4 .. PWM_ON1. So your x in { 0 ... 3 } is wrong. (Luckily, having some x range over { 0 ... 3 } and others orver { 1 ... 4 } is prone to error and confusion.)

Will fix it. Should I do the same for MC33XS2410_PWM_CTRL1_POL_INV ?

Quoted 4 lines

For MC33XS2410_MAX_PERIOD_STEP maybe use a different variable name than for the others. For the register definitions the range is over hwpwm (which might be a good name there?), for MC33XS2410_MAX_PERIOD_STEP it's about MC33XS2410_PWM_FREQ_STEP.

What about MC33XS2410_PWM_MAX_PERIOD(x) ?

Quoted 9 lines

Quoted 9 lines

drivers/pwm/pwm-mc33xs2410.c new file +9 -0

+#define MC33XS2410_MAX_TRANSFERS	5
+#define MC33XS2410_WORD_LEN		2
+
+struct mc33xs2410_pwm {
+	struct spi_device *spi;
+};
+
+static inline struct mc33xs2410_pwm *mc33xs2410_from_chip(struct pwm_chip *chip)
+{

Quoted 8 lines

drivers/pwm/pwm-mc33xs2410.c line 74

Quoted 5 lines

drivers/pwm/pwm-mc33xs2410.c new file +5 -0

+	return pwmchip_get_drvdata(chip);
+}
+
+static int mc33xs2410_xfer_regs(struct spi_device *spi, bool read, u8 *reg,
+				u16 *val, bool *ctrl, int len)

Unless I missed something all ctrl[x] are always identical. If so represent that by a single bool.

Yes, they are identical. I added the crtl[x] to be able read from ctrl and diag registers. I will change it so it is represented by a single bool, if the feature is needed in the future I can still add it.

Quoted 29 lines

Quoted 29 lines

drivers/pwm/pwm-mc33xs2410.c new file +29 -0

+{
+	struct spi_transfer t[MC33XS2410_MAX_TRANSFERS] = { { 0 } };
+	u8 tx[MC33XS2410_MAX_TRANSFERS * MC33XS2410_WORD_LEN];
+	u8 rx[MC33XS2410_MAX_TRANSFERS * MC33XS2410_WORD_LEN];
+	int i, ret, reg_i, val_i;
+
+	if (!len)
+		return 0;
+
+	if (read)
+		len++;
+
+	if (len > MC33XS2410_MAX_TRANSFERS)
+		return -EINVAL;
+
+	for (i = 0; i < len; i++) {
+		reg_i = i * MC33XS2410_WORD_LEN;
+		val_i = reg_i + 1;
+		if (read) {
+			if (i < len - 1) {
+				tx[reg_i] = reg[i];
+				tx[val_i] = ctrl[i] ? MC33XS2410_RD_CTRL : 0;
+				t[i].tx_buf = &tx[reg_i];
+			}
+
+			if (i > 0)
+				t[i].rx_buf = &rx[reg_i - MC33XS2410_WORD_LEN];
+		} else {
+			tx[reg_i] = reg[i] | MC33XS2410_WR;

Quoted 8 lines

drivers/pwm/pwm-mc33xs2410.c line 109

Quoted 6 lines

drivers/pwm/pwm-mc33xs2410.c new file +6 -0

+			tx[val_i] = val[i];
+			t[i].tx_buf = &tx[reg_i];
+		}
+
+		t[i].len = MC33XS2410_WORD_LEN;
+		t[i].cs_change = 1;

Not sure if MC33XS2410_WORD_LEN really improves readability here.

It is used throughout in the function and improves readability overall, maybe not here but for consistency I would stick to it.

Quoted 3 lines

Why is this done using $len transfers, wouldn't a single one do (and maybe be more performant and not rely on a spi controller that supports cs_change)?

Without cs_change after every 16 bit, requests aren't processed by the device. Reading/writing from/to device fails. The SPI controller therefore must support cs_change. Single transfer is not possible because of the cs_change after every 16bit.

Quoted 18 lines

Quoted 18 lines

drivers/pwm/pwm-mc33xs2410.c new file +17 -0

+	/* polarity */
+	mask = MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm);
+	val[2] = (state->polarity == PWM_POLARITY_INVERSED) ?
+		 (val[2] | mask) : (val[2] & ~mask);
+
+	/* enable output */
+	mask = MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm);
+	val[3] = (state->enabled && rel_dc >= 0) ? (val[3] | mask) :
+						   (val[3] & ~mask);
+
+	return mc33xs2410_write_regs(spi, reg, val, 4);
+}
+
+static int mc33xs2410_pwm_get_state(struct pwm_chip *chip,
+				    struct pwm_device *pwm,
+				    struct pwm_state *state)
+{
[...]

Quoted 8 lines

drivers/pwm/pwm-mc33xs2410.c line 328

Quoted 5 lines

drivers/pwm/pwm-mc33xs2410.c new file +5 -0

+	state->period = mc33xs2410_pwm_get_period(val[0]);
+	state->polarity = (val[2] & MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm)) ?
+			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+	state->enabled = !!(val[3] & MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm));
+	mc33xs2410_pwm_set_relative_duty_cycle(state, val[1]);

No need to set state->duty_cycle = 0 if state->enabled is false. This is another function I suggest to unroll as it hides more than it abstracts.

Function can be unrolled, but the check for state->enabled is needed. The device is unable to generate a 0% duty cycle, so it is turned off to generate a 0% duty cylce.

Quoted 6 lines

Quoted 6 lines

[...]

drivers/pwm/pwm-mc33xs2410.c new file +4 -0

+static int mc33xs2410_probe(struct spi_device *spi)
+{
[...]
+	/* Transition to normal mode */
+	ret = mc33xs2410_modify_reg(spi, MC33XS2410_GLB_CTRL,

Quoted 9 lines

drivers/pwm/pwm-mc33xs2410.c line 389

Quoted 5 lines

drivers/pwm/pwm-mc33xs2410.c new file +5 -0

+				    MC33XS2410_GLB_CTRL_MODE,
+				    MC33XS2410_GLB_CTRL_MODE_NORMAL);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Failed to transition to normal mode\n");

What is the effect of this register write if the PWM was already setup by the bootloader?

When its setup is done in the bootloader and the watchdog is disabled in the bootloader it shouldn't have any impact.

Hello Dimitri,

Quoted 39 lines · Dimitri Fedrau

Am Tue, Oct 22, 2024 at 09:54:50AM +0200 schrieb Uwe Kleine-König:

Quoted 37 lines

Quoted 25 lines

drivers/pwm/pwm-mc33xs2410.c new file +25 -0

+{
+	struct spi_transfer t[MC33XS2410_MAX_TRANSFERS] = { { 0 } };
+	u8 tx[MC33XS2410_MAX_TRANSFERS * MC33XS2410_WORD_LEN];
+	u8 rx[MC33XS2410_MAX_TRANSFERS * MC33XS2410_WORD_LEN];
+	int i, ret, reg_i, val_i;
+
+	if (!len)
+		return 0;
+
+	if (read)
+		len++;
+
+	if (len > MC33XS2410_MAX_TRANSFERS)
+		return -EINVAL;
+
+	for (i = 0; i < len; i++) {
+		reg_i = i * MC33XS2410_WORD_LEN;
+		val_i = reg_i + 1;
+		if (read) {
+			if (i < len - 1) {
+				tx[reg_i] = reg[i];
+				tx[val_i] = ctrl[i] ? MC33XS2410_RD_CTRL : 0;
+				t[i].tx_buf = &tx[reg_i];
+			}
+

drivers/pwm/pwm-mc33xs2410.c line 109

Quoted 10 lines

drivers/pwm/pwm-mc33xs2410.c new file +10 -0

+			if (i > 0)
+				t[i].rx_buf = &rx[reg_i - MC33XS2410_WORD_LEN];
+		} else {
+			tx[reg_i] = reg[i] | MC33XS2410_WR;
+			tx[val_i] = val[i];
+			t[i].tx_buf = &tx[reg_i];
+		}
+
+		t[i].len = MC33XS2410_WORD_LEN;
+		t[i].cs_change = 1;

Not sure if MC33XS2410_WORD_LEN really improves readability here.

Quoted 2 lines

It is used throughout in the function and improves readability overall, maybe not here but for consistency I would stick to it.

Seems to be subjective.

Quoted 8 lines

Quoted 3 lines

Why is this done using $len transfers, wouldn't a single one do (and maybe be more performant and not rely on a spi controller that supports cs_change)?

Without cs_change after every 16 bit, requests aren't processed by the device. Reading/writing from/to device fails. The SPI controller therefore must support cs_change. Single transfer is not possible because of the cs_change after every 16bit.

There is SPI_CS_WORD for this usecase.

Quoted 27 lines

Quoted 26 lines

Quoted 13 lines

drivers/pwm/pwm-mc33xs2410.c new file +13 -0

+	/* polarity */
+	mask = MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm);
+	val[2] = (state->polarity == PWM_POLARITY_INVERSED) ?
+		 (val[2] | mask) : (val[2] & ~mask);
+
+	/* enable output */
+	mask = MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm);
+	val[3] = (state->enabled && rel_dc >= 0) ? (val[3] | mask) :
+						   (val[3] & ~mask);
+
+	return mc33xs2410_write_regs(spi, reg, val, 4);
+}
+

drivers/pwm/pwm-mc33xs2410.c line 328

Quoted 10 lines

drivers/pwm/pwm-mc33xs2410.c new file +9 -0

+static int mc33xs2410_pwm_get_state(struct pwm_chip *chip,
+				    struct pwm_device *pwm,
+				    struct pwm_state *state)
+{
[...]
+	state->period = mc33xs2410_pwm_get_period(val[0]);
+	state->polarity = (val[2] & MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm)) ?
+			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+	state->enabled = !!(val[3] & MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm));
+	mc33xs2410_pwm_set_relative_duty_cycle(state, val[1]);

No need to set state->duty_cycle = 0 if state->enabled is false. This is another function I suggest to unroll as it hides more than it abstracts.

Quoted 3 lines

Function can be unrolled, but the check for state->enabled is needed. The device is unable to generate a 0% duty cycle, so it is turned off to generate a 0% duty cylce.

What breaks if you drop the check for state->enabled?

Am Thu, Oct 24, 2024 at 11:19:16PM +0200 schrieb Uwe Kleine-König:

Quoted 2 lines

What breaks if you drop the check for state->enabled?

The device is unable to generate a 0% duty cycle, to support this you proposed in an earlier review to disable the output. Without checking if the output is disabled, the mc33xs2410_pwm_get_state function returns the wrong duty cycle for a previously setted 0% duty cycle. A "0" value in the MC33XS2410_PWM_DC register means that the relative duty cylce is 1/256. As a result there are complaints if PWM_DEBUG is enabled.

Hello Dimitri,

Quoted 9 lines · Dimitri Fedrau

Am Thu, Oct 24, 2024 at 11:19:16PM +0200 schrieb Uwe Kleine-König:

Quoted 2 lines

What breaks if you drop the check for state->enabled?

The device is unable to generate a 0% duty cycle, to support this you proposed in an earlier review to disable the output. Without checking if the output is disabled, the mc33xs2410_pwm_get_state function returns the wrong duty cycle for a previously setted 0% duty cycle. A "0" value in the MC33XS2410_PWM_DC register means that the relative duty cylce is 1/256. As a result there are complaints if PWM_DEBUG is enabled.

I fail to follow. If .enabled=true + .duty_cycle=0 is requested you disable. That's fine. However it shouldn't be necessary to use state->enabled in .get_state(). I didn't look at the actual code, but if you provide a sequence of writes to /sys that trigger a PWM_DEBUG output, I'll take another look.

Hello Uwe,

Am Sun, Nov 03, 2024 at 09:19:36PM +0100 schrieb Uwe Kleine-König:

Quoted 13 lines

Hello Dimitri,

Quoted 9 lines · Dimitri Fedrau

Am Thu, Oct 24, 2024 at 11:19:16PM +0200 schrieb Uwe Kleine-König:

Quoted 2 lines

What breaks if you drop the check for state->enabled?

The device is unable to generate a 0% duty cycle, to support this you proposed in an earlier review to disable the output. Without checking if the output is disabled, the mc33xs2410_pwm_get_state function returns the wrong duty cycle for a previously setted 0% duty cycle. A "0" value in the MC33XS2410_PWM_DC register means that the relative duty cylce is 1/256. As a result there are complaints if PWM_DEBUG is enabled.

Quoted 6 lines

I fail to follow. If .enabled=true + .duty_cycle=0 is requested you disable. That's fine. However it shouldn't be necessary to use state->enabled in .get_state(). I didn't look at the actual code, but if you provide a sequence of writes to /sys that trigger a PWM_DEBUG output, I'll take another look.

Apply 0% duty cycle: .enabled=false + .duty_cycle=0 Below some writes triggering PWM_DEBUG output:

# echo 488282 > /sys/class/pwm/pwmchip3/pwm0/period
# echo 244140 > /sys/class/pwm/pwmchip3/pwm0/duty_cycle
# echo 0 > /sys/class/pwm/pwmchip3/pwm0/duty_cycle
[   91.813513] mc33xs2410-pwm spi0.0: .apply is supposed to round down duty_cycle (requested: 0/488282, applied: 1908/488282)

Quoted 22 lines

Hello Uwe,

Am Sun, Nov 03, 2024 at 09:19:36PM +0100 schrieb Uwe Kleine-König:

Quoted 13 lines

Hello Dimitri,

Quoted 9 lines · Dimitri Fedrau

Am Thu, Oct 24, 2024 at 11:19:16PM +0200 schrieb Uwe Kleine-König:

Quoted 2 lines

What breaks if you drop the check for state->enabled?

The device is unable to generate a 0% duty cycle, to support this you proposed in an earlier review to disable the output. Without checking if the output is disabled, the mc33xs2410_pwm_get_state function returns the wrong duty cycle for a previously setted 0% duty cycle. A "0" value in the MC33XS2410_PWM_DC register means that the relative duty cylce is 1/256. As a result there are complaints if PWM_DEBUG is enabled.

Quoted 6 lines

I fail to follow. If .enabled=true + .duty_cycle=0 is requested you disable. That's fine. However it shouldn't be necessary to use state->enabled in .get_state(). I didn't look at the actual code, but if you provide a sequence of writes to /sys that trigger a PWM_DEBUG output, I'll take another look.

Quoted 7 lines

Apply 0% duty cycle: .enabled=false + .duty_cycle=0 Below some writes triggering PWM_DEBUG output:

# echo 488282 > /sys/class/pwm/pwmchip3/pwm0/period
# echo 244140 > /sys/class/pwm/pwmchip3/pwm0/duty_cycle
# echo 0 > /sys/class/pwm/pwmchip3/pwm0/duty_cycle
[   91.813513] mc33xs2410-pwm spi0.0: .apply is supposed to round down duty_cycle (requested: 0/488282, applied: 1908/488282)

I don't understand that. We're talking about

drivers/pwm/pwm-mc33xs2410.c +1 -10

index f9a334a5e69b..14f5f7312d0a 100644
@@ -244,15 +244,6 @@ static int mc33xs2410_pwm_get_relative_duty_cycle(u64 period, u64 duty_cycle)
 	return duty_cycle - 1;
 }
 
-static void mc33xs2410_pwm_set_relative_duty_cycle(struct pwm_state *state,
-						   u16 duty_cycle)
-{
-	if (!state->enabled)
-		state->duty_cycle = 0;
-	else
-		state->duty_cycle = DIV_ROUND_UP_ULL((duty_cycle + 1) * state->period, 256);
-}
-
 static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 				const struct pwm_state *state)
 {

@@ -325,7 +316,7 @@ static int mc33xs2410_pwm_get_state(struct pwm_chip *chip,
 	state->polarity = (val[2] & MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm)) ?
 			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
 	state->enabled = !!(val[3] & MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm));
-	mc33xs2410_pwm_set_relative_duty_cycle(state, val[1]);
+	state->duty_cycle = DIV_ROUND_UP_ULL((duty_cycle + 1) * state->period, 256);
 	return 0;
 }
 

on top of your patch, right?

echo 0 > /sys/class/pwm/pwmchip3/pwm0/duty_cycle should result in MC33XS2410_PWM_CTRL3 having MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm) cleared. When mc33xs2410_pwm_get_state() is called then it returns state->enabled = false and in that case the above mentioned warning doesn't trigger.

Where is the misunderstanding?

Am Mon, Nov 04, 2024 at 09:52:51AM +0100 schrieb Uwe Kleine-König:

Quoted 4 lines

echo 0 > /sys/class/pwm/pwmchip3/pwm0/duty_cycle should result in MC33XS2410_PWM_CTRL3 having MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm) cleared. When mc33xs2410_pwm_get_state() is called then it returns state->enabled = false and in that case the above mentioned warning doesn't trigger.

Yes, as you explained. But the warning is shown.

Quoted 1 line

Where is the misunderstanding?

if (state->enabled && state->duty_cycle < s2.duty_cycle)

	dev_warn(pwmchip_parent(chip),
		".apply is supposed to round down duty_cycle (requested: %llu/%llu, applied: %llu/%llu)\n",
		state->duty_cycle, state->period,
		s2.duty_cycle, s2.period);

state has previously applied settings and is parameter of pwm_apply_debug, in that case s2=s1, and s1 is returned by get_state:

state->enabled=true
state->duty_cycle=0
s2.enabled=false
s2.duty_cycle=1908

Due to the code the warning should be raised. If it shouldn't the check should be different, something like if (state->enabled && s2.enabled && ...)