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

[PATCH v5 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

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                  | 419 ++++++++++++++++++
 4 files changed, 550 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/nxp,mc33xs2410.yaml
 create mode 100644 drivers/pwm/pwm-mc33xs2410.c

-- 
2.39.2

[PATCH v5 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.2

[PATCH v5 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 | 419 +++++++++++++++++++++++++++++++++++
 3 files changed, 432 insertions(+)
 create mode 100644 drivers/pwm/pwm-mc33xs2410.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 1dd7921194f5..1e873a19a1cf 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -380,6 +380,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 90913519f11a..b9b202f7fe7e 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -33,6 +33,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..63e6a48b0d02
--- /dev/null
+++ b/drivers/pwm/pwm-mc33xs2410.c
@@ -0,0 +1,419 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2024 Liebherr-Electronics and Drives GmbH
+ *
+ * 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_MASK	GENMASK(7, 6)
+#define MC33XS2410_GLB_CTRL_NORMAL_MODE	BIT(6)
+#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))
+#define MC33XS2410_WDT			0x14
+
+#define MC33XS2410_WR			BIT(7)
+#define MC33XS2410_RD_CTRL		BIT(7)
+#define MC33XS2410_RD_DATA_MASK		GENMASK(13, 0)
+
+#define MC33XS2410_MIN_PERIOD_STEP0	31250000
+#define MC33XS2410_MAX_PERIOD_STEP0	2000000000
+/* x in { 0 ... 3 } */
+#define MC33XS2410_MIN_PERIOD_STEP(x)	(MC33XS2410_MIN_PERIOD_STEP0 >> (2 * x))
+/* x in { 0 ... 3 } */
+#define MC33XS2410_MAX_PERIOD_STEP(x)	(MC33XS2410_MAX_PERIOD_STEP0 >> (2 * x))
+
+#define MC33XS2410_MAX_TRANSFERS	5
+#define MC33XS2410_WORD_LEN		2
+
+struct mc33xs2410_pwm {
+	struct spi_device *spi;
+};
+
+static
+inline struct mc33xs2410_pwm *to_pwm_mc33xs2410_chip(struct pwm_chip *chip)
+{
+	return pwmchip_get_drvdata(chip);
+}
+
+static int mc33xs2410_xfer_regs(struct spi_device *spi, bool read, u8 *reg,
+				u16 *val, bool *ctrl, int len)
+{
+	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;
+			tx[val_i] = val[i];
+			t[i].tx_buf = &tx[reg_i];
+		}
+
+		t[i].len = MC33XS2410_WORD_LEN;
+		t[i].cs_change = 1;
+	}
+
+	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]));
+		}
+	}
+
+	return 0;
+}
+
+static
+int mc33xs2410_write_regs(struct spi_device *spi, u8 *reg, u16 *val, int len)
+{
+
+	return mc33xs2410_xfer_regs(spi, false, reg, val, NULL, len);
+}
+
+static int mc33xs2410_read_regs(struct spi_device *spi, u8 *reg, bool *ctrl,
+				u16 *val, u8 len)
+{
+	return mc33xs2410_xfer_regs(spi, true, reg, val, ctrl, len);
+}
+
+
+static int mc33xs2410_write_reg(struct spi_device *spi, u8 reg, u16 val)
+{
+	return mc33xs2410_write_regs(spi, &reg, &val, 1);
+}
+
+static
+int mc33xs2410_read_reg(struct spi_device *spi, u8 reg, u16 *val, bool ctrl)
+{
+	return mc33xs2410_read_regs(spi, &reg, &ctrl, val, 1);
+}
+
+static int mc33xs2410_read_reg_ctrl(struct spi_device *spi, u8 reg, u16 *val)
+{
+	return mc33xs2410_read_reg(spi, reg, val, true);
+}
+
+static
+int mc33xs2410_modify_reg(struct spi_device *spi, u8 reg, u16 mask, u16 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_MIN_PERIOD_STEP(3) + 1 ... MC33XS2410_MAX_PERIOD_STEP(3):
+		step = 3;
+		break;
+	case MC33XS2410_MAX_PERIOD_STEP(3) + 1 ... MC33XS2410_MAX_PERIOD_STEP(2):
+		step = 2;
+		break;
+	case MC33XS2410_MAX_PERIOD_STEP(2) + 1 ... MC33XS2410_MAX_PERIOD_STEP(1):
+		step = 1;
+		break;
+	case MC33XS2410_MAX_PERIOD_STEP(1) + 1 ... MC33XS2410_MAX_PERIOD_STEP(0):
+		step = 0;
+		break;
+	}
+
+	count = DIV_ROUND_UP((u32)MC33XS2410_MAX_PERIOD_STEP(step), (u32)period);
+	return FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step) |
+	       FIELD_PREP(MC33XS2410_PWM_FREQ_COUNT_MASK, count - 1);
+}
+
+static u64 mc33xs2410_pwm_get_period(u8 reg)
+{
+	u32 freq, code, steps;
+
+	/*
+	 * steps:
+	 *   - 0 = 0.5Hz
+	 *   - 1 = 2Hz
+	 *   - 2 = 8Hz
+	 *   - 3 = 32Hz
+	 * frequency = (code + 1) x steps.
+	 *
+	 * To avoid division in case steps value is zero we scale the steps
+	 * value for now by two and keep it in mind when calculating the period
+	 * that we have doubled the frequency.
+	 */
+	steps = 1 << (FIELD_GET(MC33XS2410_PWM_FREQ_STEP_MASK, reg) * 2);
+	code = FIELD_GET(MC33XS2410_PWM_FREQ_COUNT_MASK, reg);
+	freq = (code + 1) * steps;
+
+	/* Convert frequency to period, considering the doubled frequency. */
+	return DIV_ROUND_UP((u32)(2 * NSEC_PER_SEC), freq);
+}
+
+static int mc33xs2410_pwm_get_relative_duty_cycle(u64 period, u64 duty_cycle)
+{
+	/*
+	 * duty_cycle cannot overflow and period is not zero, since this is
+	 * guaranteed by the caller.
+	 */
+	duty_cycle *= 256;
+	duty_cycle = div64_u64(duty_cycle, period);
+
+	return duty_cycle - 1;
+}
+
+static void mc33xs2410_pwm_set_relative_duty_cycle(struct pwm_state *state,
+						   u16 duty_cycle)
+{
+	if (!duty_cycle && !state->enabled)
+		state->duty_cycle = 0;
+	else
+		state->duty_cycle = DIV_ROUND_UP_ULL((u64)(duty_cycle + 1) * state->period, 256);
+}
+
+static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+				const struct pwm_state *state)
+{
+	struct mc33xs2410_pwm *mc33xs2410 = to_pwm_mc33xs2410_chip(chip);
+	struct spi_device *spi = mc33xs2410->spi;
+	u8 reg[4] = {
+			MC33XS2410_PWM_FREQ(pwm->hwpwm + 1),
+			MC33XS2410_PWM_DC(pwm->hwpwm + 1),
+			MC33XS2410_PWM_CTRL1,
+			MC33XS2410_PWM_CTRL3
+		    };
+	bool ctrl[2] = { true, true };
+	u64 period, duty_cycle;
+	int ret, rel_dc;
+	u16 val[4];
+	u8 mask;
+
+	period = min(state->period, MC33XS2410_MAX_PERIOD_STEP(0));
+	if (period < MC33XS2410_MIN_PERIOD_STEP(3) + 1)
+		return -EINVAL;
+
+	ret = mc33xs2410_read_regs(spi, &reg[2], &ctrl[0], &val[2], 2);
+	if (ret < 0)
+		return ret;
+
+	/* 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);
+	rel_dc = mc33xs2410_pwm_get_relative_duty_cycle(period, duty_cycle);
+	val[1] = rel_dc < 0 ? 0 : rel_dc;
+
+	/* 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)
+{
+	struct mc33xs2410_pwm *mc33xs2410 = to_pwm_mc33xs2410_chip(chip);
+	struct spi_device *spi = mc33xs2410->spi;
+	u8 reg[4] = {
+			MC33XS2410_PWM_FREQ(pwm->hwpwm + 1),
+			MC33XS2410_PWM_DC(pwm->hwpwm + 1),
+			MC33XS2410_PWM_CTRL1,
+			MC33XS2410_PWM_CTRL3,
+		    };
+	bool ctrl[4] = { true, true, true, true };
+	u16 val[4];
+	int ret;
+
+	ret = mc33xs2410_read_regs(spi, reg, ctrl, val, 4);
+	if (ret < 0)
+		return ret;
+
+	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]);
+	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 mc33xs2410_pwm *mc33xs2410;
+	struct device *dev = &spi->dev;
+	struct pwm_chip *chip;
+	int ret;
+
+	chip = devm_pwmchip_alloc(dev, 4, sizeof(*mc33xs2410));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	mc33xs2410 = to_pwm_mc33xs2410_chip(chip);
+	mc33xs2410->spi = 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_MASK,
+				    MC33XS2410_GLB_CTRL_NORMAL_MODE);
+	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.2

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

[Uwe Kleine-König]

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

Hello,

On Fri, Aug 02, 2024 at 05:44:08PM +0200, 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 | 419 +++++++++++++++++++++++++++++++++++
>  3 files changed, 432 insertions(+)
>  create mode 100644 drivers/pwm/pwm-mc33xs2410.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 1dd7921194f5..1e873a19a1cf 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -380,6 +380,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 90913519f11a..b9b202f7fe7e 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -33,6 +33,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..63e6a48b0d02
> --- /dev/null
> +++ b/drivers/pwm/pwm-mc33xs2410.c
> @@ -0,0 +1,419 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024 Liebherr-Electronics and Drives GmbH
> + *
Please add a link to the manual here. I found
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_MASK	GENMASK(7, 6)
> +#define MC33XS2410_GLB_CTRL_NORMAL_MODE	BIT(6)

I would have defined these as:

#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))
> +#define MC33XS2410_WDT			0x14
> +
> +#define MC33XS2410_WR			BIT(7)
> +#define MC33XS2410_RD_CTRL		BIT(7)
> +#define MC33XS2410_RD_DATA_MASK		GENMASK(13, 0)
> +
> +#define MC33XS2410_MIN_PERIOD_STEP0	31250000
> +#define MC33XS2410_MAX_PERIOD_STEP0	2000000000
> +/* x in { 0 ... 3 } */
> +#define MC33XS2410_MIN_PERIOD_STEP(x)	(MC33XS2410_MIN_PERIOD_STEP0 >> (2 * x))
> +/* x in { 0 ... 3 } */
> +#define MC33XS2410_MAX_PERIOD_STEP(x)	(MC33XS2410_MAX_PERIOD_STEP0 >> (2 * x))

So 
MC33XS2410_MIN_PERIOD_STEP(3) = 31250000 >> 6 which is mathematically
488281.25. I haven't thought deeply about it, but I wonder if that .25
is relevant in the calculation of the step to select.

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

personally I'd prefer to call this mc33xs2410_from_chip() or something
similar to have it use the same prefix as the other functions. But given
there is some inconsistency and other people feel strong here and
(rightly) claim this type of function is often called "to_*", I won't
insist.

> +{
> +	return pwmchip_get_drvdata(chip);
> +}
> [...]
> +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_MIN_PERIOD_STEP(3) + 1 ... MC33XS2410_MAX_PERIOD_STEP(3):
> +		step = 3;
> +		break;
> +	case MC33XS2410_MAX_PERIOD_STEP(3) + 1 ... MC33XS2410_MAX_PERIOD_STEP(2):
> +		step = 2;
> +		break;
> +	case MC33XS2410_MAX_PERIOD_STEP(2) + 1 ... MC33XS2410_MAX_PERIOD_STEP(1):
> +		step = 1;
> +		break;
> +	case MC33XS2410_MAX_PERIOD_STEP(1) + 1 ... MC33XS2410_MAX_PERIOD_STEP(0):
> +		step = 0;
> +		break;
> +	}
> +
> +	count = DIV_ROUND_UP((u32)MC33XS2410_MAX_PERIOD_STEP(step), (u32)period);

