[PATCH 1/2] PWM: ECAP: PWM driver support for ECAP APWM.

[Thierry Reding]

On Tue, Jul 24, 2012 at 07:52:06AM +0000, Philip, Avinash wrote:
> On Mon, Jul 23, 2012 at 19:07:04, Thierry Reding wrote:
> > On Mon, Jul 23, 2012 at 12:44:38PM +0000, Philip, Avinash wrote:
> > > On Mon, Jul 23, 2012 at 14:52:04, Thierry Reding wrote:
> > > > On Mon, Jul 23, 2012 at 09:10:09AM +0000, Philip, Avinash wrote:
> > > > > On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote:
> > > > > > On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote:
> > > > > > > +	/*
> > > > > > > +	 * Enable 'Free run Time stamp counter mode' to start counter
> > > > > > > +	 * and  'APWM mode' to enable APWM output
> > > > > > > +	 */
> > > > > > > +	reg_val = readw(pc->mmio_base + ECCTL2);
> > > > > > > +	reg_val |= ECCTL2_TSCTR_FREERUN | ECCTL2_APWM_MODE;
> > > > > > 
> > > > > > You already set the APWM_MODE bit in .config(), why is it needed here
> > > > > > again? Seeing that .disable() clears the bit as well, perhaps leaving it
> > > > > > clear in .config() is the better option.
> > > > > 
> > > > > Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low 
> > > > > in idle state).
> > > > > 
> > > > > The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2)
> > > > > During PWM configuration. To enable loading from Shadow registers, APWM mode 
> > > > > should be set.
> > > > > The same is done in .config().
> > > > 
> > > > My point was that if you do it in .enable(), then why do you still set
> > > > it in .config()? Since the sequence is typically .config() followed by
> > > > .enable(), setting the bit in both seems redundant. It should be enough
> > > > to load the registers during .enable(), right?
> > > 
> > > Consider scenarios where .enable() can be called without calling .config().
> > > Example I just need to stop the pwm signal momentarily and re-enable.
> > > In this case, .config() need not be called. So, APWM mode bit needs to be 
> > > set in .enable()
> > > 
> > > Now, considering .config() followed by .enable().
> > > Currently in PWM driver, the CAP1 & CAP2 registers is copied to shadow 
> > > Registers (CAP3 & CAP4) by hardwaare in .config(). This requires APWM
> > > mode bit to be set. 
> > > 
> > > The same can be done in .enable() also. However, we again need to pass 
> > > the pwm parameters (period & duty cycle values) to the .enable(). 
> > > We don't want to duplicate this parameter passing. 
> > > To avoid this we enable the APWM mode bit in both .config() & .enable().
> > 
> > That's weird. I assumed that the values written to the shadow registers
> > would automatically be copied to the active registers on each new PWM
> > period.
> 
> This is correct in case if PWM is running.
> 
> > If I understand correctly what you're saying, however, the eCAP
> > requires the APWM bit to be set in order to write the shadow registers
> > at all.
> 
> In APWM mode, writing to CAP1/CAP2 (active registers) will also write the
> same value to the corresponding CAP3/CAP4 (shadow registers). This emulates
> immediate mode. 
> 
> Writing directly to the shadow registers CAP3/CAP4 will invoke the
> shadow mode.
> 
> If PWM is not running, cycle & duty values written to active registers, not
> to shadow registers. To copy these value to shadow registers APWM mode to be
> set. This way reloading from shadow registers can be ensured on next PWM
> period/cycle.

I think this is the part I don't understand. If you wrote cycle and duty
values to the active registers already, then the shadow registers should
be ignored by the hardware. There should be no need to reload the active
registers.

> If PWM is running, cycle & duty values written to shadow registers. So that
> it won't disturb current PWM period. On the next PWM period, new values will
> be reloaded from shadow registers to active registers.
> 
> So APWM mode to be set to copy shadow registers from active registers
> (immediate mode).

I realize that I'm just talking from a general understanding of shadow
registers and maybe you're hardware doesn't work quite that way. If this
is really the only way that you can make the hardware work then I will
no longer object.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120724/41985e23/attachment-0001.sig>