[PATCH v2 0/4] Introduce Allwinner H616 PWM controller

[PATCH v2 0/4] Introduce Allwinner H616 PWM controller

[Richard Genoud]

Allwinner H616 PWM controller is quite different from the A10 one.

It can drive 6 PWM channels, and like for the A10, each channel has a
bypass that permits to output a clock, bypassing the PWM logic, when
enabled.

But, the channels are paired 2 by 2, sharing a first set of
MUX/prescaler/gate.
Then, for each channel, there's another prescaler (that will be bypassed
if the bypass is enabled for this channel).

It looks like that:
            _____      ______      ________
OSC24M --->|     |    |      |    |        |
APB1 ----->| Mux |--->| Gate |--->| /div_m |-----> PWM_clock_src_xy
           |_____|    |______|    |________|
                          ________
                         |        |
                      +->| /div_k |---> PWM_clock_x
                      |  |________|
                      |    ______
                      |   |      |
                      +-->| Gate |----> PWM_bypass_clock_x
                      |   |______|
PWM_clock_src_xy -----+   ________
                      |  |        |
                      +->| /div_k |---> PWM_clock_y
                      |  |________|
                      |    ______
                      |   |      |
                      +-->| Gate |----> PWM_bypass_clock_y
                          |______|

Where xy can be 0/1, 2/3, 4/5

PWM_clock_x/y serve for the PWM purpose.
PWM_bypass_clock_x/y serve for the clock-provider purpose.
The common clock framework has been used to manage those clocks.

This PWM driver serves as a clock-provider for PWM_bypass_clocks.
This is needed for example by the embedded AC300 PHY which clock comes
from PMW5 pin (PB12).

This series is based onto v6.19-rc1

Changes since v1:
- rebase onto v6.19-rc1
- add missing headers
- remove MODULE_ALIAS (suggested by Krzysztof)
- use sun4i-pwm binding instead of creating a new one (suggested by Krzysztof)
- retrieve the parent clocks from the devicetree
- switch num_parents to unsigned int

Richard Genoud (4):
  dt-bindings: pwm: allwinner: add h616 pwm compatible
  pwm: sun50i: Add H616 PWM support
  arm64: dts: allwinner: h616: add PWM controller
  MAINTAINERS: Add entry on Allwinner H616 PWM driver

 .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml |  19 +-
 MAINTAINERS                                   |   5 +
 .../arm64/boot/dts/allwinner/sun50i-h616.dtsi |  47 +
 drivers/pwm/Kconfig                           |  12 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-sun50i-h616.c                 | 892 ++++++++++++++++++
 6 files changed, 975 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pwm/pwm-sun50i-h616.c


base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
-- 
2.47.3

[PATCH v2 1/4] dt-bindings: pwm: allwinner: add h616 pwm compatible

[Richard Genoud]

Allwinner H616 PWM block is quite different from the A10 or H6, but at
the end, it uses the same clocks as the H6; so the sun4i pwm binding can
be used.
It has 6 channels than can generate PWM waveforms or clocks if bypass is
enabled.

Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>
Signed-off-by: Richard Genoud <richard.genoud@bootlin.com>
---
 .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
index 1197858e431f..4f58110ec98f 100644
--- a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
+++ b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
@@ -14,6 +14,9 @@ properties:
   "#pwm-cells":
     const: 3
 
+  "#clock-cells":
+    const: 1
+
   compatible:
     oneOf:
       - const: allwinner,sun4i-a10-pwm
@@ -36,6 +39,7 @@ properties:
           - const: allwinner,sun50i-h5-pwm
           - const: allwinner,sun5i-a13-pwm
       - const: allwinner,sun50i-h6-pwm
+      - const: allwinner,sun50i-h616-pwm
 
   reg:
     maxItems: 1
@@ -62,7 +66,9 @@ allOf:
       properties:
         compatible:
           contains:
-            const: allwinner,sun50i-h6-pwm
+            enum:
+              - allwinner,sun50i-h6-pwm
+              - allwinner,sun50i-h616-pwm
 
     then:
       properties:
@@ -83,6 +89,17 @@ allOf:
         clocks:
           maxItems: 1
 
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              const: allwinner,sun50i-h616-pwm
+
+    then:
+      properties:
+        "#clock-cells": false
+
 required:
   - compatible
   - reg
-- 
2.47.3

Re: [PATCH v2 1/4] dt-bindings: pwm: allwinner: add h616 pwm compatible

[Krzysztof Kozlowski]

On 17/12/2025 09:25, Richard Genoud wrote:
> Allwinner H616 PWM block is quite different from the A10 or H6, but at
> the end, it uses the same clocks as the H6; so the sun4i pwm binding can
> be used.
> It has 6 channels than can generate PWM waveforms or clocks if bypass is
> enabled.
> 
> Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>

Hm? Where did I suggest anything about adding clock cells or documenting
this allwinner device?


Best regards,
Krzysztof

Re: [PATCH v2 1/4] dt-bindings: pwm: allwinner: add h616 pwm compatible

[Richard GENOUD]

Le 18/12/2025 à 11:15, Krzysztof Kozlowski a écrit :
> On 17/12/2025 09:25, Richard Genoud wrote:
>> Allwinner H616 PWM block is quite different from the A10 or H6, but at
>> the end, it uses the same clocks as the H6; so the sun4i pwm binding can
>> be used.
>> It has 6 channels than can generate PWM waveforms or clocks if bypass is
>> enabled.
>>
>> Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> Hm? Where did I suggest anything about adding clock cells or documenting
> this allwinner device?
Hum, indeed.
The suggestion, as I understood it, was to re-use the sun4i PWM binding, 
which is why this patch exists now. But I agree, the suggestion wasn't 
on the content itself.
Sorry if it's misleading.

> 
> 
> Best regards,
> Krzysztof

Regards,
Richard

Re: [PATCH v2 1/4] dt-bindings: pwm: allwinner: add h616 pwm compatible

[Jernej Škrabec]

Dne sreda, 17. december 2025 ob 09:25:01 Srednjeevropski standardni čas je Richard Genoud napisal(a):
> Allwinner H616 PWM block is quite different from the A10 or H6, but at
> the end, it uses the same clocks as the H6; so the sun4i pwm binding can
> be used.
> It has 6 channels than can generate PWM waveforms or clocks if bypass is
> enabled.
> 
> Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Richard Genoud <richard.genoud@bootlin.com>
> ---
>  .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> index 1197858e431f..4f58110ec98f 100644
> --- a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> @@ -14,6 +14,9 @@ properties:
>    "#pwm-cells":
>      const: 3
>  
> +  "#clock-cells":
> +    const: 1

Why #clock-cells? I don't see any reason to add it.

Other properties seem fine.

Best regards,
Jernej

> +
>    compatible:
>      oneOf:
>        - const: allwinner,sun4i-a10-pwm
> @@ -36,6 +39,7 @@ properties:
>            - const: allwinner,sun50i-h5-pwm
>            - const: allwinner,sun5i-a13-pwm
>        - const: allwinner,sun50i-h6-pwm
> +      - const: allwinner,sun50i-h616-pwm
>  
>    reg:
>      maxItems: 1
> @@ -62,7 +66,9 @@ allOf:
>        properties:
>          compatible:
>            contains:
> -            const: allwinner,sun50i-h6-pwm
> +            enum:
> +              - allwinner,sun50i-h6-pwm
> +              - allwinner,sun50i-h616-pwm
>  
>      then:
>        properties:
> @@ -83,6 +89,17 @@ allOf:
>          clocks:
>            maxItems: 1
>  
> +  - if:
> +      not:
> +        properties:
> +          compatible:
> +            contains:
> +              const: allwinner,sun50i-h616-pwm
> +
> +    then:
> +      properties:
> +        "#clock-cells": false
> +
>  required:
>    - compatible
>    - reg
> 

[PATCH v2 2/4] pwm: sun50i: Add H616 PWM support

[Richard Genoud]

Add driver for Allwinner H616 PWM controller, supporting up to 6
channels.
Those channels output can be either a PWM signal output or a clock
output, thanks to the bypass.

The channels are paired (0/1, 2/3 and 4/5) and each pair has a
prescaler/mux/gate.
Moreover, each channel has its own prescaler and bypass.

Tested-by: Joao Schim <joao@schimsalabim.eu>
Signed-off-by: Richard Genoud <richard.genoud@bootlin.com>
---
 drivers/pwm/Kconfig           |  12 +
 drivers/pwm/Makefile          |   1 +
 drivers/pwm/pwm-sun50i-h616.c | 892 ++++++++++++++++++++++++++++++++++
 3 files changed, 905 insertions(+)
 create mode 100644 drivers/pwm/pwm-sun50i-h616.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 6f3147518376..66534e033761 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -736,6 +736,18 @@ config PWM_SUN4I
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-sun4i.
 
+config PWM_SUN50I_H616
+	tristate "Allwinner H616 PWM support"
+	depends on ARCH_SUNXI || COMPILE_TEST
+	depends on HAS_IOMEM && COMMON_CLK
+	help
+	  Generic PWM framework driver for Allwinner H616 SoCs.
+	  It supports generic PWM, but can also provides a plain clock.
+	  The AC300 PHY integrated in H616 SoC needs such a clock.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-h616.
+
 config PWM_SUNPLUS
 	tristate "Sunplus PWM support"
 	depends on ARCH_SUNPLUS || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 0dc0d2b69025..a16ae9eef9e5 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
 obj-$(CONFIG_PWM_STM32_LP)	+= pwm-stm32-lp.o
 obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
 obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
+obj-$(CONFIG_PWM_SUN50I_H616)	+= pwm-sun50i-h616.o
 obj-$(CONFIG_PWM_SUNPLUS)	+= pwm-sunplus.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
 obj-$(CONFIG_PWM_TH1520)	+= pwm_th1520.o
