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
