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

[PATCH v2 0/3] 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

Dimitri Fedrau (3):
  dt-bindings: pwm: add support for MC33XS2410
  pwm: add support for NXPs high-side switch MC33XS2410
  pwm: mc33xs2410: add support for direct inputs

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

-- 
2.39.2

[PATCH v2 1/3] dt-bindings: pwm: add support for MC33XS2410

[Dimitri Fedrau]

Adding documentation for NXPs MC33XS2410 high side switch.

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

Re: [PATCH v2 1/3] dt-bindings: pwm: add support for MC33XS2410

[Krzysztof Kozlowski]

On 01/03/2024 12:11, Dimitri Fedrau wrote:
> Adding documentation for NXPs MC33XS2410 high side switch.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> ---
>  .../bindings/pwm/nxp,mc33xs2410.yaml          | 118 ++++++++++++++++++
>  1 file changed, 118 insertions(+)


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


---

This is an automated instruction, just in case, because many review tags
are being ignored. If you know the process, you can skip it (please do
not feel offended by me posting it here - no bad intentions intended).
If you do not know the process, here is a short explanation:

Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. However, there's no need to repost patches *only* to add
the tags. The upstream maintainer will do that for tags received on the
version they apply.

https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577

Best regards,
Krzysztof

[PATCH v2 2/3] 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 | 324 +++++++++++++++++++++++++++++++++++
 3 files changed, 337 insertions(+)
 create mode 100644 drivers/pwm/pwm-mc33xs2410.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 4b956d661755..da7048899ea7 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -384,6 +384,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 c5ec9e168ee7..6e7904e82c42 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -34,6 +34,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..35753039da6b
