Skip to contentSkip to conversation

riscv: pwm: sophgo: add pwm support for SG2044

PATCH Superseded Longbin Li 7 participants 11 messages Apr 7, 2025 – Apr 9, 2025

Subsystems (4)
Device tree bindings RISC-V PWM Sophgo devicetrees and drivers

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Longbin Li

author
#0 lore raw

cover riscv: pwm: sophgo: add pwm support for SG2044

This patch adds PWM controller support for four independent PWM channel outputs.

ghost (2):
dt-bindings: pwm: sophgo: add pwm controller for SG2044
pwm: sophgo: add driver for SG2044

 .../bindings/pwm/sophgo,sg2042-pwm.yaml       |   4 +-
 drivers/pwm/pwm-sophgo-sg2042.c               | 162 +++++++++++++++---
 2 files changed, 141 insertions(+), 25 deletions(-)

--
2.48.1

author
#1 lore raw

PATCH 1/2 dt-bindings: pwm: sophgo: add pwm controller for SG2044

From: ghost <[email protected]>

Add compatible string for PWM controller on SG2044.

Signed-off-by Longbin Li <[email protected]>
---
 Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml +3 -1
index bbb6326d47d7..e0e91aa237ec 100644
@@ -17,7 +17,9 @@ allOf:
 
 properties:
   compatible:
-    const: sophgo,sg2042-pwm
+    enum:
+      - sophgo,sg2042-pwm
+      - sophgo,sg2044-pwm
 
   reg:
     maxItems: 1

--
2.48.1

maintainer
#2 lore raw

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

Hello,

Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml allOf: · line 22

Quoted 12 lines · Longbin Li
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml +3 -1
index bbb6326d47d7..e0e91aa237ec 100644
@@ -17,7 +17,9 @@ allOf:
 
 properties:
   compatible:
-    const: sophgo,sg2042-pwm
+    enum:
+      - sophgo,sg2042-pwm
+      - sophgo,sg2044-pwm

Given that the sg2044 has more registers (to e.g. implement different polarity), but the sg2042 registers are identical, I wonder if the 2044 device should use:

	compatible = "sophgo,sg2044-pwm", "sophgo,sg2042-pwm";

Note, I'm unsure here, only providing input to people who are more knowledgeable in DT that I am.

Best regards
Uwe

maintainer
#3 lore raw

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

Quoted 20 lines · Uwe Kleine-König

Hello,

Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml allOf: · line 22

Quoted 12 lines · Longbin Li
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml +3 -1
index bbb6326d47d7..e0e91aa237ec 100644
@@ -17,7 +17,9 @@ allOf:
 
 properties:
   compatible:
-    const: sophgo,sg2042-pwm
+    enum:
+      - sophgo,sg2042-pwm
+      - sophgo,sg2044-pwm

Given that the sg2044 has more registers (to e.g. implement different polarity), but the sg2042 registers are identical, I wonder if the 2044 device should use:

Quoted 4 lines
	compatible = "sophgo,sg2044-pwm", "sophgo,sg2042-pwm";

Note, I'm unsure here, only providing input to people who are more knowledgeable in DT that I am.

Depends if s/w only understanding "sophgo,sg2042-pwm" will work on the 2044. IOW, will a kernel without the driver change here work?

Rob

maintainer
#4 lore raw

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

Quoted 21 lines · Rob Herring
Quoted 20 lines · Uwe Kleine-König

Hello,

Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml allOf: · line 22

Quoted 12 lines · Longbin Li
Documentation/devicetree/bindings/pwm/sophgo,sg2042-pwm.yaml +3 -1
index bbb6326d47d7..e0e91aa237ec 100644
@@ -17,7 +17,9 @@ allOf:
 
 properties:
   compatible:
-    const: sophgo,sg2042-pwm
+    enum:
+      - sophgo,sg2042-pwm
+      - sophgo,sg2044-pwm

Given that the sg2044 has more registers (to e.g. implement different polarity), but the sg2042 registers are identical, I wonder if the 2044 device should use:

Quoted 8 lines
Quoted 4 lines
	compatible = "sophgo,sg2044-pwm", "sophgo,sg2042-pwm";

Note, I'm unsure here, only providing input to people who are more knowledgeable in DT that I am.

Depends if s/w only understanding "sophgo,sg2042-pwm" will work on the 2044. IOW, will a kernel without the driver change here work?

No luck, the logic for SG2042 is broken on SG2044. In fact, it seems to be more familiar to the pwm ip on CV1800 than it on SG2042.

Regards,
Inochi

author
#6 lore raw

PATCH 2/2 pwm: sophgo: add driver for SG2044

In reply to Longbin Li

From: ghost <[email protected]>

Add PWM controller for SG2044.

Signed-off-by Longbin Li <[email protected]>
---
 drivers/pwm/pwm-sophgo-sg2042.c | 162 +++++++++++++++++++++++++++-----
 1 file changed, 138 insertions(+), 24 deletions(-)
