[PATCH v6 0/3] pwm: Add pwm driver for Sophgo SG2042

[PATCH v6 0/3] pwm: Add pwm driver for Sophgo SG2042

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

Add driver for pwm controller of Sophgo SG2042 SoC.

Thanks,
Chen

---

Changes in v6:
  Nothing major changes just rebased onto v6.13-rc1 and retested.

Changes in v5:
  The patch series is based on v6.12-rc1. You can simply review or test
  the patches at the link [5].

  Updated driver to add resets property for pwm controller node as per
  suggestion from Inochi.

Changes in v4:
  The patch series is based on v6.12-rc1. You can simply review or test
  the patches at the link [4].

  Updated driver to set property atomic of pwm_chip to true as per suggestion
  from Sean.

Changes in v3:
  The patch series is catched up with v6.12-rc1. You can simply review or test
  the patches at the link [3].

  Add patch #3 for dts part change.

Changes in v2:
  The patch series is based on v6.11-rc6. You can simply review or test the
  patches at the link [2].

  Fixed following issues as per comments from Yixun Lan, Krzysztof Kozlowski
  and Uwe Kleine-König, thanks.

  - Some minor issues in dt-bindings.
  - driver issues, use macros with name prefix for registers access; add
    limitations comments; fixed potential calculation overflow problem;
    add .get_state() callback and other miscellaneous code improvements.

Changes in v1:
  The patch series is based on v6.11-rc6. You can simply review or test the
  patches at the link [1].

Link: https://lore.kernel.org/linux-riscv/cover.1725536870.git.unicorn_wang@outlook.com/ [1]
Link: https://lore.kernel.org/linux-riscv/cover.1725931796.git.unicorn_wang@outlook.com/ [2]
Link: https://lore.kernel.org/linux-riscv/cover.1728355974.git.unicorn_wang@outlook.com/ [3]
Link: https://lore.kernel.org/linux-riscv/cover.1729037302.git.unicorn_wang@outlook.com/ [4]
Link: https://lore.kernel.org/linux-riscv/cover.1729843087.git.unicorn_wang@outlook.com/ [5]
---

Chen Wang (3):
  dt-bindings: pwm: sophgo: add PWM controller for SG2042
  pwm: sophgo: add driver for Sophgo SG2042 PWM
  riscv: sophgo: dts: add pwm controller for SG2042 SoC

 .../bindings/pwm/sophgo,sg2042-pwm.yaml       |  58 ++++++
 arch/riscv/boot/dts/sophgo/sg2042.dtsi        |   9 +
 drivers/pwm/Kconfig                           |  10 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-sophgo-sg2042.c               | 194 ++++++++++++++++++
 5 files changed, 272 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml
 create mode 100644 drivers/pwm/pwm-sophgo-sg2042.c


base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
-- 
2.34.1

[PATCH v6 1/3] dt-bindings: pwm: sophgo: add PWM controller for SG2042

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

Sophgo SG2042 contains a PWM controller, which has 4 channels and
can generate PWM waveforms output.

Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/pwm/sophgo,sg2042-pwm.yaml       | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml

diff --git a/Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml b/Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml
new file mode 100644
index 000000000000..5dea41fa4c44
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/sophgo,sg2042-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sophgo SG2042 PWM controller
+
+maintainers:
+  - Chen Wang <unicorn_wang@outlook.com>
+
+description:
+  This controller contains 4 channels which can generate PWM waveforms.
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    const: sophgo,sg2042-pwm
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: apb
+
+  resets:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - resets
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/reset/sophgo,sg2042-reset.h>
+
+    pwm@7f006000 {
+        compatible = "sophgo,sg2042-pwm";
+        reg = <0x7f006000 0x1000>;
+        #pwm-cells = <2>;
+        clocks = <&clock 67>;
+        clock-names = "apb";
+        resets = <&rstgen RST_PWM>;
+    };
-- 
2.34.1

[PATCH v6 2/3] pwm: sophgo: add driver for Sophgo SG2042 PWM

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

Add a PWM driver for PWM controller in Sophgo SG2042 SoC.

Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/pwm/Kconfig             |  10 ++
 drivers/pwm/Makefile            |   1 +
 drivers/pwm/pwm-sophgo-sg2042.c | 194 ++++++++++++++++++++++++++++++++
 3 files changed, 205 insertions(+)
 create mode 100644 drivers/pwm/pwm-sophgo-sg2042.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 0915c1e7df16..ec85f3895936 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -584,6 +584,16 @@ config PWM_SL28CPLD
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-sl28cpld.
 
+config PWM_SOPHGO_SG2042
+	tristate "Sophgo SG2042 PWM support"
+	depends on ARCH_SOPHGO || COMPILE_TEST
+	help
+	  PWM driver for the PWM controller on Sophgo SG2042 SoC. The PWM
+	  controller supports outputing 4 channels of PWM waveforms.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm_sophgo_sg2042.
+
 config PWM_SPEAR
 	tristate "STMicroelectronics SPEAr PWM support"
 	depends on PLAT_SPEAR || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9081e0c0e9e0..539e0def3f82 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_PWM_RZ_MTU3)	+= pwm-rz-mtu3.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
 obj-$(CONFIG_PWM_SL28CPLD)	+= pwm-sl28cpld.o
+obj-$(CONFIG_PWM_SOPHGO_SG2042)	+= pwm-sophgo-sg2042.o
 obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
 obj-$(CONFIG_PWM_SPRD)		+= pwm-sprd.o
 obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
