On Wed, 25 Nov 2015 16:16:27 +0100
Thierry Reding wrote:
> On Wed, Nov 25, 2015 at 04:30:19PM +0800, Jisheng Zhang wrote:
> > On Tue, 24 Nov 2015 17:23:06 +0100 Thierry Reding wrote:
> > > On Tue, Nov 24, 2015 at 01:43:05PM +0800, Jisheng Zhang wrote:
> > > > This patch adds S2R support for berlin pwm driver.
> > > >
> > > > Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
> > > > ---
> > > > drivers/pwm/pwm-berlin.c | 57
> > > > +++++++++++++++++++++++++++++++++++++++++++++++-
> > > > 1 file changed, 56 insertions(+), 1 deletion(-)
> [...]
> > > > + for (i = 0; i < pwm->chip.npwm; i++) {
> > > > + struct berlin_pwm_context *ctx = &pwm->ctx[i];
> > > > +
> > > > + berlin_pwm_writel(pwm, i, ctx->ctrl,
> > > > BERLIN_PWM_CONTROL);
> > > > + berlin_pwm_writel(pwm, i, ctx->duty, BERLIN_PWM_DUTY);
> > > > + berlin_pwm_writel(pwm, i, ctx->tcnt, BERLIN_PWM_TCNT);
> > > > + berlin_pwm_writel(pwm, i, ctx->enable,
> > > > BERLIN_PWM_ENABLE);
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static SIMPLE_DEV_PM_OPS(berlin_pwm_pm_ops, berlin_pwm_suspend,
> > > > + berlin_pwm_resume);
> > > > +#define BERLIN_PWM_PM_OPS (&berlin_pwm_pm_ops)
> > > > +#else
> > > > +#define BERLIN_PWM_PM_OPS NULL
> > > > +#endif
> > >
> > > This is a weird way of writing this. I think a more typical way would be
> > > to have the #ifdef contain only the implementation and then define the
> > > dev_pm_ops variable unconditonally, so you don't need a separate macro
> > > for it.
> > >
> >
> > The reason why I introduced one more macro is: struct dev_pm_ops contains
> > 23 pointers now, if there's no BERLIN_PWM_PM_OPS macro, there will be
> > always a
> > dev_pm_ops even if PM_SLEEP isn't enabled. I dunno whether there's any
> > elegant solution for this case.
>
> I wouldn't bother. PM_SLEEP is in almost all cases going to be enabled.
> If it isn't enabled it's likely going to be in test builds, at which
> point nobody will care about the extra 23 pointers.
>
> > How about define SIMPLE_DEV_PM_OPS as NULL if PM_SLEEP isn't enabled?
>
> That won't work, "static NULL;" wouldn't be valid syntax. Like I said,
> if you go through the trouble of implementing suspend/resume, you're
> almost certainly going to want to enable it, so just define it
> unconditionally.
>
Thanks for detailed explanation. In yesterday's v2, the BERLIN_PWM_PM_OPS
was removed.
Thanks for review,
Jisheng
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