drivers/pwm/pwm-sophgo-sg2042.c +138 -24
index ff4639d849ce..c62e8c758d87 100644
@@ -26,20 +26,22 @@
 #include <linux/pwm.h>
 #include <linux/reset.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_PWM_HLPERIOD(chan) ((chan) * 8 + 0)
-#define SG2042_PWM_PERIOD(chan) ((chan) * 8 + 4)
+#define REG_HLPERIOD		0x0
+#define REG_PERIOD		0x4
+#define REG_GROUP		0x8
+#define REG_POLARITY		0x40
+
+#define REG_PWMSTART		0x44
+#define REG_PWMUPDATE		0x4C
+#define REG_SHIFTCOUNT		0x80
+#define REG_SHIFTSTART		0x90
+#define REG_PWM_OE		0xD0
+
+#define PWM_REG_NUM		0x80
+
+#define PWM_POLARITY_MASK(n) BIT(n)
+#define PWM_HLPERIOD(chan) ((chan) * REG_GROUP + REG_HLPERIOD)
+#define PWM_PERIOD(chan) ((chan) * REG_GROUP + REG_PERIOD)
 
 #define SG2042_PWM_CHANNELNUM	4
 

@@ -51,6 +53,12 @@
 struct sg2042_pwm_ddata {
 	void __iomem *base;
 	unsigned long clk_rate_hz;
+	struct mutex lock;
+};
+
+struct sg2042_chip_data {
+	const struct pwm_ops ops;
+	bool atomic;
 };
 
 /*

@@ -62,8 +70,8 @@ static void pwm_sg2042_config(struct sg2042_pwm_ddata *ddata, unsigned int chan,
 {
 	void __iomem *base = ddata->base;
 
-	writel(period_ticks, base + SG2042_PWM_PERIOD(chan));
-	writel(hlperiod_ticks, base + SG2042_PWM_HLPERIOD(chan));
+	writel(period_ticks, base + PWM_PERIOD(chan));
+	writel(hlperiod_ticks, base + PWM_HLPERIOD(chan));
 }
 
 static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm,

@@ -104,8 +112,8 @@ static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	u32 hlperiod_ticks;
 	u32 period_ticks;
 
-	period_ticks = readl(ddata->base + SG2042_PWM_PERIOD(chan));
-	hlperiod_ticks = readl(ddata->base + SG2042_PWM_HLPERIOD(chan));
+	period_ticks = readl(ddata->base + PWM_PERIOD(chan));
+	hlperiod_ticks = readl(ddata->base + PWM_HLPERIOD(chan));
 
 	if (!period_ticks) {
 		state->enabled = false;

@@ -123,13 +131,112 @@ static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
-static const struct pwm_ops pwm_sg2042_ops = {
-	.apply = pwm_sg2042_apply,
-	.get_state = pwm_sg2042_get_state,
+static void pwm_sg2044_config(struct sg2042_pwm_ddata *ddata, struct pwm_device *pwm, bool enabled)
+{
+	u32 pwm_value;
+
+	pwm_value = readl(ddata->base + REG_PWMSTART);
+
+	if (enabled)
+		writel(pwm_value | BIT(pwm->hwpwm), ddata->base + REG_PWMSTART);
+	else
+		writel(pwm_value & ~BIT(pwm->hwpwm), ddata->base + REG_PWMSTART);
+}
+
+static void pwm_sg2044_set_outputenable(struct sg2042_pwm_ddata *ddata, struct pwm_device *pwm,
+					bool enabled)
+{
+	u32 pwm_value;
+
+	pwm_value = readl(ddata->base + REG_PWM_OE);
+
+	if (enabled)
+		writel(pwm_value | BIT(pwm->hwpwm), ddata->base + REG_PWM_OE);
+	else
+		writel(pwm_value & ~BIT(pwm->hwpwm), ddata->base + REG_PWM_OE);
+}
+
+static int pwm_sg2044_set_polarity(struct sg2042_pwm_ddata *ddata, struct pwm_device *pwm,
+				   const struct pwm_state *state)
+{
+	enum pwm_polarity polarity;
+	u32 pwm_value;
+
+	pwm_value = readl(ddata->base + REG_POLARITY);
+
+	polarity = state->polarity;
+
+	if (polarity == PWM_POLARITY_NORMAL)
+		pwm_value &= ~BIT(pwm->hwpwm);
+	else
+		pwm_value |= BIT(pwm->hwpwm);
+
+	writel(pwm_value, ddata->base + REG_POLARITY);
+
+	return 0;
+}
+
+static int pwm_sg2044_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_ticks;
+	u32 period_ticks;
+
+	if (!state->enabled) {
+		pwm_sg2044_config(ddata, pwm, false);
+		return 0;
+	}
+
+	pwm_sg2044_set_polarity(ddata, pwm, state);
+
+	/*
+	 * Duration of High level (duty_cycle) = HLPERIOD x Period_of_input_clk
+	 * Duration of One Cycle (period) = PERIOD x Period_of_input_clk
+	 */
+	period_ticks = min(mul_u64_u64_div_u64(ddata->clk_rate_hz, state->period,
+					       NSEC_PER_SEC), U32_MAX);
+	hlperiod_ticks = min(mul_u64_u64_div_u64(ddata->clk_rate_hz, state->duty_cycle,
+						 NSEC_PER_SEC), U32_MAX);
+
+	dev_dbg(pwmchip_parent(chip), "chan[%u]: PERIOD=%u, HLPERIOD=%u\n",
+		pwm->hwpwm, period_ticks, hlperiod_ticks);
+
+	pwm_sg2042_config(ddata, pwm->hwpwm, period_ticks, hlperiod_ticks);
+
+	guard(mutex)(&ddata->lock);
+
+	/*
+	 * re-enable PWMSTART to refresh the register period
+	 */
+	pwm_sg2044_config(ddata, pwm, false);
+	pwm_sg2044_set_outputenable(ddata, pwm, true);
+	pwm_sg2044_config(ddata, pwm, true);
+
+	return 0;
+}
+
+static const struct sg2042_chip_data sg2042_chip_data = {
+	.ops = {
+		.apply = pwm_sg2042_apply,
+		.get_state = pwm_sg2042_get_state,
+	},
+	.atomic = true,
+};
+
+static const struct sg2042_chip_data sg2044_chip_data = {
+	.ops = {
+		.apply = pwm_sg2044_apply,
+		.get_state = pwm_sg2042_get_state,
+	},
+	.atomic = false,
 };
 
 static const struct of_device_id sg2042_pwm_ids[] = {
-	{ .compatible = "sophgo,sg2042-pwm" },
+	{ .compatible = "sophgo,sg2042-pwm",
+	  .data = &sg2042_chip_data },
+	{ .compatible = "sophgo,sg2044-pwm",
+	  .data = &sg2044_chip_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sg2042_pwm_ids);

@@ -137,17 +244,24 @@ MODULE_DEVICE_TABLE(of, sg2042_pwm_ids);
 static int pwm_sg2042_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	const struct sg2042_chip_data *chip_data;
 	struct sg2042_pwm_ddata *ddata;
 	struct reset_control *rst;
 	struct pwm_chip *chip;
 	struct clk *clk;
 	int ret;
 
+	chip_data = device_get_match_data(dev);
+	if (!chip_data)
+		return -ENODEV;
+
 	chip = devm_pwmchip_alloc(dev, SG2042_PWM_CHANNELNUM, sizeof(*ddata));
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);
 	ddata = pwmchip_get_drvdata(chip);
 
