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().
>
> > > > + pc->chip.dev = &pdev->dev;
> > > > + pc->chip.ops = &ecap_pwm_ops;
> > > > + pc->chip.base = -1;
> > > > + pc->chip.npwm = 1;
> > >
> > > The cover letter mentions that the AM335x has 3 instances of the ECAP.
> > > Is there any chance that they can be unified in one driver instance
> > > (i.e. .npwm = 3?).
> >
> > No. All instances have separate resources (clocks, memory ..).
> >
> > >
> > > > +
> > > > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ecap_reg");
> > > > + if (r == NULL) {
> > >
> > > You use "if (!ptr)" everywhere else, maybe you should make this
> > > consistent?
> > Ok
> > > Also the platform_get_resource_byname() is really only
> > > useful for devices that have several register regions associated with
> > > them. I don't know your hardware in detail, but I doubt that a PWM
> > > device has more than one register region.
> >
> > In fact PWM Module has 4 different register space (eCAP, eHRPWM, eQEP &
> > Common Config space). So I need to use platform_get_resource_byname()
>
> Above you say that all instances have separate resources, so how come
> you need to specify 4 register spaces? The eCAP registers should clearly
> be passed to the eCAP device, while the eHRPWM registers should go to
> the eHRPWM device.
>
> My point is that if you need to refer to the register region by name,
> then the driver should clearly be using more than a single region.
> Neither the eCAP nor eHRPWM do that.
On AM335x SoC, this common config space is shared by the other 3
modules (eCAP, eHRPWM, eQEP).
However, on Davinci platform don't have any common config space.
So from driver reusability view, usage of platform_get_resource_byname()
is better choice.
Thanks
Avinash
>
> Thierry
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
