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

[Alex Courbot]

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.

> > +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.

> > @@ -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.

Alex.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/