Hello Uwe,
Thank you for the review!
> From: Uwe Kleine-Konig, Sent: Tuesday, January 8, 2019 4:44 PM
<snip>
> > +static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> > + struct pwm_state cur_state;
> > + int div, ret;
> > +
> > + /* This HW doesn't support changing polarity */
> > + pwm_get_state(pwm, &cur_state);
> > + if (state->polarity != cur_state.polarity)
> > + return -ENOTSUPP;
>
> Does the driver only support normal polarity or only inversed polarity?
> If so checking against that would be more clear here.
This driver only supports normal polarity. So, I'll change the code as the following:
/* This HW/driver only support normal polarity */
pwm_get_state(pwm, &cur_state);
if (state->polarity != PWM_POLARITY_NORMAL)
return -ENOTSUPP;
> > + div = rcar_pwm_get_clock_division(rp, state->period);
> > + if (div < 0)
> > + return div;
> > +
> > + rcar_pwm_update(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
> > +
> > + ret = rcar_pwm_set_counter(rp, div, state->duty_cycle, state->period);
> > + if (!ret)
> > + rcar_pwm_set_clock_control(rp, div);
> > +
> > + /* The SYNC should be set to 0 even if rcar_pwm_set_counter failed */
> > + rcar_pwm_update(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
> > +
> > + if (!ret && state->enabled)
> > + ret = rcar_pwm_enable(chip, pwm);
> > +
> > + if (!state->enabled) {
> > + rcar_pwm_disable(chip, pwm);
> > + ret = 0;
> > + }
>
> Assume the PWM runs with duty cycle 33% when
> pwm_apply_state({ .enabled=0, .duty_cycle=66, .period=100 }) is called.
>
> Does this might result in a 66% wave form being emitted? If yes, this
> needs fixing.
Thank you for the pointing it out. This code is possible to output 66% wave form
between the second rcar_pwm_update() and rcar_pwm_disable(). So, I'll fix this.
Best regards,
Yoshihiro Shimoda
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
