Re: [RFC PATCH] leds: add sgm3140 driver

[Luca Weiss]

Hi Pavel

On Sonntag, 8. März 2020 13:08:55 CET Pavel Machek wrote:
> Hi!
> 
> > Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
> 
> That's pinephone, right?

Yes

> 
> > This device is controller by two GPIO lines, one for enabling the LED
> > and the second one for switching between torch and flash mode.
> > 
> > The device will automatically switch to torch mode after being in flash
> > mode for about 250-300ms, so after that time the driver will turn the
> > LED off again automatically.
> 
> I don't quite see how this is supposed to work.
> 

The sgm3140 works like this:

Set EN pin to low  -> off
Set EN pin to high and FLASH pin low -> torch mode
Set EN pin to high and FLASH pin high -> flash mode for 200-300ms, then it 
switches automatically to torch mode

If it's still unclear, here's the datasheet which explains it nicely imo:
http://www.sg-micro.com/uploads/soft/20190626/1561535688.pdf

For strobe/flash operation the driver from this patch sets up a timer (using 
mod_timer) in sgm3140_strobe_set to turn off the flash completely after 250ms so 
that it doesn't remain in torch mode after these 200-300ms. In case the strobe 
is disabled earlier, the timer is cancelled (del_timer_sync). That's 
essentially what the driver is doing.

> > Hi, this driver is controllable via sysfs and v4l2 APIs (as documented
> > in Documentation/leds/leds-class-flash.rst).
> > 
> > The following is possible:
> > 
> > # Torch on
> > echo 1 > /sys/class/leds/white\:flash/brightness
> > # Torch off
> > echo 0 > /sys/class/leds/white\:flash/brightness
> > # Activate flash
> > echo 1 > /sys/class/leds/white\:flash/flash_strobe
> 
> So.. "activate flash" will turn the LED on in very bright mode, then
> put it back to previous brightness after a timeout?
> 
> What happens if some kind of malware does flash_strobe every 300msec?
> 

Then the LED will essentially be in torch mode until the sgm3140 determines 
that it can use flash mode again.

> > # Torch on
> > v4l2-ctl -d /dev/video1 -c led_mode=2
> > # Torch off
> > v4l2-ctl -d /dev/video1 -c led_mode=0
> > # Activate flash
> > v4l2-ctl -d /dev/video1 -c strobe=1
> > 
> > Unfortunately the last command (enabling the 'flash' via v4l2 results in
> > 
> > the following being printed and nothing happening:
> >   VIDIOC_S_EXT_CTRLS: failed: Resource busy
> >   strobe: Resource busy
> > 
> > Unfortunately I couldn't figure out the reason so I'm hoping to get some
> > guidance for this. iirc it worked at some point but then stopped.
> 
> Actually, LED flash drivers are getting quite common. Having common
> code (so we could just say this is led flash, register it to both v4l
> and LED) might be quite interesting.
> 
> Unfortunately, some LED flashes also have integrated red LED for
> indication, further complicating stuff.
> 

See https://www.kernel.org/doc/html/latest/leds/leds-class-flash.html ? That's 
what I am using in this driver.

> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index 2da39e896ce8..38d57dd53e4b 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -85,6 +85,7 @@ obj-$(CONFIG_LEDS_LM3601X)		+= leds-
lm3601x.o
> > 
> >  obj-$(CONFIG_LEDS_TI_LMU_COMMON)	+= leds-ti-lmu-common.o
> >  obj-$(CONFIG_LEDS_LM3697)		+= leds-lm3697.o
> >  obj-$(CONFIG_LEDS_LM36274)		+= leds-lm36274.o
> > 
> > +obj-$(CONFIG_LEDS_SGM3140)		+= leds-sgm3140.o
> 
> I would not mind "flash" drivers going to separate directory.
> 

That would apply to these existing drivers as well at least:
* drivers/leds/leds-aat1290.c
* drivers/leds/leds-as3645a.c
* drivers/leds/leds-max77693.c
* drivers/leds/leds-lm3601x.c (probably should be made to use v4l2_flash_init 
as well)

> > +int sgm3140_brightness_set(struct led_classdev *led_cdev,
> > +			   enum led_brightness brightness)
> > +{
> > +	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
> > +	struct sgm3140 *priv = flcdev_to_sgm3140(fled_cdev);
> > +
> > +	if (brightness == LED_OFF)
> > +		gpiod_set_value_cansleep(priv->enable_gpio, 0);
> > +	else
> > +		gpiod_set_value_cansleep(priv->enable_gpio, 1);
> > +
> > +	return 0;
> > +}
> 
> Umm. So this cancels running strobe?
> 

Setting brightness to 0 here turns off the flash, yes.

> > +static void sgm3140_powerdown_timer(struct timer_list *t)
> > +{
> > +	struct sgm3140 *priv = from_timer(priv, t, powerdown_timer);
> > +
> > +	gpiod_set_value_cansleep(priv->enable_gpio, 0);
> > +	gpiod_set_value_cansleep(priv->flash_gpio, 0);
> > +}
> 
> And this does not return to previous brightness.
> 

There's no real "brightness" level, it's either on or off. Or do you mean that 
when the torch is on and the strobe is activated it should go back to torch 
mode instead of being turned off?

> Do we want to provide the "strobe" functionality through sysfs at all?
> Should we make it v4l-only, and independend of the LED stuff?
> 

I've just followed 
https://www.kernel.org/doc/html/latest/leds/leds-class-flash.html , but I like 
the simple sysfs interface for simple uses and for more advanced applications 
(e.g. camera apps) the v4l2 interface.

> Best regards,
> 								
	Pavel

Regards
Luca