| Message ID | 1416621212-11701-2-git-send-email-Naidu.Tellapati@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, Nov 22, 2014 at 07:23:29AM +0530, naidu.tellapati@gmail.com wrote: > From: Naidu Tellapati <Naidu.Tellapati@imgtec.com> > > The Pistachio SOC from Imagination Technologies includes a Pulse Width > Modulation DAC which produces 1 to 4 digital bit-outputs which represent > digital waveforms. These PWM outputs are primarily in charge of controlling > backlight LED devices. > > Signed-off-by: Naidu Tellapati <Naidu.Tellapati@imgtec.com> > Signed-off-by: Sai Masarapu <Sai.Masarapu@imgtec.com> > --- > drivers/pwm/Kconfig | 12 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-img.c | 270 +++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 283 insertions(+), 0 deletions(-) > create mode 100644 drivers/pwm/pwm-img.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index ef2dd2e..6b4581a 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -110,6 +110,18 @@ config PWM_FSL_FTM > To compile this driver as a module, choose M here: the module > will be called pwm-fsl-ftm. > > +config PWM_IMG This sounds really generic to me. Basically this says that every PWM IP developed by Imagination Technologies will be compatible with this one. It's typical to name modules after <vendor>-<soc> to avoid this type of ambiguity. Is there any reason why this can't be called PWM_IMG_PISTACHIO? > + tristate "Imagination Technologies PWM driver" > + depends on MFD_SYSCON > + depends on HAS_IOMEM I think you'll need at least COMMON_CLK here as well. > diff --git a/drivers/pwm/pwm-img.c b/drivers/pwm/pwm-img.c [...] > +/* PWM registers */ > +#define CR_PWM_CTRL_CFG 0x0000 > +#define CR_PWM_CTRL_CFG_NO_SUB_DIV 0 > +#define CR_PWM_CTRL_CFG_SUB_DIV0 1 > +#define CR_PWM_CTRL_CFG_SUB_DIV1 2 > +#define CR_PWM_CTRL_CFG_SUB_DIV0_DIV1 3 > +#define CR_PWM_CTRL_CFG_DIV_SHIFT(ch) ((ch) * 2 + 4) > +#define CR_PWM_CTRL_CFG_DIV_MASK 0x3 > + > +#define CR_PWM_CH_CFG(ch) (0x4 + (ch) * 4) > +#define CR_PWM_CH_CFG_TMBASE_SHIFT 0 > +#define CR_PWM_CH_CFG_DUTY_SHIFT 16 What's with the CR_ prefix here? What does it stand for? Can't you just drop it? > +#define CR_PERIP_PWM_PDM_CONTROL 0x0140 > +#define CR_PERIP_PWM_PDM_CONTROL_CH_MASK 0x1 > +#define CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(ch) ((ch) * 4) > + > +#define IMG_NUM_PWM 4 I don't think you need this. See below for more details. > +static inline void img_pwm_writel(struct img_pwm_chip *chip, > + unsigned int reg, unsigned long val) > +{ > + writel(val, chip->base + reg); > +} > + > +static inline unsigned int img_pwm_readl(struct img_pwm_chip *chip, > + unsigned int reg) > +{ > + return readl(chip->base + reg); > +} readl() and writel() deal with u32 data, please use consistent types. > +static int img_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > + int duty_ns, int period_ns) > +{ > + unsigned int div; > + unsigned int duty_steps; > + unsigned int tmbase_steps; > + unsigned long val; > + unsigned long mul; > + unsigned long output_clk_hz; > + unsigned long input_clk_hz; Many of these can be folded into single lines. And again, val is used to store register contents and should be u32. > + struct img_pwm_chip *pwm_chip; > + > + pwm_chip = to_img_pwm_chip(chip); > + > + input_clk_hz = clk_get_rate(pwm_chip->pwm_clk); > + output_clk_hz = DIV_ROUND_UP(NSEC_PER_SEC, period_ns); > + > + mul = DIV_ROUND_UP(input_clk_hz, output_clk_hz); > + if (mul > MAX_TMBASE_STEPS * 512) { > + dev_err(chip->dev, > + "failed to configure timebase steps/divider value.\n"); > + return -EINVAL; > + } I think it'd be more readable if this was the final else in the block of if/else if below. > + > + if (mul <= MAX_TMBASE_STEPS) { > + div = CR_PWM_CTRL_CFG_NO_SUB_DIV; > + tmbase_steps = DIV_ROUND_UP(mul, 1); > + } else if (mul <= MAX_TMBASE_STEPS * 8) { > + div = CR_PWM_CTRL_CFG_SUB_DIV0; > + tmbase_steps = DIV_ROUND_UP(mul, 8); > + } else if (mul <= MAX_TMBASE_STEPS * 64) { > + div = CR_PWM_CTRL_CFG_SUB_DIV1; > + tmbase_steps = DIV_ROUND_UP(mul, 64); > + } else if (mul <= MAX_TMBASE_STEPS * 512) { > + div = CR_PWM_CTRL_CFG_SUB_DIV0_DIV1; > + tmbase_steps = DIV_ROUND_UP(mul, 512); > + } > + > + duty_steps = DIV_ROUND_UP(tmbase_steps * duty_ns, period_ns); > + > + val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG); > + val &= ~(CR_PWM_CTRL_CFG_DIV_MASK << > + CR_PWM_CTRL_CFG_DIV_SHIFT(pwm->hwpwm)); > + val |= (div & CR_PWM_CTRL_CFG_DIV_MASK) << > + CR_PWM_CTRL_CFG_DIV_SHIFT(pwm->hwpwm); If you leave out the CR_ prefix these will actually fit on a single line. > + img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val); > + > + val = (duty_steps << CR_PWM_CH_CFG_DUTY_SHIFT) | > + (tmbase_steps << CR_PWM_CH_CFG_TMBASE_SHIFT); I prefer subsequent lines to be aligned with the first, like so: val = (duty_steps ...) | (tmbase_steps ...); Also I'd leave out the _steps suffix, it doesn't really add information. > +static int img_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + unsigned int val; Should be u32 as well. There are other occurrences like this in the remainder of the driver, but I haven't commented on all of them explicitly. Please fix them all up to be consistent. > + struct img_pwm_chip *pwm_chip; > + > + pwm_chip = to_img_pwm_chip(chip); The above can be a single line. > + > + val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG); > + val |= BIT(pwm->hwpwm); > + img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val); > + > + regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL, > + CR_PERIP_PWM_PDM_CONTROL_CH_MASK << > + CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 0); This smells like pinmux and should probably be a separate driver. > +static void img_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + unsigned int val; > + struct img_pwm_chip *pwm_chip; > + > + pwm_chip = to_img_pwm_chip(chip); > + > + val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG); > + val &= ~BIT(pwm->hwpwm); > + img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val); > + > + regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL, > + CR_PERIP_PWM_PDM_CONTROL_CH_MASK << > + CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 1); > +} Same comments as for img_pwm_enable(). > +static int img_pwm_probe(struct platform_device *pdev) > +{ [...] > + pwm->chip.dev = &pdev->dev; > + pwm->chip.ops = &img_pwm_ops; > + pwm->chip.base = -1; > + pwm->chip.npwm = IMG_NUM_PWM; You can directly substitute 4 here since it's the only place you need it. > +static int img_pwm_remove(struct platform_device *pdev) > +{ > + unsigned int i; > + unsigned int val; > + > + struct img_pwm_chip *pwm_chip = platform_get_drvdata(pdev); This should go at the very top of the variable declarations. > + > + for (i = 0; i < IMG_NUM_PWM; i++) { This would better be pwm_chip->chip.npwm. Thierry
Hi Thierry, Many thanks for the review. > On Sat, Nov 22, 2014 at 07:23:29AM +0530, naidu.tellapati@gmail.com wrote: >> From: Naidu Tellapati <Naidu.Tellapati@imgtec.com> >> >> The Pistachio SOC from Imagination Technologies includes a Pulse Width >> Modulation DAC which produces 1 to 4 digital bit-outputs which represent >> digital waveforms. These PWM outputs are primarily in charge of controlling >> backlight LED devices. >> >> Signed-off-by: Naidu Tellapati <Naidu.Tellapati@imgtec.com> >> Signed-off-by: Sai Masarapu <Sai.Masarapu@imgtec.com> >> --- >> drivers/pwm/Kconfig | 12 ++ >> drivers/pwm/Makefile | 1 + >> drivers/pwm/pwm-img.c | 270 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 283 insertions(+), 0 deletions(-) >> create mode 100644 drivers/pwm/pwm-img.c >> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >> index ef2dd2e..6b4581a 100644 >> --- a/drivers/pwm/Kconfig >> +++ b/drivers/pwm/Kconfig >> @@ -110,6 +110,18 @@ config PWM_FSL_FTM >> To compile this driver as a module, choose M here: the module >> will be called pwm-fsl-ftm. >> >> +config PWM_IMG > This sounds really generic to me. Basically this says that every PWM IP > developed by Imagination Technologies will be compatible with this one. > It's typical to name modules after <vendor>-<soc> to avoid this type of > ambiguity. > Is there any reason why this can't be called PWM_IMG_PISTACHIO? At this moment, this IP Block is available at least on one more SOC from Imagination Technologies. It is possible that the IP will be available on more SOCs in future. Shall we go ahead with PWM_IMG? >> + tristate "Imagination Technologies PWM driver" >> + depends on MFD_SYSCON >> + depends on HAS_IOMEM > I think you'll need at least COMMON_CLK here as well. OK. Will address in the next Patch set. >> diff --git a/drivers/pwm/pwm-img.c b/drivers/pwm/pwm-img.c [...] >> +/* PWM registers */ >> +#define CR_PWM_CTRL_CFG 0x0000 >> +#define CR_PWM_CTRL_CFG_NO_SUB_DIV 0 >> +#define CR_PWM_CTRL_CFG_SUB_DIV0 1 >> +#define CR_PWM_CTRL_CFG_SUB_DIV1 2 >> +#define CR_PWM_CTRL_CFG_SUB_DIV0_DIV1 3 >> +#define CR_PWM_CTRL_CFG_DIV_SHIFT(ch) ((ch) * 2 + 4) >> +#define CR_PWM_CTRL_CFG_DIV_MASK 0x3 >> + >> +#define CR_PWM_CH_CFG(ch) (0x4 + (ch) * 4) >> +#define CR_PWM_CH_CFG_TMBASE_SHIFT 0 >> +#define CR_PWM_CH_CFG_DUTY_SHIFT 16 > What's with the CR_ prefix here? What does it stand for? Can't you just > drop it? CR stands for Control Register. We have picked it from the datasheet and wanted to make the register names compatible with the same in the datasheet. Hope Andrew agrees with Thierry's comments here. Hope we can drop it. Andrew, any comments here, please. >> +#define CR_PERIP_PWM_PDM_CONTROL 0x0140 >> +#define CR_PERIP_PWM_PDM_CONTROL_CH_MASK 0x1 >> +#define CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(ch) ((ch) * 4) >> + >> +#define IMG_NUM_PWM 4 > I don't think you need this. See below for more details. OK. Will address in the next patch set. >> +static inline void img_pwm_writel(struct img_pwm_chip *chip, >> + unsigned int reg, unsigned long val) >> +{ >> + writel(val, chip->base + reg); >> +} >> + >> +static inline unsigned int img_pwm_readl(struct img_pwm_chip *chip, >> + unsigned int reg) >> +{ >> + return readl(chip->base + reg); >> +} > readl() and writel() deal with u32 data, please use consistent types. OK. Will address in the next patch set. >> +static int img_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, >> + int duty_ns, int period_ns) >> +{ >> + unsigned int div; >> + unsigned int duty_steps; >> + unsigned int tmbase_steps; >> + unsigned long val; >> + unsigned long mul; >> + unsigned long output_clk_hz; >> + unsigned long input_clk_hz; > Many of these can be folded into single lines. And again, val is used to > store register contents and should be u32. Agree. Will address in the next patch set. >> + struct img_pwm_chip *pwm_chip; >> + >> + pwm_chip = to_img_pwm_chip(chip); >> + >> + input_clk_hz = clk_get_rate(pwm_chip->pwm_clk); >> + output_clk_hz = DIV_ROUND_UP(NSEC_PER_SEC, period_ns); >> + >> + mul = DIV_ROUND_UP(input_clk_hz, output_clk_hz); >> + if (mul > MAX_TMBASE_STEPS * 512) { >> + dev_err(chip->dev, >> + "failed to configure timebase steps/divider value.\n"); >> + return -EINVAL; >> + } > I think it'd be more readable if this was the final else in the block of > if/else if below. OK. Will address in the next patch set. >> + >> + if (mul <= MAX_TMBASE_STEPS) { >> + div = CR_PWM_CTRL_CFG_NO_SUB_DIV; >> + tmbase_steps = DIV_ROUND_UP(mul, 1); >> + } else if (mul <= MAX_TMBASE_STEPS * 8) { >> + div = CR_PWM_CTRL_CFG_SUB_DIV0; >> + tmbase_steps = DIV_ROUND_UP(mul, 8); >> + } else if (mul <= MAX_TMBASE_STEPS * 64) { >> + div = CR_PWM_CTRL_CFG_SUB_DIV1; >> + tmbase_steps = DIV_ROUND_UP(mul, 64); >> + } else if (mul <= MAX_TMBASE_STEPS * 512) { >> + div = CR_PWM_CTRL_CFG_SUB_DIV0_DIV1; >> + tmbase_steps = DIV_ROUND_UP(mul, 512); >> + } >> + >> + duty_steps = DIV_ROUND_UP(tmbase_steps * duty_ns, period_ns); >> + >> + val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG); >> + val &= ~(CR_PWM_CTRL_CFG_DIV_MASK << >> + CR_PWM_CTRL_CFG_DIV_SHIFT(pwm->hwpwm)); >> + val |= (div & CR_PWM_CTRL_CFG_DIV_MASK) << >> + CR_PWM_CTRL_CFG_DIV_SHIFT(pwm->hwpwm); > If you leave out the CR_ prefix these will actually fit on a single > line. OK. Will address in the next patch set. >> + img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val); >> + >> + val = (duty_steps << CR_PWM_CH_CFG_DUTY_SHIFT) | >> + (tmbase_steps << CR_PWM_CH_CFG_TMBASE_SHIFT); > I prefer subsequent lines to be aligned with the first, like so: OK. Will address in the next patch set. > val = (duty_steps ...) | > (tmbase_steps ...); > Also I'd leave out the _steps suffix, it doesn't really add information. OK. Will address in the next patch set. >> +static int img_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) >> +{ >> + unsigned int val; > Should be u32 as well. There are other occurrences like this in the > remainder of the driver, but I haven't commented on all of them > explicitly. Please fix them all up to be consistent. OK. Will fix the issue at all the places in the driver. >> + struct img_pwm_chip *pwm_chip; >> + >> + pwm_chip = to_img_pwm_chip(chip); > The above can be a single line. OK. Will address in the next patch set. >> + >> + val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG); >> + val |= BIT(pwm->hwpwm); >> + img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val); >> + >> + regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL, >> + CR_PERIP_PWM_PDM_CONTROL_CH_MASK << >> + CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 0); > This smells like pinmux and should probably be a separate driver. Hope you are suggesting us to have a separate driver for pin muxing (might already have one for Pistachio) and call the pin muxing driver APIs from here. Please correct me if my understanding is wrong? >> +static void img_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) >> +{ >> + unsigned int val; >> + struct img_pwm_chip *pwm_chip; >> + >> + pwm_chip = to_img_pwm_chip(chip); >> + >> + val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG); >> + val &= ~BIT(pwm->hwpwm); >> + img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val); >> + >> + regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL, >> + CR_PERIP_PWM_PDM_CONTROL_CH_MASK << >> + CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 1); >> +} > Same comments as for img_pwm_enable(). OK. >> +static int img_pwm_probe(struct platform_device *pdev) >> +{ [...] >> + pwm->chip.dev = &pdev->dev; >> + pwm->chip.ops = &img_pwm_ops; >> + pwm->chip.base = -1; >> + pwm->chip.npwm = IMG_NUM_PWM; > You can directly substitute 4 here since it's the only place you need > it. OK. Will address in the next patch set. >> +static int img_pwm_remove(struct platform_device *pdev) >> +{ >> + unsigned int i; >> + unsigned int val; >> + >> + struct img_pwm_chip *pwm_chip = platform_get_drvdata(pdev); > This should go at the very top of the variable declarations. OK. Will address in the next patch set. >> + >> + for (i = 0; i < IMG_NUM_PWM; i++) { > This would better be pwm_chip->chip.npwm. OK. Will address in the next patch set. > Thierry Thanks and regards, Naidu. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 24, 2014 at 11:22:31AM +0000, Naidu Tellapati wrote: > Hi Thierry, > > Many thanks for the review. > > > On Sat, Nov 22, 2014 at 07:23:29AM +0530, naidu.tellapati@gmail.com wrote: > >> From: Naidu Tellapati <Naidu.Tellapati@imgtec.com> > >> > >> The Pistachio SOC from Imagination Technologies includes a Pulse Width > >> Modulation DAC which produces 1 to 4 digital bit-outputs which represent > >> digital waveforms. These PWM outputs are primarily in charge of controlling > >> backlight LED devices. > >> > >> Signed-off-by: Naidu Tellapati <Naidu.Tellapati@imgtec.com> > >> Signed-off-by: Sai Masarapu <Sai.Masarapu@imgtec.com> > >> --- > >> drivers/pwm/Kconfig | 12 ++ > >> drivers/pwm/Makefile | 1 + > >> drivers/pwm/pwm-img.c | 270 +++++++++++++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 283 insertions(+), 0 deletions(-) > >> create mode 100644 drivers/pwm/pwm-img.c > >> > >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > >> index ef2dd2e..6b4581a 100644 > >> --- a/drivers/pwm/Kconfig > >> +++ b/drivers/pwm/Kconfig > >> @@ -110,6 +110,18 @@ config PWM_FSL_FTM > >> To compile this driver as a module, choose M here: the module > >> will be called pwm-fsl-ftm. > >> > >> +config PWM_IMG > > > This sounds really generic to me. Basically this says that every PWM IP > > developed by Imagination Technologies will be compatible with this one. > > It's typical to name modules after <vendor>-<soc> to avoid this type of > > ambiguity. > > > Is there any reason why this can't be called PWM_IMG_PISTACHIO? > > At this moment, this IP Block is available at least on one more SOC from Imagination Technologies. > It is possible that the IP will be available on more SOCs in future. > > Shall we go ahead with PWM_IMG? Alright, if ever there's a different PWM IP block from Imagination Technologies, the driver for that can have a more specific name. > >> + > >> + val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG); > >> + val |= BIT(pwm->hwpwm); > >> + img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val); > >> + > >> + regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL, > >> + CR_PERIP_PWM_PDM_CONTROL_CH_MASK << > >> + CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 0); > > > This smells like pinmux and should probably be a separate driver. > > Hope you are suggesting us to have a separate driver for pin muxing (might already have one for Pistachio) > and call the pin muxing driver APIs from here. > > Please correct me if my understanding is wrong? I don't think you need to call the pinmux API from here. Rather I'll assume that the muxing is fixed on a given board, so this configuration would be part of the static board-level pinmux so it will automatically be applied at boot time. Thierry
Hi James, > On Mon, Nov 24, 2014 at 11:22:31AM +0000, Naidu Tellapati wrote: >> Hi Thierry, >> >> Many thanks for the review. >> >> > On Sat, Nov 22, 2014 at 07:23:29AM +0530, naidu.tellapati@gmail.com wrote: >> >> From: Naidu Tellapati <Naidu.Tellapati@imgtec.com> >> >> >> >> The Pistachio SOC from Imagination Technologies includes a Pulse Width >> >> Modulation DAC which produces 1 to 4 digital bit-outputs which represent >> >> digital waveforms. These PWM outputs are primarily in charge of controlling >> >> backlight LED devices. >> >> >> >> Signed-off-by: Naidu Tellapati <Naidu.Tellapati@imgtec.com> >> >> Signed-off-by: Sai Masarapu <Sai.Masarapu@imgtec.com> >> >> --- >>> >> drivers/pwm/Kconfig | 12 ++ >> >> drivers/pwm/Makefile | 1 + >> >> drivers/pwm/pwm-img.c | 270 +++++++++++++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 283 insertions(+), 0 deletions(-) >> >> create mode 100644 drivers/pwm/pwm-img.c >> >> >> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >> >> index ef2dd2e..6b4581a 100644 >> >> --- a/drivers/pwm/Kconfig >> >> +++ b/drivers/pwm/Kconfig >> >> @@ -110,6 +110,18 @@ config PWM_FSL_FTM >> >> To compile this driver as a module, choose M here: the module >> >> will be called pwm-fsl-ftm. >> >> >> >> +config PWM_IMG >> >> > This sounds really generic to me. Basically this says that every PWM IP >> > developed by Imagination Technologies will be compatible with this one. >> > It's typical to name modules after <vendor>-<soc> to avoid this type of >> > ambiguity. >> >> > Is there any reason why this can't be called PWM_IMG_PISTACHIO? >> >> At this moment, this IP Block is available at least on one more SOC from Imagination Technologies. >> It is possible that the IP will be available on more SOCs in future. >> >> Shall we go ahead with PWM_IMG? > Alright, if ever there's a different PWM IP block from Imagination > Technologies, the driver for that can have a more specific name. >> >> + >> >> + val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG); >> >> + val |= BIT(pwm->hwpwm); >> >> + img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val); >> >> + >> >> + regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL, >> >> + CR_PERIP_PWM_PDM_CONTROL_CH_MASK << >> >> + CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 0); >> >> > This smells like pinmux and should probably be a separate driver. >> >> Hope you are suggesting us to have a separate driver for pin muxing (might already have one for Pistachio) >> and call the pin muxing driver APIs from here. >> >> Please correct me if my understanding is wrong? > I don't think you need to call the pinmux API from here. Rather I'll > assume that the muxing is fixed on a given board, so this configuration > would be part of the static board-level pinmux so it will automatically > be applied at boot time. Could you please suggest us on the output pin muxing between PWM & PDM. As of now we are configuring the mux control register from the PWM & PDM drivers. Regards, Naidu.-- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> diff --git a/drivers/pwm/pwm-img.c b/drivers/pwm/pwm-img.c > [...] >>> +/* PWM registers */ >>> +#define CR_PWM_CTRL_CFG 0x0000 >>> +#define CR_PWM_CTRL_CFG_NO_SUB_DIV 0 >>> +#define CR_PWM_CTRL_CFG_SUB_DIV0 1 >>> +#define CR_PWM_CTRL_CFG_SUB_DIV1 2 >>> +#define CR_PWM_CTRL_CFG_SUB_DIV0_DIV1 3 >>> +#define CR_PWM_CTRL_CFG_DIV_SHIFT(ch) ((ch) * 2 + 4) >>> +#define CR_PWM_CTRL_CFG_DIV_MASK 0x3 >>> + >>> +#define CR_PWM_CH_CFG(ch) (0x4 + (ch) * 4) >>> +#define CR_PWM_CH_CFG_TMBASE_SHIFT 0 >>> +#define CR_PWM_CH_CFG_DUTY_SHIFT 16 > >> What's with the CR_ prefix here? What does it stand for? Can't you just >> drop it? > > CR stands for Control Register. We have picked it from the datasheet and wanted to make the > register names compatible with the same in the datasheet. > > Hope Andrew agrees with Thierry's comments here. Hope we can drop it. > > Andrew, any comments here, please. I didn't think the CR_ prefix was totally necessary, but it matches what's in the TRM, so I didn't have an issue with it. I'm fine with dropping it. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> >> + val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG); >> >> + val |= BIT(pwm->hwpwm); >> >> + img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val); >> >> + >> >> + regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL, >> >> + CR_PERIP_PWM_PDM_CONTROL_CH_MASK << >> >> + CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 0); >> >> > This smells like pinmux and should probably be a separate driver. >> >> Hope you are suggesting us to have a separate driver for pin muxing (might already have one for Pistachio) >> and call the pin muxing driver APIs from here. >> >> Please correct me if my understanding is wrong? > > I don't think you need to call the pinmux API from here. Rather I'll > assume that the muxing is fixed on a given board, so this configuration > would be part of the static board-level pinmux so it will automatically > be applied at boot time. The issue here is that this register does not belong to the pinmux/pinctrl block on this SoC and instead is part of the "peripheral control" block which primarily provides system interface gate clocks and resets (for which I was planning to write a clock provider driver), but also has a bunch of control registers for various IP blocks thrown together (like this one) which we're using syscon to access. I'm not sure it makes sense to create a pinmux driver for these 4 bits, but I'll defer to others on that... Another option would be to have the main pinctrl driver for Pistachio control these bits, but that doesn't seem quite right since these bits are completely separate from the pinctrl block. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24 Nov 2014, at 18:19, Andrew Bresticker <abrestic@chromium.org> wrote: >>>>> + val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG); >>>>> + val |= BIT(pwm->hwpwm); >>>>> + img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val); >>>>> + >>>>> + regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL, >>>>> + CR_PERIP_PWM_PDM_CONTROL_CH_MASK << >>>>> + CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 0); >>> >>>> This smells like pinmux and should probably be a separate driver. >>> >>> Hope you are suggesting us to have a separate driver for pin muxing (might already have one for Pistachio) >>> and call the pin muxing driver APIs from here. >>> >>> Please correct me if my understanding is wrong? >> >> I don't think you need to call the pinmux API from here. Rather I'll >> assume that the muxing is fixed on a given board, so this configuration >> would be part of the static board-level pinmux so it will automatically >> be applied at boot time. > > The issue here is that this register does not belong to the > pinmux/pinctrl block on this SoC and instead is part of the > "peripheral control" block which primarily provides system interface > gate clocks and resets (for which I was planning to write a clock > provider driver), but also has a bunch of control registers for > various IP blocks thrown together (like this one) which we're using > syscon to access. I'm not sure it makes sense to create a pinmux > driver for these 4 bits, but I'll defer to others on that... > > Another option would be to have the main pinctrl driver for Pistachio > control these bits, but that doesn't seem quite right since these bits > are completely separate from the pinctrl block. I would think the pinctrl driver should deal with this - yes it's a separate set of registers, but the config is expected to be static and can be represented in the DT. James. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi James, Andrew, Thierry and all, > On 24 Nov 2014, at 18:19, Andrew Bresticker <abrestic@chromium.org> wrote: >>>>>> + val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG); >>>>>> + val |= BIT(pwm->hwpwm); >>>>>> + img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val); >>>>>> + >>>>>> + regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL, >>>>>> + CR_PERIP_PWM_PDM_CONTROL_CH_MASK << >>>>>> + CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 0); >>>> >>>>> This smells like pinmux and should probably be a separate driver. >>>> >>>> Hope you are suggesting us to have a separate driver for pin muxing (might already have one for Pistachio) >>>> and call the pin muxing driver APIs from here. >>>> >>>> Please correct me if my understanding is wrong? >>> >>> I don't think you need to call the pinmux API from here. Rather I'll >>> assume that the muxing is fixed on a given board, so this configuration >>> would be part of the static board-level pinmux so it will automatically >>> be applied at boot time. >> >> The issue here is that this register does not belong to the >> pinmux/pinctrl block on this SoC and instead is part of the >> "peripheral control" block which primarily provides system interface >> gate clocks and resets (for which I was planning to write a clock >> provider driver), but also has a bunch of control registers for >> various IP blocks thrown together (like this one) which we're using >> syscon to access. I'm not sure it makes sense to create a pinmux >> driver for these 4 bits, but I'll defer to others on that... >> >> Another option would be to have the main pinctrl driver for Pistachio >> control these bits, but that doesn't seem quite right since these bits >> are completely separate from the pinctrl block. > I would think the pinctrl driver should deal with this - yes it's a separate set of registers, > but the config is expected to be static and can be represented in the DT. > James. I have discussed with the hardware team one more time today and discovered that the value we configure to CR_PERIP_PWM_PDM_CONTROL not only configures PWM/PDM output mux, but also enables control signal for PDM blocks. As an example, if the PDM user wants to enable PDM block 2, he needs to set bit 8 to 1 in the register CR_PERIP_PWM_PDM_CONTROL. Please also note that setting bit 8 to 1 not only configures the output PWM/PDM mux, but also enables control signal for PDM block 2. He needs access to this register from the PDM driver at least to configure (enable/disable) PDM control signal. And also because of the reasons explained by Andrew above, I think it is better to use syscon (instead of handling it in Pinctrl driver) to access this register both in PWM and PDM drivers. Please let us know comments and suggestions. Thanks and regards, Naidu. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index ef2dd2e..6b4581a 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -110,6 +110,18 @@ config PWM_FSL_FTM To compile this driver as a module, choose M here: the module will be called pwm-fsl-ftm. +config PWM_IMG + tristate "Imagination Technologies PWM driver" + depends on MFD_SYSCON + depends on HAS_IOMEM + depends on MIPS || COMPILE_TEST + help + Generic PWM framework driver for Imagination Technologies + PWM block which supports 4 channels. + + To compile this driver as a module, choose M here: the module + will be called pwm-img. + config PWM_IMX tristate "i.MX PWM support" depends on ARCH_MXC diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index c458606..29655fd 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -8,6 +8,7 @@ obj-$(CONFIG_PWM_BFIN) += pwm-bfin.o obj-$(CONFIG_PWM_CLPS711X) += pwm-clps711x.o obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o +obj-$(CONFIG_PWM_IMG) += pwm-img.o obj-$(CONFIG_PWM_IMX) += pwm-imx.o obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o diff --git a/drivers/pwm/pwm-img.c b/drivers/pwm/pwm-img.c new file mode 100644 index 0000000..79baac2 --- /dev/null +++ b/drivers/pwm/pwm-img.c @@ -0,0 +1,270 @@ +/* + * Imagination Technologies Pulse Width Modulator driver + * + * Copyright (c) 2014, Imagination Technologies + * + * Based on drivers/pwm/pwm-tegra.c, Copyright (c) 2010, NVIDIA Corporation + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/regmap.h> +#include <linux/slab.h> + +/* PWM registers */ +#define CR_PWM_CTRL_CFG 0x0000 +#define CR_PWM_CTRL_CFG_NO_SUB_DIV 0 +#define CR_PWM_CTRL_CFG_SUB_DIV0 1 +#define CR_PWM_CTRL_CFG_SUB_DIV1 2 +#define CR_PWM_CTRL_CFG_SUB_DIV0_DIV1 3 +#define CR_PWM_CTRL_CFG_DIV_SHIFT(ch) ((ch) * 2 + 4) +#define CR_PWM_CTRL_CFG_DIV_MASK 0x3 + +#define CR_PWM_CH_CFG(ch) (0x4 + (ch) * 4) +#define CR_PWM_CH_CFG_TMBASE_SHIFT 0 +#define CR_PWM_CH_CFG_DUTY_SHIFT 16 + +#define CR_PERIP_PWM_PDM_CONTROL 0x0140 +#define CR_PERIP_PWM_PDM_CONTROL_CH_MASK 0x1 +#define CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(ch) ((ch) * 4) + +#define IMG_NUM_PWM 4 +#define MAX_TMBASE_STEPS 65536 + +struct img_pwm_chip { + struct device *dev; + struct pwm_chip chip; + struct clk *pwm_clk; + struct clk *sys_clk; + void __iomem *base; + struct regmap *periph_regs; +}; + +static inline struct img_pwm_chip *to_img_pwm_chip(struct pwm_chip *chip) +{ + return container_of(chip, struct img_pwm_chip, chip); +} + +static inline void img_pwm_writel(struct img_pwm_chip *chip, + unsigned int reg, unsigned long val) +{ + writel(val, chip->base + reg); +} + +static inline unsigned int img_pwm_readl(struct img_pwm_chip *chip, + unsigned int reg) +{ + return readl(chip->base + reg); +} + +static int img_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, + int duty_ns, int period_ns) +{ + unsigned int div; + unsigned int duty_steps; + unsigned int tmbase_steps; + unsigned long val; + unsigned long mul; + unsigned long output_clk_hz; + unsigned long input_clk_hz; + struct img_pwm_chip *pwm_chip; + + pwm_chip = to_img_pwm_chip(chip); + + input_clk_hz = clk_get_rate(pwm_chip->pwm_clk); + output_clk_hz = DIV_ROUND_UP(NSEC_PER_SEC, period_ns); + + mul = DIV_ROUND_UP(input_clk_hz, output_clk_hz); + if (mul > MAX_TMBASE_STEPS * 512) { + dev_err(chip->dev, + "failed to configure timebase steps/divider value.\n"); + return -EINVAL; + } + + if (mul <= MAX_TMBASE_STEPS) { + div = CR_PWM_CTRL_CFG_NO_SUB_DIV; + tmbase_steps = DIV_ROUND_UP(mul, 1); + } else if (mul <= MAX_TMBASE_STEPS * 8) { + div = CR_PWM_CTRL_CFG_SUB_DIV0; + tmbase_steps = DIV_ROUND_UP(mul, 8); + } else if (mul <= MAX_TMBASE_STEPS * 64) { + div = CR_PWM_CTRL_CFG_SUB_DIV1; + tmbase_steps = DIV_ROUND_UP(mul, 64); + } else if (mul <= MAX_TMBASE_STEPS * 512) { + div = CR_PWM_CTRL_CFG_SUB_DIV0_DIV1; + tmbase_steps = DIV_ROUND_UP(mul, 512); + } + + duty_steps = DIV_ROUND_UP(tmbase_steps * duty_ns, period_ns); + + val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG); + val &= ~(CR_PWM_CTRL_CFG_DIV_MASK << + CR_PWM_CTRL_CFG_DIV_SHIFT(pwm->hwpwm)); + val |= (div & CR_PWM_CTRL_CFG_DIV_MASK) << + CR_PWM_CTRL_CFG_DIV_SHIFT(pwm->hwpwm); + img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val); + + val = (duty_steps << CR_PWM_CH_CFG_DUTY_SHIFT) | + (tmbase_steps << CR_PWM_CH_CFG_TMBASE_SHIFT); + img_pwm_writel(pwm_chip, CR_PWM_CH_CFG(pwm->hwpwm), val); + + return 0; +} + +static int img_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + unsigned int val; + struct img_pwm_chip *pwm_chip; + + pwm_chip = to_img_pwm_chip(chip); + + val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG); + val |= BIT(pwm->hwpwm); + img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val); + + regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL, + CR_PERIP_PWM_PDM_CONTROL_CH_MASK << + CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 0); + + return 0; +} + +static void img_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + unsigned int val; + struct img_pwm_chip *pwm_chip; + + pwm_chip = to_img_pwm_chip(chip); + + val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG); + val &= ~BIT(pwm->hwpwm); + img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val); + + regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL, + CR_PERIP_PWM_PDM_CONTROL_CH_MASK << + CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 1); +} + +static const struct pwm_ops img_pwm_ops = { + .config = img_pwm_config, + .enable = img_pwm_enable, + .disable = img_pwm_disable, + .owner = THIS_MODULE, +}; + +static int img_pwm_probe(struct platform_device *pdev) +{ + int ret; + struct resource *res; + struct img_pwm_chip *pwm; + + pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL); + if (!pwm) + return -ENOMEM; + + pwm->dev = &pdev->dev; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + pwm->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(pwm->base)) + return PTR_ERR(pwm->base); + + pwm->periph_regs = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, + "img,cr-periph"); + if (IS_ERR(pwm->periph_regs)) + return PTR_ERR(pwm->periph_regs); + + pwm->sys_clk = devm_clk_get(&pdev->dev, "sys"); + if (IS_ERR(pwm->sys_clk)) { + dev_err(&pdev->dev, "failed to get system clock.\n"); + return PTR_ERR(pwm->sys_clk); + } + + pwm->pwm_clk = devm_clk_get(&pdev->dev, "pwm"); + if (IS_ERR(pwm->pwm_clk)) { + dev_err(&pdev->dev, "failed to get pwm clock.\n"); + return PTR_ERR(pwm->pwm_clk); + } + + ret = clk_prepare_enable(pwm->sys_clk); + if (ret < 0) { + dev_err(&pdev->dev, "could not prepare or enable sys clock.\n"); + return ret; + } + + ret = clk_prepare_enable(pwm->pwm_clk); + if (ret < 0) { + dev_err(&pdev->dev, "could not prepare or enable pwm clock.\n"); + goto disable_sysclk; + } + + pwm->chip.dev = &pdev->dev; + pwm->chip.ops = &img_pwm_ops; + pwm->chip.base = -1; + pwm->chip.npwm = IMG_NUM_PWM; + + ret = pwmchip_add(&pwm->chip); + if (ret < 0) { + dev_err(&pdev->dev, "pwmchip_add failed: %d\n", ret); + goto disable_pwmclk; + } + + platform_set_drvdata(pdev, pwm); + +disable_pwmclk: + clk_disable_unprepare(pwm->pwm_clk); +disable_sysclk: + clk_disable_unprepare(pwm->sys_clk); + + return 0; +} + +static int img_pwm_remove(struct platform_device *pdev) +{ + unsigned int i; + unsigned int val; + + struct img_pwm_chip *pwm_chip = platform_get_drvdata(pdev); + + for (i = 0; i < IMG_NUM_PWM; i++) { + val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG); + val &= ~BIT(i); + img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val); + } + + clk_disable_unprepare(pwm_chip->pwm_clk); + clk_disable_unprepare(pwm_chip->sys_clk); + + return pwmchip_remove(&pwm_chip->chip); +} + +static const struct of_device_id img_pwm_of_match[] = { + { .compatible = "img,pistachio-pwm", }, + { } +}; +MODULE_DEVICE_TABLE(of, img_pwm_of_match); + +static struct platform_driver img_pwm_driver = { + .driver = { + .name = "img-pwm", + .of_match_table = of_match_ptr(img_pwm_of_match), + }, + .probe = img_pwm_probe, + .remove = img_pwm_remove, +}; +module_platform_driver(img_pwm_driver); + +MODULE_AUTHOR("Sai Masarapu <Sai.Masarapu@imgtec.com>"); +MODULE_DESCRIPTION("Imagination Technologies PWM DAC driver"); +MODULE_LICENSE("GPL v2");