[PATCH v8 0/2] pwm: add support for NXPs high-side switch MC33XS2410

[PATCH v8 0/2] pwm: add support for NXPs high-side switch MC33XS2410

[Dimitri Fedrau]

The MC33XS2410 is a four channel high-side switch. Featuring advanced
monitoring and control function, the device is operational from 3.0 V to
60 V. The device is controlled by SPI port for configuration.

Changes in V2:
  - fix title in devicetree binding
  - fix commit message in devicetree binding patch
  - remove external clock from pwms and create clocks property
  - switch to unevaluatedProperties: false
  - add missing properties for complete example:
    - pwm-names
    - pwms
    - interrupts
    - clocks

Changes in V3:
  - Add description of the general behaviour of the device (limitations)
  - Drop unused defines
  - Add ranges comments for defines with parameters
  - Drop MC33XS2410_PERIOD_MAX, MC33XS2410_PERIOD_MIN defines
  - Drop mc33xs2410_period variable
  - Round down when calculating period and duty cycle
  - Use switch instead of loop when calculating frequency
  - Removed ret variable in mc33xs2410_pwm_get_freq
  - Handle all accesses in a single call to spi_sync_transfer
  - Fix comments in function mc33xs2410_pwm_get_period
  - Fix call pwm_set_relative_duty_cycle(state, ret, 255), instead
    pwm_set_relative_duty_cycle(state, val[1] + 1, 256);
  - Use devm_pwmchip_alloc
  - Fix typo s/Transitition/Transition/
  - Drop driver_data
  - Removed patch for direct inputs from series
  - Tested with PWM_DEBUG enabled, didn't before !

Changes in V4:
  - removed include of math.h, already included in math64.h
  - removed include of mutex.h, no mutexes are used
  - added include of bitfield.h(FIELD_GET, FIELD_PREP), fixes errors
    discovered by kernel test robot
  - cast parameters in DIV_ROUND_UP to u32, fixes errors discovered by
    kernel test robot

Changes in V5:
  - Fix comment in mc33xs2410_pwm_get_freq, selecting steps instead of
    period
  - Add comment in mc33xs2410_pwm_get_relative_duty_cycle that duty_cycle
    cannot overflow and and period is not zero, guaranteed by the caller
  - Hardware emits a low level when disabled, disable if duty_cycle = 0 is
    requested.
  - Fix complaints when PWM_DEBUG enabled, round-down division in
    mc33xs2410_pwm_apply and round-up in mc33xs2410_pwm_get_state.
  - Add comment for disabling watchdog in probe

Changes in V6:
  - Add link to manual
  - Redefine MC33XS2410_GLB_CTRL_MODE_MASK as MC33XS2410_GLB_CTRL_MODE and
    MC33XS2410_GLB_CTRL_NORMAL_MODE as MC33XS2410_GLB_CTRL_MODE_NORMAL
  - Remove define MC33XS2410_MIN_PERIOD_STEP(x) as there is no need for
    parameters and use instead MC33XS2410_MIN_PERIOD
  - Rename function to_pwm_mc33xs2410_chip to mc33xs2410_from_chip
  - Add comment in mc33xs2410_pwm_get_freq why count should be rounded up
  - Fix incorrect comment in mc33xs2410_pwm_get_period
  - Rename steps variable to doubled_steps in function
    mc33xs2410_pwm_get_freq
  - remove cast in mc33xs2410_pwm_set_relative_duty_cycle
  - remove duty_cycle from if in mc33xs2410_pwm_set_relative_duty_cycle

Changes in V7:
  - Add empty lines for defines with parameter
  - Hardcode into:
    - MC33XS2410_PWM_DC1 -> MC33XS2410_PWM_DC
    - MC33XS2410_PWM_FREQ1 -> MC33XS2410_PWM_FREQ
    - MC33XS2410_MAX_PERIOD_STEP0 -> MC33XS2410_MAX_PERIOD_STEP
  - Rename:
    - MC33XS2410_MIN_PERIOD -> MC33XS2410_PWM_MIN_PERIOD
    - MC33XS2410_MAX_PERIOD_STEP -> MC33XS2410_PWM_MAX_PERIOD
    - MC33XS2410_WR -> MC33XS2410_FRAME_IN_ADDR_WR
    - MC33XS2410_RD_CTRL -> MC33XS2410_FRAME_IN_DATA_RD
    - MC33XS2410_RD_DATA_MASK -> MC33XS2410_FRAME_OUT_DATA
  - Change range for MC33XS2410_PWM_CTRL1_POL_INV
  - Drop mask suffixes
  - Switch to unsigned int len in mc33xs2410_write_regs and
    mc33xs2410_read_regs
  - Remove define MC33XS2410_WORD_LEN
  - Use a single spi transfer in mc33xs2410_write_regs and
    mc33xs2410_read_regs by using SPI_CS_WORD and 16 bits per word
  - Break the line in the argument list instead of having a static in its
    own for mc33xs2410_read_reg definition
  - Remove u32 cast mc33xs2410_pwm_get_period
  - Unroll mc33xs2410_pwm_get_relative_duty_cycle
  - Unroll mc33xs2410_pwm_set_relative_duty_cycle and remove check for
    state->enabled
  - Remove ctrl[x] and use instead a u8 flag which indicates from which
    registers to read

Changes in V8:
  - Replace x for MC33XS2410_PWM_CTRL1, MC33XS2410_PWM_CTRL3_EN,
    MC33XS2410_PWM_FREQ and MC33XS2410_PWM_DC with chan
  - Add bracketing for parameters in defines
  - Replace x for MC33XS2410_PWM_MAX_PERIOD with step
  - Add comment in mc33xs2410_pwm_apply
  - Remove hardcoded 4 with ARRAY_SIZE(reg) in:
    ret = mc33xs2410_read_regs(spi, reg, MC33XS2410_FRAME_IN_DATA_RD, val, 4);
  - Remove struct mc33xs2410_pwm and save on indirection
  - Fixed wrong bitoffset of MC33XS2410_PWM_CTRL1_POL_INV:
    BIT((chan) + 1) instead of BIT((chan) - 1)