It took me a while to verify that DIV_ROUND_UP is right here. The
reasoning is that a higher count results in a higher frequency and so a
smaller period.

> +	return FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step) |
> +	       FIELD_PREP(MC33XS2410_PWM_FREQ_COUNT_MASK, count - 1);
> +}
> +
> +static u64 mc33xs2410_pwm_get_period(u8 reg)
> +{
> +	u32 freq, code, steps;
> +
> +	/*
> +	 * steps:
> +	 *   - 0 = 0.5Hz
> +	 *   - 1 = 2Hz
> +	 *   - 2 = 8Hz
> +	 *   - 3 = 32Hz
> +	 * frequency = (code + 1) x steps.
> +	 *
> +	 * To avoid division in case steps value is zero we scale the steps

Technically you don't avoid a division, but "only" avoid loosing
precision in case you have to do (integer) division by 0.5.

> +	 * value for now by two and keep it in mind when calculating the period
> +	 * that we have doubled the frequency.

Maybe reflect that doubling in the variable naming? "doubled_steps"?

> +	 */
> +	steps = 1 << (FIELD_GET(MC33XS2410_PWM_FREQ_STEP_MASK, reg) * 2);
> +	code = FIELD_GET(MC33XS2410_PWM_FREQ_COUNT_MASK, reg);
> +	freq = (code + 1) * steps;
> +
> +	/* Convert frequency to period, considering the doubled frequency. */
> +	return DIV_ROUND_UP((u32)(2 * NSEC_PER_SEC), freq);
> +}
> +
> +static int mc33xs2410_pwm_get_relative_duty_cycle(u64 period, u64 duty_cycle)
> +{
> +	/*
> +	 * duty_cycle cannot overflow and period is not zero, since this is
> +	 * guaranteed by the caller.
> +	 */
> +	duty_cycle *= 256;
> +	duty_cycle = div64_u64(duty_cycle, period);
> +
> +	return duty_cycle - 1;
> +}
> +
> +static void mc33xs2410_pwm_set_relative_duty_cycle(struct pwm_state *state,
> +						   u16 duty_cycle)
> +{
> +	if (!duty_cycle && !state->enabled)
> +		state->duty_cycle = 0;
> +	else
> +		state->duty_cycle = DIV_ROUND_UP_ULL((u64)(duty_cycle + 1) * state->period, 256);

Why does !duty_cycle matter in the if condition. I would have expected

	if (!state->enabled)
		state->duty_cycle = 0;
	else
		state->duty_cycle = DIV_ROUND_UP_ULL....)

