[PATCH v2 0/2] pwm: Add the Ingenic X1600 PWM controller

[PATCH v2 0/2] pwm: Add the Ingenic X1600 PWM controller

[Olivier Louvignes]

The Ingenic X1600 has a PWM controller of its own, separate from the TCU
timers that pwm-jz4740 drives on the earlier Ingenic SoCs. This adds its
binding and a driver built on the waveform callbacks.

Changes in v2 (both found by the Sashiko review of v1):
- A channel that is off with both counts still loaded, as one stopped
  behind the driver's back leaves it, now reads back as disabled. v1 read
  it as running, so asking for the same waveform again didn't switch it
  on. A channel off with one count zero is how 0 and 100% are written; the
  idle level gives which.
- A clock rate of 0 is refused with -EINVAL when rounding a waveform and
  when writing one, instead of switching the channel off silently or
  dividing by zero in the update timeout.

Tested on a HiBy R1 (X1600), whose backlight is channel 0, through
pwm-backlight: brightness 1, 25, 50, 60 and 100% give the expected high and
low counts, 0 and 100% switch the channel off at the right idle level,
and an inversed 250 us of 1 ms on channel 1 reads back as INL 0, LOW
9375 then HIGH 28125 with the clock divided by 8. PWM_DEBUG stayed
silent. For v2 also: channel 1 running 250 us of 1 ms, switched off
through ENC directly, then requested again through sysfs, reads back as
disabled, and the same waveform asked again switches it back on; v1's
driver read it as enabled and left it off.

The X1600 has no device tree upstream yet; its support is being posted
one subsystem at a time.

v1: https://lore.kernel.org/r/20261025084643.93167-1-olivier@mgcrea.io

Olivier Louvignes (2):
  dt-bindings: pwm: Add the Ingenic X1600 PWM controller
  pwm: Add a driver for the Ingenic X1600 PWM controller

 .../bindings/pwm/ingenic,x1600-pwm.yaml       |  47 +++
 MAINTAINERS                                   |   6 +
 drivers/pwm/Kconfig                           |  11 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-ingenic-x1600.c               | 280 ++++++++++++++++++
 5 files changed, 345 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/ingenic,x1600-pwm.yaml
 create mode 100644 drivers/pwm/pwm-ingenic-x1600.c


base-commit: 00e917df050d79ac26ec700717680ad6421d2808
-- 
2.39.5

[PATCH v2 1/2] dt-bindings: pwm: Add the Ingenic X1600 PWM controller

[Olivier Louvignes]

The X1600 has an eight-channel PWM controller of its own, separate from
the TCU timers that drive the PWMs of earlier Ingenic SoCs
(ingenic,tcu.yaml), so it gets a binding of its own.

Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
---
 .../bindings/pwm/ingenic,x1600-pwm.yaml       | 47 +++++++++++++++++++
 MAINTAINERS                                   |  5 ++
 2 files changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/ingenic,x1600-pwm.yaml