diff --git a/drivers/pwm/pwm-sophgo-sg2042.c b/drivers/pwm/pwm-sophgo-sg2042.c
new file mode 100644
index 000000000000..a3d12505e4aa
--- /dev/null
+++ b/drivers/pwm/pwm-sophgo-sg2042.c
@@ -0,0 +1,194 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Sophgo SG2042 PWM Controller Driver
+ *
+ * Copyright (C) 2024 Sophgo Technology Inc.
+ * Copyright (C) 2024 Chen Wang <unicorn_wang@outlook.com>
+ *
+ * Limitations:
+ * - After reset, the output of the PWM channel is always high.
+ *   The value of HLPERIOD/PERIOD is 0.
+ * - When HLPERIOD or PERIOD is reconfigured, PWM will start to
+ *   output waveforms with the new configuration after completing
+ *   the running period.
+ * - When PERIOD and HLPERIOD is set to 0, the PWM wave output will
+ *   be stopped and the output is pulled to high.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/reset.h>
+
+#include <asm/div64.h>
+
+/*
+ * Offset RegisterName
+ * 0x0000 HLPERIOD0
+ * 0x0004 PERIOD0
+ * 0x0008 HLPERIOD1
+ * 0x000C PERIOD1
+ * 0x0010 HLPERIOD2
+ * 0x0014 PERIOD2
+ * 0x0018 HLPERIOD3
+ * 0x001C PERIOD3
+ * Four groups and every group is composed of HLPERIOD & PERIOD
+ */
+#define SG2042_HLPERIOD(chan) ((chan) * 8 + 0)
+#define SG2042_PERIOD(chan) ((chan) * 8 + 4)
+
+#define SG2042_PWM_CHANNELNUM	4
+
+/**
+ * struct sg2042_pwm_ddata - private driver data
+ * @base:		base address of mapped PWM registers
+ * @clk_rate_hz:	rate of base clock in HZ
+ */
+struct sg2042_pwm_ddata {
+	void __iomem *base;
+	unsigned long clk_rate_hz;
+};
+
+static void pwm_sg2042_config(void __iomem *base, unsigned int chan, u32 period, u32 hlperiod)
+{
+	writel(period, base + SG2042_PERIOD(chan));
+	writel(hlperiod, base + SG2042_HLPERIOD(chan));
+}
+
+static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			    const struct pwm_state *state)
+{
+	struct sg2042_pwm_ddata *ddata = pwmchip_get_drvdata(chip);
+	u32 hlperiod;
+	u32 period;
+
+	if (state->polarity == PWM_POLARITY_INVERSED)
+		return -EINVAL;
+
+	if (!state->enabled) {
+		pwm_sg2042_config(ddata->base, pwm->hwpwm, 0, 0);
+		return 0;
+	}
+
+	/*
+	 * Period of High level (duty_cycle) = HLPERIOD x Period_clk
+	 * Period of One Cycle (period) = PERIOD x Period_clk
+	 */
+	period = min(mul_u64_u64_div_u64(ddata->clk_rate_hz, state->period, NSEC_PER_SEC), U32_MAX);
+	hlperiod = min(mul_u64_u64_div_u64(ddata->clk_rate_hz, state->duty_cycle, NSEC_PER_SEC), U32_MAX);
+
+	if (hlperiod > period) {
+		dev_err(pwmchip_parent(chip), "period < hlperiod, failed to apply current setting\n");
+		return -EINVAL;
+	}
+
+	dev_dbg(pwmchip_parent(chip), "chan[%u]: period=%u, hlperiod=%u\n",
+		pwm->hwpwm, period, hlperiod);
+
+	pwm_sg2042_config(ddata->base, pwm->hwpwm, period, hlperiod);
+
+	return 0;
+}
+
+static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				struct pwm_state *state)
+{
+	struct sg2042_pwm_ddata *ddata = pwmchip_get_drvdata(chip);
+	unsigned int chan = pwm->hwpwm;
+	u32 hlperiod;
+	u32 period;
+
+	period = readl(ddata->base + SG2042_PERIOD(chan));
+	hlperiod = readl(ddata->base + SG2042_HLPERIOD(chan));
+
+	if (!period && !hlperiod)
+		state->enabled = false;
+	else
+		state->enabled = true;
+
+	state->period = DIV_ROUND_UP_ULL((u64)period * NSEC_PER_SEC, ddata->clk_rate_hz);
+	state->duty_cycle = DIV_ROUND_UP_ULL((u64)hlperiod * NSEC_PER_SEC, ddata->clk_rate_hz);
+
+	state->polarity = PWM_POLARITY_NORMAL;
+
+	return 0;
+}
+
+static const struct pwm_ops pwm_sg2042_ops = {
+	.apply = pwm_sg2042_apply,
+	.get_state = pwm_sg2042_get_state,
+};
+
+static const struct of_device_id sg2042_pwm_ids[] = {
+	{ .compatible = "sophgo,sg2042-pwm" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sg2042_pwm_ids);
+
+static int pwm_sg2042_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sg2042_pwm_ddata *ddata;
+	struct reset_control *rst;
+	struct pwm_chip *chip;
+	struct clk *clk;
+	int ret;
+
+	chip = devm_pwmchip_alloc(dev, SG2042_PWM_CHANNELNUM, sizeof(*ddata));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+	ddata = pwmchip_get_drvdata(chip);
+
+	ddata->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ddata->base))
+		return PTR_ERR(ddata->base);
+
+	clk = devm_clk_get_enabled(dev, "apb");
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk), "failed to get base clk\n");
+
+	ret = devm_clk_rate_exclusive_get(dev, clk);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to get exclusive rate\n");
+
+	ddata->clk_rate_hz = clk_get_rate(clk);
+	if (!ddata->clk_rate_hz || ddata->clk_rate_hz > NSEC_PER_SEC)
+		return dev_err_probe(dev, -EINVAL,
+				     "Invalid clock rate: %lu\n", ddata->clk_rate_hz);
+
+	rst = devm_reset_control_get_optional_shared(dev, NULL);
+	if (IS_ERR(rst))
+		return dev_err_probe(dev, PTR_ERR(rst), "failed to get reset\n");
+
+	/* Deassert reset */
+	ret = reset_control_deassert(rst);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to deassert\n");
+
+	chip->ops = &pwm_sg2042_ops;
+	chip->atomic = true;
+
+	ret = devm_pwmchip_add(dev, chip);
+	if (ret < 0) {
+		reset_control_assert(rst);
+		return dev_err_probe(dev, ret, "failed to register PWM chip\n");
+	}
+
+	return 0;
+}
+
+static struct platform_driver pwm_sg2042_driver = {
+	.driver	= {
+		.name = "sg2042-pwm",
+		.of_match_table = sg2042_pwm_ids,
+	},
+	.probe = pwm_sg2042_probe,
+};
+module_platform_driver(pwm_sg2042_driver);
+
+MODULE_AUTHOR("Chen Wang");
+MODULE_DESCRIPTION("Sophgo SG2042 PWM driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

[PATCH v6 3/3] riscv: sophgo: dts: add pwm controller for SG2042 SoC

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

SG2042 has one PWM controller, which has 4 pwm output channels.

Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
---
 arch/riscv/boot/dts/sophgo/sg2042.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/riscv/boot/dts/sophgo/sg2042.dtsi b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
index e62ac51ac55a..77dd65d79946 100644
--- a/arch/riscv/boot/dts/sophgo/sg2042.dtsi
+++ b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
@@ -165,6 +165,15 @@ port2a: gpio-controller@0 {
 			};
 		};
 
