Messages in this thread | | | Date | Fri, 8 Mar 2013 08:23:00 +0100 | From | Thierry Reding <> | Subject | Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator |
| |
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@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 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 [unhandled content-type:application/pgp-signature] | |