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

[PATCH 0/2] 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.

Chen Wang (2):
  dt-bindings: pwm: sophgo: add bindings for sg2042
  pwm: sophgo: add driver for Sophgo SG2042 PWM

 .../bindings/pwm/sophgo,sg2042-pwm.yaml       |  52 ++++++
 drivers/pwm/Kconfig                           |   9 ++
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-sophgo-sg2042.c               | 148 ++++++++++++++++++
 4 files changed, 210 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml
 create mode 100644 drivers/pwm/pwm-sophgo-sg2042.c


base-commit: 431c1646e1f86b949fa3685efc50b660a364c2b6
-- 
2.34.1

[PATCH 1/2] dt-bindings: pwm: sophgo: add bindings for sg2042

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

Add binding document for sophgo,sg2042-pwm.

Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
---
 .../bindings/pwm/sophgo,sg2042-pwm.yaml       | 52 +++++++++++++++++++
 1 file changed, 52 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..10212694dd41
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml
@@ -0,0 +1,52 @@
+# 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
+
+  "#pwm-cells":
+    # See pwm.yaml in this directory for a description of the cells format.
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    pwm@7f006000 {
+        compatible = "sophgo,sg2042-pwm";
+        reg = <0x7f006000 0x1000>;
+        #pwm-cells = <2>;
+        clocks = <&clock 67>;
+        clock-names = "apb";
+    };
-- 
2.34.1

[PATCH 2/2] 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>
---
 drivers/pwm/Kconfig             |   9 ++
 drivers/pwm/Makefile            |   1 +
 drivers/pwm/pwm-sophgo-sg2042.c | 148 ++++++++++++++++++++++++++++++++
 3 files changed, 158 insertions(+)
 create mode 100644 drivers/pwm/pwm-sophgo-sg2042.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 3e53838990f5..6287d63a84fd 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -577,6 +577,15 @@ 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 Sophgo SG2042 PWM controller.