diff --git a/drivers/pwm/pwm-sun50i-h616.c b/drivers/pwm/pwm-sun50i-h616.c
new file mode 100644
index 000000000000..b002ea5935e4
--- /dev/null
+++ b/drivers/pwm/pwm-sun50i-h616.c
@@ -0,0 +1,892 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for Allwinner H616 Pulse Width Modulation Controller
+ *
+ * (C) Copyright 2025 Richard Genoud, Bootlin <richard.genoud@bootlin.com>
+ *
+ * Based on drivers/pwm/pwm-sun4i.c with Copyright:
+ *
+ * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@bootlin.com>
+ *
+ * Limitations:
+ * - When outputing the source clock directly, the PWM logic is bypassed and the
+ *   currently running period is not guaranteed to be completed.
+ * - As the channels are paired (0/1, 2/3, 4/5), they share the same clock
+ *   source and prescaler(div_m), but they also have their own prescaler(div_k)
+ *   and bypass.
+ *
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/reset.h>
+#include <linux/spinlock.h>
+#include <linux/time.h>
+
+#ifndef UINT32_MAX
+#define UINT32_MAX 0xffffffffU
+#endif
+
+/* PWM IRQ Enable Register */
+#define PWM_IER				0x0
+
+/* PWM IRQ Status Register */
+#define PWM_ISR				0x4
+
+/* PWM Capture IRQ Enable Register */
+#define PWM_CIER			0x10
+
+/* PWM Capture IRQ Status Register */
+#define PWM_CISR			0x14
+
+/* PWMCC Pairs Clock Configuration Registers */
+#define PWM_XY_CLK_CR(pair)		(0x20 + ((pair) * 0x4))
+#define PWM_XY_CLK_CR_SRC_SHIFT		7
+#define PWM_XY_CLK_CR_SRC_MASK		1
+#define PWM_XY_CLK_CR_GATE_BIT		4
+#define PWM_XY_CLK_CR_BYPASS_BIT(chan)	((chan) % 2 + 5)
+#define PWM_XY_CLK_CR_DIV_M_SHIFT	0
+
+/* PWMCC Pairs Dead Zone Control Registers */
+#define PWM_XY_DZ(pair)			(0x30 + ((pair) * 0x4))
+
+/* PWM Enable Register */
+#define PWM_ENR				0x40
+#define PWM_ENABLE(x)			BIT(x)
+
+/* PWM Capture Enable Register */
+#define PWM_CER				0x44
+
+/* PWM Control Register */
+#define PWM_CTRL_REG(chan)		(0x60 + (chan) * 0x20)
+#define PWM_CTRL_PRESCAL_K_SHIFT	0
+#define PWM_CTRL_PRESCAL_K_WIDTH	8
+#define PWM_CTRL_ACTIVE_STATE		BIT(8)
+
+/* PWM Period Register */
+#define PWM_PERIOD_REG(ch)		(0x64 + (ch) * 0x20)
+#define PWM_PERIOD_MASK			GENMASK(31, 16)
+#define PWM_DUTY_MASK			GENMASK(15, 0)
+#define PWM_REG_PERIOD(reg)		(FIELD_GET(PWM_PERIOD_MASK, reg) + 1)
+#define PWM_REG_DUTY(reg)		FIELD_GET(PWM_DUTY_MASK, reg)
+#define PWM_PERIOD(prd)			FIELD_PREP(PWM_PERIOD_MASK, (prd) - 1)
+#define PWM_DUTY(dty)			FIELD_PREP(PWM_DUTY_MASK, dty)
+#define PWM_PERIOD_MAX			FIELD_MAX(PWM_PERIOD_MASK)
+
+
+/* PWM Count Register */
+#define PWM_CNT_REG(x)			(0x68 + (x) * 0x20)
+
+/* PWM Capture Control Register */
+#define PWM_CCR(x)			(0x6c + (x) * 0x20)
+
+/* PWM Capture Rise Lock Register */
+#define PWM_CRLR(x)			(0x70 + (x) * 0x20)
+
+/* PWM Capture Fall Lock Register */
+#define PWM_CFLR(x)			(0x74 + (x) * 0x20)
+
+#define PWM_PAIR_IDX(chan)		((chan) >> 2)
+
+/*
+ * Block diagram of the PWM clock controller:
+ *
+ *             _____      ______      ________
+ * OSC24M --->|     |    |      |    |        |
+ * APB1 ----->| Mux |--->| Gate |--->| /div_m |-----> PWM_clock_src_xy
+ *            |_____|    |______|    |________|
+ *                           ________
+ *                          |        |
+ *                       +->| /div_k |---> PWM_clock_x
+ *                       |  |________|
+ *                       |    ______
+ *                       |   |      |
+ *                       +-->| Gate |----> PWM_bypass_clock_x
+ *                       |   |______|
+ * PWM_clock_src_xy -----+   ________
+ *                       |  |        |
+ *                       +->| /div_k |---> PWM_clock_y
+ *                       |  |________|
+ *                       |    ______
+ *                       |   |      |
+ *                       +-->| Gate |----> PWM_bypass_clock_y
+ *                           |______|
+ *
+ * NB: when the bypass is set, all the PWM logic is bypassed.
+ * So, the duty cycle and polarity can't be modified (we just have a clock).
+ * The bypass in PWM mode is used to achieve a 1/2 duty cycle with the fastest
+ * clock.
+ *
+ * PWM_clock_x/y serve for the PWM purpose.
+ * PWM_bypass_clock_x/y serve for the clock-provider purpose.
+ *
+ */
+
+/*
+ * Table used for /div_m (diviser before obtaining PWM_clock_src_xy)
+ * It's actually CLK_DIVIDER_POWER_OF_TWO, but limited to /256
+ */
+static const struct clk_div_table clk_table_div_m[] = {
+	{ .val = 0, .div = 1, },
+	{ .val = 1, .div = 2, },
+	{ .val = 2, .div = 4, },
+	{ .val = 3, .div = 8, },
+	{ .val = 4, .div = 16, },
+	{ .val = 5, .div = 32, },
+	{ .val = 6, .div = 64, },
+	{ .val = 7, .div = 128, },
+	{ .val = 8, .div = 256, },
+	{ .val = 0, .div = 0, }, /* last entry */
+};
+
+#define PWM_XY_SRC_GATE(_pair, _reg)			\
+struct clk_gate gate_xy_##_pair = {			\
+	.reg = (void *)_reg,				\
+	.bit_idx = PWM_XY_CLK_CR_GATE_BIT,		\
+	.hw.init = &(struct clk_init_data){		\
+		.ops =  &clk_gate_ops,			\
+	}						\
+}
+
+#define PWM_XY_SRC_MUX(_pair, _reg)			\
+struct clk_mux mux_xy_##_pair = {			\
+	.reg = (void *)_reg,				\
+	.shift = PWM_XY_CLK_CR_SRC_SHIFT,		\
+	.mask = PWM_XY_CLK_CR_SRC_MASK,			\
+	.flags = CLK_MUX_ROUND_CLOSEST,			\
+	.hw.init = &(struct clk_init_data){		\
+		.ops =  &clk_mux_ops,			\
+	}						\
+}
+
+#define PWM_XY_SRC_DIV(_pair, _reg)			\
+struct clk_divider rate_xy_##_pair = {			\
+	.reg = (void *)_reg,				\
+	.shift = PWM_XY_CLK_CR_DIV_M_SHIFT,		\
+	.table = clk_table_div_m,			\
+	.hw.init = &(struct clk_init_data){		\
+		.ops =  &clk_divider_ops,		\
+	}						\
+}
+
+#define PWM_X_DIV(_idx, _reg)				\
+struct clk_divider rate_x_##_idx = {			\
+	.reg = (void *)_reg,				\
+	.shift = PWM_CTRL_PRESCAL_K_SHIFT,		\
+	.width = PWM_CTRL_PRESCAL_K_WIDTH,		\
+	.hw.init = &(struct clk_init_data){		\
+		.ops =  &clk_divider_ops,		\
+	}						\
+}
+
+#define PWM_X_BYPASS_GATE(_idx)				\
+struct clk_gate gate_x_bypass_##_idx = {		\
+	.reg = (void *)PWM_ENR,				\
+	.bit_idx = _idx,				\
+	.hw.init = &(struct clk_init_data){		\
+		.ops =  &clk_gate_ops,			\
+	}						\
+}
+
+#define PWM_XY_CLK_SRC(_pair, _reg)			\
+	static PWM_XY_SRC_MUX(_pair, _reg);		\
+	static PWM_XY_SRC_GATE(_pair, _reg);		\
+	static PWM_XY_SRC_DIV(_pair, _reg)
+
+#define PWM_X_CLK(_idx)					\
+	static PWM_X_DIV(_idx, PWM_CTRL_REG(_idx))
+
+#define PWM_X_BYPASS_CLK(_idx)						\
+	PWM_X_BYPASS_GATE(_idx)
+
+#define REF_CLK_XY_SRC(_pair)						\
+	{								\
+		.name = "pwm-clk-src" #_pair,				\
+		.mux_hw = &mux_xy_##_pair.hw,				\
+		.gate_hw = &gate_xy_##_pair.hw,				\
+		.rate_hw = &rate_xy_##_pair.hw,				\
+	}
+
+#define REF_CLK_X(_idx, _pair)						\
+	{								\
+		.name = "pwm-clk" #_idx,				\
+		.parent_names = (const char *[]){ "pwm-clk-src" #_pair }, \
+		.num_parents = 1,					\
+		.rate_hw = &rate_x_##_idx.hw,				\
+		.flags = CLK_SET_RATE_PARENT,				\
+	}
+
+#define REF_CLK_BYPASS(_idx, _pair)					\
+	{								\
+		.name = "pwm-clk-bypass" #_idx,				\
+		.parent_names = (const char *[]){ "pwm-clk-src" #_pair }, \
+		.num_parents = 1,					\
+		.gate_hw = &gate_x_bypass_##_idx.hw,			\
+		.flags = CLK_SET_RATE_PARENT,	\
+	}
+
+/*
+ * PWM_clock_src_xy generation:
+ *             _____      ______      ________
+ * OSC24M --->|     |    |      |    |        |
+ * APB1 ----->| Mux |--->| Gate |--->| /div_m |-----> PWM_clock_src_xy
+ *            |_____|    |______|    |________|
+ */
+PWM_XY_CLK_SRC(01, PWM_XY_CLK_CR(0));
+PWM_XY_CLK_SRC(23, PWM_XY_CLK_CR(1));
+PWM_XY_CLK_SRC(45, PWM_XY_CLK_CR(2));
+
+/*
+ * PWM_clock_x_div generation:
+ *                       ________
+ *                      |        | PWM_clock_x/y
+ * PWM_clock_src_xy --->| /div_k |--------------->
+ *                      |________|
+ */
+PWM_X_CLK(0);
+PWM_X_CLK(1);
+PWM_X_CLK(2);
+PWM_X_CLK(3);
+PWM_X_CLK(4);
+PWM_X_CLK(5);
+
+/*
+ * PWM_bypass_clock_xy generation:
+ *                        ______
+ *                       |      |
+ * PWM_clock_src_xy ---->| Gate |-------> PWM_bypass_clock_x
+ *                       |______|
+ *
+ * The gate is actually PWM_ENR register.
+ */
+PWM_X_BYPASS_CLK(0);
+PWM_X_BYPASS_CLK(1);
+PWM_X_BYPASS_CLK(2);
+PWM_X_BYPASS_CLK(3);
+PWM_X_BYPASS_CLK(4);
+PWM_X_BYPASS_CLK(5);
+
+struct clk_pwm_data {
+	const char *name;
+	const char **parent_names;
+	unsigned int num_parents;
+	struct clk_hw *mux_hw;
+	struct clk_hw *rate_hw;
+	struct clk_hw *gate_hw;
+	unsigned long flags;
+};
+
+#define CLK_BYPASS(h616chip, ch) ((h616chip)->data->npwm + (ch))
+#define CLK_XY_SRC_IDX(h616chip, ch) ((h616chip)->data->npwm * 2 + ((ch) >> 1))
+static struct clk_pwm_data pwmcc_data[] = {
+	REF_CLK_X(0, 01),
+	REF_CLK_X(1, 01),
+	REF_CLK_X(2, 23),
+	REF_CLK_X(3, 23),
+	REF_CLK_X(4, 45),
+	REF_CLK_X(5, 45),
+	REF_CLK_BYPASS(0, 01),
+	REF_CLK_BYPASS(1, 01),
+	REF_CLK_BYPASS(2, 23),
+	REF_CLK_BYPASS(3, 23),
+	REF_CLK_BYPASS(4, 45),
+	REF_CLK_BYPASS(5, 45),
+	REF_CLK_XY_SRC(01),
+	REF_CLK_XY_SRC(23),
+	REF_CLK_XY_SRC(45),
+	{ /* sentinel */ },
+};
+
+enum h616_pwm_mode {
+	H616_PWM_MODE_NONE,
+	H616_PWM_MODE_PWM,
+	H616_PWM_MODE_CLK,
+};
+
+struct h616_pwm_data {
+	unsigned int npwm;
+};
+
+struct h616_pwm_channel {
+	struct clk *pwm_clk;
+	unsigned long rate;
+	unsigned int entire_cycles;
+	unsigned int active_cycles;
+	enum h616_pwm_mode mode;
+	bool bypass;
+};
+
+struct clk_pwm_pdata {
+	struct clk_hw_onecell_data *hw_data;
+	spinlock_t lock;
+	void __iomem *reg;
+};
+
+struct h616_pwm_chip {
+	struct clk_pwm_pdata *clk_pdata;
+	struct h616_pwm_channel *channels;
+	struct clk *bus_clk;
+	struct reset_control *rst;
+	void __iomem *base;
+	const struct h616_pwm_data *data;
+};
+
+static inline struct h616_pwm_chip *to_h616_pwm_chip(struct pwm_chip *chip)
+{
+	return pwmchip_get_drvdata(chip);
+}
+
+static inline u32 h616_pwm_readl(struct h616_pwm_chip *h616chip,
+				 unsigned long offset)
+{
+	return readl(h616chip->base + offset);
+}
+
+static inline void h616_pwm_writel(struct h616_pwm_chip *h616chip,
+				   u32 val, unsigned long offset)
+{
+	writel(val, h616chip->base + offset);
+}
+
+static void h616_pwm_set_bypass(struct h616_pwm_chip *h616chip, unsigned int idx,
+				bool en_bypass)
+{
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&h616chip->clk_pdata->lock, flags);
+
+	val = h616_pwm_readl(h616chip, PWM_XY_CLK_CR(PWM_PAIR_IDX(idx)));
+	if (en_bypass)
+		val |= BIT(PWM_XY_CLK_CR_BYPASS_BIT(idx));
+	else
+		val &= ~BIT(PWM_XY_CLK_CR_BYPASS_BIT(idx));
+
+	h616_pwm_writel(h616chip, val, PWM_XY_CLK_CR(PWM_PAIR_IDX(idx)));
+
+	spin_unlock_irqrestore(&h616chip->clk_pdata->lock, flags);
+}
+
+static int h616_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct h616_pwm_chip *h616chip = to_h616_pwm_chip(chip);
+	struct h616_pwm_channel *chan = &h616chip->channels[pwm->hwpwm];
+	struct device *dev = pwmchip_parent(chip);
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&h616chip->clk_pdata->lock, flags);
+
+	if (chan->mode == H616_PWM_MODE_CLK)
+		ret = -EBUSY;
+	else
+		chan->mode = H616_PWM_MODE_PWM;
+
+	spin_unlock_irqrestore(&h616chip->clk_pdata->lock, flags);
+	if (ret)
+		goto out;
+
+	ret = clk_prepare_enable(chan->pwm_clk);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enable clock %s: %d\n",
+			__clk_get_name(chan->pwm_clk), ret);
+	}
+out:
+	return ret;
+}
+
+static void h616_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct h616_pwm_chip *h616chip = to_h616_pwm_chip(chip);
+	struct h616_pwm_channel *chan = &h616chip->channels[pwm->hwpwm];
+
+	clk_disable_unprepare(chan->pwm_clk);
+	chan->mode = H616_PWM_MODE_NONE;
+}
+
+static int h616_pwm_get_state(struct pwm_chip *chip,
+			      struct pwm_device *pwm,
+			      struct pwm_state *state)
+{
+	struct h616_pwm_chip *h616chip = to_h616_pwm_chip(chip);
+	struct h616_pwm_channel *chan = &h616chip->channels[pwm->hwpwm];
+	u64 clk_rate, tmp;
+	u32 val;
+
+	clk_rate = clk_get_rate(chan->pwm_clk);
+	if (!clk_rate)
+		return -EINVAL;
+
+	val = h616_pwm_readl(h616chip, PWM_ENR);
+	state->enabled = !!(PWM_ENABLE(pwm->hwpwm) & val);
+
+	val = h616_pwm_readl(h616chip, PWM_XY_CLK_CR(PWM_PAIR_IDX(pwm->hwpwm)));
+	if (val & BIT(PWM_XY_CLK_CR_BYPASS_BIT(pwm->hwpwm))) {
+		/*
+		 * When bypass is enabled, the PWM logic is inactive.
+		 * The PWM_clock_src_xy is directly routed to PWM_clock_x
+		 */
+		state->period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, clk_rate);
+		state->duty_cycle = DIV_ROUND_UP_ULL(state->period, 2);
+		state->polarity = PWM_POLARITY_NORMAL;
+		return 0;
+	}
+
+	state->enabled &= !!(BIT(PWM_XY_CLK_CR_GATE_BIT) & val);
+
+	val = h616_pwm_readl(h616chip, PWM_CTRL_REG(pwm->hwpwm));
+	if (val & PWM_CTRL_ACTIVE_STATE)
+		state->polarity = PWM_POLARITY_NORMAL;
+	else
+		state->polarity = PWM_POLARITY_INVERSED;
+
+	val = h616_pwm_readl(h616chip, PWM_PERIOD_REG(pwm->hwpwm));
+
+	tmp = NSEC_PER_SEC * PWM_REG_DUTY(val);
+	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
+
+	tmp = NSEC_PER_SEC * PWM_REG_PERIOD(val);
+	state->period = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
+
+	return 0;
+}
+
+static int h616_pwm_calc(struct pwm_chip *chip, unsigned int idx,
+			 const struct pwm_state *state)
+{
+	struct h616_pwm_chip *h616chip = to_h616_pwm_chip(chip);
+	struct h616_pwm_channel *chan = &h616chip->channels[idx];
+	unsigned int cnt, duty_cnt;
+	unsigned long max_rate;
+	long calc_rate;
+	u64 duty, period, freq;
+
+	duty = state->duty_cycle;
+	period = state->period;
+
+	max_rate = clk_round_rate(chan->pwm_clk, UINT32_MAX);
+
+	dev_dbg(pwmchip_parent(chip), "max_rate: %ld Hz\n", max_rate);
+
+	if ((period * max_rate >= NSEC_PER_SEC) &&
+	    (period * max_rate < 2 * NSEC_PER_SEC) &&
+	    (duty * max_rate * 2 >= NSEC_PER_SEC)) {
+		/*
+		 * If the requested period is to small to be generated by the
+		 * PWM, we can just select the highest clock and bypass the
+		 * PWM logic
+		 */
+		dev_dbg(pwmchip_parent(chip), "Setting bypass (period=%lld)\n",
+			period);
+		freq = div64_u64(NSEC_PER_SEC, period);
+		chan->bypass = true;
+		duty = period / 2;
+	} else {
+		chan->bypass = false;
+		freq = div64_u64(NSEC_PER_SEC * (u64)PWM_PERIOD_MAX, period);
+		if (freq > UINT32_MAX)
+			freq = UINT32_MAX;
+	}
+
+	calc_rate = clk_round_rate(chan->pwm_clk, freq);
+	if (calc_rate <= 0) {
+		dev_err(pwmchip_parent(chip),
+			"Invalid source clock frequency %llu\n", freq);
+		return calc_rate ? calc_rate : -EINVAL;
+	}
+
+	dev_dbg(pwmchip_parent(chip), "calc_rate: %ld Hz\n", calc_rate);
+
+	cnt = mul_u64_u64_div_u64(calc_rate, period, NSEC_PER_SEC);
+	if ((cnt == 0) || (cnt > PWM_PERIOD_MAX)) {
+		dev_err(pwmchip_parent(chip), "Period out of range\n");
+		return -EINVAL;
+	}
+
+	duty_cnt = mul_u64_u64_div_u64(calc_rate, duty, NSEC_PER_SEC);
+
+	if (duty_cnt >= cnt)
+		duty_cnt = cnt - 1;
+
+	dev_dbg(pwmchip_parent(chip), "period=%llu cnt=%u duty=%llu duty_cnt=%u\n",
+		period, cnt, duty, duty_cnt);
+
+	chan->active_cycles = duty_cnt;
+	chan->entire_cycles = cnt;
+
+	chan->rate = calc_rate;
+
+	return 0;
+}
+
+static int h616_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			  const struct pwm_state *state)
+{
+	struct h616_pwm_chip *h616chip = to_h616_pwm_chip(chip);
+	struct h616_pwm_channel *chan = &h616chip->channels[pwm->hwpwm];
+	struct pwm_state cstate;
+	unsigned long flags;
+	u32 val;
+	int ret;
+
+	ret = h616_pwm_calc(chip, pwm->hwpwm, state);
+	if (ret) {
+		dev_err(pwmchip_parent(chip), "period exceeds the maximum value\n");
+		return ret;
+	}
+
+	pwm_get_state(pwm, &cstate);
+
+	ret = clk_set_rate(chan->pwm_clk, chan->rate);
+	if (ret) {
+		dev_err(pwmchip_parent(chip), "failed to set PWM %d clock rate to %lu\n",
+			pwm->hwpwm, chan->rate);
+		return ret;
+	}
+
+	h616_pwm_set_bypass(h616chip, pwm->hwpwm, chan->bypass);
+
+	/*
+	 * If bypass is set, the PWM logic (polarity, duty) can't be applied
+	 */
+
+	if (chan->bypass && (state->polarity == PWM_POLARITY_INVERSED)) {
+		dev_warn(pwmchip_parent(chip),
+			 "Can't set inversed polarity with bypass enabled\n");
+	} else {
+		val = h616_pwm_readl(h616chip, PWM_CTRL_REG(pwm->hwpwm));
+		val &= ~PWM_CTRL_ACTIVE_STATE;
+		if (state->polarity == PWM_POLARITY_NORMAL)
+			val |= PWM_CTRL_ACTIVE_STATE;
+		h616_pwm_writel(h616chip, val, PWM_CTRL_REG(pwm->hwpwm));
+	}
+
+	if (chan->bypass && (state->duty_cycle * 2 != state->period)) {
+		dev_warn(pwmchip_parent(chip),
+			 "Can't set a duty cycle with bypass enabled\n");
+	}
+
+	if (!chan->bypass) {
+		val = PWM_DUTY(chan->active_cycles);
+		val |= PWM_PERIOD(chan->entire_cycles);
+		h616_pwm_writel(h616chip, val, PWM_PERIOD_REG(pwm->hwpwm));
+	}
+
+	if (state->enabled == cstate.enabled)
+		return 0;
+
+	if (cstate.enabled) {
+		unsigned long delay_us;
+
+		/*
+		 * We need a full period to elapse before
+		 * disabling the channel.
+		 */
+		delay_us = DIV_ROUND_UP_ULL(cstate.period, NSEC_PER_USEC);
+		fsleep(delay_us);
+	}
+
+	spin_lock_irqsave(&h616chip->clk_pdata->lock, flags);
+
+	val = h616_pwm_readl(h616chip, PWM_ENR);
+	if (state->enabled)
+		val |= PWM_ENABLE(pwm->hwpwm);
+	else
+		val &= ~PWM_ENABLE(pwm->hwpwm);
+	h616_pwm_writel(h616chip, val, PWM_ENR);
+
+	spin_unlock_irqrestore(&h616chip->clk_pdata->lock, flags);
+
+	return 0;
+}
+
+static const struct pwm_ops h616_pwm_ops = {
+	.apply = h616_pwm_apply,
+	.get_state = h616_pwm_get_state,
+	.request = h616_pwm_request,
+	.free = h616_pwm_free,
+};
+
+static struct clk_hw *h616_pwm_of_clk_get(struct of_phandle_args *clkspec,
+					  void *data)
+{
+	struct h616_pwm_chip *h616chip = data;
+	struct clk_hw_onecell_data *hw_data = h616chip->clk_pdata->hw_data;
+	unsigned int idx = clkspec->args[0];
+	struct h616_pwm_channel *chan;
+	struct clk_hw *ret_clk = NULL;
+	unsigned long flags;
+
+	if (idx >= h616chip->data->npwm)
+		return ERR_PTR(-EINVAL);
+
+	chan = &h616chip->channels[idx];
+
+	spin_lock_irqsave(&h616chip->clk_pdata->lock, flags);
+
+	if (chan->mode == H616_PWM_MODE_PWM) {
+		ret_clk = ERR_PTR(-EBUSY);
+	} else {
+		chan->mode = H616_PWM_MODE_CLK;
+		ret_clk = hw_data->hws[CLK_BYPASS(h616chip, idx)];
+	}
+	spin_unlock_irqrestore(&h616chip->clk_pdata->lock, flags);
+
+	if (IS_ERR(ret_clk))
+		goto out;
+
+	chan->bypass = true;
+	h616_pwm_set_bypass(h616chip, idx, chan->bypass);
+out:
+	return ret_clk;
+}
+
+static int h616_add_composite_clk(struct clk_pwm_data *data,
+				  void __iomem *reg, spinlock_t *lock,
+				  struct device *dev, struct clk_hw **hw)
+{
+	const struct clk_ops *mux_ops = NULL, *gate_ops = NULL, *rate_ops = NULL;
+	struct clk_hw *mux_hw = NULL, *gate_hw = NULL, *rate_hw = NULL;
+	struct device_node *node = dev->of_node;
+
+	if (data->mux_hw) {
+		struct clk_mux *mux;
+
+		mux_hw = data->mux_hw;
+		mux = to_clk_mux(mux_hw);
+		mux->lock = lock;
+		mux_ops = mux_hw->init->ops;
+		mux->reg = (u64)mux->reg + reg;
+	}
+
+	if (data->gate_hw) {
+		struct clk_gate *gate;
+
+		gate_hw = data->gate_hw;
+		gate = to_clk_gate(gate_hw);
+		gate->lock = lock;
+		gate_ops = gate_hw->init->ops;
+		gate->reg = (u64)gate->reg + reg;
+	}
+
+	if (data->rate_hw) {
+		struct clk_divider *rate;
+
+		rate_hw = data->rate_hw;
+		rate = to_clk_divider(rate_hw);
+		rate_ops = rate_hw->init->ops;
+		rate->lock = lock;
+		rate->reg = (u64)rate->reg + reg;
+
+		if (rate->table) {
+			const struct clk_div_table *clkt;
+			int table_size = 0;
+
+			for (clkt = rate->table; clkt->div; clkt++)
+				table_size++;
+			rate->width = order_base_2(table_size);
+		}
+	}
+
+	/*
+	 * Retrieve the parent clock names from DTS for pwm-clk-srcxy
+	 */
+	if (!data->parent_names) {
+		data->num_parents = of_clk_get_parent_count(node);
+		if (data->num_parents == 0)
+			return -ENOENT;
+
+		data->parent_names = devm_kzalloc(dev,
+						  sizeof(*data->parent_names),
+						  GFP_KERNEL);
+		for (unsigned int i = 0; i < data->num_parents; i++)
+			data->parent_names[i] = of_clk_get_parent_name(node, i);
+	}
+
+	*hw = clk_hw_register_composite(dev, data->name, data->parent_names,
+					data->num_parents, mux_hw,
+					mux_ops, rate_hw, rate_ops,
+					gate_hw, gate_ops, data->flags);
+
+	return PTR_ERR_OR_ZERO(*hw);
+}
+
+static int h616_pwm_init_clocks(struct platform_device *pdev,
+				struct h616_pwm_chip *h616chip)
+{
+	struct clk_pwm_pdata *pdata;
+	struct device *dev = &pdev->dev;
+	int num_clocks = 0;
+	int ret;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	while (pwmcc_data[num_clocks].name)
+		num_clocks++;
+
+	pdata->hw_data = devm_kzalloc(dev, struct_size(pdata->hw_data, hws, num_clocks),
+				      GFP_KERNEL);
+	if (!pdata->hw_data)
+		return -ENOMEM;
+
+	pdata->hw_data->num = num_clocks;
+	pdata->reg = h616chip->base;
+
+	spin_lock_init(&pdata->lock);
+
+	for (int i = 0; i < num_clocks; i++) {
+		struct clk_hw **hw = &pdata->hw_data->hws[i];
+
+		ret = h616_add_composite_clk(&pwmcc_data[i], pdata->reg,
+					     &pdata->lock, dev, hw);
+		if (ret) {
+			dev_err_probe(dev, ret,
+				      "Failed to register hw clock %s\n",
+				      pwmcc_data[i].name);
+			for (i--; i >= 0; i--)
+				clk_hw_unregister_composite(pdata->hw_data->hws[i]);
+			return ret;
+		}
+	}
+
+	h616chip->clk_pdata = pdata;
+
+	return 0;
+}
+
+static int h616_pwm_probe(struct platform_device *pdev)
+{
+	const struct h616_pwm_data *data;
+	struct device *dev = &pdev->dev;
+	struct h616_pwm_chip *h616chip;
+	struct pwm_chip *chip;
+	int ret;
+
+	data = of_device_get_match_data(dev);
+	if (!data)
+		return -ENODEV;
+
+	chip = devm_pwmchip_alloc(dev, data->npwm, sizeof(*h616chip));
+	if (IS_ERR(chip))
+		return dev_err_probe(dev, PTR_ERR(chip),
+				     "Failed to allocate pwmchip\n");
+
+	h616chip = to_h616_pwm_chip(chip);
+	h616chip->data = data;
+	h616chip->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(h616chip->base))
+		return dev_err_probe(dev, PTR_ERR(h616chip->base),
+				     "Failed to get PWM base address\n");
+
+	h616chip->bus_clk = devm_clk_get_enabled(dev, "bus");
+	if (IS_ERR(h616chip->bus_clk))
+		return dev_err_probe(dev, PTR_ERR(h616chip->bus_clk),
+				     "Failed to get bus clock\n");
+
+	h616chip->channels = devm_kmalloc_array(dev, data->npwm,
+						sizeof(*(h616chip->channels)),
+						GFP_KERNEL);
+	if (!h616chip->channels)
+		return dev_err_probe(dev, -ENOMEM,
+				     "Failed to allocate %d channels array\n",
+				     data->npwm);
+
+	h616chip->rst = devm_reset_control_get_shared(dev, NULL);
+	if (IS_ERR(h616chip->rst))
+		return dev_err_probe(dev, PTR_ERR(h616chip->rst),
+				     "Failed to get reset control\n");
+
+	chip->ops = &h616_pwm_ops;
+
+	ret = h616_pwm_init_clocks(pdev, h616chip);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to initialize clocks\n");
+
+	for (unsigned int i = 0; i < data->npwm; i++) {
+		struct h616_pwm_channel *chan = &h616chip->channels[i];
+		struct clk_hw **hw = &h616chip->clk_pdata->hw_data->hws[i];
+
+		chan->pwm_clk = devm_clk_hw_get_clk(dev, *hw, NULL);
+		if (IS_ERR(chan->pwm_clk)) {
+			ret = dev_err_probe(dev, PTR_ERR(chan->pwm_clk),
+					    "Failed to register PWM clock %d\n", i);
+			goto err_get_clk;
+		}
+		chan->mode = H616_PWM_MODE_NONE;
+	}
+
+	ret = devm_of_clk_add_hw_provider(dev, h616_pwm_of_clk_get, h616chip);
+	if (ret) {
+		dev_err_probe(dev, ret, "Failed to add HW clock provider\n");
+		goto err_add_clk_provider;
+	}
+
+	/* Deassert reset */
+	ret = reset_control_deassert(h616chip->rst);
+	if (ret) {
+		dev_err_probe(dev, ret, "Cannot deassert reset control\n");
+		goto err_ctrl_deassert;
+	}
+
+	ret = pwmchip_add(chip);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "Failed to add PWM chip\n");
+		goto err_pwm_add;
+	}
+
+	platform_set_drvdata(pdev, chip);
+
+	return 0;
+
+err_pwm_add:
+	reset_control_assert(h616chip->rst);
+
+err_ctrl_deassert:
+err_add_clk_provider:
+err_get_clk:
+	for (int i = 0; i < h616chip->clk_pdata->hw_data->num; i++)
+		clk_hw_unregister_composite(h616chip->clk_pdata->hw_data->hws[i]);
+
+	return ret;
+}
+
+static const struct h616_pwm_data sun50i_h616_pwm_data = {
+	.npwm = 6,
+};
+
+static const struct of_device_id h616_pwm_dt_ids[] = {
+	{
+		.compatible = "allwinner,sun50i-h616-pwm",
+		.data = &sun50i_h616_pwm_data,
+	}, {
+		/* sentinel */
+	},
+};
+MODULE_DEVICE_TABLE(of, h616_pwm_dt_ids);
+
+
+static struct platform_driver h616_pwm_driver = {
+	.driver = {
+		.name = "h616-pwm",
+		.of_match_table = h616_pwm_dt_ids,
+	},
+	.probe = h616_pwm_probe,
+};
+module_platform_driver(h616_pwm_driver);
+
+MODULE_AUTHOR("Richard Genoud <richard.genoud@bootlin.com>");
+MODULE_DESCRIPTION("Allwinner H616 PWM driver");
+MODULE_LICENSE("GPL");
-- 
2.47.3

