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

[PATCH v3 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 !

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

-- 
2.39.2

[PATCH v3 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 v3 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 | 410 +++++++++++++++++++++++++++++++++++
 3 files changed, 423 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..1904d1ee0652
--- /dev/null
+++ b/drivers/pwm/pwm-mc33xs2410.c
@@ -0,0 +1,410 @@
+// 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/delay.h>
+#include <linux/err.h>
+#include <linux/math.h>
+#include <linux/math64.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/mutex.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 if period is within the limits of each of the four frequency
+	 * ranges, 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(MC33XS2410_MAX_PERIOD_STEP(step), period) - 1;
+
+	return FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step) |
+	       FIELD_PREP(MC33XS2410_PWM_FREQ_COUNT_MASK, count);
+}
+
+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(2 * NSEC_PER_SEC, freq);
+}
+
+static int mc33xs2410_pwm_get_relative_duty_cycle(u64 period, u64 duty_cycle)
+{
+	if (!period)
+		return 0;
+
+	duty_cycle *= 256;
+	duty_cycle = DIV_ROUND_CLOSEST_ULL(duty_cycle, period);
+
+	/* Device is not able to generate 0% duty cycle */
+	if (!duty_cycle)
+		return -ERANGE;
+
+	return duty_cycle - 1;
+}
+
+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;
+	u16 val[4];
+	u8 mask;
+	int ret;
+
+	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);
+
+	/* duty cycle */
+	duty_cycle = min(period, state->duty_cycle);
+	ret = mc33xs2410_pwm_get_relative_duty_cycle(period, duty_cycle);
+	if (ret < 0)
+		return ret;
+
+	val[1] = ret;
+	/* 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) ? (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]);
+	pwm_set_relative_duty_cycle(state, val[1] + 1, 256);
+	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));
+
+	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 */
+	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 v3 2/2] pwm: add support for NXPs high-side switch MC33XS2410

[kernel test robot]

