Re: [PATCH v5 2/2] pwm: Add support for R-Car PWM Timer

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

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

Sorry for taking an awful long time to get around to this. The driver
looks generally okay, but I have a few minor comments...

On Mon, Jun 15, 2015 at 06:08:44PM +0900, Yoshihiro Shimoda wrote:
> This patch adds support for R-Car SoCs PWM Timer.

This could be a little more verbose. You could say for example how many
channels the driver exposes, or mention typical use-cases (if any).

> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
[...]
> +static int rcar_pwm_get_clock_division(struct rcar_pwm_chip *rp, int period_ns)
> +{
> +	int div;

Can be unsigned int.

> +	unsigned long clk_rate = clk_get_rate(rp->clk);
> +	unsigned long long max;	/* max cycle / nanoseconds */
> +
> +	if (!clk_rate)

I prefer it when these are explicit: clk_rate == 0. This goes for
numerical comparisons. For booleans, or NULL pointer comparisons the
!expression is fine.

> +static int rcar_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   int duty_ns, int period_ns)
> +{
> +	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> +	int div = rcar_pwm_get_clock_division(rp, period_ns);
> +	int ret;
> +
> +	if (div < 0)
> +		return div;
> +
> +	/* Let the core driver set pwm->period if disabled and duty_ns == 0 */
> +	if (!test_bit(PWMF_ENABLED, &pwm->flags) && !duty_ns)
> +		return 0;
> +
> +	rcar_pwm_bit_modify(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
> +	ret = rcar_pwm_set_counter(rp, div, duty_ns, period_ns);
> +	rcar_pwm_set_clock_control(rp, div);
> +	rcar_pwm_bit_modify(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
> +
> +	return ret;
> +}
> +
> +static int rcar_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> +	u32 pwmcnt;
> +
> +	/* Don't enable the PWM device if CYC0 or PH0 is 0 */
> +	pwmcnt = rcar_pwm_read(rp, RCAR_PWMCNT);
> +	if (!(pwmcnt & RCAR_PWMCNT_CYC0_MASK) ||
> +	    !(pwmcnt & RCAR_PWMCNT_PH0_MASK))
> +		return -EINVAL;

This looks wrong. Any errors in configuration should've been caught by
the ->config() implementation. Why can't you return -EINVAL on this
condition in ->config()? ->enable() failing should only be the case if
truly the PWM can't be enabled, not if it's badly configured.

> +static struct platform_driver rcar_pwm_driver = {
> +	.probe		= rcar_pwm_probe,
> +	.remove		= rcar_pwm_remove,
> +	.driver		= {
> +		.name	= "pwm-rcar",
> +		.of_match_table = of_match_ptr(rcar_pwm_of_table),
> +	}
> +};

This doesn't need the artificial padding. A single space around = is
enough.

Thierry

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