Dimitri Fedrau (2):
  dt-bindings: pwm: add support for MC33XS2410
  pwm: add support for NXPs high-side switch MC33XS2410

 .../bindings/pwm/nxp,mc33xs2410.yaml          | 118 ++++++
 drivers/pwm/Kconfig                           |  12 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-mc33xs2410.c                  | 378 ++++++++++++++++++
 4 files changed, 509 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/nxp,mc33xs2410.yaml
 create mode 100644 drivers/pwm/pwm-mc33xs2410.c

-- 
2.39.5

[PATCH v8 1/2] dt-bindings: pwm: add support for MC33XS2410

[Dimitri Fedrau]

Adding documentation for NXPs MC33XS2410 high side switch.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 .../bindings/pwm/nxp,mc33xs2410.yaml          | 118 ++++++++++++++++++
 1 file changed, 118 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/nxp,mc33xs2410.yaml

diff --git a/Documentation/devicetree/bindings/pwm/nxp,mc33xs2410.yaml b/Documentation/devicetree/bindings/pwm/nxp,mc33xs2410.yaml
new file mode 100644
index 000000000000..1729fe5c3dfb
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/nxp,mc33xs2410.yaml
@@ -0,0 +1,118 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/nxp,mc33xs2410.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: High-side switch MC33XS2410
+
+maintainers:
+  - Dimitri Fedrau <dima.fedrau@gmail.com>
+
+allOf:
+  - $ref: pwm.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    const: nxp,mc33xs2410
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 10000000
+
+  spi-cpha: true
+
+  spi-cs-setup-delay-ns:
+    minimum: 100
+    default: 100
+
+  spi-cs-hold-delay-ns:
+    minimum: 10
+    default: 10
+
+  spi-cs-inactive-delay-ns:
+    minimum: 300
+    default: 300
+
+  reset-gpios:
+    description:
+      GPIO connected to the active low reset pin.
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 3
+
+  pwm-names:
+    items:
+      - const: di0
+      - const: di1
+      - const: di2
+      - const: di3
+
+  pwms:
+    description:
+      Direct inputs(di0-3) are used to directly turn-on or turn-off the
+      outputs.
+    maxItems: 4
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    description:
+      The external clock can be used if the internal clock doesn't meet
+      timing requirements over temperature and voltage operating range.
+    maxItems: 1
+
+  vdd-supply:
+    description:
+      Logic supply voltage
+
+  vspi-supply:
+    description:
+      Supply voltage for SPI
+
+  vpwr-supply:
+    description:
+      Power switch supply
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+       pwm@0 {
+           compatible = "nxp,mc33xs2410";
+           reg = <0x0>;
+           spi-max-frequency = <4000000>;
+           spi-cpha;
+           spi-cs-setup-delay-ns = <100>;
+           spi-cs-hold-delay-ns = <10>;
+           spi-cs-inactive-delay-ns = <300>;
+           reset-gpios = <&gpio3 22 GPIO_ACTIVE_LOW>;
+           #pwm-cells = <3>;
+           pwm-names = "di0", "di1", "di2", "di3";
+           pwms = <&pwm0 0 1000000>,
+                  <&pwm1 0 1000000>,
+                  <&pwm2 0 1000000>,
+                  <&pwm3 0 1000000>;
+           interrupt-parent = <&gpio0>;
+           interrupts = <31 IRQ_TYPE_LEVEL_LOW>;
+           clocks = <&clk_ext_fixed>;
+           vdd-supply = <&reg_3v3>;
+           vspi-supply = <&reg_3v3>;
+           vpwr-supply = <&reg_24v0>;
+       };
+    };
-- 
2.39.5

[PATCH v8 2/2] pwm: add support for NXPs high-side switch MC33XS2410

[Dimitri Fedrau]

The MC33XS2410 is a four channel high-side switch. Featuring advanced
monitoring and control function, the device is operational from 3.0 V to
60 V. The device is controlled by SPI port for configuration.

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/pwm/Kconfig          |  12 ++
 drivers/pwm/Makefile         |   1 +
 drivers/pwm/pwm-mc33xs2410.c | 378 +++++++++++++++++++++++++++++++++++
 3 files changed, 391 insertions(+)
 create mode 100644 drivers/pwm/pwm-mc33xs2410.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 0915c1e7df16..f513513f9b2f 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -411,6 +411,18 @@ config PWM_LPSS_PLATFORM
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-lpss-platform.
 
+config PWM_MC33XS2410
+	tristate "MC33XS2410 PWM support"
+	depends on OF
+	depends on SPI
+	help
+	  NXP MC33XS2410 high-side switch driver. The MC33XS2410 is a four
+	  channel high-side switch. The device is operational from 3.0 V
+	  to 60 V. The device is controlled by SPI port for configuration.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-mc33xs2410.
+
 config PWM_MESON
 	tristate "Amlogic Meson PWM driver"
 	depends on ARCH_MESON || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9081e0c0e9e0..c75deeeace40 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
 obj-$(CONFIG_PWM_LPSS)		+= pwm-lpss.o
 obj-$(CONFIG_PWM_LPSS_PCI)	+= pwm-lpss-pci.o
 obj-$(CONFIG_PWM_LPSS_PLATFORM)	+= pwm-lpss-platform.o
+obj-$(CONFIG_PWM_MC33XS2410)	+= pwm-mc33xs2410.o
 obj-$(CONFIG_PWM_MESON)		+= pwm-meson.o
 obj-$(CONFIG_PWM_MEDIATEK)	+= pwm-mediatek.o
 obj-$(CONFIG_PWM_MICROCHIP_CORE)	+= pwm-microchip-core.o