Hi Dimitri,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.9 next-20240516]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Dimitri-Fedrau/dt-bindings-pwm-add-support-for-MC33XS2410/20240515-192237
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240515112034.298116-3-dima.fedrau%40gmail.com
patch subject: [PATCH v3 2/2] pwm: add support for NXPs high-side switch MC33XS2410
config: openrisc-allmodconfig (https://download.01.org/0day-ci/archive/20240516/202405162306.aFLe0sSZ-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240516/202405162306.aFLe0sSZ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405162306.aFLe0sSZ-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/pwm/pwm-mc33xs2410.c: In function 'mc33xs2410_xfer_regs':
>> drivers/pwm/pwm-mc33xs2410.c:123:34: error: implicit declaration of function 'FIELD_GET' [-Werror=implicit-function-declaration]
     123 |                         val[i] = FIELD_GET(MC33XS2410_RD_DATA_MASK,
         |                                  ^~~~~~~~~
   drivers/pwm/pwm-mc33xs2410.c: In function 'mc33xs2410_pwm_get_freq':
>> drivers/pwm/pwm-mc33xs2410.c:206:16: error: implicit declaration of function 'FIELD_PREP' [-Werror=implicit-function-declaration]
     206 |         return FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step) |
         |                ^~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/FIELD_GET +123 drivers/pwm/pwm-mc33xs2410.c

    74	
    75	static int mc33xs2410_xfer_regs(struct spi_device *spi, bool read, u8 *reg,
    76					u16 *val, bool *ctrl, int len)
    77	{
    78		struct spi_transfer t[MC33XS2410_MAX_TRANSFERS] = { { 0 } };
    79		u8 tx[MC33XS2410_MAX_TRANSFERS * MC33XS2410_WORD_LEN];
    80		u8 rx[MC33XS2410_MAX_TRANSFERS * MC33XS2410_WORD_LEN];
    81		int i, ret, reg_i, val_i;
    82	
    83		if (!len)
    84			return 0;
    85	
    86		if (read)
    87			len++;
    88	
    89		if (len > MC33XS2410_MAX_TRANSFERS)
    90			return -EINVAL;
    91	
    92		for (i = 0; i < len; i++) {
    93			reg_i = i * MC33XS2410_WORD_LEN;
    94			val_i = reg_i + 1;
    95			if (read) {
    96				if (i < len - 1) {
    97					tx[reg_i] = reg[i];
    98					tx[val_i] = ctrl[i] ? MC33XS2410_RD_CTRL : 0;
    99					t[i].tx_buf = &tx[reg_i];
   100				}
   101	
   102				if (i > 0)
   103					t[i].rx_buf = &rx[reg_i - MC33XS2410_WORD_LEN];
   104			} else {
   105				tx[reg_i] = reg[i] | MC33XS2410_WR;
   106				tx[val_i] = val[i];
   107				t[i].tx_buf = &tx[reg_i];
   108			}
   109	
   110			t[i].len = MC33XS2410_WORD_LEN;
   111			t[i].cs_change = 1;
   112		}
   113	
   114		t[len - 1].cs_change = 0;
   115	
   116		ret = spi_sync_transfer(spi, &t[0], len);
   117		if (ret < 0)
   118			return ret;
   119	
   120		if (read) {
   121			for (i = 0; i < len - 1; i++) {
   122				reg_i = i * MC33XS2410_WORD_LEN;
 > 123				val[i] = FIELD_GET(MC33XS2410_RD_DATA_MASK,
   124						   get_unaligned_be16(&rx[reg_i]));
   125			}
   126		}
   127	
   128		return 0;
   129	}
   130	
   131	static
   132	int mc33xs2410_write_regs(struct spi_device *spi, u8 *reg, u16 *val, int len)
   133	{
   134	
   135		return mc33xs2410_xfer_regs(spi, false, reg, val, NULL, len);
   136	}
   137	
   138	static int mc33xs2410_read_regs(struct spi_device *spi, u8 *reg, bool *ctrl,
   139					u16 *val, u8 len)
   140	{
   141		return mc33xs2410_xfer_regs(spi, true, reg, val, ctrl, len);
   142	}
   143	
   144	
   145	static int mc33xs2410_write_reg(struct spi_device *spi, u8 reg, u16 val)
   146	{
   147		return mc33xs2410_write_regs(spi, &reg, &val, 1);
   148	}
   149	
   150	static
   151	int mc33xs2410_read_reg(struct spi_device *spi, u8 reg, u16 *val, bool ctrl)
   152	{
   153		return mc33xs2410_read_regs(spi, &reg, &ctrl, val, 1);
   154	}
   155	
   156	static int mc33xs2410_read_reg_ctrl(struct spi_device *spi, u8 reg, u16 *val)
   157	{
   158		return mc33xs2410_read_reg(spi, reg, val, true);
   159	}
   160	
   161	static
   162	int mc33xs2410_modify_reg(struct spi_device *spi, u8 reg, u16 mask, u16 val)
   163	{
   164		u16 tmp;
   165		int ret;
   166	
   167		ret = mc33xs2410_read_reg_ctrl(spi, reg, &tmp);
   168		if (ret < 0)
   169			return ret;
   170	
   171		tmp &= ~mask;
   172		tmp |= val & mask;
   173	
   174		return mc33xs2410_write_reg(spi, reg, tmp);
   175	}
   176	
   177	static u8 mc33xs2410_pwm_get_freq(u64 period)
   178	{
   179		u8 step, count;
   180	
   181		/*
   182		 * Check if period is within the limits of each of the four frequency
   183		 * ranges, starting with the highest frequency(lowest period). Higher
   184		 * frequencies are represented with better resolution by the device.
   185		 * Therefore favor frequency range with the better resolution to
   186		 * minimize error introduced by the frequency steps.
   187		 */
   188	
   189		switch (period) {
   190		case MC33XS2410_MIN_PERIOD_STEP(3) + 1 ... MC33XS2410_MAX_PERIOD_STEP(3):
   191			step = 3;
   192			break;
   193		case MC33XS2410_MAX_PERIOD_STEP(3) + 1 ... MC33XS2410_MAX_PERIOD_STEP(2):
   194			step = 2;
   195			break;
   196		case MC33XS2410_MAX_PERIOD_STEP(2) + 1 ... MC33XS2410_MAX_PERIOD_STEP(1):
   197			step = 1;
   198			break;
   199		case MC33XS2410_MAX_PERIOD_STEP(1) + 1 ... MC33XS2410_MAX_PERIOD_STEP(0):
   200			step = 0;
   201			break;
   202		}
   203	
   204		count = DIV_ROUND_UP(MC33XS2410_MAX_PERIOD_STEP(step), period) - 1;
   205	
 > 206		return FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step) |
   207		       FIELD_PREP(MC33XS2410_PWM_FREQ_COUNT_MASK, count);
   208	}
   209	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Dimitri,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.9 next-20240516]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Dimitri-Fedrau/dt-bindings-pwm-add-support-for-MC33XS2410/20240515-192237
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240515112034.298116-3-dima.fedrau%40gmail.com
patch subject: [PATCH v3 2/2] pwm: add support for NXPs high-side switch MC33XS2410
config: arm-randconfig-r121-20240517 (https://download.01.org/0day-ci/archive/20240517/202405170826.pUFGJfD7-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240517/202405170826.pUFGJfD7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405170826.pUFGJfD7-lkp@intel.com/

All errors (new ones prefixed by >>):

   arm-linux-gnueabi-ld: drivers/pwm/pwm-mc33xs2410.o: in function `mc33xs2410_pwm_apply':
>> pwm-mc33xs2410.c:(.text+0x3c0): undefined reference to `__aeabi_uldivmod'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Uwe Kleine-König]

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

Hello,

On Wed, May 15, 2024 at 01:20:34PM +0200, Dimitri Fedrau wrote:
> diff --git a/drivers/pwm/pwm-mc33xs2410.c b/drivers/pwm/pwm-mc33xs2410.c
> new file mode 100644
> index 000000000000..1904d1ee0652
> --- /dev/null
> +++ b/drivers/pwm/pwm-mc33xs2410.c
> @@ -0,0 +1,410 @@
> +// 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/delay.h>
> +#include <linux/err.h>
> +#include <linux/math.h>
> +#include <linux/math64.h>
> +#include <linux/minmax.h>
> +#include <linux/module.h>
> +#include <linux/mutex.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;

Huh, this is complicated. Isn't that covered by regmap somehow?

> +}
> +
> [...]
> +
> +static u8 mc33xs2410_pwm_get_freq(u64 period)
> +{
> +	u8 step, count;
> +
> +	/*
> +	 * Check if period is within the limits of each of the four frequency
> +	 * ranges, 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.

I'm not a native English speaker, but I find that misleading. That
period is in the "possible" range is already asserted by the caller. So
the switch is about "Check which step is appropriate for the given
period", right?

> +	 */
> +
> +	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(MC33XS2410_MAX_PERIOD_STEP(step), period) - 1;
> +
> +	return FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step) |
> +	       FIELD_PREP(MC33XS2410_PWM_FREQ_COUNT_MASK, count);
> +}
> +
> [...]
> +
> +static int mc33xs2410_pwm_get_relative_duty_cycle(u64 period, u64 duty_cycle)
> +{
> +	if (!period)
> +		return 0;
> +
> +	duty_cycle *= 256;

This might overflow.

> +	duty_cycle = DIV_ROUND_CLOSEST_ULL(duty_cycle, period);

round-closest is most probably wrong. Please test your driver with
PWM_DEBUG enabled and increasing and decreasing series of duty_cycle and
period.

> +
> +	/* Device is not able to generate 0% duty cycle */
> +	if (!duty_cycle)
> +		return -ERANGE;

Given that the hardware emits a low level when disabled, please disable
if duty_cycle = 0 is requested.

> +	return duty_cycle - 1;
> +}
> +
> [...]
> +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]);
> +	pwm_set_relative_duty_cycle(state, val[1] + 1, 256);