Re: [PATCH v2 2/4] pwm: sun50i: Add H616 PWM support

[Uwe Kleine-König]

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

Hello,

this patch isn't checkpatch clean.

On Wed, Dec 17, 2025 at 09:25:02AM +0100, Richard Genoud wrote:
> Add driver for Allwinner H616 PWM controller, supporting up to 6
> channels.
> Those channels output can be either a PWM signal output or a clock
> output, thanks to the bypass.
> 
> The channels are paired (0/1, 2/3 and 4/5) and each pair has a
> prescaler/mux/gate.
> Moreover, each channel has its own prescaler and bypass.
> 
> Tested-by: Joao Schim <joao@schimsalabim.eu>
> Signed-off-by: Richard Genoud <richard.genoud@bootlin.com>
> ---
>  drivers/pwm/Kconfig           |  12 +
>  drivers/pwm/Makefile          |   1 +
>  drivers/pwm/pwm-sun50i-h616.c | 892 ++++++++++++++++++++++++++++++++++
>  3 files changed, 905 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sun50i-h616.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 6f3147518376..66534e033761 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -736,6 +736,18 @@ config PWM_SUN4I
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-sun4i.
>  
> +config PWM_SUN50I_H616
> +	tristate "Allwinner H616 PWM support"
> +	depends on ARCH_SUNXI || COMPILE_TEST
> +	depends on HAS_IOMEM && COMMON_CLK
> +	help
> +	  Generic PWM framework driver for Allwinner H616 SoCs.
> +	  It supports generic PWM, but can also provides a plain clock.
> +	  The AC300 PHY integrated in H616 SoC needs such a clock.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-h616.
> +
>  config PWM_SUNPLUS
>  	tristate "Sunplus PWM support"
>  	depends on ARCH_SUNPLUS || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 0dc0d2b69025..a16ae9eef9e5 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -67,6 +67,7 @@ obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
>  obj-$(CONFIG_PWM_STM32_LP)	+= pwm-stm32-lp.o
>  obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
>  obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
> +obj-$(CONFIG_PWM_SUN50I_H616)	+= pwm-sun50i-h616.o
>  obj-$(CONFIG_PWM_SUNPLUS)	+= pwm-sunplus.o
>  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
>  obj-$(CONFIG_PWM_TH1520)	+= pwm_th1520.o
> diff --git a/drivers/pwm/pwm-sun50i-h616.c b/drivers/pwm/pwm-sun50i-h616.c
> new file mode 100644
> index 000000000000..b002ea5935e4
> --- /dev/null
> +++ b/drivers/pwm/pwm-sun50i-h616.c
> @@ -0,0 +1,892 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Driver for Allwinner H616 Pulse Width Modulation Controller
> + *
> + * (C) Copyright 2025 Richard Genoud, Bootlin <richard.genoud@bootlin.com>
> + *
> + * Based on drivers/pwm/pwm-sun4i.c with Copyright:
> + *
> + * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@bootlin.com>
> + *
> + * Limitations:
> + * - When outputing the source clock directly, the PWM logic is bypassed and the
> + *   currently running period is not guaranteed to be completed.
> + * - As the channels are paired (0/1, 2/3, 4/5), they share the same clock
> + *   source and prescaler(div_m), but they also have their own prescaler(div_k)
> + *   and bypass.
> + *
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/reset.h>
> +#include <linux/spinlock.h>
> +#include <linux/time.h>
> +
> +#ifndef UINT32_MAX
> +#define UINT32_MAX 0xffffffffU
> +#endif

Please include <linux/limits.h> and use U32_MAX.

> +
> +/* PWM IRQ Enable Register */
> +#define PWM_IER				0x0

Use a driver specific prefix for your defines. Otherwise these constants
look more generic than they are.

> +/* PWM IRQ Status Register */
> +#define PWM_ISR				0x4
> +
> +/* PWM Capture IRQ Enable Register */
> +#define PWM_CIER			0x10
> +
> +/* PWM Capture IRQ Status Register */
> +#define PWM_CISR			0x14
> +
> +/* PWMCC Pairs Clock Configuration Registers */
> +#define PWM_XY_CLK_CR(pair)		(0x20 + ((pair) * 0x4))
> +#define PWM_XY_CLK_CR_SRC_SHIFT		7
> +#define PWM_XY_CLK_CR_SRC_MASK		1

Please do:

	#define H616_PWM_XY_CLK_CR_SRC		BIT(7)

and then use the helper functions from bitfield.h 

> +#define PWM_XY_CLK_CR_GATE_BIT		4
> +#define PWM_XY_CLK_CR_BYPASS_BIT(chan)	((chan) % 2 + 5)
> +#define PWM_XY_CLK_CR_DIV_M_SHIFT	0
> +
> +/* PWMCC Pairs Dead Zone Control Registers */
> +#define PWM_XY_DZ(pair)			(0x30 + ((pair) * 0x4))
> +
> +/* PWM Enable Register */
> +#define PWM_ENR				0x40
> +#define PWM_ENABLE(x)			BIT(x)

Please use the full register name as prefix for bitfields.

> +/* PWM Capture Enable Register */
> +#define PWM_CER				0x44
> +
> +/* PWM Control Register */
> +#define PWM_CTRL_REG(chan)		(0x60 + (chan) * 0x20)
> +#define PWM_CTRL_PRESCAL_K_SHIFT	0
> +#define PWM_CTRL_PRESCAL_K_WIDTH	8
> +#define PWM_CTRL_ACTIVE_STATE		BIT(8)
> +
> +/* PWM Period Register */
> +#define PWM_PERIOD_REG(ch)		(0x64 + (ch) * 0x20)
> +#define PWM_PERIOD_MASK			GENMASK(31, 16)
> +#define PWM_DUTY_MASK			GENMASK(15, 0)
> +#define PWM_REG_PERIOD(reg)		(FIELD_GET(PWM_PERIOD_MASK, reg) + 1)
> +#define PWM_REG_DUTY(reg)		FIELD_GET(PWM_DUTY_MASK, reg)
> +#define PWM_PERIOD(prd)			FIELD_PREP(PWM_PERIOD_MASK, (prd) - 1)
> +#define PWM_DUTY(dty)			FIELD_PREP(PWM_DUTY_MASK, dty)
> +#define PWM_PERIOD_MAX			FIELD_MAX(PWM_PERIOD_MASK)
> +
> +
> +/* PWM Count Register */
> +#define PWM_CNT_REG(x)			(0x68 + (x) * 0x20)
> +
> +/* PWM Capture Control Register */
> +#define PWM_CCR(x)			(0x6c + (x) * 0x20)
> +
> +/* PWM Capture Rise Lock Register */
> +#define PWM_CRLR(x)			(0x70 + (x) * 0x20)
> +
> +/* PWM Capture Fall Lock Register */
> +#define PWM_CFLR(x)			(0x74 + (x) * 0x20)
> +
> +#define PWM_PAIR_IDX(chan)		((chan) >> 2)
> +
> +/*
> + * Block diagram of the PWM clock controller:
> + *
> + *             _____      ______      ________
> + * OSC24M --->|     |    |      |    |        |
> + * APB1 ----->| Mux |--->| Gate |--->| /div_m |-----> PWM_clock_src_xy
> + *            |_____|    |______|    |________|
> + *                           ________
> + *                          |        |
> + *                       +->| /div_k |---> PWM_clock_x
> + *                       |  |________|
> + *                       |    ______
> + *                       |   |      |
> + *                       +-->| Gate |----> PWM_bypass_clock_x
> + *                       |   |______|
> + * PWM_clock_src_xy -----+   ________
> + *                       |  |        |
> + *                       +->| /div_k |---> PWM_clock_y
> + *                       |  |________|
> + *                       |    ______
> + *                       |   |      |
> + *                       +-->| Gate |----> PWM_bypass_clock_y
> + *                           |______|
> + *
> + * NB: when the bypass is set, all the PWM logic is bypassed.
> + * So, the duty cycle and polarity can't be modified (we just have a clock).
> + * The bypass in PWM mode is used to achieve a 1/2 duty cycle with the fastest
> + * clock.

1/2 *relative* duty cycle.

> + *
> + * PWM_clock_x/y serve for the PWM purpose.
> + * PWM_bypass_clock_x/y serve for the clock-provider purpose.
> + *
> + */
> +
> +/*
> + * Table used for /div_m (diviser before obtaining PWM_clock_src_xy)
> + * It's actually CLK_DIVIDER_POWER_OF_TWO, but limited to /256
> + */
> +static const struct clk_div_table clk_table_div_m[] = {
> +	{ .val = 0, .div = 1, },
> +	{ .val = 1, .div = 2, },
> +	{ .val = 2, .div = 4, },
> +	{ .val = 3, .div = 8, },
> +	{ .val = 4, .div = 16, },
> +	{ .val = 5, .div = 32, },
> +	{ .val = 6, .div = 64, },
> +	{ .val = 7, .div = 128, },
> +	{ .val = 8, .div = 256, },
> +	{ .val = 0, .div = 0, }, /* last entry */
> +};

.div is just 1 << .val, so I would expect you can better use math
expressions instead of this table.

> +#define PWM_XY_SRC_GATE(_pair, _reg)			\
> +struct clk_gate gate_xy_##_pair = {			\
> +	.reg = (void *)_reg,				\
> +	.bit_idx = PWM_XY_CLK_CR_GATE_BIT,		\
> +	.hw.init = &(struct clk_init_data){		\
> +		.ops =  &clk_gate_ops,			\
> +	}						\

I would consider

	.hw.init.ops = ...;

and

	.hw = {
		.init = {
			.ops = ...;
		},
	},

natural here. The middleway you chose looks strange to me.
Also s/  / /.

> +}
> +
> +#define PWM_XY_SRC_MUX(_pair, _reg)			\
> +struct clk_mux mux_xy_##_pair = {			\
> +	.reg = (void *)_reg,				\
> +	.shift = PWM_XY_CLK_CR_SRC_SHIFT,		\
> +	.mask = PWM_XY_CLK_CR_SRC_MASK,			\

Huh, why does this structure has both a shift and a mask value? What is
the difference between

	.shift = 7,
	.mask = 1,

and

	.shift = 0,
	.mask = 1 << 7,

? If the latter is equivalent, you could just pass
H616_PWM_XY_CLK_CR_SRC and get rid of the extra definitions for _SHIFT
and _MASK.

> +	.flags = CLK_MUX_ROUND_CLOSEST,			\
> +	.hw.init = &(struct clk_init_data){		\
> +		.ops =  &clk_mux_ops,			\
> +	}						\
> +}
> +
> +#define PWM_XY_SRC_DIV(_pair, _reg)			\
> +struct clk_divider rate_xy_##_pair = {			\
> +	.reg = (void *)_reg,				\
> +	.shift = PWM_XY_CLK_CR_DIV_M_SHIFT,		\
> +	.table = clk_table_div_m,			\
> +	.hw.init = &(struct clk_init_data){		\
> +		.ops =  &clk_divider_ops,		\
> +	}						\
> +}
> +
> +#define PWM_X_DIV(_idx, _reg)				\
> +struct clk_divider rate_x_##_idx = {			\
> +	.reg = (void *)_reg,				\
> +	.shift = PWM_CTRL_PRESCAL_K_SHIFT,		\
> +	.width = PWM_CTRL_PRESCAL_K_WIDTH,		\
> +	.hw.init = &(struct clk_init_data){		\
> +		.ops =  &clk_divider_ops,		\
> +	}						\
> +}
> +
> +#define PWM_X_BYPASS_GATE(_idx)				\
> +struct clk_gate gate_x_bypass_##_idx = {		\
> +	.reg = (void *)PWM_ENR,				\
> +	.bit_idx = _idx,				\
> +	.hw.init = &(struct clk_init_data){		\
> +		.ops =  &clk_gate_ops,			\
> +	}						\
> +}
> +
> +#define PWM_XY_CLK_SRC(_pair, _reg)			\
> +	static PWM_XY_SRC_MUX(_pair, _reg);		\
> +	static PWM_XY_SRC_GATE(_pair, _reg);		\
> +	static PWM_XY_SRC_DIV(_pair, _reg)
> +
> +#define PWM_X_CLK(_idx)					\
> +	static PWM_X_DIV(_idx, PWM_CTRL_REG(_idx))
> +
> +#define PWM_X_BYPASS_CLK(_idx)						\
> +	PWM_X_BYPASS_GATE(_idx)
> +
> +#define REF_CLK_XY_SRC(_pair)						\
> +	{								\
> +		.name = "pwm-clk-src" #_pair,				\
> +		.mux_hw = &mux_xy_##_pair.hw,				\
> +		.gate_hw = &gate_xy_##_pair.hw,				\
> +		.rate_hw = &rate_xy_##_pair.hw,				\
> +	}
> +
> +#define REF_CLK_X(_idx, _pair)						\
> +	{								\
> +		.name = "pwm-clk" #_idx,				\
> +		.parent_names = (const char *[]){ "pwm-clk-src" #_pair }, \
> +		.num_parents = 1,					\
> +		.rate_hw = &rate_x_##_idx.hw,				\
> +		.flags = CLK_SET_RATE_PARENT,				\
> +	}
> +
> +#define REF_CLK_BYPASS(_idx, _pair)					\
> +	{								\
> +		.name = "pwm-clk-bypass" #_idx,				\
> +		.parent_names = (const char *[]){ "pwm-clk-src" #_pair }, \
> +		.num_parents = 1,					\
> +		.gate_hw = &gate_x_bypass_##_idx.hw,			\
> +		.flags = CLK_SET_RATE_PARENT,	\
> +	}
> +
> +/*
> + * PWM_clock_src_xy generation:
> + *             _____      ______      ________
> + * OSC24M --->|     |    |      |    |        |
> + * APB1 ----->| Mux |--->| Gate |--->| /div_m |-----> PWM_clock_src_xy
> + *            |_____|    |______|    |________|
> + */
> +PWM_XY_CLK_SRC(01, PWM_XY_CLK_CR(0));
> +PWM_XY_CLK_SRC(23, PWM_XY_CLK_CR(1));
> +PWM_XY_CLK_SRC(45, PWM_XY_CLK_CR(2));
> +
> +/*
> + * PWM_clock_x_div generation:
> + *                       ________
> + *                      |        | PWM_clock_x/y
> + * PWM_clock_src_xy --->| /div_k |--------------->
> + *                      |________|
> + */
> +PWM_X_CLK(0);
> +PWM_X_CLK(1);
> +PWM_X_CLK(2);
> +PWM_X_CLK(3);
> +PWM_X_CLK(4);
> +PWM_X_CLK(5);
> +
> +/*
> + * PWM_bypass_clock_xy generation:
> + *                        ______
> + *                       |      |
> + * PWM_clock_src_xy ---->| Gate |-------> PWM_bypass_clock_x
> + *                       |______|
> + *
> + * The gate is actually PWM_ENR register.
> + */
> +PWM_X_BYPASS_CLK(0);
> +PWM_X_BYPASS_CLK(1);
> +PWM_X_BYPASS_CLK(2);
> +PWM_X_BYPASS_CLK(3);
> +PWM_X_BYPASS_CLK(4);
> +PWM_X_BYPASS_CLK(5);
> +
> +struct clk_pwm_data {
> +	const char *name;
> +	const char **parent_names;
> +	unsigned int num_parents;
> +	struct clk_hw *mux_hw;
> +	struct clk_hw *rate_hw;
> +	struct clk_hw *gate_hw;
> +	unsigned long flags;
> +};
> +
> +#define CLK_BYPASS(h616chip, ch) ((h616chip)->data->npwm + (ch))
> +#define CLK_XY_SRC_IDX(h616chip, ch) ((h616chip)->data->npwm * 2 + ((ch) >> 1))
> +static struct clk_pwm_data pwmcc_data[] = {
> +	REF_CLK_X(0, 01),
> +	REF_CLK_X(1, 01),
> +	REF_CLK_X(2, 23),
> +	REF_CLK_X(3, 23),
> +	REF_CLK_X(4, 45),
> +	REF_CLK_X(5, 45),
> +	REF_CLK_BYPASS(0, 01),
> +	REF_CLK_BYPASS(1, 01),
> +	REF_CLK_BYPASS(2, 23),
> +	REF_CLK_BYPASS(3, 23),
> +	REF_CLK_BYPASS(4, 45),
> +	REF_CLK_BYPASS(5, 45),
> +	REF_CLK_XY_SRC(01),
> +	REF_CLK_XY_SRC(23),
> +	REF_CLK_XY_SRC(45),
> +	{ /* sentinel */ },
> +};
> +
> +enum h616_pwm_mode {
> +	H616_PWM_MODE_NONE,
> +	H616_PWM_MODE_PWM,
> +	H616_PWM_MODE_CLK,
> +};
> +
> +struct h616_pwm_data {
> +	unsigned int npwm;
> +};
> +
> +struct h616_pwm_channel {
> +	struct clk *pwm_clk;
> +	unsigned long rate;
> +	unsigned int entire_cycles;
> +	unsigned int active_cycles;
> +	enum h616_pwm_mode mode;
> +	bool bypass;
> +};
> +
> +struct clk_pwm_pdata {
> +	struct clk_hw_onecell_data *hw_data;
> +	spinlock_t lock;
> +	void __iomem *reg;
> +};
> +
> +struct h616_pwm_chip {
> +	struct clk_pwm_pdata *clk_pdata;
> +	struct h616_pwm_channel *channels;
> +	struct clk *bus_clk;
> +	struct reset_control *rst;
> +	void __iomem *base;
> +	const struct h616_pwm_data *data;
> +};
> +
> +static inline struct h616_pwm_chip *to_h616_pwm_chip(struct pwm_chip *chip)

It probably doesn't help much, but conceptually this could be

	static inline struct h616_pwm_chip *to_h616_pwm_chip(const struct pwm_chip *chip)

> +{
> +	return pwmchip_get_drvdata(chip);
> +}
> +
> +static inline u32 h616_pwm_readl(struct h616_pwm_chip *h616chip,
> +				 unsigned long offset)
> +{
> +	return readl(h616chip->base + offset);
> +}
> +
> +static inline void h616_pwm_writel(struct h616_pwm_chip *h616chip,
> +				   u32 val, unsigned long offset)
> +{
> +	writel(val, h616chip->base + offset);
> +}
> +
> +static void h616_pwm_set_bypass(struct h616_pwm_chip *h616chip, unsigned int idx,
> +				bool en_bypass)
> +{
> +	unsigned long flags;
> +	u32 val;
> +
> +	spin_lock_irqsave(&h616chip->clk_pdata->lock, flags);
> +
> +	val = h616_pwm_readl(h616chip, PWM_XY_CLK_CR(PWM_PAIR_IDX(idx)));
> +	if (en_bypass)
> +		val |= BIT(PWM_XY_CLK_CR_BYPASS_BIT(idx));
> +	else
> +		val &= ~BIT(PWM_XY_CLK_CR_BYPASS_BIT(idx));
> +
> +	h616_pwm_writel(h616chip, val, PWM_XY_CLK_CR(PWM_PAIR_IDX(idx)));
> +
> +	spin_unlock_irqrestore(&h616chip->clk_pdata->lock, flags);
> +}
> +
> +static int h616_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct h616_pwm_chip *h616chip = to_h616_pwm_chip(chip);
> +	struct h616_pwm_channel *chan = &h616chip->channels[pwm->hwpwm];
> +	struct device *dev = pwmchip_parent(chip);
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(&h616chip->clk_pdata->lock, flags);
> +
> +	if (chan->mode == H616_PWM_MODE_CLK)
> +		ret = -EBUSY;
> +	else
> +		chan->mode = H616_PWM_MODE_PWM;
> +
> +	spin_unlock_irqrestore(&h616chip->clk_pdata->lock, flags);
> +	if (ret)
> +		goto out;

You could simplify that to:

	scoped_guard(spinlock_irqsave, &h616chip->clk_pdata->lock) {
		if (chan->mode == H616_PWM_MODE_CLK)
			return -EBUSY;
		else
			chan->mode = H616_PWM_MODE_PWM;
	}

> +
> +	ret = clk_prepare_enable(chan->pwm_clk);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enable clock %s: %d\n",
> +			__clk_get_name(chan->pwm_clk), ret);
> +	}