diff --git a/drivers/pwm/pwm-mc33xs2410.c b/drivers/pwm/pwm-mc33xs2410.c
new file mode 100644
index 000000000000..432693b4f8eb
--- /dev/null
+++ b/drivers/pwm/pwm-mc33xs2410.c
@@ -0,0 +1,378 @@
+// 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 <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
+/* x in { 1 ... 4 } */
+#define MC33XS2410_PWM_CTRL1_POL_INV(chan)	BIT((chan) + 1)
+
+#define MC33XS2410_PWM_CTRL3			0x07
+/* x in { 1 ... 4 } */
+#define MC33XS2410_PWM_CTRL3_EN(chan)		BIT(4 + (chan) - 1)
+
+/* x in { 1 ... 4 } */
+#define MC33XS2410_PWM_FREQ(chan)		(0x08 + (chan) - 1)
+#define MC33XS2410_PWM_FREQ_STEP		GENMASK(7, 6)
+#define MC33XS2410_PWM_FREQ_COUNT		GENMASK(5, 0)
+
+/* x in { 1 ... 4 } */
+#define MC33XS2410_PWM_DC(chan)			(0x0c + (chan) - 1)
+
+#define MC33XS2410_WDT				0x14
+
+#define MC33XS2410_PWM_MIN_PERIOD		488282
+/* x in { 0 ... 3 } */
+#define MC33XS2410_PWM_MAX_PERIOD(step)		(2000000000 >> (2 * (step)))
+
+#define MC33XS2410_FRAME_IN_ADDR		GENMASK(15, 8)
+#define MC33XS2410_FRAME_IN_DATA		GENMASK(7, 0)
+#define MC33XS2410_FRAME_IN_ADDR_WR		BIT(7)
+#define MC33XS2410_FRAME_IN_DATA_RD		BIT(7)
+#define MC33XS2410_FRAME_OUT_DATA		GENMASK(13, 0)
+
+#define MC33XS2410_MAX_TRANSFERS		5
+
+static int mc33xs2410_write_regs(struct spi_device *spi, u8 *reg, u8 *val,
+				 unsigned int len)
+{
+	u16 tx[MC33XS2410_MAX_TRANSFERS];
+	int i;
+
+	if (len > MC33XS2410_MAX_TRANSFERS)
+		return -EINVAL;
+
+	for (i = 0; i < len; i++)
+		tx[i] = FIELD_PREP(MC33XS2410_FRAME_IN_DATA, val[i]) |
+			FIELD_PREP(MC33XS2410_FRAME_IN_ADDR,
+				   MC33XS2410_FRAME_IN_ADDR_WR | reg[i]);
+
+	return spi_write(spi, tx, len * 2);
+}
+
+static int mc33xs2410_read_regs(struct spi_device *spi, u8 *reg, u8 flag,
+				u16 *val, unsigned int len)
+{
+	u16 tx[MC33XS2410_MAX_TRANSFERS];
+	u16 rx[MC33XS2410_MAX_TRANSFERS];
+	struct spi_transfer t = {
+		.tx_buf = tx,
+		.rx_buf = rx,
+	};
+	int i, ret;
+
+	len++;
+	if (len > MC33XS2410_MAX_TRANSFERS)
+		return -EINVAL;
+
+	t.len = len * 2;
+	for (i = 0; i < len - 1; i++)
+		tx[i] = FIELD_PREP(MC33XS2410_FRAME_IN_DATA, flag) |
+			FIELD_PREP(MC33XS2410_FRAME_IN_ADDR, reg[i]);
+
+	ret = spi_sync_transfer(spi, &t, 1);
+	if (ret < 0)
+		return ret;
+
+	for (i = 1; i < len; i++)
+		val[i - 1] = FIELD_GET(MC33XS2410_FRAME_OUT_DATA, rx[i]);
+
+	return 0;
+}
+
+static int mc33xs2410_write_reg(struct spi_device *spi, u8 reg, u8 val)
+{
+	return mc33xs2410_write_regs(spi, &reg, &val, 1);
+}
+
+static int mc33xs2410_read_reg(struct spi_device *spi, u8 reg, u16 *val, u8 flag)
+{
+	return mc33xs2410_read_regs(spi, &reg, flag, val, 1);
+}
+
+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_modify_reg(struct spi_device *spi, u8 reg, u8 mask, u8 val)
+{
+	u16 tmp;
+	int ret;
+
+	ret = mc33xs2410_read_reg_ctrl(spi, reg, &tmp);
+	if (ret < 0)
+		return ret;
+
+	tmp &= ~mask;
+	tmp |= val & mask;
+
+	return mc33xs2410_write_reg(spi, reg, tmp);
+}
+
+static u8 mc33xs2410_pwm_get_freq(u64 period)
+{
+	u8 step, count;
+
+	/*
+	 * Check which step is appropriate for the given period, starting with
+	 * the highest frequency(lowest period). Higher frequencies are
+	 * represented with better resolution by the device. Therefore favor
+	 * frequency range with the better resolution to minimize error
+	 * introduced by the frequency steps.
+	 */
+
+	switch (period) {
+	case MC33XS2410_PWM_MIN_PERIOD ... MC33XS2410_PWM_MAX_PERIOD(3):
+		step = 3;
+		break;
+	case MC33XS2410_PWM_MAX_PERIOD(3) + 1 ... MC33XS2410_PWM_MAX_PERIOD(2):
+		step = 2;
+		break;
+	case MC33XS2410_PWM_MAX_PERIOD(2) + 1 ... MC33XS2410_PWM_MAX_PERIOD(1):
+		step = 1;
+		break;
+	case MC33XS2410_PWM_MAX_PERIOD(1) + 1 ... MC33XS2410_PWM_MAX_PERIOD(0):
+		step = 0;
+		break;
+	}
+
+	/*
+	 * Round up here because a higher count results in a higher frequency
+	 * and so a smaller period.
+	 */
+	count = DIV_ROUND_UP((u32)MC33XS2410_PWM_MAX_PERIOD(step), (u32)period);
+	return FIELD_PREP(MC33XS2410_PWM_FREQ_STEP, step) |
+	       FIELD_PREP(MC33XS2410_PWM_FREQ_COUNT, count - 1);
+}
+
+static u64 mc33xs2410_pwm_get_period(u8 reg)
+{
+	u32 freq, code, doubled_steps;
+
+	/*
+	 * steps:
+	 *   - 0 = 0.5Hz
+	 *   - 1 = 2Hz
+	 *   - 2 = 8Hz
+	 *   - 3 = 32Hz
+	 * frequency = (code + 1) x steps.
+	 *
+	 * To avoid losing precision in case steps value is zero, scale the
+	 * steps value for now by two and keep it in mind when calculating the
+	 * period that the frequency had been doubled.
+	 */
+	doubled_steps = 1 << (FIELD_GET(MC33XS2410_PWM_FREQ_STEP, reg) * 2);
+	code = FIELD_GET(MC33XS2410_PWM_FREQ_COUNT, reg);
+	freq = (code + 1) * doubled_steps;
+
+	/* Convert frequency to period, considering the doubled frequency. */
+	return DIV_ROUND_UP(2 * NSEC_PER_SEC, freq);
+}
+
+static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+				const struct pwm_state *state)
+{
+	struct spi_device *spi = pwmchip_get_drvdata(chip);
+	u8 reg[4] = {
+			MC33XS2410_PWM_FREQ(pwm->hwpwm + 1),
+			MC33XS2410_PWM_DC(pwm->hwpwm + 1),
+			MC33XS2410_PWM_CTRL1,
+			MC33XS2410_PWM_CTRL3
+		    };
+	u64 period, duty_cycle;
+	int ret, rel_dc;
+	u16 rd_val[2];
+	u8 wr_val[4];
+	u8 mask;
+
+	period = min(state->period, MC33XS2410_PWM_MAX_PERIOD(0));
+	if (period < MC33XS2410_PWM_MIN_PERIOD)
+		return -EINVAL;
+
+	ret = mc33xs2410_read_regs(spi, &reg[2], MC33XS2410_FRAME_IN_DATA_RD, rd_val, 2);
+	if (ret < 0)
+		return ret;
+
+	/* frequency */
+	wr_val[0] = mc33xs2410_pwm_get_freq(period);
+	/* Continue calculations with the possibly truncated period */
+	period = mc33xs2410_pwm_get_period(wr_val[0]);
+
+	/* duty cycle */
+	duty_cycle = min(period, state->duty_cycle);
+	rel_dc = div64_u64(duty_cycle * 256, period) - 1;
+	if (rel_dc < 0)
+		wr_val[1] = 0;
+	else
+		wr_val[1] = rel_dc;
+
+	/* polarity */
+	mask = MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm + 1);
+	wr_val[2] = (state->polarity == PWM_POLARITY_INVERSED) ?
+		    (rd_val[0] | mask) : (rd_val[0] & ~mask);
+
+	/*
+	 * As the hardware cannot generate a 0% relative duty cycle but emits a
+	 * constant low signal when disabled, also disable in the duty_cycle = 0
+	 * case.
+	 */
+	mask = MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm + 1);
+	wr_val[3] = (state->enabled && rel_dc >= 0) ? (rd_val[1] | mask) :
+						      (rd_val[1] & ~mask);
+
+	return mc33xs2410_write_regs(spi, reg, wr_val, 4);
+}
+
+static int mc33xs2410_pwm_get_state(struct pwm_chip *chip,
+				    struct pwm_device *pwm,
+				    struct pwm_state *state)
+{
+	struct spi_device *spi = pwmchip_get_drvdata(chip);
+	u8 reg[4] = {
+			MC33XS2410_PWM_FREQ(pwm->hwpwm + 1),
+			MC33XS2410_PWM_DC(pwm->hwpwm + 1),
+			MC33XS2410_PWM_CTRL1,
+			MC33XS2410_PWM_CTRL3,
+		    };
+	u16 val[4];
+	int ret;
+
+	ret = mc33xs2410_read_regs(spi, reg, MC33XS2410_FRAME_IN_DATA_RD, val,
+				   ARRAY_SIZE(reg));
+	if (ret < 0)
+		return ret;
+
+	state->period = mc33xs2410_pwm_get_period(val[0]);
+	state->polarity = (val[2] & MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm + 1)) ?
+			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+	state->enabled = !!(val[3] & MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm + 1));
+	state->duty_cycle = DIV_ROUND_UP_ULL((val[1] + 1) * state->period, 256);
+
+	return 0;
+}
+
+static const struct pwm_ops mc33xs2410_pwm_ops = {
+	.apply = mc33xs2410_pwm_apply,
+	.get_state = mc33xs2410_pwm_get_state,
+};
+
+static int mc33xs2410_reset(struct device *dev)
+{
+	struct gpio_desc *reset_gpio;
+
+	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR_OR_NULL(reset_gpio))
+		return PTR_ERR_OR_ZERO(reset_gpio);
+
+	fsleep(1000);
+	gpiod_set_value_cansleep(reset_gpio, 0);
+	/* Wake-up time */
+	fsleep(10000);
+
+	return 0;
+}
+
+static int mc33xs2410_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct pwm_chip *chip;
+	int ret;
+
+	chip = devm_pwmchip_alloc(dev, 4, 0);
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	spi->bits_per_word = 16;
+	spi->mode |= SPI_CS_WORD;
+	ret = spi_setup(spi);
+	if (ret < 0)
+		return ret;
+
+	pwmchip_set_drvdata(chip, spi);
+	chip->ops = &mc33xs2410_pwm_ops;
+	ret = mc33xs2410_reset(dev);
+	if (ret)
+		return ret;
+
+	/*
+	 * Disable watchdog and keep in mind that the watchdog won't trigger a
+	 * reset of the machine when running into an timeout, instead the
+	 * control over the outputs is handed over to the INx input logic
+	 * signals of the device. Disabling it here just deactivates this
+	 * feature until a proper solution is found.
+	 */
+	ret = mc33xs2410_write_reg(spi, MC33XS2410_WDT, 0x0);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to disable watchdog\n");
+
+	/* Transition to normal mode */
+	ret = mc33xs2410_modify_reg(spi, MC33XS2410_GLB_CTRL,
+				    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");
+
+	ret = devm_pwmchip_add(dev, chip);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to add pwm chip\n");
+
+	return 0;
+}
+
+static const struct spi_device_id mc33xs2410_spi_id[] = {
+	{ "mc33xs2410" },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, mc33xs2410_spi_id);
+
+static const struct of_device_id mc33xs2410_of_match[] = {
+	{ .compatible = "nxp,mc33xs2410" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mc33xs2410_of_match);
+
+static struct spi_driver mc33xs2410_driver = {
+	.driver = {
+		.name = "mc33xs2410-pwm",
+		.of_match_table = mc33xs2410_of_match,
+	},
+	.probe = mc33xs2410_probe,
+	.id_table = mc33xs2410_spi_id,
+};
+module_spi_driver(mc33xs2410_driver);
+
+MODULE_DESCRIPTION("NXP MC33XS2410 high-side switch driver");
+MODULE_AUTHOR("Dimitri Fedrau <dima.fedrau@gmail.com>");
+MODULE_LICENSE("GPL");
-- 
2.39.5

Re: [PATCH v8 2/2] pwm: add support for NXPs high-side switch MC33XS2410

[Uwe Kleine-König]

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

Hello Dimitri,

On Fri, Jan 10, 2025 at 08:37:55AM +0100, Dimitri Fedrau wrote:
> The MC33XS2410 is a four channel high-side switch. Featuring advanced
> monitoring and control function, the device is operational from 3.0 V to
> 60 V. The device is controlled by SPI port for configuration.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> ---
>  drivers/pwm/Kconfig          |  12 ++
>  drivers/pwm/Makefile         |   1 +
>  drivers/pwm/pwm-mc33xs2410.c | 378 +++++++++++++++++++++++++++++++++++
>  3 files changed, 391 insertions(+)
>  create mode 100644 drivers/pwm/pwm-mc33xs2410.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 0915c1e7df16..f513513f9b2f 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -411,6 +411,18 @@ config PWM_LPSS_PLATFORM
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-lpss-platform.
>  
> +config PWM_MC33XS2410
> +	tristate "MC33XS2410 PWM support"
> +	depends on OF
> +	depends on SPI
> +	help
> +	  NXP MC33XS2410 high-side switch driver. The MC33XS2410 is a four
> +	  channel high-side switch. The device is operational from 3.0 V
> +	  to 60 V. The device is controlled by SPI port for configuration.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-mc33xs2410.
> +
>  config PWM_MESON
>  	tristate "Amlogic Meson PWM driver"
>  	depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9081e0c0e9e0..c75deeeace40 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
>  obj-$(CONFIG_PWM_LPSS)		+= pwm-lpss.o
>  obj-$(CONFIG_PWM_LPSS_PCI)	+= pwm-lpss-pci.o
>  obj-$(CONFIG_PWM_LPSS_PLATFORM)	+= pwm-lpss-platform.o
> +obj-$(CONFIG_PWM_MC33XS2410)	+= pwm-mc33xs2410.o
>  obj-$(CONFIG_PWM_MESON)		+= pwm-meson.o
>  obj-$(CONFIG_PWM_MEDIATEK)	+= pwm-mediatek.o
>  obj-$(CONFIG_PWM_MICROCHIP_CORE)	+= pwm-microchip-core.o
> diff --git a/drivers/pwm/pwm-mc33xs2410.c b/drivers/pwm/pwm-mc33xs2410.c
> new file mode 100644
> index 000000000000..432693b4f8eb
> --- /dev/null
> +++ b/drivers/pwm/pwm-mc33xs2410.c
> @@ -0,0 +1,378 @@
> +// 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 <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
> +/* x in { 1 ... 4 } */

s/x/chan/

> +#define MC33XS2410_PWM_CTRL1_POL_INV(chan)	BIT((chan) + 1)
> +
> +#define MC33XS2410_PWM_CTRL3			0x07
> +/* x in { 1 ... 4 } */
> +#define MC33XS2410_PWM_CTRL3_EN(chan)		BIT(4 + (chan) - 1)
> +
> +/* x in { 1 ... 4 } */
> +#define MC33XS2410_PWM_FREQ(chan)		(0x08 + (chan) - 1)
> +#define MC33XS2410_PWM_FREQ_STEP		GENMASK(7, 6)
> +#define MC33XS2410_PWM_FREQ_COUNT		GENMASK(5, 0)
> +
> +/* x in { 1 ... 4 } */
> +#define MC33XS2410_PWM_DC(chan)			(0x0c + (chan) - 1)
> +
> +#define MC33XS2410_WDT				0x14
> +
> +#define MC33XS2410_PWM_MIN_PERIOD		488282
> +/* x in { 0 ... 3 } */

s/x/step/

> +#define MC33XS2410_PWM_MAX_PERIOD(step)		(2000000000 >> (2 * (step)))
> +
> +#define MC33XS2410_FRAME_IN_ADDR		GENMASK(15, 8)
> +#define MC33XS2410_FRAME_IN_DATA		GENMASK(7, 0)
> +#define MC33XS2410_FRAME_IN_ADDR_WR		BIT(7)
> +#define MC33XS2410_FRAME_IN_DATA_RD		BIT(7)
> +#define MC33XS2410_FRAME_OUT_DATA		GENMASK(13, 0)
> +
> +#define MC33XS2410_MAX_TRANSFERS		5
> +
> +static int mc33xs2410_write_regs(struct spi_device *spi, u8 *reg, u8 *val,
> +				 unsigned int len)
> +{
> +	u16 tx[MC33XS2410_MAX_TRANSFERS];
> +	int i;
> +
> +	if (len > MC33XS2410_MAX_TRANSFERS)
> +		return -EINVAL;
> +
> +	for (i = 0; i < len; i++)
> +		tx[i] = FIELD_PREP(MC33XS2410_FRAME_IN_DATA, val[i]) |
> +			FIELD_PREP(MC33XS2410_FRAME_IN_ADDR,
> +				   MC33XS2410_FRAME_IN_ADDR_WR | reg[i]);
> +
> +	return spi_write(spi, tx, len * 2);
> +}
> +
> +static int mc33xs2410_read_regs(struct spi_device *spi, u8 *reg, u8 flag,
> +				u16 *val, unsigned int len)
> +{
> +	u16 tx[MC33XS2410_MAX_TRANSFERS];
> +	u16 rx[MC33XS2410_MAX_TRANSFERS];
> +	struct spi_transfer t = {
> +		.tx_buf = tx,
> +		.rx_buf = rx,
> +	};
> +	int i, ret;
> +
> +	len++;
> +	if (len > MC33XS2410_MAX_TRANSFERS)
> +		return -EINVAL;
> +
> +	t.len = len * 2;
> +	for (i = 0; i < len - 1; i++)
> +		tx[i] = FIELD_PREP(MC33XS2410_FRAME_IN_DATA, flag) |
> +			FIELD_PREP(MC33XS2410_FRAME_IN_ADDR, reg[i]);
> +
> +	ret = spi_sync_transfer(spi, &t, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 1; i < len; i++)
> +		val[i - 1] = FIELD_GET(MC33XS2410_FRAME_OUT_DATA, rx[i]);
> +
> +	return 0;
> +}
> +
> +static int mc33xs2410_write_reg(struct spi_device *spi, u8 reg, u8 val)
> +{
> +	return mc33xs2410_write_regs(spi, &reg, &val, 1);
> +}
> +
> +static int mc33xs2410_read_reg(struct spi_device *spi, u8 reg, u16 *val, u8 flag)
> +{
> +	return mc33xs2410_read_regs(spi, &reg, flag, val, 1);
> +}
> +
> +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_modify_reg(struct spi_device *spi, u8 reg, u8 mask, u8 val)
> +{
> +	u16 tmp;
> +	int ret;
> +
> +	ret = mc33xs2410_read_reg_ctrl(spi, reg, &tmp);
> +	if (ret < 0)
> +		return ret;
> +
> +	tmp &= ~mask;
> +	tmp |= val & mask;
> +
> +	return mc33xs2410_write_reg(spi, reg, tmp);
> +}
> +
> +static u8 mc33xs2410_pwm_get_freq(u64 period)
> +{
> +	u8 step, count;
> +
> +	/*
> +	 * Check which step is appropriate for the given period, starting with
> +	 * the highest frequency(lowest period). Higher frequencies are
> +	 * represented with better resolution by the device. Therefore favor
> +	 * frequency range with the better resolution to minimize error
> +	 * introduced by the frequency steps.

This is hard to understand as the argument is presented in several
steps:
	algo starts with high step values (that's not in the comment,
		you have to take that from the switch statement)
	high step represents high freq = low period
	high freq yield better resolution
	better resolution is what is favoured.

After investing some time to reunderstand the rational here
I suggest:

	/*
	 * Check which step [0 .. 3] is appropriate for the given
	 * period. The period ranges for the different step values
	 * overlap. Prefer big step values as these allow more
	 * finegrained duty cycle selection.
	 */

> +	 */
> +
> +	switch (period) {
> +	case MC33XS2410_PWM_MIN_PERIOD ... MC33XS2410_PWM_MAX_PERIOD(3):
> +		step = 3;
> +		break;
> +	case MC33XS2410_PWM_MAX_PERIOD(3) + 1 ... MC33XS2410_PWM_MAX_PERIOD(2):
> +		step = 2;
> +		break;
> +	case MC33XS2410_PWM_MAX_PERIOD(2) + 1 ... MC33XS2410_PWM_MAX_PERIOD(1):
> +		step = 1;
> +		break;
> +	case MC33XS2410_PWM_MAX_PERIOD(1) + 1 ... MC33XS2410_PWM_MAX_PERIOD(0):
> +		step = 0;
> +		break;
> +	}
> +
> +	/*
> +	 * Round up here because a higher count results in a higher frequency
> +	 * and so a smaller period.
> +	 */
> +	count = DIV_ROUND_UP((u32)MC33XS2410_PWM_MAX_PERIOD(step), (u32)period);
> +	return FIELD_PREP(MC33XS2410_PWM_FREQ_STEP, step) |
> +	       FIELD_PREP(MC33XS2410_PWM_FREQ_COUNT, count - 1);
> +}
> +
> +static u64 mc33xs2410_pwm_get_period(u8 reg)
> +{
> +	u32 freq, code, doubled_steps;
> +
> +	/*
> +	 * steps:
> +	 *   - 0 = 0.5Hz
> +	 *   - 1 = 2Hz
> +	 *   - 2 = 8Hz
> +	 *   - 3 = 32Hz
> +	 * frequency = (code + 1) x steps.
> +	 *
> +	 * To avoid losing precision in case steps value is zero, scale the
> +	 * steps value for now by two and keep it in mind when calculating the
> +	 * period that the frequency had been doubled.
> +	 */
> +	doubled_steps = 1 << (FIELD_GET(MC33XS2410_PWM_FREQ_STEP, reg) * 2);
> +	code = FIELD_GET(MC33XS2410_PWM_FREQ_COUNT, reg);
> +	freq = (code + 1) * doubled_steps;

doubled_freq?

> +	/* Convert frequency to period, considering the doubled frequency. */
> +	return DIV_ROUND_UP(2 * NSEC_PER_SEC, freq);
> +}
> +
> +static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +				const struct pwm_state *state)
> +{
> +	struct spi_device *spi = pwmchip_get_drvdata(chip);
> +	u8 reg[4] = {
> +			MC33XS2410_PWM_FREQ(pwm->hwpwm + 1),
> +			MC33XS2410_PWM_DC(pwm->hwpwm + 1),
> +			MC33XS2410_PWM_CTRL1,
> +			MC33XS2410_PWM_CTRL3
> +		    };
> +	u64 period, duty_cycle;
> +	int ret, rel_dc;
> +	u16 rd_val[2];
> +	u8 wr_val[4];
> +	u8 mask;
> +
> +	period = min(state->period, MC33XS2410_PWM_MAX_PERIOD(0));
> +	if (period < MC33XS2410_PWM_MIN_PERIOD)
> +		return -EINVAL;
> +
> +	ret = mc33xs2410_read_regs(spi, &reg[2], MC33XS2410_FRAME_IN_DATA_RD, rd_val, 2);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* frequency */
> +	wr_val[0] = mc33xs2410_pwm_get_freq(period);
> +	/* Continue calculations with the possibly truncated period */
> +	period = mc33xs2410_pwm_get_period(wr_val[0]);
> +
> +	/* duty cycle */
> +	duty_cycle = min(period, state->duty_cycle);
> +	rel_dc = div64_u64(duty_cycle * 256, period) - 1;
> +	if (rel_dc < 0)
> +		wr_val[1] = 0;
> +	else
> +		wr_val[1] = rel_dc;
> +
> +	/* polarity */
> +	mask = MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm + 1);
> +	wr_val[2] = (state->polarity == PWM_POLARITY_INVERSED) ?
> +		    (rd_val[0] | mask) : (rd_val[0] & ~mask);
> +
> +	/*
> +	 * As the hardware cannot generate a 0% relative duty cycle but emits a
> +	 * constant low signal when disabled, also disable in the duty_cycle = 0
> +	 * case.
> +	 */
> +	mask = MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm + 1);
> +	wr_val[3] = (state->enabled && rel_dc >= 0) ? (rd_val[1] | mask) :
> +						      (rd_val[1] & ~mask);

