Date:	Thu, 7 Mar 2013 13:07:51 -0800
From:	Andrew Chew <AChew@...dia.com>
To:	Thierry Reding <thierry.reding@...onic-design.de>,
	Alex Courbot <acourbot@...dia.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable
 regulator

> From: Thierry Reding [mailto:thierry.reding@...onic-design.de]
> Sent: Thursday, March 07, 2013 3:27 AM
> To: Alex Courbot
> Cc: Andrew Chew; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable
> regulator
> 
> * PGP Signed by an unknown key
> 
> On Thu, Mar 07, 2013 at 07:11:25PM +0900, Alex Courbot wrote:
> > On 03/07/2013 04:11 PM, Thierry Reding wrote:
> > >>+	bool			en_supply_enabled;
> > >
> > >This boolean can be dropped. As discussed in a previous thread, the
> > >pwm-backlight driver shouldn't need to know about any other uses of
> > >the regulator.
> >
> > Sorry for being obstinate - but I'm still not convinced we can get rid
> > of it. I checked the regulator code, and as you mentioned in the
> > previous version, calls to regulator_enable() and
> > regulator_disable() *must* be balanced in this driver.
> >
> > Without this variable we would call regulator_enable() every time
> > pwm_backlight_enable() is called (and same thing when disabling).
> > Now imagine the driver is asked to set the following intensities: 5,
> > 12, then 0. You would have two calls to regulator_enable() but only
> > one to regulator_disable(), which would result in the enable GPIO
> > remaining active even though it would be shut down. Or I missed
> > something obvious.
> >
> > The regulator must be enabled/disabled on transitions from/to 0, and
> > AFAICT there is no way for this driver to detect them.
> 
> Yes, that's true, but I don't think it should be solved for just this one
> regulator. Instead if we need to track the enable state we might as well track
> it for *any* resource so that the PWM isn't enabled/disabled twice either.

That makes sense, but I'm confused due to previous comments.  The most
obvious way to do this seems to be to have a bool track the enable state.
Do you still want me to do away with this bool?  I can satisfy your very
last comment by keeping the bool (renaming it to something more generic)
and encapsulating the pwm_enable()/pwm_disable() call within.
I'll send out v5 today to show what I mean.

> > >This effectively makes the regulator mandatory, so the board files
> > >that use pwm-backlight need to be updated or otherwise will break.
> >
> > Yes. Btw, should such changes go into the same patch? This seems
> > difficult to split without breaking things at some point.
> 
> I expect that if the changes are split up then the board-setup code changes
> need to be done prior to the driver change. Using the lookup tables should
> make this easy because they aren't tied to the platform data and can be
> added independently. The patches should probably go through the same
> subsystem tree to take care of the dependencies.
> 
> Keeping everything in one patch would work too, but it's certainly more
> chaotic.

Am I supposed to handle those patches?  I'm concerned that I don't have
hardware to test properly, but I can give it a shot if it's my responsibility.
--
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/

Powered by blists - more mailing lists