[PATCH 0/3] Add sgm3140 flash led driver

[PATCH 0/3] Add sgm3140 flash led driver

[Luca Weiss]

This series introduces a driver for the SGMICRO SGM3140 charge pump
used in the PinePhone smartphone.

Luca Weiss (3):
  dt-bindings: Add vendor prefix for SG Micro Corp
  dt-bindings: leds: Add binding for sgm3140
  leds: add sgm3140 driver

 .../bindings/leds/leds-sgm3140.yaml           |  53 ++++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 drivers/leds/Kconfig                          |   9 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-sgm3140.c                   | 260 ++++++++++++++++++
 5 files changed, 325 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
 create mode 100644 drivers/leds/leds-sgm3140.c

-- 
2.25.1

[PATCH 1/3] dt-bindings: Add vendor prefix for SG Micro Corp

[Luca Weiss]

"SG Micro Corp (SGMICRO) specializes in high performance, high quality
analog IC design, marketing and sales." (http://www.sg-micro.com/)

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
Changes since RFC:
- new patch

 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 8d306af435a3..059ff7bc0c79 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -843,6 +843,8 @@ patternProperties:
     description: Small Form Factor Committee
   "^sgd,.*":
     description: Solomon Goldentek Display Corporation
+  "^sgmicro,.*":
+    description: SG Micro Corp
   "^sgx,.*":
     description: SGX Sensortech
   "^sharp,.*":
-- 
2.25.1

[PATCH 2/3] dt-bindings: leds: Add binding for sgm3140

[Luca Weiss]

Add YAML devicetree binding for SGMICRO SGM3140 charge pump used for
camera flash LEDs.

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
Changes since RFC:
- new patch

I'm not sure about the completeness of this binding as it doesn't
mention the led subnode at all.
The only existing led yaml binding is leds/leds-max77650.yaml which
mentions the subnode but duplicates properties from documented in 
leds/common.txt.

 .../bindings/leds/leds-sgm3140.yaml           | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-sgm3140.yaml

diff --git a/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml b/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
new file mode 100644
index 000000000000..be9384573d02
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-sgm3140.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SGMICRO SGM3140 500mA Buck/Boost Charge Pump LED Driver
+
+maintainers:
+  - Luca Weiss <luca@z3ntu.xyz>
+
+description: |
+  The SGM3140 is a current-regulated charge pump which can regulate two current
+  levels for Flash and Torch modes.
+
+  It is controlled with two GPIO pins.
+
+  The data sheet can be found at:
+    http://www.sg-micro.com/uploads/soft/20190626/1561535688.pdf
+
+properties:
+  compatible:
+    const: sgmicro,sgm3140
+
+  enable-gpios:
+    maxItems: 1
+    description: A connection to the 'EN' pin.
+
+  flash-gpios:
+    maxItems: 1
+    description: A connection to the 'FLASH' pin.
+
+required:
+  - compatible
+  - flash-gpios
+  - enable-gpios
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/leds/common.h>
+
+    sgm3140 {
+        compatible = "sgmicro,sgm3140";
+        flash-gpios = <&pio 3 24 GPIO_ACTIVE_HIGH>; /* PD24 */
+        enable-gpios = <&pio 2 3 GPIO_ACTIVE_HIGH>; /* PC3 */
+
+        sgm3140_flash: led {
+            function = LED_FUNCTION_FLASH;
+            color = <LED_COLOR_ID_WHITE>;
+            flash-max-timeout-us = <250000>;
+        };
+    };
-- 
2.25.1

[PATCH 3/3] leds: add sgm3140 driver

[Luca Weiss]

Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.

This device is controlled by two GPIO pins, one for enabling and the
second one for switching between torch and flash mode.

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
Changes since RFC:
- address review comments from Jacek Anaszewski:
  - implement strobe_get op
  - implement timeout_set op
  - init v4l2_sd_cfg variable
  - remove init_data.devicename assignemnt
  - use devm_ version of led_classdev_flash_register_ext
  - release child_node in case of success

 drivers/leds/Kconfig        |   9 ++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-sgm3140.c | 260 ++++++++++++++++++++++++++++++++++++
 3 files changed, 270 insertions(+)
 create mode 100644 drivers/leds/leds-sgm3140.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 4b68520ac251..9206fc66799d 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.
+
 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..357f4cbb279a