+		pwm: pwm@703000c000 {
+			compatible = "sophgo,sg2042-pwm";
+			reg = <0x70 0x3000c000 0x0 0x20>;
+			#pwm-cells = <2>;
+			clocks = <&clkgen GATE_CLK_APB_PWM>;
+			clock-names = "apb";
+			resets = <&rstgen RST_PWM>;
+		};
+
 		pllclk: clock-controller@70300100c0 {
 			compatible = "sophgo,sg2042-pll";
 			reg = <0x70 0x300100c0 0x0 0x40>;
-- 
2.34.1

Re: [PATCH v6 0/3] pwm: Add pwm driver for Sophgo SG2042

[Chen Wang]

Hi, Uwe,

I just upgraded this patchset and rebased it to the latest v6.13-rc1, 
hope you can merge it in v6.14.

BTW, you just please handle the bindings & driver part, I will handle 
the left DTS part.

Thanks,

Chen

On 2024/12/4 11:15, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
>
> Add driver for pwm controller of Sophgo SG2042 SoC.
>
> Thanks,
> Chen
>
> ---
>
> Changes in v6:
>    Nothing major changes just rebased onto v6.13-rc1 and retested.
>
> Changes in v5:
>    The patch series is based on v6.12-rc1. You can simply review or test
>    the patches at the link [5].
>
>    Updated driver to add resets property for pwm controller node as per
>    suggestion from Inochi.
>
> Changes in v4:
>    The patch series is based on v6.12-rc1. You can simply review or test
>    the patches at the link [4].
>
>    Updated driver to set property atomic of pwm_chip to true as per suggestion
>    from Sean.
>
> Changes in v3:
>    The patch series is catched up with v6.12-rc1. You can simply review or test
>    the patches at the link [3].
>
>    Add patch #3 for dts part change.
>
> Changes in v2:
>    The patch series is based on v6.11-rc6. You can simply review or test the
>    patches at the link [2].
>
>    Fixed following issues as per comments from Yixun Lan, Krzysztof Kozlowski
>    and Uwe Kleine-König, thanks.
>
>    - Some minor issues in dt-bindings.
>    - driver issues, use macros with name prefix for registers access; add
>      limitations comments; fixed potential calculation overflow problem;
>      add .get_state() callback and other miscellaneous code improvements.
>
> Changes in v1:
>    The patch series is based on v6.11-rc6. You can simply review or test the
>    patches at the link [1].
>
> Link: https://lore.kernel.org/linux-riscv/cover.1725536870.git.unicorn_wang@outlook.com/ [1]
> Link: https://lore.kernel.org/linux-riscv/cover.1725931796.git.unicorn_wang@outlook.com/ [2]
> Link: https://lore.kernel.org/linux-riscv/cover.1728355974.git.unicorn_wang@outlook.com/ [3]
> Link: https://lore.kernel.org/linux-riscv/cover.1729037302.git.unicorn_wang@outlook.com/ [4]
> Link: https://lore.kernel.org/linux-riscv/cover.1729843087.git.unicorn_wang@outlook.com/ [5]
> ---
>
> Chen Wang (3):
>    dt-bindings: pwm: sophgo: add PWM controller for SG2042
>    pwm: sophgo: add driver for Sophgo SG2042 PWM
>    riscv: sophgo: dts: add pwm controller for SG2042 SoC
>
>   .../bindings/pwm/sophgo,sg2042-pwm.yaml       |  58 ++++++
>   arch/riscv/boot/dts/sophgo/sg2042.dtsi        |   9 +
>   drivers/pwm/Kconfig                           |  10 +
>   drivers/pwm/Makefile                          |   1 +
>   drivers/pwm/pwm-sophgo-sg2042.c               | 194 ++++++++++++++++++
>   5 files changed, 272 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml
>   create mode 100644 drivers/pwm/pwm-sophgo-sg2042.c
>
>
> base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37

Re: [PATCH v6 0/3] pwm: Add pwm driver for Sophgo SG2042

[Chen Wang]

Hello & Happy new year, Uwe,

Just ping and want to know if this will be picked by pwm/for-next for 
targeting 6.14?

Thanks,

Chen

On 2024/12/4 11:15, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
>
> Add driver for pwm controller of Sophgo SG2042 SoC.
>
> Thanks,
> Chen
>
> ---
>
> Changes in v6:
>    Nothing major changes just rebased onto v6.13-rc1 and retested.
>
> Changes in v5:
>    The patch series is based on v6.12-rc1. You can simply review or test
>    the patches at the link [5].
>
>    Updated driver to add resets property for pwm controller node as per
>    suggestion from Inochi.
>
> Changes in v4:
>    The patch series is based on v6.12-rc1. You can simply review or test
>    the patches at the link [4].
>
>    Updated driver to set property atomic of pwm_chip to true as per suggestion
>    from Sean.
>
> Changes in v3:
>    The patch series is catched up with v6.12-rc1. You can simply review or test
>    the patches at the link [3].
>
>    Add patch #3 for dts part change.
>
> Changes in v2:
>    The patch series is based on v6.11-rc6. You can simply review or test the
>    patches at the link [2].
>
>    Fixed following issues as per comments from Yixun Lan, Krzysztof Kozlowski
>    and Uwe Kleine-König, thanks.
>
>    - Some minor issues in dt-bindings.
>    - driver issues, use macros with name prefix for registers access; add
>      limitations comments; fixed potential calculation overflow problem;
>      add .get_state() callback and other miscellaneous code improvements.
>
> Changes in v1:
>    The patch series is based on v6.11-rc6. You can simply review or test the
>    patches at the link [1].
>
> Link: https://lore.kernel.org/linux-riscv/cover.1725536870.git.unicorn_wang@outlook.com/ [1]
> Link: https://lore.kernel.org/linux-riscv/cover.1725931796.git.unicorn_wang@outlook.com/ [2]
> Link: https://lore.kernel.org/linux-riscv/cover.1728355974.git.unicorn_wang@outlook.com/ [3]
> Link: https://lore.kernel.org/linux-riscv/cover.1729037302.git.unicorn_wang@outlook.com/ [4]
> Link: https://lore.kernel.org/linux-riscv/cover.1729843087.git.unicorn_wang@outlook.com/ [5]
> ---
>
> Chen Wang (3):
>    dt-bindings: pwm: sophgo: add PWM controller for SG2042
>    pwm: sophgo: add driver for Sophgo SG2042 PWM
>    riscv: sophgo: dts: add pwm controller for SG2042 SoC
>
>   .../bindings/pwm/sophgo,sg2042-pwm.yaml       |  58 ++++++
>   arch/riscv/boot/dts/sophgo/sg2042.dtsi        |   9 +
>   drivers/pwm/Kconfig                           |  10 +
>   drivers/pwm/Makefile                          |   1 +
>   drivers/pwm/pwm-sophgo-sg2042.c               | 194 ++++++++++++++++++
>   5 files changed, 272 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml
>   create mode 100644 drivers/pwm/pwm-sophgo-sg2042.c
>
>
> base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37

Re: [PATCH v6 2/3] pwm: sophgo: add driver for Sophgo SG2042 PWM

[Uwe Kleine-König]

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

On Wed, Dec 04, 2024 at 11:17:18AM +0800, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
> 
> Add a PWM driver for PWM controller in Sophgo SG2042 SoC.
> 
> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/pwm/Kconfig             |  10 ++
>  drivers/pwm/Makefile            |   1 +
>  drivers/pwm/pwm-sophgo-sg2042.c | 194 ++++++++++++++++++++++++++++++++
>  3 files changed, 205 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sophgo-sg2042.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 0915c1e7df16..ec85f3895936 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -584,6 +584,16 @@ config PWM_SL28CPLD
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-sl28cpld.
>  
> +config PWM_SOPHGO_SG2042
> +	tristate "Sophgo SG2042 PWM support"
> +	depends on ARCH_SOPHGO || COMPILE_TEST
> +	help
> +	  PWM driver for the PWM controller on Sophgo SG2042 SoC. The PWM
> +	  controller supports outputing 4 channels of PWM waveforms.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm_sophgo_sg2042.
> +
>  config PWM_SPEAR
>  	tristate "STMicroelectronics SPEAr PWM support"
>  	depends on PLAT_SPEAR || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9081e0c0e9e0..539e0def3f82 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_PWM_RZ_MTU3)	+= pwm-rz-mtu3.o
>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
>  obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
>  obj-$(CONFIG_PWM_SL28CPLD)	+= pwm-sl28cpld.o
> +obj-$(CONFIG_PWM_SOPHGO_SG2042)	+= pwm-sophgo-sg2042.o
>  obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
>  obj-$(CONFIG_PWM_SPRD)		+= pwm-sprd.o
>  obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
> diff --git a/drivers/pwm/pwm-sophgo-sg2042.c b/drivers/pwm/pwm-sophgo-sg2042.c
> new file mode 100644
> index 000000000000..a3d12505e4aa
> --- /dev/null
> +++ b/drivers/pwm/pwm-sophgo-sg2042.c
> @@ -0,0 +1,194 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Sophgo SG2042 PWM Controller Driver
> + *
> + * Copyright (C) 2024 Sophgo Technology Inc.
> + * Copyright (C) 2024 Chen Wang <unicorn_wang@outlook.com>
> + *
> + * Limitations:
> + * - After reset, the output of the PWM channel is always high.
> + *   The value of HLPERIOD/PERIOD is 0.
> + * - When HLPERIOD or PERIOD is reconfigured, PWM will start to
> + *   output waveforms with the new configuration after completing
> + *   the running period.
> + * - When PERIOD and HLPERIOD is set to 0, the PWM wave output will
> + *   be stopped and the output is pulled to high.

