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

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- Attachment #1: Type: text/plain, Size: 1517 bytes --]

On Wed, Mar 06, 2013 at 01:53:27PM +0900, Alex Courbot wrote:
> On 03/06/2013 11:41 AM, Andrew Chew wrote:
> >>>   struct pwm_bl_data {
> >>>   	struct pwm_device	*pwm;
> >>>   	struct device		*dev;
> >>>+	struct regulator	*en_supply;
> >>>+	bool			en_supply_enabled;
> >>
> >>Couldn't you use regulator_is_enabled() and get rid of en_supply_enabled?
> >>It would also ensure the driver performs correctly no matter what the initial
> >>state of the regulator is.
> >
> >Are you sure this works?  I'm concerned about the (bizarre and unlikely) case
> >where this supply is shared with another driver, so I use en_supply_enabled
> >to track the state of the supply such that I can ignore that case.
> 
> You're right, consumers can share regulators and the calls to
> enable/disable need to be balanced. Also there is no way to check
> the intensity of the backlight prior to the change to detect a
> transition, so I guess your approach is indeed the most appropriate
> here.

I think the right thing to do here is just enable the regulator when
the pwm-backlight driver needs it. If it is shared with other devices
they'll have to do the same and the reference counting should only
disable the regulator when there are no users.

Tracking this via platform data won't work because platform data is
statically defined at compile time. So if indeed there was another user
of the regulator it enable/disable the regulator at any time and your
en_supply_enabled would be wrong.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]