--- /dev/null
+++ b/drivers/leds/leds-sgm3140.c
@@ -0,0 +1,260 @@
+// 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 FLASH_TIMEOUT_DEFAULT		250000 /* 250ms */
+#define FLASH_MAX_TIMEOUT_DEFAULT	300000 /* 300ms */
+
+struct sgm3140 {
+	struct gpio_desc *flash_gpio;
+	struct gpio_desc *enable_gpio;
+
+	/* current timeout in us */
+	u32 timeout;
+	/* maximum timeout in us */
+	u32 max_timeout;
+
+	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);
+}
+
+static 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 + usecs_to_jiffies(priv->timeout));
+	} 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;
+}
+
+static int sgm3140_strobe_get(struct led_classdev_flash *fled_cdev, bool *state)
+{
+	struct sgm3140 *priv = flcdev_to_sgm3140(fled_cdev);
+
+	*state = timer_pending(&priv->powerdown_timer);
+
+	return 0;
+}
+
+static int sgm3140_timeout_set(struct led_classdev_flash *fled_cdev, u32 timeout)
+{
+	struct sgm3140 *priv = flcdev_to_sgm3140(fled_cdev);
+
+	priv->timeout = timeout;
+
+	return 0;
+}
+
+struct led_flash_ops sgm3140_flash_ops = {
+	.strobe_set = sgm3140_strobe_set,
+	.strobe_get = sgm3140_strobe_get,
+	.timeout_set = sgm3140_timeout_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);
+}
+
+static void sgm3140_init_flash_timeout(struct sgm3140 *priv)
+{
+	struct led_classdev_flash *fled_cdev = &priv->fled_cdev;
+	struct led_flash_setting *s;
+
+	/* Init flash timeout setting */
+	s = &fled_cdev->timeout;
+	s->min = 1;
+	s->max = priv->max_timeout;
+	s->step = 1;
+	s->val = FLASH_TIMEOUT_DEFAULT;
+}
+
+#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));
+
+	/* Init flash intensity setting */
+	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;
+	}
+
+
+	ret = of_property_read_u32(child_node, "flash-max-timeout-us",
+				   &priv->max_timeout);
+	if (ret < 0) {
+		priv->max_timeout = FLASH_MAX_TIMEOUT_DEFAULT;
+		dev_warn(&pdev->dev, "flash-max-timeout-us DT property missing\n");
+	}
+
+	/*
+	 * Set default timeout to FLASH_DEFAULT_TIMEOUT except if max_timeout
+	 * from DT is lower.
+	 */
+	priv->timeout = min(priv->max_timeout, (u32)FLASH_TIMEOUT_DEFAULT);
+
+	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;
+
+	sgm3140_init_flash_timeout(priv);
+
+	init_data.fwnode = of_fwnode_handle(child_node);
+
+	platform_set_drvdata(pdev, priv);
+
+	/* Register in the LED subsystem */
+	ret = devm_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;
+	}
+
+	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;
+	}
+
+err:
+	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);
+
+	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: [PATCH 3/3] leds: add sgm3140 driver

[Sakari Ailus]

Hi Luca,

Thanks for the patch.

On Mon, Mar 09, 2020 at 09:35:58PM +0100, Luca Weiss wrote:
> Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
> 
> This device is controlled by two GPIO pins, one for enabling and the
> second one for switching between torch and flash mode.
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
> Changes since RFC:
> - address review comments from Jacek Anaszewski:
>   - implement strobe_get op
>   - implement timeout_set op
>   - init v4l2_sd_cfg variable
>   - remove init_data.devicename assignemnt
>   - use devm_ version of led_classdev_flash_register_ext
>   - release child_node in case of success
> 
>  drivers/leds/Kconfig        |   9 ++
>  drivers/leds/Makefile       |   1 +
>  drivers/leds/leds-sgm3140.c | 260 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 270 insertions(+)
>  create mode 100644 drivers/leds/leds-sgm3140.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 4b68520ac251..9206fc66799d 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.
> +
>  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..357f4cbb279a
> --- /dev/null
> +++ b/drivers/leds/leds-sgm3140.c
> @@ -0,0 +1,260 @@
> +// 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 FLASH_TIMEOUT_DEFAULT		250000 /* 250ms */
> +#define FLASH_MAX_TIMEOUT_DEFAULT	300000 /* 300ms */

Add U, and you can remove the cast elsewhere.

