linux-kernel - Re: [PATCH 1/2] pwm: add period and polarity to struct pwm

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

Message-ID: <20140409205246.GA2879@piout.net>
Date:	Wed, 9 Apr 2014 22:52:46 +0200
From:	Alexandre Belloni <alexandre.belloni@...e-electrons.com>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
Cc:	Thierry Reding <thierry.reding@...il.com>,
	linux-pwm@...r.kernel.org, linux-sh@...r.kernel.org,
	Tony Lindgren <tony@...mide.com>,
	Magnus Damm <magnus.damm@...il.com>,
	linux-kernel@...r.kernel.org, Simon Horman <horms@...ge.net.au>,
	Philipp Zabel <philipp.zabel@...il.com>,
	Paul Parsons <lost.distance@...oo.com>,
	linux-omap@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/2] pwm: add period and polarity to struct pwm_lookup

On 09/04/2014 at 20:37:06 +0100, Russell King - ARM Linux wrote :
> On Wed, Apr 09, 2014 at 08:04:08PM +0200, Alexandre Belloni wrote:
> > Adds a period and a polarity member to struct pwm_lookup so that when performing
> > a lookup using the lookup table instead of device tree, we are able to set the
> > period and the polarity accordingly like what is done in
> > of_pwm_xlate_with_flags.
> > 
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@...e-electrons.com>
> > ---
> >  drivers/pwm/core.c  | 5 +++++
> >  include/linux/pwm.h | 2 ++
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index a80471399c20..206e5996359c 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -663,6 +663,11 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
> >  
> >  	if (chip)
> >  		pwm = pwm_request_from_chip(chip, index, con_id ?: dev_id);
> > +	if (IS_ERR(pwm))
> > +		return pwm;
> > +
> > +	pwm_set_period(pwm, p->period);
> > +	pwm_set_polarity(pwm, p->polarity);
> >  
> >  	mutex_unlock(&pwm_lookup_lock);
> 
> Clearly, this is not right.  Returning while leaving the mutex locked?
> No.
> 

Sure, I will fix that crap, sorry about that and thanks for pointing it
out.

> The second issue is... with _just_ this patch applied, we end up with
> "period" and "polarity" presumably initialised to zero, which means we
> now end up with the above explicitly setting the period and polarity as
> such.  Isn't that going to change the behaviour of this?
> 

I actually checked that.

For the polarity, for now, it is assumed that it is normal unless
specified otherwise.
The only driver that was supporting inverting it using platform_data is
pwm-renesas-tpu. It is used by board-armadillo800eva.c that I am
modifying now (and I just now realise that I forgot to invert it).

The only PWM controller that I know of that by default has its polarity
inversed is the allwinner one and in the driver I submitted, I actually
switch it to normal polarity in the probe instead of e.g. doing
pwm->polarity = PWM_POLARITY_INVERSED;

For the period, all the driver are assuming 0 after initialization.

I think this is not specified. If you think that may be a concern then I
suggest creating another macro and using a bitfield to know which value
is set.

I would also argue that when using device tree,
of_pwm_xlate_with_flags() will set the period and the polarity
unconditionally, this is replicating that behaviour.

However, I could agree that we may need to test for
pwm->chip->ops->set_polarity before calling pwm_set_polarity as we will
get an error if it is NULL (but we actually discard that return value).

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
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/