+	mutex_init(&ddata->lock);
+
 	ddata->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(ddata->base))
 		return PTR_ERR(ddata->base);

@@ -170,8 +284,8 @@ static int pwm_sg2042_probe(struct platform_device *pdev)
 	if (IS_ERR(rst))
 		return dev_err_probe(dev, PTR_ERR(rst), "Failed to get reset\n");
 
-	chip->ops = &pwm_sg2042_ops;
-	chip->atomic = true;
+	chip->ops = &chip_data->ops;
+	chip->atomic = chip_data->atomic;
 
 	ret = devm_pwmchip_add(dev, chip);
 	if (ret < 0)

--
2.48.1

maintainer
#7 lore raw

Re: [PATCH 2/2] pwm: sophgo: add driver for SG2044

Quoted 1 line · Longbin Li

From: ghost <[email protected]>

Huh, is that a real name?

drivers/pwm/pwm-sophgo-sg2042.c

Quoted 11 lines

Add PWM controller for SG2044.

Signed-off-by Longbin Li <[email protected]>
---
 drivers/pwm/pwm-sophgo-sg2042.c | 162 +++++++++++++++++++++++++++-----
 1 file changed, 138 insertions(+), 24 deletions(-)
drivers/pwm/pwm-sophgo-sg2042.c +0 -0
index ff4639d849ce..c62e8c758d87 100644

The Limitations paragraph needs updating. E.g. SG2044 seems to support polarity while SG2042 doesn't.

Quoted 11 lines
drivers/pwm/pwm-sophgo-sg2042.c +0 -7
@@ -26,20 +26,22 @@
 #include <linux/pwm.h>
 #include <linux/reset.h>
 
-/*
- * Offset RegisterName
- * 0x0000 HLPERIOD0
- * 0x0004 PERIOD0
- * 0x0008 HLPERIOD1
- * 0x000C PERIOD1
- * 0x0010 HLPERIOD2

drivers/pwm/pwm-sophgo-sg2042.c line 31

Quoted 10 lines
drivers/pwm/pwm-sophgo-sg2042.c +3 -7
- * 0x0014 PERIOD2
- * 0x0018 HLPERIOD3
- * 0x001C PERIOD3
- * Four groups and every group is composed of HLPERIOD & PERIOD
- */
-#define SG2042_PWM_HLPERIOD(chan) ((chan) * 8 + 0)
-#define SG2042_PWM_PERIOD(chan) ((chan) * 8 + 4)
+#define REG_HLPERIOD		0x0
+#define REG_PERIOD		0x4
+#define REG_GROUP		0x8

