linux-kernel - Re: [PATCH] pwm: berlin: Add PM support

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] [day] [month] [year] [list]

Date:	Thu, 26 Nov 2015 15:41:09 +0800
From:	Jisheng Zhang <jszhang@...vell.com>
To:	Thierry Reding <thierry.reding@...il.com>
CC:	<sebastian.hesselbarth@...il.com>,
	<antoine.tenart@...e-electrons.com>, <linux-pwm@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] pwm: berlin: Add PM support

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 <jszhang@...vell.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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/