> From: Thierry Reding [mailto:thierry.red...@avionic-design.de] > Sent: Thursday, March 07, 2013 3:27 AM > To: Alex Courbot > Cc: Andrew Chew; linux-kernel@vger.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 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/