Maybe I already asked: If there is a public manual for this chip, please
add a link to it here.

> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/reset.h>
> +
> +#include <asm/div64.h>

The canonical include for that is <linux/math64.h>. This is also the
header that defines mul_u64_u64_div_u64(). Your driver seems to compile
only because clk.h includes math64.h via <linux/notifier.h> ->
<linux/srcu.h> -> <linux/workqueue.h> -> <linux/timer.h> ->
<linux/ktime.h> -> <linux/jiffies.h> -> <linux/math64.h>.

> +/*
> + * Offset RegisterName
> + * 0x0000 HLPERIOD0
> + * 0x0004 PERIOD0
> + * 0x0008 HLPERIOD1
> + * 0x000C PERIOD1
> + * 0x0010 HLPERIOD2
> + * 0x0014 PERIOD2
> + * 0x0018 HLPERIOD3
> + * 0x001C PERIOD3
> + * Four groups and every group is composed of HLPERIOD & PERIOD
> + */
> +#define SG2042_HLPERIOD(chan) ((chan) * 8 + 0)
> +#define SG2042_PERIOD(chan) ((chan) * 8 + 4)

s/SG2042_/SG2042_PWM_/ to match the function prefix and driver name?

> +
> +#define SG2042_PWM_CHANNELNUM	4
> +
> +/**
> + * struct sg2042_pwm_ddata - private driver data
> + * @base:		base address of mapped PWM registers
> + * @clk_rate_hz:	rate of base clock in HZ
> + */
> +struct sg2042_pwm_ddata {
> +	void __iomem *base;
> +	unsigned long clk_rate_hz;
> +};
> +
> +static void pwm_sg2042_config(void __iomem *base, unsigned int chan, u32 period, u32 hlperiod)

