On Thu, Dec 13, 2012 at 01:38:28PM +1100, NeilBrown wrote:
> On Wed, 12 Dec 2012 12:31:45 +0100 Thierry Reding
> <thierry.reding@xxxxxxxxxxxxxxxxx> wrote:
>
> > On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
[...]
> > > + struct omap_dm_timer *dm_timer;
> > > + unsigned int polarity;
> >
> > The PWM subsystem already has enum pwm_polarity for this.
> >
>
> I'll use that then .... and as there is a pwm_set_polarity() interface, that
> probably means that I don't need to configure the polarity via the platform
> data? That would be a lot cleaner.
I guess the answer to that question is: it depends. If the user can set
the polarity (via platform or other means), then yes, you don't have to
pass it in here. However there may be users that don't support setting
the polarity or there may even be situations where the PWM goes through
an additional inverter on the board and therefore doesn't need the
polarity inversed after all, even if the user driver requests it.
Generally though I think that it is up to the user drivers to take care
of this and call pwm_set_polarity() as appropriate, so yes, I don't
think you have to explicitly pass it via platform data at all.
> > > + if (omap->duty_ns == duty_ns &&
> > > + omap->period_ns == period_ns)
> > > + /* No change - don't cause any transients */
> > > + return 0;
> >
> > Note to self: this might be a candidate to put in the core.
>
> might be useful, though the core doesn't currently "know" the current values.
Yes, but that can be changed. PWM is still a very young subsystem and
I'm trying to be cautious not to add too much cruft to it unless it's
really worth it.
> > > + omap_dm_timer_set_pwm(omap->dm_timer,
> > > + !omap->polarity,
> > > + toggle,
> > > + trigger);
> >
> > This doesn't either. Also you should be explicit about the polarity
> > parameter, since enum pwm_polarity is an enum and therefore negating it
> > isn't very nice (it should work though).
> >
> > You could solve this by doing something like:
> >
> > if (omap->polarity == PWM_POLARITY_NORMAL)
> > polarity = 1;
> > else
> > polarity = 0;
>
> (omap->polarity == PWM_POLARITY_NORMAL)
>
> would have the same effect.
Yes, that should work as well. However I'm not a friend of using such
expressions in a function call. But since you'll probably be reworking
this anyway because of the pwm_set_polarity() comments from above you
might just want to stick the proper value into omap->polarity in your
.set_polarity() implementation and not need the extra negation here.
> > > +static int __devinit omap_pwm_probe(struct platform_device *pdev)
> >
> > No more __devinit, please.
>
> If you say so (having no idea what it did :-)
This was used to mark functions depending on whether HOTPLUG was enabled
or not. For instance functions marked __devinit could be discarded after
the init stage if HOTPLUG was disabled because it would be guaranteed to
not be called after the init stage. Recently however HOTPLUG was changed
to be always enabled because the gains were very small and most people
would get them wrong anyway.
> > > +#if CONFIG_PM
> > > +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state)
> > > +{
> > > + struct omap_chip *omap = platform_get_drvdata(pdev);
> > > + /* No one preserve these values during suspend so reset them
> > > + * Otherwise driver leaves PWM unconfigured if same values
> > > + * passed to pwm_config
> > > + */
> > > + omap->period_ns = 0;
> > > + omap->duty_ns = 0;
> > > +
> > > + return 0;
> > > +}
> > > +#else
> > > +#define omap_pwm_suspend NULL
> > > +#endif
> >
> > This doesn't look right. You should implement .resume() if you really
> > care, in which case the resume callback would have to reconfigure with
> > the cached values. In that case maybe you should switch to dev_pm_ops
> > and SIMPLE_DEV_PM_OPS() as well.
> >
> > If you don't, just resetting these values will not make the PWM work
> > properly after resume either since it will have to be explicitly
> > reconfigured.
>
> I just copied that from pwm-samsung.c
>
> I think the point is to avoid the "no transients" short-circuit in
> omap_pwm_config if the config is unchanged.
>
> The assumption is that pwm_disable() will be called before suspend and
> pwm_config()/pwm_enable() after resume. So there is no point actually
> configuring anything in .resume() - it makes sense to wait until pwm_config()
> is called (if ever). But we want to make sure that pwm_config actually does
> something.
Okay, that makes sense. User drivers should actually be better suited to
reset PWM devices to their proper state on resume.
> > > +MODULE_AUTHOR("Grant Erickson <marathon96@xxxxxxxxx>");
> > > +MODULE_AUTHOR("NeilBrown <neilb@xxxxxxx>");
> >
> > Shouldn't this be "Neil Brown"? I noticed you use the concatenated form
> > in the email address as well, so maybe that's on purpose?
>
> Yes, it is on purpose. With a surname like "Brown", one likes finding ways
> to be distinctive :-)
Hehe, alright then =).
Thierry
Attachment:
pgponoBcMhDBl.pgp
Description: PGP signature