--- /dev/null
+++ b/drivers/pwm/pwm-mc33xs2410.c
@@ -0,0 +1,324 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2024 Liebherr-Electronics and Drives GmbH
+ */
+
+#include <linux/delay.h>
+#include <linux/err.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_GLB_CTRL_SAFE_MODE	BIT(7)
+#define MC33XS2410_OUT1_4_CTRL		0x02
+#define MC33XS2410_PWM_CTRL1		0x05
+#define MC33XS2410_PWM_CTRL1_POL_INV(x)	BIT(x)
+#define MC33XS2410_PWM_CTRL3		0x07
+#define MC33XS2410_PWM_CTRL3_EN(x)	BIT(4 + (x))
+#define MC33XS2410_PWM_CTRL3_EN_MASK	GENMASK(7, 4)
+#define MC33XS2410_PWM_FREQ1		0x08
+#define MC33XS2410_PWM_FREQ(x)		(MC33XS2410_PWM_FREQ1 + (x))
+#define MC33XS2410_PWM_FREQ_STEP_MASK	GENMASK(7, 6)
+#define MC33XS2410_PWM_FREQ_MASK	GENMASK(5, 0)
+#define MC33XS2410_PWM_DC1		0x0c
+#define MC33XS2410_PWM_DC(x)		(MC33XS2410_PWM_DC1 + (x))
+#define MC33XS2410_WDT			0x14
+
+#define MC33XS2410_IN_OUT_STA		0x01
+#define MC33XS2410_IN_OUT_STA_OUT_EN(x)	BIT(4 + (x))
+
+#define MC33XS2410_WR_FLAG		BIT(7)
+#define MC33XS2410_RD_CTRL_FLAG		BIT(7)
+#define MC33XS2410_RD_DATA_MASK		GENMASK(13, 0)
+
+#define MC33XS2410_PERIOD_MAX	0
+#define MC33XS2410_PERIOD_MIN	1
+
+struct mc33xs2410_pwm {
+	struct pwm_chip chip;
+	struct spi_device *spi;
+	struct mutex lock;
+};
+
+enum mc33xs2410_freq_steps {
+	STEP_05HZ,
+	STEP_2HZ,
+	STEP_8HZ,
+	STEP_32HZ,
+};
+
+/*
+ * When outputs are controlled by SPI, the device supports four frequency ranges
+ * 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
+ * Below are the minimum and maximum frequencies converted to periods in ns for
+ * each of the four frequency ranges.
+ */
+static const u32 mc33xs2410_period[4][2] = {
+	[STEP_05HZ] = { 2000000000, 31250000 },
+	[STEP_2HZ] = { 500000000, 7812500 },
+	[STEP_8HZ] = { 125000000, 1953125 },
+	[STEP_32HZ] = { 31250000, 488281 },
+};
+
+static struct mc33xs2410_pwm *mc33xs2410_pwm_from_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct mc33xs2410_pwm, chip);
+}
+
+static int mc33xs2410_write_reg(struct spi_device *spi, u8 reg, u8 val)
+{
+	u8 tx[2];
+
+	tx[0] = reg | MC33XS2410_WR_FLAG;
+	tx[1] = val;
+
+	return spi_write(spi, tx, 2);
+}
+
+static int mc33xs2410_read_reg(struct spi_device *spi, u8 reg, bool ctrl)
+{
+	u8 tx[2], rx[2];
+	int ret;
+
+	tx[0] = reg;
+	tx[1] = ctrl ? MC33XS2410_RD_CTRL_FLAG : 0;
+
+	ret = spi_write(spi, tx, 2);
+	if (ret < 0)
+		return ret;
+
+	ret = spi_read(spi, rx, 2);
+	if (ret < 0)
+		return ret;
+
+	return FIELD_GET(MC33XS2410_RD_DATA_MASK, get_unaligned_be16(rx));
+}
+
+static int mc33xs2410_read_reg_ctrl(struct spi_device *spi, u8 reg)
+{
+	return mc33xs2410_read_reg(spi, reg, true);
+}
+
+static int mc33xs2410_modify_reg(struct spi_device *spi, u8 reg, u8 mask, u8 val)
+{
+	int ret;
+
+	ret = mc33xs2410_read_reg_ctrl(spi, reg);
+	if (ret < 0)
+		return ret;
+
+	ret &= ~mask;
+	ret |= val & mask;
+
+	return mc33xs2410_write_reg(spi, reg, ret);
+}
+
+static int mc33xs2410_read_reg_diag(struct spi_device *spi, u8 reg)
+{
+	return mc33xs2410_read_reg(spi, reg, false);
+}
+
+static u8 mc33xs2410_pwm_get_freq(const struct pwm_state *state)
+{
+	u32 period, freq, max, min;
+	int step;
+	u8 ret;
+
+	period = state->period;
+	/*
+	 * 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.
+	 */
+	for (step = STEP_32HZ; step >= STEP_05HZ; step--) {
+		min = mc33xs2410_period[step][MC33XS2410_PERIOD_MIN];
+		max = mc33xs2410_period[step][MC33XS2410_PERIOD_MAX];
+		if ((period <= max) && (period >= min))
+			break;
+	}
+
+	freq = DIV_ROUND_CLOSEST(max, period) - 1;
+	ret = FIELD_PREP(MC33XS2410_PWM_FREQ_MASK, freq);
+	return (ret | FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step));
+}
+
+static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+				const struct pwm_state *state)
+{
+	struct mc33xs2410_pwm *mc33xs2410 = mc33xs2410_pwm_from_chip(chip);
+	struct spi_device *spi = mc33xs2410->spi;
+	u8 mask, val;
+	int ret;
+
+	if (state->period > mc33xs2410_period[STEP_05HZ][MC33XS2410_PERIOD_MAX])
+		return -EINVAL;
+
+	if (state->period < mc33xs2410_period[STEP_32HZ][MC33XS2410_PERIOD_MIN])
+		return -EINVAL;
+
+	guard(mutex)(&mc33xs2410->lock);
+	mask = MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm);
+	val = (state->polarity == PWM_POLARITY_INVERSED) ? mask : 0;
+	ret = mc33xs2410_modify_reg(spi, MC33XS2410_PWM_CTRL1, mask, val);
+	if (ret < 0)
+		return ret;
+
+	ret = mc33xs2410_write_reg(spi, MC33XS2410_PWM_FREQ(pwm->hwpwm),
+				   mc33xs2410_pwm_get_freq(state));
+	if (ret < 0)
+		return ret;
+
+	ret = mc33xs2410_write_reg(spi, MC33XS2410_PWM_DC(pwm->hwpwm),
+				   pwm_get_relative_duty_cycle(state, 255));
+	if (ret < 0)
+		return ret;
+
+	mask = MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm);
+	val = (state->enabled) ? mask : 0;
+	return mc33xs2410_modify_reg(spi, MC33XS2410_PWM_CTRL3, mask, val);
+}
+
+static int mc33xs2410_pwm_get_state(struct pwm_chip *chip,
+				    struct pwm_device *pwm,
+				    struct pwm_state *state)
+{
+	struct mc33xs2410_pwm *mc33xs2410 = mc33xs2410_pwm_from_chip(chip);
+	struct spi_device *spi = mc33xs2410->spi;
+	u32 freq, code, steps;
+	int ret;
+
+	guard(mutex)(&mc33xs2410->lock);
+	ret = mc33xs2410_read_reg_ctrl(spi, MC33XS2410_PWM_CTRL1);
+	if (ret < 0)
+		return ret;
+
+	state->polarity = (ret & MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm)) ?
+			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+
+	ret = mc33xs2410_read_reg_ctrl(spi, MC33XS2410_PWM_FREQ(pwm->hwpwm));
+	if (ret < 0)
+		return ret;
+
+	/* Lowest frequency steps are starting with 0.5Hz, scale them by two. */
+	steps = (FIELD_GET(MC33XS2410_PWM_FREQ_STEP_MASK, ret) * 2) << 1;
+	code = FIELD_GET(MC33XS2410_PWM_FREQ_MASK, ret);
+	/* Frequency = (code + 1) x steps */
+	freq = (code + 1) * steps;
+	/* Convert frequency to period in ns, considering scaled steps value. */
+	state->period = 2000000000ULL / (freq);
+
+	ret = mc33xs2410_read_reg_ctrl(spi, MC33XS2410_PWM_DC(pwm->hwpwm));
+	if (ret < 0)
+		return ret;
+
+	ret = pwm_set_relative_duty_cycle(state, ret, 255);
+	if (ret)
+		return ret;
+
+	ret = mc33xs2410_read_reg_diag(spi, MC33XS2410_IN_OUT_STA);
+	if (ret < 0)
+		return ret;
+
+	state->enabled = !!(ret & MC33XS2410_IN_OUT_STA_OUT_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;
+	int ret;
+
+	mc33xs2410 = devm_kzalloc(&spi->dev, sizeof(*mc33xs2410), GFP_KERNEL);
+	if (!mc33xs2410)
+		return -ENOMEM;
+
+	mc33xs2410->chip.dev = dev;
+	mc33xs2410->chip.ops = &mc33xs2410_pwm_ops;
+	mc33xs2410->chip.npwm = 4;
+	mc33xs2410->spi = spi;
+	mutex_init(&mc33xs2410->lock);
+
+	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");
+
+	/* Transitition 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, &mc33xs2410->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", 0 },
+	{ }
+};
+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 v2 2/3] pwm: add support for NXPs high-side switch MC33XS2410

[Uwe Kleine-König]

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

Hello Dimitri,

On Fri, Mar 01, 2024 at 12:11:23PM +0100, Dimitri Fedrau wrote:
> diff --git a/drivers/pwm/pwm-mc33xs2410.c b/drivers/pwm/pwm-mc33xs2410.c
> new file mode 100644
> index 000000000000..35753039da6b
> --- /dev/null
> +++ b/drivers/pwm/pwm-mc33xs2410.c
> @@ -0,0 +1,324 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024 Liebherr-Electronics and Drives GmbH
> + */

Please document the general behaviour of the device here. For that
please stick to the format used in other drivers such that

	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c

does the right thing for your driver.

> +
> +#include <linux/delay.h>
> +#include <linux/err.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_GLB_CTRL_SAFE_MODE	BIT(7)
> +#define MC33XS2410_OUT1_4_CTRL		0x02
> +#define MC33XS2410_PWM_CTRL1		0x05
> +#define MC33XS2410_PWM_CTRL1_POL_INV(x)	BIT(x)
> +#define MC33XS2410_PWM_CTRL3		0x07
> +#define MC33XS2410_PWM_CTRL3_EN(x)	BIT(4 + (x))

Maybe add the valid range for x here. Something like:

#define MC33XS2410_PWM_CTRL3_EN(x)	BIT(4 + (x)) /* x in {0 ... 3} */

> +#define MC33XS2410_PWM_CTRL3_EN_MASK	GENMASK(7, 4)

MC33XS2410_PWM_CTRL3_EN_MASK is unused.

> +#define MC33XS2410_PWM_FREQ1		0x08
> +#define MC33XS2410_PWM_FREQ(x)		(MC33XS2410_PWM_FREQ1 + (x))

Huh, is it expected that MC33XS2410_PWM_FREQ(1) != MC33XS2410_PWM_FREQ1?
I guess the hardware manual numbers these registers from 1 .. max but
you're passing hwpwm which starts at 0? Hmm.

I think I'd use:

#define MC33XS2410_PWM_FREQ(x)		(MC33XS2410_PWM_FREQ1 + (x) - 1)

and pass hwpwm + 1.

> +#define MC33XS2410_PWM_FREQ_STEP_MASK	GENMASK(7, 6)
> +#define MC33XS2410_PWM_FREQ_MASK	GENMASK(5, 0)
> +#define MC33XS2410_PWM_DC1		0x0c
> +#define MC33XS2410_PWM_DC(x)		(MC33XS2410_PWM_DC1 + (x))
> +#define MC33XS2410_WDT			0x14
> +
> +#define MC33XS2410_IN_OUT_STA		0x01
> +#define MC33XS2410_IN_OUT_STA_OUT_EN(x)	BIT(4 + (x))
> +
> +#define MC33XS2410_WR_FLAG		BIT(7)
> +#define MC33XS2410_RD_CTRL_FLAG		BIT(7)
> +#define MC33XS2410_RD_DATA_MASK		GENMASK(13, 0)
> +
> +#define MC33XS2410_PERIOD_MAX	0
> +#define MC33XS2410_PERIOD_MIN	1

This deserves a comment. (Or drop it after following my suggestion to
drop mc33xs2410_period[][].)


> +struct mc33xs2410_pwm {
> +	struct pwm_chip chip;
> +	struct spi_device *spi;
> +	struct mutex lock;
> +};
> +
> +enum mc33xs2410_freq_steps {
> +	STEP_05HZ,
> +	STEP_2HZ,
> +	STEP_8HZ,
> +	STEP_32HZ,
> +};
> +
> +/*
> + * When outputs are controlled by SPI, the device supports four frequency ranges
> + * 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
> + * Below are the minimum and maximum frequencies converted to periods in ns for
> + * each of the four frequency ranges.
> + */
> +static const u32 mc33xs2410_period[4][2] = {
> +	[STEP_05HZ] = { 2000000000, 31250000 },
> +	[STEP_2HZ] = { 500000000, 7812500 },
> +	[STEP_8HZ] = { 125000000, 1953125 },
> +	[STEP_32HZ] = { 31250000, 488281 },
> +};
> +
> +static struct mc33xs2410_pwm *mc33xs2410_pwm_from_chip(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct mc33xs2410_pwm, chip);
> +}
> +
> +static int mc33xs2410_write_reg(struct spi_device *spi, u8 reg, u8 val)
> +{
> +	u8 tx[2];
> +
> +	tx[0] = reg | MC33XS2410_WR_FLAG;
> +	tx[1] = val;
> +
> +	return spi_write(spi, tx, 2);
> +}
> +
> +static int mc33xs2410_read_reg(struct spi_device *spi, u8 reg, bool ctrl)
> +{
> +	u8 tx[2], rx[2];
> +	int ret;
> +
> +	tx[0] = reg;
> +	tx[1] = ctrl ? MC33XS2410_RD_CTRL_FLAG : 0;
> +
> +	ret = spi_write(spi, tx, 2);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = spi_read(spi, rx, 2);
> +	if (ret < 0)
> +		return ret;

This could benefit from using spi_write_then_read().

> +
> +	return FIELD_GET(MC33XS2410_RD_DATA_MASK, get_unaligned_be16(rx));
> +}
> +
> +static int mc33xs2410_read_reg_ctrl(struct spi_device *spi, u8 reg)
> +{
> +	return mc33xs2410_read_reg(spi, reg, true);
> +}
> +
> +static int mc33xs2410_modify_reg(struct spi_device *spi, u8 reg, u8 mask, u8 val)
> +{
> +	int ret;
> +
> +	ret = mc33xs2410_read_reg_ctrl(spi, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret &= ~mask;
> +	ret |= val & mask;
> +
> +	return mc33xs2410_write_reg(spi, reg, ret);
> +}
> +
> +static int mc33xs2410_read_reg_diag(struct spi_device *spi, u8 reg)
> +{
> +	return mc33xs2410_read_reg(spi, reg, false);
> +}
> +
> +static u8 mc33xs2410_pwm_get_freq(const struct pwm_state *state)
> +{
> +	u32 period, freq, max, min;
> +	int step;
> +	u8 ret;

When reading "ret" I'd expect this to be an integer representing an
error code. Maybe call it "freq" instead?

> +	period = state->period;
> +	/*
> +	 * 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.
> +	 */
> +	for (step = STEP_32HZ; step >= STEP_05HZ; step--) {
> +		min = mc33xs2410_period[step][MC33XS2410_PERIOD_MIN];
> +		max = mc33xs2410_period[step][MC33XS2410_PERIOD_MAX];
> +		if ((period <= max) && (period >= min))
> +			break;
> +	}

Given that mc33xs2410_period[step][0] is 2000000000 >> (2 * step) and
mc33xs2410_period[step][1] = 31250000 >> (2 * step), this can be
calculated without a loop.

Something like:

	step = (fls((31250000 - 1) / period) + 1) / 2

or given there are only four options this can also be done as follows:

	switch (period) {
		case 488281 .. 31250000:
			step = 3;
			break;
		case 31250001 .. 125000000:
			...
	}

which gives the compiler a real chance to implement it efficiently. Also
then you could drop mc33xs2410_period[][].

> +	freq = DIV_ROUND_CLOSEST(max, period) - 1;
> +	ret = FIELD_PREP(MC33XS2410_PWM_FREQ_MASK, freq);
> +	return (ret | FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step));

Also using DIV_ROUND_CLOSEST smells wrong. Did you test with PWM_DEBUG
enabled?

> +}
> +
> +static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +				const struct pwm_state *state)
> +{
> +	struct mc33xs2410_pwm *mc33xs2410 = mc33xs2410_pwm_from_chip(chip);
> +	struct spi_device *spi = mc33xs2410->spi;
> +	u8 mask, val;
> +	int ret;
> +
> +	if (state->period > mc33xs2410_period[STEP_05HZ][MC33XS2410_PERIOD_MAX])
> +		return -EINVAL;

Please make this:

	u64 period = min(state->period, mc33xs2410_period[STEP_05HZ][MC33XS2410_PERIOD_MAX]);

> +
> +	if (state->period < mc33xs2410_period[STEP_32HZ][MC33XS2410_PERIOD_MIN])
> +		return -EINVAL;
> +
> +	guard(mutex)(&mc33xs2410->lock);

Huh, didn't know this syntax for locking. Interesting. However with the
pending changes for the next merge window, calls to .apply() are
serialized per chip already by the core, so you don't need locking.

> +	mask = MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm);
> +	val = (state->polarity == PWM_POLARITY_INVERSED) ? mask : 0;
> +	ret = mc33xs2410_modify_reg(spi, MC33XS2410_PWM_CTRL1, mask, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = mc33xs2410_write_reg(spi, MC33XS2410_PWM_FREQ(pwm->hwpwm),
> +				   mc33xs2410_pwm_get_freq(state));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = mc33xs2410_write_reg(spi, MC33XS2410_PWM_DC(pwm->hwpwm),
> +				   pwm_get_relative_duty_cycle(state, 255));
> +	if (ret < 0)
> +		return ret;
> +
> +	mask = MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm);
> +	val = (state->enabled) ? mask : 0;
> +	return mc33xs2410_modify_reg(spi, MC33XS2410_PWM_CTRL3, mask, val);

