On Wed, Oct 01, 2014 at 04:53:00PM +0200, Boris Brezillon wrote:
[...]

This isn't really necessary since MFD_ATMEL_HLCDC already depends on OF.

[...]

You might want to use an extra pair of parentheses around the "x" above.

+struct atmel_hlcdc_pwm_chip {

Can we make this...

+	struct pwm_chip chip;
+	struct atmel_hlcdc *hlcdc;
+	struct clk *cur_clk;
+};
+
+static inline struct atmel_hlcdc_pwm_chip *
+pwm_chip_to_atmel_hlcdc_pwm_chip(struct pwm_chip *chip)

... and this a little shorter? There is a lot of line-wrapping below
only because this is very long. It seems like just dropping the
pwm_chip_ prefix on this function would be enough to not exceed the
78/80 character limit.

+{
+	return container_of(chip, struct atmel_hlcdc_pwm_chip, chip);
+}
+
+static int atmel_hlcdc_pwm_config(struct pwm_chip *c,
+				  struct pwm_device *pwm,
+				  int duty_ns, int period_ns)
+{
+	struct atmel_hlcdc_pwm_chip *chip =
+				pwm_chip_to_atmel_hlcdc_pwm_chip(c);
+	struct atmel_hlcdc *hlcdc = chip->hlcdc;
+	struct clk *new_clk = hlcdc->slow_clk;
+	u64 pwmcval = duty_ns * 256;
+	unsigned long clk_freq;
+	u64 clk_period_ns;
+	u32 pwmcfg;
+	int pres;
+
+	clk_freq = clk_get_rate(new_clk);
+	clk_period_ns = 1000000000;

NSEC_PER_SEC?

+	clk_period_ns *= 256;

Perhaps collapse the above two in a single line:

	clk_period_ns = NSEC_PER_SEC * 256;

?

+	do_div(clk_period_ns, clk_freq);
+
+	if (clk_period_ns > period_ns) {
+		new_clk = hlcdc->sys_clk;
+		clk_freq = clk_get_rate(new_clk);
+		clk_period_ns = 1000000000;
+		clk_period_ns *= 256;

Maybe:

	clk_period_ns = NSEC_PER_SEC * 256;

?

+		do_div(clk_period_ns, clk_freq);
+	}
+
+	for (pres = 0; pres <= ATMEL_HLCDC_PWMPS_MAX; pres++) {
+		if ((clk_period_ns << pres) >= period_ns)
+			break;
+	}

Technically there's no need for the curly braces.

+
+	if (pres > ATMEL_HLCDC_PWMPS_MAX)
+		return -EINVAL;

I think the condition above needs to be "pres == ATMEL_HLCDC_PWMPS_MAX",
otherwise this will never be true.

+
+	pwmcfg = ATMEL_HLCDC_PWMPS(pres);
+
+	if (new_clk != chip->cur_clk) {
+		u32 gencfg = 0;
+
+		clk_prepare_enable(new_clk);

This can fail so it needs error-checking.

+		clk_disable_unprepare(chip->cur_clk);
+		chip->cur_clk = new_clk;
+
+		if (new_clk != hlcdc->slow_clk)
+			gencfg = ATMEL_HLCDC_CLKPWMSEL;

There are lots of negations here, which caused me to think that there
was a third clock involved here, but it seems like new_clk can either be
slow_clk or sys_clk.

Perhaps making this condition "new_clk == hlcdc->sys_clk" would improve
clarity here. Maybe a comment somewhere would help?

+		regmap_update_bits(hlcdc->regmap, ATMEL_HLCDC_CFG(0),
+				   ATMEL_HLCDC_CLKPWMSEL, gencfg);
+	}
+
+	do_div(pwmcval, period_ns);
+	if (pwmcval > 255)

The PWM core already makes sure that duty_ns <= period_ns, so pwmcval
could be anywhere between 0 and 256 here. Where does the disconnect come
from? Why not make pwmcval = duty_ns * 255 if that's the maximum?

+		pwmcval = 255;
+
+	pwmcfg |= ATMEL_HLCDC_PWMCVAL(pwmcval);
+
+	regmap_update_bits(hlcdc->regmap, ATMEL_HLCDC_CFG(6),
+			   ATMEL_HLCDC_PWMCVAL_MASK | ATMEL_HLCDC_PWMPS_MASK,
+			   pwmcfg);
+
+	return 0;
+}
+
+static int atmel_hlcdc_pwm_set_polarity(struct pwm_chip *c,
+					struct pwm_device *pwm,
+					enum pwm_polarity polarity)
+{
+	struct atmel_hlcdc_pwm_chip *chip =
+				pwm_chip_to_atmel_hlcdc_pwm_chip(c);
+	struct atmel_hlcdc *hlcdc = chip->hlcdc;
+	u32 cfg = 0;
+
+	if (polarity == PWM_POLARITY_NORMAL)
+		cfg = ATMEL_HLCDC_PWMPOL;

That's strange. Inverse polarity is the default on this hardware?

+static int atmel_hlcdc_pwm_enable(struct pwm_chip *c,
+				  struct pwm_device *pwm)

There's no need for line-wrapping here. The above fits on one line just
fine.

+{
+	struct atmel_hlcdc_pwm_chip *chip =
+				pwm_chip_to_atmel_hlcdc_pwm_chip(c);
+	struct atmel_hlcdc *hlcdc = chip->hlcdc;
+	u32 status;
+
+	regmap_write(hlcdc->regmap, ATMEL_HLCDC_EN, ATMEL_HLCDC_PWM);
+	while (!regmap_read(hlcdc->regmap, ATMEL_HLCDC_SR, &status) &&
+	       !(status & ATMEL_HLCDC_PWM))
+		;

This loop isn't very readable. Can you improve it? Perhaps:

	do {
		err = regmap_read(hlcdc->regmap, ATMEL_HLCDC_SR, &status);
		if (err < 0)
			return err;
	} while ((status & ATMEL_HLCDC_PWM) == 0);

That also allows errors to be properly propagated. Perhaps you also want
to put a usleep_range() or similar in there.

+static void atmel_hlcdc_pwm_disable(struct pwm_chip *c,
+				    struct pwm_device *pwm)
+{
+	struct atmel_hlcdc_pwm_chip *chip =
+				pwm_chip_to_atmel_hlcdc_pwm_chip(c);
+	struct atmel_hlcdc *hlcdc = chip->hlcdc;
+	u32 status;
+
+	regmap_write(hlcdc->regmap, ATMEL_HLCDC_DIS, ATMEL_HLCDC_PWM);
+	while (!regmap_read(hlcdc->regmap, ATMEL_HLCDC_SR, &status) &&
+	       (status & ATMEL_HLCDC_PWM))
+		;

Same here.

+static int atmel_hlcdc_pwm_probe(struct platform_device *pdev)
+{
+	struct atmel_hlcdc_pwm_chip *chip;
+	struct device *dev = &pdev->dev;
+	struct atmel_hlcdc *hlcdc;
+	int ret;
+
+	hlcdc = dev_get_drvdata(dev->parent);
+	if (!hlcdc)
+		return -EINVAL;

Can this really happen?

+	ret = clk_prepare_enable(hlcdc->periph_clk);
+	if (ret)
+		return ret;
+
+	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;

Don't you want to disable and unprepare the clock here? Perhaps in order
to avoid this call clk_prepare_enable() only after all resources have
been allocated.

+MODULE_ALIAS("platform:atmel-hlcdc-pwm");
+MODULE_AUTHOR("Boris Brezillon [off-list ref]");
+MODULE_DESCRIPTION("Atmel HLCDC PWM driver");
+MODULE_LICENSE("GPL");

According to the file header this needs to be "GPL v2".

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141006/b6e0a85b/attachment.sig>