On Mon, Oct 06, 2014 at 01:50:09PM +0200, Boris Brezillon wrote:
> On Mon, 6 Oct 2014 12:46:35 +0200 Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
> > On Wed, Oct 01, 2014 at 04:53:00PM +0200, Boris Brezillon wrote:
[...]
> > > + if (pres > ATMEL_HLCDC_PWMPS_MAX)
> > > + return -EINVAL;
> >
> > I think the condition above needs to be "pres == ATMEL_HLCDC_PWMPS_MAX",
> > otherwise this will never be true.
>
> Actually the previous loop is:
>
> for (pres = 0; pres *<=* ATMEL_HLCDC_PWMPS_MAX; pres++)
>
> thus pres will be equal to ATMEL_HLCDC_PWMPS_MAX + 1 when no
> appropriate prescaler is found.
Indeed so.
> > > + regmap_update_bits(hlcdc->regmap, ATMEL_HLCDC_CFG(0),
> > > + ATMEL_HLCDC_CLKPWMSEL, gencfg);
> > > + }
> > > +
> > > + do_div(pwmcval, period_ns);
> > > + if (pwmcval > 255)
> >
> > The PWM core already makes sure that duty_ns <= period_ns, so pwmcval
> > could be anywhere between 0 and 256 here. Where does the disconnect come
> > from? Why not make pwmcval = duty_ns * 255 if that's the maximum?
>
> Here is what the datasheet says:
>
> "Due to the comparison mechanism, the output pulse has a width between
> zero and 255 PWM counter cycles. Thus by adding a simple passive filter
> outside the chip, an analog voltage between 0 and (255/256) × VDD can
> be obtained (for the positive polarity case, or between (1/256) × VDD
> and VDD for the negative polarity case). Other voltage values can be
> obtained by adding active external circuitry."
>
> Given this explanation we should divide by 256, but 256/256 is a
> forbidden value, hence I just use the maximum available one (255) when
> I'm asked to configure a duty cycle occupying the whole period.
Okay, perhaps you can summarize the above explanation from the datasheet
in a comment to clarify.
> > > + pwmcval = 255;
> > > +
> > > + pwmcfg |= ATMEL_HLCDC_PWMCVAL(pwmcval);
> > > +
> > > + regmap_update_bits(hlcdc->regmap, ATMEL_HLCDC_CFG(6),
> > > + ATMEL_HLCDC_PWMCVAL_MASK | ATMEL_HLCDC_PWMPS_MASK,
> > > + pwmcfg);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int atmel_hlcdc_pwm_set_polarity(struct pwm_chip *c,
> > > + struct pwm_device *pwm,
> > > + enum pwm_polarity polarity)
> > > +{
> > > + struct atmel_hlcdc_pwm_chip *chip =
> > > + pwm_chip_to_atmel_hlcdc_pwm_chip(c);
> > > + struct atmel_hlcdc *hlcdc = chip->hlcdc;
> > > + u32 cfg = 0;
> > > +
> > > + if (polarity == PWM_POLARITY_NORMAL)
> > > + cfg = ATMEL_HLCDC_PWMPOL;
> >
> > That's strange. Inverse polarity is the default on this hardware?
>
> Quote from the datasheet:
>
> "
> • PWMPOL: LCD Controller PWM Signal Polarity
> This bit defines the polarity of the PWM output signal. If set to one,
> the output pulses are high level (the output will be high when- ever
> the value in the counter is less than the value CVAL) If set to zero,
> the output pulses are low level.
> "
>
> My understanding is that ATMEL_HLCDC_PWMPOL should be set when using
> normal polarity (and my tests confirm that it works as expected ;-)).
Yes, sounds good then.
Thierry
Attachment:
pgpDwUmbR4P3F.pgp
Description: PGP signature