Maybe pass ddata here instead of base and add

	void __iomem *base = ddata->base;

to the function body. Then the calls simplify from

	pwm_sg2042_config(ddata->base, pwm->hwpwm, period, hlperiod);

to

	pwm_sg2042_config(ddata, pwm->hwpwm, period, hlperiod);

.

> +{
> +	writel(period, base + SG2042_PERIOD(chan));
> +	writel(hlperiod, base + SG2042_HLPERIOD(chan));
> +}
> +
> +static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			    const struct pwm_state *state)
> +{
> +	struct sg2042_pwm_ddata *ddata = pwmchip_get_drvdata(chip);
> +	u32 hlperiod;
> +	u32 period;

state->period is measured in ns, the local variable period however
holds a value measured in clock ticks. To make this still clearer than
it already is, I suggest to rename the variable to period_ticks. Ditto
for hlperiod.

> +	if (state->polarity == PWM_POLARITY_INVERSED)
> +		return -EINVAL;
> +
> +	if (!state->enabled) {
> +		pwm_sg2042_config(ddata->base, pwm->hwpwm, 0, 0);
> +		return 0;
> +	}
> +
> +	/*
> +	 * Period of High level (duty_cycle) = HLPERIOD x Period_clk
> +	 * Period of One Cycle (period) = PERIOD x Period_clk

s/Period/Duration/ ? What is Period_clk?

> +	 */
> +	period = min(mul_u64_u64_div_u64(ddata->clk_rate_hz, state->period, NSEC_PER_SEC), U32_MAX);
> +	hlperiod = min(mul_u64_u64_div_u64(ddata->clk_rate_hz, state->duty_cycle, NSEC_PER_SEC), U32_MAX);
> +
> +	if (hlperiod > period) {
> +		dev_err(pwmchip_parent(chip), "period < hlperiod, failed to apply current setting\n");
> +		return -EINVAL;

No need to check for that, .apply() is only called with
state->duty_cycle <= state->period.

> +	}
> +
> +	dev_dbg(pwmchip_parent(chip), "chan[%u]: period=%u, hlperiod=%u\n",
> +		pwm->hwpwm, period, hlperiod);
> +
> +	pwm_sg2042_config(ddata->base, pwm->hwpwm, period, hlperiod);
> +
> +	return 0;
> +}
> +
> +static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +				struct pwm_state *state)
> +{
> +	struct sg2042_pwm_ddata *ddata = pwmchip_get_drvdata(chip);
> +	unsigned int chan = pwm->hwpwm;
> +	u32 hlperiod;
> +	u32 period;
> +
> +	period = readl(ddata->base + SG2042_PERIOD(chan));
> +	hlperiod = readl(ddata->base + SG2042_HLPERIOD(chan));
> +
> +	if (!period && !hlperiod)
> +		state->enabled = false;
> +	else
> +		state->enabled = true;

What happens if hlperiod > period? Isn't period==0 enough for
state->enabled = false? Also if period==0 there is no use in determining
state->period and state->duty_cycle.

So I would expect here:

	period_ticks = readl(ddata->base + SG2042_PERIOD(chan));
	hlperiod_ticks = readl(ddata->base + SG2042_HLPERIOD(chan));

	if (!period_ticks) {
		state->enabled = false;
		return 0;
	}

	if (hlperiod_ticks > period_ticks)
		hlperiod_ticks = period_ticks;

	state->enabled = true;
	state->period = DIV_ROUND_UP_ULL((u64)period_ticks * NSEC_PER_SEC, ddata->clk_rate_hz);
	state->duty_cycle = DIV_ROUND_UP_ULL((u64)hlperiod_ticks * NSEC_PER_SEC, ddata->clk_rate_hz);
	state->polarity = PWM_POLARITY_NORMAL;

> +	state->period = DIV_ROUND_UP_ULL((u64)period * NSEC_PER_SEC, ddata->clk_rate_hz);
> +	state->duty_cycle = DIV_ROUND_UP_ULL((u64)hlperiod * NSEC_PER_SEC, ddata->clk_rate_hz);
> +
> +	state->polarity = PWM_POLARITY_NORMAL;
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops pwm_sg2042_ops = {
> +	.apply = pwm_sg2042_apply,
> +	.get_state = pwm_sg2042_get_state,
> +};
> +
> +static const struct of_device_id sg2042_pwm_ids[] = {
> +	{ .compatible = "sophgo,sg2042-pwm" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, sg2042_pwm_ids);
> +
> +static int pwm_sg2042_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sg2042_pwm_ddata *ddata;
> +	struct reset_control *rst;
> +	struct pwm_chip *chip;
> +	struct clk *clk;
> +	int ret;
> +
> +	chip = devm_pwmchip_alloc(dev, SG2042_PWM_CHANNELNUM, sizeof(*ddata));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +	ddata = pwmchip_get_drvdata(chip);
> +
> +	ddata->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(ddata->base))
> +		return PTR_ERR(ddata->base);
> +
> +	clk = devm_clk_get_enabled(dev, "apb");
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk), "failed to get base clk\n");