This is wrong for inversed polarity I think.

> +	return mc33xs2410_write_regs(spi, reg, wr_val, 4);
> +}
> +
> [..]
> +static int mc33xs2410_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct pwm_chip *chip;
> +	int ret;
> +
> +	chip = devm_pwmchip_alloc(dev, 4, 0);
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	spi->bits_per_word = 16;
> +	spi->mode |= SPI_CS_WORD;
> +	ret = spi_setup(spi);
> +	if (ret < 0)
> +		return ret;
> +
> +	pwmchip_set_drvdata(chip, spi);
> +	chip->ops = &mc33xs2410_pwm_ops;
> +	ret = mc33xs2410_reset(dev);

What does this reset? Does it change the output signal if the bootloader
already setup the hardware? The answer to that has to go into a comment.
(And if it interupts the output, better skip the reset part.)

> +	if (ret)
> +		return ret;

Best regards
Uwe

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

Re: [PATCH v8 2/2] pwm: add support for NXPs high-side switch MC33XS2410

[Dimitri Fedrau]

Hi Uwe,

Am Wed, Mar 19, 2025 at 04:37:43PM +0100 schrieb Uwe Kleine-König:

[...]

> > +#define MC33XS2410_PWM_CTRL1			0x05
> > +/* x in { 1 ... 4 } */
> 
> s/x/chan/
>
Will fix it.