+
+	  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 0be4f3e6dd43..ef2555e83183 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -52,6 +52,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..cf11ad54b4de
--- /dev/null
+++ b/drivers/pwm/pwm-sophgo-sg2042.c
@@ -0,0 +1,148 @@
+// 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>
+ */
+
+#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 <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 REG_HLPERIOD	0x0
+#define REG_PERIOD	0x4
+
+#define REG_GROUP	0x8
+
+#define SG2042_PWM_CHANNELNUM	4
+
+/**
+ * struct sg2042_pwm_chip - private data of PWM chip
+ * @base:		base address of mapped PWM registers
+ * @base_clk:		base clock used to drive the pwm controller
+ */
+struct sg2042_pwm_chip {
+	void __iomem *base;
+	struct clk *base_clk;
+};
+
+static inline
+struct sg2042_pwm_chip *to_sg2042_pwm_chip(struct pwm_chip *chip)
+{
+	return pwmchip_get_drvdata(chip);
+}
+
+static void pwm_sg2042_config(void __iomem *base, unsigned int channo, u32 period, u32 hlperiod)
+{
+	writel(period, base + REG_GROUP * channo + REG_PERIOD);
+	writel(hlperiod, base + REG_GROUP * channo + REG_HLPERIOD);
+}
+
+static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			    const struct pwm_state *state)
+{
+	struct sg2042_pwm_chip *sg2042_pwm = to_sg2042_pwm_chip(chip);
+	u32 hlperiod;
+	u32 period;
+	u64 f_clk;
+	u64 p;
+
+	if (!state->enabled) {
+		pwm_sg2042_config(sg2042_pwm->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
+	 */
+	f_clk = clk_get_rate(sg2042_pwm->base_clk);
+
+	p = f_clk * state->period;
+	do_div(p, NSEC_PER_SEC);
+	period = (u32)p;
+
+	p = f_clk * state->duty_cycle;
+	do_div(p, NSEC_PER_SEC);
+	hlperiod = (u32)p;
+
+	dev_dbg(pwmchip_parent(chip), "chan[%d]: period=%u, hlperiod=%u\n",
+		pwm->hwpwm, period, hlperiod);
+
+	pwm_sg2042_config(sg2042_pwm->base, pwm->hwpwm, period, hlperiod);
+
+	return 0;
+}
+
+static const struct pwm_ops pwm_sg2042_ops = {
+	.apply		= pwm_sg2042_apply,
+};
+
+static const struct of_device_id sg2042_pwm_match[] = {
+	{ .compatible = "sophgo,sg2042-pwm" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, sg2042_pwm_match);
+
+static int pwm_sg2042_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sg2042_pwm_chip *sg2042_pwm;
+	struct pwm_chip *chip;
+	int ret;
+
+	chip = devm_pwmchip_alloc(&pdev->dev, SG2042_PWM_CHANNELNUM, sizeof(*sg2042_pwm));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+	sg2042_pwm = to_sg2042_pwm_chip(chip);
+
+	chip->ops = &pwm_sg2042_ops;
+
+	sg2042_pwm->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(sg2042_pwm->base))
+		return PTR_ERR(sg2042_pwm->base);
+
+	sg2042_pwm->base_clk = devm_clk_get_enabled(&pdev->dev, "apb");
+	if (IS_ERR(sg2042_pwm->base_clk))
+		return dev_err_probe(dev, PTR_ERR(sg2042_pwm->base_clk),
+				     "failed to get base clk\n");
+
+	ret = devm_pwmchip_add(&pdev->dev, chip);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to register PWM chip\n");
+
+	platform_set_drvdata(pdev, chip);
+
+	return 0;
+}
+
+static struct platform_driver pwm_sg2042_driver = {
+	.driver	= {
+		.name	= "sg2042-pwm",
+		.of_match_table = of_match_ptr(sg2042_pwm_match),
+	},
+	.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

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

[Uwe Kleine-König]

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

Hello,

On Thu, Sep 05, 2024 at 08:10:42PM +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>
> ---
>  drivers/pwm/Kconfig             |   9 ++
>  drivers/pwm/Makefile            |   1 +
>  drivers/pwm/pwm-sophgo-sg2042.c | 148 ++++++++++++++++++++++++++++++++
>  3 files changed, 158 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sophgo-sg2042.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 3e53838990f5..6287d63a84fd 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -577,6 +577,15 @@ 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 Sophgo SG2042 PWM controller.
> +
> +	  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 0be4f3e6dd43..ef2555e83183 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -52,6 +52,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..cf11ad54b4de
> --- /dev/null
> +++ b/drivers/pwm/pwm-sophgo-sg2042.c
> @@ -0,0 +1,148 @@
> +// 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>
> + */
> +
> +#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 <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 REG_HLPERIOD	0x0
> +#define REG_PERIOD	0x4
> +
> +#define REG_GROUP	0x8
> +
> +#define SG2042_PWM_CHANNELNUM	4
> +
> +/**
> + * struct sg2042_pwm_chip - private data of PWM chip
> + * @base:		base address of mapped PWM registers
> + * @base_clk:		base clock used to drive the pwm controller
> + */
> +struct sg2042_pwm_chip {
> +	void __iomem *base;
> +	struct clk *base_clk;
> +};
> +
> +static inline
> +struct sg2042_pwm_chip *to_sg2042_pwm_chip(struct pwm_chip *chip)
> +{
> +	return pwmchip_get_drvdata(chip);
> +}
> +
> +static void pwm_sg2042_config(void __iomem *base, unsigned int channo, u32 period, u32 hlperiod)
> +{
> +	writel(period, base + REG_GROUP * channo + REG_PERIOD);
> +	writel(hlperiod, base + REG_GROUP * channo + REG_HLPERIOD);
> +}

I suggest to use the following instead:

	#define SG2042_HLPERIOD(chan) ((chan) * 8 + 0)
	#define SG2042_PERIOD(chan) ((chan) * 8 + 4)

	...

	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));
	}

The (subjective?) advantage is that the definition of SG2042_HLPERIOD
contains information about all channel's HLPERIOD register and the usage
in pwm_sg2042_config is obviously(?) right.