Is this procedure atomic? Or can it happen that the output pin does
something that is neither the old nor the new state in between?

Maybe it's worth the effort doing that in a single spi transfer, both to
make the procedure quicker and (maybe?) atomic.

> +}
> +
> +static int mc33xs2410_pwm_get_state(struct pwm_chip *chip,
> +				    struct pwm_device *pwm,
> +				    struct pwm_state *state)
> +{
> +	struct mc33xs2410_pwm *mc33xs2410 = mc33xs2410_pwm_from_chip(chip);
> +	struct spi_device *spi = mc33xs2410->spi;
> +	u32 freq, code, steps;
> +	int ret;
> +
> +	guard(mutex)(&mc33xs2410->lock);
> +	ret = mc33xs2410_read_reg_ctrl(spi, MC33XS2410_PWM_CTRL1);
> +	if (ret < 0)
> +		return ret;
> +
> +	state->polarity = (ret & MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm)) ?
> +			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> +
> +	ret = mc33xs2410_read_reg_ctrl(spi, MC33XS2410_PWM_FREQ(pwm->hwpwm));
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Lowest frequency steps are starting with 0.5Hz, scale them by two. */
> +	steps = (FIELD_GET(MC33XS2410_PWM_FREQ_STEP_MASK, ret) * 2) << 1;