> > +#define MC33XS2410_PWM_CTRL1_POL_INV(chan)	BIT((chan) + 1)
> > +
> > +#define MC33XS2410_PWM_CTRL3			0x07
> > +/* x in { 1 ... 4 } */
> > +#define MC33XS2410_PWM_CTRL3_EN(chan)		BIT(4 + (chan) - 1)
> > +
> > +/* x in { 1 ... 4 } */
> > +#define MC33XS2410_PWM_FREQ(chan)		(0x08 + (chan) - 1)
> > +#define MC33XS2410_PWM_FREQ_STEP		GENMASK(7, 6)
> > +#define MC33XS2410_PWM_FREQ_COUNT		GENMASK(5, 0)
> > +
> > +/* x in { 1 ... 4 } */
> > +#define MC33XS2410_PWM_DC(chan)			(0x0c + (chan) - 1)
> > +
> > +#define MC33XS2410_WDT				0x14
> > +
> > +#define MC33XS2410_PWM_MIN_PERIOD		488282
> > +/* x in { 0 ... 3 } */
> 
> s/x/step/
>
Will fix it.

[...]

> > +static u8 mc33xs2410_pwm_get_freq(u64 period)
> > +{
> > +	u8 step, count;
> > +
> > +	/*
> > +	 * Check which step is appropriate for the given period, starting with
> > +	 * the highest frequency(lowest period). Higher frequencies are
> > +	 * represented with better resolution by the device. Therefore favor
> > +	 * frequency range with the better resolution to minimize error
> > +	 * introduced by the frequency steps.
> 
> This is hard to understand as the argument is presented in several
> steps:
> 	algo starts with high step values (that's not in the comment,
> 		you have to take that from the switch statement)
> 	high step represents high freq = low period
> 	high freq yield better resolution
> 	better resolution is what is favoured.
> 
> After investing some time to reunderstand the rational here
> I suggest:
> 
> 	/*
> 	 * Check which step [0 .. 3] is appropriate for the given
> 	 * period. The period ranges for the different step values
> 	 * overlap. Prefer big step values as these allow more
> 	 * finegrained duty cycle selection.
> 	 */
> 
I think this is way better then before. But did you mean:
Prefer big step values as these allow more finegrained "periods" then
"duty cylces" ?

[...]

> > +
> > +	/*
> > +	 * steps:
> > +	 *   - 0 = 0.5Hz
> > +	 *   - 1 = 2Hz
> > +	 *   - 2 = 8Hz
> > +	 *   - 3 = 32Hz
> > +	 * frequency = (code + 1) x steps.
> > +	 *
> > +	 * To avoid losing precision in case steps value is zero, scale the
> > +	 * steps value for now by two and keep it in mind when calculating the
> > +	 * period that the frequency had been doubled.
> > +	 */
> > +	doubled_steps = 1 << (FIELD_GET(MC33XS2410_PWM_FREQ_STEP, reg) * 2);
> > +	code = FIELD_GET(MC33XS2410_PWM_FREQ_COUNT, reg);
> > +	freq = (code + 1) * doubled_steps;
> 
> doubled_freq?
> 
Will change to doubled_freq.

> > +	/* Convert frequency to period, considering the doubled frequency. */
> > +	return DIV_ROUND_UP(2 * NSEC_PER_SEC, freq);
> > +}
> > +
> > +static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +				const struct pwm_state *state)
> > +{
> > +	struct spi_device *spi = pwmchip_get_drvdata(chip);
> > +	u8 reg[4] = {
> > +			MC33XS2410_PWM_FREQ(pwm->hwpwm + 1),
> > +			MC33XS2410_PWM_DC(pwm->hwpwm + 1),
> > +			MC33XS2410_PWM_CTRL1,
> > +			MC33XS2410_PWM_CTRL3
> > +		    };
> > +	u64 period, duty_cycle;
> > +	int ret, rel_dc;
> > +	u16 rd_val[2];
> > +	u8 wr_val[4];
> > +	u8 mask;
> > +
> > +	period = min(state->period, MC33XS2410_PWM_MAX_PERIOD(0));
> > +	if (period < MC33XS2410_PWM_MIN_PERIOD)
> > +		return -EINVAL;
> > +
> > +	ret = mc33xs2410_read_regs(spi, &reg[2], MC33XS2410_FRAME_IN_DATA_RD, rd_val, 2);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* frequency */
> > +	wr_val[0] = mc33xs2410_pwm_get_freq(period);
> > +	/* Continue calculations with the possibly truncated period */
> > +	period = mc33xs2410_pwm_get_period(wr_val[0]);
> > +
> > +	/* duty cycle */
> > +	duty_cycle = min(period, state->duty_cycle);
> > +	rel_dc = div64_u64(duty_cycle * 256, period) - 1;
> > +	if (rel_dc < 0)
> > +		wr_val[1] = 0;
> > +	else
> > +		wr_val[1] = rel_dc;
> > +
> > +	/* polarity */
> > +	mask = MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm + 1);
> > +	wr_val[2] = (state->polarity == PWM_POLARITY_INVERSED) ?
> > +		    (rd_val[0] | mask) : (rd_val[0] & ~mask);
> > +
> > +	/*
> > +	 * As the hardware cannot generate a 0% relative duty cycle but emits a
> > +	 * constant low signal when disabled, also disable in the duty_cycle = 0
> > +	 * case.
> > +	 */
> > +	mask = MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm + 1);
> > +	wr_val[3] = (state->enabled && rel_dc >= 0) ? (rd_val[1] | mask) :
> > +						      (rd_val[1] & ~mask);
> 
> This is wrong for inversed polarity I think.