That cast to (u64) in the last line can be dropped.

> +}
> +
> +static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +				const struct pwm_state *state)
> +{
> +	struct mc33xs2410_pwm *mc33xs2410 = to_pwm_mc33xs2410_chip(chip);
> +	struct spi_device *spi = mc33xs2410->spi;
> +	u8 reg[4] = {
> +			MC33XS2410_PWM_FREQ(pwm->hwpwm + 1),
> +			MC33XS2410_PWM_DC(pwm->hwpwm + 1),
> +			MC33XS2410_PWM_CTRL1,
> +			MC33XS2410_PWM_CTRL3
> +		    };
> +	bool ctrl[2] = { true, true };
> +	u64 period, duty_cycle;
> +	int ret, rel_dc;
> +	u16 val[4];
> +	u8 mask;
> +
> +	period = min(state->period, MC33XS2410_MAX_PERIOD_STEP(0));
> +	if (period < MC33XS2410_MIN_PERIOD_STEP(3) + 1)
> +		return -EINVAL;

That is only right because in the expression for
MC33XS2410_MIN_PERIOD_STEP(3) the shift results in a one being shifted
out. If there were only zeros, the right check would be

	if (period < MC33XS2410_MIN_PERIOD_STEP(3))

. That's a bit unfortunate because it's unintuitive and at first sight
I'd expect that MC33XS2410_MIN_PERIOD_STEP(3) is a possible period.

Hmm, you could only fix that by doing scaled math or a good code
comment.

> +	ret = mc33xs2410_read_regs(spi, &reg[2], &ctrl[0], &val[2], 2);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* 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);
> +	rel_dc = mc33xs2410_pwm_get_relative_duty_cycle(period, duty_cycle);
> +	val[1] = rel_dc < 0 ? 0 : rel_dc;
> +
> +	/* 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);
> +}

Best regards
Uwe

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

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

[Dimitri Fedrau]

Hi Uwe,

Am Tue, Sep 10, 2024 at 11:24:42AM +0200 schrieb Uwe Kleine-König:
> Hello,
> 
> On Fri, Aug 02, 2024 at 05:44:08PM +0200, 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 | 419 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 432 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-mc33xs2410.c
> > 
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 1dd7921194f5..1e873a19a1cf 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -380,6 +380,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 90913519f11a..b9b202f7fe7e 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -33,6 +33,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..63e6a48b0d02
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-mc33xs2410.c
> > @@ -0,0 +1,419 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2024 Liebherr-Electronics and Drives GmbH
> > + *
> Please add a link to the manual here. I found
> https://www.nxp.com/docs/en/data-sheet/MC33XS2410.pdf.
>
Sure, will add the link.

> > + * 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_MASK	GENMASK(7, 6)
> > +#define MC33XS2410_GLB_CTRL_NORMAL_MODE	BIT(6)
> 
> I would have defined these as:
> 
> #define MC33XS2410_GLB_CTRL_MODE		GENMASK(7, 6)
> #define MC33XS2410_GLB_CTRL_MODE_NORMAL		FIELD_PREP(MC33XS2410_GLB_CTRL_MODE, 1)
>
Will fix it in V6.

> > +#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))
> > +#define MC33XS2410_WDT			0x14
> > +
> > +#define MC33XS2410_WR			BIT(7)
> > +#define MC33XS2410_RD_CTRL		BIT(7)
> > +#define MC33XS2410_RD_DATA_MASK		GENMASK(13, 0)
> > +
> > +#define MC33XS2410_MIN_PERIOD_STEP0	31250000
> > +#define MC33XS2410_MAX_PERIOD_STEP0	2000000000
> > +/* x in { 0 ... 3 } */
> > +#define MC33XS2410_MIN_PERIOD_STEP(x)	(MC33XS2410_MIN_PERIOD_STEP0 >> (2 * x))
> > +/* x in { 0 ... 3 } */
> > +#define MC33XS2410_MAX_PERIOD_STEP(x)	(MC33XS2410_MAX_PERIOD_STEP0 >> (2 * x))
> 
> So 
> MC33XS2410_MIN_PERIOD_STEP(3) = 31250000 >> 6 which is mathematically
> 488281.25. I haven't thought deeply about it, but I wonder if that .25
> is relevant in the calculation of the step to select.
>
It is relevant and used in mc33xs2410_pwm_get_freq to select the step
and in mc33xs2410_pwm_apply to check if the period is in range. As a
workaround I add +1 to make sure that the period is bigger then 488281.