(Another advantage is that the register names have a prefix unique to
the driver, so there is no danger of mixing it up with some other
hardware's driver that might also have a register named "PERIOD".)

Is this racy? i.e. can it happen that between the two writel the output
is defined by the new period and old duty_cycle?

How does the hardware behave on reconfiguration? Does it complete the
currently running period? Please document that in a comment at the start
of the driver like many other drivers do. (See 

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

)

> +static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			    const struct pwm_state *state)
> +{
> +	struct sg2042_pwm_chip *sg2042_pwm = to_sg2042_pwm_chip(chip);
> +	u32 hlperiod;
> +	u32 period;
> +	u64 f_clk;
> +	u64 p;
> +
> +	if (!state->enabled) {
> +		pwm_sg2042_config(sg2042_pwm->base, pwm->hwpwm, 0, 0);
> +		return 0;
> +	}

Here you're missing (I guess):

	if (state->polarity == PWM_POLARITY_INVERSED)
		return -EINVAL;

> +	/*
> +	 * Period of High level (duty_cycle) = HLPERIOD x Period_clk
> +	 * Period of One Cycle (period) = PERIOD x Period_clk
> +	 */
> +	f_clk = clk_get_rate(sg2042_pwm->base_clk);
> +
> +	p = f_clk * state->period;

This might overflow.

> +	do_div(p, NSEC_PER_SEC);
> +	period = (u32)p;

This gets very wrong if p happens to be bigger than U32_MAX.

If you ensure f_clk <= NSEC_PER_SEC in .probe() (in combination with a
call to clk_rate_exclusive_get()), you can do:

	period_cycles = min(mul_u64_u64_div_u64(f_clk, state->period, NSEC_PER_SEC), U32_MAX);
	duty_cycles = min(mul_u64_u64_div_u64(f_clk, state->duty_cycle, NSEC_PER_SEC), U32_MAX);

This would also allow to store the clkrate in the driver private struct
and drop the pointer to the clk from there.

> +	p = f_clk * state->duty_cycle;
> +	do_div(p, NSEC_PER_SEC);
> +	hlperiod = (u32)p;
> +
> +	dev_dbg(pwmchip_parent(chip), "chan[%d]: period=%u, hlperiod=%u\n",
> +		pwm->hwpwm, period, hlperiod);

pwm->hwpwm is an unsigned int, so use %u for it.

> +	pwm_sg2042_config(sg2042_pwm->base, pwm->hwpwm, period, hlperiod);
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops pwm_sg2042_ops = {
> +	.apply		= pwm_sg2042_apply,

No .get_state() possible? Please use a single space before =.

> +};
> +
> +static const struct of_device_id sg2042_pwm_match[] = {
> +	{ .compatible = "sophgo,sg2042-pwm" },
> +	{ },

Please drop the , after the sentinel entry.

> +};
> +MODULE_DEVICE_TABLE(of, sg2042_pwm_match);
> +
> +static int pwm_sg2042_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sg2042_pwm_chip *sg2042_pwm;
> +	struct pwm_chip *chip;
> +	int ret;
> +
> +	chip = devm_pwmchip_alloc(&pdev->dev, SG2042_PWM_CHANNELNUM, sizeof(*sg2042_pwm));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +	sg2042_pwm = to_sg2042_pwm_chip(chip);
> +
> +	chip->ops = &pwm_sg2042_ops;
> +
> +	sg2042_pwm->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(sg2042_pwm->base))
> +		return PTR_ERR(sg2042_pwm->base);
> +
> +	sg2042_pwm->base_clk = devm_clk_get_enabled(&pdev->dev, "apb");
> +	if (IS_ERR(sg2042_pwm->base_clk))
> +		return dev_err_probe(dev, PTR_ERR(sg2042_pwm->base_clk),
> +				     "failed to get base clk\n");
> +
> +	ret = devm_pwmchip_add(&pdev->dev, chip);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to register PWM chip\n");
> +
> +	platform_set_drvdata(pdev, chip);

This is unused and should/can be dropped.

> +
> +	return 0;
> +}
> +
> +static struct platform_driver pwm_sg2042_driver = {
> +	.driver	= {
> +		.name	= "sg2042-pwm",
> +		.of_match_table = of_match_ptr(sg2042_pwm_match),
> +	},
> +	.probe = pwm_sg2042_probe,
> +};
> +module_platform_driver(pwm_sg2042_driver);

Please use a single space before =.