pwm_set_relative_duty_cycle doesn't use the right rounding for
.get_state().

> +	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));
> +
> +	return 0;
> +}
> +
> [...]
> +static int mc33xs2410_probe(struct spi_device *spi)
> +{
> [...]
> +	/* Disable watchdog */
> +	ret = mc33xs2410_write_reg(spi, MC33XS2410_WDT, 0x0);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to disable watchdog\n");

Wouldn't the watchdog functionality better be handled by a dedicated
watchdog driver? Disabling it here unconditionally looks wrong.

> +	/* Transition to normal mode */
> +	ret = mc33xs2410_modify_reg(spi, MC33XS2410_GLB_CTRL,
> +				    MC33XS2410_GLB_CTRL_MODE_MASK,
> +				    MC33XS2410_GLB_CTRL_NORMAL_MODE);
> [...]

Best regards
Uwe

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

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

[Dimitri Fedrau]

Am Mon, Jul 29, 2024 at 11:28:56PM +0200 schrieb Uwe Kleine-König:
Hi Uwe,

[...]

> > +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;
> 
> Huh, this is complicated. Isn't that covered by regmap somehow?
>

AFAIK it isn't supported. The main reason why regmap-spi doesn't work for
this device is that the device needs a CS change after transmitting 16
bits. This is not covered by regmap-spi. So I would end up implementing
reg_read, regmap_write should be fine in regmap-spi. Besides that if I
want to come as close as possible to an atomic configuration, which is not
possible, I would have to implement some bulk read/write operations and
end up with a similar implementation. I would stick to the current
implementation if you agree.

> > +}
> > +
> > [...]
> > +
> > +static u8 mc33xs2410_pwm_get_freq(u64 period)
> > +{
> > +	u8 step, count;
> > +
> > +	/*
> > +	 * Check if period is within the limits of each of the four frequency
> > +	 * ranges, 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.
> 
> I'm not a native English speaker, but I find that misleading. That
> period is in the "possible" range is already asserted by the caller. So
> the switch is about "Check which step is appropriate for the given
> period", right?
> 

Yes, you are right. Will fix it in the next version.

> > +	 */
> > +
> > +	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(MC33XS2410_MAX_PERIOD_STEP(step), period) - 1;
> > +
> > +	return FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step) |
> > +	       FIELD_PREP(MC33XS2410_PWM_FREQ_COUNT_MASK, count);
> > +}
> > +
> > [...]
> > +
> > +static int mc33xs2410_pwm_get_relative_duty_cycle(u64 period, u64 duty_cycle)
> > +{
> > +	if (!period)
> > +		return 0;
> > +
> > +	duty_cycle *= 256;
> 
> This might overflow.
> 

How ? Max period and duty_cycle is checked by the caller and can be
maximum 2000000000, 2000000000 * 256 = 512000000000, fits in u64. Did I
miss anything ?

> > +	duty_cycle = DIV_ROUND_CLOSEST_ULL(duty_cycle, period);
> 
> round-closest is most probably wrong. Please test your driver with
> PWM_DEBUG enabled and increasing and decreasing series of duty_cycle and
> period.
>

Yes, I should probably round it down. But I tested with PWM_DEBUG enabled
and it gave me the best results so far. There are still few cases where
there are complaints. I try to fix it.

> > +
> > +	/* Device is not able to generate 0% duty cycle */
> > +	if (!duty_cycle)
> > +		return -ERANGE;
> 
> Given that the hardware emits a low level when disabled, please disable
> if duty_cycle = 0 is requested.
> 

Ok.

> > +	return duty_cycle - 1;
> > +}
> > +
> > [...]
> > +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]);
> > +	pwm_set_relative_duty_cycle(state, val[1] + 1, 256);
> 
> pwm_set_relative_duty_cycle doesn't use the right rounding for
> .get_state().
> 