I could get rid of the MC33XS2410_MIN_PERIOD_STEP define as it is used
only twice and both times with "MC33XS2410_MIN_PERIOD_STEP(3)" and
instead define "MC33XS2410_MIN_PERIOD	488282".

> > +
> > +#define MC33XS2410_MAX_TRANSFERS	5
> > +#define MC33XS2410_WORD_LEN		2
> > +
> > +struct mc33xs2410_pwm {
> > +	struct spi_device *spi;
> > +};
> > +
> > +static
> > +inline struct mc33xs2410_pwm *to_pwm_mc33xs2410_chip(struct pwm_chip *chip)
> 
> personally I'd prefer to call this mc33xs2410_from_chip() or something
> similar to have it use the same prefix as the other functions. But given
> there is some inconsistency and other people feel strong here and
> (rightly) claim this type of function is often called "to_*", I won't
> insist.
> 
I don't have a preference on this, renaming wouldn't be a big problem.

> > +{
> > +	return pwmchip_get_drvdata(chip);
> > +}
> > [...]
> > +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_MIN_PERIOD_STEP(3) + 1 ... MC33XS2410_MAX_PERIOD_STEP(3):
> > +		step = 3;
> > +		break;
> > +	case MC33XS2410_MAX_PERIOD_STEP(3) + 1 ... MC33XS2410_MAX_PERIOD_STEP(2):
> > +		step = 2;
> > +		break;
> > +	case MC33XS2410_MAX_PERIOD_STEP(2) + 1 ... MC33XS2410_MAX_PERIOD_STEP(1):
> > +		step = 1;
> > +		break;
> > +	case MC33XS2410_MAX_PERIOD_STEP(1) + 1 ... MC33XS2410_MAX_PERIOD_STEP(0):
> > +		step = 0;
> > +		break;
> > +	}
> > +
> > +	count = DIV_ROUND_UP((u32)MC33XS2410_MAX_PERIOD_STEP(step), (u32)period);
> 
> It took me a while to verify that DIV_ROUND_UP is right here. The
> reasoning is that a higher count results in a higher frequency and so a
> smaller period.
> 
I could add a comment.