> +
> +struct sgm3140 {
> +	struct gpio_desc *flash_gpio;
> +	struct gpio_desc *enable_gpio;
> +
> +	/* current timeout in us */
> +	u32 timeout;
> +	/* maximum timeout in us */
> +	u32 max_timeout;
> +
> +	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);
> +}
> +
> +static 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 + usecs_to_jiffies(priv->timeout));
> +	} 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;
> +}
> +
> +static int sgm3140_strobe_get(struct led_classdev_flash *fled_cdev, bool *state)
> +{
> +	struct sgm3140 *priv = flcdev_to_sgm3140(fled_cdev);
> +
> +	*state = timer_pending(&priv->powerdown_timer);
> +
> +	return 0;
> +}
> +
> +static int sgm3140_timeout_set(struct led_classdev_flash *fled_cdev, u32 timeout)
> +{
> +	struct sgm3140 *priv = flcdev_to_sgm3140(fled_cdev);
> +
> +	priv->timeout = timeout;
> +
> +	return 0;
> +}
> +
> +struct led_flash_ops sgm3140_flash_ops = {

const?

> +	.strobe_set = sgm3140_strobe_set,
> +	.strobe_get = sgm3140_strobe_get,
> +	.timeout_set = sgm3140_timeout_set,
> +};
> +
> +int sgm3140_brightness_set(struct led_classdev *led_cdev,
> +			   enum led_brightness brightness)

static

> +{
> +	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'll need a work queue to use sleeping gpiod_set. In this case, I'd use
gpiod_set_value() instead, as elsewhere. I think it's unlikely you'd come
across a board where it'd sleep.

> +}
> +
> +static void sgm3140_init_flash_timeout(struct sgm3140 *priv)
> +{
> +	struct led_classdev_flash *fled_cdev = &priv->fled_cdev;
> +	struct led_flash_setting *s;
> +
> +	/* Init flash timeout setting */
> +	s = &fled_cdev->timeout;
> +	s->min = 1;
> +	s->max = priv->max_timeout;
> +	s->step = 1;
> +	s->val = FLASH_TIMEOUT_DEFAULT;
> +}
> +
> +#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));

strscpy()?

> +
> +	/* Init flash intensity setting */
> +	s = &v4l2_sd_cfg->intensity;
> +	s->min = 0;
> +	s->max = 1;
> +	s->step = 1;
> +	s->val = 1;
> +}
> +
> +#else

The prototype doesn't change. I'd put the #if ... #endif inside the
function. Up to you.

> +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");

You could wrap this. There are a few more above and below, too.

> +		return -EINVAL;
> +	}
> +
> +
> +	ret = of_property_read_u32(child_node, "flash-max-timeout-us",
> +				   &priv->max_timeout);
> +	if (ret < 0) {
> +		priv->max_timeout = FLASH_MAX_TIMEOUT_DEFAULT;
> +		dev_warn(&pdev->dev, "flash-max-timeout-us DT property missing\n");
> +	}
> +
> +	/*
> +	 * Set default timeout to FLASH_DEFAULT_TIMEOUT except if max_timeout
> +	 * from DT is lower.
> +	 */
> +	priv->timeout = min(priv->max_timeout, (u32)FLASH_TIMEOUT_DEFAULT);
> +
> +	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;
> +
> +	sgm3140_init_flash_timeout(priv);
> +
> +	init_data.fwnode = of_fwnode_handle(child_node);
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	/* Register in the LED subsystem */
> +	ret = devm_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;
> +	}
> +
> +	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;
> +	}
> +
> +err:
> +	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);
> +
> +	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");

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/3] dt-bindings: leds: Add binding for sgm3140

[Sakari Ailus]

Hi Luca,

On Mon, Mar 09, 2020 at 09:35:57PM +0100, Luca Weiss wrote:
> Add YAML devicetree binding for SGMICRO SGM3140 charge pump used for
> camera flash LEDs.
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
> Changes since RFC:
> - new patch
> 
> I'm not sure about the completeness of this binding as it doesn't
> mention the led subnode at all.

I guess you'll need one --- the driver expects it as well.

> The only existing led yaml binding is leds/leds-max77650.yaml which
> mentions the subnode but duplicates properties from documented in 
> leds/common.txt.
> 
>  .../bindings/leds/leds-sgm3140.yaml           | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml b/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
> new file mode 100644
> index 000000000000..be9384573d02
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-sgm3140.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SGMICRO SGM3140 500mA Buck/Boost Charge Pump LED Driver
> +
> +maintainers:
> +  - Luca Weiss <luca@z3ntu.xyz>
> +
> +description: |
> +  The SGM3140 is a current-regulated charge pump which can regulate two current
> +  levels for Flash and Torch modes.
> +
> +  It is controlled with two GPIO pins.
> +
> +  The data sheet can be found at:
> +    http://www.sg-micro.com/uploads/soft/20190626/1561535688.pdf
> +
> +properties:
> +  compatible:
> +    const: sgmicro,sgm3140
> +
> +  enable-gpios:
> +    maxItems: 1
> +    description: A connection to the 'EN' pin.
> +
> +  flash-gpios:
> +    maxItems: 1
> +    description: A connection to the 'FLASH' pin.

