linux-kernel - Re: [PATCH v7 03/11] pwm: add support for atmel-hlcdc-pwm device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141006122856.GB26833@ulmo>
Date:	Mon, 6 Oct 2014 14:28:57 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Boris Brezillon <boris.brezillon@...e-electrons.com>
Cc:	David Airlie <airlied@...ux.ie>, dri-devel@...ts.freedesktop.org,
	Nicolas Ferre <nicolas.ferre@...el.com>,
	Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
	Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
	Andrew Victor <linux@...im.org.za>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Lee Jones <lee.jones@...aro.org>, linux-pwm@...r.kernel.org,
	Rob Clark <robdclark@...il.com>,
	linux-arm-kernel@...ts.infradead.org,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, Mark Yao <mark.yao@...k-chips.com>
Subject: Re: [PATCH v7 03/11] pwm: add support for atmel-hlcdc-pwm device

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@...il.com> 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

Content of type "application/pgp-signature" skipped