I like error messages to start consistently with a capital letter.

> +	ret = devm_clk_rate_exclusive_get(dev, clk);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get exclusive rate\n");
> +
> +	ddata->clk_rate_hz = clk_get_rate(clk);
> +	if (!ddata->clk_rate_hz || ddata->clk_rate_hz > NSEC_PER_SEC)

Please add a comment about why you check for > NSEC_PER_SEC.

> +		return dev_err_probe(dev, -EINVAL,
> +				     "Invalid clock rate: %lu\n", ddata->clk_rate_hz);
> +
> +	rst = devm_reset_control_get_optional_shared(dev, NULL);
> +	if (IS_ERR(rst))
> +		return dev_err_probe(dev, PTR_ERR(rst), "failed to get reset\n");
> +
> +	/* Deassert reset */
> +	ret = reset_control_deassert(rst);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to deassert\n");

There is devm_reset_control_get_optional_shared_deasserted() that does
the two calls devm_reset_control_get_optional_shared() and
reset_control_deassert() together and also cares for reasserting the
reset at remove time.

> +	chip->ops = &pwm_sg2042_ops;
> +	chip->atomic = true;
> +
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret < 0) {
> +		reset_control_assert(rst);
> +		return dev_err_probe(dev, ret, "failed to register PWM chip\n");
> +	}
> +
> +	return 0;
> +}

Best regards
Uwe

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

Re: [PATCH v6 1/3] dt-bindings: pwm: sophgo: add PWM controller for SG2042

[Uwe Kleine-König]

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

Hello,

On Wed, Dec 04, 2024 at 11:16:22AM +0800, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
> 
> Sophgo SG2042 contains a PWM controller, which has 4 channels and
> can generate PWM waveforms output.
> 
> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Nitpick: Put your S-o-b last.

> ---
>  .../bindings/pwm/sophgo,sg2042-pwm.yaml       | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml b/Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml
> new file mode 100644
> index 000000000000..5dea41fa4c44
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/sophgo,sg2042-pwm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sophgo SG2042 PWM controller
> +
> +maintainers:
> +  - Chen Wang <unicorn_wang@outlook.com>
> +
> +description:
> +  This controller contains 4 channels which can generate PWM waveforms.
> +
> +allOf:
> +  - $ref: pwm.yaml#
> +
> +properties:
> +  compatible:
> +    const: sophgo,sg2042-pwm
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: apb
> +
> +  resets:
> +    maxItems: 1
> +
> +  "#pwm-cells":
> +    const: 2

Please use 3 here.

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - resets
> +
> +unevaluatedProperties: false

Best regards
Uwe

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

Re: [PATCH v6 1/3] dt-bindings: pwm: sophgo: add PWM controller for SG2042

[Chen Wang]

On 2025/1/21 19:16, Uwe Kleine-König wrote:
> Hello,
>
> On Wed, Dec 04, 2024 at 11:16:22AM +0800, Chen Wang wrote:
>> From: Chen Wang <unicorn_wang@outlook.com>
>>
>> Sophgo SG2042 contains a PWM controller, which has 4 channels and
>> can generate PWM waveforms output.
>>
>> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Nitpick: Put your S-o-b last.
ok.
>> ---
>>   .../bindings/pwm/sophgo,sg2042-pwm.yaml       | 58 +++++++++++++++++++
>>   1 file changed, 58 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml b/Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml
>> new file mode 100644
>> index 000000000000..5dea41fa4c44
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml
>> @@ -0,0 +1,58 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pwm/sophgo,sg2042-pwm.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Sophgo SG2042 PWM controller
>> +
>> +maintainers:
>> +  - Chen Wang <unicorn_wang@outlook.com>
>> +
>> +description:
>> +  This controller contains 4 channels which can generate PWM waveforms.
>> +
>> +allOf:
>> +  - $ref: pwm.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    const: sophgo,sg2042-pwm
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    items:
>> +      - const: apb
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +  "#pwm-cells":
>> +    const: 2
> Please use 3 here.

Accepted.

Thanks.

Chen

[......]

Re: [PATCH v6 2/3] pwm: sophgo: add driver for Sophgo SG2042 PWM

[Chen Wang]

