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?
> + 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
Attachment:
pgpppNX2qHubu.pgp
Description: PGP signature