You're multiplying by 2 twice here. I fail to follow.

> +	code = FIELD_GET(MC33XS2410_PWM_FREQ_MASK, ret);
> +	/* Frequency = (code + 1) x steps */
> +	freq = (code + 1) * steps;
> +	/* Convert frequency to period in ns, considering scaled steps value. */
> +	state->period = 2000000000ULL / (freq);

Please make 2000000000ULL a define. This can then be used also in the
calculations that currently involve mc33xs2410_period[][].

Also you need to round up here.

> +	ret = mc33xs2410_read_reg_ctrl(spi, MC33XS2410_PWM_DC(pwm->hwpwm));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = pwm_set_relative_duty_cycle(state, ret, 255);
> +	if (ret)
> +		return ret;

Pretty sure this is also wrong and fails if you enable PWM_DEBUG.

> +	ret = mc33xs2410_read_reg_diag(spi, MC33XS2410_IN_OUT_STA);
> +	if (ret < 0)
> +		return ret;
> +
> +	state->enabled = !!(ret & MC33XS2410_IN_OUT_STA_OUT_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;
> +	int ret;
> +
> +	mc33xs2410 = devm_kzalloc(&spi->dev, sizeof(*mc33xs2410), GFP_KERNEL);

After struct device *dev = &spi->dev you could better use dev here
instead of &spi->dev.

> +	if (!mc33xs2410)
> +		return -ENOMEM;

Please use devm_pwmchip_alloc(). See
11ee0a124cb48bb837a1d90c3504a9c3376e96d1 for a simple example to copy
from.

> +	mc33xs2410->chip.dev = dev;
> +	mc33xs2410->chip.ops = &mc33xs2410_pwm_ops;
> +	mc33xs2410->chip.npwm = 4;
> +	mc33xs2410->spi = spi;
> +	mutex_init(&mc33xs2410->lock);
> +
> +	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");
> +
> +	/* Transitition to normal mode */

s/Transitition/Transition/

> +	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, &mc33xs2410->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", 0 },

driver_data is unused here, please drop it.

> +	{ }
> +};
> +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");

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

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

[Dimitri Fedrau]

Am Tue, Mar 19, 2024 at 05:35:41PM +0100 schrieb Uwe Kleine-König:
> Hello Dimitri,
>
Hi Uwe,

thanks for reviewing and sorry for the late reply.

> On Fri, Mar 01, 2024 at 12:11:23PM +0100, Dimitri Fedrau wrote:
> > diff --git a/drivers/pwm/pwm-mc33xs2410.c b/drivers/pwm/pwm-mc33xs2410.c
> > new file mode 100644
> > index 000000000000..35753039da6b
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-mc33xs2410.c
> > @@ -0,0 +1,324 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2024 Liebherr-Electronics and Drives GmbH
> > + */
> 
> Please document the general behaviour of the device here. For that
> please stick to the format used in other drivers such that
> 
> 	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c
> 
> does the right thing for your driver.
>
Will add the description and limitations in the next version of the
driver.

