| Message ID | 20190809123031.24219-2-paul@crapouillou.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm: jz4740: Driver update | expand |
On Fri, Aug 09, 2019 at 02:30:25PM +0200, Paul Cercueil wrote: > The TCU registers are shared between a handful of drivers, accessing > them through the same regmap. > > While this driver is devicetree-compatible, it is never (as of now) > probed from devicetree, so this change does not introduce a ABI problem > with current devicetree files. If you adapt the binding also update the binding doc please. > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > Tested-by: Mathieu Malaterre <malat@debian.org> > Tested-by: Artur Rojek <contact@artur-rojek.eu> nitpick: put your S-o-b line after the other tags you added. > --- > drivers/pwm/Kconfig | 1 + > drivers/pwm/pwm-jz4740.c | 80 ++++++++++++++++++++++++++-------------- > 2 files changed, 53 insertions(+), 28 deletions(-) > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index a7e57516959e..cc4df0ec978a 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -225,6 +225,7 @@ config PWM_IMX_TPM > config PWM_JZ4740 > tristate "Ingenic JZ47xx PWM support" > depends on MACH_INGENIC > + select MFD_SYSCON > help > Generic PWM framework driver for Ingenic JZ47xx based > machines. > diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c > index f901e8a0d33d..7aea5e0c6e18 100644 > --- a/drivers/pwm/pwm-jz4740.c > +++ b/drivers/pwm/pwm-jz4740.c > @@ -8,18 +8,20 @@ > #include <linux/err.h> > #include <linux/gpio.h> > #include <linux/kernel.h> > +#include <linux/mfd/ingenic-tcu.h> > +#include <linux/mfd/syscon.h> > #include <linux/module.h> > #include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/pwm.h> > - > -#include <asm/mach-jz4740/timer.h> > +#include <linux/regmap.h> > > #define NUM_PWM 8 > > struct jz4740_pwm_chip { > struct pwm_chip chip; > struct clk *clk; > + struct regmap *map; > }; > > static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip) > @@ -29,6 +31,8 @@ static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip) > > static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > { > + struct jz4740_pwm_chip *jz = to_jz4740(chip); > + > /* > * Timers 0 and 1 are used for system tasks, so they are unavailable > * for use as PWMs. > @@ -36,50 +40,53 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > if (pwm->hwpwm < 2) > return -EBUSY; > > - jz4740_timer_start(pwm->hwpwm); > + regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm)); jz4740_timer_start does writel(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_STOP_CLEAR); with #define JZ_REG_TIMER_STOP_CLEAR 0x2C and #define TCU_REG_TSCR 0x3c I wonder why the offsets are different. > > return 0; > } > > static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) > { > - jz4740_timer_set_ctrl(pwm->hwpwm, 0); > + struct jz4740_pwm_chip *jz = to_jz4740(chip); > > - jz4740_timer_stop(pwm->hwpwm); > + regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm)); jz4740_timer_set_ctrl writes to offset (((pwm->hwpwm) * 0x10) + 0x3C) and jz4740_timer_stop to offset 0x1c. The regmap_write doesn't do both of them but instead writes to offset 0x3c. So this doesn't seem to be a 1:1 conversion. This either needs fixing, splitting into several patches or a better commit log. Stopping my review here. Best regards Uwe
Hi Uwe, Le ven. 9 août 2019 à 18:51, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= <u.kleine-koenig@pengutronix.de> a écrit : > On Fri, Aug 09, 2019 at 02:30:25PM +0200, Paul Cercueil wrote: >> The TCU registers are shared between a handful of drivers, accessing >> them through the same regmap. >> >> While this driver is devicetree-compatible, it is never (as of now) >> probed from devicetree, so this change does not introduce a ABI >> problem >> with current devicetree files. > > If you adapt the binding also update the binding doc please. It's already updated, in mips-next, so it'll be in the next -rc1. >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> Tested-by: Mathieu Malaterre <malat@debian.org> >> Tested-by: Artur Rojek <contact@artur-rojek.eu> > > nitpick: put your S-o-b line after the other tags you added. > >> --- >> drivers/pwm/Kconfig | 1 + >> drivers/pwm/pwm-jz4740.c | 80 >> ++++++++++++++++++++++++++-------------- >> 2 files changed, 53 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >> index a7e57516959e..cc4df0ec978a 100644 >> --- a/drivers/pwm/Kconfig >> +++ b/drivers/pwm/Kconfig >> @@ -225,6 +225,7 @@ config PWM_IMX_TPM >> config PWM_JZ4740 >> tristate "Ingenic JZ47xx PWM support" >> depends on MACH_INGENIC >> + select MFD_SYSCON >> help >> Generic PWM framework driver for Ingenic JZ47xx based >> machines. >> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c >> index f901e8a0d33d..7aea5e0c6e18 100644 >> --- a/drivers/pwm/pwm-jz4740.c >> +++ b/drivers/pwm/pwm-jz4740.c >> @@ -8,18 +8,20 @@ >> #include <linux/err.h> >> #include <linux/gpio.h> >> #include <linux/kernel.h> >> +#include <linux/mfd/ingenic-tcu.h> >> +#include <linux/mfd/syscon.h> >> #include <linux/module.h> >> #include <linux/of_device.h> >> #include <linux/platform_device.h> >> #include <linux/pwm.h> >> - >> -#include <asm/mach-jz4740/timer.h> >> +#include <linux/regmap.h> >> >> #define NUM_PWM 8 >> >> struct jz4740_pwm_chip { >> struct pwm_chip chip; >> struct clk *clk; >> + struct regmap *map; >> }; >> >> static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip >> *chip) >> @@ -29,6 +31,8 @@ static inline struct jz4740_pwm_chip >> *to_jz4740(struct pwm_chip *chip) >> >> static int jz4740_pwm_request(struct pwm_chip *chip, struct >> pwm_device *pwm) >> { >> + struct jz4740_pwm_chip *jz = to_jz4740(chip); >> + >> /* >> * Timers 0 and 1 are used for system tasks, so they are >> unavailable >> * for use as PWMs. >> @@ -36,50 +40,53 @@ static int jz4740_pwm_request(struct pwm_chip >> *chip, struct pwm_device *pwm) >> if (pwm->hwpwm < 2) >> return -EBUSY; >> >> - jz4740_timer_start(pwm->hwpwm); >> + regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm)); > > jz4740_timer_start does > > writel(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_STOP_CLEAR); > > with > > #define JZ_REG_TIMER_STOP_CLEAR 0x2C > > and > > #define TCU_REG_TSCR 0x3c > > I wonder why the offsets are different. The offset are different because the base is different. >> >> return 0; >> } >> >> static void jz4740_pwm_free(struct pwm_chip *chip, struct >> pwm_device *pwm) >> { >> - jz4740_timer_set_ctrl(pwm->hwpwm, 0); >> + struct jz4740_pwm_chip *jz = to_jz4740(chip); >> >> - jz4740_timer_stop(pwm->hwpwm); >> + regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm)); > > jz4740_timer_set_ctrl writes to offset (((pwm->hwpwm) * 0x10) + 0x3C) > and jz4740_timer_stop to offset 0x1c. The regmap_write doesn't do both > of them but instead writes to offset 0x3c. I guess it should have been TCU_REG_TSSR ("Timer Stop Set Register") and I didn't notice because the next patch drops this write anyway. I'll do as you suggested in your other reply and swap the two patches if it makes things easier, it'll get rid of this write. > So this doesn't seem to be a 1:1 conversion. This either needs fixing, > splitting into several patches or a better commit log. > > Stopping my review here. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König > | > Industrial Linux Solutions | > http://www.pengutronix.de/ |
Hello Paul, On Fri, Aug 09, 2019 at 07:04:56PM +0200, Paul Cercueil wrote: > Le ven. 9 août 2019 à 18:51, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= > <u.kleine-koenig@pengutronix.de> a écrit : > > On Fri, Aug 09, 2019 at 02:30:25PM +0200, Paul Cercueil wrote: > > > The TCU registers are shared between a handful of drivers, accessing > > > them through the same regmap. > > > > > > While this driver is devicetree-compatible, it is never (as of now) > > > probed from devicetree, so this change does not introduce a ABI > > > problem > > > with current devicetree files. > > > > If you adapt the binding also update the binding doc please. > > It's already updated, in mips-next, so it'll be in the next -rc1. This is a useful information for the commit log. > > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > > > Tested-by: Mathieu Malaterre <malat@debian.org> > > > Tested-by: Artur Rojek <contact@artur-rojek.eu> > > > > nitpick: put your S-o-b line after the other tags you added. > > > > > --- > > > drivers/pwm/Kconfig | 1 + > > > drivers/pwm/pwm-jz4740.c | 80 ++++++++++++++++++++++++++-------------- > > > 2 files changed, 53 insertions(+), 28 deletions(-) > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > index a7e57516959e..cc4df0ec978a 100644 > > > --- a/drivers/pwm/Kconfig > > > +++ b/drivers/pwm/Kconfig > > > @@ -225,6 +225,7 @@ config PWM_IMX_TPM > > > config PWM_JZ4740 > > > tristate "Ingenic JZ47xx PWM support" > > > depends on MACH_INGENIC > > > + select MFD_SYSCON > > > help > > > Generic PWM framework driver for Ingenic JZ47xx based > > > machines. > > > diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c > > > index f901e8a0d33d..7aea5e0c6e18 100644 > > > --- a/drivers/pwm/pwm-jz4740.c > > > +++ b/drivers/pwm/pwm-jz4740.c > > > @@ -8,18 +8,20 @@ > > > #include <linux/err.h> > > > #include <linux/gpio.h> > > > #include <linux/kernel.h> > > > +#include <linux/mfd/ingenic-tcu.h> > > > +#include <linux/mfd/syscon.h> > > > #include <linux/module.h> > > > #include <linux/of_device.h> > > > #include <linux/platform_device.h> > > > #include <linux/pwm.h> > > > - > > > -#include <asm/mach-jz4740/timer.h> > > > +#include <linux/regmap.h> > > > > > > #define NUM_PWM 8 > > > > > > struct jz4740_pwm_chip { > > > struct pwm_chip chip; > > > struct clk *clk; > > > + struct regmap *map; > > > }; > > > > > > static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip) > > > @@ -29,6 +31,8 @@ static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip) > > > > > > static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > > > { > > > + struct jz4740_pwm_chip *jz = to_jz4740(chip); > > > + > > > /* > > > * Timers 0 and 1 are used for system tasks, so they are unavailable > > > * for use as PWMs. > > > @@ -36,50 +40,53 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > > > if (pwm->hwpwm < 2) > > > return -EBUSY; > > > > > > - jz4740_timer_start(pwm->hwpwm); > > > + regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm)); > > > > jz4740_timer_start does > > > > writel(BIT(timer), jz4740_timer_base + JZ_REG_TIMER_STOP_CLEAR); > > > > with > > > > #define JZ_REG_TIMER_STOP_CLEAR 0x2C > > > > and > > > > #define TCU_REG_TSCR 0x3c > > > > I wonder why the offsets are different. > > The offset are different because the base is different. This is a useful information for the commit log. > > > > > > return 0; > > > } > > > > > > static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) > > > { > > > - jz4740_timer_set_ctrl(pwm->hwpwm, 0); > > > + struct jz4740_pwm_chip *jz = to_jz4740(chip); > > > > > > - jz4740_timer_stop(pwm->hwpwm); > > > + regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm)); > > > > jz4740_timer_set_ctrl writes to offset (((pwm->hwpwm) * 0x10) + 0x3C) > > and jz4740_timer_stop to offset 0x1c. The regmap_write doesn't do both > > of them but instead writes to offset 0x3c. > > I guess it should have been TCU_REG_TSSR ("Timer Stop Set Register") and > I didn't notice because the next patch drops this write anyway. > > I'll do as you suggested in your other reply and swap the two patches if > it makes things easier, it'll get rid of this write. ok. Best regards Uwe
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index a7e57516959e..cc4df0ec978a 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -225,6 +225,7 @@ config PWM_IMX_TPM config PWM_JZ4740 tristate "Ingenic JZ47xx PWM support" depends on MACH_INGENIC + select MFD_SYSCON help Generic PWM framework driver for Ingenic JZ47xx based machines. diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c index f901e8a0d33d..7aea5e0c6e18 100644 --- a/drivers/pwm/pwm-jz4740.c +++ b/drivers/pwm/pwm-jz4740.c @@ -8,18 +8,20 @@ #include <linux/err.h> #include <linux/gpio.h> #include <linux/kernel.h> +#include <linux/mfd/ingenic-tcu.h> +#include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/pwm.h> - -#include <asm/mach-jz4740/timer.h> +#include <linux/regmap.h> #define NUM_PWM 8 struct jz4740_pwm_chip { struct pwm_chip chip; struct clk *clk; + struct regmap *map; }; static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip) @@ -29,6 +31,8 @@ static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip) static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) { + struct jz4740_pwm_chip *jz = to_jz4740(chip); + /* * Timers 0 and 1 are used for system tasks, so they are unavailable * for use as PWMs. @@ -36,50 +40,53 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) if (pwm->hwpwm < 2) return -EBUSY; - jz4740_timer_start(pwm->hwpwm); + regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm)); return 0; } static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) { - jz4740_timer_set_ctrl(pwm->hwpwm, 0); + struct jz4740_pwm_chip *jz = to_jz4740(chip); - jz4740_timer_stop(pwm->hwpwm); + regmap_write(jz->map, TCU_REG_TSCR, BIT(pwm->hwpwm)); } static int jz4740_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) { - uint32_t ctrl = jz4740_timer_get_ctrl(pwm->pwm); + struct jz4740_pwm_chip *jz = to_jz4740(chip); + + /* Enable PWM output */ + regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm), + TCU_TCSR_PWM_EN, TCU_TCSR_PWM_EN); - ctrl |= JZ_TIMER_CTRL_PWM_ENABLE; - jz4740_timer_set_ctrl(pwm->hwpwm, ctrl); - jz4740_timer_enable(pwm->hwpwm); + /* Start counter */ + regmap_write(jz->map, TCU_REG_TESR, BIT(pwm->hwpwm)); return 0; } static void jz4740_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) { - uint32_t ctrl = jz4740_timer_get_ctrl(pwm->hwpwm); + struct jz4740_pwm_chip *jz = to_jz4740(chip); /* * Set duty > period. This trick allows the TCU channels in TCU2 mode to * properly return to their init level. */ - jz4740_timer_set_duty(pwm->hwpwm, 0xffff); - jz4740_timer_set_period(pwm->hwpwm, 0x0); + regmap_write(jz->map, TCU_REG_TDHRc(pwm->hwpwm), 0xffff); + regmap_write(jz->map, TCU_REG_TDFRc(pwm->hwpwm), 0x0); /* * Disable PWM output. * In TCU2 mode (channel 1/2 on JZ4750+), this must be done before the * counter is stopped, while in TCU1 mode the order does not matter. */ - ctrl &= ~JZ_TIMER_CTRL_PWM_ENABLE; - jz4740_timer_set_ctrl(pwm->hwpwm, ctrl); + regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm), + TCU_TCSR_PWM_EN, 0); /* Stop counter */ - jz4740_timer_disable(pwm->hwpwm); + regmap_write(jz->map, TCU_REG_TECR, BIT(pwm->hwpwm)); } static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, @@ -89,7 +96,6 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, unsigned long long tmp; unsigned long period, duty; unsigned int prescaler = 0; - uint16_t ctrl; tmp = (unsigned long long)clk_get_rate(jz4740->clk) * state->period; do_div(tmp, 1000000000); @@ -112,26 +118,37 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, jz4740_pwm_disable(chip, pwm); - jz4740_timer_set_count(pwm->hwpwm, 0); - jz4740_timer_set_duty(pwm->hwpwm, duty); - jz4740_timer_set_period(pwm->hwpwm, period); + /* Set abrupt shutdown */ + regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm), + TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD); + + /* Set clock prescale */ + regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm), + TCU_TCSR_PRESCALE_MASK, + prescaler << TCU_TCSR_PRESCALE_LSB); + + /* Reset counter to 0 */ + regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0); - ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | JZ_TIMER_CTRL_SRC_EXT | - JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN; + /* Set duty */ + regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty); - jz4740_timer_set_ctrl(pwm->hwpwm, ctrl); + /* Set period */ + regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period); + /* Set polarity */ switch (state->polarity) { case PWM_POLARITY_NORMAL: - ctrl &= ~JZ_TIMER_CTRL_PWM_ACTIVE_LOW; + regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm), + TCU_TCSR_PWM_INITL_HIGH, 0); break; case PWM_POLARITY_INVERSED: - ctrl |= JZ_TIMER_CTRL_PWM_ACTIVE_LOW; + regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm), + TCU_TCSR_PWM_INITL_HIGH, + TCU_TCSR_PWM_INITL_HIGH); break; } - jz4740_timer_set_ctrl(pwm->hwpwm, ctrl); - if (state->enabled) jz4740_pwm_enable(chip, pwm); @@ -148,8 +165,9 @@ static const struct pwm_ops jz4740_pwm_ops = { static int jz4740_pwm_probe(struct platform_device *pdev) { struct jz4740_pwm_chip *jz4740; + struct device *dev = &pdev->dev; - jz4740 = devm_kzalloc(&pdev->dev, sizeof(*jz4740), GFP_KERNEL); + jz4740 = devm_kzalloc(dev, sizeof(*jz4740), GFP_KERNEL); if (!jz4740) return -ENOMEM; @@ -157,7 +175,13 @@ static int jz4740_pwm_probe(struct platform_device *pdev) if (IS_ERR(jz4740->clk)) return PTR_ERR(jz4740->clk); - jz4740->chip.dev = &pdev->dev; + jz4740->map = device_node_to_regmap(dev->parent->of_node); + if (!jz4740->map) { + dev_err(dev, "regmap not found\n"); + return -EINVAL; + } + + jz4740->chip.dev = dev; jz4740->chip.ops = &jz4740_pwm_ops; jz4740->chip.npwm = NUM_PWM; jz4740->chip.base = -1;