[RFC PATCH] leds: add sgm3140 driver

[RFC PATCH] leds: add sgm3140 driver

[Luca Weiss]

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 <luca@z3ntu.xyz>
---
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 <luca@z3ntu.xyz>
+
+#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 <luca@z3ntu.xyz>");
+MODULE_DESCRIPTION("SG Micro SGM3140 charge pump led driver");
+MODULE_LICENSE("GPL v2");
-- 
2.25.1

Re: [RFC PATCH] leds: add sgm3140 driver

[Dan Murphy]

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 <luca@z3ntu.xyz>
> ---

You seem to be missing the devictree bindings doc for the GPIOs.

Dan

Re: [RFC PATCH] leds: add sgm3140 driver

[Luca Weiss]

[-- Attachment #1: Type: text/plain, Size: 910 bytes --]

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 <luca@z3ntu.xyz>
> > ---
> 
> 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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] leds: add sgm3140 driver

[Dan Murphy]

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 <luca@z3ntu.xyz>
>>> ---
>> 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

Re: [RFC PATCH] leds: add sgm3140 driver

[Jacek Anaszewski]

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 <luca@z3ntu.xyz>
> ---
> 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 <luca@z3ntu.xyz>
> +
> +#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 <luca@z3ntu.xyz>");
> +MODULE_DESCRIPTION("SG Micro SGM3140 charge pump led driver");
> +MODULE_LICENSE("GPL v2");
> 

-- 
Best regards,
Jacek Anaszewski

Re: [RFC PATCH] leds: add sgm3140 driver

[Luca Weiss]

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 <luca@z3ntu.xyz>
> > ---
> > 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 <luca@z3ntu.xyz>");
> > +MODULE_DESCRIPTION("SG Micro SGM3140 charge pump led driver");
> > +MODULE_LICENSE("GPL v2");

Re: [RFC PATCH] leds: add sgm3140 driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3460 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC PATCH] leds: add sgm3140 driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 906 bytes --]

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 <luca@z3ntu.xyz>

Cc: ~postmarketos/upstreaming@lists.sr.ht, Luca Weiss <luca@z3ntu.xyz>, 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

Re: [RFC PATCH] leds: add sgm3140 driver

[Luca Weiss]

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 <luca@z3ntu.xyz>
> 
> Cc: ~postmarketos/upstreaming@lists.sr.ht, Luca Weiss <luca@z3ntu.xyz>,
> Jacek Anaszewski
> 
> Strange entry in cc list...?

You mean the '~postmarketos/upstreaming@lists.sr.ht' 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

Re: [RFC PATCH] leds: add sgm3140 driver

[Jacek Anaszewski]

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 <luca@z3ntu.xyz>
>>> ---
>>> 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

Re: [RFC PATCH] leds: add sgm3140 driver

[Luca Weiss]

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 <luca@z3ntu.xyz>
> >>> ---
> >>> 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

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

Re: [RFC PATCH] leds: add sgm3140 driver

[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 <luca@z3ntu.xyz>
>>>>> ---
>>>>> 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