As mentioned above, will try to fix it.

> > +	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));
> > +
> > +	return 0;
> > +}
> > +
> > [...]
> > +static int mc33xs2410_probe(struct spi_device *spi)
> > +{
> > [...]
> > +	/* Disable watchdog */
> > +	ret = mc33xs2410_write_reg(spi, MC33XS2410_WDT, 0x0);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "Failed to disable watchdog\n");
> 
> Wouldn't the watchdog functionality better be handled by a dedicated
> watchdog driver? Disabling it here unconditionally looks wrong.
> 

Yes, would be better. I planned this after this patchset is accepted.
Without disabling the watchdog, the device is not able to operate. So I
would stick to it for now and come up with a patch later on.

> > +	/* Transition to normal mode */
> > +	ret = mc33xs2410_modify_reg(spi, MC33XS2410_GLB_CTRL,
> > +				    MC33XS2410_GLB_CTRL_MODE_MASK,
> > +				    MC33XS2410_GLB_CTRL_NORMAL_MODE);
> > [...]
> 
> Best regards
> Uwe

Best regards
Dimitri

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

[Uwe Kleine-König]

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

Hello Dimitri,

On Wed, Jul 31, 2024 at 10:46:48AM +0200, Dimitri Fedrau wrote:
> Am Mon, Jul 29, 2024 at 11:28:56PM +0200 schrieb Uwe Kleine-König:
> > > +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;
> > 
> > Huh, this is complicated. Isn't that covered by regmap somehow?
> 
> AFAIK it isn't supported. The main reason why regmap-spi doesn't work for
> this device is that the device needs a CS change after transmitting 16
> bits. This is not covered by regmap-spi. So I would end up implementing
> reg_read, regmap_write should be fine in regmap-spi. Besides that if I
> want to come as close as possible to an atomic configuration, which is not
> possible, I would have to implement some bulk read/write operations and
> end up with a similar implementation. I would stick to the current
> implementation if you agree.