> > +
> > +#include <linux/delay.h>
> > +#include <linux/err.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_GLB_CTRL_SAFE_MODE	BIT(7)
> > +#define MC33XS2410_OUT1_4_CTRL		0x02
> > +#define MC33XS2410_PWM_CTRL1		0x05
> > +#define MC33XS2410_PWM_CTRL1_POL_INV(x)	BIT(x)
> > +#define MC33XS2410_PWM_CTRL3		0x07
> > +#define MC33XS2410_PWM_CTRL3_EN(x)	BIT(4 + (x))
> 
> Maybe add the valid range for x here. Something like:
> 
> #define MC33XS2410_PWM_CTRL3_EN(x)	BIT(4 + (x)) /* x in {0 ... 3} */
>
Ok.

> > +#define MC33XS2410_PWM_CTRL3_EN_MASK	GENMASK(7, 4)
> 
> MC33XS2410_PWM_CTRL3_EN_MASK is unused.
> 
Will remove it.

> > +#define MC33XS2410_PWM_FREQ1		0x08
> > +#define MC33XS2410_PWM_FREQ(x)		(MC33XS2410_PWM_FREQ1 + (x))
> 
> Huh, is it expected that MC33XS2410_PWM_FREQ(1) != MC33XS2410_PWM_FREQ1?
> I guess the hardware manual numbers these registers from 1 .. max but
> you're passing hwpwm which starts at 0? Hmm.
> 
> I think I'd use:
> 
> #define MC33XS2410_PWM_FREQ(x)		(MC33XS2410_PWM_FREQ1 + (x) - 1)
> 
> and pass hwpwm + 1.
> 
Ok.

> > +#define MC33XS2410_PWM_FREQ_STEP_MASK	GENMASK(7, 6)
> > +#define MC33XS2410_PWM_FREQ_MASK	GENMASK(5, 0)
> > +#define MC33XS2410_PWM_DC1		0x0c
> > +#define MC33XS2410_PWM_DC(x)		(MC33XS2410_PWM_DC1 + (x))
> > +#define MC33XS2410_WDT			0x14
> > +
> > +#define MC33XS2410_IN_OUT_STA		0x01
> > +#define MC33XS2410_IN_OUT_STA_OUT_EN(x)	BIT(4 + (x))
> > +
> > +#define MC33XS2410_WR_FLAG		BIT(7)
> > +#define MC33XS2410_RD_CTRL_FLAG		BIT(7)
> > +#define MC33XS2410_RD_DATA_MASK		GENMASK(13, 0)
> > +
> > +#define MC33XS2410_PERIOD_MAX	0
> > +#define MC33XS2410_PERIOD_MIN	1
> 
> This deserves a comment. (Or drop it after following my suggestion to
> drop mc33xs2410_period[][].)
> 
>
I will drop it.

> > +struct mc33xs2410_pwm {
> > +	struct pwm_chip chip;
> > +	struct spi_device *spi;
> > +	struct mutex lock;
> > +};
> > +
> > +enum mc33xs2410_freq_steps {
> > +	STEP_05HZ,
> > +	STEP_2HZ,
> > +	STEP_8HZ,
> > +	STEP_32HZ,
> > +};
> > +
> > +/*
> > + * When outputs are controlled by SPI, the device supports four frequency ranges
> > + * 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
> > + * Below are the minimum and maximum frequencies converted to periods in ns for
> > + * each of the four frequency ranges.
> > + */
> > +static const u32 mc33xs2410_period[4][2] = {
> > +	[STEP_05HZ] = { 2000000000, 31250000 },
> > +	[STEP_2HZ] = { 500000000, 7812500 },
> > +	[STEP_8HZ] = { 125000000, 1953125 },
> > +	[STEP_32HZ] = { 31250000, 488281 },
> > +};
> > +
> > +static struct mc33xs2410_pwm *mc33xs2410_pwm_from_chip(struct pwm_chip *chip)
> > +{
> > +	return container_of(chip, struct mc33xs2410_pwm, chip);
> > +}
> > +
> > +static int mc33xs2410_write_reg(struct spi_device *spi, u8 reg, u8 val)
> > +{
> > +	u8 tx[2];
> > +
> > +	tx[0] = reg | MC33XS2410_WR_FLAG;
> > +	tx[1] = val;
> > +
> > +	return spi_write(spi, tx, 2);
> > +}
> > +
> > +static int mc33xs2410_read_reg(struct spi_device *spi, u8 reg, bool ctrl)
> > +{
> > +	u8 tx[2], rx[2];
> > +	int ret;
> > +
> > +	tx[0] = reg;
> > +	tx[1] = ctrl ? MC33XS2410_RD_CTRL_FLAG : 0;
> > +
> > +	ret = spi_write(spi, tx, 2);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = spi_read(spi, rx, 2);
> > +	if (ret < 0)
> > +		return ret;
> 
> This could benefit from using spi_write_then_read().
> 
The device needs the chip select to go inactive after two bytes are
transmitted. I could go for spi_sync_transfer and set cs_change in the
first spi_transfer to 1 to get this done.

> > +
> > +	return FIELD_GET(MC33XS2410_RD_DATA_MASK, get_unaligned_be16(rx));
> > +}
> > +
> > +static int mc33xs2410_read_reg_ctrl(struct spi_device *spi, u8 reg)
> > +{
> > +	return mc33xs2410_read_reg(spi, reg, true);
> > +}
> > +
> > +static int mc33xs2410_modify_reg(struct spi_device *spi, u8 reg, u8 mask, u8 val)
> > +{
> > +	int ret;
> > +
> > +	ret = mc33xs2410_read_reg_ctrl(spi, reg);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret &= ~mask;
> > +	ret |= val & mask;
> > +
> > +	return mc33xs2410_write_reg(spi, reg, ret);
> > +}
> > +
> > +static int mc33xs2410_read_reg_diag(struct spi_device *spi, u8 reg)
> > +{
> > +	return mc33xs2410_read_reg(spi, reg, false);
> > +}
> > +
> > +static u8 mc33xs2410_pwm_get_freq(const struct pwm_state *state)
> > +{
> > +	u32 period, freq, max, min;
> > +	int step;
> > +	u8 ret;
> 
> When reading "ret" I'd expect this to be an integer representing an
> error code. Maybe call it "freq" instead?
> 
Ok.

> > +	period = state->period;
> > +	/*
> > +	 * 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.
> > +	 */
> > +	for (step = STEP_32HZ; step >= STEP_05HZ; step--) {
> > +		min = mc33xs2410_period[step][MC33XS2410_PERIOD_MIN];
> > +		max = mc33xs2410_period[step][MC33XS2410_PERIOD_MAX];
> > +		if ((period <= max) && (period >= min))
> > +			break;
> > +	}
> 
> Given that mc33xs2410_period[step][0] is 2000000000 >> (2 * step) and
> mc33xs2410_period[step][1] = 31250000 >> (2 * step), this can be
> calculated without a loop.
> 
> Something like:
> 
> 	step = (fls((31250000 - 1) / period) + 1) / 2
> 
> or given there are only four options this can also be done as follows:
> 
> 	switch (period) {
> 		case 488281 .. 31250000:
> 			step = 3;
> 			break;
> 		case 31250001 .. 125000000:
> 			...
> 	}
>
> which gives the compiler a real chance to implement it efficiently. Also
> then you could drop mc33xs2410_period[][].
>
I would stick to the switch and drop mc33xs2410_period[][].

> > +	freq = DIV_ROUND_CLOSEST(max, period) - 1;
> > +	ret = FIELD_PREP(MC33XS2410_PWM_FREQ_MASK, freq);
> > +	return (ret | FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step));
> 
> Also using DIV_ROUND_CLOSEST smells wrong. Did you test with PWM_DEBUG
> enabled?
> 
At least I enabled it and tested some cases. But I think
DIV_ROUND_CLOSEST is what I want. The device is able to generate
frequencies in four frequency ranges with different steps of resolution.
I want to minimize the error introduced by this approach. When a user
wants to set 1041 Hz, the device is not able to generate this frequency.
It is able to generate 1024 Hz or 1056Hz and 1024 Hz is more accurate,
which hopefully can be accomplished with DIV_ROUND_CLOSEST.

