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@xxxxxxxxxxxxxxxxxx>
> Tested-by: Anthony Harivel <anthony.harivel@xxxxxxxxxx>
> Tested-by: Ludovic Desroches <ludovic.desroches@xxxxxxxxx>
> Acked-by: Thierry Reding <thierry.reding@xxxxxxxxx>
> ---
> 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:
pgp7Lqxopqi7l.pgp
Description: PGP signature