> > +	return FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step) |
> > +	       FIELD_PREP(MC33XS2410_PWM_FREQ_COUNT_MASK, count - 1);
> > +}
> > +
> > +static u64 mc33xs2410_pwm_get_period(u8 reg)
> > +{
> > +	u32 freq, code, steps;
> > +
> > +	/*
> > +	 * steps:
> > +	 *   - 0 = 0.5Hz
> > +	 *   - 1 = 2Hz
> > +	 *   - 2 = 8Hz
> > +	 *   - 3 = 32Hz
> > +	 * frequency = (code + 1) x steps.
> > +	 *
> > +	 * To avoid division in case steps value is zero we scale the steps
> 
> Technically you don't avoid a division, but "only" avoid loosing
> precision in case you have to do (integer) division by 0.5.
> 
Yes, will fix the comment.

> > +	 * value for now by two and keep it in mind when calculating the period
> > +	 * that we have doubled the frequency.
> 
> Maybe reflect that doubling in the variable naming? "doubled_steps"?
> 
Ok.

> > +	 */
> > +	steps = 1 << (FIELD_GET(MC33XS2410_PWM_FREQ_STEP_MASK, reg) * 2);
> > +	code = FIELD_GET(MC33XS2410_PWM_FREQ_COUNT_MASK, reg);
> > +	freq = (code + 1) * steps;
> > +
> > +	/* Convert frequency to period, considering the doubled frequency. */
> > +	return DIV_ROUND_UP((u32)(2 * NSEC_PER_SEC), freq);
> > +}
> > +
> > +static int mc33xs2410_pwm_get_relative_duty_cycle(u64 period, u64 duty_cycle)
> > +{
> > +	/*
> > +	 * duty_cycle cannot overflow and period is not zero, since this is
> > +	 * guaranteed by the caller.
> > +	 */
> > +	duty_cycle *= 256;
> > +	duty_cycle = div64_u64(duty_cycle, period);
> > +
> > +	return duty_cycle - 1;
> > +}
> > +
> > +static void mc33xs2410_pwm_set_relative_duty_cycle(struct pwm_state *state,
> > +						   u16 duty_cycle)
> > +{
> > +	if (!duty_cycle && !state->enabled)
> > +		state->duty_cycle = 0;
> > +	else
> > +		state->duty_cycle = DIV_ROUND_UP_ULL((u64)(duty_cycle + 1) * state->period, 256);
> 
> Why does !duty_cycle matter in the if condition. I would have expected
> 
> 	if (!state->enabled)
> 		state->duty_cycle = 0;
> 	else
> 		state->duty_cycle = DIV_ROUND_UP_ULL....)
> 
I think you are right, just wanted to keep the duty_cycle information when
disabling the output by "echo 0 > enable". Will test this with PWM_DEBUG
and fix it if there aren't any complaints.

> That cast to (u64) in the last line can be dropped.
> 
Ok.

> > +}
> > +
> > +static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +				const struct pwm_state *state)
> > +{
> > +	struct mc33xs2410_pwm *mc33xs2410 = to_pwm_mc33xs2410_chip(chip);
> > +	struct spi_device *spi = mc33xs2410->spi;
> > +	u8 reg[4] = {
> > +			MC33XS2410_PWM_FREQ(pwm->hwpwm + 1),
> > +			MC33XS2410_PWM_DC(pwm->hwpwm + 1),
> > +			MC33XS2410_PWM_CTRL1,
> > +			MC33XS2410_PWM_CTRL3
> > +		    };
> > +	bool ctrl[2] = { true, true };
> > +	u64 period, duty_cycle;
> > +	int ret, rel_dc;
> > +	u16 val[4];
> > +	u8 mask;
> > +
> > +	period = min(state->period, MC33XS2410_MAX_PERIOD_STEP(0));
> > +	if (period < MC33XS2410_MIN_PERIOD_STEP(3) + 1)
> > +		return -EINVAL;
> 
> That is only right because in the expression for
> MC33XS2410_MIN_PERIOD_STEP(3) the shift results in a one being shifted
> out. If there were only zeros, the right check would be
> 
> 	if (period < MC33XS2410_MIN_PERIOD_STEP(3))
> 
> . That's a bit unfortunate because it's unintuitive and at first sight
> I'd expect that MC33XS2410_MIN_PERIOD_STEP(3) is a possible period.
> 
> Hmm, you could only fix that by doing scaled math or a good code
> comment.
>
Thanks for pointing out. See my comment above regarding define
MC33XS2410_MIN_PERIOD_STEP. Introducing MC33XS2410_MIN_PERIOD and
removing MC33XS2410_MIN_PERIOD_STEP define should fix this.

Best regards

Dimitri