diff --git a/Documentation/devicetree/bindings/pwm/ingenic,x1600-pwm.yaml b/Documentation/devicetree/bindings/pwm/ingenic,x1600-pwm.yaml
new file mode 100644
index 000000000000..3ecaa92a5ca2
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/ingenic,x1600-pwm.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/ingenic,x1600-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Ingenic X1600 PWM controller
+
+maintainers:
+  - Olivier Louvignes <olivier@mgcrea.io>
+
+description:
+  The eight-channel PWM controller of the Ingenic X1600 SoC, separate from
+  the TCU timers that drive the PWMs of earlier Ingenic SoCs.
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    const: ingenic,x1600-pwm
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 3
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - "#pwm-cells"
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    pwm@134c0000 {
+        compatible = "ingenic,x1600-pwm";
+        reg = <0x134c0000 0x200>;
+        clocks = <&cgu 21>;
+        #pwm-cells = <3>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 3a19da74d00c..c905aa13bcbc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12878,6 +12878,11 @@ F:	include/linux/mfd/ingenic-tcu.h
 F:	sound/soc/codecs/jz47*
 F:	sound/soc/jz4740/
 
+INGENIC X1600 PWM DRIVER
+M:	Olivier Louvignes <olivier@mgcrea.io>
+S:	Maintained
+F:	Documentation/devicetree/bindings/pwm/ingenic,x1600-pwm.yaml
+
 INJOINIC IP5xxx POWER BANK IC DRIVER
 M:	Samuel Holland <samuel@sholland.org>
 S:	Maintained
-- 
2.39.5

[PATCH v2 2/2] pwm: Add a driver for the Ingenic X1600 PWM controller

[Olivier Louvignes]

The X1600 has a PWM controller of its own, separate from the TCU timers
that pwm-jz4740 drives on earlier Ingenic SoCs (X1600 programming manual,
chapter 13): eight channels, each with a power-of-two clock divider and a
waveform word holding the counts at the high and at the low level, the
channel's initial level picking which comes first. So the duty offset is
0 or period - duty, and since neither count may be zero, a duty cycle of 0
or 100% switches the channel off with its idle level set accordingly.

The driver implements the waveform callbacks. It follows the manual's
order: the divider is only changed with the channel off, and a running
channel takes a new word through the update register.

Tested on a HiBy R1, whose backlight is channel 0.

Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
---
v2:
- Read a channel that is off with both counts loaded as disabled.
- Refuse a clock rate of 0 in round_waveform_tohw() and write_waveform().

 MAINTAINERS                     |   1 +
 drivers/pwm/Kconfig             |  11 ++
 drivers/pwm/Makefile            |   1 +
 drivers/pwm/pwm-ingenic-x1600.c | 280 ++++++++++++++++++++++++++++++++
 4 files changed, 293 insertions(+)
 create mode 100644 drivers/pwm/pwm-ingenic-x1600.c

diff --git a/MAINTAINERS b/MAINTAINERS
index c905aa13bcbc..c090512468a4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12882,6 +12882,7 @@ INGENIC X1600 PWM DRIVER
 M:	Olivier Louvignes <olivier@mgcrea.io>
 S:	Maintained
 F:	Documentation/devicetree/bindings/pwm/ingenic,x1600-pwm.yaml
+F:	drivers/pwm/pwm-ingenic-x1600.c
 
 INJOINIC IP5xxx POWER BANK IC DRIVER
 M:	Samuel Holland <samuel@sholland.org>
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 729776086879..244c8f4f140e 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -381,6 +381,17 @@ config PWM_JZ4740
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-jz4740.
 
+config PWM_INGENIC_X1600
+	tristate "Ingenic X1600 PWM support"
+	depends on MACH_INGENIC || COMPILE_TEST
+	depends on COMMON_CLK && OF && HAS_IOMEM
+	help
+	  Generic PWM framework driver for the PWM controller of the
+	  Ingenic X1600.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-ingenic-x1600.
+
 config PWM_KEEMBAY
 	tristate "Intel Keem Bay PWM driver"
 	depends on ARCH_KEEMBAY || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 5630a521a7cf..5c59b54e0029 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
 obj-$(CONFIG_PWM_INTEL_LGM)	+= pwm-intel-lgm.o
 obj-$(CONFIG_PWM_IPQ)		+= pwm-ipq.o
 obj-$(CONFIG_PWM_IQS620A)	+= pwm-iqs620a.o
+obj-$(CONFIG_PWM_INGENIC_X1600)	+= pwm-ingenic-x1600.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
 obj-$(CONFIG_PWM_KEEMBAY)	+= pwm-keembay.o
 obj-$(CONFIG_PWM_LOONGSON)	+= pwm-loongson.o
diff --git a/drivers/pwm/pwm-ingenic-x1600.c b/drivers/pwm/pwm-ingenic-x1600.c
new file mode 100644
index 000000000000..5865ff0cf298
--- /dev/null
+++ b/drivers/pwm/pwm-ingenic-x1600.c
@@ -0,0 +1,280 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PWM controller of the Ingenic X1600
+ *
+ * Copyright (c) 2026 Olivier Louvignes <olivier@mgcrea.io>
+ *
+ * Eight channels, each with a power-of-two clock divider and a waveform word
+ * holding the counts at the high and at the low level; the channel's initial
+ * level picks which comes first (X1600 programming manual, chapter 13). Unlike
+ * the JZ47xx and X1000, whose PWMs are TCU timer channels (pwm-jz4740), the
+ * X1600 has this separate controller.
+ *
+ * Limitations:
+ * - The duty offset is 0 or period - duty (the high level last).
+ * - Neither count may be zero, so a duty cycle of 0 or 100% switches the
+ *   channel off and holds its output at the idle level instead.
+ * - A disabled channel drives its idle level, low.
+ * - Changing the divider or the initial level stops the channel first, which
+ *   cuts the current period short.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/math64.h>
+#include <linux/minmax.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/time.h>
+#include <linux/types.h>
+
+#define X1600_PWM_ENS			0x00	/* write 1: switch a channel on */
+#define X1600_PWM_ENC			0x04	/* write 1: switch a channel off */
+#define X1600_PWM_EN			0x08	/* the channels switched on */
+#define X1600_PWM_UPT			0x10	/* write 1: load the waveform word */
+#define X1600_PWM_BUSY			0x14	/* a load is pending */
+#define X1600_PWM_MS			0x20	/* 0: waveform word, 1: DMA */
+#define X1600_PWM_INL			0x24	/* the level a period starts with */
+#define X1600_PWM_IDL			0x28	/* the level while off */
+#define X1600_PWM_CCFG(n)		(0x40 + 4 * (n))
+#define X1600_PWM_WCFG(n)		(0x80 + 4 * (n))
+
+#define X1600_PWM_CCFG_PRESCALE		GENMASK(3, 0)	/* divide by 2^PRESCALE */
+#define X1600_PWM_WCFG_HIGH		GENMASK(31, 16)
+#define X1600_PWM_WCFG_LOW		GENMASK(15, 0)
+
+#define X1600_PWM_NUM			8
+#define X1600_PWM_MAX_COUNT		0xffff
+#define X1600_PWM_MAX_PRESCALE		7
+
+struct x1600_pwm {
+	void __iomem *base;
+	struct clk *clk;
+};
+
+struct x1600_pwm_wfhw {
+	u32 wcfg;
+	u8 prescale;
+	bool enabled;
+	bool inl;
+	bool idl;
+};
+
+static u64 x1600_pwm_counts(u64 ns, unsigned long rate, unsigned int prescale)
+{
+	return mul_u64_u64_div_u64(ns, rate, (u64)NSEC_PER_SEC << prescale);
+}
+
+static u64 x1600_pwm_ns(u64 counts, unsigned long rate, unsigned int prescale)
+{
+	return DIV64_U64_ROUND_UP((counts * NSEC_PER_SEC) << prescale, rate);
+}
+
+static int x1600_pwm_round_waveform_tohw(struct pwm_chip *chip, struct pwm_device *pwm,
+					 const struct pwm_waveform *wf, void *_wfhw)
+{
+	struct x1600_pwm *x = pwmchip_get_drvdata(chip);
+	unsigned long rate = clk_get_rate(x->clk);
+	struct x1600_pwm_wfhw *wfhw = _wfhw;
+	u64 period, high, offset;
+	unsigned int prescale;
+	int ret = 0;
+
+	*wfhw = (struct x1600_pwm_wfhw){ };
+	if (!wf->period_length_ns)
+		return 0;
+
+	if (!rate)
+		return -EINVAL;
+
+	for (prescale = 0; prescale < X1600_PWM_MAX_PRESCALE; prescale++)
+		if (x1600_pwm_counts(wf->period_length_ns, rate, prescale) <=
+		    X1600_PWM_MAX_COUNT)
+			break;
+
+	period = min(x1600_pwm_counts(wf->period_length_ns, rate, prescale),
+		     X1600_PWM_MAX_COUNT);
+	if (period < 2) {
+		period = 2;
+		ret = 1;
+	}
+	high = min(x1600_pwm_counts(wf->duty_length_ns, rate, prescale), period);
+	offset = x1600_pwm_counts(wf->duty_offset_ns, rate, prescale);
+
+	wfhw->prescale = prescale;
+	wfhw->wcfg = FIELD_PREP(X1600_PWM_WCFG_HIGH, high) |
+		     FIELD_PREP(X1600_PWM_WCFG_LOW, period - high);
+	wfhw->enabled = high && high < period;
+	/* Low first only if the requested offset reaches period - duty. */
+	wfhw->inl = !wfhw->enabled || offset < period - high;
+	wfhw->idl = high == period;
+
+	return ret;
+}
+
+static int x1600_pwm_round_waveform_fromhw(struct pwm_chip *chip, struct pwm_device *pwm,
+					   const void *_wfhw, struct pwm_waveform *wf)
+{
+	struct x1600_pwm *x = pwmchip_get_drvdata(chip);
+	unsigned long rate = clk_get_rate(x->clk);
+	const struct x1600_pwm_wfhw *wfhw = _wfhw;
+	u32 high = FIELD_GET(X1600_PWM_WCFG_HIGH, wfhw->wcfg);
+	u32 low = FIELD_GET(X1600_PWM_WCFG_LOW, wfhw->wcfg);
+
+	*wf = (struct pwm_waveform){ };
+
+	/*
+	 * A channel that is off drives its idle level. Both counts set means
+	 * it was stopped with a waveform still loaded: it is off. One count
+	 * zero is how 0 and 100% are written, and the idle level says which.
+	 */
+	if (!wfhw->enabled && high && low)
+		return 0;
+
+	if (!rate)
+		return -EINVAL;
+
+	wf->period_length_ns = x1600_pwm_ns(high + low, rate, wfhw->prescale);
+	if (!wfhw->enabled) {
+		wf->duty_length_ns = wfhw->idl ? wf->period_length_ns : 0;
+		return 0;
+	}
+
+	wf->duty_length_ns = x1600_pwm_ns(high, rate, wfhw->prescale);
+	wf->duty_offset_ns = wfhw->inl ? 0 : x1600_pwm_ns(low, rate, wfhw->prescale);
+
+	return 0;
+}
+
+static int x1600_pwm_read_waveform(struct pwm_chip *chip, struct pwm_device *pwm,
+				   void *_wfhw)
+{
+	struct x1600_pwm *x = pwmchip_get_drvdata(chip);
+	struct x1600_pwm_wfhw *wfhw = _wfhw;
+	unsigned int ch = pwm->hwpwm;
+
+	wfhw->enabled = readl(x->base + X1600_PWM_EN) & BIT(ch);
+	wfhw->inl = readl(x->base + X1600_PWM_INL) & BIT(ch);
+	wfhw->idl = readl(x->base + X1600_PWM_IDL) & BIT(ch);
+	wfhw->prescale = FIELD_GET(X1600_PWM_CCFG_PRESCALE,
+				   readl(x->base + X1600_PWM_CCFG(ch)));
+	wfhw->wcfg = readl(x->base + X1600_PWM_WCFG(ch));
+
+	return 0;
+}
+
+static void x1600_pwm_set_bit(struct x1600_pwm *x, unsigned int reg,
+			      unsigned int ch, bool set)
+{
+	u32 val = readl(x->base + reg);
+
+	if (set)
+		val |= BIT(ch);
+	else
+		val &= ~BIT(ch);
+	writel(val, x->base + reg);
+}
+
+static int x1600_pwm_write_waveform(struct pwm_chip *chip, struct pwm_device *pwm,
+				    const void *_wfhw)
+{
+	struct x1600_pwm *x = pwmchip_get_drvdata(chip);
+	const struct x1600_pwm_wfhw *wfhw = _wfhw;
+	unsigned long rate = clk_get_rate(x->clk);
+	struct x1600_pwm_wfhw cur;
+	unsigned int ch = pwm->hwpwm;
+	u32 busy;
+
+	if (!rate)
+		return -EINVAL;
+
+	x1600_pwm_read_waveform(chip, pwm, &cur);
+
+	/*
+	 * Running with the same divider and initial level: load the new word,
+	 * which the channel takes at the end of its period.
+	 */
+	if (cur.enabled && wfhw->enabled && cur.prescale == wfhw->prescale &&
+	    cur.inl == wfhw->inl) {
+		writel(wfhw->wcfg, x->base + X1600_PWM_WCFG(ch));
+		writel(BIT(ch), x->base + X1600_PWM_UPT);
+		return readl_poll_timeout(x->base + X1600_PWM_BUSY, busy,
+					  !(busy & BIT(ch)), 10,
+					  DIV_ROUND_UP_ULL(2ULL * USEC_PER_SEC *
+							   (X1600_PWM_MAX_COUNT << cur.prescale),
+							   rate));
+	}
+
+	/* The idle level first, so that stopping holds the right level. */
+	x1600_pwm_set_bit(x, X1600_PWM_IDL, ch, wfhw->idl);
+	if (cur.enabled)
+		writel(BIT(ch), x->base + X1600_PWM_ENC);
+
+	/* The prescaler only changes while the channel is off. */
+	writel(FIELD_PREP(X1600_PWM_CCFG_PRESCALE, wfhw->prescale),
+	       x->base + X1600_PWM_CCFG(ch));
+	x1600_pwm_set_bit(x, X1600_PWM_INL, ch, wfhw->inl);
+	writel(wfhw->wcfg, x->base + X1600_PWM_WCFG(ch));
+	x1600_pwm_set_bit(x, X1600_PWM_MS, ch, false);
+
+	if (wfhw->enabled)
+		writel(BIT(ch), x->base + X1600_PWM_ENS);
+
+	return 0;
+}
+
+static const struct pwm_ops x1600_pwm_ops = {
+	.sizeof_wfhw = sizeof(struct x1600_pwm_wfhw),
+	.round_waveform_tohw = x1600_pwm_round_waveform_tohw,
+	.round_waveform_fromhw = x1600_pwm_round_waveform_fromhw,
+	.read_waveform = x1600_pwm_read_waveform,
+	.write_waveform = x1600_pwm_write_waveform,
+};
+
+static int x1600_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pwm_chip *chip;
+	struct x1600_pwm *x;
+
+	chip = devm_pwmchip_alloc(dev, X1600_PWM_NUM, sizeof(*x));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+	x = pwmchip_get_drvdata(chip);
+
+	x->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(x->base))
+		return PTR_ERR(x->base);
+
+	x->clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(x->clk))
+		return dev_err_probe(dev, PTR_ERR(x->clk), "Unable to get clock\n");
+
+	chip->ops = &x1600_pwm_ops;
+	return devm_pwmchip_add(dev, chip);
+}
+
+static const struct of_device_id x1600_pwm_of_match[] = {
+	{ .compatible = "ingenic,x1600-pwm" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, x1600_pwm_of_match);
+
+static struct platform_driver x1600_pwm_driver = {
+	.driver = {
+		.name = "ingenic-x1600-pwm",
+		.of_match_table = x1600_pwm_of_match,
+	},
+	.probe = x1600_pwm_probe,
+};
+module_platform_driver(x1600_pwm_driver);
+
+MODULE_AUTHOR("Olivier Louvignes <olivier@mgcrea.io>");
+MODULE_DESCRIPTION("Ingenic X1600 PWM driver");
+MODULE_LICENSE("GPL");
-- 
2.39.5

Re: [PATCH v2 2/2] pwm: Add a driver for the Ingenic X1600 PWM controller

[Uwe Kleine-König]

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

Hello,

On Sun, Oct 25, 2026 at 08:16:24PM +0100, Olivier Louvignes wrote:
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 729776086879..244c8f4f140e 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -381,6 +381,17 @@ config PWM_JZ4740
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-jz4740.
>  
> +config PWM_INGENIC_X1600

Alphabetic ordering please.

> +	tristate "Ingenic X1600 PWM support"
> +	depends on MACH_INGENIC || COMPILE_TEST
> +	depends on COMMON_CLK && OF && HAS_IOMEM
> +	help
> +	  Generic PWM framework driver for the PWM controller of the
> +	  Ingenic X1600.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-ingenic-x1600.
> +
>  config PWM_KEEMBAY
>  	tristate "Intel Keem Bay PWM driver"
>  	depends on ARCH_KEEMBAY || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 5630a521a7cf..5c59b54e0029 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -31,6 +31,7 @@ obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
>  obj-$(CONFIG_PWM_INTEL_LGM)	+= pwm-intel-lgm.o
>  obj-$(CONFIG_PWM_IPQ)		+= pwm-ipq.o
>  obj-$(CONFIG_PWM_IQS620A)	+= pwm-iqs620a.o
> +obj-$(CONFIG_PWM_INGENIC_X1600)	+= pwm-ingenic-x1600.o

Alphabetic ordering please.

>  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
>  obj-$(CONFIG_PWM_KEEMBAY)	+= pwm-keembay.o
>  obj-$(CONFIG_PWM_LOONGSON)	+= pwm-loongson.o
> diff --git a/drivers/pwm/pwm-ingenic-x1600.c b/drivers/pwm/pwm-ingenic-x1600.c
> new file mode 100644
> index 000000000000..5865ff0cf298
> --- /dev/null
> +++ b/drivers/pwm/pwm-ingenic-x1600.c
> @@ -0,0 +1,280 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PWM controller of the Ingenic X1600
> + *
> + * Copyright (c) 2026 Olivier Louvignes <olivier@mgcrea.io>
> + *
> + * Eight channels, each with a power-of-two clock divider and a waveform word
> + * holding the counts at the high and at the low level; the channel's initial
> + * level picks which comes first (X1600 programming manual, chapter 13). Unlike
> + * the JZ47xx and X1000, whose PWMs are TCU timer channels (pwm-jz4740), the
> + * X1600 has this separate controller.

A link to a reference manual would be awesome (if available).

> + * Limitations:
> + * - The duty offset is 0 or period - duty (the high level last).
> + * - Neither count may be zero, so a duty cycle of 0 or 100% switches the
> + *   channel off and holds its output at the idle level instead.

That reads like 100% wasn't possible, but looking at the code it is?!

> + * - A disabled channel drives its idle level, low.
> + * - Changing the divider or the initial level stops the channel first, which
> + *   cuts the current period short.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/math64.h>
> +#include <linux/minmax.h>
> +#include <linux/mod_devicetable.h>

That is wrong, of_device_id is already provided by
<linux/platform_device.h> and <linux/mod_devicetable.h> pulls in many
unneeded dependencies. So just drop the latter.

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/time.h>
> +#include <linux/types.h>
> +
> +#define X1600_PWM_ENS			0x00	/* write 1: switch a channel on */
> +#define X1600_PWM_ENC			0x04	/* write 1: switch a channel off */
> +#define X1600_PWM_EN			0x08	/* the channels switched on */
> +#define X1600_PWM_UPT			0x10	/* write 1: load the waveform word */
> +#define X1600_PWM_BUSY			0x14	/* a load is pending */
> +#define X1600_PWM_MS			0x20	/* 0: waveform word, 1: DMA */
> +#define X1600_PWM_INL			0x24	/* the level a period starts with */
> +#define X1600_PWM_IDL			0x28	/* the level while off */
> +#define X1600_PWM_CCFG(n)		(0x40 + 4 * (n))
> +#define X1600_PWM_WCFG(n)		(0x80 + 4 * (n))
> +
> +#define X1600_PWM_CCFG_PRESCALE		GENMASK(3, 0)	/* divide by 2^PRESCALE */
> +#define X1600_PWM_WCFG_HIGH		GENMASK(31, 16)
> +#define X1600_PWM_WCFG_LOW		GENMASK(15, 0)
> +
> +#define X1600_PWM_NUM			8
> +#define X1600_PWM_MAX_COUNT		0xffff

This can be determined from X1600_PWM_WCFG_HIGH and/or
X1600_PWM_WCFG_LOW using FIELD_MAX().

> +#define X1600_PWM_MAX_PRESCALE		7

Why are there 4 register bits to store a value in the range [0 .. 7]?

> +struct x1600_pwm {
> +	void __iomem *base;
> +	struct clk *clk;
> +};
> +
> +struct x1600_pwm_wfhw {
> +	u32 wcfg;
> +	u8 prescale;
> +	bool enabled;
> +	bool inl;
> +	bool idl;
> +};
> +
> +static u64 x1600_pwm_counts(u64 ns, unsigned long rate, unsigned int prescale)
> +{
> +	return mul_u64_u64_div_u64(ns, rate, (u64)NSEC_PER_SEC << prescale);
> +}
> +
> +static u64 x1600_pwm_ns(u64 counts, unsigned long rate, unsigned int prescale)
> +{

Please add a comment about the ranges of `counts` and `prescale` here to
make it clear that there are no problems with overflows.

> +	return DIV64_U64_ROUND_UP((counts * NSEC_PER_SEC) << prescale, rate);
> +}
> +
> +static int x1600_pwm_round_waveform_tohw(struct pwm_chip *chip, struct pwm_device *pwm,
> +					 const struct pwm_waveform *wf, void *_wfhw)
> +{
> +	struct x1600_pwm *x = pwmchip_get_drvdata(chip);
> +	unsigned long rate = clk_get_rate(x->clk);
> +	struct x1600_pwm_wfhw *wfhw = _wfhw;
> +	u64 period, high, offset;
> +	unsigned int prescale;
> +	int ret = 0;
> +
> +	*wfhw = (struct x1600_pwm_wfhw){ };
> +	if (!wf->period_length_ns)
> +		return 0;
> +
> +	if (!rate)
> +		return -EINVAL;
> +
> +	for (prescale = 0; prescale < X1600_PWM_MAX_PRESCALE; prescale++)
> +		if (x1600_pwm_counts(wf->period_length_ns, rate, prescale) <=
> +		    X1600_PWM_MAX_COUNT)
> +			break;

I'm sure prescale can be calculated without a loop.

> +	period = min(x1600_pwm_counts(wf->period_length_ns, rate, prescale),
> +		     X1600_PWM_MAX_COUNT);

After determining prescale, you might already have calculated period.

> +	if (period < 2) {
> +		period = 2;
> +		ret = 1;
> +	}
> +	high = min(x1600_pwm_counts(wf->duty_length_ns, rate, prescale), period);
> +	offset = x1600_pwm_counts(wf->duty_offset_ns, rate, prescale);
> +
> +	wfhw->prescale = prescale;
> +	wfhw->wcfg = FIELD_PREP(X1600_PWM_WCFG_HIGH, high) |
> +		     FIELD_PREP(X1600_PWM_WCFG_LOW, period - high);
> +	wfhw->enabled = high && high < period;
> +	/* Low first only if the requested offset reaches period - duty. */
> +	wfhw->inl = !wfhw->enabled || offset < period - high;
> +	wfhw->idl = high == period;

inl and idl are the levels before and after the (potential) mid-period
edge, right? Doesn't idl need to depend on offset, too?

> +	return ret;
> +}
> +
> +static int x1600_pwm_round_waveform_fromhw(struct pwm_chip *chip, struct pwm_device *pwm,
> +					   const void *_wfhw, struct pwm_waveform *wf)
> +{
> +	struct x1600_pwm *x = pwmchip_get_drvdata(chip);
> +	unsigned long rate = clk_get_rate(x->clk);
> +	const struct x1600_pwm_wfhw *wfhw = _wfhw;
> +	u32 high = FIELD_GET(X1600_PWM_WCFG_HIGH, wfhw->wcfg);
> +	u32 low = FIELD_GET(X1600_PWM_WCFG_LOW, wfhw->wcfg);
> +
> +	*wf = (struct pwm_waveform){ };
> +
> +	/*
> +	 * A channel that is off drives its idle level. Both counts set means
> +	 * it was stopped with a waveform still loaded: it is off. One count
> +	 * zero is how 0 and 100% are written, and the idle level says which.
> +	 */
> +	if (!wfhw->enabled && high && low)
> +		return 0;

The comment doesn't really match the code, does it? The return value
only ever says 0% relative duty, but the comment also talks about 100%.

> +	if (!rate)
> +		return -EINVAL;
> +
> +	wf->period_length_ns = x1600_pwm_ns(high + low, rate, wfhw->prescale);
> +	if (!wfhw->enabled) {
> +		wf->duty_length_ns = wfhw->idl ? wf->period_length_ns : 0;

That is wrong if idl = inl?

> +		return 0;
> +	}
> +
> +	wf->duty_length_ns = x1600_pwm_ns(high, rate, wfhw->prescale);
> +	wf->duty_offset_ns = wfhw->inl ? 0 : x1600_pwm_ns(low, rate, wfhw->prescale);
> +
> +	return 0;
> +}
> +
> [...]
> +static int x1600_pwm_write_waveform(struct pwm_chip *chip, struct pwm_device *pwm,
> +				    const void *_wfhw)
> +{
> +	struct x1600_pwm *x = pwmchip_get_drvdata(chip);
> +	const struct x1600_pwm_wfhw *wfhw = _wfhw;
> +	unsigned long rate = clk_get_rate(x->clk);
> +	struct x1600_pwm_wfhw cur;
> +	unsigned int ch = pwm->hwpwm;
> +	u32 busy;
> +
> +	if (!rate)
> +		return -EINVAL;
> +
> +	x1600_pwm_read_waveform(chip, pwm, &cur);
> +
> +	/*
> +	 * Running with the same divider and initial level: load the new word,
> +	 * which the channel takes at the end of its period.
> +	 */
> +	if (cur.enabled && wfhw->enabled && cur.prescale == wfhw->prescale &&
> +	    cur.inl == wfhw->inl) {

You also need cur.idl = wfhw->idl I think.

> +		writel(wfhw->wcfg, x->base + X1600_PWM_WCFG(ch));
> +		writel(BIT(ch), x->base + X1600_PWM_UPT);
> +		return readl_poll_timeout(x->base + X1600_PWM_BUSY, busy,
> +					  !(busy & BIT(ch)), 10,
> +					  DIV_ROUND_UP_ULL(2ULL * USEC_PER_SEC *
> +							   (X1600_PWM_MAX_COUNT << cur.prescale),
> +							   rate));

What is the rationale here? You can return without waiting here even if
the setting isn't active yet. But maybe you have to delay writing if the
busy bit is still set on the next invocation of .write_waveform().

> +	}
> +
> +	/* The idle level first, so that stopping holds the right level. */
> +	x1600_pwm_set_bit(x, X1600_PWM_IDL, ch, wfhw->idl);
> +	if (cur.enabled)
> +		writel(BIT(ch), x->base + X1600_PWM_ENC);
> +
> +	/* The prescaler only changes while the channel is off. */
> +	writel(FIELD_PREP(X1600_PWM_CCFG_PRESCALE, wfhw->prescale),
> +	       x->base + X1600_PWM_CCFG(ch));
> +	x1600_pwm_set_bit(x, X1600_PWM_INL, ch, wfhw->inl);
> +	writel(wfhw->wcfg, x->base + X1600_PWM_WCFG(ch));
> +	x1600_pwm_set_bit(x, X1600_PWM_MS, ch, false);
> +
> +	if (wfhw->enabled)
> +		writel(BIT(ch), x->base + X1600_PWM_ENS);
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops x1600_pwm_ops = {
> +	.sizeof_wfhw = sizeof(struct x1600_pwm_wfhw),
> +	.round_waveform_tohw = x1600_pwm_round_waveform_tohw,
> +	.round_waveform_fromhw = x1600_pwm_round_waveform_fromhw,
> +	.read_waveform = x1600_pwm_read_waveform,
> +	.write_waveform = x1600_pwm_write_waveform,
> +};
> +
> +static int x1600_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct pwm_chip *chip;
> +	struct x1600_pwm *x;
> +
> +	chip = devm_pwmchip_alloc(dev, X1600_PWM_NUM, sizeof(*x));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +	x = pwmchip_get_drvdata(chip);
> +
> +	x->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(x->base))
> +		return PTR_ERR(x->base);
> +
> +	x->clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(x->clk))
> +		return dev_err_probe(dev, PTR_ERR(x->clk), "Unable to get clock\n");

Please make use of clk_rate_exclusive_get().

If you can write a new configuration even if the busy bit is active you
can also call clk_get_rate() here, cache the result and then set
chip->atomic.

> +
> +	chip->ops = &x1600_pwm_ops;
> +	return devm_pwmchip_add(dev, chip);
> +}
> +
> +static const struct of_device_id x1600_pwm_of_match[] = {
> +	{ .compatible = "ingenic,x1600-pwm" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, x1600_pwm_of_match);
> +
> +static struct platform_driver x1600_pwm_driver = {
> +	.driver = {
> +		.name = "ingenic-x1600-pwm",
> +		.of_match_table = x1600_pwm_of_match,
> +	},
> +	.probe = x1600_pwm_probe,
> +};
> +module_platform_driver(x1600_pwm_driver);
> +
> +MODULE_AUTHOR("Olivier Louvignes <olivier@mgcrea.io>");
> +MODULE_DESCRIPTION("Ingenic X1600 PWM driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.39.5
> 

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