Thanks for finding this. Yes, it is for the case when duty_cycle=0 and
polarity=PWM_POLARITY_INVERSED. The device should then output a constant
high signal. This could be done by:
enabled=1
duty_cycle=100%
polarity=normal

Just tested it, it works. What do you think ?

> 
> > +	return mc33xs2410_write_regs(spi, reg, wr_val, 4);
> > +}
> > +
> > [..]
> > +static int mc33xs2410_probe(struct spi_device *spi)
> > +{
> > +	struct device *dev = &spi->dev;
> > +	struct pwm_chip *chip;
> > +	int ret;
> > +
> > +	chip = devm_pwmchip_alloc(dev, 4, 0);
> > +	if (IS_ERR(chip))
> > +		return PTR_ERR(chip);
> > +
> > +	spi->bits_per_word = 16;
> > +	spi->mode |= SPI_CS_WORD;
> > +	ret = spi_setup(spi);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	pwmchip_set_drvdata(chip, spi);
> > +	chip->ops = &mc33xs2410_pwm_ops;
> > +	ret = mc33xs2410_reset(dev);
> 
> What does this reset? Does it change the output signal if the bootloader
> already setup the hardware? The answer to that has to go into a comment.
> (And if it interupts the output, better skip the reset part.)
> 
If reset of the device is asserted on probe it deasserts the reset and
makes the device available. On the other hand if it is already setup by
the bootloader the reset will clear all registers to default values which
means that the output signal will change. The reset gpio is optional, so
the user can decide wether to use it or not. Both usecases are
supported. I will add an comment, but will keep the reset handling.

Best regards,
Dimitri Fedrau

Re: [PATCH v8 2/2] pwm: add support for NXPs high-side switch MC33XS2410

[Uwe Kleine-König]

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

Hello Dimitri,

On Thu, Mar 20, 2025 at 07:51:18PM +0100, Dimitri Fedrau wrote:
> Am Wed, Mar 19, 2025 at 04:37:43PM +0100 schrieb Uwe Kleine-König:

funny denglisch. :-)

