On Mon, Oct 06, 2014 at 01:50:09PM +0200, Boris Brezillon wrote:

On Mon, 6 Oct 2014 12:46:35 +0200 Thierry Reding [off-list ref] wrote:

quoted

On Wed, Oct 01, 2014 at 04:53:00PM +0200, Boris Brezillon wrote:

[...]

quoted

quoted

+	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.

quoted

quoted

+		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.

quoted

quoted

+		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