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@xxxxxxxxx wrote: > >> From: Naidu Tellapati <Naidu.Tellapati@xxxxxxxxxx> > >> > >> 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@xxxxxxxxxx> > >> Signed-off-by: Sai Masarapu <Sai.Masarapu@xxxxxxxxxx> > >> --- > >> 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:
pgpMywBjB8GGh.pgp
Description: PGP signature
