Date:	Mon, 6 Oct 2014 12:46:35 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Boris Brezillon <boris.brezillon@...e-electrons.com>
Cc:	David Airlie <airlied@...ux.ie>, dri-devel@...ts.freedesktop.org,
	Nicolas Ferre <nicolas.ferre@...el.com>,
	Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
	Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
	Andrew Victor <linux@...im.org.za>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Lee Jones <lee.jones@...aro.org>, linux-pwm@...r.kernel.org,
	Rob Clark <robdclark@...il.com>,
	linux-arm-kernel@...ts.infradead.org,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, Mark Yao <mark.yao@...k-chips.com>
Subject: Re: [PATCH v7 03/11] pwm: add support for atmel-hlcdc-pwm device

On Wed, Oct 01, 2014 at 04:53:00PM +0200, Boris Brezillon wrote:
[...]
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index b800783..afb896b 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -50,6 +50,16 @@ config PWM_ATMEL
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-atmel.
>  
> +config PWM_ATMEL_HLCDC_PWM
> +	tristate "Atmel HLCDC PWM support"
> +	select MFD_ATMEL_HLCDC
> +	depends on OF

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

> diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
[...]
> new file mode 100644
> index 0000000..0238f7a
> --- /dev/null
> +++ b/drivers/pwm/pwm-atmel-hlcdc.c
> @@ -0,0 +1,229 @@
> +/*
> + * Copyright (C) 2014 Free Electrons
> + * Copyright (C) 2014 Atmel
> + *
> + * Author: Boris BREZILLON <boris.brezillon@...e-electrons.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/mfd/atmel-hlcdc.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +#define ATMEL_HLCDC_PWMCVAL_MASK	GENMASK(15, 8)
> +#define ATMEL_HLCDC_PWMCVAL(x)		((x << 8) & ATMEL_HLCDC_PWMCVAL_MASK)

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 <boris.brezillon@...e-electrons.com>");
> +MODULE_DESCRIPTION("Atmel HLCDC PWM driver");
> +MODULE_LICENSE("GPL");

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

Thierry

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists