* [PATCH v2 0/2] Add sgm3140 flash led driver
@ 2020-03-30 19:47 Luca Weiss
2020-03-30 19:47 ` [PATCH v2 1/2] dt-bindings: leds: Add binding for sgm3140 Luca Weiss
2020-03-30 19:47 ` [PATCH v2 2/2] leds: add sgm3140 driver Luca Weiss
0 siblings, 2 replies; 10+ messages in thread
From: Luca Weiss @ 2020-03-30 19:47 UTC (permalink / raw)
To: linux-leds
Cc: Dan Murphy, Heiko Stuebner, Icenowy Zheng, Jacek Anaszewski,
Laurent Pinchart, Mark Rutland, Maxime Ripard, Pavel Machek,
Rob Herring, Shawn Guo, devicetree, linux-kernel,
~postmarketos/upstreaming, Luca Weiss
This series introduces a driver for the SGMICRO SGM3140 charge pump
used in the PinePhone smartphone.
Luca Weiss (2):
dt-bindings: leds: Add binding for sgm3140
leds: add sgm3140 driver
.../bindings/leds/leds-sgm3140.yaml | 61 ++++
drivers/leds/Kconfig | 9 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-sgm3140.c | 317 ++++++++++++++++++
4 files changed, 388 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
create mode 100644 drivers/leds/leds-sgm3140.c
--
2.26.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] dt-bindings: leds: Add binding for sgm3140
2020-03-30 19:47 [PATCH v2 0/2] Add sgm3140 flash led driver Luca Weiss
@ 2020-03-30 19:47 ` Luca Weiss
2020-04-03 17:21 ` Dan Murphy
2020-04-10 17:41 ` Rob Herring
2020-03-30 19:47 ` [PATCH v2 2/2] leds: add sgm3140 driver Luca Weiss
1 sibling, 2 replies; 10+ messages in thread
From: Luca Weiss @ 2020-03-30 19:47 UTC (permalink / raw)
To: linux-leds
Cc: Dan Murphy, Heiko Stuebner, Icenowy Zheng, Jacek Anaszewski,
Laurent Pinchart, Mark Rutland, Maxime Ripard, Pavel Machek,
Rob Herring, Shawn Guo, devicetree, linux-kernel,
~postmarketos/upstreaming, 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 v1:
- Add vin-supply
- Add led subnode (common.yaml)
.../bindings/leds/leds-sgm3140.yaml | 61 +++++++++++++++++++
1 file changed, 61 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..24ca178e5d0a
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
@@ -0,0 +1,61 @@
+# 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.
+
+ 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.
+
+ vin-supply:
+ description: Regulator providing power to the 'VIN' pin.
+
+ led:
+ allOf:
+ - $ref: common.yaml#
+
+required:
+ - compatible
+ - flash-gpios
+ - enable-gpios
+
+additionalProperties: false
+
+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 */
+ vin-supply = <®_dcdc1>;
+
+ sgm3140_flash: led {
+ function = LED_FUNCTION_FLASH;
+ color = <LED_COLOR_ID_WHITE>;
+ flash-max-timeout-us = <250000>;
+ };
+ };
--
2.26.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] leds: add sgm3140 driver
2020-03-30 19:47 [PATCH v2 0/2] Add sgm3140 flash led driver Luca Weiss
2020-03-30 19:47 ` [PATCH v2 1/2] dt-bindings: leds: Add binding for sgm3140 Luca Weiss
@ 2020-03-30 19:47 ` Luca Weiss
2020-04-03 17:31 ` Dan Murphy
2020-04-04 9:58 ` Andy Shevchenko
1 sibling, 2 replies; 10+ messages in thread
From: Luca Weiss @ 2020-03-30 19:47 UTC (permalink / raw)
To: linux-leds
Cc: Dan Murphy, Heiko Stuebner, Icenowy Zheng, Jacek Anaszewski,
Laurent Pinchart, Mark Rutland, Maxime Ripard, Pavel Machek,
Rob Herring, Shawn Guo, devicetree, linux-kernel,
~postmarketos/upstreaming, 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 v1:
- Add vin-supply (keep track of 'enabled' state for that)
- Wrap lines
- static const -ify some structs and methods
- use strscpy instead of strlcpy
- remove u32 cast by adding 'U' suffix to constants
- rebase on linux-next
drivers/leds/Kconfig | 9 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-sgm3140.c | 317 ++++++++++++++++++++++++++++++++++++
3 files changed, 327 insertions(+)
create mode 100644 drivers/leds/leds-sgm3140.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 7599dbee8de1..f5beeff16bdd 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -871,6 +871,15 @@ config LEDS_IP30
To compile this driver as a module, choose M here: the module
will be called leds-ip30.
+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 fd61421f7d40..f60ed0c09d4c 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_LEDS_PWM) += leds-pwm.o
obj-$(CONFIG_LEDS_REGULATOR) += leds-regulator.o
obj-$(CONFIG_LEDS_S3C24XX) += leds-s3c24xx.o
obj-$(CONFIG_LEDS_SC27XX_BLTC) += leds-sc27xx-bltc.o
+obj-$(CONFIG_LEDS_SGM3140) += leds-sgm3140.o
obj-$(CONFIG_LEDS_SUNFIRE) += leds-sunfire.o
obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o
obj-$(CONFIG_LEDS_TCA6507) += leds-tca6507.o
diff --git a/drivers/leds/leds-sgm3140.c b/drivers/leds/leds-sgm3140.c
new file mode 100644
index 000000000000..28fe5e34f931
--- /dev/null
+++ b/drivers/leds/leds-sgm3140.c
@@ -0,0 +1,317 @@
+// 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/regulator/consumer.h>
+#include <linux/platform_device.h>
+
+#include <media/v4l2-flash-led-class.h>
+
+#define FLASH_TIMEOUT_DEFAULT 250000U /* 250ms */
+#define FLASH_MAX_TIMEOUT_DEFAULT 300000U /* 300ms */
+
+struct sgm3140 {
+ bool enabled;
+ struct gpio_desc *flash_gpio;
+ struct gpio_desc *enable_gpio;
+ struct regulator *vin_regulator;
+
+ /* 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);
+ int ret;
+
+ if (priv->enabled == state)
+ return 0;
+
+ if (state) {
+ ret = regulator_enable(priv->vin_regulator);
+ if (ret) {
+ dev_err(fled_cdev->led_cdev.dev,
+ "failed to enable regulator: %d\n", ret);
+ return ret;
+ }
+ 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 {
+ del_timer_sync(&priv->powerdown_timer);
+ gpiod_set_value_cansleep(priv->enable_gpio, 0);
+ gpiod_set_value_cansleep(priv->flash_gpio, 0);
+ ret = regulator_disable(priv->vin_regulator);
+ if (ret) {
+ dev_err(fled_cdev->led_cdev.dev,
+ "failed to disable regulator: %d\n", ret);
+ return ret;
+ }
+ }
+
+ priv->enabled = state;
+
+ 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;
+}
+
+static const struct led_flash_ops sgm3140_flash_ops = {
+ .strobe_set = sgm3140_strobe_set,
+ .strobe_get = sgm3140_strobe_get,
+ .timeout_set = sgm3140_timeout_set,
+};
+
+static 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);
+ bool enable = brightness == LED_ON;
+ int ret;
+
+ if (priv->enabled == enable)
+ return 0;
+
+ if (enable) {
+ ret = regulator_enable(priv->vin_regulator);
+ if (ret) {
+ dev_err(led_cdev->dev,
+ "failed to enable regulator: %d\n", ret);
+ return ret;
+ }
+ gpiod_set_value_cansleep(priv->enable_gpio, 1);
+ } else {
+ gpiod_set_value_cansleep(priv->enable_gpio, 0);
+ ret = regulator_disable(priv->vin_regulator);
+ if (ret) {
+ dev_err(led_cdev->dev,
+ "failed to disable regulator: %d\n", ret);
+ return ret;
+ }
+ }
+
+ priv->enabled = enable;
+
+ return 0;
+}
+
+static void sgm3140_powerdown_timer(struct timer_list *t)
+{
+ struct sgm3140 *priv = from_timer(priv, t, powerdown_timer);
+
+ gpiod_set_value(priv->enable_gpio, 0);
+ gpiod_set_value(priv->flash_gpio, 0);
+ regulator_disable(priv->vin_regulator);
+
+ priv->enabled = false;
+}
+
+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;
+
+ strscpy(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;
+ }
+
+ priv->vin_regulator = devm_regulator_get(&pdev->dev, "vin");
+ ret = PTR_ERR_OR_ZERO(priv->vin_regulator);
+ if (ret) {
+ if (ret != -EPROBE_DEFER)
+ dev_err(&pdev->dev,
+ "Failed to request regulator: %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) {
+ 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, 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) {
+ 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.26.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: leds: Add binding for sgm3140
2020-03-30 19:47 ` [PATCH v2 1/2] dt-bindings: leds: Add binding for sgm3140 Luca Weiss
@ 2020-04-03 17:21 ` Dan Murphy
2020-04-10 17:41 ` Rob Herring
1 sibling, 0 replies; 10+ messages in thread
From: Dan Murphy @ 2020-04-03 17:21 UTC (permalink / raw)
To: Luca Weiss, linux-leds
Cc: Heiko Stuebner, Icenowy Zheng, Jacek Anaszewski,
Laurent Pinchart, Mark Rutland, Maxime Ripard, Pavel Machek,
Rob Herring, Shawn Guo, devicetree, linux-kernel,
~postmarketos/upstreaming
Luca
On 3/30/20 2:47 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 v1:
> - Add vin-supply
> - Add led subnode (common.yaml)
>
> .../bindings/leds/leds-sgm3140.yaml | 61 +++++++++++++++++++
> 1 file changed, 61 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..24ca178e5d0a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
> @@ -0,0 +1,61 @@
> +# 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.
> +
> + 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.
> +
> + vin-supply:
> + description: Regulator providing power to the 'VIN' pin.
> +
> + led:
> + allOf:
> + - $ref: common.yaml#
> +
> +required:
> + - compatible
> + - flash-gpios
> + - enable-gpios
> +
> +additionalProperties: false
> +
> +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 */
> + vin-supply = <®_dcdc1>;
> +
> + sgm3140_flash: led {
> + function = LED_FUNCTION_FLASH;
> + color = <LED_COLOR_ID_WHITE>;
> + flash-max-timeout-us = <250000>;
> + };
> + };
Reviewed-by: Dan Murphy <dmurphy@ti.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] leds: add sgm3140 driver
2020-03-30 19:47 ` [PATCH v2 2/2] leds: add sgm3140 driver Luca Weiss
@ 2020-04-03 17:31 ` Dan Murphy
2020-04-04 9:36 ` Luca Weiss
2020-04-04 9:58 ` Andy Shevchenko
1 sibling, 1 reply; 10+ messages in thread
From: Dan Murphy @ 2020-04-03 17:31 UTC (permalink / raw)
To: Luca Weiss, linux-leds
Cc: Heiko Stuebner, Icenowy Zheng, Jacek Anaszewski,
Laurent Pinchart, Mark Rutland, Maxime Ripard, Pavel Machek,
Rob Herring, Shawn Guo, devicetree, linux-kernel,
~postmarketos/upstreaming
Luca
On 3/30/20 2:47 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.
>
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
> Changes since v1:
> - Add vin-supply (keep track of 'enabled' state for that)
> - Wrap lines
> - static const -ify some structs and methods
> - use strscpy instead of strlcpy
> - remove u32 cast by adding 'U' suffix to constants
> - rebase on linux-next
>
> drivers/leds/Kconfig | 9 +
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-sgm3140.c | 317 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 327 insertions(+)
> create mode 100644 drivers/leds/leds-sgm3140.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 7599dbee8de1..f5beeff16bdd 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -871,6 +871,15 @@ config LEDS_IP30
> To compile this driver as a module, choose M here: the module
> will be called leds-ip30.
>
> +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 fd61421f7d40..f60ed0c09d4c 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -77,6 +77,7 @@ obj-$(CONFIG_LEDS_PWM) += leds-pwm.o
> obj-$(CONFIG_LEDS_REGULATOR) += leds-regulator.o
> obj-$(CONFIG_LEDS_S3C24XX) += leds-s3c24xx.o
> obj-$(CONFIG_LEDS_SC27XX_BLTC) += leds-sc27xx-bltc.o
> +obj-$(CONFIG_LEDS_SGM3140) += leds-sgm3140.o
> obj-$(CONFIG_LEDS_SUNFIRE) += leds-sunfire.o
> obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o
> obj-$(CONFIG_LEDS_TCA6507) += leds-tca6507.o
> diff --git a/drivers/leds/leds-sgm3140.c b/drivers/leds/leds-sgm3140.c
> new file mode 100644
> index 000000000000..28fe5e34f931
> --- /dev/null
> +++ b/drivers/leds/leds-sgm3140.c
> @@ -0,0 +1,317 @@
> +// 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/regulator/consumer.h>
> +#include <linux/platform_device.h>
> +
> +#include <media/v4l2-flash-led-class.h>
> +
> +#define FLASH_TIMEOUT_DEFAULT 250000U /* 250ms */
> +#define FLASH_MAX_TIMEOUT_DEFAULT 300000U /* 300ms */
> +
> +struct sgm3140 {
> + bool enabled;
> + struct gpio_desc *flash_gpio;
> + struct gpio_desc *enable_gpio;
> + struct regulator *vin_regulator;
> +
> + /* 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);
> + int ret;
> +
> + if (priv->enabled == state)
> + return 0;
> +
> + if (state) {
> + ret = regulator_enable(priv->vin_regulator);
> + if (ret) {
> + dev_err(fled_cdev->led_cdev.dev,
> + "failed to enable regulator: %d\n", ret);
> + return ret;
> + }
> + 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 {
> + del_timer_sync(&priv->powerdown_timer);
> + gpiod_set_value_cansleep(priv->enable_gpio, 0);
> + gpiod_set_value_cansleep(priv->flash_gpio, 0);
> + ret = regulator_disable(priv->vin_regulator);
> + if (ret) {
> + dev_err(fled_cdev->led_cdev.dev,
> + "failed to disable regulator: %d\n", ret);
> + return ret;
> + }
> + }
> +
> + priv->enabled = state;
> +
> + 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;
> +}
> +
> +static const struct led_flash_ops sgm3140_flash_ops = {
> + .strobe_set = sgm3140_strobe_set,
> + .strobe_get = sgm3140_strobe_get,
> + .timeout_set = sgm3140_timeout_set,
> +};
> +
> +static 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);
> + bool enable = brightness == LED_ON;
> + int ret;
> +
> + if (priv->enabled == enable)
> + return 0;
> +
> + if (enable) {
> + ret = regulator_enable(priv->vin_regulator);
> + if (ret) {
> + dev_err(led_cdev->dev,
> + "failed to enable regulator: %d\n", ret);
> + return ret;
> + }
> + gpiod_set_value_cansleep(priv->enable_gpio, 1);
> + } else {
> + gpiod_set_value_cansleep(priv->enable_gpio, 0);
> + ret = regulator_disable(priv->vin_regulator);
> + if (ret) {
> + dev_err(led_cdev->dev,
> + "failed to disable regulator: %d\n", ret);
> + return ret;
> + }
> + }
> +
> + priv->enabled = enable;
> +
> + return 0;
> +}
> +
> +static void sgm3140_powerdown_timer(struct timer_list *t)
> +{
> + struct sgm3140 *priv = from_timer(priv, t, powerdown_timer);
> +
> + gpiod_set_value(priv->enable_gpio, 0);
> + gpiod_set_value(priv->flash_gpio, 0);
> + regulator_disable(priv->vin_regulator);
> +
> + priv->enabled = false;
> +}
> +
> +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;
> +
> + strscpy(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;
> + }
> +
> + priv->vin_regulator = devm_regulator_get(&pdev->dev, "vin");
> + ret = PTR_ERR_OR_ZERO(priv->vin_regulator);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->dev,
> + "Failed to request regulator: %d\n", ret);
> + return ret;
This regulator is optional so why would you return here? You should
only return if -EPROBE_DEFER.
> + }
> +
> + child_node = of_get_next_available_child(pdev->dev.of_node, NULL);
Maybe this should be the first check before doing all the processing to
make sure that the DT is not
malformed.
> + 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) {
> + 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, 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) {
> + 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;
Not sure why this is here you are not in a for loop and this will fall
through anyway to the err label.
> + }
> +
> +err:
> + of_node_put(child_node);
> + return ret;
> +}
> +
Dan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] leds: add sgm3140 driver
2020-04-03 17:31 ` Dan Murphy
@ 2020-04-04 9:36 ` Luca Weiss
2020-04-04 13:56 ` Dan Murphy
0 siblings, 1 reply; 10+ messages in thread
From: Luca Weiss @ 2020-04-04 9:36 UTC (permalink / raw)
To: linux-leds, Dan Murphy
Cc: Heiko Stuebner, Icenowy Zheng, Jacek Anaszewski,
Laurent Pinchart, Mark Rutland, Maxime Ripard, Pavel Machek,
Rob Herring, Shawn Guo, devicetree, linux-kernel,
~postmarketos/upstreaming
Hi Dan,
On Freitag, 3. April 2020 19:31:52 CEST Dan Murphy wrote:
> Luca
>
> On 3/30/20 2:47 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.
> >
> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > ---
> > Changes since v1:
> > - Add vin-supply (keep track of 'enabled' state for that)
> > - Wrap lines
> > - static const -ify some structs and methods
> > - use strscpy instead of strlcpy
> > - remove u32 cast by adding 'U' suffix to constants
> > - rebase on linux-next
> >
> > drivers/leds/Kconfig | 9 +
> > drivers/leds/Makefile | 1 +
> > drivers/leds/leds-sgm3140.c | 317 ++++++++++++++++++++++++++++++++++++
> > 3 files changed, 327 insertions(+)
> > create mode 100644 drivers/leds/leds-sgm3140.c
> >
-snip-
> > +
> > + priv->vin_regulator = devm_regulator_get(&pdev->dev, "vin");
> > + ret = PTR_ERR_OR_ZERO(priv->vin_regulator);
> > + if (ret) {
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(&pdev->dev,
> > + "Failed to request regulator:
%d\n", ret);
> > + return ret;
>
> This regulator is optional so why would you return here? You should
> only return if -EPROBE_DEFER.
If the regulator is not specified in the dts, then a dummy regulator will be
used:
[ 1.027114] sgm3140 sgm3140: sgm3140 supply vin not found, using dummy
regulator
So this code will only be called if something really failed (or was defered)
>
> > + }
> > +
> > + child_node = of_get_next_available_child(pdev->dev.of_node, NULL);
>
> Maybe this should be the first check before doing all the processing to
> make sure that the DT is not
>
> malformed.
If e.g. the devm_gpiod_get calls fail (because the gpios weren't declared in
the dts) then the dt is also "malformed" which isn't as different as the
subnode being missing imo. I don't think it matters much here. And this way I
don't have to care about calling of_node_put in case of an error for the
statements above.
>
> > + 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) {
> > + 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, 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) {
> > + 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;
>
> Not sure why this is here you are not in a for loop and this will fall
> through anyway to the err label.
>
I kept the goto in, in case more code is added below that statement so the
author doesn't forget that this error needs to be handled.
If wanted I can remove it of course.
> > + }
> > +
> > +err:
> > + of_node_put(child_node);
> > + return ret;
> > +}
> > +
>
> Dan
Regards
Luca
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] leds: add sgm3140 driver
2020-03-30 19:47 ` [PATCH v2 2/2] leds: add sgm3140 driver Luca Weiss
2020-04-03 17:31 ` Dan Murphy
@ 2020-04-04 9:58 ` Andy Shevchenko
2020-04-05 18:45 ` Luca Weiss
1 sibling, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2020-04-04 9:58 UTC (permalink / raw)
To: Luca Weiss
Cc: Linux LED Subsystem, Dan Murphy, Heiko Stuebner, Icenowy Zheng,
Jacek Anaszewski, Laurent Pinchart, Mark Rutland, Maxime Ripard,
Pavel Machek, Rob Herring, Shawn Guo, devicetree,
Linux Kernel Mailing List, ~postmarketos/upstreaming
On Mon, Mar 30, 2020 at 10:49 PM Luca Weiss <luca@z3ntu.xyz> 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.
...
> +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
depends on OF || COMPILE_TEST ?
But hold on...
...
> +#include <linux/of.h>
Perhaps switch this to property.h and replace OF with more generic
device property / fwnode API?
...
> +struct sgm3140 {
> + bool enabled;
> + struct gpio_desc *flash_gpio;
> + struct gpio_desc *enable_gpio;
> + struct regulator *vin_regulator;
> +
> + /* current timeout in us */
> + u32 timeout;
> + /* maximum timeout in us */
> + u32 max_timeout;
> +
> + struct led_classdev_flash fled_cdev;
I guess it might be slightly better to make it first member of the
struct (I didn't check but the rationale is to put more often used
members at the beginning to utilize cachelines).
> + 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);
> +}
...and this becomes a no-op AFAICS (doesn't mean you need to remove it).
...
> + struct device_node *child_node;
> + child_node = of_get_next_available_child(pdev->dev.of_node, NULL);
> + ret = of_property_read_u32(child_node, "flash-max-timeout-us",
> + &priv->max_timeout);
> + init_data.fwnode = of_fwnode_handle(child_node);
> + of_node_put(child_node);
Device property / fwnode API?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] leds: add sgm3140 driver
2020-04-04 9:36 ` Luca Weiss
@ 2020-04-04 13:56 ` Dan Murphy
0 siblings, 0 replies; 10+ messages in thread
From: Dan Murphy @ 2020-04-04 13:56 UTC (permalink / raw)
To: Luca Weiss, linux-leds
Cc: Heiko Stuebner, Icenowy Zheng, Jacek Anaszewski,
Laurent Pinchart, Mark Rutland, Maxime Ripard, Pavel Machek,
Rob Herring, Shawn Guo, devicetree, linux-kernel,
~postmarketos/upstreaming
Luca
On 4/4/20 4:36 AM, Luca Weiss wrote:
> + fled_cdev, NULL,
<snip>
>>> + &v4l2_sd_cfg);
>>> + if (IS_ERR(priv->v4l2_flash)) {
>>> + ret = PTR_ERR(priv->v4l2_flash);
>>> + goto err;
>> Not sure why this is here you are not in a for loop and this will fall
>> through anyway to the err label.
>>
> I kept the goto in, in case more code is added below that statement so the
> author doesn't forget that this error needs to be handled.
> If wanted I can remove it of course.
>
I am ok with all the reasoning in the previous comments. This one I
would say just fall through to out.
If there is other code added after this then it can be added in.
Dan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] leds: add sgm3140 driver
2020-04-04 9:58 ` Andy Shevchenko
@ 2020-04-05 18:45 ` Luca Weiss
0 siblings, 0 replies; 10+ messages in thread
From: Luca Weiss @ 2020-04-05 18:45 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linux LED Subsystem, Dan Murphy, Heiko Stuebner, Icenowy Zheng,
Jacek Anaszewski, Laurent Pinchart, Mark Rutland, Maxime Ripard,
Pavel Machek, Rob Herring, Shawn Guo, devicetree,
Linux Kernel Mailing List, ~postmarketos/upstreaming
Hi Andy,
On Samstag, 4. April 2020 11:58:31 CEST Andy Shevchenko wrote:
> On Mon, Mar 30, 2020 at 10:49 PM Luca Weiss <luca@z3ntu.xyz> 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.
>
> ...
>
> > +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
>
> depends on OF || COMPILE_TEST ?
> But hold on...
>
> ...
>
> > +#include <linux/of.h>
>
> Perhaps switch this to property.h and replace OF with more generic
> device property / fwnode API?
>
I didn't find clear documentation on this, the functions in drivers/base/
property.c can be used instead of the of_* (device tree) functions?
As far as I can tell, the device_property_* functions are supposed to be used
for simple "give me a property for this 'struct device*'" while the fwnode_*
functions are used as generic equivalent of the of_* functions?
So in this case I can replace
struct device_node *child_node;
child_node = of_get_next_available_child(pdev->dev.of_node, NULL);
with
struct fwnode_handle *child_node;
child_node = fwnode_get_next_available_child_node(pdev->dev.fwnode, NULL);
and then instead of
ret = of_property_read_u32(child_node, "flash-max-timeout-us",
&priv->max_timeout);
use
ret = fwnode_property_read_u32(child_node, "flash-max-timeout-us",
&priv->max_timeout);
and finally instead of
init_data.fwnode = of_fwnode_handle(child_node);
I can probably directly do
init_data.fwnode = child_node;
Does that sound correct?
> ...
>
> > +struct sgm3140 {
> > + bool enabled;
> > + struct gpio_desc *flash_gpio;
> > + struct gpio_desc *enable_gpio;
> > + struct regulator *vin_regulator;
> > +
> > + /* current timeout in us */
> > + u32 timeout;
> > + /* maximum timeout in us */
> > + u32 max_timeout;
> > +
> >
> > + struct led_classdev_flash fled_cdev;
>
> I guess it might be slightly better to make it first member of the
> struct (I didn't check but the rationale is to put more often used
> members at the beginning to utilize cachelines).
>
> > + 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);
> > +}
>
> ...and this becomes a no-op AFAICS (doesn't mean you need to remove it).
>
> ...
>
> > + struct device_node *child_node;
> >
> > + child_node = of_get_next_available_child(pdev->dev.of_node, NULL);
> >
> > + ret = of_property_read_u32(child_node, "flash-max-timeout-us",
> > + &priv->max_timeout);
> >
> > + init_data.fwnode = of_fwnode_handle(child_node);
> >
> > + of_node_put(child_node);
>
> Device property / fwnode API?
>
> --
> With Best Regards,
> Andy Shevchenko
Regards
Luca
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: leds: Add binding for sgm3140
2020-03-30 19:47 ` [PATCH v2 1/2] dt-bindings: leds: Add binding for sgm3140 Luca Weiss
2020-04-03 17:21 ` Dan Murphy
@ 2020-04-10 17:41 ` Rob Herring
1 sibling, 0 replies; 10+ messages in thread
From: Rob Herring @ 2020-04-10 17:41 UTC (permalink / raw)
To: Luca Weiss
Cc: linux-leds, Dan Murphy, Heiko Stuebner, Icenowy Zheng,
Jacek Anaszewski, Laurent Pinchart, Mark Rutland, Maxime Ripard,
Pavel Machek, Shawn Guo, devicetree, linux-kernel,
~postmarketos/upstreaming
On Mon, Mar 30, 2020 at 09:47:56PM +0200, 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 v1:
> - Add vin-supply
> - Add led subnode (common.yaml)
>
> .../bindings/leds/leds-sgm3140.yaml | 61 +++++++++++++++++++
> 1 file changed, 61 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..24ca178e5d0a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-sgm3140.yaml
> @@ -0,0 +1,61 @@
> +# 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.
> +
> + 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.
> +
> + vin-supply:
> + description: Regulator providing power to the 'VIN' pin.
> +
> + led:
Needs 'type: object'
With that,
Reviewed-by: Rob Herring <robh@kernel.org>
> + allOf:
> + - $ref: common.yaml#
> +
> +required:
> + - compatible
> + - flash-gpios
> + - enable-gpios
> +
> +additionalProperties: false
> +
> +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 */
> + vin-supply = <®_dcdc1>;
> +
> + sgm3140_flash: led {
> + function = LED_FUNCTION_FLASH;
> + color = <LED_COLOR_ID_WHITE>;
> + flash-max-timeout-us = <250000>;
> + };
> + };
> --
> 2.26.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-04-10 17:42 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-30 19:47 [PATCH v2 0/2] Add sgm3140 flash led driver Luca Weiss
2020-03-30 19:47 ` [PATCH v2 1/2] dt-bindings: leds: Add binding for sgm3140 Luca Weiss
2020-04-03 17:21 ` Dan Murphy
2020-04-10 17:41 ` Rob Herring
2020-03-30 19:47 ` [PATCH v2 2/2] leds: add sgm3140 driver Luca Weiss
2020-04-03 17:31 ` Dan Murphy
2020-04-04 9:36 ` Luca Weiss
2020-04-04 13:56 ` Dan Murphy
2020-04-04 9:58 ` Andy Shevchenko
2020-04-05 18:45 ` Luca Weiss
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).