On Sat, Nov 22, 2014 at 07:23:29AM +0530, naidu.tellapati@gmail.com wrote:

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.

[...]

+/* 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