REG_GROUP belongs to a different category than REG_PERIOD. So please use a different schema to name it (or drop it, see below).

drivers/pwm/pwm-sophgo-sg2042.c line 37

Quoted 6 lines
drivers/pwm/pwm-sophgo-sg2042.c +6 -0
+#define REG_POLARITY		0x40
+
+#define REG_PWMSTART		0x44
+#define REG_PWMUPDATE		0x4C
+#define REG_SHIFTCOUNT		0x80
+#define REG_SHIFTSTART		0x90

REG_SHIFTCOUNT and REG_SHIFTSTART are unused.

drivers/pwm/pwm-sophgo-sg2042.c line 38

Quoted 1 line
drivers/pwm/pwm-sophgo-sg2042.c +1 -0
+#define REG_PWM_OE		0xD0

Actually I liked the old prefix better. E.g. "REG_POLARITY" looks more generic that it actually is.

drivers/pwm/pwm-sophgo-sg2042.c line 40

Quoted 2 lines
drivers/pwm/pwm-sophgo-sg2042.c +2 -0
+
+#define PWM_REG_NUM		0x80

This is unused?

drivers/pwm/pwm-sophgo-sg2042.c line 44

Quoted 4 lines
drivers/pwm/pwm-sophgo-sg2042.c +4 -0
+
+#define PWM_POLARITY_MASK(n) BIT(n)
+#define PWM_HLPERIOD(chan) ((chan) * REG_GROUP + REG_HLPERIOD)
+#define PWM_PERIOD(chan) ((chan) * REG_GROUP + REG_PERIOD)

((chan) * 8 + 0) is IMHO better. I guess this is subjective because at least the *8 is repeated several times, but the advantage of not using a define for 8 (and 0 and 4) is that by looking at

	#define SG2042_PWM_HLPERIOD(chan) ((chan) * 8 + 0)

you immediatly see the offsets of the HLPERIOD register, while for

	#define PWM_HLPERIOD(chan) ((chan) * REG_GROUP + REG_HLPERIOD)

you have to lookup two additional symbols.

Also PWM is a prefix that is too generic.

drivers/pwm/pwm-sophgo-sg2042.c line 56

Quoted 7 lines
drivers/pwm/pwm-sophgo-sg2042.c +1 -0
 #define SG2042_PWM_CHANNELNUM	4
 