> +MODULE_AUTHOR("Chen Wang");
> +MODULE_DESCRIPTION("Sophgo SG2042 PWM driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.34.1
> 

Best regards
Uwe

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

Re: [PATCH 1/2] dt-bindings: pwm: sophgo: add bindings for sg2042

[Yixun Lan]

On 20:10 Thu 05 Sep     , Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
> 
> Add binding document for sophgo,sg2042-pwm.
> 
> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
> ---
>  .../bindings/pwm/sophgo,sg2042-pwm.yaml       | 52 +++++++++++++++++++
>  1 file changed, 52 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..10212694dd41
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml
> @@ -0,0 +1,52 @@
> +# 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: |
you can drop | here
> +  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
> +
> +  "#pwm-cells":
..
> +    # See pwm.yaml in this directory for a description of the cells format.
I think you can drop this comment, since no useful information added
also it's already refered to standard pwm.yaml
> +    const: 2
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
also "#pwm-cells"?
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pwm@7f006000 {
> +        compatible = "sophgo,sg2042-pwm";
> +        reg = <0x7f006000 0x1000>;
> +        #pwm-cells = <2>;
> +        clocks = <&clock 67>;
> +        clock-names = "apb";
> +    };
> -- 
> 2.34.1
> 

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

Re: [PATCH 1/2] dt-bindings: pwm: sophgo: add bindings for sg2042

[Krzysztof Kozlowski]

On Thu, Sep 05, 2024 at 08:10:25PM +0800, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
> 
> Add binding document for sophgo,sg2042-pwm.

A nit, subject: drop second/last, redundant "bindings for". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

Say something useful about hardware instead. The same in commit msg -
you keep saying obvious and duplicated commit subject.

> 
> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
> ---
>  .../bindings/pwm/sophgo,sg2042-pwm.yaml       | 52 +++++++++++++++++++
>  1 file changed, 52 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..10212694dd41
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml
> @@ -0,0 +1,52 @@
> +# 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: |

drop |

> +  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
> +
> +  "#pwm-cells":
> +    # See pwm.yaml in this directory for a description of the cells format.

Drop

> +    const: 2
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false

unevaluatedProperties instead

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: pwm: sophgo: add bindings for sg2042

[Krzysztof Kozlowski]

On 06/09/2024 04:44, Yixun Lan wrote:
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
> also "#pwm-cells"?

pwm.yaml has it?

Best regards,
Krzysztof

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

[kernel test robot]

Hi Chen,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 431c1646e1f86b949fa3685efc50b660a364c2b6]