Please no error messages in the runtime callbacks.

> +out:
> +	return ret;
> +}
> +
> +static void h616_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct h616_pwm_chip *h616chip = to_h616_pwm_chip(chip);
> +	struct h616_pwm_channel *chan = &h616chip->channels[pwm->hwpwm];
> +
> +	clk_disable_unprepare(chan->pwm_clk);
> +	chan->mode = H616_PWM_MODE_NONE;
> +}
> +
> +static int h616_pwm_get_state(struct pwm_chip *chip,
> +			      struct pwm_device *pwm,
> +			      struct pwm_state *state)
> +{
> +	struct h616_pwm_chip *h616chip = to_h616_pwm_chip(chip);
> +	struct h616_pwm_channel *chan = &h616chip->channels[pwm->hwpwm];
> +	u64 clk_rate, tmp;
> +	u32 val;
> +
> +	clk_rate = clk_get_rate(chan->pwm_clk);
> +	if (!clk_rate)
> +		return -EINVAL;
> +
> +	val = h616_pwm_readl(h616chip, PWM_ENR);
> +	state->enabled = !!(PWM_ENABLE(pwm->hwpwm) & val);
> +
> +	val = h616_pwm_readl(h616chip, PWM_XY_CLK_CR(PWM_PAIR_IDX(pwm->hwpwm)));
> +	if (val & BIT(PWM_XY_CLK_CR_BYPASS_BIT(pwm->hwpwm))) {
> +		/*
> +		 * When bypass is enabled, the PWM logic is inactive.
> +		 * The PWM_clock_src_xy is directly routed to PWM_clock_x
> +		 */
> +		state->period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, clk_rate);
> +		state->duty_cycle = DIV_ROUND_UP_ULL(state->period, 2);
> +		state->polarity = PWM_POLARITY_NORMAL;
> +		return 0;
> +	}
> +
> +	state->enabled &= !!(BIT(PWM_XY_CLK_CR_GATE_BIT) & val);
> +
> +	val = h616_pwm_readl(h616chip, PWM_CTRL_REG(pwm->hwpwm));
> +	if (val & PWM_CTRL_ACTIVE_STATE)
> +		state->polarity = PWM_POLARITY_NORMAL;
> +	else
> +		state->polarity = PWM_POLARITY_INVERSED;
> +
> +	val = h616_pwm_readl(h616chip, PWM_PERIOD_REG(pwm->hwpwm));
> +
> +	tmp = NSEC_PER_SEC * PWM_REG_DUTY(val);
> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
> +
> +	tmp = NSEC_PER_SEC * PWM_REG_PERIOD(val);
> +	state->period = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);

This needs to use uprounding. Ideally enabling CONFIG_PWM_DEBUG and
extensive testing should catch that error.

> +
> +	return 0;
> +}
> +
> +static int h616_pwm_calc(struct pwm_chip *chip, unsigned int idx,
> +			 const struct pwm_state *state)
> +{
> +	struct h616_pwm_chip *h616chip = to_h616_pwm_chip(chip);
> +	struct h616_pwm_channel *chan = &h616chip->channels[idx];
> +	unsigned int cnt, duty_cnt;
> +	unsigned long max_rate;
> +	long calc_rate;
> +	u64 duty, period, freq;
> +
> +	duty = state->duty_cycle;
> +	period = state->period;
> +
> +	max_rate = clk_round_rate(chan->pwm_clk, UINT32_MAX);

Huh, is this an artificial limitation? Do you rely on how clk_round_rate
picks the return value? (I.e. nearest value? nearest time? round up?
round down?)

> +	dev_dbg(pwmchip_parent(chip), "max_rate: %ld Hz\n", max_rate);
> +
> +	if ((period * max_rate >= NSEC_PER_SEC) &&

If period is big, this overflows.

> +	    (period * max_rate < 2 * NSEC_PER_SEC) &&
> +	    (duty * max_rate * 2 >= NSEC_PER_SEC)) {
> +		/*
> +		 * If the requested period is to small to be generated by the
> +		 * PWM, we can just select the highest clock and bypass the
> +		 * PWM logic
> +		 */
> +		dev_dbg(pwmchip_parent(chip), "Setting bypass (period=%lld)\n",
> +			period);
> +		freq = div64_u64(NSEC_PER_SEC, period);
> +		chan->bypass = true;
> +		duty = period / 2;

You must not round up duty. So if a small period and duty_cycle <
period/2 is requested, you have to return an error value. (Note that
with waveforms the semantic is a bit different.)

> +	} else {
> +		chan->bypass = false;
> +		freq = div64_u64(NSEC_PER_SEC * (u64)PWM_PERIOD_MAX, period);
> +		if (freq > UINT32_MAX)
> +			freq = UINT32_MAX;
> +	}
> +
> +	calc_rate = clk_round_rate(chan->pwm_clk, freq);
> +	if (calc_rate <= 0) {
> +		dev_err(pwmchip_parent(chip),
> +			"Invalid source clock frequency %llu\n", freq);
> +		return calc_rate ? calc_rate : -EINVAL;
> +	}
> +
> +	dev_dbg(pwmchip_parent(chip), "calc_rate: %ld Hz\n", calc_rate);
> +
> +	cnt = mul_u64_u64_div_u64(calc_rate, period, NSEC_PER_SEC);

Please use a better name here, something like period_ticks would be
good.

> +	if ((cnt == 0) || (cnt > PWM_PERIOD_MAX)) {
> +		dev_err(pwmchip_parent(chip), "Period out of range\n");
> +		return -EINVAL;
> +	}

If the requested value is too big, clamp it to the maximal possible
value.

> +	duty_cnt = mul_u64_u64_div_u64(calc_rate, duty, NSEC_PER_SEC);
> +
> +	if (duty_cnt >= cnt)
> +		duty_cnt = cnt - 1;

Does that mean the PWM cannot produce a 100% relative duty cycle?

> +	dev_dbg(pwmchip_parent(chip), "period=%llu cnt=%u duty=%llu duty_cnt=%u\n",
> +		period, cnt, duty, duty_cnt);

This is little helpful without the input parameters.

> +	chan->active_cycles = duty_cnt;
> +	chan->entire_cycles = cnt;
> +
> +	chan->rate = calc_rate;
> +
> +	return 0;
> +}
> +
> +static int h616_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			  const struct pwm_state *state)
> +{

Please consider implementing .round_waveform_tohw(),
.round_waveform_fromhw(), .read_waveform() and .write_waveform() instead
of .apply() and .get_state().

> +	struct h616_pwm_chip *h616chip = to_h616_pwm_chip(chip);
> +	struct h616_pwm_channel *chan = &h616chip->channels[pwm->hwpwm];
> +	struct pwm_state cstate;
> +	unsigned long flags;
> +	u32 val;
> +	int ret;
> +
> +	ret = h616_pwm_calc(chip, pwm->hwpwm, state);
> +	if (ret) {
> +		dev_err(pwmchip_parent(chip), "period exceeds the maximum value\n");
> +		return ret;
> +	}
> +
> +	pwm_get_state(pwm, &cstate);

Don't call PWM API functions. (h616_pwm_apply() is called by
pwm_apply_{might_sleep,atomic} which grabs the chip lock. That
pwm_get_state() doesn't is an implementation detail that you should not
rely on.) I would prefer that you don't access pwm->state at all but
determine the state by looking at the registers.

> +	ret = clk_set_rate(chan->pwm_clk, chan->rate);
> +	if (ret) {
> +		dev_err(pwmchip_parent(chip), "failed to set PWM %d clock rate to %lu\n",
> +			pwm->hwpwm, chan->rate);
> +		return ret;
> +	}
> +
> +	h616_pwm_set_bypass(h616chip, pwm->hwpwm, chan->bypass);
> +
> +	/*
> +	 * If bypass is set, the PWM logic (polarity, duty) can't be applied
> +	 */
> +
> +	if (chan->bypass && (state->polarity == PWM_POLARITY_INVERSED)) {
> +		dev_warn(pwmchip_parent(chip),
> +			 "Can't set inversed polarity with bypass enabled\n");
> +	} else {
> +		val = h616_pwm_readl(h616chip, PWM_CTRL_REG(pwm->hwpwm));
> +		val &= ~PWM_CTRL_ACTIVE_STATE;
> +		if (state->polarity == PWM_POLARITY_NORMAL)
> +			val |= PWM_CTRL_ACTIVE_STATE;
> +		h616_pwm_writel(h616chip, val, PWM_CTRL_REG(pwm->hwpwm));
> +	}
> +
> +	if (chan->bypass && (state->duty_cycle * 2 != state->period)) {
> +		dev_warn(pwmchip_parent(chip),
> +			 "Can't set a duty cycle with bypass enabled\n");
> +	}

Please no output in runtime callbacks. (And even then

	state->period / 2 != state->duty_cycle

would probaly be the nicer check as it also works for uneven periods.)

> +
> +	if (!chan->bypass) {
> +		val = PWM_DUTY(chan->active_cycles);
> +		val |= PWM_PERIOD(chan->entire_cycles);
> +		h616_pwm_writel(h616chip, val, PWM_PERIOD_REG(pwm->hwpwm));
> +	}
> +
> +	if (state->enabled == cstate.enabled)
> +		return 0;
> +
> +	if (cstate.enabled) {
> +		unsigned long delay_us;
> +
> +		/*
> +		 * We need a full period to elapse before
> +		 * disabling the channel.

You need a full period between what and disabling? Assuming this is
about period and duty setting: Why not skip it if you disable?

> +		 */
> +		delay_us = DIV_ROUND_UP_ULL(cstate.period, NSEC_PER_USEC);
> +		fsleep(delay_us);

Huu, sleeping while holding a spin_lock and having disabled irqs :-\

> +	}
> +
> +	spin_lock_irqsave(&h616chip->clk_pdata->lock, flags);
> +
> +	val = h616_pwm_readl(h616chip, PWM_ENR);
> +	if (state->enabled)
> +		val |= PWM_ENABLE(pwm->hwpwm);
> +	else
> +		val &= ~PWM_ENABLE(pwm->hwpwm);
> +	h616_pwm_writel(h616chip, val, PWM_ENR);
> +
> +	spin_unlock_irqrestore(&h616chip->clk_pdata->lock, flags);
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops h616_pwm_ops = {
> +	.apply = h616_pwm_apply,
> +	.get_state = h616_pwm_get_state,
> +	.request = h616_pwm_request,
> +	.free = h616_pwm_free,
> +};
> +
> +static struct clk_hw *h616_pwm_of_clk_get(struct of_phandle_args *clkspec,
> +					  void *data)
> +{
> +	struct h616_pwm_chip *h616chip = data;
> +	struct clk_hw_onecell_data *hw_data = h616chip->clk_pdata->hw_data;
> +	unsigned int idx = clkspec->args[0];
> +	struct h616_pwm_channel *chan;
> +	struct clk_hw *ret_clk = NULL;
> +	unsigned long flags;
> +
> +	if (idx >= h616chip->data->npwm)
> +		return ERR_PTR(-EINVAL);
> +
> +	chan = &h616chip->channels[idx];
> +
> +	spin_lock_irqsave(&h616chip->clk_pdata->lock, flags);
> +
> +	if (chan->mode == H616_PWM_MODE_PWM) {
> +		ret_clk = ERR_PTR(-EBUSY);
> +	} else {
> +		chan->mode = H616_PWM_MODE_CLK;
> +		ret_clk = hw_data->hws[CLK_BYPASS(h616chip, idx)];
> +	}
> +	spin_unlock_irqrestore(&h616chip->clk_pdata->lock, flags);
> +
> +	if (IS_ERR(ret_clk))
> +		goto out;
> +
> +	chan->bypass = true;
> +	h616_pwm_set_bypass(h616chip, idx, chan->bypass);
> +out:
> +	return ret_clk;
> +}
> +
> +static int h616_add_composite_clk(struct clk_pwm_data *data,
> +				  void __iomem *reg, spinlock_t *lock,
> +				  struct device *dev, struct clk_hw **hw)
> +{
> +	const struct clk_ops *mux_ops = NULL, *gate_ops = NULL, *rate_ops = NULL;
> +	struct clk_hw *mux_hw = NULL, *gate_hw = NULL, *rate_hw = NULL;
> +	struct device_node *node = dev->of_node;
> +
> +	if (data->mux_hw) {
> +		struct clk_mux *mux;
> +
> +		mux_hw = data->mux_hw;
> +		mux = to_clk_mux(mux_hw);
> +		mux->lock = lock;
> +		mux_ops = mux_hw->init->ops;
> +		mux->reg = (u64)mux->reg + reg;
> +	}
> +
> +	if (data->gate_hw) {
> +		struct clk_gate *gate;
> +
> +		gate_hw = data->gate_hw;
> +		gate = to_clk_gate(gate_hw);
> +		gate->lock = lock;
> +		gate_ops = gate_hw->init->ops;
> +		gate->reg = (u64)gate->reg + reg;
> +	}
> +
> +	if (data->rate_hw) {
> +		struct clk_divider *rate;
> +
> +		rate_hw = data->rate_hw;
> +		rate = to_clk_divider(rate_hw);
> +		rate_ops = rate_hw->init->ops;
> +		rate->lock = lock;
> +		rate->reg = (u64)rate->reg + reg;
> +
> +		if (rate->table) {
> +			const struct clk_div_table *clkt;
> +			int table_size = 0;
> +
> +			for (clkt = rate->table; clkt->div; clkt++)
> +				table_size++;
> +			rate->width = order_base_2(table_size);
> +		}
> +	}
> +
> +	/*
> +	 * Retrieve the parent clock names from DTS for pwm-clk-srcxy
> +	 */
> +	if (!data->parent_names) {
> +		data->num_parents = of_clk_get_parent_count(node);
> +		if (data->num_parents == 0)
> +			return -ENOENT;
> +
> +		data->parent_names = devm_kzalloc(dev,
> +						  sizeof(*data->parent_names),
> +						  GFP_KERNEL);
> +		for (unsigned int i = 0; i < data->num_parents; i++)
> +			data->parent_names[i] = of_clk_get_parent_name(node, i);
> +	}
> +
> +	*hw = clk_hw_register_composite(dev, data->name, data->parent_names,
> +					data->num_parents, mux_hw,
> +					mux_ops, rate_hw, rate_ops,
> +					gate_hw, gate_ops, data->flags);
> +
> +	return PTR_ERR_OR_ZERO(*hw);
> +}
> +
> +static int h616_pwm_init_clocks(struct platform_device *pdev,
> +				struct h616_pwm_chip *h616chip)
> +{
> +	struct clk_pwm_pdata *pdata;
> +	struct device *dev = &pdev->dev;
> +	int num_clocks = 0;
> +	int ret;
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	while (pwmcc_data[num_clocks].name)
> +		num_clocks++;
> +
> +	pdata->hw_data = devm_kzalloc(dev, struct_size(pdata->hw_data, hws, num_clocks),
> +				      GFP_KERNEL);
> +	if (!pdata->hw_data)
> +		return -ENOMEM;
> +
> +	pdata->hw_data->num = num_clocks;
> +	pdata->reg = h616chip->base;
> +
> +	spin_lock_init(&pdata->lock);
> +
> +	for (int i = 0; i < num_clocks; i++) {
> +		struct clk_hw **hw = &pdata->hw_data->hws[i];
> +
> +		ret = h616_add_composite_clk(&pwmcc_data[i], pdata->reg,
> +					     &pdata->lock, dev, hw);
> +		if (ret) {
> +			dev_err_probe(dev, ret,
> +				      "Failed to register hw clock %s\n",
> +				      pwmcc_data[i].name);
> +			for (i--; i >= 0; i--)
> +				clk_hw_unregister_composite(pdata->hw_data->hws[i]);
> +			return ret;
> +		}
> +	}
> +
> +	h616chip->clk_pdata = pdata;
> +
> +	return 0;
> +}
> +
> +static int h616_pwm_probe(struct platform_device *pdev)
> +{
> +	const struct h616_pwm_data *data;
> +	struct device *dev = &pdev->dev;
> +	struct h616_pwm_chip *h616chip;
> +	struct pwm_chip *chip;
> +	int ret;
> +
> +	data = of_device_get_match_data(dev);
> +	if (!data)
> +		return -ENODEV;

error message please. (Or at least a comment that this cannot happen.)

> +
> +	chip = devm_pwmchip_alloc(dev, data->npwm, sizeof(*h616chip));
> +	if (IS_ERR(chip))
> +		return dev_err_probe(dev, PTR_ERR(chip),
> +				     "Failed to allocate pwmchip\n");
> +
> +	h616chip = to_h616_pwm_chip(chip);
> +	h616chip->data = data;
> +	h616chip->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(h616chip->base))
> +		return dev_err_probe(dev, PTR_ERR(h616chip->base),
> +				     "Failed to get PWM base address\n");
> +
> +	h616chip->bus_clk = devm_clk_get_enabled(dev, "bus");
> +	if (IS_ERR(h616chip->bus_clk))
> +		return dev_err_probe(dev, PTR_ERR(h616chip->bus_clk),
> +				     "Failed to get bus clock\n");
> +
> +	h616chip->channels = devm_kmalloc_array(dev, data->npwm,
> +						sizeof(*(h616chip->channels)),
> +						GFP_KERNEL);

There is a big amount of allocations here. Would be great to have this
reduced to save some memory management overhead and get some cache
locality in return.