> > +}
> > +
> > +static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +				const struct pwm_state *state)
> > +{
> > +	struct mc33xs2410_pwm *mc33xs2410 = mc33xs2410_pwm_from_chip(chip);
> > +	struct spi_device *spi = mc33xs2410->spi;
> > +	u8 mask, val;
> > +	int ret;
> > +
> > +	if (state->period > mc33xs2410_period[STEP_05HZ][MC33XS2410_PERIOD_MAX])
> > +		return -EINVAL;
> 
> Please make this:
> 
> 	u64 period = min(state->period, mc33xs2410_period[STEP_05HZ][MC33XS2410_PERIOD_MAX]);
>
Ok.
> > +
> > +	if (state->period < mc33xs2410_period[STEP_32HZ][MC33XS2410_PERIOD_MIN])
> > +		return -EINVAL;
> > +
> > +	guard(mutex)(&mc33xs2410->lock);
> 
> Huh, didn't know this syntax for locking. Interesting. However with the
> pending changes for the next merge window, calls to .apply() are
> serialized per chip already by the core, so you don't need locking.
> 
Ok.

> > +	mask = MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm);
> > +	val = (state->polarity == PWM_POLARITY_INVERSED) ? mask : 0;
> > +	ret = mc33xs2410_modify_reg(spi, MC33XS2410_PWM_CTRL1, mask, val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = mc33xs2410_write_reg(spi, MC33XS2410_PWM_FREQ(pwm->hwpwm),
> > +				   mc33xs2410_pwm_get_freq(state));
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = mc33xs2410_write_reg(spi, MC33XS2410_PWM_DC(pwm->hwpwm),
> > +				   pwm_get_relative_duty_cycle(state, 255));
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	mask = MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm);
> > +	val = (state->enabled) ? mask : 0;
> > +	return mc33xs2410_modify_reg(spi, MC33XS2410_PWM_CTRL3, mask, val);
> 
> Is this procedure atomic? Or can it happen that the output pin does
> something that is neither the old nor the new state in between?
> 
It is not atomic and I didn't find any information in the datasheet how
to achieve this. Data is taken and valid only after the chip select is
going inactive after each write transaction. Write transactions can't be
bundled to make it atomic, not documented, but I still tried it without
success. It is possible that the output pin does something that is
neither the old nor the new state. At least this is what I see on my
scope. Tested it with the lowest frequency 0.5Hz.

> Maybe it's worth the effort doing that in a single spi transfer, both to
> make the procedure quicker and (maybe?) atomic.
> 
I could go for spi_sync_transfer with multiple transfers which change
chip select. This could maybe improve the procedure a little bit.