@@ -51,6 +53,12 @@
 struct sg2042_pwm_ddata {
 	void __iomem *base;
 	unsigned long clk_rate_hz;
+	struct mutex lock;

What does this lock protect? Note that there is a chip lock that is held when .apply() is called, to serialize apply calls for a single chip. I guess this can and should be dropped.

Quoted 9 lines
drivers/pwm/pwm-sophgo-sg2042.c +5 -0
+};
+
+struct sg2042_chip_data {
+	const struct pwm_ops ops;
+	bool atomic;
 };
 
 /*

drivers/pwm/pwm-sophgo-sg2042.c static void pwm_sg2042_config(struct sg2042_pwm_ddata *ddata, unsigned int chan, · line 74

Quoted 7 lines
drivers/pwm/pwm-sophgo-sg2042.c +2 -2
@@ -62,8 +70,8 @@ static void pwm_sg2042_config(struct sg2042_pwm_ddata *ddata, unsigned int chan,
 {
 	void __iomem *base = ddata->base;
 
-	writel(period_ticks, base + SG2042_PWM_PERIOD(chan));
-	writel(hlperiod_ticks, base + SG2042_PWM_HLPERIOD(chan));
+	writel(period_ticks, base + PWM_PERIOD(chan));
+	writel(hlperiod_ticks, base + PWM_HLPERIOD(chan));

The register renaming adds really quite some noise that is actually unrelated to this patch. If you really think the register defines need renaming, do that in a separate patch (and justify it well).

Quoted 52 lines
drivers/pwm/pwm-sophgo-sg2042.c +33 -5
 }
 
 static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm,

@@ -104,8 +112,8 @@ static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	u32 hlperiod_ticks;
 	u32 period_ticks;
 
-	period_ticks = readl(ddata->base + SG2042_PWM_PERIOD(chan));
-	hlperiod_ticks = readl(ddata->base + SG2042_PWM_HLPERIOD(chan));
+	period_ticks = readl(ddata->base + PWM_PERIOD(chan));
+	hlperiod_ticks = readl(ddata->base + PWM_HLPERIOD(chan));
 
 	if (!period_ticks) {
 		state->enabled = false;

@@ -123,13 +131,112 @@ static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
-static const struct pwm_ops pwm_sg2042_ops = {
-	.apply = pwm_sg2042_apply,
-	.get_state = pwm_sg2042_get_state,
+static void pwm_sg2044_config(struct sg2042_pwm_ddata *ddata, struct pwm_device *pwm, bool enabled)
+{
+	u32 pwm_value;
+
+	pwm_value = readl(ddata->base + REG_PWMSTART);
+
+	if (enabled)
+		writel(pwm_value | BIT(pwm->hwpwm), ddata->base + REG_PWMSTART);
+	else
+		writel(pwm_value & ~BIT(pwm->hwpwm), ddata->base + REG_PWMSTART);
+}
+
+static void pwm_sg2044_set_outputenable(struct sg2042_pwm_ddata *ddata, struct pwm_device *pwm,
+					bool enabled)
+{
+	u32 pwm_value;
+
+	pwm_value = readl(ddata->base + REG_PWM_OE);
+
+	if (enabled)
+		writel(pwm_value | BIT(pwm->hwpwm), ddata->base + REG_PWM_OE);
+	else
+		writel(pwm_value & ~BIT(pwm->hwpwm), ddata->base + REG_PWM_OE);
+}
+
+static int pwm_sg2044_set_polarity(struct sg2042_pwm_ddata *ddata, struct pwm_device *pwm,
+				   const struct pwm_state *state)
+{
+	enum pwm_polarity polarity;
+	u32 pwm_value;
+

drivers/pwm/pwm-sophgo-sg2042.c static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm, · line 174

Quoted 10 lines
drivers/pwm/pwm-sophgo-sg2042.c +10 -0
+	pwm_value = readl(ddata->base + REG_POLARITY);
+
+	polarity = state->polarity;
+
+	if (polarity == PWM_POLARITY_NORMAL)
+		pwm_value &= ~BIT(pwm->hwpwm);
+	else
+		pwm_value |= BIT(pwm->hwpwm);
+
+	writel(pwm_value, ddata->base + REG_POLARITY);

I like this idiom better than the one used in pwm_sg2044_set_outputenable() and pwm_sg2044_config(). However drop the local variable polarity.

Quoted 15 lines
drivers/pwm/pwm-sophgo-sg2042.c +15 -0
+	return 0;
+}
+
+static int pwm_sg2044_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_ticks;
+	u32 period_ticks;
+
+	if (!state->enabled) {
+		pwm_sg2044_config(ddata, pwm, false);
+		return 0;
+	}
+

drivers/pwm/pwm-sophgo-sg2042.c static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm, · line 200

Quoted 10 lines
drivers/pwm/pwm-sophgo-sg2042.c +10 -0
+	pwm_sg2044_set_polarity(ddata, pwm, state);
+
+	/*
+	 * Duration of High level (duty_cycle) = HLPERIOD x Period_of_input_clk
+	 * Duration of One Cycle (period) = PERIOD x Period_of_input_clk
+	 */
+	period_ticks = min(mul_u64_u64_div_u64(ddata->clk_rate_hz, state->period,
+					       NSEC_PER_SEC), U32_MAX);
+	hlperiod_ticks = min(mul_u64_u64_div_u64(ddata->clk_rate_hz, state->duty_cycle,
+						 NSEC_PER_SEC), U32_MAX);

This is the same calculation as for sg2042. I think I'd put that in a function that is used by both variants.

drivers/pwm/pwm-sophgo-sg2042.c static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm, · line 203

Quoted 2 lines
drivers/pwm/pwm-sophgo-sg2042.c +2 -0
+	dev_dbg(pwmchip_parent(chip), "chan[%u]: PERIOD=%u, HLPERIOD=%u\n",
+		pwm->hwpwm, period_ticks, hlperiod_ticks);

Now that there are more register values, please add them all to the debug output.

drivers/pwm/pwm-sophgo-sg2042.c static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm, · line 212

Quoted 8 lines
drivers/pwm/pwm-sophgo-sg2042.c +8 -0
+	pwm_sg2042_config(ddata, pwm->hwpwm, period_ticks, hlperiod_ticks);
+
+	guard(mutex)(&ddata->lock);
+
+	/*
+	 * re-enable PWMSTART to refresh the register period
+	 */
+	pwm_sg2044_config(ddata, pwm, false);

pwm_sg2044_config() is conceptually different to pwm_sg2042_config(). This is irritating, so please find a better name.

