Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
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.
Signed-off-by: Luca Weiss <[email protected]>
---
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
# 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.
I will also write dt bindings for the driver once I have "strobe"
working.
Regards,
Luca
drivers/leds/Kconfig | 9 ++
drivers/leds/Makefile | 1 +
drivers/leds/leds-sgm3140.c | 210 ++++++++++++++++++++++++++++++++++++
3 files changed, 220 insertions(+)
create mode 100644 drivers/leds/leds-sgm3140.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 4b68520ac251..7c391af8b380 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -836,6 +836,15 @@ config LEDS_LM36274
Say Y to enable the LM36274 LED driver for TI LMU devices.
This supports the LED device LM36274.
+config LEDS_SGM3140
+ tristate "LED support for the SGM3140"
+ depends on LEDS_CLASS_FLASH
+ depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
+ depends on OF
+ help
+ This option enables support for the SGM3140 500mA Buck/Boost Charge
+ Pump LED Driver. It has supports flash and torch mode.
+
comment "LED Triggers"
source "drivers/leds/trigger/Kconfig"
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
# LED SPI Drivers
obj-$(CONFIG_LEDS_CR0014114) += leds-cr0014114.o
diff --git a/drivers/leds/leds-sgm3140.c b/drivers/leds/leds-sgm3140.c
new file mode 100644
index 000000000000..9e91392f0343
--- /dev/null
+++ b/drivers/leds/leds-sgm3140.c
@@ -0,0 +1,210 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2020 Luca Weiss <[email protected]>
+
+#include <linux/gpio/consumer.h>
+#include <linux/led-class-flash.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include <media/v4l2-flash-led-class.h>
+
+#define SGM3140_NAME "sgm3140"
+
+struct sgm3140 {
+ struct gpio_desc *flash_gpio;
+ struct gpio_desc *enable_gpio;
+
+ struct led_classdev_flash fled_cdev;
+ struct v4l2_flash *v4l2_flash;
+
+ struct timer_list powerdown_timer;
+};
+
+static struct sgm3140 *flcdev_to_sgm3140(struct led_classdev_flash *flcdev)
+{
+ return container_of(flcdev, struct sgm3140, fled_cdev);
+}
+
+int sgm3140_strobe_set(struct led_classdev_flash *fled_cdev, bool state)
+{
+ struct sgm3140 *priv = flcdev_to_sgm3140(fled_cdev);
+
+ if (state) {
+ gpiod_set_value_cansleep(priv->flash_gpio, 1);
+ gpiod_set_value_cansleep(priv->enable_gpio, 1);
+ mod_timer(&priv->powerdown_timer,
+ jiffies + msecs_to_jiffies(250));
+ } else {
+ gpiod_set_value_cansleep(priv->enable_gpio, 0);
+ gpiod_set_value_cansleep(priv->flash_gpio, 0);
+ del_timer_sync(&priv->powerdown_timer);
+ }
+
+ return 0;
+}
+
+struct led_flash_ops sgm3140_flash_ops = {
+ .strobe_set = sgm3140_strobe_set,
+};
+
+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;
+}
+
+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);
+}
+
+#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
+static void sgm3140_init_v4l2_flash_config(struct sgm3140 *priv,
+ struct v4l2_flash_config *v4l2_sd_cfg)
+{
+ struct led_classdev *led_cdev = &priv->fled_cdev.led_cdev;
+ struct led_flash_setting *s;
+
+ strlcpy(v4l2_sd_cfg->dev_name, led_cdev->dev->kobj.name,
+ sizeof(v4l2_sd_cfg->dev_name));
+
+ s = &v4l2_sd_cfg->intensity;
+ s->min = 0;
+ s->max = 1;
+ s->step = 1;
+ s->val = 1;
+}
+
+#else
+static void sgm3140_init_v4l2_flash_config(struct sgm3140 *priv,
+ struct v4l2_flash_config *v4l2_sd_cfg)
+{
+}
+#endif
+
+static int sgm3140_probe(struct platform_device *pdev)
+{
+ struct sgm3140 *priv;
+ struct led_classdev *led_cdev;
+ struct led_classdev_flash *fled_cdev;
+ struct led_init_data init_data = {};
+ struct device_node *child_node;
+ struct v4l2_flash_config v4l2_sd_cfg;
+ int ret;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->flash_gpio = devm_gpiod_get(&pdev->dev, "flash", GPIOD_OUT_LOW);
+ ret = PTR_ERR_OR_ZERO(priv->flash_gpio);
+ if (ret) {
+ if (ret != -EPROBE_DEFER)
+ dev_err(&pdev->dev, "Failed to request flash gpio: %d\n",
+ ret);
+ return ret;
+ }
+
+ priv->enable_gpio = devm_gpiod_get(&pdev->dev, "enable", GPIOD_OUT_LOW);
+ ret = PTR_ERR_OR_ZERO(priv->enable_gpio);
+ if (ret) {
+ if (ret != -EPROBE_DEFER)
+ dev_err(&pdev->dev, "Failed to request enable gpio: %d\n",
+ ret);
+ return ret;
+ }
+
+ child_node = of_get_next_available_child(pdev->dev.of_node, NULL);
+ if (!child_node) {
+ dev_err(&pdev->dev, "No DT child node found for connected LED.\n");
+ return -EINVAL;
+ }
+
+ timer_setup(&priv->powerdown_timer, sgm3140_powerdown_timer, 0);
+
+ fled_cdev = &priv->fled_cdev;
+ led_cdev = &fled_cdev->led_cdev;
+
+ fled_cdev->ops = &sgm3140_flash_ops;
+
+ led_cdev->brightness_set_blocking = sgm3140_brightness_set;
+ led_cdev->max_brightness = LED_ON;
+ led_cdev->flags |= LED_DEV_CAP_FLASH;
+
+ init_data.fwnode = of_fwnode_handle(child_node);
+ init_data.devicename = SGM3140_NAME;
+
+ platform_set_drvdata(pdev, priv);
+
+ /* Register in the LED subsystem */
+ ret = led_classdev_flash_register_ext(&pdev->dev, fled_cdev, &init_data);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Failed to register flash device: %d\n",
+ ret);
+ goto err_flash_register;
+ }
+
+ sgm3140_init_v4l2_flash_config(priv, &v4l2_sd_cfg);
+
+ /* Create V4L2 Flash subdev */
+ priv->v4l2_flash = v4l2_flash_init(&pdev->dev, of_fwnode_handle(child_node),
+ fled_cdev, NULL,
+ &v4l2_sd_cfg);
+ if (IS_ERR(priv->v4l2_flash)) {
+ ret = PTR_ERR(priv->v4l2_flash);
+ goto err_v4l2_flash_init;
+ }
+
+ return 0;
+
+err_v4l2_flash_init:
+ led_classdev_flash_unregister(fled_cdev);
+err_flash_register:
+ of_node_put(child_node);
+ return ret;
+}
+
+static int sgm3140_remove(struct platform_device *pdev)
+{
+ struct sgm3140 *priv = platform_get_drvdata(pdev);
+
+ del_timer_sync(&priv->powerdown_timer);
+
+ v4l2_flash_release(priv->v4l2_flash);
+ led_classdev_flash_unregister(&priv->fled_cdev);
+
+ return 0;
+}
+
+static const struct of_device_id sgm3140_dt_match[] = {
+ { .compatible = "sgmicro,sgm3140" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sgm3140_dt_match);
+
+static struct platform_driver sgm3140_driver = {
+ .probe = sgm3140_probe,
+ .remove = sgm3140_remove,
+ .driver = {
+ .name = "sgm3140",
+ .of_match_table = sgm3140_dt_match,
+ },
+};
+
+module_platform_driver(sgm3140_driver);
+
+MODULE_AUTHOR("Luca Weiss <[email protected]>");
+MODULE_DESCRIPTION("SG Micro SGM3140 charge pump led driver");
+MODULE_LICENSE("GPL v2");
--
2.25.1
Luca
On 2/27/20 12:50 PM, Luca Weiss wrote:
> Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
>
> 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.
>
> Signed-off-by: Luca Weiss <[email protected]>
> ---
You seem to be missing the devictree bindings doc for the GPIOs.
Dan
On Donnerstag, 27. Februar 2020 20:50:40 CET Dan Murphy wrote:
> Luca
>
> On 2/27/20 12:50 PM, Luca Weiss wrote:
> > Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
> >
> > 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.
> >
> > Signed-off-by: Luca Weiss <[email protected]>
> > ---
>
> You seem to be missing the devictree bindings doc for the GPIOs.
As written in the initial email:
> I will also write dt bindings for the driver once I have "strobe"
> working.
I was hoping to get some guidance on the code by posting the WIP patch - the
issues I see are documented in the initial email.
Regards
Luca
Luca
On 3/5/20 5:01 AM, Luca Weiss wrote:
> On Donnerstag, 27. Februar 2020 20:50:40 CET Dan Murphy wrote:
>> Luca
>>
>> On 2/27/20 12:50 PM, Luca Weiss wrote:
>>> Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
>>>
>>> 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.
>>>
>>> Signed-off-by: Luca Weiss <[email protected]>
>>> ---
>> You seem to be missing the devictree bindings doc for the GPIOs.
> As written in the initial email:
>
>> I will also write dt bindings for the driver once I have "strobe"
>> working.
NACK.
We review bindings as part of the code.? And we want to understand what
bindings the code will be using
You will get better guidance if the patch set contains the documentation
so we can understand what is being proposed.
Dan
Hi Luca,
Thank you for the patch.
On 2/27/20 7:50 PM, Luca Weiss wrote:
> Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
>
> 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.
>
> Signed-off-by: Luca Weiss <[email protected]>
> ---
> 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
>
> # 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
What is /dev/video1 ? Did you register vl42 flash subdev
in some v4l2 media controller device?
>
> 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.
You have to be in flash mode to strobe i.e. led_mode=1.
> I will also write dt bindings for the driver once I have "strobe"
> working.
>
> Regards,
> Luca
>
> drivers/leds/Kconfig | 9 ++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-sgm3140.c | 210 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 220 insertions(+)
> create mode 100644 drivers/leds/leds-sgm3140.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 4b68520ac251..7c391af8b380 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -836,6 +836,15 @@ config LEDS_LM36274
> Say Y to enable the LM36274 LED driver for TI LMU devices.
> This supports the LED device LM36274.
>
> +config LEDS_SGM3140
> + tristate "LED support for the SGM3140"
> + depends on LEDS_CLASS_FLASH
> + depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
> + depends on OF
> + help
> + This option enables support for the SGM3140 500mA Buck/Boost Charge
> + Pump LED Driver. It has supports flash and torch mode.
> +
> comment "LED Triggers"
> source "drivers/leds/trigger/Kconfig"
>
> 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
>
> # LED SPI Drivers
> obj-$(CONFIG_LEDS_CR0014114) += leds-cr0014114.o
> diff --git a/drivers/leds/leds-sgm3140.c b/drivers/leds/leds-sgm3140.c
> new file mode 100644
> index 000000000000..9e91392f0343
> --- /dev/null
> +++ b/drivers/leds/leds-sgm3140.c
> @@ -0,0 +1,210 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2020 Luca Weiss <[email protected]>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/led-class-flash.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#include <media/v4l2-flash-led-class.h>
> +
> +#define SGM3140_NAME "sgm3140"
> +
> +struct sgm3140 {
> + struct gpio_desc *flash_gpio;
> + struct gpio_desc *enable_gpio;
> +
> + struct led_classdev_flash fled_cdev;
> + struct v4l2_flash *v4l2_flash;
> +
> + struct timer_list powerdown_timer;
> +};
> +
> +static struct sgm3140 *flcdev_to_sgm3140(struct led_classdev_flash *flcdev)
> +{
> + return container_of(flcdev, struct sgm3140, fled_cdev);
> +}
> +
> +int sgm3140_strobe_set(struct led_classdev_flash *fled_cdev, bool state)
> +{
> + struct sgm3140 *priv = flcdev_to_sgm3140(fled_cdev);
> +
> + if (state) {
> + gpiod_set_value_cansleep(priv->flash_gpio, 1);
> + gpiod_set_value_cansleep(priv->enable_gpio, 1);
> + mod_timer(&priv->powerdown_timer,
> + jiffies + msecs_to_jiffies(250));
> + } else {
> + gpiod_set_value_cansleep(priv->enable_gpio, 0);
> + gpiod_set_value_cansleep(priv->flash_gpio, 0);
> + del_timer_sync(&priv->powerdown_timer);
> + }
> +
> + return 0;
> +}
> +
> +struct led_flash_ops sgm3140_flash_ops = {
> + .strobe_set = sgm3140_strobe_set,
> +};
> +
> +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;
> +}
> +
> +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);
You could also implement strobe_get op and return from it a flag
indicating if LED is strobing.
> +}
> +
> +#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
> +static void sgm3140_init_v4l2_flash_config(struct sgm3140 *priv,
> + struct v4l2_flash_config *v4l2_sd_cfg)
> +{
> + struct led_classdev *led_cdev = &priv->fled_cdev.led_cdev;
> + struct led_flash_setting *s;
> +
> + strlcpy(v4l2_sd_cfg->dev_name, led_cdev->dev->kobj.name,
> + sizeof(v4l2_sd_cfg->dev_name));
> +
> + s = &v4l2_sd_cfg->intensity;
> + s->min = 0;
> + s->max = 1;
> + s->step = 1;
> + s->val = 1;
> +}
> +
> +#else
> +static void sgm3140_init_v4l2_flash_config(struct sgm3140 *priv,
> + struct v4l2_flash_config *v4l2_sd_cfg)
> +{
> +}
> +#endif
> +
> +static int sgm3140_probe(struct platform_device *pdev)
> +{
> + struct sgm3140 *priv;
> + struct led_classdev *led_cdev;
> + struct led_classdev_flash *fled_cdev;
> + struct led_init_data init_data = {};
> + struct device_node *child_node;
> + struct v4l2_flash_config v4l2_sd_cfg;
s/v4l2_sd_cfg;/v4l2_sd_cfg = {};/
Otherwise it is possible that some controls would be initialized
to random values.
> + int ret;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->flash_gpio = devm_gpiod_get(&pdev->dev, "flash", GPIOD_OUT_LOW);
> + ret = PTR_ERR_OR_ZERO(priv->flash_gpio);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "Failed to request flash gpio: %d\n",
> + ret);
> + return ret;
> + }
> +
> + priv->enable_gpio = devm_gpiod_get(&pdev->dev, "enable", GPIOD_OUT_LOW);
> + ret = PTR_ERR_OR_ZERO(priv->enable_gpio);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "Failed to request enable gpio: %d\n",
> + ret);
> + return ret;
> + }
> +
> + child_node = of_get_next_available_child(pdev->dev.of_node, NULL);
> + if (!child_node) {
> + dev_err(&pdev->dev, "No DT child node found for connected LED.\n");
> + return -EINVAL;
> + }
> +
> + timer_setup(&priv->powerdown_timer, sgm3140_powerdown_timer, 0);
> +
> + fled_cdev = &priv->fled_cdev;
> + led_cdev = &fled_cdev->led_cdev;
> +
> + fled_cdev->ops = &sgm3140_flash_ops;
> +
> + led_cdev->brightness_set_blocking = sgm3140_brightness_set;
> + led_cdev->max_brightness = LED_ON;
> + led_cdev->flags |= LED_DEV_CAP_FLASH;
> +
> + init_data.fwnode = of_fwnode_handle(child_node);
> + init_data.devicename = SGM3140_NAME;
devicename should be skipped in new drivers.
> +
> + platform_set_drvdata(pdev, priv);
> +
> + /* Register in the LED subsystem */
> + ret = led_classdev_flash_register_ext(&pdev->dev, fled_cdev, &init_data);
We already have devm_* prefixed version thereof.
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Failed to register flash device: %d\n",
> + ret);
> + goto err_flash_register;
> + }
> +
> + sgm3140_init_v4l2_flash_config(priv, &v4l2_sd_cfg);
> +
> + /* Create V4L2 Flash subdev */
> + priv->v4l2_flash = v4l2_flash_init(&pdev->dev, of_fwnode_handle(child_node),
> + fled_cdev, NULL,
> + &v4l2_sd_cfg);
> + if (IS_ERR(priv->v4l2_flash)) {
> + ret = PTR_ERR(priv->v4l2_flash);
> + goto err_v4l2_flash_init;
> + }
> +
> + return 0;
> +
> +err_v4l2_flash_init:
> + led_classdev_flash_unregister(fled_cdev);
> +err_flash_register:
> + of_node_put(child_node);
You need to relase of_node also in case of success.
> + return ret;
> +}
> +
> +static int sgm3140_remove(struct platform_device *pdev)
> +{
> + struct sgm3140 *priv = platform_get_drvdata(pdev);
> +
> + del_timer_sync(&priv->powerdown_timer);
> +
> + v4l2_flash_release(priv->v4l2_flash);
> + led_classdev_flash_unregister(&priv->fled_cdev);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id sgm3140_dt_match[] = {
> + { .compatible = "sgmicro,sgm3140" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sgm3140_dt_match);
> +
> +static struct platform_driver sgm3140_driver = {
> + .probe = sgm3140_probe,
> + .remove = sgm3140_remove,
> + .driver = {
> + .name = "sgm3140",
> + .of_match_table = sgm3140_dt_match,
> + },
> +};
> +
> +module_platform_driver(sgm3140_driver);
> +
> +MODULE_AUTHOR("Luca Weiss <[email protected]>");
> +MODULE_DESCRIPTION("SG Micro SGM3140 charge pump led driver");
> +MODULE_LICENSE("GPL v2");
>
--
Best regards,
Jacek Anaszewski
Hi Jacek,
Thanks for your review! Replies are inline below.
I'm wondering if I should implement support for the flash-max-timeout-us dt
property ("Maximum timeout in microseconds after which the flash LED is turned
off.") to configure the timeout to turn the flash off as I've currently hardcoded
250ms but this might not be ideal for all uses of the sgm3140. The datasheet
states:
> Flash mode is usually used with a pulse of about 200 to 300 milliseconds to
> generate a high intensity Flash.
so it might be useful to have this configurable in the devicetree. The value of
250ms works fine for my use case.
Theoretically also the .timeout_set op could be implemented but I'm not sure
if this fits nicely into the existing "timeout" API and if it even makes sense
to implement that.
Regards,
Luca
On Donnerstag, 5. M?rz 2020 22:09:16 CET Jacek Anaszewski wrote:
> Hi Luca,
>
> Thank you for the patch.
>
> On 2/27/20 7:50 PM, Luca Weiss wrote:
> > Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
> >
> > 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.
> >
> > Signed-off-by: Luca Weiss <[email protected]>
> > ---
> > 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
> >
> > # 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
>
> What is /dev/video1 ? Did you register vl42 flash subdev
> in some v4l2 media controller device?
On the Allwinner A64 SoC /dev/video0 is the node for cedrus (video encoder/
decoder), so the sun6i-csi driver gets to be /dev/video1
# v4l2-ctl --list-devices
cedrus (platform:cedrus):
/dev/video0
/dev/media0
sun6i-csi (platform:csi):
/dev/video1
Allwinner Video Capture Device (platform:sun6i-csi):
/dev/media1
Here's the relevant part from my dts:
sgm3140 {
compatible = "sgmicro,sgm3140";
flash-gpios = <&pio 3 24 GPIO_ACTIVE_HIGH>; /* FLASH_TRIGOUT: PD24 */
enable-gpios = <&pio 2 3 GPIO_ACTIVE_HIGH>; /* FLASH_EN: PC3 */
sgm3140_flash: led {
function = LED_FUNCTION_FLASH;
color = <LED_COLOR_ID_WHITE>;
};
};
/* as subnode of csi (compatible: allwinner,sun50i-a64-csi) */
ov5640: rear-camera@4c {
compatible = "ovti,ov5640";
<snip>
flash-leds = <&sgm3140_flash>;
};
>
> > 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.
>
> You have to be in flash mode to strobe i.e. led_mode=1.
Of course..! Makes sense, I just never realized the v4l2 device had to be in
this mode for the strobe button to work. It works nicely with that, thanks!
> <<snip>>
> > +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);
>
> You could also implement strobe_get op and return from it a flag
> indicating if LED is strobing.
Makes sense.
> > +}
> > +
> > +#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
> > +static void sgm3140_init_v4l2_flash_config(struct sgm3140 *priv,
> > + struct
v4l2_flash_config *v4l2_sd_cfg)
> > +{
> > + struct led_classdev *led_cdev = &priv->fled_cdev.led_cdev;
> > + struct led_flash_setting *s;
> > +
> > + strlcpy(v4l2_sd_cfg->dev_name, led_cdev->dev->kobj.name,
> > + sizeof(v4l2_sd_cfg->dev_name));
> > +
> > + s = &v4l2_sd_cfg->intensity;
> > + s->min = 0;
> > + s->max = 1;
> > + s->step = 1;
> > + s->val = 1;
> > +}
> > +
> > +#else
> > +static void sgm3140_init_v4l2_flash_config(struct sgm3140 *priv,
> > + struct
v4l2_flash_config *v4l2_sd_cfg)
> > +{
> > +}
> > +#endif
> > +
> > +static int sgm3140_probe(struct platform_device *pdev)
> > +{
> > + struct sgm3140 *priv;
> > + struct led_classdev *led_cdev;
> > + struct led_classdev_flash *fled_cdev;
> > + struct led_init_data init_data = {};
> > + struct device_node *child_node;
> > + struct v4l2_flash_config v4l2_sd_cfg;
>
> s/v4l2_sd_cfg;/v4l2_sd_cfg = {};/
>
> Otherwise it is possible that some controls would be initialized
> to random values.
>
Ack
> > + int ret;
> > +
> > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->flash_gpio = devm_gpiod_get(&pdev->dev, "flash",
GPIOD_OUT_LOW);
> > + ret = PTR_ERR_OR_ZERO(priv->flash_gpio);
> > + if (ret) {
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(&pdev->dev, "Failed to request flash
gpio: %d\n",
> > + ret);
> > + return ret;
> > + }
> > +
> > + priv->enable_gpio = devm_gpiod_get(&pdev->dev, "enable",
GPIOD_OUT_LOW);
> > + ret = PTR_ERR_OR_ZERO(priv->enable_gpio);
> > + if (ret) {
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(&pdev->dev, "Failed to request
enable gpio: %d\n",
> > + ret);
> > + return ret;
> > + }
> > +
> > + child_node = of_get_next_available_child(pdev->dev.of_node, NULL);
> > + if (!child_node) {
> > + dev_err(&pdev->dev, "No DT child node found for
connected LED.\n");
> > + return -EINVAL;
> > + }
> > +
> > + timer_setup(&priv->powerdown_timer, sgm3140_powerdown_timer, 0);
> > +
> > + fled_cdev = &priv->fled_cdev;
> > + led_cdev = &fled_cdev->led_cdev;
> > +
> > + fled_cdev->ops = &sgm3140_flash_ops;
> > +
> > + led_cdev->brightness_set_blocking = sgm3140_brightness_set;
> > + led_cdev->max_brightness = LED_ON;
> > + led_cdev->flags |= LED_DEV_CAP_FLASH;
> > +
> > + init_data.fwnode = of_fwnode_handle(child_node);
> > + init_data.devicename = SGM3140_NAME;
>
> devicename should be skipped in new drivers.
>
Ack
> > +
> > + platform_set_drvdata(pdev, priv);
> > +
> > + /* Register in the LED subsystem */
> > + ret = led_classdev_flash_register_ext(&pdev->dev, fled_cdev,
> > &init_data);
>
> We already have devm_* prefixed version thereof.
>
Ack, switched to the devm_ variant
> > + if (ret < 0) {
> > + dev_err(&pdev->dev, "Failed to register flash device:
%d\n",
> > + ret);
> > + goto err_flash_register;
> > + }
> > +
> > + sgm3140_init_v4l2_flash_config(priv, &v4l2_sd_cfg);
> > +
> > + /* Create V4L2 Flash subdev */
> > + priv->v4l2_flash = v4l2_flash_init(&pdev->dev,
> > of_fwnode_handle(child_node), +
fled_cdev, NULL,
> > + &v4l2_sd_cfg);
> > + if (IS_ERR(priv->v4l2_flash)) {
> > + ret = PTR_ERR(priv->v4l2_flash);
> > + goto err_v4l2_flash_init;
> > + }
> > +
> > + return 0;
> > +
> > +err_v4l2_flash_init:
> > + led_classdev_flash_unregister(fled_cdev);
> > +err_flash_register:
> > + of_node_put(child_node);
>
> You need to relase of_node also in case of success.
>
Done.
> > + return ret;
> > +}
> > +
> > +static int sgm3140_remove(struct platform_device *pdev)
> > +{
> > + struct sgm3140 *priv = platform_get_drvdata(pdev);
> > +
> > + del_timer_sync(&priv->powerdown_timer);
> > +
> > + v4l2_flash_release(priv->v4l2_flash);
> > + led_classdev_flash_unregister(&priv->fled_cdev);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id sgm3140_dt_match[] = {
> > + { .compatible = "sgmicro,sgm3140" },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, sgm3140_dt_match);
> > +
> > +static struct platform_driver sgm3140_driver = {
> > + .probe = sgm3140_probe,
> > + .remove = sgm3140_remove,
> > + .driver = {
> > + .name = "sgm3140",
> > + .of_match_table = sgm3140_dt_match,
> > + },
> > +};
> > +
> > +module_platform_driver(sgm3140_driver);
> > +
> > +MODULE_AUTHOR("Luca Weiss <[email protected]>");
> > +MODULE_DESCRIPTION("SG Micro SGM3140 charge pump led driver");
> > +MODULE_LICENSE("GPL v2");
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.
> 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?
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
>
> 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.
>
> Signed-off-by: Luca Weiss <[email protected]>
Cc: ~postmarketos/[email protected], Luca Weiss <[email protected]>, Jacek Anaszewski
Strange entry in cc list...?
And btw if you get the dt parts, and simple LED-only driver w/o the
strobe functinality, you may be able to get it merged rather quickly.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
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
Hi Pavel,
On Sonntag, 8. M?rz 2020 13:11:32 CET Pavel Machek wrote:
> Hi!
>
> > Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
> >
> > 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.
> >
> > Signed-off-by: Luca Weiss <[email protected]>
>
> Cc: ~postmarketos/[email protected], Luca Weiss <[email protected]>,
> Jacek Anaszewski
>
> Strange entry in cc list...?
You mean the '~postmarketos/[email protected]' entry with a tilde and a
slash character? Both are valid characters in email addresses and you can view
the archive of that mailing list here:
https://lists.sr.ht/~postmarketos/upstreaming
See also https://man.sr.ht/lists.sr.ht/#listssrht-manual
> And btw if you get the dt parts, and simple LED-only driver w/o the
> strobe functinality, you may be able to get it merged rather quickly.
>
I'm not really interested in having a torch-only driver merged if a full
driver with torch & strobe is already working. For the PinePhone we maintain a
separate kernel repository anyways so it doesn't matter much when exactly the
driver is going to get merged.
> Best regards,
>
Pavel
Regards
Luca
Hi Luca,
On 3/8/20 12:32 PM, Luca Weiss wrote:
> Hi Jacek,
>
> Thanks for your review! Replies are inline below.
>
> I'm wondering if I should implement support for the flash-max-timeout-us dt
> property ("Maximum timeout in microseconds after which the flash LED is turned
> off.") to configure the timeout to turn the flash off as I've currently hardcoded
> 250ms but this might not be ideal for all uses of the sgm3140. The datasheet
> states:
>
>> Flash mode is usually used with a pulse of about 200 to 300 milliseconds to
>> generate a high intensity Flash.
>
> so it might be useful to have this configurable in the devicetree. The value of
> 250ms works fine for my use case.
Yeah, I was to mentioned that.
>
> Theoretically also the .timeout_set op could be implemented but I'm not sure
> if this fits nicely into the existing "timeout" API and if it even makes sense
> to implement that.
Why wouldn't it fit? You can implement timeout_set op and cache flash
timeout value in it. Then that cached value would be passed in
strobe_set to mod_timer() in place of currently hard coded 250.
>
> Regards,
> Luca
>
> On Donnerstag, 5. März 2020 22:09:16 CET Jacek Anaszewski wrote:
>> Hi Luca,
>>
>> Thank you for the patch.
>>
>> On 2/27/20 7:50 PM, Luca Weiss wrote:
>>> Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
>>>
>>> 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.
>>>
>>> Signed-off-by: Luca Weiss <[email protected]>
>>> ---
>>> 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
>>>
>>> # 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
>>
>> What is /dev/video1 ? Did you register vl42 flash subdev
>> in some v4l2 media controller device?
>
> On the Allwinner A64 SoC /dev/video0 is the node for cedrus (video encoder/
> decoder), so the sun6i-csi driver gets to be /dev/video1
>
> # v4l2-ctl --list-devices
> cedrus (platform:cedrus):
> /dev/video0
> /dev/media0
>
> sun6i-csi (platform:csi):
> /dev/video1
>
> Allwinner Video Capture Device (platform:sun6i-csi):
> /dev/media1
>
>
> Here's the relevant part from my dts:
>
> sgm3140 {
> compatible = "sgmicro,sgm3140";
> flash-gpios = <&pio 3 24 GPIO_ACTIVE_HIGH>; /* FLASH_TRIGOUT: PD24 */
> enable-gpios = <&pio 2 3 GPIO_ACTIVE_HIGH>; /* FLASH_EN: PC3 */
>
> sgm3140_flash: led {
> function = LED_FUNCTION_FLASH;
> color = <LED_COLOR_ID_WHITE>;
> };
> };
This needs to be documented in DT bindings for this driver.
> /* as subnode of csi (compatible: allwinner,sun50i-a64-csi) */
> ov5640: rear-camera@4c {
> compatible = "ovti,ov5640";
> <snip>
> flash-leds = <&sgm3140_flash>;
> };
And this in camera bindings.
--
Best regards,
Jacek Anaszewski
Hi Jacek,
On Sonntag, 8. M?rz 2020 17:47:17 CET Jacek Anaszewski wrote:
> Hi Luca,
>
> On 3/8/20 12:32 PM, Luca Weiss wrote:
> > Hi Jacek,
> >
> > Thanks for your review! Replies are inline below.
> >
> > I'm wondering if I should implement support for the flash-max-timeout-us
> > dt
> > property ("Maximum timeout in microseconds after which the flash LED is
> > turned off.") to configure the timeout to turn the flash off as I've
> > currently hardcoded 250ms but this might not be ideal for all uses of the
> > sgm3140. The datasheet>
> > states:
> >> Flash mode is usually used with a pulse of about 200 to 300 milliseconds
> >> to
> >> generate a high intensity Flash.
> >
> > so it might be useful to have this configurable in the devicetree. The
> > value of 250ms works fine for my use case.
>
> Yeah, I was to mentioned that.
>
> > Theoretically also the .timeout_set op could be implemented but I'm not
> > sure if this fits nicely into the existing "timeout" API and if it even
> > makes sense to implement that.
>
> Why wouldn't it fit? You can implement timeout_set op and cache flash
> timeout value in it. Then that cached value would be passed in
> strobe_set to mod_timer() in place of currently hard coded 250.
>
I'll implement that then.
> > Regards,
> > Luca
> >
> > On Donnerstag, 5. M?rz 2020 22:09:16 CET Jacek Anaszewski wrote:
> >> Hi Luca,
> >>
> >> Thank you for the patch.
> >>
> >> On 2/27/20 7:50 PM, Luca Weiss wrote:
> >>> Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
> >>>
> >>> 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.
> >>>
> >>> Signed-off-by: Luca Weiss <[email protected]>
> >>> ---
> >>> 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
> >>>
> >>> # 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
> >>
> >> What is /dev/video1 ? Did you register vl42 flash subdev
> >> in some v4l2 media controller device?
> >
> > On the Allwinner A64 SoC /dev/video0 is the node for cedrus (video
> > encoder/
> > decoder), so the sun6i-csi driver gets to be /dev/video1
> >
> > # v4l2-ctl --list-devices
> >
> > cedrus (platform:cedrus):
> > /dev/video0
> > /dev/media0
> >
> > sun6i-csi (platform:csi):
> > /dev/video1
> >
> > Allwinner Video Capture Device (platform:sun6i-csi):
> > /dev/media1
> >
> > Here's the relevant part from my dts:
> >
> > sgm3140 {
> >
> > compatible = "sgmicro,sgm3140";
> > flash-gpios = <&pio 3 24 GPIO_ACTIVE_HIGH>; /* FLASH_TRIGOUT: PD24 */
> > enable-gpios = <&pio 2 3 GPIO_ACTIVE_HIGH>; /* FLASH_EN: PC3 */
> >
> > sgm3140_flash: led {
> >
> > function = LED_FUNCTION_FLASH;
> > color = <LED_COLOR_ID_WHITE>;
> >
> > };
> >
> > };
>
> This needs to be documented in DT bindings for this driver.
>
I have already written some yesterday, will post them with my v1 :)
> > /* as subnode of csi (compatible: allwinner,sun50i-a64-csi) */
> > ov5640: rear-camera@4c {
> >
> > compatible = "ovti,ov5640";
> > <snip>
> > flash-leds = <&sgm3140_flash>;
> >
> > };
>
> And this in camera bindings.
This is documented at
Documentation/devicetree/bindings/media/video-interfaces.txt:
- flash-leds: An array of phandles, each referring to a flash LED, a sub-node
of the LED driver device node.
Without referencing the flash device in a camera node, the v4l2 controls won't
even show up from what I saw.
The binding is apparently only used in omap3-n9 and omap3-n950 currently; only
phones have flash leds normally and the phones that are currently in mainline
Linux don't have camera support yet.
Regards
Luca
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
On 3/8/20 5:55 PM, Luca Weiss wrote:
> Hi Jacek,
>
> On Sonntag, 8. März 2020 17:47:17 CET Jacek Anaszewski wrote:
>> Hi Luca,
>>
>> On 3/8/20 12:32 PM, Luca Weiss wrote:
>>> Hi Jacek,
>>>
>>> Thanks for your review! Replies are inline below.
>>>
>>> I'm wondering if I should implement support for the flash-max-timeout-us
>>> dt
>>> property ("Maximum timeout in microseconds after which the flash LED is
>>> turned off.") to configure the timeout to turn the flash off as I've
>>> currently hardcoded 250ms but this might not be ideal for all uses of the
>>> sgm3140. The datasheet>
>>> states:
>>>> Flash mode is usually used with a pulse of about 200 to 300 milliseconds
>>>> to
>>>> generate a high intensity Flash.
>>>
>>> so it might be useful to have this configurable in the devicetree. The
>>> value of 250ms works fine for my use case.
>>
>> Yeah, I was to mentioned that.
>>
>>> Theoretically also the .timeout_set op could be implemented but I'm not
>>> sure if this fits nicely into the existing "timeout" API and if it even
>>> makes sense to implement that.
>>
>> Why wouldn't it fit? You can implement timeout_set op and cache flash
>> timeout value in it. Then that cached value would be passed in
>> strobe_set to mod_timer() in place of currently hard coded 250.
>>
>
> I'll implement that then.
>
>>> Regards,
>>> Luca
>>>
>>> On Donnerstag, 5. März 2020 22:09:16 CET Jacek Anaszewski wrote:
>>>> Hi Luca,
>>>>
>>>> Thank you for the patch.
>>>>
>>>> On 2/27/20 7:50 PM, Luca Weiss wrote:
>>>>> Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
>>>>>
>>>>> 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.
>>>>>
>>>>> Signed-off-by: Luca Weiss <[email protected]>
>>>>> ---
>>>>> 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
>>>>>
>>>>> # 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
>>>>
>>>> What is /dev/video1 ? Did you register vl42 flash subdev
>>>> in some v4l2 media controller device?
>>>
>>> On the Allwinner A64 SoC /dev/video0 is the node for cedrus (video
>>> encoder/
>>> decoder), so the sun6i-csi driver gets to be /dev/video1
>>>
>>> # v4l2-ctl --list-devices
>>>
>>> cedrus (platform:cedrus):
>>> /dev/video0
>>> /dev/media0
>>>
>>> sun6i-csi (platform:csi):
>>> /dev/video1
>>>
>>> Allwinner Video Capture Device (platform:sun6i-csi):
>>> /dev/media1
>>>
>>> Here's the relevant part from my dts:
>>>
>>> sgm3140 {
>>>
>>> compatible = "sgmicro,sgm3140";
>>> flash-gpios = <&pio 3 24 GPIO_ACTIVE_HIGH>; /* FLASH_TRIGOUT: PD24 */
>>> enable-gpios = <&pio 2 3 GPIO_ACTIVE_HIGH>; /* FLASH_EN: PC3 */
>>>
>>> sgm3140_flash: led {
>>>
>>> function = LED_FUNCTION_FLASH;
>>> color = <LED_COLOR_ID_WHITE>;
>>>
>>> };
>>>
>>> };
>>
>> This needs to be documented in DT bindings for this driver.
>>
>
> I have already written some yesterday, will post them with my v1 :)
Good :-)
>
>>> /* as subnode of csi (compatible: allwinner,sun50i-a64-csi) */
>>> ov5640: rear-camera@4c {
>>>
>>> compatible = "ovti,ov5640";
>>> <snip>
>>> flash-leds = <&sgm3140_flash>;
>>>
>>> };
>>
>> And this in camera bindings.
>
> This is documented at
> Documentation/devicetree/bindings/media/video-interfaces.txt:
>
> - flash-leds: An array of phandles, each referring to a flash LED, a sub-node
> of the LED driver device node.
>
> Without referencing the flash device in a camera node, the v4l2 controls won't
> even show up from what I saw.
> The binding is apparently only used in omap3-n9 and omap3-n950 currently; only
> phones have flash leds normally and the phones that are currently in mainline
> Linux don't have camera support yet.
I was rather thinking of mentioning this e.g. in
Documentation/devicetree/bindings/media/i2c/ov5640.txt in the form like
below (copied other occurrences thereof):
- flash-leds: See ../video-interfaces.txt
--
Best regards,
Jacek Anaszewski