How about a regulator supply?

I guess the chip is meant to be connected to a li-ion cell but still...

> +
> +required:
> +  - compatible
> +  - flash-gpios
> +  - enable-gpios
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/leds/common.h>
> +
> +    sgm3140 {
> +        compatible = "sgmicro,sgm3140";
> +        flash-gpios = <&pio 3 24 GPIO_ACTIVE_HIGH>; /* PD24 */
> +        enable-gpios = <&pio 2 3 GPIO_ACTIVE_HIGH>; /* PC3 */
> +
> +        sgm3140_flash: led {
> +            function = LED_FUNCTION_FLASH;
> +            color = <LED_COLOR_ID_WHITE>;
> +            flash-max-timeout-us = <250000>;
> +        };
> +    };

-- 
Regards,

Sakari Ailus

Re: [PATCH 3/3] leds: add sgm3140 driver

[Pavel Machek]

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

Hi!

> > +#define FLASH_TIMEOUT_DEFAULT		250000 /* 250ms */
> > +#define FLASH_MAX_TIMEOUT_DEFAULT	300000 /* 300ms */
> 
> Add U, and you can remove the cast elsewhere.

I'll disagree here. Avoid U, avoid cast. Neither is needed.

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: [PATCH 3/3] leds: add sgm3140 driver

[Luca Weiss]

Hi Pavel

On Montag, 9. März 2020 23:49:27 CET Pavel Machek wrote:
> Hi!
> 
> > > +#define FLASH_TIMEOUT_DEFAULT		250000 /* 250ms */
> > > +#define FLASH_MAX_TIMEOUT_DEFAULT	300000 /* 300ms */
> > 
> > Add U, and you can remove the cast elsewhere.
> 
> I'll disagree here. Avoid U, avoid cast. Neither is needed.

If neither cast to u32 nor the U suffix is used, then this warning will be 
printed:

In file included from ./include/asm-generic/bug.h:19,
                 from ./arch/arm64/include/asm/bug.h:26,
                 from ./include/linux/bug.h:5,
                 from ./include/linux/gpio/consumer.h:5,
                 from drivers/leds/leds-sgm3140.c:4:
drivers/leds/leds-sgm3140.c: In function 'sgm3140_probe':
./include/linux/kernel.h:835:29: warning: comparison of distinct pointer types 
lacks a cast
  835 |   (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
      |                             ^~
./include/linux/kernel.h:849:4: note: in expansion of macro '__typecheck'
  849 |   (__typecheck(x, y) && __no_side_effects(x, y))
      |    ^~~~~~~~~~~
./include/linux/kernel.h:859:24: note: in expansion of macro '__safe_cmp'
  859 |  __builtin_choose_expr(__safe_cmp(x, y), \
      |                        ^~~~~~~~~~
./include/linux/kernel.h:868:19: note: in expansion of macro '__careful_cmp'
  868 | #define min(x, y) __careful_cmp(x, y, <)
      |                   ^~~~~~~~~~~~~
drivers/leds/leds-sgm3140.c:187:18: note: in expansion of macro 'min'
  187 |  priv->timeout = min(priv->max_timeout, FLASH_TIMEOUT_DEFAULT);
      |                  ^~~

So one of both is needed.

> 
> Best regards,
> 								
	Pavel

Regards
Luca

Re: [PATCH 2/3] dt-bindings: leds: Add binding for sgm3140

[Dan Murphy]

Luca

On 3/9/20 3:35 PM, Luca Weiss wrote:
> Add YAML devicetree binding for SGMICRO SGM3140 charge pump used for
> camera flash LEDs.
>
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
> Changes since RFC:
> - new patch
>
> I'm not sure about the completeness of this binding as it doesn't
> mention the led subnode at all.
> The only existing led yaml binding is leds/leds-max77650.yaml which
> mentions the subnode but duplicates properties from documented in
> leds/common.txt.
>
>   .../bindings/leds/leds-sgm3140.yaml           | 53 +++++++++++++++++++
>   1 file changed, 53 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml b/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
> new file mode 100644
> index 000000000000..be9384573d02
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-sgm3140.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SGMICRO SGM3140 500mA Buck/Boost Charge Pump LED Driver
> +
> +maintainers:
> +  - Luca Weiss <luca@z3ntu.xyz>
> +
> +description: |
> +  The SGM3140 is a current-regulated charge pump which can regulate two current
> +  levels for Flash and Torch modes.
> +
> +  It is controlled with two GPIO pins.
Please define "It".  Not sure what is controlled here.


Dan

Re: [PATCH 3/3] leds: add sgm3140 driver

[Dan Murphy]

Luca

On 3/9/20 3:35 PM, Luca Weiss wrote:
> Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
>
> This device is controlled by two GPIO pins, one for enabling and the
> second one for switching between torch and flash mode.

How does one enable torch and one enable flash?

Is the flash-gpio control this or does the enable-gpio enable the flash?

The DT binding did not indicate what the GPIOs are really going to control.

>
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
> Changes since RFC:
> - address review comments from Jacek Anaszewski:
>    - implement strobe_get op
>    - implement timeout_set op
>    - init v4l2_sd_cfg variable
>    - remove init_data.devicename assignemnt
>    - use devm_ version of led_classdev_flash_register_ext
>    - release child_node in case of success
>
>   drivers/leds/Kconfig        |   9 ++
>   drivers/leds/Makefile       |   1 +
>   drivers/leds/leds-sgm3140.c | 260 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 270 insertions(+)
>   create mode 100644 drivers/leds/leds-sgm3140.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 4b68520ac251..9206fc66799d 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

What is the purpose of this?  Enable if V4L2_FLASH_LED_CLASS is enabled 
or if it is not enabled?

Seems to be a do nothing dependency in the Kconfig

> +	depends on OF
> +	help
> +	  This option enables support for the SGM3140 500mA Buck/Boost Charge
> +	  Pump LED Driver.
> +
>   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..357f4cbb279a
> --- /dev/null
> +++ b/drivers/leds/leds-sgm3140.c
> @@ -0,0 +1,260 @@
> +// 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 FLASH_TIMEOUT_DEFAULT		250000 /* 250ms */
> +#define FLASH_MAX_TIMEOUT_DEFAULT	300000 /* 300ms */
> +
> +struct sgm3140 {
> +	struct gpio_desc *flash_gpio;
> +	struct gpio_desc *enable_gpio;
> +
> +	/* current timeout in us */
> +	u32 timeout;
> +	/* maximum timeout in us */
> +	u32 max_timeout;
> +
> +	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);
> +}
> +
> +static 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 + usecs_to_jiffies(priv->timeout));
> +	} 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;
> +}
> +
> +static int sgm3140_strobe_get(struct led_classdev_flash *fled_cdev, bool *state)
> +{
> +	struct sgm3140 *priv = flcdev_to_sgm3140(fled_cdev);
> +
> +	*state = timer_pending(&priv->powerdown_timer);
> +
> +	return 0;
> +}
> +
> +static int sgm3140_timeout_set(struct led_classdev_flash *fled_cdev, u32 timeout)
> +{
> +	struct sgm3140 *priv = flcdev_to_sgm3140(fled_cdev);
> +
> +	priv->timeout = timeout;
> +
> +	return 0;
> +}
> +
> +struct led_flash_ops sgm3140_flash_ops = {
> +	.strobe_set = sgm3140_strobe_set,
> +	.strobe_get = sgm3140_strobe_get,
> +	.timeout_set = sgm3140_timeout_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);
> +}
> +
> +static void sgm3140_init_flash_timeout(struct sgm3140 *priv)
> +{
> +	struct led_classdev_flash *fled_cdev = &priv->fled_cdev;
> +	struct led_flash_setting *s;
> +
> +	/* Init flash timeout setting */
> +	s = &fled_cdev->timeout;
> +	s->min = 1;
> +	s->max = priv->max_timeout;
> +	s->step = 1;
> +	s->val = FLASH_TIMEOUT_DEFAULT;
> +}
> +
> +#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));
> +
> +	/* Init flash intensity setting */
> +	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);

Please use the device_property api's to retrieve DT settings.

> +	if (!child_node) {
> +		dev_err(&pdev->dev, "No DT child node found for connected LED.\n");
> +		return -EINVAL;
> +	}
> +
> +
> +	ret = of_property_read_u32(child_node, "flash-max-timeout-us",
> +				   &priv->max_timeout);
Please use the device_property api's to retrieve DT settings.  Then 
there is no need to release the "of" child node.
> +	if (ret < 0) {
if (ret)
> +		priv->max_timeout = FLASH_MAX_TIMEOUT_DEFAULT;
> +		dev_warn(&pdev->dev, "flash-max-timeout-us DT property missing\n");
> +	}
> +
> +	/*
> +	 * Set default timeout to FLASH_DEFAULT_TIMEOUT except if max_timeout
> +	 * from DT is lower.
> +	 */
> +	priv->timeout = min(priv->max_timeout, (u32)FLASH_TIMEOUT_DEFAULT);
> +
> +	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;
> +
> +	sgm3140_init_flash_timeout(priv);
> +
> +	init_data.fwnode = of_fwnode_handle(child_node);
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	/* Register in the LED subsystem */
> +	ret = devm_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;
> +	}
> +
> +	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;

