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:
signature.asc
Description: PGP signature