> +	if (!h616chip->channels)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "Failed to allocate %d channels array\n",
> +				     data->npwm);
> +
> +	h616chip->rst = devm_reset_control_get_shared(dev, NULL);
> +	if (IS_ERR(h616chip->rst))
> +		return dev_err_probe(dev, PTR_ERR(h616chip->rst),
> +				     "Failed to get reset control\n");
> +
> +	chip->ops = &h616_pwm_ops;
> +
> +	ret = h616_pwm_init_clocks(pdev, h616chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to initialize clocks\n");

h616_pwm_init_clocks() already emits error messages.

> +	for (unsigned int i = 0; i < data->npwm; i++) {

Huh, AFAIK we're not using declarations in for loops in the kernel.

> +		struct h616_pwm_channel *chan = &h616chip->channels[i];
> +		struct clk_hw **hw = &h616chip->clk_pdata->hw_data->hws[i];
> +
> +		chan->pwm_clk = devm_clk_hw_get_clk(dev, *hw, NULL);
> +		if (IS_ERR(chan->pwm_clk)) {
> +			ret = dev_err_probe(dev, PTR_ERR(chan->pwm_clk),
> +					    "Failed to register PWM clock %d\n", i);
> +			goto err_get_clk;
> +		}
> +		chan->mode = H616_PWM_MODE_NONE;
> +	}
> +
> +	ret = devm_of_clk_add_hw_provider(dev, h616_pwm_of_clk_get, h616chip);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Failed to add HW clock provider\n");
> +		goto err_add_clk_provider;
> +	}
> +
> +	/* Deassert reset */
> +	ret = reset_control_deassert(h616chip->rst);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Cannot deassert reset control\n");
> +		goto err_ctrl_deassert;
> +	}
> +
> +	ret = pwmchip_add(chip);
> +	if (ret < 0) {
> +		dev_err_probe(dev, ret, "Failed to add PWM chip\n");
> +		goto err_pwm_add;
> +	}
> +
> +	platform_set_drvdata(pdev, chip);

this is unused.

> +	return 0;
> +
> +err_pwm_add:
> +	reset_control_assert(h616chip->rst);
> +
> +err_ctrl_deassert:
> +err_add_clk_provider:
> +err_get_clk:
> +	for (int i = 0; i < h616chip->clk_pdata->hw_data->num; i++)
> +		clk_hw_unregister_composite(h616chip->clk_pdata->hw_data->hws[i]);
> +
> +	return ret;
> +}

You need to implement a .remove() callback to not leak resources (or use
devm functions).

> +
> +static const struct h616_pwm_data sun50i_h616_pwm_data = {
> +	.npwm = 6,
> +};
> +
> +static const struct of_device_id h616_pwm_dt_ids[] = {
> +	{
> +		.compatible = "allwinner,sun50i-h616-pwm",
> +		.data = &sun50i_h616_pwm_data,
> +	}, {
> +		/* sentinel */
> +	},

no , here.

> +};
> +MODULE_DEVICE_TABLE(of, h616_pwm_dt_ids);
> +
> +

A single empty line is enough here.

> +static struct platform_driver h616_pwm_driver = {
> +	.driver = {
> +		.name = "h616-pwm",
> +		.of_match_table = h616_pwm_dt_ids,
> +	},
> +	.probe = h616_pwm_probe,
> +};
> +module_platform_driver(h616_pwm_driver);
> +
> +MODULE_AUTHOR("Richard Genoud <richard.genoud@bootlin.com>");
> +MODULE_DESCRIPTION("Allwinner H616 PWM driver");
> +MODULE_LICENSE("GPL");

Best regards
Uwe

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

Re: [PATCH v2 2/4] pwm: sun50i: Add H616 PWM support

[Richard GENOUD]

Hi,

Le 24/12/2025 à 10:54, Uwe Kleine-König a écrit :
> Hello,
> 
> this patch isn't checkpatch clean.

Yes, I've seen that.
It's because checkpatch doesn't detect that PWM_XY_SRC_MUX/GATE/DIV are
structures
If I unwrap PWM_XY_SRC_MUX/GATE/DIV and PWM_X_DIV, checkpatch doesn't
complain anymore (indeed, do/while loops are not allowed at file-scope)

I can drop PWM_XY_SRC_MUX/GATE/DIV PWM_X_DIV and declare the structures
directly under PWM_XY_CLK_SRC() and PWM_X_CLK() if you prefer, but I
find it less readable than the current form.

> 
> On Wed, Dec 17, 2025 at 09:25:02AM +0100, Richard Genoud wrote:
>> Add driver for Allwinner H616 PWM controller, supporting up to 6
>> channels.
>> Those channels output can be either a PWM signal output or a clock
>> output, thanks to the bypass.
>>
>> The channels are paired (0/1, 2/3 and 4/5) and each pair has a
>> prescaler/mux/gate.
>> Moreover, each channel has its own prescaler and bypass.
>>
>> Tested-by: Joao Schim <joao@schimsalabim.eu>
>> Signed-off-by: Richard Genoud <richard.genoud@bootlin.com>
>> ---
>>   drivers/pwm/Kconfig           |  12 +
>>   drivers/pwm/Makefile          |   1 +
>>   drivers/pwm/pwm-sun50i-h616.c | 892 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 905 insertions(+)
>>   create mode 100644 drivers/pwm/pwm-sun50i-h616.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 6f3147518376..66534e033761 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -736,6 +736,18 @@ config PWM_SUN4I
>>   	  To compile this driver as a module, choose M here: the module
>>   	  will be called pwm-sun4i.
>>   
>> +config PWM_SUN50I_H616
>> +	tristate "Allwinner H616 PWM support"
>> +	depends on ARCH_SUNXI || COMPILE_TEST
>> +	depends on HAS_IOMEM && COMMON_CLK
>> +	help
>> +	  Generic PWM framework driver for Allwinner H616 SoCs.
>> +	  It supports generic PWM, but can also provides a plain clock.
>> +	  The AC300 PHY integrated in H616 SoC needs such a clock.
>> +
>> +	  To compile this driver as a module, choose M here: the module
>> +	  will be called pwm-h616.
>> +
>>   config PWM_SUNPLUS
>>   	tristate "Sunplus PWM support"
>>   	depends on ARCH_SUNPLUS || COMPILE_TEST
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index 0dc0d2b69025..a16ae9eef9e5 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -67,6 +67,7 @@ obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
>>   obj-$(CONFIG_PWM_STM32_LP)	+= pwm-stm32-lp.o
>>   obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
>>   obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
>> +obj-$(CONFIG_PWM_SUN50I_H616)	+= pwm-sun50i-h616.o
>>   obj-$(CONFIG_PWM_SUNPLUS)	+= pwm-sunplus.o
>>   obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
>>   obj-$(CONFIG_PWM_TH1520)	+= pwm_th1520.o
>> diff --git a/drivers/pwm/pwm-sun50i-h616.c b/drivers/pwm/pwm-sun50i-h616.c
>> new file mode 100644
>> index 000000000000..b002ea5935e4
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-sun50i-h616.c
>> @@ -0,0 +1,892 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Driver for Allwinner H616 Pulse Width Modulation Controller
>> + *
>> + * (C) Copyright 2025 Richard Genoud, Bootlin <richard.genoud@bootlin.com>
>> + *
>> + * Based on drivers/pwm/pwm-sun4i.c with Copyright:
>> + *
>> + * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@bootlin.com>
>> + *
>> + * Limitations:
>> + * - When outputing the source clock directly, the PWM logic is bypassed and the
>> + *   currently running period is not guaranteed to be completed.
>> + * - As the channels are paired (0/1, 2/3, 4/5), they share the same clock
>> + *   source and prescaler(div_m), but they also have their own prescaler(div_k)
>> + *   and bypass.
>> + *
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/bits.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/math64.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/reset.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/time.h>
>> +
>> +#ifndef UINT32_MAX
>> +#define UINT32_MAX 0xffffffffU
>> +#endif
> 
> Please include <linux/limits.h> and use U32_MAX.
OK

> 
>> +
>> +/* PWM IRQ Enable Register */
>> +#define PWM_IER				0x0
> 
> Use a driver specific prefix for your defines. Otherwise these constants
> look more generic than they are.
> 
OK


>> +/* PWM IRQ Status Register */
>> +#define PWM_ISR				0x4
>> +
>> +/* PWM Capture IRQ Enable Register */
>> +#define PWM_CIER			0x10
>> +
>> +/* PWM Capture IRQ Status Register */
>> +#define PWM_CISR			0x14
>> +
>> +/* PWMCC Pairs Clock Configuration Registers */
>> +#define PWM_XY_CLK_CR(pair)		(0x20 + ((pair) * 0x4))
>> +#define PWM_XY_CLK_CR_SRC_SHIFT		7
>> +#define PWM_XY_CLK_CR_SRC_MASK		1
> 
> Please do:
> 
> 	#define H616_PWM_XY_CLK_CR_SRC		BIT(7)
> 
> and then use the helper functions from bitfield.h
> 
That was my initial though, but clk_mux struct wants a shift *and* an
unshifted mask.
cf:
https://elixir.bootlin.com/linux/v6.18.3/source/drivers/clk/clk-mux.c#L93-L94

>> +#define PWM_XY_CLK_CR_GATE_BIT		4
>> +#define PWM_XY_CLK_CR_BYPASS_BIT(chan)	((chan) % 2 + 5)
>> +#define PWM_XY_CLK_CR_DIV_M_SHIFT	0
>> +
>> +/* PWMCC Pairs Dead Zone Control Registers */
>> +#define PWM_XY_DZ(pair)			(0x30 + ((pair) * 0x4))
>> +
>> +/* PWM Enable Register */
>> +#define PWM_ENR				0x40
>> +#define PWM_ENABLE(x)			BIT(x)
> 
> Please use the full register name as prefix for bitfields.
> 
OK


>> +/* PWM Capture Enable Register */
>> +#define PWM_CER				0x44
>> +
>> +/* PWM Control Register */
>> +#define PWM_CTRL_REG(chan)		(0x60 + (chan) * 0x20)
>> +#define PWM_CTRL_PRESCAL_K_SHIFT	0
>> +#define PWM_CTRL_PRESCAL_K_WIDTH	8
>> +#define PWM_CTRL_ACTIVE_STATE		BIT(8)
>> +
>> +/* PWM Period Register */
>> +#define PWM_PERIOD_REG(ch)		(0x64 + (ch) * 0x20)
>> +#define PWM_PERIOD_MASK			GENMASK(31, 16)
>> +#define PWM_DUTY_MASK			GENMASK(15, 0)
>> +#define PWM_REG_PERIOD(reg)		(FIELD_GET(PWM_PERIOD_MASK, reg) + 1)
>> +#define PWM_REG_DUTY(reg)		FIELD_GET(PWM_DUTY_MASK, reg)
>> +#define PWM_PERIOD(prd)			FIELD_PREP(PWM_PERIOD_MASK, (prd) - 1)
>> +#define PWM_DUTY(dty)			FIELD_PREP(PWM_DUTY_MASK, dty)
>> +#define PWM_PERIOD_MAX			FIELD_MAX(PWM_PERIOD_MASK)
>> +
>> +
>> +/* PWM Count Register */
>> +#define PWM_CNT_REG(x)			(0x68 + (x) * 0x20)
>> +
>> +/* PWM Capture Control Register */
>> +#define PWM_CCR(x)			(0x6c + (x) * 0x20)
>> +
>> +/* PWM Capture Rise Lock Register */
>> +#define PWM_CRLR(x)			(0x70 + (x) * 0x20)
>> +
>> +/* PWM Capture Fall Lock Register */
>> +#define PWM_CFLR(x)			(0x74 + (x) * 0x20)
>> +
>> +#define PWM_PAIR_IDX(chan)		((chan) >> 2)
>> +
>> +/*
>> + * Block diagram of the PWM clock controller:
>> + *
>> + *             _____      ______      ________
>> + * OSC24M --->|     |    |      |    |        |
>> + * APB1 ----->| Mux |--->| Gate |--->| /div_m |-----> PWM_clock_src_xy
>> + *            |_____|    |______|    |________|
>> + *                           ________
>> + *                          |        |
>> + *                       +->| /div_k |---> PWM_clock_x
>> + *                       |  |________|
>> + *                       |    ______
>> + *                       |   |      |
>> + *                       +-->| Gate |----> PWM_bypass_clock_x
>> + *                       |   |______|
>> + * PWM_clock_src_xy -----+   ________
>> + *                       |  |        |
>> + *                       +->| /div_k |---> PWM_clock_y
>> + *                       |  |________|
>> + *                       |    ______
>> + *                       |   |      |
>> + *                       +-->| Gate |----> PWM_bypass_clock_y
>> + *                           |______|
>> + *
>> + * NB: when the bypass is set, all the PWM logic is bypassed.
>> + * So, the duty cycle and polarity can't be modified (we just have a clock).
>> + * The bypass in PWM mode is used to achieve a 1/2 duty cycle with the fastest
>> + * clock.
> 
> 1/2 *relative* duty cycle.
> 
OK

>> + *
>> + * PWM_clock_x/y serve for the PWM purpose.
>> + * PWM_bypass_clock_x/y serve for the clock-provider purpose.
>> + *
>> + */
>> +
>> +/*
>> + * Table used for /div_m (diviser before obtaining PWM_clock_src_xy)
>> + * It's actually CLK_DIVIDER_POWER_OF_TWO, but limited to /256
>> + */
>> +static const struct clk_div_table clk_table_div_m[] = {
>> +	{ .val = 0, .div = 1, },
>> +	{ .val = 1, .div = 2, },
>> +	{ .val = 2, .div = 4, },
>> +	{ .val = 3, .div = 8, },
>> +	{ .val = 4, .div = 16, },
>> +	{ .val = 5, .div = 32, },
>> +	{ .val = 6, .div = 64, },
>> +	{ .val = 7, .div = 128, },
>> +	{ .val = 8, .div = 256, },
>> +	{ .val = 0, .div = 0, }, /* last entry */
>> +};
> 
> .div is just 1 << .val, so I would expect you can better use math
> expressions instead of this table.
> 
OK

>> +#define PWM_XY_SRC_GATE(_pair, _reg)			\
>> +struct clk_gate gate_xy_##_pair = {			\
>> +	.reg = (void *)_reg,				\
>> +	.bit_idx = PWM_XY_CLK_CR_GATE_BIT,		\
>> +	.hw.init = &(struct clk_init_data){		\
>> +		.ops =  &clk_gate_ops,			\
>> +	}						\
> 
> I would consider
> 
> 	.hw.init.ops = ...;
> 
> and
> 
> 	.hw = {
> 		.init = {
> 			.ops = ...;
> 		},
> 	},
> 
> natural here. The middleway you chose looks strange to me.

Actually, it's:
	.hw.init->ops = ... ;
That's why I used this middleway construct.


> Also s/  / /.
> 
OK

>> +}
>> +
>> +#define PWM_XY_SRC_MUX(_pair, _reg)			\
>> +struct clk_mux mux_xy_##_pair = {			\
>> +	.reg = (void *)_reg,				\
>> +	.shift = PWM_XY_CLK_CR_SRC_SHIFT,		\
>> +	.mask = PWM_XY_CLK_CR_SRC_MASK,			\
> 
> Huh, why does this structure has both a shift and a mask value? What is
> the difference between
> 
> 	.shift = 7,
> 	.mask = 1,
> 
> and
> 
> 	.shift = 0,
> 	.mask = 1 << 7,
> 
> ? If the latter is equivalent, you could just pass
> H616_PWM_XY_CLK_CR_SRC and get rid of the extra definitions for _SHIFT
> and _MASK.
> 
Unfortunately, struct clk_mux wants a shift and an unshifted mask, and
according to:
https://elixir.bootlin.com/linux/v6.18.3/source/drivers/clk/clk-mux.c#L93-L94
using a 0 shift and mask = 1 << 7 won't work.

