Re: [PATCH v7 2/2] leds: Add control of the voltage/current regulator to the LED core

[Tomi Valkeinen]

Hi,

On 04/12/2019 14:37, Pavel Machek wrote:
> Hi!
>
> > A LED is usually powered by a voltage/current regulator. Let the LED core
> > know about it. This allows the LED core to turn on or off the power supply
> > as needed.
> >
> > Because turning ON/OFF a regulator might block, it is not done
> > synchronously but done in a workqueue. Turning ON the regulator is
> > always

JJ had to leave this unfinished, and now I'm trying to pick this work up.

> How will this interact with LEDs that can be used from atomic context?

I think the idea here is that if the LED uses a regulator, and the regulator needs to be enabled, 
then the whole work is scheduled. If there's no regulator or if the regulator is already enabled, 
the brightness is set directly.

> > +static ssize_t regulator_auto_off_store(struct device *dev,
> > +		struct device_attribute *attr, const char *buf, size_t size)
> > +{
> > +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> > +	ssize_t ret = size;
> > +	bool auto_off;
> > +
> > +	if (strncmp(buf, "enable\n", size) == 0)
> > +		auto_off = true;
> > +	else if (strncmp(buf, "disable\n", size) == 0)
> > +		auto_off = false;
> > +	else
> > +		return -EINVAL;
>
> Sounds like device power management to me. Is it compatible with that?

Can you elaborate what you mean?

> > @@ -135,6 +203,8 @@ static void set_brightness_delayed(struct work_struct *ws)
> >   	    (led_cdev->flags & LED_HW_PLUGGABLE)))
> >   		dev_err(led_cdev->dev,
> >   			"Setting an LED's brightness failed (%d)\n", ret);
> > +
> > +	 led_handle_regulator(led_cdev);
> >   }
>
> You only modify set_brigthness_delays, so this will not work at all
> for non-blocking LEDs, right?

I'm not that familiar with led framework yet, but if setting of the brightness always goes via 
led_set_brightness_nopm(), then I think this would work for all LEDs, as led_set_brightness_nopm 
will schedule the work if needed (which goes to set_brightness_delayed).

> >   static void led_set_software_blink(struct led_classdev *led_cdev,
> > @@ -189,6 +259,7 @@ static void led_blink_setup(struct led_classdev *led_cdev,
> >   void led_init_core(struct led_classdev *led_cdev)
> >   {
> >   	INIT_WORK(&led_cdev->set_brightness_work, set_brightness_delayed);
> > +	INIT_DELAYED_WORK(&led_cdev->reg_off_work, turn_off_regulator_delayed);
> >   
>
> Could this re-use the workqueue? Many systems will not need
> regulators, so this is overhead...

You mean re-use the 'set_brightness_work' work? I'm not sure how that would be done, they're a bit 
different things. set_brightness_work is used to change the context from atomic to non-atomic, and 
reg_off_work is used to turn the regulator off after a specified delay.

> > +			/*
> > +			 * the regulator must  be turned off. This cannot be
>
> Use "The", and fix double spaces between must and be.
>
> > +		} else if (regulator_on && old == REG_R_OFF_U_OFF) {
> > +			/*
> > +			 * the regulator must be enabled. This cannot be here
>
> "The"
>
> > +		/*
> > +		 * small optimization. Cancel the work that had been started
>
> "Small."
>
> > +#include <linux/regulator/consumer.h>
> > +
> > +/*
> > + * The regulator state tracks 2 boolean variables:
> > + * - the state of regulator (or more precisely the state required by
> > + *   led core layer, as many users can interact with the same regulator).
> > + *   It is tracked by bit 0.
> > + * - the state last asked-for by the LED user. It is tracked by bit 1.
> > + */
> > +#define REG_R_ON BIT(0)
> > +#define REG_U_ON BIT(1)
> > +
> > +enum {	REG_R_OFF_U_OFF = 0,
> > +	REG_R_ON_U_OFF = REG_R_ON,
> > +	REG_R_OFF_U_ON = REG_U_ON,
> > +	REG_R_ON_U_ON = REG_R_ON | REG_U_ON
> > +};
>
> That's quite weird use of enum.

Yes, these could as well be defines, if that's what you mean.

> > +++ b/include/linux/leds.h
> > @@ -149,6 +149,15 @@ struct led_classdev {
> >   
> >   	/* Ensures consistent access to the LED Flash Class device */
> >   	struct mutex		led_access;
> > +
> > +	/* regulator */
>
> "Regulator".

Fixed this and the other cosmetic changes, thanks!

Overall, I wonder if all this is worth it as this is somewhat complex change, compared to just 
having the regulator always enabled...

 Tomi

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki