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

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

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

On Mon, Jun 15, 2015 at 05:48:00AM +0000, Yoshihiro Shimoda wrote:
[...]
> > > +static void rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div,
> > > +				 int duty_ns, int period_ns)
> > > +{
> > > +	unsigned long long one_cycle, tmp;	/* 0.01 nanoseconds */
> > > +	unsigned long clk_rate = clk_get_rate(rp->clk);
> > > +	u32 cyc, ph;
> > > +
> > > +	one_cycle = (unsigned long long)NSEC_PER_SEC * 100ULL * (1 << div);
> > > +	do_div(one_cycle, clk_rate);
> > > +
> > > +	tmp = period_ns * 100ULL;
> > > +	do_div(tmp, one_cycle);
> > > +	cyc = (tmp << RCAR_PWMCNT_CYC0_SHIFT) & RCAR_PWMCNT_CYC0_MASK;
> > > +
> > > +	tmp = duty_ns * 100ULL;
> > > +	do_div(tmp, one_cycle);
> > > +	ph = tmp & RCAR_PWMCNT_PH0_MASK;
> > > +
> > > +	/* Avoid prohibited setting */
> > > +	if (cyc && ph)
> > > +		rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);
> > 
> > So if a period and duty-cycle are passed in that yield the prohibited
> > setting the operation will simply be silently ignored?
> 
> Yes, to update values of pwm->duty_cycle and ->period by pwm_config(),
> this code will be silently ignored.

That's not right. If someone passes in invalid values you should report
an error.

Also, are you sure the check is correct? The current check:

	if (cyc && ph)

is the same as

	if (cyc != 0 && ph != 0)

which means that you will never be able to set a zero duty cycle. Is
that really what you want here?

Thierry

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