| Message ID | 20250428013501.6354-3-looong.bin@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | riscv: pwm: sophgo: add pwm support for SG2044 | expand |
Hello, On Mon, Apr 28, 2025 at 09:34:49AM +0800, Longbin Li wrote: > As the driver logic can be used in both SG2042 and SG2044, it > will be better to reorganize the code structure. > > Signed-off-by: Longbin Li <looong.bin@gmail.com> > Reviewed-by: Chen Wang <unicorn_wang@outlook.com> > --- > drivers/pwm/pwm-sophgo-sg2042.c | 62 +++++++++++++++++++-------------- > 1 file changed, 35 insertions(+), 27 deletions(-) > > diff --git a/drivers/pwm/pwm-sophgo-sg2042.c b/drivers/pwm/pwm-sophgo-sg2042.c > index ff4639d849ce..23a83843ba53 100644 > --- a/drivers/pwm/pwm-sophgo-sg2042.c > +++ b/drivers/pwm/pwm-sophgo-sg2042.c > @@ -26,18 +26,6 @@ > #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 > - */ This seems to be still correct? Why remove it then? > #define SG2042_PWM_HLPERIOD(chan) ((chan) * 8 + 0) > #define SG2042_PWM_PERIOD(chan) ((chan) * 8 + 4) > > @@ -53,6 +41,10 @@ struct sg2042_pwm_ddata { > unsigned long clk_rate_hz; > }; > > +struct sg2042_chip_data { > + const struct pwm_ops ops; > +}; > + > /* > * period_ticks: PERIOD > * hlperiod_ticks: HLPERIOD > @@ -66,21 +58,13 @@ static void pwm_sg2042_config(struct sg2042_pwm_ddata *ddata, unsigned int chan, > writel(hlperiod_ticks, base + SG2042_PWM_HLPERIOD(chan)); > } > > -static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm, > - const struct pwm_state *state) > +static void pwm_set_dutycycle(struct pwm_chip *chip, struct pwm_device *pwm, This is not a global pwm API function, so please stick to the pwm_sg2042 prefix. > + const struct pwm_state *state) > { > struct sg2042_pwm_ddata *ddata = pwmchip_get_drvdata(chip); > u32 hlperiod_ticks; > u32 period_ticks; > > - if (state->polarity == PWM_POLARITY_INVERSED) > - return -EINVAL; > - > - if (!state->enabled) { > - pwm_sg2042_config(ddata, pwm->hwpwm, 0, 0); > - return 0; > - } > - > /* > * Duration of High level (duty_cycle) = HLPERIOD x Period_of_input_clk > * Duration of One Cycle (period) = PERIOD x Period_of_input_clk > [...] > @@ -123,13 +123,16 @@ 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 const struct sg2042_chip_data sg2042_chip_data = { > + .ops = { > + .apply = pwm_sg2042_apply, > + .get_state = pwm_sg2042_get_state, > + } > }; > > static const struct of_device_id sg2042_pwm_ids[] = { > - { .compatible = "sophgo,sg2042-pwm" }, > + { .compatible = "sophgo,sg2042-pwm", > + .data = &sg2042_chip_data }, I would have expected that checkpatch doesn't like that. At least I don't. Please make this { .compatible = ...; .data = ...; }, > { } > }; > MODULE_DEVICE_TABLE(of, sg2042_pwm_ids);
On Mon, May 26, 2025 at 04:59:34PM +0200, Uwe Kleine-König wrote: > Hello, > > On Mon, Apr 28, 2025 at 09:34:49AM +0800, Longbin Li wrote: > > As the driver logic can be used in both SG2042 and SG2044, it > > will be better to reorganize the code structure. > > > > Signed-off-by: Longbin Li <looong.bin@gmail.com> > > Reviewed-by: Chen Wang <unicorn_wang@outlook.com> > > --- > > drivers/pwm/pwm-sophgo-sg2042.c | 62 +++++++++++++++++++-------------- > > 1 file changed, 35 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/pwm/pwm-sophgo-sg2042.c b/drivers/pwm/pwm-sophgo-sg2042.c > > index ff4639d849ce..23a83843ba53 100644 > > --- a/drivers/pwm/pwm-sophgo-sg2042.c > > +++ b/drivers/pwm/pwm-sophgo-sg2042.c > > @@ -26,18 +26,6 @@ > > #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 > > - */ > > This seems to be still correct? Why remove it then? > The following "#define" has already expressd it. For clarity, i will add it back. > > #define SG2042_PWM_HLPERIOD(chan) ((chan) * 8 + 0) > > #define SG2042_PWM_PERIOD(chan) ((chan) * 8 + 4) > > > > @@ -53,6 +41,10 @@ struct sg2042_pwm_ddata { > > unsigned long clk_rate_hz; > > }; > > > > +struct sg2042_chip_data { > > + const struct pwm_ops ops; > > +}; > > + > > /* > > * period_ticks: PERIOD > > * hlperiod_ticks: HLPERIOD > > @@ -66,21 +58,13 @@ static void pwm_sg2042_config(struct sg2042_pwm_ddata *ddata, unsigned int chan, > > writel(hlperiod_ticks, base + SG2042_PWM_HLPERIOD(chan)); > > } > > > > -static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > - const struct pwm_state *state) > > +static void pwm_set_dutycycle(struct pwm_chip *chip, struct pwm_device *pwm, > > This is not a global pwm API function, so please stick to the pwm_sg2042 > prefix. > I will modify that, thanks. > > + const struct pwm_state *state) > > { > > struct sg2042_pwm_ddata *ddata = pwmchip_get_drvdata(chip); > > u32 hlperiod_ticks; > > u32 period_ticks; > > > > - if (state->polarity == PWM_POLARITY_INVERSED) > > - return -EINVAL; > > - > > - if (!state->enabled) { > > - pwm_sg2042_config(ddata, pwm->hwpwm, 0, 0); > > - return 0; > > - } > > - > > /* > > * Duration of High level (duty_cycle) = HLPERIOD x Period_of_input_clk > > * Duration of One Cycle (period) = PERIOD x Period_of_input_clk > > [...] > > @@ -123,13 +123,16 @@ 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 const struct sg2042_chip_data sg2042_chip_data = { > > + .ops = { > > + .apply = pwm_sg2042_apply, > > + .get_state = pwm_sg2042_get_state, > > + } > > }; > > > > static const struct of_device_id sg2042_pwm_ids[] = { > > - { .compatible = "sophgo,sg2042-pwm" }, > > + { .compatible = "sophgo,sg2042-pwm", > > + .data = &sg2042_chip_data }, > > I would have expected that checkpatch doesn't like that. At least I > don't. Please make this > > { > .compatible = ...; > .data = ...; > }, > I will check that, thanks. > > { } > > }; > > MODULE_DEVICE_TABLE(of, sg2042_pwm_ids);
diff --git a/drivers/pwm/pwm-sophgo-sg2042.c b/drivers/pwm/pwm-sophgo-sg2042.c index ff4639d849ce..23a83843ba53 100644 --- a/drivers/pwm/pwm-sophgo-sg2042.c +++ b/drivers/pwm/pwm-sophgo-sg2042.c @@ -26,18 +26,6 @@ #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) @@ -53,6 +41,10 @@ struct sg2042_pwm_ddata { unsigned long clk_rate_hz; }; +struct sg2042_chip_data { + const struct pwm_ops ops; +}; + /* * period_ticks: PERIOD * hlperiod_ticks: HLPERIOD @@ -66,21 +58,13 @@ static void pwm_sg2042_config(struct sg2042_pwm_ddata *ddata, unsigned int chan, writel(hlperiod_ticks, base + SG2042_PWM_HLPERIOD(chan)); } -static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm, - const struct pwm_state *state) +static void pwm_set_dutycycle(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->polarity == PWM_POLARITY_INVERSED) - return -EINVAL; - - if (!state->enabled) { - pwm_sg2042_config(ddata, pwm->hwpwm, 0, 0); - return 0; - } - /* * Duration of High level (duty_cycle) = HLPERIOD x Period_of_input_clk * Duration of One Cycle (period) = PERIOD x Period_of_input_clk @@ -92,6 +76,22 @@ static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm, pwm->hwpwm, period_ticks, hlperiod_ticks); pwm_sg2042_config(ddata, pwm->hwpwm, period_ticks, hlperiod_ticks); +} + +static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct sg2042_pwm_ddata *ddata = pwmchip_get_drvdata(chip); + + if (state->polarity == PWM_POLARITY_INVERSED) + return -EINVAL; + + if (!state->enabled) { + pwm_sg2042_config(ddata, pwm->hwpwm, 0, 0); + return 0; + } + + pwm_set_dutycycle(chip, pwm, state); return 0; } @@ -123,13 +123,16 @@ 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 const struct sg2042_chip_data sg2042_chip_data = { + .ops = { + .apply = pwm_sg2042_apply, + .get_state = pwm_sg2042_get_state, + } }; static const struct of_device_id sg2042_pwm_ids[] = { - { .compatible = "sophgo,sg2042-pwm" }, + { .compatible = "sophgo,sg2042-pwm", + .data = &sg2042_chip_data }, { } }; MODULE_DEVICE_TABLE(of, sg2042_pwm_ids); @@ -137,12 +140,17 @@ 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); @@ -170,7 +178,7 @@ 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->ops = &chip_data->ops; chip->atomic = true; ret = devm_pwmchip_add(dev, chip);