Re: [PATCH RESEND v4 1/4] pwm: Imagination Technologies PWM DAC driver

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2929 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]