> > +}
> > +
> > +static int mc33xs2410_pwm_get_state(struct pwm_chip *chip,
> > +				    struct pwm_device *pwm,
> > +				    struct pwm_state *state)
> > +{
> > +	struct mc33xs2410_pwm *mc33xs2410 = mc33xs2410_pwm_from_chip(chip);
> > +	struct spi_device *spi = mc33xs2410->spi;
> > +	u32 freq, code, steps;
> > +	int ret;
> > +
> > +	guard(mutex)(&mc33xs2410->lock);
> > +	ret = mc33xs2410_read_reg_ctrl(spi, MC33XS2410_PWM_CTRL1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	state->polarity = (ret & MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm)) ?
> > +			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> > +
> > +	ret = mc33xs2410_read_reg_ctrl(spi, MC33XS2410_PWM_FREQ(pwm->hwpwm));
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Lowest frequency steps are starting with 0.5Hz, scale them by two. */
> > +	steps = (FIELD_GET(MC33XS2410_PWM_FREQ_STEP_MASK, ret) * 2) << 1;
> 
> You're multiplying by 2 twice here. I fail to follow.
> 
steps:
  - 0 = 0.5Hz
  - 1 = 2Hz
  - 2 = 8Hz
  - 3 = 32Hz
frequency = (code + 1) x steps

To avoid division in case steps is zero, I scale steps value by two
keeping in mind that I doubled the frequency. This is important when
calculating the period. I will document this properly in next version of
the driver.

> > +	code = FIELD_GET(MC33XS2410_PWM_FREQ_MASK, ret);
> > +	/* Frequency = (code + 1) x steps */
> > +	freq = (code + 1) * steps;
> > +	/* Convert frequency to period in ns, considering scaled steps value. */
> > +	state->period = 2000000000ULL / (freq);
> 
> Please make 2000000000ULL a define. This can then be used also in the
> calculations that currently involve mc33xs2410_period[][].
> 
Ok.
> Also you need to round up here.
> 
Ok.
> > +	ret = mc33xs2410_read_reg_ctrl(spi, MC33XS2410_PWM_DC(pwm->hwpwm));
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = pwm_set_relative_duty_cycle(state, ret, 255);
> > +	if (ret)
> > +		return ret;
> 
> Pretty sure this is also wrong and fails if you enable PWM_DEBUG.
> 
Yes, you are right. Would go for:
pwm_set_relative_duty_cycle(state, ret + 1, 256);

> > +	ret = mc33xs2410_read_reg_diag(spi, MC33XS2410_IN_OUT_STA);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	state->enabled = !!(ret & MC33XS2410_IN_OUT_STA_OUT_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;
> > +	int ret;
> > +
> > +	mc33xs2410 = devm_kzalloc(&spi->dev, sizeof(*mc33xs2410), GFP_KERNEL);
> 
> After struct device *dev = &spi->dev you could better use dev here
> instead of &spi->dev.
>
Ok.

> > +	if (!mc33xs2410)
> > +		return -ENOMEM;
> 
> Please use devm_pwmchip_alloc(). See
> 11ee0a124cb48bb837a1d90c3504a9c3376e96d1 for a simple example to copy
> from.
>
Ok.

> > +	mc33xs2410->chip.dev = dev;
> > +	mc33xs2410->chip.ops = &mc33xs2410_pwm_ops;
> > +	mc33xs2410->chip.npwm = 4;
> > +	mc33xs2410->spi = spi;
> > +	mutex_init(&mc33xs2410->lock);
> > +
> > +	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");
> > +
> > +	/* Transitition to normal mode */
> 
> s/Transitition/Transition/
>
Ok.

> > +	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, &mc33xs2410->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", 0 },
> 
> driver_data is unused here, please drop it.
>
Ok.

> > +	{ }
> > +};
> > +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");
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Best regards
Dimitri

[PATCH v2 3/3] pwm: mc33xs2410: add support for direct inputs

[Dimitri Fedrau]

Add support for direct inputs, which are used to directly turn-on or
turn-off the outputs. Direct inputs have the advantage over the SPI
controlled outputs that they aren't limited to the frequency steps.
Frequency resolution depends on the input signal, range is still
from 0.5Hz to 2.048kHz.

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/pwm/pwm-mc33xs2410.c | 116 +++++++++++++++++++++++++++++++----
 1 file changed, 105 insertions(+), 11 deletions(-)

diff --git a/drivers/pwm/pwm-mc33xs2410.c b/drivers/pwm/pwm-mc33xs2410.c
index 35753039da6b..828a67227185 100644
--- a/drivers/pwm/pwm-mc33xs2410.c
+++ b/drivers/pwm/pwm-mc33xs2410.c
@@ -18,7 +18,10 @@
 #define MC33XS2410_GLB_CTRL_MODE_MASK	GENMASK(7, 6)
 #define MC33XS2410_GLB_CTRL_NORMAL_MODE	BIT(6)
 #define MC33XS2410_GLB_CTRL_SAFE_MODE	BIT(7)
+#define MC33XS2410_GLB_CTRL_CMOS_LEVEL	BIT(0)
 #define MC33XS2410_OUT1_4_CTRL		0x02
+#define MC33XS2410_IN_CTRL1		0x03
+#define MC33XS2410_IN_CTRL1_IN_EN(x)	BIT(x)
 #define MC33XS2410_PWM_CTRL1		0x05
 #define MC33XS2410_PWM_CTRL1_POL_INV(x)	BIT(x)
 #define MC33XS2410_PWM_CTRL3		0x07
@@ -45,6 +48,7 @@
 struct mc33xs2410_pwm {
 	struct pwm_chip chip;
 	struct spi_device *spi;
+	struct pwm_device *di[4];
 	struct mutex lock;
 };
 
@@ -154,20 +158,15 @@ static u8 mc33xs2410_pwm_get_freq(const struct pwm_state *state)
 	return (ret | FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step));
 }
 
-static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-				const struct pwm_state *state)
+static int mc33xs2410_pwm_apply_spi(struct pwm_chip *chip,
+				    struct pwm_device *pwm,
+				    const struct pwm_state *state)
 {
 	struct mc33xs2410_pwm *mc33xs2410 = mc33xs2410_pwm_from_chip(chip);
 	struct spi_device *spi = mc33xs2410->spi;
 	u8 mask, val;
 	int ret;
 
-	if (state->period > mc33xs2410_period[STEP_05HZ][MC33XS2410_PERIOD_MAX])
-		return -EINVAL;
-
-	if (state->period < mc33xs2410_period[STEP_32HZ][MC33XS2410_PERIOD_MIN])
-		return -EINVAL;
-
 	guard(mutex)(&mc33xs2410->lock);
 	mask = MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm);
 	val = (state->polarity == PWM_POLARITY_INVERSED) ? mask : 0;
@@ -190,9 +189,38 @@ static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	return mc33xs2410_modify_reg(spi, MC33XS2410_PWM_CTRL3, mask, val);
 }
 
