riscv: pwm: sophgo: add pwm support for SG2044
Subsystems (4)
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
PATCH 1/2 dt-bindings: pwm: sophgo: add pwm controller for SG2044
From: ghost <[email protected]>
Add compatible string for PWM controller on SG2044.
--- 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
@@ -17,7 +17,9 @@ allOf: properties: compatible: - const: sophgo,sg2042-pwm + enum: + - sophgo,sg2042-pwm + - sophgo,sg2044-pwm reg: maxItems: 1
--
2.48.1
PATCH 2/2 pwm: sophgo: add driver for SG2044
In reply to Longbin LiFrom: ghost <[email protected]>
Add PWM controller for SG2044.
--- drivers/pwm/pwm-sophgo-sg2042.c | 162 +++++++++++++++++++++++++++----- 1 file changed, 138 insertions(+), 24 deletions(-)
drivers/pwm/pwm-sophgo-sg2042.c +138 -24
@@ -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
Uwe Kleine-König
maintainerRe: [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
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
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
-0The 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_GROUPbelongs to a different category thanREG_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_SHIFTCOUNTandREG_SHIFTSTARTare 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 0xD0Actually 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 0x80This 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()andpwm_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 topwm_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
Uwe Kleine-König
maintainerRe: [PATCH 1/2] dt-bindings: pwm: sophgo: add pwm controller for SG2044
In reply to Longbin LiHello,
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@@ -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
Rob Herring
maintainerRe: [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@@ -17,7 +17,9 @@ allOf: properties: compatible: - const: sophgo,sg2042-pwm + enum: + - sophgo,sg2042-pwm + - sophgo,sg2044-pwmGiven 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
kernel test robot
build botRe: [PATCH 2/2] pwm: sophgo: add driver for SG2044
In reply to Longbin Likernel 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
Re: [PATCH 2/2] pwm: sophgo: add driver for SG2044
In reply to Longbin LiQuoted 5 lines · Longbin Li
From: ghost <[email protected]>
Add PWM controller for SG2044.
Signed-off-by Longbin Li <[email protected]>
Consider updating "MODULE_AUTHOR" (add your name) and "MODULE_DESCRIPTION"(add SG2044).
Thanks,
Chen
[......]
Inochi Amaoto
maintainerRe: [PATCH 1/2] dt-bindings: pwm: sophgo: add pwm controller for SG2044
In reply to Rob HerringQuoted 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@@ -17,7 +17,9 @@ allOf: properties: compatible: - const: sophgo,sg2042-pwm + enum: + - sophgo,sg2042-pwm + - sophgo,sg2044-pwmGiven 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
Re: [PATCH 1/2] dt-bindings: pwm: sophgo: add pwm controller for SG2044
In reply to Longbin LiQuoted 5 lines · Longbin Li
From: ghost <[email protected]>
Add compatible string for PWM controller on SG2044.
Signed-off-by Longbin Li <[email protected]>
Messed SoB and From.
Use known identities, not ghosts.
Best regards,
Krzysztof