On 2025/1/21 19:14, Uwe Kleine-König wrote:
> On Wed, Dec 04, 2024 at 11:17:18AM +0800, Chen Wang wrote:
>> From: Chen Wang <unicorn_wang@outlook.com>
>>
>> Add a PWM driver for PWM controller in Sophgo SG2042 SoC.
>>
>> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
>> Signed-off-by: Sean Young <sean@mess.org>
>> ---
>>   drivers/pwm/Kconfig             |  10 ++
>>   drivers/pwm/Makefile            |   1 +
>>   drivers/pwm/pwm-sophgo-sg2042.c | 194 ++++++++++++++++++++++++++++++++
>>   3 files changed, 205 insertions(+)
>>   create mode 100644 drivers/pwm/pwm-sophgo-sg2042.c
[......]
>> diff --git a/drivers/pwm/pwm-sophgo-sg2042.c b/drivers/pwm/pwm-sophgo-sg2042.c
>> new file mode 100644
>> index 000000000000..a3d12505e4aa
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-sophgo-sg2042.c
>> @@ -0,0 +1,194 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Sophgo SG2042 PWM Controller Driver
>> + *
>> + * Copyright (C) 2024 Sophgo Technology Inc.
>> + * Copyright (C) 2024 Chen Wang <unicorn_wang@outlook.com>
>> + *
>> + * Limitations:
>> + * - After reset, the output of the PWM channel is always high.
>> + *   The value of HLPERIOD/PERIOD is 0.
>> + * - When HLPERIOD or PERIOD is reconfigured, PWM will start to
>> + *   output waveforms with the new configuration after completing
>> + *   the running period.
>> + * - When PERIOD and HLPERIOD is set to 0, the PWM wave output will
>> + *   be stopped and the output is pulled to high.
> Maybe I already asked: If there is a public manual for this chip, please
> add a link to it here.
There is a TRM on line, I will add the link here in next version.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/reset.h>
>> +
>> +#include <asm/div64.h>
> The canonical include for that is <linux/math64.h>. This is also the
> header that defines mul_u64_u64_div_u64(). Your driver seems to compile
> only because clk.h includes math64.h via <linux/notifier.h> ->
> <linux/srcu.h> -> <linux/workqueue.h> -> <linux/timer.h> ->
> <linux/ktime.h> -> <linux/jiffies.h> -> <linux/math64.h>.
Thanks, I will correct this.
>> +/*
>> + * Offset RegisterName
>> + * 0x0000 HLPERIOD0
>> + * 0x0004 PERIOD0
>> + * 0x0008 HLPERIOD1
>> + * 0x000C PERIOD1
>> + * 0x0010 HLPERIOD2
>> + * 0x0014 PERIOD2
>> + * 0x0018 HLPERIOD3
>> + * 0x001C PERIOD3
>> + * Four groups and every group is composed of HLPERIOD & PERIOD
>> + */
>> +#define SG2042_HLPERIOD(chan) ((chan) * 8 + 0)
>> +#define SG2042_PERIOD(chan) ((chan) * 8 + 4)
> s/SG2042_/SG2042_PWM_/ to match the function prefix and driver name?
Accepted.
>> +
>> +#define SG2042_PWM_CHANNELNUM	4
>> +
>> +/**
>> + * struct sg2042_pwm_ddata - private driver data
>> + * @base:		base address of mapped PWM registers
>> + * @clk_rate_hz:	rate of base clock in HZ
>> + */
>> +struct sg2042_pwm_ddata {
>> +	void __iomem *base;
>> +	unsigned long clk_rate_hz;
>> +};
>> +
>> +static void pwm_sg2042_config(void __iomem *base, unsigned int chan, u32 period, u32 hlperiod)
> Maybe pass ddata here instead of base and add
>
> 	void __iomem *base = ddata->base;
>
> to the function body. Then the calls simplify from
>
> 	pwm_sg2042_config(ddata->base, pwm->hwpwm, period, hlperiod);
>
> to
>
> 	pwm_sg2042_config(ddata, pwm->hwpwm, period, hlperiod);
>
> .

ok