Do you need to jump here?  This should just fall out and go through err 
anyway.

Dan

Re: [PATCH 3/3] leds: add sgm3140 driver

[Jacek Anaszewski]

Dan,

On 3/11/20 2:02 PM, Dan Murphy wrote:
> Luca
> 
> On 3/9/20 3:35 PM, Luca Weiss wrote:
>> Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
>>
>> This device is controlled by two GPIO pins, one for enabling and the
>> second one for switching between torch and flash mode.
> 
> How does one enable torch and one enable flash?
> 
> Is the flash-gpio control this or does the enable-gpio enable the flash?
> 
> The DT binding did not indicate what the GPIOs are really going to control.
> 
>>
>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
>> ---
>> Changes since RFC:
>> - address review comments from Jacek Anaszewski:
>>    - implement strobe_get op
>>    - implement timeout_set op
>>    - init v4l2_sd_cfg variable
>>    - remove init_data.devicename assignemnt
>>    - use devm_ version of led_classdev_flash_register_ext
>>    - release child_node in case of success
>>
>>   drivers/leds/Kconfig        |   9 ++
>>   drivers/leds/Makefile       |   1 +
>>   drivers/leds/leds-sgm3140.c | 260 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 270 insertions(+)
>>   create mode 100644 drivers/leds/leds-sgm3140.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 4b68520ac251..9206fc66799d 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
> 
> What is the purpose of this?  Enable if V4L2_FLASH_LED_CLASS is enabled
> or if it is not enabled?
> 
> Seems to be a do nothing dependency in the Kconfig

See the patch: 58d1809b9d61 ("leds: fix aat1290 build errors")

and Documentation/kbuild/kconfig-language.rst,
chapter "Menu dependencies".

According to that ('!' <expr>)  boils down to (2-/expr/),
where

"An expression can have a value of 'n', 'm' or 'y' (or 0, 1, 2
respectively for calculations)".

In a result:
- in case V4L2_FLASH_LED_CLASS=n it evaluates to 2,
- in case V4L2_FLASH_LED_CLASS=m it evaluates to 1,
- in case V4L2_FLASH_LED_CLASS=y it evaluates to 0

And (<expr> '||' <expr>) returns the result of max(/expr/, /expr/).

It allows to restrict LEDS_SGM3140 to m if CONFIG_V4L2_FLASH_LED_CLASS=m
(we will have max(1,1) then)
and ignore the dependency if CONFIG_V4L2_FLASH_LED_CLASS=n
(max(0,2)).

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 3/3] leds: add sgm3140 driver

[Luca Weiss]

Hi Dan,

On Mittwoch, 11. März 2020 14:02:44 CET Dan Murphy wrote:
> Luca
> 
> On 3/9/20 3:35 PM, Luca Weiss wrote:
> > Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
> > 
> > This device is controlled by two GPIO pins, one for enabling and the
> > second one for switching between torch and flash mode.
> 
> How does one enable torch and one enable flash?
> 
> Is the flash-gpio control this or does the enable-gpio enable the flash?

Enabling torch mode means making EN pin high and FLASH pin low.
Enabling flash mode means making EN pin high and FLASH pin high.

The users of this driver can just use standard v4l2 apis or sysfs so I 
wouldn't say this is relevant for the users.

> 
> The DT binding did not indicate what the GPIOs are really going to control.

I'm not sure if this is relevant in the dt bindings because how the device 
works is described in the datasheet and not really relevant for users of the 
binding? I also didn't necessarily want to copy-paste the datasheet into the 
dt bindings because of copyright.

> 
> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > ---
> > Changes since RFC:
> > 
> > - address review comments from Jacek Anaszewski:
> >    - implement strobe_get op
> >    - implement timeout_set op
> >    - init v4l2_sd_cfg variable
> >    - remove init_data.devicename assignemnt
> >    - use devm_ version of led_classdev_flash_register_ext
> >    - release child_node in case of success
> >   
> >   drivers/leds/Kconfig        |   9 ++
> >   drivers/leds/Makefile       |   1 +
> >   drivers/leds/leds-sgm3140.c | 260 ++++++++++++++++++++++++++++++++++++
> >   3 files changed, 270 insertions(+)
> >   create mode 100644 drivers/leds/leds-sgm3140.c
> > 

> SNIP

