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

[Thierry Reding]

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.

> >>+static void pwm_backlight_enable(struct backlight_device *bl)
> >>+{
> >>+	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
> >>+
> >>+	pwm_enable(pb->pwm);
> >>+
> >>+	if (pb->en_supply && !pb->en_supply_enabled) {
> >>+		if (regulator_enable(pb->en_supply) != 0)
> >>+			dev_warn(&bl->dev, "Failed to enable regulator");
> >>+		else
> >>+			pb->en_supply_enabled = true;
> >>+	}
> >>+}
> >>+
> >>+static void pwm_backlight_disable(struct backlight_device *bl)
> >>+{
> >>+	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
> >>+
> >>+	if (pb->en_supply && pb->en_supply_enabled) {
> >>+		if (regulator_disable(pb->en_supply) != 0)
> >>+			dev_warn(&bl->dev, "Failed to disable regulator");
> >>+		else
> >>+			pb->en_supply_enabled = false;
> >>+	}
> >>+
> >>+	pwm_disable(pb->pwm);
> >>+}
> >
> >Alex already brought this up: shouldn't the sequences be reversed. That
> >is, when enabling the backlight, turn on the regulator first, then
> >enable the PWM. When disabling, disable the PWM first, then turn off the
> >regulator?
> 
> Actually the current sequence seems to make sense - the PWM is
> always active when the enable GPIO is switched. If we do the
> contrary, we might have a short time where the backlight is enabled
> without receiving anything from the PWM. Don't think that would be
> serious, but the current behavior is similar to e.g. panels which we
> enable only after a signal is available.

Okay, let's leave it like that then.

> >>@@ -213,6 +238,13 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> >>  	pb->exit = data->exit;
> >>  	pb->dev = &pdev->dev;
> >>
> >>+	pb->en_supply = devm_regulator_get(&pdev->dev, "enable");
> >>+	if (IS_ERR(pb->en_supply)) {
> >>+		ret = PTR_ERR(pb->en_supply);
> >>+		pb->en_supply = NULL;
> >>+		goto err_alloc;
> >>+	}
> >
> >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.

Thierry

Attachment: pgp00000.pgp
Description: PGP signature