Quoted 10 lines
drivers/pwm/pwm-sophgo-sg2042.c +10 -0
+	pwm_sg2044_set_outputenable(ddata, pwm, true);
+	pwm_sg2044_config(ddata, pwm, true);
+
+	return 0;
+}
+
+static const struct sg2042_chip_data sg2042_chip_data = {
+	.ops = {
+		.apply = pwm_sg2042_apply,
+		.get_state = pwm_sg2042_get_state,

drivers/pwm/pwm-sophgo-sg2042.c static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm, · line 232

Quoted 10 lines
drivers/pwm/pwm-sophgo-sg2042.c +10 -0
+	},
+	.atomic = true,
+};
+
+static const struct sg2042_chip_data sg2044_chip_data = {
+	.ops = {
+		.apply = pwm_sg2044_apply,
+		.get_state = pwm_sg2042_get_state,
+	},
+	.atomic = false,

If you drop the mutex don't forget to drop this one, too.

drivers/pwm/pwm-sophgo-sg2042.c static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm, · line 230

Quoted 1 line
drivers/pwm/pwm-sophgo-sg2042.c +0 -0
 };

Best regards
Uwe

author
#8 lore raw

Re: [PATCH 2/2] pwm: sophgo: add driver for SG2044

Quoted 5 lines · Uwe Kleine-König
Quoted 1 line · Longbin Li

From: ghost <[email protected]>

Huh, is that a real name?

My fault,i will change it.

Quoted 7 lines
Quoted 7 lines

Add PWM controller for SG2044.

Signed-off-by Longbin Li <[email protected]>
---
 drivers/pwm/pwm-sophgo-sg2042.c | 162 +++++++++++++++++++++++++++-----
 1 file changed, 138 insertions(+), 24 deletions(-)
Quoted 8 lines

drivers/pwm/pwm-sophgo-sg2042.c

Quoted 4 lines
drivers/pwm/pwm-sophgo-sg2042.c +0 -0
index ff4639d849ce..c62e8c758d87 100644

The Limitations paragraph needs updating. E.g. SG2044 seems to support polarity while SG2042 doesn't.

Thanks,i will add that.

Quoted 16 lines
Quoted 16 lines
drivers/pwm/pwm-sophgo-sg2042.c +0 -12
@@ -26,20 +26,22 @@
 #include <linux/pwm.h>
 #include <linux/reset.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
- */
Quoted 9 lines

drivers/pwm/pwm-sophgo-sg2042.c line 31

Quoted 5 lines
drivers/pwm/pwm-sophgo-sg2042.c +3 -2
-#define SG2042_PWM_HLPERIOD(chan) ((chan) * 8 + 0)
-#define SG2042_PWM_PERIOD(chan) ((chan) * 8 + 4)
+#define REG_HLPERIOD		0x0
+#define REG_PERIOD		0x4
+#define REG_GROUP		0x8

REG_GROUP belongs to a different category than REG_PERIOD. So please use a different schema to name it (or drop it, see below).

I will improve it.

Quoted 9 lines

drivers/pwm/pwm-sophgo-sg2042.c line 37

Quoted 6 lines
drivers/pwm/pwm-sophgo-sg2042.c +6 -0
+#define REG_POLARITY		0x40
+
+#define REG_PWMSTART		0x44
+#define REG_PWMUPDATE		0x4C
+#define REG_SHIFTCOUNT		0x80
+#define REG_SHIFTSTART		0x90

REG_SHIFTCOUNT and REG_SHIFTSTART are unused.

I will drop it.

Quoted 5 lines

drivers/pwm/pwm-sophgo-sg2042.c line 38

Quoted 1 line
drivers/pwm/pwm-sophgo-sg2042.c +1 -0
+#define REG_PWM_OE		0xD0

Actually I liked the old prefix better. E.g. "REG_POLARITY" looks more generic that it actually is.

I will reconsider that.

Quoted 5 lines

drivers/pwm/pwm-sophgo-sg2042.c line 40

Quoted 2 lines
drivers/pwm/pwm-sophgo-sg2042.c +2 -0
+
+#define PWM_REG_NUM		0x80

This is unused?

I will drop it,thanks!

Quoted 11 lines

drivers/pwm/pwm-sophgo-sg2042.c line 44

Quoted 4 lines
drivers/pwm/pwm-sophgo-sg2042.c +4 -0
+
+#define PWM_POLARITY_MASK(n) BIT(n)
+#define PWM_HLPERIOD(chan) ((chan) * REG_GROUP + REG_HLPERIOD)
+#define PWM_PERIOD(chan) ((chan) * REG_GROUP + REG_PERIOD)

((chan) * 8 + 0) is IMHO better. I guess this is subjective because at least the *8 is repeated several times, but the advantage of not using a define for 8 (and 0 and 4) is that by looking at

	#define SG2042_PWM_HLPERIOD(chan) ((chan) * 8 + 0)
Quoted 8 lines

you immediatly see the offsets of the HLPERIOD register, while for

	#define PWM_HLPERIOD(chan) ((chan) * REG_GROUP + REG_HLPERIOD)

you have to lookup two additional symbols.

Also PWM is a prefix that is too generic.

I will reconsider that.

Quoted 12 lines

drivers/pwm/pwm-sophgo-sg2042.c line 56

Quoted 7 lines
drivers/pwm/pwm-sophgo-sg2042.c +1 -0
 #define SG2042_PWM_CHANNELNUM	4
 