> > +	if (!child_node) {
> > +		dev_err(&pdev->dev, "No DT child node found for 
connected LED.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +
> > +	ret = of_property_read_u32(child_node, "flash-max-timeout-us",
> > +				   &priv->max_timeout);
> 
> Please use the device_property api's to retrieve DT settings.  Then
> there is no need to release the "of" child node.
> 

I'm guessing you mean

device_property_read_u32(&pdev->dev, "flash-max-timeout-us", &priv-
>max_timeout);

?

I still need the child_node for "init_data.fwnode" and v4l2_flash_init so I 
still have to call of_node_put, right?

> > +	if (ret < 0) {
> 
> if (ret)

Ack

> SNIP

> > +	/* 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;
> 
> Do you need to jump here?  This should just fall out and go through err
> anyway.

Should I just do

if (IS_ERR(priv->v4l2_flash))
	ret = PTR_ERR(priv->v4l2_flash);

?

I thought about removing the goto but I decided to keep it in case code is 
added below that statement so that the goto wouldn't be forgotten. But I can 
change it of course if wanted.

> 
> Dan

Regards
Luca

Re: [PATCH 2/3] dt-bindings: leds: Add binding for sgm3140

[Luca Weiss]

Hi Dan

On Mittwoch, 11. März 2020 13:49:35 CET Dan Murphy wrote:
> Luca
> 
> On 3/9/20 3:35 PM, Luca Weiss wrote:
> > Add YAML devicetree binding for SGMICRO SGM3140 charge pump used for
> > camera flash LEDs.
> > 
> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > ---
> > Changes since RFC:
> > - new patch
> > 
> > I'm not sure about the completeness of this binding as it doesn't
> > mention the led subnode at all.
> > The only existing led yaml binding is leds/leds-max77650.yaml which
> > mentions the subnode but duplicates properties from documented in
> > leds/common.txt.
> > 
> >   .../bindings/leds/leds-sgm3140.yaml           | 53 +++++++++++++++++++
> >   1 file changed, 53 insertions(+)
> >   create mode 100644
> >   Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
> > b/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml new file mode
> > 100644
> > index 000000000000..be9384573d02
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
> > @@ -0,0 +1,53 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/leds/leds-sgm3140.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: SGMICRO SGM3140 500mA Buck/Boost Charge Pump LED Driver
> > +
> > +maintainers:
> > +  - Luca Weiss <luca@z3ntu.xyz>
> > +
> > +description: |
> > +  The SGM3140 is a current-regulated charge pump which can regulate two
> > current +  levels for Flash and Torch modes.
> > +
> > +  It is controlled with two GPIO pins.
> 
> Please define "It".  Not sure what is controlled here.
> 

"It" means the SGM3140. Not sure how else to write that or what the correct 
term for such a component is.

> 
> Dan

Regards
Luca

Re: [PATCH 2/3] dt-bindings: leds: Add binding for sgm3140

[Laurent Pinchart]

Hi Luca,

On Sun, Mar 15, 2020 at 11:47:36AM +0100, Luca Weiss wrote:
> On Mittwoch, 11. März 2020 13:49:35 CET Dan Murphy wrote:
> > On 3/9/20 3:35 PM, Luca Weiss wrote:
> > > Add YAML devicetree binding for SGMICRO SGM3140 charge pump used for
> > > camera flash LEDs.
> > > 
> > > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > > ---
> > > Changes since RFC:
> > > - new patch
> > > 
> > > I'm not sure about the completeness of this binding as it doesn't
> > > mention the led subnode at all.
> > > The only existing led yaml binding is leds/leds-max77650.yaml which
> > > mentions the subnode but duplicates properties from documented in
> > > leds/common.txt.
> > > 
> > >   .../bindings/leds/leds-sgm3140.yaml           | 53 +++++++++++++++++++
> > >   1 file changed, 53 insertions(+)
> > >   create mode 100644
> > >   Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
> > > b/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml new file mode
> > > 100644
> > > index 000000000000..be9384573d02
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
> > > @@ -0,0 +1,53 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/leds/leds-sgm3140.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: SGMICRO SGM3140 500mA Buck/Boost Charge Pump LED Driver
> > > +
> > > +maintainers:
> > > +  - Luca Weiss <luca@z3ntu.xyz>
> > > +
> > > +description: |
> > > +  The SGM3140 is a current-regulated charge pump which can regulate two
> > > current +  levels for Flash and Torch modes.
> > > +
> > > +  It is controlled with two GPIO pins.
> > 
> > Please define "It".  Not sure what is controlled here.
> > 
> 
> "It" means the SGM3140. Not sure how else to write that or what the correct 
> term for such a component is.

Maybe "The device" ? I think Dan's concern is that he wasn't sure if
"It" referred to "the device" or to "flash and torch modes".

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] dt-bindings: Add vendor prefix for SG Micro Corp

[Rob Herring]

On Mon,  9 Mar 2020 21:35:56 +0100, Luca Weiss wrote:
> "SG Micro Corp (SGMICRO) specializes in high performance, high quality
> analog IC design, marketing and sales." (http://www.sg-micro.com/)
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
> Changes since RFC:
> - new patch
> 
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 

Applied, thanks.

Rob

Re: [PATCH 2/3] dt-bindings: leds: Add binding for sgm3140

[Rob Herring]

On Mon, Mar 09, 2020 at 09:35:57PM +0100, Luca Weiss wrote:
> Add YAML devicetree binding for SGMICRO SGM3140 charge pump used for
> camera flash LEDs.
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
> Changes since RFC:
> - new patch
> 
> I'm not sure about the completeness of this binding as it doesn't
> mention the led subnode at all.
> The only existing led yaml binding is leds/leds-max77650.yaml which
> mentions the subnode but duplicates properties from documented in 
> leds/common.txt.

It's common.yaml now. Reference it from a child node defined here.

> 
>  .../bindings/leds/leds-sgm3140.yaml           | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-sgm3140.yaml

Re: [PATCH 2/3] dt-bindings: leds: Add binding for sgm3140

[Dan Murphy]

Luca

On 3/15/20 5:53 AM, Laurent Pinchart wrote:
> Hi Luca,
>
> On Sun, Mar 15, 2020 at 11:47:36AM +0100, Luca Weiss wrote:
>> On Mittwoch, 11. März 2020 13:49:35 CET Dan Murphy wrote:
>>> On 3/9/20 3:35 PM, Luca Weiss wrote:
>>>> Add YAML devicetree binding for SGMICRO SGM3140 charge pump used for
>>>> camera flash LEDs.
>>>>
>>>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
>>>> ---
>>>> Changes since RFC:
>>>> - new patch
>>>>
>>>> I'm not sure about the completeness of this binding as it doesn't
>>>> mention the led subnode at all.
>>>> The only existing led yaml binding is leds/leds-max77650.yaml which
>>>> mentions the subnode but duplicates properties from documented in
>>>> leds/common.txt.
>>>>
>>>>    .../bindings/leds/leds-sgm3140.yaml           | 53 +++++++++++++++++++
>>>>    1 file changed, 53 insertions(+)
>>>>    create mode 100644
>>>>    Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
>>>> b/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml new file mode
>>>> 100644
>>>> index 000000000000..be9384573d02
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
>>>> @@ -0,0 +1,53 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/leds/leds-sgm3140.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: SGMICRO SGM3140 500mA Buck/Boost Charge Pump LED Driver
>>>> +
>>>> +maintainers:
>>>> +  - Luca Weiss <luca@z3ntu.xyz>
>>>> +
>>>> +description: |
>>>> +  The SGM3140 is a current-regulated charge pump which can regulate two
>>>> current +  levels for Flash and Torch modes.
>>>> +
>>>> +  It is controlled with two GPIO pins.
>>> Please define "It".  Not sure what is controlled here.
>>>
>> "It" means the SGM3140. Not sure how else to write that or what the correct
>> term for such a component is.
> Maybe "The device" ? I think Dan's concern is that he wasn't sure if
> "It" referred to "the device" or to "flash and torch modes".

Laurent is correct.  Are the flash and torch modes controlled by GPIOs 
the device or the current levels?



>

Re: [PATCH 2/3] dt-bindings: leds: Add binding for sgm3140

[Luca Weiss]

Hi Rob,

On Montag, 23. März 2020 21:57:27 CET Rob Herring wrote:
> On Mon, Mar 09, 2020 at 09:35:57PM +0100, Luca Weiss wrote:
> > Add YAML devicetree binding for SGMICRO SGM3140 charge pump used for
> > camera flash LEDs.
> > 
> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > ---
> > Changes since RFC:
> > - new patch
> > 
> > I'm not sure about the completeness of this binding as it doesn't
> > mention the led subnode at all.
> > The only existing led yaml binding is leds/leds-max77650.yaml which
> > mentions the subnode but duplicates properties from documented in
> > leds/common.txt.
> 
> It's common.yaml now. Reference it from a child node defined here.

Thanks, that helps a lot!

> 
> >  .../bindings/leds/leds-sgm3140.yaml           | 53 +++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> >  create mode 100644
> >  Documentation/devicetree/bindings/leds/leds-sgm3140.yaml

Regards
Luca