>> +{
>> +	writel(period, base + SG2042_PERIOD(chan));
>> +	writel(hlperiod, base + SG2042_HLPERIOD(chan));
>> +}
>> +
>> +static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> +			    const struct pwm_state *state)
>> +{
>> +	struct sg2042_pwm_ddata *ddata = pwmchip_get_drvdata(chip);
>> +	u32 hlperiod;
>> +	u32 period;
> state->period is measured in ns, the local variable period however
> holds a value measured in clock ticks. To make this still clearer than
> it already is, I suggest to rename the variable to period_ticks. Ditto
> for hlperiod.
Agree, changing the name would indeed make it clearer
>> +	if (state->polarity == PWM_POLARITY_INVERSED)
>> +		return -EINVAL;
>> +
>> +	if (!state->enabled) {
>> +		pwm_sg2042_config(ddata->base, pwm->hwpwm, 0, 0);
>> +		return 0;
>> +	}
>> +
>> +	/*
>> +	 * Period of High level (duty_cycle) = HLPERIOD x Period_clk
>> +	 * Period of One Cycle (period) = PERIOD x Period_clk
> s/Period/Duration/ ? What is Period_clk?
Period_clk is period of the input clock, i.e. 1/(ddata->clk_rate_hz).
>
>> +	 */
>> +	period = min(mul_u64_u64_div_u64(ddata->clk_rate_hz, state->period, NSEC_PER_SEC), U32_MAX);
>> +	hlperiod = min(mul_u64_u64_div_u64(ddata->clk_rate_hz, state->duty_cycle, NSEC_PER_SEC), U32_MAX);
>> +
>> +	if (hlperiod > period) {
>> +		dev_err(pwmchip_parent(chip), "period < hlperiod, failed to apply current setting\n");
>> +		return -EINVAL;
> No need to check for that, .apply() is only called with
> state->duty_cycle <= state->period.
ok.
>> +	}
>> +
>> +	dev_dbg(pwmchip_parent(chip), "chan[%u]: period=%u, hlperiod=%u\n",
>> +		pwm->hwpwm, period, hlperiod);
>> +
>> +	pwm_sg2042_config(ddata->base, pwm->hwpwm, period, hlperiod);
>> +
>> +	return 0;
>> +}
>> +
>> +static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>> +				struct pwm_state *state)
>> +{
>> +	struct sg2042_pwm_ddata *ddata = pwmchip_get_drvdata(chip);
>> +	unsigned int chan = pwm->hwpwm;
>> +	u32 hlperiod;
>> +	u32 period;
>> +
>> +	period = readl(ddata->base + SG2042_PERIOD(chan));
>> +	hlperiod = readl(ddata->base + SG2042_HLPERIOD(chan));
>> +
>> +	if (!period && !hlperiod)
>> +		state->enabled = false;
>> +	else
>> +		state->enabled = true;
> What happens if hlperiod > period? Isn't period==0 enough for
> state->enabled = false? Also if period==0 there is no use in determining
> state->period and state->duty_cycle.
>
> So I would expect here:
>
> 	period_ticks = readl(ddata->base + SG2042_PERIOD(chan));
> 	hlperiod_ticks = readl(ddata->base + SG2042_HLPERIOD(chan));
>
> 	if (!period_ticks) {
> 		state->enabled = false;
> 		return 0;
> 	}
>
> 	if (hlperiod_ticks > period_ticks)
> 		hlperiod_ticks = period_ticks;
>
> 	state->enabled = true;
> 	state->period = DIV_ROUND_UP_ULL((u64)period_ticks * NSEC_PER_SEC, ddata->clk_rate_hz);
> 	state->duty_cycle = DIV_ROUND_UP_ULL((u64)hlperiod_ticks * NSEC_PER_SEC, ddata->clk_rate_hz);
> 	state->polarity = PWM_POLARITY_NORMAL;
Thanks, I will improve this.
>> +	state->period = DIV_ROUND_UP_ULL((u64)period * NSEC_PER_SEC, ddata->clk_rate_hz);
>> +	state->duty_cycle = DIV_ROUND_UP_ULL((u64)hlperiod * NSEC_PER_SEC, ddata->clk_rate_hz);
>> +
>> +	state->polarity = PWM_POLARITY_NORMAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct pwm_ops pwm_sg2042_ops = {
>> +	.apply = pwm_sg2042_apply,
>> +	.get_state = pwm_sg2042_get_state,
>> +};
>> +
>> +static const struct of_device_id sg2042_pwm_ids[] = {
>> +	{ .compatible = "sophgo,sg2042-pwm" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, sg2042_pwm_ids);
>> +
>> +static int pwm_sg2042_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct sg2042_pwm_ddata *ddata;
>> +	struct reset_control *rst;
>> +	struct pwm_chip *chip;
>> +	struct clk *clk;
>> +	int ret;
>> +
>> +	chip = devm_pwmchip_alloc(dev, SG2042_PWM_CHANNELNUM, sizeof(*ddata));
>> +	if (IS_ERR(chip))
>> +		return PTR_ERR(chip);
>> +	ddata = pwmchip_get_drvdata(chip);
>> +
>> +	ddata->base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(ddata->base))
>> +		return PTR_ERR(ddata->base);
>> +
>> +	clk = devm_clk_get_enabled(dev, "apb");
>> +	if (IS_ERR(clk))
>> +		return dev_err_probe(dev, PTR_ERR(clk), "failed to get base clk\n");
> I like error messages to start consistently with a capital letter.
ok.
>
>> +	ret = devm_clk_rate_exclusive_get(dev, clk);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to get exclusive rate\n");
>> +
>> +	ddata->clk_rate_hz = clk_get_rate(clk);
>> +	if (!ddata->clk_rate_hz || ddata->clk_rate_hz > NSEC_PER_SEC)
> Please add a comment about why you check for > NSEC_PER_SEC.
Seems no need to check this (> NSEC_PER_SEC ), I will remove it in next 
version.
>> +		return dev_err_probe(dev, -EINVAL,
>> +				     "Invalid clock rate: %lu\n", ddata->clk_rate_hz);
>> +
>> +	rst = devm_reset_control_get_optional_shared(dev, NULL);
>> +	if (IS_ERR(rst))
>> +		return dev_err_probe(dev, PTR_ERR(rst), "failed to get reset\n");
>> +
>> +	/* Deassert reset */
>> +	ret = reset_control_deassert(rst);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to deassert\n");
> There is devm_reset_control_get_optional_shared_deasserted() that does
> the two calls devm_reset_control_get_optional_shared() and
> reset_control_deassert() together and also cares for reasserting the
> reset at remove time.
ok, I will check this out in next version.
>> +	chip->ops = &pwm_sg2042_ops;
>> +	chip->atomic = true;
>> +
>> +	ret = devm_pwmchip_add(dev, chip);
>> +	if (ret < 0) {
>> +		reset_control_assert(rst);
>> +		return dev_err_probe(dev, ret, "failed to register PWM chip\n");
>> +	}
>> +
>> +	return 0;
>> +}
> Best regards
> Uwe

Thanks,

Chen.

Re: [PATCH v6 2/3] pwm: sophgo: add driver for Sophgo SG2042 PWM

[Uwe Kleine-König]

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

Hello,

On Wed, Jan 22, 2025 at 04:58:26PM +0800, Chen Wang wrote:
> On 2025/1/21 19:14, Uwe Kleine-König wrote:
> > On Wed, Dec 04, 2024 at 11:17:18AM +0800, Chen Wang wrote:
> > > +	ret = devm_clk_rate_exclusive_get(dev, clk);
> > > +	if (ret)
> > > +		return dev_err_probe(dev, ret, "failed to get exclusive rate\n");
> > > +
> > > +	ddata->clk_rate_hz = clk_get_rate(clk);
> > > +	if (!ddata->clk_rate_hz || ddata->clk_rate_hz > NSEC_PER_SEC)
> >
> > Please add a comment about why you check for > NSEC_PER_SEC.
> 
> Seems no need to check this (> NSEC_PER_SEC ), I will remove it in next
> version.

The rational is usually that

	mul_u64_u64_div_u64(ddata->clk_rate_hz, state->period, NSEC_PER_SEC)

cannot overflow. Your driver relies on that, too. (Which somewhat
proves that a comment is indeed necessary :-)

Best regards
Uwe

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

Re: [PATCH v6 2/3] pwm: sophgo: add driver for Sophgo SG2042 PWM

[Chen Wang]

On 2025/1/22 18:03, Uwe Kleine-König wrote:
> The rational is usually that
>
> 	mul_u64_u64_div_u64(ddata->clk_rate_hz, state->period, NSEC_PER_SEC)
>
> cannot overflow. Your driver relies on that, too. (Which somewhat
> proves that a comment is indeed necessary 🙂

Oh, thanks for pointing that out, I almost forgot about that. Will check 
this out in the next version.

Regards,

Chen