Re: [PATCH v4 13/20] pwm: Add new pwm-samsung driver

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

On 07/20/2013 02:04 AM, Tomasz Figa wrote:
> This patch introduces new Samsung PWM driver, which uses Samsung
> PWM/timer master driver to control shared parts of the hardware.
>
> Signed-off-by: Tomasz Figa<tomasz.figa@gmail.com>
> ---
>   drivers/pwm/Makefile      |   1 +
>   drivers/pwm/pwm-samsung.c | 606 ++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 607 insertions(+)
>   create mode 100644 drivers/pwm/pwm-samsung.c
[...]
> +static const struct samsung_pwm_variant s3c64xx_variant = {
> +	.bits		= 32,
> +	.div_base	= 0,

Initialization to 0 could be omitted, since it is implicit.

> +	.has_tint_cstat	= true,
> +	.tclk_mask	= BIT(7) | BIT(6) | BIT(5),
> +};
> +
> +static const struct samsung_pwm_variant s5p64x0_variant = {
> +	.bits		= 32,
> +	.div_base	= 0,

Ditto.

> +	.has_tint_cstat	= true,
> +};
> +
> +static const struct samsung_pwm_variant s5p_variant = {
> +	.bits		= 32,
> +	.div_base	= 0,

Ditto.

> +	.has_tint_cstat	= true,
> +	.tclk_mask	= BIT(5),
> +};
> +
[...]
> +static int pwm_samsung_remove(struct platform_device *pdev)
> +{
> +	struct samsung_pwm_chip *chip = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = pwmchip_remove(&chip->chip);
> +	if (ret<  0)
> +		return ret;

Since return value of the remove() callback is happily ignored by
the driver core I think there is no point in returning an error
here like this. Wouldn't it be more sensible to just call all the
cleanup functions unconditionally ? At least potentially wrong state
of the clock could be avoided.

> +
> +	clk_disable_unprepare(chip->base_clk);
> +
> +	return 0;
> +}

--
Regards,
Sylwester