-static int mc33xs2410_pwm_get_state(struct pwm_chip *chip,
-				    struct pwm_device *pwm,
-				    struct pwm_state *state)
+static int mc33xs2410_pwm_apply_direct_inputs(struct pwm_chip *chip,
+					      struct pwm_device *pwm,
+					      const struct pwm_state *state)
+{
+	struct mc33xs2410_pwm *mc33xs2410 = mc33xs2410_pwm_from_chip(chip);
+	struct pwm_device *di = mc33xs2410->di[pwm->hwpwm];
+
+	guard(mutex)(&mc33xs2410->lock);
+
+	return pwm_apply_state(di, state);
+}
+
+static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+				const struct pwm_state *state)
+{
+	struct mc33xs2410_pwm *mc33xs2410 = mc33xs2410_pwm_from_chip(chip);
+
+	if (state->period > mc33xs2410_period[STEP_05HZ][MC33XS2410_PERIOD_MAX])
+		return -EINVAL;
+
+	if (state->period < mc33xs2410_period[STEP_32HZ][MC33XS2410_PERIOD_MIN])
+		return -EINVAL;
+
+	if (mc33xs2410->di[pwm->hwpwm])
+		return mc33xs2410_pwm_apply_direct_inputs(chip, pwm, state);
+	else
+		return mc33xs2410_pwm_apply_spi(chip, pwm, state);
+}
+
+static int mc33xs2410_pwm_get_state_spi(struct pwm_chip *chip,
+					struct pwm_device *pwm,
+					struct pwm_state *state)
 {
 	struct mc33xs2410_pwm *mc33xs2410 = mc33xs2410_pwm_from_chip(chip);
 	struct spi_device *spi = mc33xs2410->spi;
@@ -236,6 +264,28 @@ static int mc33xs2410_pwm_get_state(struct pwm_chip *chip,
 	return 0;
 }
 
+static int mc33xs2410_pwm_get_state_direct_inputs(struct pwm_chip *chip,
+						  struct pwm_device *pwm,
+						  struct pwm_state *state)
+{
+	struct mc33xs2410_pwm *mc33xs2410 = mc33xs2410_pwm_from_chip(chip);
+
+	pwm_get_state(mc33xs2410->di[pwm->hwpwm], state);
+	return 0;
+}
+
+static int mc33xs2410_pwm_get_state(struct pwm_chip *chip,
+				    struct pwm_device *pwm,
+				    struct pwm_state *state)
+{
+	struct mc33xs2410_pwm *mc33xs2410 = mc33xs2410_pwm_from_chip(chip);
+
+	if (mc33xs2410->di[pwm->hwpwm])
+		return mc33xs2410_pwm_get_state_direct_inputs(chip, pwm, state);
+	else
+		return mc33xs2410_pwm_get_state_spi(chip, pwm, state);
+}
+
 static const struct pwm_ops mc33xs2410_pwm_ops = {
 	.apply = mc33xs2410_pwm_apply,
 	.get_state = mc33xs2410_pwm_get_state,
@@ -257,6 +307,45 @@ static int mc33xs2410_reset(struct device *dev)
 	return 0;
 }
 
+static int mc33xs2410_direct_inputs_probe(struct mc33xs2410_pwm *mc33xs2410)
+{
+	struct device *dev = &mc33xs2410->spi->dev;
+	u16 di_en = 0;
+	char buf[4];
+	int ret, ch;
+
+	for (ch = 0; ch < 4; ch++) {
+		sprintf(buf, "di%d", ch);
+		mc33xs2410->di[ch] = devm_pwm_get(dev, buf);
+		ret = PTR_ERR_OR_ZERO(mc33xs2410->di[ch]);
+		switch (ret) {
+		case 0:
+			di_en |= MC33XS2410_IN_CTRL1_IN_EN(ch);
+			break;
+		case -ENODATA:
+			mc33xs2410->di[ch] = NULL;
+			break;
+		case -EPROBE_DEFER:
+			return ret;
+		default:
+			dev_err(dev, "Failed to request %s: %d\n", buf, ret);
+			return ret;
+		}
+	}
+
+	if (!di_en)
+		return 0;
+
+	/* CMOS input logic level */
+	ret = mc33xs2410_modify_reg(mc33xs2410->spi, MC33XS2410_GLB_CTRL,
+				    MC33XS2410_GLB_CTRL_CMOS_LEVEL,
+				    MC33XS2410_GLB_CTRL_CMOS_LEVEL);
+	if (ret < 0)
+		return ret;
+
+	return mc33xs2410_write_reg(mc33xs2410->spi, MC33XS2410_IN_CTRL1, di_en);
+}
+
 static int mc33xs2410_probe(struct spi_device *spi)
 {
 	struct mc33xs2410_pwm *mc33xs2410;
@@ -290,6 +379,11 @@ static int mc33xs2410_probe(struct spi_device *spi)
 		return dev_err_probe(dev, ret,
 				     "Failed to transition to normal mode\n");
 
+	/* Enable direct inputs */
+	ret = mc33xs2410_direct_inputs_probe(mc33xs2410);
+	if (ret)
+		return ret;
+
 	ret = devm_pwmchip_add(dev, &mc33xs2410->chip);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to add pwm chip\n");
-- 
2.39.2

Re: [PATCH v2 3/3] pwm: mc33xs2410: add support for direct inputs

[Uwe Kleine-König]

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

Hello Dimitri,

On Fri, Mar 01, 2024 at 12:11:24PM +0100, Dimitri Fedrau wrote:
> Add support for direct inputs, which are used to directly turn-on or
> turn-off the outputs. Direct inputs have the advantage over the SPI
> controlled outputs that they aren't limited to the frequency steps.
> Frequency resolution depends on the input signal, range is still
> from 0.5Hz to 2.048kHz.

I didn't make a big effort, but I fail to understand the concept here.
I'll delay giving more feedback till the next review round for this
driver. Maybe then the description is easier to grasp.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v2 3/3] pwm: mc33xs2410: add support for direct inputs

[Dimitri Fedrau]

Am Tue, Mar 19, 2024 at 05:38:52PM +0100 schrieb Uwe Kleine-König:
> Hello Dimitri,
>
Hi Uwe,

> On Fri, Mar 01, 2024 at 12:11:24PM +0100, Dimitri Fedrau wrote:
> > Add support for direct inputs, which are used to directly turn-on or
> > turn-off the outputs. Direct inputs have the advantage over the SPI
> > controlled outputs that they aren't limited to the frequency steps.
> > Frequency resolution depends on the input signal, range is still
> > from 0.5Hz to 2.048kHz.
> 
> I didn't make a big effort, but I fail to understand the concept here.
> I'll delay giving more feedback till the next review round for this
> driver. Maybe then the description is easier to grasp.
>
I will drop it for now and come back later with a separate patch. I
think this would make it easier for now.

> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Best regards
Dimitri