Re: [RFC PATCH] leds: add sgm3140 driver

[Jacek Anaszewski]

On 3/8/20 1:08 PM, Pavel Machek wrote:
> Hi!
> 
>> Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
> 
> That's pinephone, right?
> 
>> 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.
> 
>> 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?
> 
>> # 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.

Everything you're talking about here is already implemented
and Luca makes use of that.

Indicator LEDs are covered as well.

>> 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.
> 
>> +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?
> 
>> +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.
> 
> Do we want to provide the "strobe" functionality through sysfs at all?
> Should we make it v4l-only, and independend of the LED stuff?

It was being discussed six years ago and the interface is in place.

Have you looked at drivers/leds/led-class-flash.c?

-- 
Best regards,
Jacek Anaszewski