Re: [PATCH v8 1/2] pwm: add support for atmel-hlcdc-pwm device

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3117 bytes --]

On Mon, Oct 06, 2014 at 04:07:02PM +0200, Boris Brezillon wrote:
> The HLCDC IP available in some Atmel SoCs (i.e. sam9x5i.e. at91sam9n12,
> at91sam9x5 family or sama5d3 family) provide a PWM device.
> 
> This driver add support for a PWM chip exposing a single PWM device (which
> will most likely be used to drive a backlight device).
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Tested-by: Anthony Harivel <anthony.harivel@emtrion.de>
> Tested-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> Acked-by: Thierry Reding <thierry.reding@gmail.com>
> ---
>  drivers/pwm/Kconfig           |   9 ++
>  drivers/pwm/Makefile          |   1 +
>  drivers/pwm/pwm-atmel-hlcdc.c | 248 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 258 insertions(+)
>  create mode 100644 drivers/pwm/pwm-atmel-hlcdc.c

Just noticed a couple more things.

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index b800783..9bb331b 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -50,6 +50,15 @@ 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
> +	help
> +	  Generic PWM framework driver for Atmel HLCDC PWM.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-atmel.

This should be "pwm-atmel-hlcdc".

> diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
[...]
> +static int atmel_hlcdc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm)
> +{
> +	struct atmel_hlcdc_pwm *chip = to_atmel_hlcdc_pwm(c);
> +	struct atmel_hlcdc *hlcdc = chip->hlcdc;
> +	u32 status;
> +	int ret;
> +
> +	regmap_write(hlcdc->regmap, ATMEL_HLCDC_EN, ATMEL_HLCDC_PWM);

I just noticed that regmap_write() can also fail. But perhaps that's
only for I2C or the like backends and you can indeed ignore it for MMIO
backends.

> +	do {
> +		usleep_range(1, 10);
> +		ret = regmap_read(hlcdc->regmap, ATMEL_HLCDC_SR, &status);
> +		if (ret)
> +			return ret;
> +	} while ((status & ATMEL_HLCDC_PWM) == 0);

A slightly better loop might be to do the sleep only after you've
determined that the status bit isn't set. That way you avoid a needless
sleep if the status bit is immediately set or an error occurs during
read.

	while (true) {
		ret = regmap_read(...);
		if (ret)
			return ret;

		if (status & ATMEL_HLCDC_PWM)
			break;

		usleep_range(1, 10);
	}

> +static int atmel_hlcdc_pwm_remove(struct platform_device *pdev)
> +{
> +	struct atmel_hlcdc_pwm *chip = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = pwmchip_remove(&chip->chip);
> +	if (ret)
> +		return ret;
> +
> +	clk_disable_unprepare(chip->hlcdc->periph_clk);

You might want to call clk_disable_unprepare() regardless of whether or
not pwmchip_remove() failed. You could simply leave out the above check
for ret and instead...

> +
> +	return 0;

"return ret;" here.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]