@@ -51,6 +53,12 @@
 struct sg2042_pwm_ddata {
 	void __iomem *base;
 	unsigned long clk_rate_hz;
+	struct mutex lock;

What does this lock protect? Note that there is a chip lock that is held when .apply() is called, to serialize apply calls for a single chip. I guess this can and should be dropped.

I will retest it, and if possible i will drop it, thanks.

Quoted 12 lines
Quoted 12 lines
drivers/pwm/pwm-sophgo-sg2042.c +5 -0
+};
+
+struct sg2042_chip_data {
+	const struct pwm_ops ops;
+	bool atomic;
 };
 
 /*

@@ -62,8 +70,8 @@ static void pwm_sg2042_config(struct sg2042_pwm_ddata *ddata, unsigned int chan,
 {
 	void __iomem *base = ddata->base;
Quoted 9 lines

drivers/pwm/pwm-sophgo-sg2042.c static void pwm_sg2042_config(struct sg2042_pwm_ddata *ddata, unsigned int chan, · line 74

Quoted 4 lines
drivers/pwm/pwm-sophgo-sg2042.c +2 -2
-	writel(period_ticks, base + SG2042_PWM_PERIOD(chan));
-	writel(hlperiod_ticks, base + SG2042_PWM_HLPERIOD(chan));
+	writel(period_ticks, base + PWM_PERIOD(chan));
+	writel(hlperiod_ticks, base + PWM_HLPERIOD(chan));

The register renaming adds really quite some noise that is actually unrelated to this patch. If you really think the register defines need renaming, do that in a separate patch (and justify it well).

Sorry, i will modify here.

Quoted 58 lines
Quoted 58 lines
drivers/pwm/pwm-sophgo-sg2042.c +39 -5
 }
 
 static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm,

@@ -104,8 +112,8 @@ static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	u32 hlperiod_ticks;
 	u32 period_ticks;
 
-	period_ticks = readl(ddata->base + SG2042_PWM_PERIOD(chan));
-	hlperiod_ticks = readl(ddata->base + SG2042_PWM_HLPERIOD(chan));
+	period_ticks = readl(ddata->base + PWM_PERIOD(chan));
+	hlperiod_ticks = readl(ddata->base + PWM_HLPERIOD(chan));
 
 	if (!period_ticks) {
 		state->enabled = false;

@@ -123,13 +131,112 @@ static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
-static const struct pwm_ops pwm_sg2042_ops = {
-	.apply = pwm_sg2042_apply,
-	.get_state = pwm_sg2042_get_state,
+static void pwm_sg2044_config(struct sg2042_pwm_ddata *ddata, struct pwm_device *pwm, bool enabled)
+{
+	u32 pwm_value;
+
+	pwm_value = readl(ddata->base + REG_PWMSTART);
+
+	if (enabled)
+		writel(pwm_value | BIT(pwm->hwpwm), ddata->base + REG_PWMSTART);
+	else
+		writel(pwm_value & ~BIT(pwm->hwpwm), ddata->base + REG_PWMSTART);
+}
+
+static void pwm_sg2044_set_outputenable(struct sg2042_pwm_ddata *ddata, struct pwm_device *pwm,
+					bool enabled)
+{
+	u32 pwm_value;
+
+	pwm_value = readl(ddata->base + REG_PWM_OE);
+
+	if (enabled)
+		writel(pwm_value | BIT(pwm->hwpwm), ddata->base + REG_PWM_OE);
+	else
+		writel(pwm_value & ~BIT(pwm->hwpwm), ddata->base + REG_PWM_OE);
+}
+
+static int pwm_sg2044_set_polarity(struct sg2042_pwm_ddata *ddata, struct pwm_device *pwm,
+				   const struct pwm_state *state)
+{
+	enum pwm_polarity polarity;
+	u32 pwm_value;
+
+	pwm_value = readl(ddata->base + REG_POLARITY);
+
+	polarity = state->polarity;
+
+	if (polarity == PWM_POLARITY_NORMAL)
+		pwm_value &= ~BIT(pwm->hwpwm);
Quoted 9 lines

drivers/pwm/pwm-sophgo-sg2042.c static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm, · line 174

Quoted 4 lines
drivers/pwm/pwm-sophgo-sg2042.c +4 -0
+	else
+		pwm_value |= BIT(pwm->hwpwm);
+
+	writel(pwm_value, ddata->base + REG_POLARITY);

I like this idiom better than the one used in pwm_sg2044_set_outputenable() and pwm_sg2044_config(). However drop the local variable polarity.

I will modify that, thanks.

Quoted 20 lines
Quoted 20 lines
drivers/pwm/pwm-sophgo-sg2042.c +20 -0
+	return 0;
+}
+
+static int pwm_sg2044_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_ticks;
+	u32 period_ticks;
+
+	if (!state->enabled) {
+		pwm_sg2044_config(ddata, pwm, false);
+		return 0;
+	}
+
+	pwm_sg2044_set_polarity(ddata, pwm, state);
+
+	/*
+	 * Duration of High level (duty_cycle) = HLPERIOD x Period_of_input_clk
+	 * Duration of One Cycle (period) = PERIOD x Period_of_input_clk
Quoted 9 lines

drivers/pwm/pwm-sophgo-sg2042.c static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm, · line 200

Quoted 5 lines
drivers/pwm/pwm-sophgo-sg2042.c +5 -0
+	 */
+	period_ticks = min(mul_u64_u64_div_u64(ddata->clk_rate_hz, state->period,
+					       NSEC_PER_SEC), U32_MAX);
+	hlperiod_ticks = min(mul_u64_u64_div_u64(ddata->clk_rate_hz, state->duty_cycle,
+						 NSEC_PER_SEC), U32_MAX);

This is the same calculation as for sg2042. I think I'd put that in a function that is used by both variants.

Thanks, i will modify that.

Quoted 6 lines

drivers/pwm/pwm-sophgo-sg2042.c static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm, · line 203

Quoted 2 lines
drivers/pwm/pwm-sophgo-sg2042.c +2 -0
+	dev_dbg(pwmchip_parent(chip), "chan[%u]: PERIOD=%u, HLPERIOD=%u\n",
+		pwm->hwpwm, period_ticks, hlperiod_ticks);

Now that there are more register values, please add them all to the debug output.

I will, thanks.

Quoted 12 lines

drivers/pwm/pwm-sophgo-sg2042.c static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm, · line 212

Quoted 8 lines
drivers/pwm/pwm-sophgo-sg2042.c +8 -0
+	pwm_sg2042_config(ddata, pwm->hwpwm, period_ticks, hlperiod_ticks);
+
+	guard(mutex)(&ddata->lock);
+
+	/*
+	 * re-enable PWMSTART to refresh the register period
+	 */
+	pwm_sg2044_config(ddata, pwm, false);

pwm_sg2044_config() is conceptually different to pwm_sg2042_config(). This is irritating, so please find a better name.

I will modify that, thanks.

Quoted 14 lines
Quoted 14 lines
drivers/pwm/pwm-sophgo-sg2042.c +14 -0
+	pwm_sg2044_set_outputenable(ddata, pwm, true);
+	pwm_sg2044_config(ddata, pwm, true);
+
+	return 0;
+}
+
+static const struct sg2042_chip_data sg2042_chip_data = {
+	.ops = {
+		.apply = pwm_sg2042_apply,
+		.get_state = pwm_sg2042_get_state,
+	},
+	.atomic = true,
+};
+
Quoted 9 lines

drivers/pwm/pwm-sophgo-sg2042.c static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm, · line 232

Quoted 6 lines
drivers/pwm/pwm-sophgo-sg2042.c +6 -0
+static const struct sg2042_chip_data sg2044_chip_data = {
+	.ops = {
+		.apply = pwm_sg2044_apply,
+		.get_state = pwm_sg2042_get_state,
+	},
+	.atomic = false,

If you drop the mutex don't forget to drop this one, too.

I will, thanks for your reminder.

Quoted 4 lines

drivers/pwm/pwm-sophgo-sg2042.c static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm, · line 230

Quoted 1 line
drivers/pwm/pwm-sophgo-sg2042.c +0 -0
 };

Best regards
Uwe

Best regards
Longbin Li

build bot
#9 lore raw

Re: [PATCH 2/2] pwm: sophgo: add driver for SG2044

In reply to Longbin Li
kernel test robot noticed the following build warnings:

Hi Longbin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on krzk-dt/for-next]
[also build test WARNING on linus/master v6.15-rc1 next-20250407] [cannot apply to robh/for-next sophgo/for-next sophgo/fixes] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Longbin-Li/dt-bindings-pwm-sophgo-add-pwm-controller-for-SG2044/20250407-153334
base:   https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux-dt.git for-next
patch link:    https://lore.kernel.org/r/20250407072056.8629-3-looong.bin%40gmail.com
patch subject: [PATCH 2/2] pwm: sophgo: add driver for SG2044
config: arm-randconfig-003-20250407 (https://download.01.org/0day-ci/archive/20250407/[email protected]/config)
compiler: arm-linux-gnueabi-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250407/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

>> drivers/pwm/pwm-sophgo-sg2042.c:57: warning: Function parameter or struct member 'lock' not described in 'sg2042_pwm_ddata'

vim +57 drivers/pwm/pwm-sophgo-sg2042.c

    47	
    48	/**
    49	 * struct sg2042_pwm_ddata - private driver data
    50	 * @base:		base address of mapped PWM registers
    51	 * @clk_rate_hz:	rate of base clock in HZ
    52	 */
    53	struct sg2042_pwm_ddata {
    54		void __iomem *base;
    55		unsigned long clk_rate_hz;
    56		struct mutex lock;
  > 57	};
    58
Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki