Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator

[Thierry Reding]

On Fri, Mar 08, 2013 at 11:21:04AM +0900, Alex Courbot wrote:
> On 03/08/2013 06:07 AM, Andrew Chew wrote:
> >>From: Thierry Reding [mailto:thierry.reding@xxxxxxxxxxxxxxxxx]
> >>Sent: Thursday, March 07, 2013 3:27 AM
> >>To: Alex Courbot
> >>Cc: Andrew Chew; linux-kernel@xxxxxxxxxxxxxxx
> >>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 think that's what Thierry meant, yes.

Yes, it is. =)

> >>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.
> 
> Yes, if you introduce incompatibilities you have the burden of
> performing the transition without breaking things at any single
> point of the git history. Since this is just about adding a dummy
> regulator, it should go fine even without testing. And in the event
> it does not, that's what linux-next is for.

Right. We'll need an Acked-by from the board/machine maintainers anyway
and if something still breaks we can always fix it after somebody's
actually done the testing.

> Make sure you also update the dts of current device tree users, as
> they will break, too.
> 
> What I don't know is if you should update all users in one big
> patch, or instead provide one patch per platform changed. Maybe
> Thierry can provide some guidance here.

I think it'd be good to split them up into per-architecture and
per-machine. Per-board would probably be too much. That'll allow the
respective maintainers to ack patches that touch their machines or
boards without having them go through all other hunks too.

Thierry

Attachment: pgp00000.pgp
Description: PGP signature