>> +	.flags = CLK_MUX_ROUND_CLOSEST,			\
>> +	.hw.init = &(struct clk_init_data){		\
>> +		.ops =  &clk_mux_ops,			\
>> +	}						\
>> +}
>> +
>> +#define PWM_XY_SRC_DIV(_pair, _reg)			\
>> +struct clk_divider rate_xy_##_pair = {			\
>> +	.reg = (void *)_reg,				\
>> +	.shift = PWM_XY_CLK_CR_DIV_M_SHIFT,		\
>> +	.table = clk_table_div_m,			\
>> +	.hw.init = &(struct clk_init_data){		\
>> +		.ops =  &clk_divider_ops,		\
>> +	}						\
>> +}
>> +
>> +#define PWM_X_DIV(_idx, _reg)				\
>> +struct clk_divider rate_x_##_idx = {			\
>> +	.reg = (void *)_reg,				\
>> +	.shift = PWM_CTRL_PRESCAL_K_SHIFT,		\
>> +	.width = PWM_CTRL_PRESCAL_K_WIDTH,		\
>> +	.hw.init = &(struct clk_init_data){		\
>> +		.ops =  &clk_divider_ops,		\
>> +	}						\
>> +}
>> +
>> +#define PWM_X_BYPASS_GATE(_idx)				\
>> +struct clk_gate gate_x_bypass_##_idx = {		\
>> +	.reg = (void *)PWM_ENR,				\
>> +	.bit_idx = _idx,				\
>> +	.hw.init = &(struct clk_init_data){		\
>> +		.ops =  &clk_gate_ops,			\
>> +	}						\
>> +}
>> +
>> +#define PWM_XY_CLK_SRC(_pair, _reg)			\
>> +	static PWM_XY_SRC_MUX(_pair, _reg);		\
>> +	static PWM_XY_SRC_GATE(_pair, _reg);		\
>> +	static PWM_XY_SRC_DIV(_pair, _reg)
>> +
>> +#define PWM_X_CLK(_idx)					\
>> +	static PWM_X_DIV(_idx, PWM_CTRL_REG(_idx))
>> +
>> +#define PWM_X_BYPASS_CLK(_idx)						\
>> +	PWM_X_BYPASS_GATE(_idx)
>> +
>> +#define REF_CLK_XY_SRC(_pair)						\
>> +	{								\
>> +		.name = "pwm-clk-src" #_pair,				\
>> +		.mux_hw = &mux_xy_##_pair.hw,				\
>> +		.gate_hw = &gate_xy_##_pair.hw,				\
>> +		.rate_hw = &rate_xy_##_pair.hw,				\
>> +	}
>> +
>> +#define REF_CLK_X(_idx, _pair)						\
>> +	{								\
>> +		.name = "pwm-clk" #_idx,				\
>> +		.parent_names = (const char *[]){ "pwm-clk-src" #_pair }, \
>> +		.num_parents = 1,					\
>> +		.rate_hw = &rate_x_##_idx.hw,				\
>> +		.flags = CLK_SET_RATE_PARENT,				\
>> +	}
>> +
>> +#define REF_CLK_BYPASS(_idx, _pair)					\
>> +	{								\
>> +		.name = "pwm-clk-bypass" #_idx,				\
>> +		.parent_names = (const char *[]){ "pwm-clk-src" #_pair }, \
>> +		.num_parents = 1,					\
>> +		.gate_hw = &gate_x_bypass_##_idx.hw,			\
>> +		.flags = CLK_SET_RATE_PARENT,	\
>> +	}
>> +
>> +/*
>> + * PWM_clock_src_xy generation:
>> + *             _____      ______      ________
>> + * OSC24M --->|     |    |      |    |        |
>> + * APB1 ----->| Mux |--->| Gate |--->| /div_m |-----> PWM_clock_src_xy
>> + *            |_____|    |______|    |________|
>> + */
>> +PWM_XY_CLK_SRC(01, PWM_XY_CLK_CR(0));
>> +PWM_XY_CLK_SRC(23, PWM_XY_CLK_CR(1));
>> +PWM_XY_CLK_SRC(45, PWM_XY_CLK_CR(2));
>> +
>> +/*
>> + * PWM_clock_x_div generation:
>> + *                       ________
>> + *                      |        | PWM_clock_x/y
>> + * PWM_clock_src_xy --->| /div_k |--------------->
>> + *                      |________|
>> + */
>> +PWM_X_CLK(0);
>> +PWM_X_CLK(1);
>> +PWM_X_CLK(2);
>> +PWM_X_CLK(3);
>> +PWM_X_CLK(4);
>> +PWM_X_CLK(5);
>> +
>> +/*
>> + * PWM_bypass_clock_xy generation:
>> + *                        ______
>> + *                       |      |
>> + * PWM_clock_src_xy ---->| Gate |-------> PWM_bypass_clock_x
>> + *                       |______|
>> + *
>> + * The gate is actually PWM_ENR register.
>> + */
>> +PWM_X_BYPASS_CLK(0);
>> +PWM_X_BYPASS_CLK(1);
>> +PWM_X_BYPASS_CLK(2);
>> +PWM_X_BYPASS_CLK(3);
>> +PWM_X_BYPASS_CLK(4);
>> +PWM_X_BYPASS_CLK(5);
>> +
>> +struct clk_pwm_data {
>> +	const char *name;
>> +	const char **parent_names;
>> +	unsigned int num_parents;
>> +	struct clk_hw *mux_hw;
>> +	struct clk_hw *rate_hw;
>> +	struct clk_hw *gate_hw;
>> +	unsigned long flags;
>> +};
>> +
>> +#define CLK_BYPASS(h616chip, ch) ((h616chip)->data->npwm + (ch))
>> +#define CLK_XY_SRC_IDX(h616chip, ch) ((h616chip)->data->npwm * 2 + ((ch) >> 1))
>> +static struct clk_pwm_data pwmcc_data[] = {
>> +	REF_CLK_X(0, 01),
>> +	REF_CLK_X(1, 01),
>> +	REF_CLK_X(2, 23),
>> +	REF_CLK_X(3, 23),
>> +	REF_CLK_X(4, 45),
>> +	REF_CLK_X(5, 45),
>> +	REF_CLK_BYPASS(0, 01),
>> +	REF_CLK_BYPASS(1, 01),
>> +	REF_CLK_BYPASS(2, 23),
>> +	REF_CLK_BYPASS(3, 23),
>> +	REF_CLK_BYPASS(4, 45),
>> +	REF_CLK_BYPASS(5, 45),
>> +	REF_CLK_XY_SRC(01),
>> +	REF_CLK_XY_SRC(23),
>> +	REF_CLK_XY_SRC(45),
>> +	{ /* sentinel */ },
>> +};
>> +
>> +enum h616_pwm_mode {
>> +	H616_PWM_MODE_NONE,
>> +	H616_PWM_MODE_PWM,
>> +	H616_PWM_MODE_CLK,
>> +};
>> +
>> +struct h616_pwm_data {
>> +	unsigned int npwm;
>> +};
>> +
>> +struct h616_pwm_channel {
>> +	struct clk *pwm_clk;
>> +	unsigned long rate;
>> +	unsigned int entire_cycles;
>> +	unsigned int active_cycles;
>> +	enum h616_pwm_mode mode;
>> +	bool bypass;
>> +};
>> +
>> +struct clk_pwm_pdata {
>> +	struct clk_hw_onecell_data *hw_data;
>> +	spinlock_t lock;
>> +	void __iomem *reg;
>> +};
>> +
>> +struct h616_pwm_chip {
>> +	struct clk_pwm_pdata *clk_pdata;
>> +	struct h616_pwm_channel *channels;
>> +	struct clk *bus_clk;
>> +	struct reset_control *rst;
>> +	void __iomem *base;
>> +	const struct h616_pwm_data *data;
>> +};
>> +
>> +static inline struct h616_pwm_chip *to_h616_pwm_chip(struct pwm_chip *chip)
> 
> It probably doesn't help much, but conceptually this could be
> 
> 	static inline struct h616_pwm_chip *to_h616_pwm_chip(const struct pwm_chip *chip)
> 
Indeed, I'll add that.

>> +{
>> +	return pwmchip_get_drvdata(chip);
>> +}
>> +
>> +static inline u32 h616_pwm_readl(struct h616_pwm_chip *h616chip,
>> +				 unsigned long offset)
>> +{
>> +	return readl(h616chip->base + offset);
>> +}
>> +
>> +static inline void h616_pwm_writel(struct h616_pwm_chip *h616chip,
>> +				   u32 val, unsigned long offset)
>> +{
>> +	writel(val, h616chip->base + offset);
>> +}
>> +
>> +static void h616_pwm_set_bypass(struct h616_pwm_chip *h616chip, unsigned int idx,
>> +				bool en_bypass)
>> +{
>> +	unsigned long flags;
>> +	u32 val;
>> +
>> +	spin_lock_irqsave(&h616chip->clk_pdata->lock, flags);
>> +
>> +	val = h616_pwm_readl(h616chip, PWM_XY_CLK_CR(PWM_PAIR_IDX(idx)));
>> +	if (en_bypass)
>> +		val |= BIT(PWM_XY_CLK_CR_BYPASS_BIT(idx));
>> +	else
>> +		val &= ~BIT(PWM_XY_CLK_CR_BYPASS_BIT(idx));
>> +
>> +	h616_pwm_writel(h616chip, val, PWM_XY_CLK_CR(PWM_PAIR_IDX(idx)));
>> +
>> +	spin_unlock_irqrestore(&h616chip->clk_pdata->lock, flags);
>> +}
>> +
>> +static int h616_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +	struct h616_pwm_chip *h616chip = to_h616_pwm_chip(chip);
>> +	struct h616_pwm_channel *chan = &h616chip->channels[pwm->hwpwm];
>> +	struct device *dev = pwmchip_parent(chip);
>> +	unsigned long flags;
>> +	int ret = 0;
>> +
>> +	spin_lock_irqsave(&h616chip->clk_pdata->lock, flags);
>> +
>> +	if (chan->mode == H616_PWM_MODE_CLK)
>> +		ret = -EBUSY;
>> +	else
>> +		chan->mode = H616_PWM_MODE_PWM;
>> +
>> +	spin_unlock_irqrestore(&h616chip->clk_pdata->lock, flags);
>> +	if (ret)
>> +		goto out;
> 
> You could simplify that to:
> 
> 	scoped_guard(spinlock_irqsave, &h616chip->clk_pdata->lock) {
> 		if (chan->mode == H616_PWM_MODE_CLK)
> 			return -EBUSY;
> 		else
> 			chan->mode = H616_PWM_MODE_PWM;
> 	}
> 
Oh! that's nice!

>> +
>> +	ret = clk_prepare_enable(chan->pwm_clk);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to enable clock %s: %d\n",
>> +			__clk_get_name(chan->pwm_clk), ret);
>> +	}
> 
> Please no error messages in the runtime callbacks.
> 
OK

>> +out:
>> +	return ret;
>> +}
>> +
>> +static void h616_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +	struct h616_pwm_chip *h616chip = to_h616_pwm_chip(chip);
>> +	struct h616_pwm_channel *chan = &h616chip->channels[pwm->hwpwm];
>> +
>> +	clk_disable_unprepare(chan->pwm_clk);
>> +	chan->mode = H616_PWM_MODE_NONE;
>> +}
>> +
>> +static int h616_pwm_get_state(struct pwm_chip *chip,
>> +			      struct pwm_device *pwm,
>> +			      struct pwm_state *state)
>> +{
>> +	struct h616_pwm_chip *h616chip = to_h616_pwm_chip(chip);
>> +	struct h616_pwm_channel *chan = &h616chip->channels[pwm->hwpwm];
>> +	u64 clk_rate, tmp;
>> +	u32 val;
>> +
>> +	clk_rate = clk_get_rate(chan->pwm_clk);
>> +	if (!clk_rate)
>> +		return -EINVAL;
>> +
>> +	val = h616_pwm_readl(h616chip, PWM_ENR);
>> +	state->enabled = !!(PWM_ENABLE(pwm->hwpwm) & val);
>> +
>> +	val = h616_pwm_readl(h616chip, PWM_XY_CLK_CR(PWM_PAIR_IDX(pwm->hwpwm)));
>> +	if (val & BIT(PWM_XY_CLK_CR_BYPASS_BIT(pwm->hwpwm))) {
>> +		/*
>> +		 * When bypass is enabled, the PWM logic is inactive.
>> +		 * The PWM_clock_src_xy is directly routed to PWM_clock_x
>> +		 */
>> +		state->period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, clk_rate);
>> +		state->duty_cycle = DIV_ROUND_UP_ULL(state->period, 2);
>> +		state->polarity = PWM_POLARITY_NORMAL;
>> +		return 0;
>> +	}
>> +
>> +	state->enabled &= !!(BIT(PWM_XY_CLK_CR_GATE_BIT) & val);
>> +
>> +	val = h616_pwm_readl(h616chip, PWM_CTRL_REG(pwm->hwpwm));
>> +	if (val & PWM_CTRL_ACTIVE_STATE)
>> +		state->polarity = PWM_POLARITY_NORMAL;
>> +	else
>> +		state->polarity = PWM_POLARITY_INVERSED;
>> +
>> +	val = h616_pwm_readl(h616chip, PWM_PERIOD_REG(pwm->hwpwm));
>> +
>> +	tmp = NSEC_PER_SEC * PWM_REG_DUTY(val);
>> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
>> +
>> +	tmp = NSEC_PER_SEC * PWM_REG_PERIOD(val);
>> +	state->period = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
> 
> This needs to use uprounding. Ideally enabling CONFIG_PWM_DEBUG and
> extensive testing should catch that error.
> 
OK


>> +
>> +	return 0;
>> +}
>> +
>> +static int h616_pwm_calc(struct pwm_chip *chip, unsigned int idx,
>> +			 const struct pwm_state *state)
>> +{
>> +	struct h616_pwm_chip *h616chip = to_h616_pwm_chip(chip);
>> +	struct h616_pwm_channel *chan = &h616chip->channels[idx];
>> +	unsigned int cnt, duty_cnt;
>> +	unsigned long max_rate;
>> +	long calc_rate;
>> +	u64 duty, period, freq;
>> +
>> +	duty = state->duty_cycle;
>> +	period = state->period;
>> +
>> +	max_rate = clk_round_rate(chan->pwm_clk, UINT32_MAX);
> 
> Huh, is this an artificial limitation? Do you rely on how clk_round_rate
> picks the return value? (I.e. nearest value? nearest time? round up?
> round down?)
> 

What I want to achieve here is to handle the lowest possible period case.
Without the bypass, the lowest period is build with the highest clock,
duty cycle = 1 and period cycle = 2 (0x10001 in period register)
So, if the input clock is 100MHz, we have a lowest period of 20ns.
Now, if we enable the bypass, the period register is ignored and we directly
have the 100MHz clock as PWM output, that can act as a 10ns period with 5ns
duty
So the logic here is to detect this specific lowest period case that can be
achieved by enabling the bypass.
For that, the highest clock is retrieved and compared to the wanted period
and duty.

Now, I learned the hard way that clk_round_rate() is actually limited to a
32 bits value:
clk_round_rate() calls at one point clk_divider_bestdiv() that uses
DIV_ROUND_UP_ULL() which in turn uses do_div(n,base):
   do_div - returns 2 values: calculate remainder and update new dividend
   @n: uint64_t dividend (will be updated)
   @base: uint32_t divisor
So, in order to get the exact highest clock rate, I use clk_round_rate()
with U32_MAX.

>> +	dev_dbg(pwmchip_parent(chip), "max_rate: %ld Hz\n", max_rate);
>> +
>> +	if ((period * max_rate >= NSEC_PER_SEC) &&
> 
> If period is big, this overflows.
> 
Indeed, I'll fix that.


>> +	    (period * max_rate < 2 * NSEC_PER_SEC) &&
>> +	    (duty * max_rate * 2 >= NSEC_PER_SEC)) {
>> +		/*
>> +		 * If the requested period is to small to be generated by the
>> +		 * PWM, we can just select the highest clock and bypass the
>> +		 * PWM logic
>> +		 */
>> +		dev_dbg(pwmchip_parent(chip), "Setting bypass (period=%lld)\n",
>> +			period);
>> +		freq = div64_u64(NSEC_PER_SEC, period);
>> +		chan->bypass = true;
>> +		duty = period / 2;
> 
> You must not round up duty. So if a small period and duty_cycle <
> period/2 is requested, you have to return an error value. (Note that
> with waveforms the semantic is a bit different.)
> 
OK


>> +	} else {
>> +		chan->bypass = false;
>> +		freq = div64_u64(NSEC_PER_SEC * (u64)PWM_PERIOD_MAX, period);
>> +		if (freq > UINT32_MAX)
>> +			freq = UINT32_MAX;
>> +	}
>> +
>> +	calc_rate = clk_round_rate(chan->pwm_clk, freq);
>> +	if (calc_rate <= 0) {
>> +		dev_err(pwmchip_parent(chip),
>> +			"Invalid source clock frequency %llu\n", freq);
>> +		return calc_rate ? calc_rate : -EINVAL;
>> +	}
>> +
>> +	dev_dbg(pwmchip_parent(chip), "calc_rate: %ld Hz\n", calc_rate);
>> +
>> +	cnt = mul_u64_u64_div_u64(calc_rate, period, NSEC_PER_SEC);
> 
> Please use a better name here, something like period_ticks would be
> good.
> 
OK

>> +	if ((cnt == 0) || (cnt > PWM_PERIOD_MAX)) {
>> +		dev_err(pwmchip_parent(chip), "Period out of range\n");
>> +		return -EINVAL;
>> +	}
> 
> If the requested value is too big, clamp it to the maximal possible
> value.
> 
OK

>> +	duty_cnt = mul_u64_u64_div_u64(calc_rate, duty, NSEC_PER_SEC);
>> +
>> +	if (duty_cnt >= cnt)
>> +		duty_cnt = cnt - 1;
> 
> Does that mean the PWM cannot produce a 100% relative duty cycle?
> 
Hum, it seems it's a left over from an old code.
I'll fix that.

>> +	dev_dbg(pwmchip_parent(chip), "period=%llu cnt=%u duty=%llu duty_cnt=%u\n",
>> +		period, cnt, duty, duty_cnt);
> 
> This is little helpful without the input parameters.
> 
as
	duty = state->duty_cycle;
	period = state->period;
The input parameters are there, right?
But I can add the missing idx.

>> +	chan->active_cycles = duty_cnt;
>> +	chan->entire_cycles = cnt;
>> +
>> +	chan->rate = calc_rate;
>> +
>> +	return 0;
>> +}
>> +
>> +static int h616_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> +			  const struct pwm_state *state)
>> +{
> 
> Please consider implementing .round_waveform_tohw(),
> .round_waveform_fromhw(), .read_waveform() and .write_waveform() instead
> of .apply() and .get_state().
> 
OK


>> +	struct h616_pwm_chip *h616chip = to_h616_pwm_chip(chip);
>> +	struct h616_pwm_channel *chan = &h616chip->channels[pwm->hwpwm];
>> +	struct pwm_state cstate;
>> +	unsigned long flags;
>> +	u32 val;
>> +	int ret;
>> +
>> +	ret = h616_pwm_calc(chip, pwm->hwpwm, state);
>> +	if (ret) {
>> +		dev_err(pwmchip_parent(chip), "period exceeds the maximum value\n");
>> +		return ret;
>> +	}
>> +
>> +	pwm_get_state(pwm, &cstate);
> 
> Don't call PWM API functions. (h616_pwm_apply() is called by
> pwm_apply_{might_sleep,atomic} which grabs the chip lock. That
> pwm_get_state() doesn't is an implementation detail that you should not
> rely on.) I would prefer that you don't access pwm->state at all but
> determine the state by looking at the registers.
> 
OK

>> +	ret = clk_set_rate(chan->pwm_clk, chan->rate);
>> +	if (ret) {
>> +		dev_err(pwmchip_parent(chip), "failed to set PWM %d clock rate to %lu\n",
>> +			pwm->hwpwm, chan->rate);
>> +		return ret;
>> +	}
>> +
>> +	h616_pwm_set_bypass(h616chip, pwm->hwpwm, chan->bypass);
>> +
>> +	/*
>> +	 * If bypass is set, the PWM logic (polarity, duty) can't be applied
>> +	 */
>> +
>> +	if (chan->bypass && (state->polarity == PWM_POLARITY_INVERSED)) {
>> +		dev_warn(pwmchip_parent(chip),
>> +			 "Can't set inversed polarity with bypass enabled\n");
>> +	} else {
>> +		val = h616_pwm_readl(h616chip, PWM_CTRL_REG(pwm->hwpwm));
>> +		val &= ~PWM_CTRL_ACTIVE_STATE;
>> +		if (state->polarity == PWM_POLARITY_NORMAL)
>> +			val |= PWM_CTRL_ACTIVE_STATE;
>> +		h616_pwm_writel(h616chip, val, PWM_CTRL_REG(pwm->hwpwm));
>> +	}
>> +
>> +	if (chan->bypass && (state->duty_cycle * 2 != state->period)) {
>> +		dev_warn(pwmchip_parent(chip),
>> +			 "Can't set a duty cycle with bypass enabled\n");
>> +	}
> 
> Please no output in runtime callbacks. (And even then
> 
> 	state->period / 2 != state->duty_cycle
> 
> would probaly be the nicer check as it also works for uneven periods.)
> 
OK