ack.

> > > +static int mc33xs2410_pwm_get_relative_duty_cycle(u64 period, u64 duty_cycle)
> > > +{
> > > +	if (!period)
> > > +		return 0;
> > > +
> > > +	duty_cycle *= 256;
> > 
> > This might overflow.
> > 
> 
> How ? Max period and duty_cycle is checked by the caller and can be
> maximum 2000000000, 2000000000 * 256 = 512000000000, fits in u64. Did I
> miss anything ?

I didn't notice the check in the caller. Please add a respective comment
for the next reader who might miss that.

> > > +	duty_cycle = DIV_ROUND_CLOSEST_ULL(duty_cycle, period);
> > 
> > round-closest is most probably wrong. Please test your driver with
> > PWM_DEBUG enabled and increasing and decreasing series of duty_cycle and
> > period.
> 
> Yes, I should probably round it down. But I tested with PWM_DEBUG enabled
> and it gave me the best results so far. There are still few cases where
> there are complaints. I try to fix it.

I don't have the hardware so I cannot test myself. Please make sure that
there are no more complaints, at least none you are aware of. PWM_DEBUG
should be happy if you pick a hardware setting where period is maximal
but not bigger than requested and then for that given period duty_cycle
is maximal but not bigger than requested. So typically use round-down
division in .apply(). In .get_state() you should return a pwm_state that
makes .apply() write the exact same state as found when .get_state() was
called. So typically you have to use round-up there.
 
> > > +	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));
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > [...]
> > > +static int mc33xs2410_probe(struct spi_device *spi)
> > > +{
> > > [...]
> > > +	/* Disable watchdog */
> > > +	ret = mc33xs2410_write_reg(spi, MC33XS2410_WDT, 0x0);
> > > +	if (ret < 0)
> > > +		return dev_err_probe(dev, ret, "Failed to disable watchdog\n");
> > 
> > Wouldn't the watchdog functionality better be handled by a dedicated
> > watchdog driver? Disabling it here unconditionally looks wrong.
> 
> Yes, would be better. I planned this after this patchset is accepted.
> Without disabling the watchdog, the device is not able to operate. So I
> would stick to it for now and come up with a patch later on.

How long is the default timeout? Don't you need to disable the watchdog
in the bootloader anyhow?

If you still think the watchdog should be disabled here, please add a
comment that it's conceptually wrong to do here, but needed until there
is a proper watchdog driver.

Should this better be a mfd driver then?

Best regards
Uwe

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

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

[Dimitri Fedrau]

Am Thu, Aug 01, 2024 at 12:24:28AM +0200 schrieb Uwe Kleine-König:
Hi Uwe,

[...]

> > > > +static int mc33xs2410_pwm_get_relative_duty_cycle(u64 period, u64 duty_cycle)
> > > > +{
> > > > +	if (!period)
> > > > +		return 0;
> > > > +
> > > > +	duty_cycle *= 256;
> > > 
> > > This might overflow.
> > > 
> > 
> > How ? Max period and duty_cycle is checked by the caller and can be
> > maximum 2000000000, 2000000000 * 256 = 512000000000, fits in u64. Did I
> > miss anything ?
> 
> I didn't notice the check in the caller. Please add a respective comment
> for the next reader who might miss that.
> 
Ok.

> > > > +	duty_cycle = DIV_ROUND_CLOSEST_ULL(duty_cycle, period);
> > > 
> > > round-closest is most probably wrong. Please test your driver with
> > > PWM_DEBUG enabled and increasing and decreasing series of duty_cycle and
> > > period.
> > 
> > Yes, I should probably round it down. But I tested with PWM_DEBUG enabled
> > and it gave me the best results so far. There are still few cases where
> > there are complaints. I try to fix it.
> 
> I don't have the hardware so I cannot test myself. Please make sure that
> there are no more complaints, at least none you are aware of. PWM_DEBUG
> should be happy if you pick a hardware setting where period is maximal
> but not bigger than requested and then for that given period duty_cycle
> is maximal but not bigger than requested. So typically use round-down
> division in .apply(). In .get_state() you should return a pwm_state that
> makes .apply() write the exact same state as found when .get_state() was
> called. So typically you have to use round-up there.
>

Thanks, this fixed the remaining complaints. Did it exactly as you
suggested.

> > > > +	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));
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > [...]
> > > > +static int mc33xs2410_probe(struct spi_device *spi)
> > > > +{
> > > > [...]
> > > > +	/* Disable watchdog */
> > > > +	ret = mc33xs2410_write_reg(spi, MC33XS2410_WDT, 0x0);
> > > > +	if (ret < 0)
> > > > +		return dev_err_probe(dev, ret, "Failed to disable watchdog\n");
> > > 
> > > Wouldn't the watchdog functionality better be handled by a dedicated
> > > watchdog driver? Disabling it here unconditionally looks wrong.
> > 
> > Yes, would be better. I planned this after this patchset is accepted.
> > Without disabling the watchdog, the device is not able to operate. So I
> > would stick to it for now and come up with a patch later on.
> 
> How long is the default timeout? Don't you need to disable the watchdog
> in the bootloader anyhow?
> 

Default and also maximum timeout is 256ms. My hardware is configured to
let the device stay in reset until the driver puts it out of reset by
setting the associated GPIO. Datasheet refers to it as "Disable mode".
Outputs are turned off.
Without having such reset logic, the device would need to have the
watchdog disabled in the bootloader and continue in "Normal mode" or it
would go into "Safe mode" while booting the kernel almost sure violating
the timeout. Outputs are then controlled by the INx input logic signals.
I think there is no single solution but rather depends on the use case.
There are three modes which the device can be in when the driver is probed:
"Disable", "Safe" and "Normal". All three are handled by this driver,
assuming the watchdog should be disabled.

> If you still think the watchdog should be disabled here, please add a
> comment that it's conceptually wrong to do here, but needed until there
> is a proper watchdog driver.
> 

I will leave a comment, but I'm not sure if I can come up with a good
solution since the maximum timeout is very low with 256ms. For interaction
with userspace it must be at least 1s.
Why is the handling of the watchdog conceptually wrong here, I could use
devm_watchdog_register_device to register it, why can't I just disable it
here ?

> Should this better be a mfd driver then?
> 

I don't thinks so, the watchdog and the outputs belong somehow together.
When the device runs into an timeout it switches into "Safe" mode and the
outputs are controlled by the INx input logic signals.
I thought of a mfd device as a device which provides multiple functions
not really belonging together. Didn't find a clear definition. What do you
think of it ?

Best regards
Dimitri

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

[Uwe Kleine-König]

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

Hello Dimitri,

On Thu, Aug 01, 2024 at 04:28:02PM +0200, Dimitri Fedrau wrote:
> Am Thu, Aug 01, 2024 at 12:24:28AM +0200 schrieb Uwe Kleine-König:
> > > > > +	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));
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > [...]
> > > > > +static int mc33xs2410_probe(struct spi_device *spi)
> > > > > +{
> > > > > [...]
> > > > > +	/* Disable watchdog */
> > > > > +	ret = mc33xs2410_write_reg(spi, MC33XS2410_WDT, 0x0);
> > > > > +	if (ret < 0)
> > > > > +		return dev_err_probe(dev, ret, "Failed to disable watchdog\n");
> > > > 
> > > > Wouldn't the watchdog functionality better be handled by a dedicated
> > > > watchdog driver? Disabling it here unconditionally looks wrong.
> > > 
> > > Yes, would be better. I planned this after this patchset is accepted.
> > > Without disabling the watchdog, the device is not able to operate. So I
> > > would stick to it for now and come up with a patch later on.
> > 
> > How long is the default timeout? Don't you need to disable the watchdog
> > in the bootloader anyhow?
> 
> Default and also maximum timeout is 256ms. My hardware is configured to
> let the device stay in reset until the driver puts it out of reset by
> setting the associated GPIO. Datasheet refers to it as "Disable mode".
> Outputs are turned off.
> Without having such reset logic, the device would need to have the
> watchdog disabled in the bootloader and continue in "Normal mode" or it
> would go into "Safe mode" while booting the kernel almost sure violating
> the timeout. Outputs are then controlled by the INx input logic signals.
> I think there is no single solution but rather depends on the use case.
> There are three modes which the device can be in when the driver is probed:
> "Disable", "Safe" and "Normal". All three are handled by this driver,
> assuming the watchdog should be disabled.
> 
> [...]
> > Should this better be a mfd driver then?
> > 
> 
> I don't thinks so, the watchdog and the outputs belong somehow together.

Ah, so the watchdog doesn't trigger a reboot of the machine, just resets
the I/O lines to input? If yes, that's not what a watchdog driver is
about and so this isn't a hint to create an mfd driver.

Best regards
Uwe

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