> > After investing some time to reunderstand the rational here
> > I suggest:
> > 
> > 	/*
> > 	 * Check which step [0 .. 3] is appropriate for the given
> > 	 * period. The period ranges for the different step values
> > 	 * overlap. Prefer big step values as these allow more
> > 	 * finegrained duty cycle selection.
> > 	 */
> 
> I think this is way better then before. But did you mean:
> Prefer big step values as these allow more finegrained "periods" then
> "duty cylces" ?

It's both. I wrote without considering period because (in my bubble)
most consumers tend to care more for finegrained duty cycle than exact
period. Feel free to add "period and" to my proposal. (Assuming you
agree that the result is correct then.)

> > > +	/*
> > > +	 * As the hardware cannot generate a 0% relative duty cycle but emits a
> > > +	 * constant low signal when disabled, also disable in the duty_cycle = 0
> > > +	 * case.
> > > +	 */
> > > +	mask = MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm + 1);
> > > +	wr_val[3] = (state->enabled && rel_dc >= 0) ? (rd_val[1] | mask) :
> > > +						      (rd_val[1] & ~mask);
> > 
> > This is wrong for inversed polarity I think.
> 
> Thanks for finding this. Yes, it is for the case when duty_cycle=0 and
> polarity=PWM_POLARITY_INVERSED. The device should then output a constant
> high signal. This could be done by:
> enabled=1
> duty_cycle=100%
> polarity=normal
> 
> Just tested it, it works. What do you think ?

Sounds like the right thing to do. Probably needs a comment as this
is an asymmetry between the two polarities.

> > > +	pwmchip_set_drvdata(chip, spi);
> > > +	chip->ops = &mc33xs2410_pwm_ops;
> > > +	ret = mc33xs2410_reset(dev);
> > 
> > What does this reset? Does it change the output signal if the bootloader
> > already setup the hardware? The answer to that has to go into a comment.
> > (And if it interupts the output, better skip the reset part.)
> > 
> If reset of the device is asserted on probe it deasserts the reset and
> makes the device available. On the other hand if it is already setup by
> the bootloader the reset will clear all registers to default values which
> means that the output signal will change. The reset gpio is optional, so
> the user can decide wether to use it or not. Both usecases are
> supported. I will add an comment, but will keep the reset handling.

Conceptually it's wrong to make the distinction between "A reset at
probe time is desired" and "Please no reset at probe" via a modification
in the device tree as this is supposed to be an application agnostic
hardware description. I have no nice and constructive suggestion here. I
think I'd go for only deasserting reset in the Linux driver and
otherwise rely on the bootloader to pass the device in a usable state.
With a sane hardware design not doing anything is a valid possibility
for the bootloader then.

Best regards
Uwe

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