>> +
>> +	if (!chan->bypass) {
>> +		val = PWM_DUTY(chan->active_cycles);
>> +		val |= PWM_PERIOD(chan->entire_cycles);
>> +		h616_pwm_writel(h616chip, val, PWM_PERIOD_REG(pwm->hwpwm));
>> +	}
>> +
>> +	if (state->enabled == cstate.enabled)
>> +		return 0;
>> +
>> +	if (cstate.enabled) {
>> +		unsigned long delay_us;
>> +
>> +		/*
>> +		 * We need a full period to elapse before
>> +		 * disabling the channel.
> 
> You need a full period between what and disabling? Assuming this is
> about period and duty setting: Why not skip it if you disable?
> 
I'll remove that, it's a leftover from pwm-sun4i.

>> +		 */
>> +		delay_us = DIV_ROUND_UP_ULL(cstate.period, NSEC_PER_USEC);
>> +		fsleep(delay_us);
> 
> Huu, sleeping while holding a spin_lock and having disabled irqs :-\
> 
ouch!

>> +	}
>> +
>> +	spin_lock_irqsave(&h616chip->clk_pdata->lock, flags);
>> +
>> +	val = h616_pwm_readl(h616chip, PWM_ENR);
>> +	if (state->enabled)
>> +		val |= PWM_ENABLE(pwm->hwpwm);
>> +	else
>> +		val &= ~PWM_ENABLE(pwm->hwpwm);
>> +	h616_pwm_writel(h616chip, val, PWM_ENR);
>> +
>> +	spin_unlock_irqrestore(&h616chip->clk_pdata->lock, flags);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct pwm_ops h616_pwm_ops = {
>> +	.apply = h616_pwm_apply,
>> +	.get_state = h616_pwm_get_state,
>> +	.request = h616_pwm_request,
>> +	.free = h616_pwm_free,
>> +};
>> +
>> +static struct clk_hw *h616_pwm_of_clk_get(struct of_phandle_args *clkspec,
>> +					  void *data)
>> +{
>> +	struct h616_pwm_chip *h616chip = data;
>> +	struct clk_hw_onecell_data *hw_data = h616chip->clk_pdata->hw_data;
>> +	unsigned int idx = clkspec->args[0];
>> +	struct h616_pwm_channel *chan;
>> +	struct clk_hw *ret_clk = NULL;
>> +	unsigned long flags;
>> +
>> +	if (idx >= h616chip->data->npwm)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	chan = &h616chip->channels[idx];
>> +
>> +	spin_lock_irqsave(&h616chip->clk_pdata->lock, flags);
>> +
>> +	if (chan->mode == H616_PWM_MODE_PWM) {
>> +		ret_clk = ERR_PTR(-EBUSY);
>> +	} else {
>> +		chan->mode = H616_PWM_MODE_CLK;
>> +		ret_clk = hw_data->hws[CLK_BYPASS(h616chip, idx)];
>> +	}
>> +	spin_unlock_irqrestore(&h616chip->clk_pdata->lock, flags);
>> +
>> +	if (IS_ERR(ret_clk))
>> +		goto out;
>> +
>> +	chan->bypass = true;
>> +	h616_pwm_set_bypass(h616chip, idx, chan->bypass);
>> +out:
>> +	return ret_clk;
>> +}
>> +
>> +static int h616_add_composite_clk(struct clk_pwm_data *data,
>> +				  void __iomem *reg, spinlock_t *lock,
>> +				  struct device *dev, struct clk_hw **hw)
>> +{
>> +	const struct clk_ops *mux_ops = NULL, *gate_ops = NULL, *rate_ops = NULL;
>> +	struct clk_hw *mux_hw = NULL, *gate_hw = NULL, *rate_hw = NULL;
>> +	struct device_node *node = dev->of_node;
>> +
>> +	if (data->mux_hw) {
>> +		struct clk_mux *mux;
>> +
>> +		mux_hw = data->mux_hw;
>> +		mux = to_clk_mux(mux_hw);
>> +		mux->lock = lock;
>> +		mux_ops = mux_hw->init->ops;
>> +		mux->reg = (u64)mux->reg + reg;
>> +	}
>> +
>> +	if (data->gate_hw) {
>> +		struct clk_gate *gate;
>> +
>> +		gate_hw = data->gate_hw;
>> +		gate = to_clk_gate(gate_hw);
>> +		gate->lock = lock;
>> +		gate_ops = gate_hw->init->ops;
>> +		gate->reg = (u64)gate->reg + reg;
>> +	}
>> +
>> +	if (data->rate_hw) {
>> +		struct clk_divider *rate;
>> +
>> +		rate_hw = data->rate_hw;
>> +		rate = to_clk_divider(rate_hw);
>> +		rate_ops = rate_hw->init->ops;
>> +		rate->lock = lock;
>> +		rate->reg = (u64)rate->reg + reg;
>> +
>> +		if (rate->table) {
>> +			const struct clk_div_table *clkt;
>> +			int table_size = 0;
>> +
>> +			for (clkt = rate->table; clkt->div; clkt++)
>> +				table_size++;
>> +			rate->width = order_base_2(table_size);
>> +		}
>> +	}
>> +
>> +	/*
>> +	 * Retrieve the parent clock names from DTS for pwm-clk-srcxy
>> +	 */
>> +	if (!data->parent_names) {
>> +		data->num_parents = of_clk_get_parent_count(node);
>> +		if (data->num_parents == 0)
>> +			return -ENOENT;
>> +
>> +		data->parent_names = devm_kzalloc(dev,
>> +						  sizeof(*data->parent_names),
>> +						  GFP_KERNEL);
>> +		for (unsigned int i = 0; i < data->num_parents; i++)
>> +			data->parent_names[i] = of_clk_get_parent_name(node, i);
>> +	}
>> +
>> +	*hw = clk_hw_register_composite(dev, data->name, data->parent_names,
>> +					data->num_parents, mux_hw,
>> +					mux_ops, rate_hw, rate_ops,
>> +					gate_hw, gate_ops, data->flags);
>> +
>> +	return PTR_ERR_OR_ZERO(*hw);
>> +}
>> +
>> +static int h616_pwm_init_clocks(struct platform_device *pdev,
>> +				struct h616_pwm_chip *h616chip)
>> +{
>> +	struct clk_pwm_pdata *pdata;
>> +	struct device *dev = &pdev->dev;
>> +	int num_clocks = 0;
>> +	int ret;
>> +
>> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +	if (!pdata)
>> +		return -ENOMEM;
>> +
>> +	while (pwmcc_data[num_clocks].name)
>> +		num_clocks++;
>> +
>> +	pdata->hw_data = devm_kzalloc(dev, struct_size(pdata->hw_data, hws, num_clocks),
>> +				      GFP_KERNEL);
>> +	if (!pdata->hw_data)
>> +		return -ENOMEM;
>> +
>> +	pdata->hw_data->num = num_clocks;
>> +	pdata->reg = h616chip->base;
>> +
>> +	spin_lock_init(&pdata->lock);
>> +
>> +	for (int i = 0; i < num_clocks; i++) {
>> +		struct clk_hw **hw = &pdata->hw_data->hws[i];
>> +
>> +		ret = h616_add_composite_clk(&pwmcc_data[i], pdata->reg,
>> +					     &pdata->lock, dev, hw);
>> +		if (ret) {
>> +			dev_err_probe(dev, ret,
>> +				      "Failed to register hw clock %s\n",
>> +				      pwmcc_data[i].name);
>> +			for (i--; i >= 0; i--)
>> +				clk_hw_unregister_composite(pdata->hw_data->hws[i]);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	h616chip->clk_pdata = pdata;
>> +
>> +	return 0;
>> +}
>> +
>> +static int h616_pwm_probe(struct platform_device *pdev)
>> +{
>> +	const struct h616_pwm_data *data;
>> +	struct device *dev = &pdev->dev;
>> +	struct h616_pwm_chip *h616chip;
>> +	struct pwm_chip *chip;
>> +	int ret;
>> +
>> +	data = of_device_get_match_data(dev);
>> +	if (!data)
>> +		return -ENODEV;
> 
> error message please. (Or at least a comment that this cannot happen.)
> 
OK

>> +
>> +	chip = devm_pwmchip_alloc(dev, data->npwm, sizeof(*h616chip));
>> +	if (IS_ERR(chip))
>> +		return dev_err_probe(dev, PTR_ERR(chip),
>> +				     "Failed to allocate pwmchip\n");
>> +
>> +	h616chip = to_h616_pwm_chip(chip);
>> +	h616chip->data = data;
>> +	h616chip->base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(h616chip->base))
>> +		return dev_err_probe(dev, PTR_ERR(h616chip->base),
>> +				     "Failed to get PWM base address\n");
>> +
>> +	h616chip->bus_clk = devm_clk_get_enabled(dev, "bus");
>> +	if (IS_ERR(h616chip->bus_clk))
>> +		return dev_err_probe(dev, PTR_ERR(h616chip->bus_clk),
>> +				     "Failed to get bus clock\n");
>> +
>> +	h616chip->channels = devm_kmalloc_array(dev, data->npwm,
>> +						sizeof(*(h616chip->channels)),
>> +						GFP_KERNEL);
> 
> There is a big amount of allocations here. Would be great to have this
> reduced to save some memory management overhead and get some cache
> locality in return.
> 
Hum, indeed, I can shrink the struct h616_pwm_channel.

>> +	if (!h616chip->channels)
>> +		return dev_err_probe(dev, -ENOMEM,
>> +				     "Failed to allocate %d channels array\n",
>> +				     data->npwm);
>> +
>> +	h616chip->rst = devm_reset_control_get_shared(dev, NULL);
>> +	if (IS_ERR(h616chip->rst))
>> +		return dev_err_probe(dev, PTR_ERR(h616chip->rst),
>> +				     "Failed to get reset control\n");
>> +
>> +	chip->ops = &h616_pwm_ops;
>> +
>> +	ret = h616_pwm_init_clocks(pdev, h616chip);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to initialize clocks\n");
> 
> h616_pwm_init_clocks() already emits error messages.
> 
OK, I'll add missing error messages in h616_pwm_init_clocks() and remove
this one

>> +	for (unsigned int i = 0; i < data->npwm; i++) {
> 
> Huh, AFAIK we're not using declarations in for loops in the kernel.
> 
Actually, I've read somewhere (in LWN I guess) that Linus seems ok with that,
but I'll remove it if you prefer.

>> +		struct h616_pwm_channel *chan = &h616chip->channels[i];
>> +		struct clk_hw **hw = &h616chip->clk_pdata->hw_data->hws[i];
>> +
>> +		chan->pwm_clk = devm_clk_hw_get_clk(dev, *hw, NULL);
>> +		if (IS_ERR(chan->pwm_clk)) {
>> +			ret = dev_err_probe(dev, PTR_ERR(chan->pwm_clk),
>> +					    "Failed to register PWM clock %d\n", i);
>> +			goto err_get_clk;
>> +		}
>> +		chan->mode = H616_PWM_MODE_NONE;
>> +	}
>> +
>> +	ret = devm_of_clk_add_hw_provider(dev, h616_pwm_of_clk_get, h616chip);
>> +	if (ret) {
>> +		dev_err_probe(dev, ret, "Failed to add HW clock provider\n");
>> +		goto err_add_clk_provider;
>> +	}
>> +
>> +	/* Deassert reset */
>> +	ret = reset_control_deassert(h616chip->rst);
>> +	if (ret) {
>> +		dev_err_probe(dev, ret, "Cannot deassert reset control\n");
>> +		goto err_ctrl_deassert;
>> +	}
>> +
>> +	ret = pwmchip_add(chip);
>> +	if (ret < 0) {
>> +		dev_err_probe(dev, ret, "Failed to add PWM chip\n");
>> +		goto err_pwm_add;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, chip);
> 
> this is unused.
> 
Indeed, I'll remove that.

>> +	return 0;
>> +
>> +err_pwm_add:
>> +	reset_control_assert(h616chip->rst);
>> +
>> +err_ctrl_deassert:
>> +err_add_clk_provider:
>> +err_get_clk:
>> +	for (int i = 0; i < h616chip->clk_pdata->hw_data->num; i++)
>> +		clk_hw_unregister_composite(h616chip->clk_pdata->hw_data->hws[i]);
>> +
>> +	return ret;
>> +}
> 
> You need to implement a .remove() callback to not leak resources (or use
> devm functions).
> 
OK

>> +
>> +static const struct h616_pwm_data sun50i_h616_pwm_data = {
>> +	.npwm = 6,
>> +};
>> +
>> +static const struct of_device_id h616_pwm_dt_ids[] = {
>> +	{
>> +		.compatible = "allwinner,sun50i-h616-pwm",
>> +		.data = &sun50i_h616_pwm_data,
>> +	}, {
>> +		/* sentinel */
>> +	},
> 
> no , here.
> 
OK

>> +};
>> +MODULE_DEVICE_TABLE(of, h616_pwm_dt_ids);
>> +
>> +
> 
> A single empty line is enough here.
> 
OK

>> +static struct platform_driver h616_pwm_driver = {
>> +	.driver = {
>> +		.name = "h616-pwm",
>> +		.of_match_table = h616_pwm_dt_ids,
>> +	},
>> +	.probe = h616_pwm_probe,
>> +};
>> +module_platform_driver(h616_pwm_driver);
>> +
>> +MODULE_AUTHOR("Richard Genoud <richard.genoud@bootlin.com>");
>> +MODULE_DESCRIPTION("Allwinner H616 PWM driver");
>> +MODULE_LICENSE("GPL");
> 
> Best regards
> Uwe

Thanks for this review!

Regards,
Richard

Re: [PATCH v2 2/4] pwm: sun50i: Add H616 PWM support

[Uwe Kleine-König]

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

Hello Richard,

On Tue, Jan 06, 2026 at 12:19:59PM +0100, Richard GENOUD wrote:
> Le 24/12/2025 à 10:54, Uwe Kleine-König a écrit :
> > this patch isn't checkpatch clean.
> 
> Yes, I've seen that.
> It's because checkpatch doesn't detect that PWM_XY_SRC_MUX/GATE/DIV are
> structures
> If I unwrap PWM_XY_SRC_MUX/GATE/DIV and PWM_X_DIV, checkpatch doesn't
> complain anymore (indeed, do/while loops are not allowed at file-scope)

At least add parenthesis around usages of _reg. (And if you know that
checkpatch points out things that you don't agree to, preempting the
critic in the cover letter is a good idea.)

> I can drop PWM_XY_SRC_MUX/GATE/DIV PWM_X_DIV and declare the structures
> directly under PWM_XY_CLK_SRC() and PWM_X_CLK() if you prefer, but I
> find it less readable than the current form.

No strong feeling here.
 
> > On Wed, Dec 17, 2025 at 09:25:02AM +0100, Richard Genoud wrote:
> > > +#define PWM_XY_SRC_GATE(_pair, _reg)			\
> > > +struct clk_gate gate_xy_##_pair = {			\
> > > +	.reg = (void *)_reg,				\
> > > +	.bit_idx = PWM_XY_CLK_CR_GATE_BIT,		\
> > > +	.hw.init = &(struct clk_init_data){		\
> > > +		.ops =  &clk_gate_ops,			\
> > > +	}						\
> > 
> > I would consider
> > 
> > 	.hw.init.ops = ...;
> > 
> > and
> > 
> > 	.hw = {
> > 		.init = {
> > 			.ops = ...;
> > 		},
> > 	},
> > 
> > natural here. The middleway you chose looks strange to me.
> 
> Actually, it's:
> 	.hw.init->ops = ... ;
> That's why I used this middleway construct.

Ah I see. Then it's fine for me.

> > > +#define PWM_XY_SRC_MUX(_pair, _reg)			\
> > > +struct clk_mux mux_xy_##_pair = {			\
> > > +	.reg = (void *)_reg,				\
> > > +	.shift = PWM_XY_CLK_CR_SRC_SHIFT,		\
> > > +	.mask = PWM_XY_CLK_CR_SRC_MASK,			\
> > 
> > Huh, why does this structure has both a shift and a mask value? What is
> > the difference between
> > 
> > 	.shift = 7,
> > 	.mask = 1,
> > 
> > and
> > 
> > 	.shift = 0,
> > 	.mask = 1 << 7,
> > 
> > ? If the latter is equivalent, you could just pass
> > H616_PWM_XY_CLK_CR_SRC and get rid of the extra definitions for _SHIFT
> > and _MASK.
> > 
> Unfortunately, struct clk_mux wants a shift and an unshifted mask, and
> according to:
> https://elixir.bootlin.com/linux/v6.18.3/source/drivers/clk/clk-mux.c#L93-L94
> using a 0 shift and mask = 1 << 7 won't work.

How annoying, I put that on my todo list ...

> > > +static inline struct h616_pwm_chip *to_h616_pwm_chip(struct pwm_chip *chip)
> > 
> > It probably doesn't help much, but conceptually this could be
> > 
> > 	static inline struct h616_pwm_chip *to_h616_pwm_chip(const struct pwm_chip *chip)
> > 
> Indeed, I'll add that.

If you rename it to h616_pwm_from_chip it even has the driver specific
prefix.

> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int h616_pwm_calc(struct pwm_chip *chip, unsigned int idx,
> > > +			 const struct pwm_state *state)
> > > +{
> > > +	struct h616_pwm_chip *h616chip = to_h616_pwm_chip(chip);
> > > +	struct h616_pwm_channel *chan = &h616chip->channels[idx];
> > > +	unsigned int cnt, duty_cnt;
> > > +	unsigned long max_rate;
> > > +	long calc_rate;
> > > +	u64 duty, period, freq;
> > > +
> > > +	duty = state->duty_cycle;
> > > +	period = state->period;
> > > +
> > > +	max_rate = clk_round_rate(chan->pwm_clk, UINT32_MAX);
> > 
> > Huh, is this an artificial limitation? Do you rely on how clk_round_rate
> > picks the return value? (I.e. nearest value? nearest time? round up?
> > round down?)
> 
> What I want to achieve here is to handle the lowest possible period case.
> Without the bypass, the lowest period is build with the highest clock,
> duty cycle = 1 and period cycle = 2 (0x10001 in period register)
> So, if the input clock is 100MHz, we have a lowest period of 20ns.
> Now, if we enable the bypass, the period register is ignored and we directly
> have the 100MHz clock as PWM output, that can act as a 10ns period with 5ns
> duty
> So the logic here is to detect this specific lowest period case that can be
> achieved by enabling the bypass.
> For that, the highest clock is retrieved and compared to the wanted period
> and duty.
> 
> Now, I learned the hard way that clk_round_rate() is actually limited to a
> 32 bits value:
> clk_round_rate() calls at one point clk_divider_bestdiv() that uses
> DIV_ROUND_UP_ULL() which in turn uses do_div(n,base):
>   do_div - returns 2 values: calculate remainder and update new dividend
>   @n: uint64_t dividend (will be updated)
>   @base: uint32_t divisor
> So, in order to get the exact highest clock rate, I use clk_round_rate()
> with U32_MAX.

IMHO this should be addressed in the clk framework. And until this
happend the code in the pwm driver needs a verbose comment.
 
> > > +	dev_dbg(pwmchip_parent(chip), "period=%llu cnt=%u duty=%llu duty_cnt=%u\n",
> > > +		period, cnt, duty, duty_cnt);
> > 
> > This is little helpful without the input parameters.
> > 
> as
> 	duty = state->duty_cycle;
> 	period = state->period;
> The input parameters are there, right?
> But I can add the missing idx.

Today I agree, don't know why I missed that. It would be nice to stick
to the format that e.g. pwm-stm32 uses for the input parameters to make
me notice even on busy days that the input parameters are there :-)

> > > +	for (unsigned int i = 0; i < data->npwm; i++) {
> > 
> > Huh, AFAIK we're not using declarations in for loops in the kernel.
>
> Actually, I've read somewhere (in LWN I guess) that Linus seems ok with that,
> but I'll remove it if you prefer.

If you find your source again, I'd be interested.

Best regards
Uwe

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

Re: [PATCH v2 2/4] pwm: sun50i: Add H616 PWM support

[Chen-Yu Tsai]

On Wed, Jan 7, 2026 at 12:27 AM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
>
> Hello Richard,
>
> On Tue, Jan 06, 2026 at 12:19:59PM +0100, Richard GENOUD wrote:
> > Le 24/12/2025 à 10:54, Uwe Kleine-König a écrit :
> > > this patch isn't checkpatch clean.
> >
> > Yes, I've seen that.
> > It's because checkpatch doesn't detect that PWM_XY_SRC_MUX/GATE/DIV are
> > structures
> > If I unwrap PWM_XY_SRC_MUX/GATE/DIV and PWM_X_DIV, checkpatch doesn't
> > complain anymore (indeed, do/while loops are not allowed at file-scope)
>
> At least add parenthesis around usages of _reg. (And if you know that
> checkpatch points out things that you don't agree to, preempting the
> critic in the cover letter is a good idea.)
>
> > I can drop PWM_XY_SRC_MUX/GATE/DIV PWM_X_DIV and declare the structures
> > directly under PWM_XY_CLK_SRC() and PWM_X_CLK() if you prefer, but I
> > find it less readable than the current form.
>
> No strong feeling here.
>
> > > On Wed, Dec 17, 2025 at 09:25:02AM +0100, Richard Genoud wrote:
> > > > +#define PWM_XY_SRC_GATE(_pair, _reg)                     \
> > > > +struct clk_gate gate_xy_##_pair = {                      \
> > > > + .reg = (void *)_reg,                            \
> > > > + .bit_idx = PWM_XY_CLK_CR_GATE_BIT,              \
> > > > + .hw.init = &(struct clk_init_data){             \
> > > > +         .ops =  &clk_gate_ops,                  \
> > > > + }                                               \
> > >
> > > I would consider
> > >
> > >     .hw.init.ops = ...;
> > >
> > > and
> > >
> > >     .hw = {
> > >             .init = {
> > >                     .ops = ...;
> > >             },
> > >     },
> > >
> > > natural here. The middleway you chose looks strange to me.
> >
> > Actually, it's:
> >       .hw.init->ops = ... ;
> > That's why I used this middleway construct.
>
> Ah I see. Then it's fine for me.
>
> > > > +#define PWM_XY_SRC_MUX(_pair, _reg)                      \
> > > > +struct clk_mux mux_xy_##_pair = {                        \
> > > > + .reg = (void *)_reg,                            \
> > > > + .shift = PWM_XY_CLK_CR_SRC_SHIFT,               \
> > > > + .mask = PWM_XY_CLK_CR_SRC_MASK,                 \
> > >
> > > Huh, why does this structure has both a shift and a mask value? What is
> > > the difference between
> > >
> > >     .shift = 7,
> > >     .mask = 1,
> > >
> > > and
> > >
> > >     .shift = 0,
> > >     .mask = 1 << 7,
> > >
> > > ? If the latter is equivalent, you could just pass
> > > H616_PWM_XY_CLK_CR_SRC and get rid of the extra definitions for _SHIFT
> > > and _MASK.
> > >
> > Unfortunately, struct clk_mux wants a shift and an unshifted mask, and
> > according to:
> > https://elixir.bootlin.com/linux/v6.18.3/source/drivers/clk/clk-mux.c#L93-L94
> > using a 0 shift and mask = 1 << 7 won't work.
>
> How annoying, I put that on my todo list ...
>
> > > > +static inline struct h616_pwm_chip *to_h616_pwm_chip(struct pwm_chip *chip)
> > >
> > > It probably doesn't help much, but conceptually this could be
> > >
> > >     static inline struct h616_pwm_chip *to_h616_pwm_chip(const struct pwm_chip *chip)
> > >
> > Indeed, I'll add that.
>
> If you rename it to h616_pwm_from_chip it even has the driver specific
> prefix.
>
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int h616_pwm_calc(struct pwm_chip *chip, unsigned int idx,
> > > > +                  const struct pwm_state *state)
> > > > +{
> > > > + struct h616_pwm_chip *h616chip = to_h616_pwm_chip(chip);
> > > > + struct h616_pwm_channel *chan = &h616chip->channels[idx];
> > > > + unsigned int cnt, duty_cnt;
> > > > + unsigned long max_rate;
> > > > + long calc_rate;
> > > > + u64 duty, period, freq;
> > > > +
> > > > + duty = state->duty_cycle;
> > > > + period = state->period;
> > > > +
> > > > + max_rate = clk_round_rate(chan->pwm_clk, UINT32_MAX);
> > >
> > > Huh, is this an artificial limitation? Do you rely on how clk_round_rate
> > > picks the return value? (I.e. nearest value? nearest time? round up?
> > > round down?)
> >
> > What I want to achieve here is to handle the lowest possible period case.
> > Without the bypass, the lowest period is build with the highest clock,
> > duty cycle = 1 and period cycle = 2 (0x10001 in period register)
> > So, if the input clock is 100MHz, we have a lowest period of 20ns.
> > Now, if we enable the bypass, the period register is ignored and we directly
> > have the 100MHz clock as PWM output, that can act as a 10ns period with 5ns
> > duty
> > So the logic here is to detect this specific lowest period case that can be
> > achieved by enabling the bypass.
> > For that, the highest clock is retrieved and compared to the wanted period
> > and duty.
> >
> > Now, I learned the hard way that clk_round_rate() is actually limited to a
> > 32 bits value:
> > clk_round_rate() calls at one point clk_divider_bestdiv() that uses
> > DIV_ROUND_UP_ULL() which in turn uses do_div(n,base):
> >   do_div - returns 2 values: calculate remainder and update new dividend
> >   @n: uint64_t dividend (will be updated)
> >   @base: uint32_t divisor
> > So, in order to get the exact highest clock rate, I use clk_round_rate()
> > with U32_MAX.
>
> IMHO this should be addressed in the clk framework. And until this
> happend the code in the pwm driver needs a verbose comment.
>
> > > > + dev_dbg(pwmchip_parent(chip), "period=%llu cnt=%u duty=%llu duty_cnt=%u\n",
> > > > +         period, cnt, duty, duty_cnt);
> > >
> > > This is little helpful without the input parameters.
> > >
> > as
> >       duty = state->duty_cycle;
> >       period = state->period;
> > The input parameters are there, right?
> > But I can add the missing idx.
>
> Today I agree, don't know why I missed that. It would be nice to stick
> to the format that e.g. pwm-stm32 uses for the input parameters to make
> me notice even on busy days that the input parameters are there :-)
>
> > > > + for (unsigned int i = 0; i < data->npwm; i++) {
> > >
> > > Huh, AFAIK we're not using declarations in for loops in the kernel.
> >
> > Actually, I've read somewhere (in LWN I guess) that Linus seems ok with that,
> > but I'll remove it if you prefer.
>
> If you find your source again, I'd be interested.

IIRC Linus said this is the one case that made sense for "declare anywhere".

I dug up this one:

https://lore.kernel.org/all/CAHk-=wgLYqYcw0xv65xrLSR7KDpS_6M+S9737m6NQorHGWsXYQ@mail.gmail.com/

 - quote -

The actual C99 version is the sane one which actually makes it easier
and clearer to have loop iterators that are clearly just in loop
scope.

That's a good idea in general, and I have wanted to start using that
in the kernel even aside from some of the loop construct macros.
Because putting variables in natural minimal scope is a GoodThing(tm).

 - end quote -

If you look around the kernel, you will see many for loops that are in
this style now.


ChenYu

Re: [PATCH v2 2/4] pwm: sun50i: Add H616 PWM support

[Richard GENOUD]

Hi Uwe,
>>>> +	for (unsigned int i = 0; i < data->npwm; i++) {
>>>
>>> Huh, AFAIK we're not using declarations in for loops in the kernel.
>>
>> Actually, I've read somewhere (in LWN I guess) that Linus seems ok with that,
>> but I'll remove it if you prefer.
> 
> If you find your source again, I'd be interested.
I took me some time, but I finally found it (thanks to Hervé):
https://lore.kernel.org/all/CAHk-=wiCOTW5UftUrAnvJkr6769D29tF7Of79gUjdQHS_TkF5A@mail.gmail.com/


> 
> Best regards
> Uwe

Regards,
Richard

[PATCH v2 3/4] arm64: dts: allwinner: h616: add PWM controller

[Richard Genoud]

The H616 has a PWM controller that can provide PWM signals, but also
plain clocks.

Add the PWM controller node and pins in the device tree.

Tested-by: Joao Schim <joao@schimsalabim.eu>
Signed-off-by: Richard Genoud <richard.genoud@bootlin.com>
---
 .../arm64/boot/dts/allwinner/sun50i-h616.dtsi | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
index 8d1110c14bad..1c7628a6e4bb 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
@@ -236,6 +236,17 @@ watchdog: watchdog@30090a0 {
 			clocks = <&osc24M>;
 		};
 
+		pwm: pwm@300a000 {
+			compatible = "allwinner,sun50i-h616-pwm";
+			reg = <0x0300a000 0x400>;
+			clocks = <&osc24M>, <&ccu CLK_BUS_PWM>;
+			clock-names = "mod", "bus";
+			resets = <&ccu RST_BUS_PWM>;
+			#pwm-cells = <3>;
+			#clock-cells = <1>;
+			status = "disabled";
+		};
+
 		pio: pinctrl@300b000 {
 			compatible = "allwinner,sun50i-h616-pinctrl";
 			reg = <0x0300b000 0x400>;
@@ -340,6 +351,42 @@ nand_rb1_pin: nand-rb1-pin {
 				bias-pull-up;
 			};
 
+			/omit-if-no-ref/
+			pwm0_pin: pwm0-pin {
+				pins = "PD28";
+				function = "pwm0";
+			};
+
+			/omit-if-no-ref/
+			pwm1_pin: pwm1-pin {
+				pins = "PG19";
+				function = "pwm1";
+			};
+
+			/omit-if-no-ref/
+			pwm2_pin: pwm2-pin {
+				pins = "PH2";
+				function = "pwm2";
+			};
+
+			/omit-if-no-ref/
+			pwm3_pin: pwm3-pin {
+				pins = "PH0";
+				function = "pwm3";
+			};
+
+			/omit-if-no-ref/
+			pwm4_pin: pwm4-pin {
+				pins = "PI14";
+				function = "pwm4";
+			};
+
+			/omit-if-no-ref/
+			pwm5_pin: pwm5-pin {
+				pins = "PA12";
+				function = "pwm5";
+			};
+
 			/omit-if-no-ref/
 			spi0_pins: spi0-pins {
 				pins = "PC0", "PC2", "PC4";
-- 
2.47.3

[PATCH v2 4/4] MAINTAINERS: Add entry on Allwinner H616 PWM driver

[Richard Genoud]

Add myself as maintainer of Allwinner H616 PWM driver and device-tree
bindings.

Signed-off-by: Richard Genoud <richard.genoud@bootlin.com>
---
 MAINTAINERS | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5b11839cba9d..df2de03e6e38 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -903,6 +903,11 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/sound/allwinner,sun50i-h6-dmic.yaml
 F:	sound/soc/sunxi/sun50i-dmic.c
 
+ALLWINNER H616 PWM DRIVER
+M:	Richard Genoud <richard.genoud@bootlin.com>
+S:	Maintained
+F:	drivers/pwm/pwm-sun50i-h616.c
+
 ALLWINNER HARDWARE SPINLOCK SUPPORT
 M:	Wilken Gottwalt <wilken.gottwalt@posteo.net>
 S:	Maintained
-- 
2.47.3

Re: [PATCH v2 0/4] Introduce Allwinner H616 PWM controller

[Jernej Škrabec]

Dne sreda, 17. december 2025 ob 09:25:00 Srednjeevropski standardni čas je Richard Genoud napisal(a):
> Allwinner H616 PWM controller is quite different from the A10 one.
> 
> It can drive 6 PWM channels, and like for the A10, each channel has a
> bypass that permits to output a clock, bypassing the PWM logic, when
> enabled.
> 
> But, the channels are paired 2 by 2, sharing a first set of
> MUX/prescaler/gate.
> Then, for each channel, there's another prescaler (that will be bypassed
> if the bypass is enabled for this channel).
> 
> It looks like that:
>             _____      ______      ________
> OSC24M --->|     |    |      |    |        |
> APB1 ----->| Mux |--->| Gate |--->| /div_m |-----> PWM_clock_src_xy
>            |_____|    |______|    |________|
>                           ________
>                          |        |
>                       +->| /div_k |---> PWM_clock_x
>                       |  |________|
>                       |    ______
>                       |   |      |
>                       +-->| Gate |----> PWM_bypass_clock_x
>                       |   |______|
> PWM_clock_src_xy -----+   ________
>                       |  |        |
>                       +->| /div_k |---> PWM_clock_y
>                       |  |________|
>                       |    ______
>                       |   |      |
>                       +-->| Gate |----> PWM_bypass_clock_y
>                           |______|
> 
> Where xy can be 0/1, 2/3, 4/5
> 
> PWM_clock_x/y serve for the PWM purpose.
> PWM_bypass_clock_x/y serve for the clock-provider purpose.
> The common clock framework has been used to manage those clocks.
> 
> This PWM driver serves as a clock-provider for PWM_bypass_clocks.
> This is needed for example by the embedded AC300 PHY which clock comes
> from PMW5 pin (PB12).

No. Drop all clocks related code and make this pure PWM driver, like pwm-sun4i
is. For AC300, AC200 or whatever other device may need clock produced by PWM,
pwm-clock can be used like this:

ac300_pwm_clk: ac300-clk {
	compatible = "pwm-clock";
	#clock-cells = <0>;
	clock-frequency = <24000000>;
	pinctrl-names = "default";
	pinctrl-0 = <&pwm1_pin>;
	pwms = <&pwm 1 42 0>;
};

ac300 {
	...
	clocks = <&ac300_pwm_clk>;
	...
};

Best regards,
Jernej

> 
> This series is based onto v6.19-rc1
> 
> Changes since v1:
> - rebase onto v6.19-rc1
> - add missing headers
> - remove MODULE_ALIAS (suggested by Krzysztof)
> - use sun4i-pwm binding instead of creating a new one (suggested by Krzysztof)
> - retrieve the parent clocks from the devicetree
> - switch num_parents to unsigned int
> 
> Richard Genoud (4):
>   dt-bindings: pwm: allwinner: add h616 pwm compatible
>   pwm: sun50i: Add H616 PWM support
>   arm64: dts: allwinner: h616: add PWM controller
>   MAINTAINERS: Add entry on Allwinner H616 PWM driver
> 
>  .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml |  19 +-
>  MAINTAINERS                                   |   5 +
>  .../arm64/boot/dts/allwinner/sun50i-h616.dtsi |  47 +
>  drivers/pwm/Kconfig                           |  12 +
>  drivers/pwm/Makefile                          |   1 +
>  drivers/pwm/pwm-sun50i-h616.c                 | 892 ++++++++++++++++++
>  6 files changed, 975 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pwm/pwm-sun50i-h616.c
> 
> 
> base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
> 

Re: [PATCH v2 0/4] Introduce Allwinner H616 PWM controller

[Richard GENOUD]

Hi Jernej,

Le 21/12/2025 à 20:12, Jernej Škrabec a écrit :
> Dne sreda, 17. december 2025 ob 09:25:00 Srednjeevropski standardni čas je Richard Genoud napisal(a):
>> Allwinner H616 PWM controller is quite different from the A10 one.
>>
>> It can drive 6 PWM channels, and like for the A10, each channel has a
>> bypass that permits to output a clock, bypassing the PWM logic, when
>> enabled.
>>
>> But, the channels are paired 2 by 2, sharing a first set of
>> MUX/prescaler/gate.
>> Then, for each channel, there's another prescaler (that will be bypassed
>> if the bypass is enabled for this channel).
>>
>> It looks like that:
>>              _____      ______      ________
>> OSC24M --->|     |    |      |    |        |
>> APB1 ----->| Mux |--->| Gate |--->| /div_m |-----> PWM_clock_src_xy
>>             |_____|    |______|    |________|
>>                            ________
>>                           |        |
>>                        +->| /div_k |---> PWM_clock_x
>>                        |  |________|
>>                        |    ______
>>                        |   |      |
>>                        +-->| Gate |----> PWM_bypass_clock_x
>>                        |   |______|
>> PWM_clock_src_xy -----+   ________
>>                        |  |        |
>>                        +->| /div_k |---> PWM_clock_y
>>                        |  |________|
>>                        |    ______
>>                        |   |      |
>>                        +-->| Gate |----> PWM_bypass_clock_y
>>                            |______|
>>
>> Where xy can be 0/1, 2/3, 4/5
>>
>> PWM_clock_x/y serve for the PWM purpose.
>> PWM_bypass_clock_x/y serve for the clock-provider purpose.
>> The common clock framework has been used to manage those clocks.
>>
>> This PWM driver serves as a clock-provider for PWM_bypass_clocks.
>> This is needed for example by the embedded AC300 PHY which clock comes
>> from PMW5 pin (PB12).
> 
> No. Drop all clocks related code and make this pure PWM driver, like pwm-sun4i
> is. For AC300, AC200 or whatever other device may need clock produced by PWM,
> pwm-clock can be used like this:
> 
> ac300_pwm_clk: ac300-clk {
> 	compatible = "pwm-clock";
> 	#clock-cells = <0>;
> 	clock-frequency = <24000000>;
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&pwm1_pin>;
> 	pwms = <&pwm 1 42 0>;
> };
> 
> ac300 {
> 	...
> 	clocks = <&ac300_pwm_clk>;
> 	...
> };
Yes, that was my first move, but after trying quite hard, I came to the 
conclusion that pwm-clock is not fit for this precise case.
If we had only one source clock, this would be a perfect fit though.

Here's the problem: the pwm-clock request a period from the PWM driver, 
without any clue that it actually wants a clock at a specific frequency, 
and not a PWM signal with duty cycle capability.
So, the PWM driver doesn't know if it can use the bypass or not, it 
doesn't even have the real accurate frequency information (23809524 Hz 
instead of 24MHz) because the pwm_apply only deals with periods.
With pwm-clock, we loose a precious information along the way (that we 
actually want a clock and not a PWM signal).
That's ok with simple PWM drivers that don't have multiple input clocks, 
but in this case, without this information, we can't know for sure which 
clock to use.
And here, for instance, if pwm-clock requests 42ns, the logic is to 
select the highest clock (100MHz), with no prescaler, and a duty cycle 
value of 2/4 => we have 25MHz instead of 24MHz.
And that's a perfectly fine choice for a PMW, because we still can 
change the duty cycle in the range [0-4]/4.
But obviously for a clock, we don't care about the duty cycle, but more 
about the clock accuracy.
When I tried to use pwm-clock, I ended up making assumptions on what was 
the intended use (if the asked period is 42 with a duty cycle of 50% 
maybe the consumer wants a clock), but that was too hack-ish.

That's why I put aside the pwm-clock and I went for this instead.


Thanks for the review!

Regards,
Richard

> 
> Best regards,
> Jernej
> 
>>
>> This series is based onto v6.19-rc1
>>
>> Changes since v1:
>> - rebase onto v6.19-rc1
>> - add missing headers
>> - remove MODULE_ALIAS (suggested by Krzysztof)
>> - use sun4i-pwm binding instead of creating a new one (suggested by Krzysztof)
>> - retrieve the parent clocks from the devicetree
>> - switch num_parents to unsigned int
>>
>> Richard Genoud (4):
>>    dt-bindings: pwm: allwinner: add h616 pwm compatible
>>    pwm: sun50i: Add H616 PWM support
>>    arm64: dts: allwinner: h616: add PWM controller
>>    MAINTAINERS: Add entry on Allwinner H616 PWM driver
>>
>>   .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml |  19 +-
>>   MAINTAINERS                                   |   5 +
>>   .../arm64/boot/dts/allwinner/sun50i-h616.dtsi |  47 +
>>   drivers/pwm/Kconfig                           |  12 +
>>   drivers/pwm/Makefile                          |   1 +
>>   drivers/pwm/pwm-sun50i-h616.c                 | 892 ++++++++++++++++++
>>   6 files changed, 975 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/pwm/pwm-sun50i-h616.c
>>
>>
>> base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
>>
> 
> 
> 
> 


-- 
Richard Genoud, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 0/4] Introduce Allwinner H616 PWM controller

[Uwe Kleine-König]

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

Hello Richard,

On Mon, Dec 22, 2025 at 10:17:07AM +0100, Richard GENOUD wrote:
> Le 21/12/2025 à 20:12, Jernej Škrabec a écrit :
> [...]
> 
> That's why I put aside the pwm-clock and I went for this instead.

I havn't tried to understand the issue in detail, but would it help to
use assigned-clocks to make pwm-clk suitable? Assuming this allows to
drop all the clk stuff from the driver that would we very appreciated.

Best regards
Uwe

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

Re: [PATCH v2 0/4] Introduce Allwinner H616 PWM controller

[Paul Kocialkowski]

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

Hello Richard,

On Wed 17 Dec 25, 09:25, Richard Genoud wrote:
> Allwinner H616 PWM controller is quite different from the A10 one.

Thanks for your patches!

For context here, the PWM controller in the H616 is an instance of the second
generation Allwinner PWM controller, which was first seen in the V5 chip from
2018.

It is also present in the following SoCs: V5, A50, H616, V536, T7, A133, V833,
R329, D1/T113, R128, V851, A523 and A733.

You probably missed it, but there is already an ongonig series to add support
for that second generation PWM controller from Aleksandr Shubin:
https://patchwork.ozlabs.org/project/linux-pwm/list/?series=454353&archive=both&state=*

And a patch was also proposed to add H616 support:
https://patchwork.ozlabs.org/project/linux-pwm/list/?series=409036&archive=both&state=*

So you should probably try these series and coordinate with their authors
instead of adding this new driver. I understand it's unfortunate that the work
was already done on your side.

All the best,

Paul

> It can drive 6 PWM channels, and like for the A10, each channel has a
> bypass that permits to output a clock, bypassing the PWM logic, when
> enabled.
> 
> But, the channels are paired 2 by 2, sharing a first set of
> MUX/prescaler/gate.
> Then, for each channel, there's another prescaler (that will be bypassed
> if the bypass is enabled for this channel).
> 
> It looks like that:
>             _____      ______      ________
> OSC24M --->|     |    |      |    |        |
> APB1 ----->| Mux |--->| Gate |--->| /div_m |-----> PWM_clock_src_xy
>            |_____|    |______|    |________|
>                           ________
>                          |        |
>                       +->| /div_k |---> PWM_clock_x
>                       |  |________|
>                       |    ______
>                       |   |      |
>                       +-->| Gate |----> PWM_bypass_clock_x
>                       |   |______|
> PWM_clock_src_xy -----+   ________
>                       |  |        |
>                       +->| /div_k |---> PWM_clock_y
>                       |  |________|
>                       |    ______
>                       |   |      |
>                       +-->| Gate |----> PWM_bypass_clock_y
>                           |______|
> 
> Where xy can be 0/1, 2/3, 4/5
> 
> PWM_clock_x/y serve for the PWM purpose.
> PWM_bypass_clock_x/y serve for the clock-provider purpose.
> The common clock framework has been used to manage those clocks.
> 
> This PWM driver serves as a clock-provider for PWM_bypass_clocks.
> This is needed for example by the embedded AC300 PHY which clock comes
> from PMW5 pin (PB12).
> 
> This series is based onto v6.19-rc1
> 
> Changes since v1:
> - rebase onto v6.19-rc1
> - add missing headers
> - remove MODULE_ALIAS (suggested by Krzysztof)
> - use sun4i-pwm binding instead of creating a new one (suggested by Krzysztof)
> - retrieve the parent clocks from the devicetree
> - switch num_parents to unsigned int
> 
> Richard Genoud (4):
>   dt-bindings: pwm: allwinner: add h616 pwm compatible
>   pwm: sun50i: Add H616 PWM support
>   arm64: dts: allwinner: h616: add PWM controller
>   MAINTAINERS: Add entry on Allwinner H616 PWM driver
> 
>  .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml |  19 +-
>  MAINTAINERS                                   |   5 +
>  .../arm64/boot/dts/allwinner/sun50i-h616.dtsi |  47 +
>  drivers/pwm/Kconfig                           |  12 +
>  drivers/pwm/Makefile                          |   1 +
>  drivers/pwm/pwm-sun50i-h616.c                 | 892 ++++++++++++++++++
>  6 files changed, 975 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pwm/pwm-sun50i-h616.c
> 
> 
> base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
> -- 
> 2.47.3
> 
> 

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

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