url:    https://github.com/intel-lab-lkp/linux/commits/Chen-Wang/dt-bindings-pwm-sophgo-add-bindings-for-sg2042/20240905-201303
base:   431c1646e1f86b949fa3685efc50b660a364c2b6
patch link:    https://lore.kernel.org/r/3985690b29340982a45314bdcc914c554621e909.1725536870.git.unicorn_wang%40outlook.com
patch subject: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM
config: m68k-randconfig-r133-20240907 (https://download.01.org/0day-ci/archive/20240908/202409080100.h6lX5Asm-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.1.0
reproduce: (https://download.01.org/0day-ci/archive/20240908/202409080100.h6lX5Asm-lkp@intel.com/reproduce)

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

All warnings (new ones prefixed by >>):

>> drivers/pwm/pwm-sophgo-sg2042.c:99:34: warning: 'sg2042_pwm_match' defined but not used [-Wunused-const-variable=]
      99 | static const struct of_device_id sg2042_pwm_match[] = {
         |                                  ^~~~~~~~~~~~~~~~


vim +/sg2042_pwm_match +99 drivers/pwm/pwm-sophgo-sg2042.c

    98	
  > 99	static const struct of_device_id sg2042_pwm_match[] = {
   100		{ .compatible = "sophgo,sg2042-pwm" },
   101		{ },
   102	};
   103	MODULE_DEVICE_TABLE(of, sg2042_pwm_match);
   104	

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

Re: [PATCH 1/2] dt-bindings: pwm: sophgo: add bindings for sg2042

[Chen Wang]

On 2024/9/6 10:44, Yixun Lan wrote:
> On 20:10 Thu 05 Sep     , Chen Wang wrote:
>> From: Chen Wang <unicorn_wang@outlook.com>
>>
>> Add binding document for sophgo,sg2042-pwm.
>>
>> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
>> ---
>>   .../bindings/pwm/sophgo,sg2042-pwm.yaml       | 52 +++++++++++++++++++
>>   1 file changed, 52 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..10212694dd41
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml
>> @@ -0,0 +1,52 @@
>> +# 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: |
> you can drop | here
Ack and thanks.
>> +  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
>> +
>> +  "#pwm-cells":
> ..
>> +    # See pwm.yaml in this directory for a description of the cells format.
> I think you can drop this comment, since no useful information added
> also it's already refered to standard pwm.yaml
I add this comment just want to reminder.  Ok, anyway, it can be removed 
to make doc clear.
>> +    const: 2
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
> also "#pwm-cells"?
I think it has been required in pwm.yaml, so no need to required it 
again here.
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    pwm@7f006000 {
>> +        compatible = "sophgo,sg2042-pwm";
>> +        reg = <0x7f006000 0x1000>;
>> +        #pwm-cells = <2>;
>> +        clocks = <&clock 67>;
>> +        clock-names = "apb";
>> +    };
>> -- 
>> 2.34.1
>>

Re: [PATCH 1/2] dt-bindings: pwm: sophgo: add bindings for sg2042

[Chen Wang]

On 2024/9/6 18:31, Krzysztof Kozlowski wrote:
> On Thu, Sep 05, 2024 at 08:10:25PM +0800, Chen Wang wrote:
>> From: Chen Wang <unicorn_wang@outlook.com>
>>
>> Add binding document for sophgo,sg2042-pwm.
> A nit, subject: drop second/last, redundant "bindings for". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>
> Say something useful about hardware instead. The same in commit msg -
> you keep saying obvious and duplicated commit subject.
OK, thanks
>> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
>> ---
>>   .../bindings/pwm/sophgo,sg2042-pwm.yaml       | 52 +++++++++++++++++++
>>   1 file changed, 52 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..10212694dd41
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml
>> @@ -0,0 +1,52 @@
>> +# 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: |
> drop |
Ack and thansk.
>> +  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
>> +
>> +  "#pwm-cells":
>> +    # See pwm.yaml in this directory for a description of the cells format.
> Drop
Ack and thanks.
>> +    const: 2
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +
>> +additionalProperties: false
> unevaluatedProperties instead
Yes, my mistake, thanks.
> Best regards,
> Krzysztof
>

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

[Chen Wang]

hi,

I wonder why CONFIG_PWM_SOPHGO_SG2042 is enabeld for m68k? Please remove 
this.

Regards,

Chen

On 2024/9/8 1:58, kernel test robot wrote:
> Hi Chen,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on 431c1646e1f86b949fa3685efc50b660a364c2b6]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Chen-Wang/dt-bindings-pwm-sophgo-add-bindings-for-sg2042/20240905-201303
> base:   431c1646e1f86b949fa3685efc50b660a364c2b6
> patch link:    https://lore.kernel.org/r/3985690b29340982a45314bdcc914c554621e909.1725536870.git.unicorn_wang%40outlook.com
> patch subject: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM
> config: m68k-randconfig-r133-20240907 (https://download.01.org/0day-ci/archive/20240908/202409080100.h6lX5Asm-lkp@intel.com/config)
> compiler: m68k-linux-gcc (GCC) 14.1.0
> reproduce: (https://download.01.org/0day-ci/archive/20240908/202409080100.h6lX5Asm-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202409080100.h6lX5Asm-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
>>> drivers/pwm/pwm-sophgo-sg2042.c:99:34: warning: 'sg2042_pwm_match' defined but not used [-Wunused-const-variable=]
>        99 | static const struct of_device_id sg2042_pwm_match[] = {
>           |                                  ^~~~~~~~~~~~~~~~
>
>
> vim +/sg2042_pwm_match +99 drivers/pwm/pwm-sophgo-sg2042.c
>
>      98	
>    > 99	static const struct of_device_id sg2042_pwm_match[] = {
>     100		{ .compatible = "sophgo,sg2042-pwm" },
>     101		{ },
>     102	};
>     103	MODULE_DEVICE_TABLE(of, sg2042_pwm_match);
>     104	
>

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

[Geert Uytterhoeven]

Hi Chen,

On Mon, Sep 9, 2024 at 10:26 AM Chen Wang <unicorn_wang@outlook.com> wrote:
> I wonder why CONFIG_PWM_SOPHGO_SG2042 is enabeld for m68k? Please remove
> this.

Because it depends on ARCH_SOPHGO || COMPILE_TEST.
So it can be enabled on all architectures when compile-testing.

> On 2024/9/8 1:58, kernel test robot wrote:
> > kernel test robot noticed the following build warnings:
> >
> > [auto build test WARNING on 431c1646e1f86b949fa3685efc50b660a364c2b6]
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/Chen-Wang/dt-bindings-pwm-sophgo-add-bindings-for-sg2042/20240905-201303
> > base:   431c1646e1f86b949fa3685efc50b660a364c2b6
> > patch link:    https://lore.kernel.org/r/3985690b29340982a45314bdcc914c554621e909.1725536870.git.unicorn_wang%40outlook.com
> > patch subject: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM
> > config: m68k-randconfig-r133-20240907 (https://download.01.org/0day-ci/archive/20240908/202409080100.h6lX5Asm-lkp@intel.com/config)
> > compiler: m68k-linux-gcc (GCC) 14.1.0
> > reproduce: (https://download.01.org/0day-ci/archive/20240908/202409080100.h6lX5Asm-lkp@intel.com/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202409080100.h6lX5Asm-lkp@intel.com/
> >
> > All warnings (new ones prefixed by >>):
> >
> >>> drivers/pwm/pwm-sophgo-sg2042.c:99:34: warning: 'sg2042_pwm_match' defined but not used [-Wunused-const-variable=]
> >        99 | static const struct of_device_id sg2042_pwm_match[] = {
> >           |                                  ^~~~~~~~~~~~~~~~
> >
> >
> > vim +/sg2042_pwm_match +99 drivers/pwm/pwm-sophgo-sg2042.c
> >
> >      98
> >    > 99       static const struct of_device_id sg2042_pwm_match[] = {
> >     100               { .compatible = "sophgo,sg2042-pwm" },
> >     101               { },
> >     102       };
> >     103       MODULE_DEVICE_TABLE(of, sg2042_pwm_match);
> >     104

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Chen Wang]

On 2024/9/9 16:45, Geert Uytterhoeven wrote:
> Hi Chen,
>
> On Mon, Sep 9, 2024 at 10:26 AM Chen Wang <unicorn_wang@outlook.com> wrote:
>> I wonder why CONFIG_PWM_SOPHGO_SG2042 is enabeld for m68k? Please remove
>> this.
> Because it depends on ARCH_SOPHGO || COMPILE_TEST.
> So it can be enabled on all architectures when compile-testing.
Thanks, it's really a potential defect when CONFIG_OF is not set, will 
fix this in next version.
>> On 2024/9/8 1:58, kernel test robot wrote:
>>> kernel test robot noticed the following build warnings:
>>>
>>> [auto build test WARNING on 431c1646e1f86b949fa3685efc50b660a364c2b6]
>>>
>>> url:    https://github.com/intel-lab-lkp/linux/commits/Chen-Wang/dt-bindings-pwm-sophgo-add-bindings-for-sg2042/20240905-201303
>>> base:   431c1646e1f86b949fa3685efc50b660a364c2b6
>>> patch link:    https://lore.kernel.org/r/3985690b29340982a45314bdcc914c554621e909.1725536870.git.unicorn_wang%40outlook.com
>>> patch subject: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM
>>> config: m68k-randconfig-r133-20240907 (https://download.01.org/0day-ci/archive/20240908/202409080100.h6lX5Asm-lkp@intel.com/config)
>>> compiler: m68k-linux-gcc (GCC) 14.1.0
>>> reproduce: (https://download.01.org/0day-ci/archive/20240908/202409080100.h6lX5Asm-lkp@intel.com/reproduce)
>>>
>>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>>> the same patch/commit), kindly add following tags
>>> | Reported-by: kernel test robot <lkp@intel.com>
>>> | Closes: https://lore.kernel.org/oe-kbuild-all/202409080100.h6lX5Asm-lkp@intel.com/
>>>
>>> All warnings (new ones prefixed by >>):
>>>
>>>>> drivers/pwm/pwm-sophgo-sg2042.c:99:34: warning: 'sg2042_pwm_match' defined but not used [-Wunused-const-variable=]
>>>         99 | static const struct of_device_id sg2042_pwm_match[] = {
>>>            |                                  ^~~~~~~~~~~~~~~~
>>>
>>>
>>> vim +/sg2042_pwm_match +99 drivers/pwm/pwm-sophgo-sg2042.c
>>>
>>>       98
>>>     > 99       static const struct of_device_id sg2042_pwm_match[] = {
>>>      100               { .compatible = "sophgo,sg2042-pwm" },
>>>      101               { },
>>>      102       };
>>>      103       MODULE_DEVICE_TABLE(of, sg2042_pwm_match);
>>>      104
> Gr{oetje,eeting}s,
>
>                          Geert
>

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

[Chen Wang]

On 2024/9/6 0:03, Uwe Kleine-König wrote:
> Hello,
>
> On Thu, Sep 05, 2024 at 08:10:42PM +0800, Chen Wang wrote:
[......]
>> +static void pwm_sg2042_config(void __iomem *base, unsigned int channo, u32 period, u32 hlperiod)
>> +{
>> +	writel(period, base + REG_GROUP * channo + REG_PERIOD);
>> +	writel(hlperiod, base + REG_GROUP * channo + REG_HLPERIOD);
>> +}
> I suggest to use the following instead:
>
> 	#define SG2042_HLPERIOD(chan) ((chan) * 8 + 0)
> 	#define SG2042_PERIOD(chan) ((chan) * 8 + 4)
>
> 	...
>
> 	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));
> 	}
>
> The (subjective?) advantage is that the definition of SG2042_HLPERIOD
> contains information about all channel's HLPERIOD register and the usage
> in pwm_sg2042_config is obviously(?) right.
>
> (Another advantage is that the register names have a prefix unique to
> the driver, so there is no danger of mixing it up with some other
> hardware's driver that might also have a register named "PERIOD".)
Agree, will fix this in next version.
> Is this racy? i.e. can it happen that between the two writel the output
> is defined by the new period and old duty_cycle?
>
> How does the hardware behave on reconfiguration? Does it complete the
> currently running period? Please document that in a comment at the start
> of the driver like many other drivers do. (See
>
> 	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c
>
> )

When hlperiod or period is modified, PWM will start to output waveforms 
with the new configuration after completing the running period. Will 
document in a comment as other drivers do.

>> +static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> +			    const struct pwm_state *state)
>> +{
>> +	struct sg2042_pwm_chip *sg2042_pwm = to_sg2042_pwm_chip(chip);
>> +	u32 hlperiod;
>> +	u32 period;
>> +	u64 f_clk;
>> +	u64 p;
>> +
>> +	if (!state->enabled) {
>> +		pwm_sg2042_config(sg2042_pwm->base, pwm->hwpwm, 0, 0);
>> +		return 0;
>> +	}
> Here you're missing (I guess):
>
> 	if (state->polarity == PWM_POLARITY_INVERSED)
> 		return -EINVAL;
Yes, it is required, will add this in next version, thanks.
>> +	/*
>> +	 * Period of High level (duty_cycle) = HLPERIOD x Period_clk
>> +	 * Period of One Cycle (period) = PERIOD x Period_clk
>> +	 */
>> +	f_clk = clk_get_rate(sg2042_pwm->base_clk);
>> +
>> +	p = f_clk * state->period;
> This might overflow.
>
>> +	do_div(p, NSEC_PER_SEC);
>> +	period = (u32)p;
> This gets very wrong if p happens to be bigger than U32_MAX.
>
> If you ensure f_clk <= NSEC_PER_SEC in .probe() (in combination with a
> call to clk_rate_exclusive_get()), you can do:
>
> 	period_cycles = min(mul_u64_u64_div_u64(f_clk, state->period, NSEC_PER_SEC), U32_MAX);
> 	duty_cycles = min(mul_u64_u64_div_u64(f_clk, state->duty_cycle, NSEC_PER_SEC), U32_MAX);
>
> This would also allow to store the clkrate in the driver private struct
> and drop the pointer to the clk from there.
f_clk should be 100M and <=NSEC_PER_SEC, will improve the code as your 
suggestion, thanks.
>> +	p = f_clk * state->duty_cycle;
>> +	do_div(p, NSEC_PER_SEC);
>> +	hlperiod = (u32)p;
>> +
>> +	dev_dbg(pwmchip_parent(chip), "chan[%d]: period=%u, hlperiod=%u\n",
>> +		pwm->hwpwm, period, hlperiod);
> pwm->hwpwm is an unsigned int, so use %u for it.
Ok, will fix.
>> +	pwm_sg2042_config(sg2042_pwm->base, pwm->hwpwm, period, hlperiod);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct pwm_ops pwm_sg2042_ops = {
>> +	.apply		= pwm_sg2042_apply,
> No .get_state() possible? Please use a single space before =.
Will add .get_state() and improve the format.
>> +};
>> +
>> +static const struct of_device_id sg2042_pwm_match[] = {
>> +	{ .compatible = "sophgo,sg2042-pwm" },
>> +	{ },
> Please drop the , after the sentinel entry.
OK
>> +};
>> +MODULE_DEVICE_TABLE(of, sg2042_pwm_match);
>> +
>> +static int pwm_sg2042_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct sg2042_pwm_chip *sg2042_pwm;
>> +	struct pwm_chip *chip;
>> +	int ret;
>> +
>> +	chip = devm_pwmchip_alloc(&pdev->dev, SG2042_PWM_CHANNELNUM, sizeof(*sg2042_pwm));
>> +	if (IS_ERR(chip))
>> +		return PTR_ERR(chip);
>> +	sg2042_pwm = to_sg2042_pwm_chip(chip);
>> +
>> +	chip->ops = &pwm_sg2042_ops;
>> +
>> +	sg2042_pwm->base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(sg2042_pwm->base))
>> +		return PTR_ERR(sg2042_pwm->base);
>> +
>> +	sg2042_pwm->base_clk = devm_clk_get_enabled(&pdev->dev, "apb");
>> +	if (IS_ERR(sg2042_pwm->base_clk))
>> +		return dev_err_probe(dev, PTR_ERR(sg2042_pwm->base_clk),
>> +				     "failed to get base clk\n");
>> +
>> +	ret = devm_pwmchip_add(&pdev->dev, chip);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "failed to register PWM chip\n");
>> +
>> +	platform_set_drvdata(pdev, chip);
> This is unused and should/can be dropped.
OK, will drop this, thanks for point it out. I should have cleaned it up.
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver pwm_sg2042_driver = {
>> +	.driver	= {
>> +		.name	= "sg2042-pwm",
>> +		.of_match_table = of_match_ptr(sg2042_pwm_match),
>> +	},
>> +	.probe = pwm_sg2042_probe,
>> +};
>> +module_platform_driver(pwm_sg2042_driver);
> Please use a single space before =.
Ok, will improve the format.
>> +MODULE_AUTHOR("Chen Wang");
>> +MODULE_DESCRIPTION("Sophgo SG2042 PWM driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.34.1
>>
> Best regards
> Uwe

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

[Uwe Kleine-König]

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

Hello Chen,

On Mon, Sep 09, 2024 at 05:08:41PM +0800, Chen Wang wrote:
> On 2024/9/9 16:45, Geert Uytterhoeven wrote:
> > On Mon, Sep 9, 2024 at 10:26 AM Chen Wang <unicorn_wang@outlook.com> wrote:
> > > I wonder why CONFIG_PWM_SOPHGO_SG2042 is enabeld for m68k? Please remove
> > > this.
> > Because it depends on ARCH_SOPHGO || COMPILE_TEST.
> > So it can be enabled on all architectures when compile-testing.
> Thanks, it's really a potential defect when CONFIG_OF is not set, will fix
> this in next version.

BTW, the right approach to fix that is to not use of_match_ptr when
initializing pwm_sg2042_driver.driver.of_match_table.

Best regards
Uwe

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

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

[Chen Wang]

On 2024/9/9 19:45, Uwe Kleine-König wrote:
> Hello Chen,
>
> On Mon, Sep 09, 2024 at 05:08:41PM +0800, Chen Wang wrote:
>> On 2024/9/9 16:45, Geert Uytterhoeven wrote:
>>> On Mon, Sep 9, 2024 at 10:26 AM Chen Wang <unicorn_wang@outlook.com> wrote:
>>>> I wonder why CONFIG_PWM_SOPHGO_SG2042 is enabeld for m68k? Please remove
>>>> this.
>>> Because it depends on ARCH_SOPHGO || COMPILE_TEST.
>>> So it can be enabled on all architectures when compile-testing.
>> Thanks, it's really a potential defect when CONFIG_OF is not set, will fix
>> this in next version.
> BTW, the right approach to fix that is to not use of_match_ptr when
> initializing pwm_sg2042_driver.driver.of_match_table.
>
> Best regards
> Uwe

Thank you Uwe, that's what I said potential defect, have fixed here.

Thanks,

Chen