Skip to contentSkip to conversation

Samsung Expressatt: Camera Flash

PATCH In review Rudraksha Gupta 4 participants 11 messages Sep 13 – Sep 15

Subsystems (4)
Device tree bindings Arm Qualcomm mailing list Arm Qualcomm support LED
lore
Share & tools
lore

Rudraksha Gupta

author
#0 lore raw

v7 cover Samsung Expressatt: Camera Flash

This small series adds camera flash to an existing similar mainline driver and adds it to the Samsung Expressatt's DTS

// Tests

// # Navigate to LED
sudo su
cd /sys/class/leds/white:flash

// # Should stay at dim brightness
echo 1 > brightness
echo 1 > brightness
echo 1 > brightness
echo 1 > brightness
echo 0 > brightness # LED_OFF

// # Max Brightness
echo 50 > brightness
echo 0 > brightness # LED_OFF
echo 99 > brightness
echo 0 > brightness # LED_OFF
echo 1000 > brightness
echo 0 > brightness # LED_OFF
echo 100 > brightness
echo 0 > brightness # LED_OFF

// # Should increase in brightness
for i in $(seq 1 16); do echo $i > brightness; sleep 1; done
echo 0 > brightness # LED_OFF

// # Test flash strobe (rt8515_led_flash_strobe_set)

cat max_flash_timeout          # check max
echo 200000 > flash_timeout    # 200ms
echo 1 > flash_strobe          # strobe ON → brightness_commit + timer
cat flash_strobe               # should read 1, then 0 after timeout

sleep 1
cat flash_strobe # should be 0 (timer fired)

// # Test manual strobe cancel
echo 1 > flash_strobe ; echo 0 > flash_strobe # immediate off

// # Check regulator error handling
dmesg | tail -20 # look for any "failed to turn off LED" msgs

// # Multiple strobes shouldn't cause errors
echo 200000 > flash_timeout
echo 1 > flash_strobe
echo 1 > flash_strobe
sleep 1
cat flash_strobe
dmesg | tail -20

// # Multiple strobes shouldn't cause errors (extreme test)
for i in $(seq 1 500); do echo 1 > flash_strobe; echo 0 > flash_strobe; done dmesg | tail -20

// # Test minimal timeout (turns on briefly)
echo 1 > flash_timeout
echo 1 > flash_strobe
sleep 0.1
cat flash_strobe
dmesg | tail -20

// # Flash strobe overrides brightness and flashes at full brightness echo 200000 > flash_timeout
echo 1 > brightness
echo 1 > flash_strobe
sleep 1
cat flash_strobe
cat brightness
dmesg | tail -20

Downstream reference:

Link https://github.com/LineageOS/android_kernel_samsung_d2/blob/stable/cm-12.0-YNG4N/drivers/leds/Makefile#L51
Link https://github.com/LineageOS/android_kernel_samsung_d2/blob/stable/cm-12.0-YNG4N/arch/arm/mach-msm/board-apexq-camera.c#L591
Signed-off-by Rudraksha Gupta <[email protected]>

---
Changes in v7:
- address Lee's comments
- PM8921_MPP_S4 instead of PM8921_GPIO_S4 for the MPP4 power-source
- Link to v6: https://lore.kernel.org/r/[email protected]

Changes in v6:
- Address Lee Jones's comments:
- formatting
- use cancel_delayed_work_sync()
- Link to v5: https://lore.kernel.org/r/[email protected]

Changes in v5:
- Address Lee Jones's comments:
- formatting
- use devm_regulator_get_optional()
- use a workqueue instead of a timer
- previously we were validating ent-gpios xor vin-supply at probe,
but was removed. update the commit msg to reflect this
- Link to v4: https://lore.kernel.org/r/[email protected]

Changes in v4:
- Driver:
- revert function renames
- add comment to use flash instead if torch pin not available
- Link to v3: https://lore.kernel.org/r/[email protected]

Changes in v3:
- DTS:
- Renamed and reordered nodes
- Driver:
- Use regulator_is_enabled() instead of reg_enabled
- remove ent xor vin check
- remove rt->reg == -ENODEV check
- rename functions to reflect what they do and added ret's
- Fixed: LED was increasing in brightness when setting the same
brightness multiple times
- Link to v2: https://lore.kernel.org/r/[email protected]

Changes in v2:
- dt-bindings: Explain the hardware and not the driver
- **/*: Use vin-supply instead of unlock-gpio
- expressatt DTS: Reorder pinctrl-*
- expressatt DTS: Define rfs-ohms to a default (couldn't find
information about this)
- Link to v1: https://lore.kernel.org/r/[email protected]

To: Lee Jones <[email protected]>
To: Pavel Machek <[email protected]>
To: Rob Herring <[email protected]>
To: Krzysztof Kozlowski <[email protected]>
To: Conor Dooley <[email protected]>
To: Linus Walleij <[email protected]>
To: Liam Girdwood <[email protected]>
To: Mark Brown <[email protected]>
To: Bjorn Andersson <[email protected]>
To: Konrad Dybcio <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

---
Rudraksha Gupta (3):
dt-bindings: leds: rt8515: Support single-GPIO flash ICs with vin supply
leds: flash: rt8515: Support single-GPIO flash ICs with vin supply
ARM: dts: qcom: msm8960: expressatt: Add camera flash

 .../devicetree/bindings/leds/richtek,rt8515.yaml   |  34 ++++-
 .../dts/qcom/qcom-msm8960-samsung-expressatt.dts   |  44 ++++++
 drivers/leds/flash/leds-rt8515.c                   | 147 +++++++++++++++------
 3 files changed, 184 insertions(+), 41 deletions(-)

---
base-commit: 237a1c39e8dfd3e1c6f1f023eea37a48ec04cc63
change-id: 20260306-expressatt_camera_flash-13c15a7427aa
prerequisite-message-id: <[email protected]>
prerequisite-patch-id: 2b24e9ba47500ef05d38e37b40d8ca2dceb4716a

Best regards,

Signature · 1 line
Rudraksha Gupta <[email protected]>
author
#1 lore raw

PATCH v7 1/3 dt-bindings: leds: rt8515: Support single-GPIO flash ICs with vin supply

Some flash ICs use the same one-wire pulse-count protocol as the RT8515 but have only a single enable line for both flash and torch modes, plus an optional input voltage supply (e.g. a GPIO-controlled fixed regulator) that gates power to the chip.

Make ent-gpios optional and add a vin-supply property to support these variants. Add a oneOf constraint requiring exactly one of ent-gpios or vin-supply. Add a binding example showing the single-GPIO configuration with an input supply.

Assisted-by Claude:claude-opus-4.6
Acked-by Conor Dooley <[email protected]>
Reviewed-by Linus Walleij <[email protected]>
Signed-off-by Rudraksha Gupta <[email protected]>
---
 .../devicetree/bindings/leds/richtek,rt8515.yaml   | 34 +++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/leds/richtek,rt8515.yaml +33 -1
index 0356371a6b01..ab3c5139132c 100644
@@ -15,6 +15,10 @@ description: |
   current for each mode is defined in hardware using two resistors
   RFS and RTS.
 
+  Some flash ICs use the same one-wire pulse-count protocol but have
+  only a single enable line for both flash and torch modes. For these
+  single-channel variants, only enf-gpios is needed.
+
 properties:
   compatible:
     const: richtek,rt8515

@@ -26,6 +30,11 @@ properties:
   ent-gpios:
     maxItems: 1
     description: A connection to the 'ENT' (enable torch) pin.
+      Not present on single-channel flash ICs that use only one enable
+      line for both flash and torch modes.
+
+  vin-supply:
+    description: Optional input supply for the flash IC.
 
   richtek,rfs-ohms:
     minimum: 7680

@@ -81,10 +90,15 @@ properties:
 
 required:
   - compatible
-  - ent-gpios
   - enf-gpios
   - led
 
+oneOf:
+  - required:
+      - ent-gpios
+  - required:
+      - vin-supply
+
 additionalProperties: false
 
 examples:

@@ -108,4 +122,22 @@ examples:
         };
     };
 
+  - |
+    /* Single-channel flash IC with input supply */
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/leds/common.h>
+
+    led-controller {
+        compatible = "richtek,rt8515";
+        enf-gpios = <&tlmm 3 GPIO_ACTIVE_HIGH>;
+        vin-supply = <&flash_reg>;
+        richtek,rfs-ohms = <16000>;
+
+        led {
+            function = LED_FUNCTION_FLASH;
+            color = <LED_COLOR_ID_WHITE>;
+            flash-max-timeout-us = <250000>;
+        };
+    };
+
 ...
Signature · 1 line
2.55.0
author
#5 lore raw

PATCH v7 2/3 leds: flash: rt8515: Support single-GPIO flash ICs with vin supply

In reply to Rudraksha Gupta

Extend the RT8515 driver to support flash ICs that use only a single GPIO for both flash and torch modes (no separate ENT pin), with an optional vin regulator that gates power to the flash IC.

Assisted-by Claude:claude-opus-4.6
Reviewed-by Linus Walleij <[email protected]>
Signed-off-by Rudraksha Gupta <[email protected]>
---
 drivers/leds/flash/leds-rt8515.c | 147 ++++++++++++++++++++++++++++-----------
 1 file changed, 107 insertions(+), 40 deletions(-)
drivers/leds/flash/leds-rt8515.c +107 -40
index 00904cc90ed6..541f61e13f01 100644
@@ -30,6 +30,7 @@
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/regulator/consumer.h>
+#include <linux/workqueue.h>
 
 #include <media/v4l2-flash-led-class.h>
 

@@ -43,6 +44,9 @@
 #define RT8515_TIMEOUT_US	250000U
 #define RT8515_MAX_TIMEOUT_US	300000U
 
+#define RT8515_OFF		0
+#define RT8515_ON		1
+
 struct rt8515 {
 	struct led_classdev_flash fled;
 	struct device *dev;

@@ -51,7 +55,7 @@ struct rt8515 {
 	struct regulator *reg;
 	struct gpio_desc *enable_torch;
 	struct gpio_desc *enable_flash;
-	struct timer_list powerdown_timer;
+	struct delayed_work powerdown_work;
 	u32 max_timeout; /* Flash max timeout */
 	int flash_max_intensity;
 	int torch_max_intensity;

@@ -62,27 +66,50 @@ static struct rt8515 *to_rt8515(struct led_classdev_flash *fled)
 	return container_of(fled, struct rt8515, fled);
 }
 
-static void rt8515_gpio_led_off(struct rt8515 *rt)
+static int rt8515_gpio_led_off(struct rt8515 *rt)
 {
-	gpiod_set_value(rt->enable_flash, 0);
-	gpiod_set_value(rt->enable_torch, 0);
+	gpiod_set_value(rt->enable_flash, RT8515_OFF);
+	gpiod_set_value(rt->enable_torch, RT8515_OFF);
+
+	if (!rt->reg)
+		return 0;
+
+	return regulator_is_enabled(rt->reg) > 0 ? regulator_disable(rt->reg) : 0;
 }
 
-static void rt8515_gpio_brightness_commit(struct gpio_desc *gpiod,
-					  int brightness)
+static int rt8515_gpio_brightness_commit(struct rt8515 *rt, struct gpio_desc *gpiod, int brightness)
 {
-	int i;
+	int ret;
+
+	/*
+	 * Reset the IC to start brightness from zero,
+	 * then re-enable and pulse to the desired level.
+	 */
+	ret = rt8515_gpio_led_off(rt);
+	if (ret)
+		return ret;
+
+	/* IC needs time to reset its brightness counter */
+	usleep_range(100, 200);
+
+	if (rt->reg) {
+		ret = regulator_enable(rt->reg);
+		if (ret)
+			return ret;
+	}
 
 	/*
 	 * Toggling a GPIO line with a small delay increases the
 	 * brightness one step at a time.
 	 */
-	for (i = 0; i < brightness; i++) {
-		gpiod_set_value(gpiod, 0);
+	for (int i = 0; i < brightness; i++) {
+		gpiod_set_value(gpiod, RT8515_OFF);
 		udelay(1);
-		gpiod_set_value(gpiod, 1);
+		gpiod_set_value(gpiod, RT8515_ON);
 		udelay(1);
 	}
+
+	return 0;
 }
 
 /* This is setting the torch light level */

@@ -91,23 +118,37 @@ static int rt8515_led_brightness_set(struct led_classdev *led,
 {
 	struct led_classdev_flash *fled = lcdev_to_flcdev(led);
 	struct rt8515 *rt = to_rt8515(fled);
+	int ret = 0;
 
 	mutex_lock(&rt->lock);
 
 	if (brightness == LED_OFF) {
-		/* Off */
-		rt8515_gpio_led_off(rt);
+		ret = rt8515_gpio_led_off(rt);
+		if (ret)
+			goto out;
 	} else if (brightness < RT8515_TORCH_MAX) {
-		/* Step it up to movie mode brightness using the flash pin */
-		rt8515_gpio_brightness_commit(rt->enable_torch, brightness);
+		/*
+		 * Step it up to movie mode brightness.
+		 * If there is no separate torch pin, use the flash pin
+		 * for torch as well.
+		 */
+		ret = rt8515_gpio_brightness_commit(rt,
+				rt->enable_torch ?: rt->enable_flash, brightness);
+		if (ret)
+			goto out;
 	} else {
-		/* Max torch brightness requested */
-		gpiod_set_value(rt->enable_torch, 1);
+		/*
+		 * Max torch brightness requested.
+		 * If there is no separate torch pin, use the flash pin
+		 * for torch as well.
+		 */
+		gpiod_set_value(rt->enable_torch ?: rt->enable_flash, RT8515_ON);
 	}
 
+out:
 	mutex_unlock(&rt->lock);
 
-	return 0;
+	return ret;
 }
 
 static int rt8515_led_flash_strobe_set(struct led_classdev_flash *fled,

@@ -116,27 +157,35 @@ static int rt8515_led_flash_strobe_set(struct led_classdev_flash *fled,
 	struct rt8515 *rt = to_rt8515(fled);
 	struct led_flash_setting *timeout = &fled->timeout;
 	int brightness = rt->flash_max_intensity;
+	int ret;
+
+	if (!state) {
+		cancel_delayed_work_sync(&rt->powerdown_work);
+		mutex_lock(&rt->lock);
+		ret = rt8515_gpio_led_off(rt);
+		if (!ret)
+			fled->led_cdev.brightness = LED_OFF;
+		mutex_unlock(&rt->lock);
+		return ret;
+	}
 
 	mutex_lock(&rt->lock);
 
-	if (state) {
-		/* Enable LED flash mode and set brightness */
-		rt8515_gpio_brightness_commit(rt->enable_flash, brightness);
-		/* Set timeout */
-		mod_timer(&rt->powerdown_timer,
-			  jiffies + usecs_to_jiffies(timeout->val));
-	} else {
-		timer_delete_sync(&rt->powerdown_timer);
-		/* Turn the LED off */
-		rt8515_gpio_led_off(rt);
-	}
+	/* Enable LED flash mode and set brightness */
+	ret = rt8515_gpio_brightness_commit(rt, rt->enable_flash, brightness);
+	if (ret)
+		goto out;
+
+	/* Set timeout */
+	schedule_delayed_work(&rt->powerdown_work, usecs_to_jiffies(timeout->val));
 
 	fled->led_cdev.brightness = LED_OFF;
 	/* After this the torch LED will be disabled */
 
+out:
 	mutex_unlock(&rt->lock);
 
-	return 0;
+	return ret;
 }
 
 static int rt8515_led_flash_strobe_get(struct led_classdev_flash *fled,

@@ -144,7 +193,7 @@ static int rt8515_led_flash_strobe_get(struct led_classdev_flash *fled,
 {
 	struct rt8515 *rt = to_rt8515(fled);
 
-	*state = timer_pending(&rt->powerdown_timer);
+	*state = delayed_work_pending(&rt->powerdown_work);
 
 	return 0;
 }

@@ -162,12 +211,17 @@ static const struct led_flash_ops rt8515_flash_ops = {
 	.timeout_set = rt8515_led_flash_timeout_set,
 };
 
-static void rt8515_powerdown_timer(struct timer_list *t)
+static void rt8515_powerdown_work(struct work_struct *work)
 {
-	struct rt8515 *rt = timer_container_of(rt, t, powerdown_timer);
+	struct rt8515 *rt = container_of(work, struct rt8515, powerdown_work.work);
+	int ret;
+
+	mutex_lock(&rt->lock);
+	ret = rt8515_gpio_led_off(rt);
+	mutex_unlock(&rt->lock);
 
-	/* Turn the LED off */
-	rt8515_gpio_led_off(rt);
+	if (ret)
+		dev_err(rt->dev, "failed to turn off LED (%d)\n", ret);
 }
 
 static void rt8515_init_flash_timeout(struct rt8515 *rt)

@@ -297,11 +351,19 @@ static int rt8515_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(rt->enable_flash),
 				     "cannot get ENF (enable flash) GPIO\n");
 
-	/* ENT - Enable Torch line */
-	rt->enable_torch = devm_gpiod_get(dev, "ent", GPIOD_OUT_LOW);
+	/* ENT - Enable Torch line (optional for single-GPIO flash ICs) */
+	rt->enable_torch = devm_gpiod_get_optional(dev, "ent", GPIOD_OUT_LOW);
 	if (IS_ERR(rt->enable_torch))
 		return dev_err_probe(dev, PTR_ERR(rt->enable_torch),
-				     "cannot get ENT (enable torch) GPIO\n");
+				     "Failed to obtain the Enable Torch GPIO\n");
+
+	rt->reg = devm_regulator_get_optional(dev, "vin");
+	if (IS_ERR(rt->reg)) {
+		ret = PTR_ERR(rt->reg);
+		if (ret != -ENODEV)
+			return dev_err_probe(dev, ret, "Failed to obtain the input supply\n");
+		rt->reg = NULL;
+	}
 
 	child = device_get_next_child_node(dev, NULL);
 	if (!child) {

@@ -327,12 +389,17 @@ static int rt8515_probe(struct platform_device *pdev)
 		dev_warn(dev,
 			 "flash-max-timeout-us property missing\n");
 	}
-	timer_setup(&rt->powerdown_timer, rt8515_powerdown_timer, 0);
+	INIT_DELAYED_WORK(&rt->powerdown_work, rt8515_powerdown_work);
 	rt8515_init_flash_timeout(rt);
 
 	fled->ops = &rt8515_flash_ops;
 
-	led->max_brightness = rt->torch_max_intensity;
+	/*
+	 * If there is no separate torch pin, use flash max intensity
+	 * as max brightness instead.
+	 */
+	led->max_brightness = rt->enable_torch ?
+		rt->torch_max_intensity : rt->flash_max_intensity;
 	led->brightness_set_blocking = rt8515_led_brightness_set;
 	led->flags |= LED_CORE_SUSPENDRESUME | LED_DEV_CAP_FLASH;
 

@@ -371,7 +438,7 @@ static void rt8515_remove(struct platform_device *pdev)
 	struct rt8515 *rt = platform_get_drvdata(pdev);
 
 	rt8515_v4l2_flash_release(rt);
-	timer_delete_sync(&rt->powerdown_timer);
+	cancel_delayed_work_sync(&rt->powerdown_work);
 	mutex_destroy(&rt->lock);
 }
Signature · 1 line
2.55.0
author
#9 lore raw

PATCH v7 3/3 ARM: dts: qcom: msm8960: expressatt: Add camera flash

In reply to Rudraksha Gupta

Add camera flash support for the Samsung Galaxy Express (expressatt).

Downstream references:

Link https://github.com/LineageOS/android_kernel_samsung_d2/blob/stable/cm-12.0-YNG4N/drivers/leds/Makefile#L51
Link https://github.com/LineageOS/android_kernel_samsung_d2/blob/stable/cm-12.0-YNG4N/arch/arm/mach-msm/board-apexq-camera.c#L591
Assisted-by Claude:claude-opus-4.6
Reviewed-by David Heidelberg <[email protected]>
Reviewed-by Konrad Dybcio <[email protected]>
Signed-off-by Rudraksha Gupta <[email protected]>
---
 .../dts/qcom/qcom-msm8960-samsung-expressatt.dts   | 44 ++++++++++++++++++++++
 1 file changed, 44 insertions(+)
arch/arm/boot/dts/qcom/qcom-msm8960-samsung-expressatt.dts +44 -0
index 5a3d88e31cf5..b7218ee1349f 100644
@@ -1,11 +1,13 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <dt-bindings/input/input.h>
+#include <dt-bindings/leds/common.h>
 #include <dt-bindings/reset/qcom,gcc-msm8960.h>
 
 #include "qcom-msm8960.dtsi"
 #include "pm8921.dtsi"
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
+#include <dt-bindings/pinctrl/qcom,pmic-mpp.h>
 #include <dt-bindings/input/gpio-keys.h>
 
 / {

@@ -61,6 +63,32 @@ touchkey_enable: touchkey-enable {
 		regulator-boot-on;
 	};
 
+	vreg_flash: regulator-flash {
+		compatible = "regulator-fixed";
+		regulator-name = "VREG_FLASH_3P3";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		gpio = <&pm8921_mpps 4 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+		pinctrl-0 = <&flash_led_unlock>;
+		pinctrl-names = "default";
+	};
+
+	led-controller {
+		compatible = "richtek,rt8515";
+		enf-gpios = <&tlmm 3 GPIO_ACTIVE_HIGH>;
+		vin-supply = <&vreg_flash>;
+		richtek,rfs-ohms = <16000>;
+		pinctrl-0 = <&cam_flash_en>;
+		pinctrl-names = "default";
+
+		led {
+			function = LED_FUNCTION_FLASH;
+			color = <LED_COLOR_ID_WHITE>;
+			flash-max-timeout-us = <250000>;
+		};
+	};
+
 	i2c-gpio-touchkey {
 		compatible = "i2c-gpio";
 		#address-cells = <1>;

@@ -171,6 +199,13 @@ touchscreen@4a {
 };
 
 &tlmm {
+	cam_flash_en: cam-flash-en-state {
+		pins = "gpio3";
+		function = "gpio";
+		drive-strength = <16>;
+		bias-pull-down;
+	};
+
 	spi1_default: spi1-default-state {
 		mosi-pins {
 			pins = "gpio6";

@@ -571,3 +606,12 @@ magnetometer@2e {
 		/* TODO: Figure out Mount Matrix */
 	};
 };
+
+&pm8921_mpps {
+	flash_led_unlock: flash-led-unlock-state {
+		pins = "mpp4";
+		function = "digital";
+		output-low;
+		power-source = <PM8921_MPP_S4>;
+	};
+};
Signature · 1 line
2.55.0
AI review
#2 lore raw

Re: [PATCH v7 1/3] dt-bindings: leds: rt8515: Support single-GPIO flash ICs with vin supply

In reply to Rudraksha Gupta
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The `oneOf` constraint incorrectly makes `ent-gpios` and `vin-supply` mutually exclusive and…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The oneOf constraint incorrectly makes ent-gpios and vin-supply mutually exclusive and strictly requires exactly one to be present, contradicting the claim that vin-supply is optional and artificially restricting valid hardware configurations. --

commit 72463777ee1c18e7913b350d5d2ad857810ece89
Author: Rudraksha Gupta <[email protected]>
Subject: dt-bindings: leds: rt8515: Support single-GPIO flash ICs with vin supply

This commit adds support for single-channel variants of the RT8515 flash ICs. It makes ent-gpios optional and introduces a vin-supply property, adding a oneOf constraint to manage their presence.

Quoted 4 lines
Documentation/devicetree/bindings/leds/richtek,rt8515.yaml +0 -0
index 0356371a6b014..ab3c5139132cc 100644

[ ... ]

Quoted 4 lines
Documentation/devicetree/bindings/leds/richtek,rt8515.yaml +0 -0
@@ -81,10 +90,15 @@ properties:
 
 required:
   - compatible

Documentation/devicetree/bindings/leds/richtek,rt8515.yaml properties: · line 101

Quoted 10 lines
Documentation/devicetree/bindings/leds/richtek,rt8515.yaml +6 -1
-  - ent-gpios
   - enf-gpios
   - led
 
+oneOf:
+  - required:
+      - ent-gpios
+  - required:
+      - vin-supply
+

[Severity: Medium]
Does this oneOf constraint incorrectly restrict valid hardware configurations?

The oneOf constraint strictly requires exactly one of its subschemas to match. If a single-channel IC (which naturally lacks ent-gpios) does not use the explicitly optional vin-supply regulator, zero subschemas will match, causing schema validation to fail.

Conversely, if a standard dual-channel IC (which requires ent-gpios) is also powered by a vin-supply regulator, two subschemas will match. This will also fail validation due to the strict exclusivity of oneOf.

Should this be modeled differently to allow vin-supply to be truly optional while safely permitting single-channel configurations?

Quoted 3 lines
Documentation/devicetree/bindings/leds/richtek,rt8515.yaml +0 -0
 additionalProperties: false
 
 examples:
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
AI review
#10 lore raw

Re: [PATCH v7 3/3] ARM: dts: qcom: msm8960: expressatt: Add camera flash

In reply to Rudraksha Gupta
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 4 lines

From: Rudraksha Gupta <[email protected]>

Add camera flash support for the Samsung Galaxy Express (expressatt).

Quoted 8 lines

Downstream references:

Link https://github.com/LineageOS/android_kernel_samsung_d2/blob/stable/cm-12.0-YNG4N/drivers/leds/Makefile#L51
Link https://github.com/LineageOS/android_kernel_samsung_d2/blob/stable/cm-12.0-YNG4N/arch/arm/mach-msm/board-apexq-camera.c#L591
Assisted-by Claude:claude-opus-4.6
Reviewed-by David Heidelberg <[email protected]>
Reviewed-by Konrad Dybcio <[email protected]>
Signed-off-by Rudraksha Gupta <[email protected]>

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
AI review
#6 lore raw

Re: [PATCH v7 2/3] leds: flash: rt8515: Support single-GPIO flash ICs with vin supply

In reply to Rudraksha Gupta
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Regulator is not enabled when requesting maximum torch brightness, preventing the LED from turning on.
- [High] Replacing mod_timer with schedule_delayed_work silently drops updates to the strobe timeout and causes premature LED power down.
- [Medium] Unbalanced regulator disable due to reliance on the global/physical regulator state instead of the driver's consumer enable count.

Pre-existing issues:
- [Critical] Premature destruction of mutex and cancellation of workqueue during device removal while the LED class device is still registered, leading to a Use-After-Free. --

commit 6a964f82a818f57f5a1a31366f614609bc9c5008
Author: Rudraksha Gupta <[email protected]>

leds: flash: rt8515: Support single-GPIO flash ICs with vin supply

This commit extends the RT8515 driver to support flash ICs that use only a single GPIO for both flash and torch modes, alongside an optional vin regulator that gates power to the flash IC.

Quoted 4 lines
drivers/leds/flash/leds-rt8515.c +0 -0
index 00904cc90ed6e..541f61e13f018 100644

[ ... ]

Quoted 6 lines
drivers/leds/flash/leds-rt8515.c +1 -1
@@ -62,27 +66,50 @@ static struct rt8515 *to_rt8515(struct led_classdev_flash *fled)
 	return container_of(fled, struct rt8515, fled);
 }
 
-static void rt8515_gpio_led_off(struct rt8515 *rt)
+static int rt8515_gpio_led_off(struct rt8515 *rt)

drivers/leds/flash/leds-rt8515.c static struct rt8515 *to_rt8515(struct led_classdev_flash *fled) · line 77

Quoted 10 lines
drivers/leds/flash/leds-rt8515.c +7 -2
 {
-	gpiod_set_value(rt->enable_flash, 0);
-	gpiod_set_value(rt->enable_torch, 0);
+	gpiod_set_value(rt->enable_flash, RT8515_OFF);
+	gpiod_set_value(rt->enable_torch, RT8515_OFF);
+
+	if (!rt->reg)
+		return 0;
+
+	return regulator_is_enabled(rt->reg) > 0 ? regulator_disable(rt->reg) : 0;

[Severity: Medium]
Since regulator_is_enabled() returns the global physical state rather than the per-consumer enable count, could this lead to an unbalanced disable if the regulator is marked always-on or shared with another consumer?

Because rt8515_gpio_led_off() is reachable from user-controllable sysfs attributes via rt8515_led_brightness_set(), repeated calls when the driver has not enabled the regulator might cause the regulator core to throw a warning and return -EIO. Should the driver maintain its own boolean state to track whether it has enabled the regulator?

Quoted 1 line
drivers/leds/flash/leds-rt8515.c +0 -0
 }

[ ... ]

Quoted 25 lines
drivers/leds/flash/leds-rt8515.c +12 -4
@@ -91,23 +118,37 @@ static int rt8515_led_brightness_set(struct led_classdev *led,
 {
 	struct led_classdev_flash *fled = lcdev_to_flcdev(led);
 	struct rt8515 *rt = to_rt8515(fled);
+	int ret = 0;
 
 	mutex_lock(&rt->lock);
 
 	if (brightness == LED_OFF) {
-		/* Off */
-		rt8515_gpio_led_off(rt);
+		ret = rt8515_gpio_led_off(rt);
+		if (ret)
+			goto out;
 	} else if (brightness < RT8515_TORCH_MAX) {
-		/* Step it up to movie mode brightness using the flash pin */
-		rt8515_gpio_brightness_commit(rt->enable_torch, brightness);
+		/*
+		 * Step it up to movie mode brightness.
+		 * If there is no separate torch pin, use the flash pin
+		 * for torch as well.
+		 */
+		ret = rt8515_gpio_brightness_commit(rt,
+				rt->enable_torch ?: rt->enable_flash, brightness);
+		if (ret)

drivers/leds/flash/leds-rt8515.c static int rt8515_led_brightness_set(struct led_classdev *led, · line 145

Quoted 10 lines
drivers/leds/flash/leds-rt8515.c +7 -2
+			goto out;
 	} else {
-		/* Max torch brightness requested */
-		gpiod_set_value(rt->enable_torch, 1);
+		/*
+		 * Max torch brightness requested.
+		 * If there is no separate torch pin, use the flash pin
+		 * for torch as well.
+		 */
+		gpiod_set_value(rt->enable_torch ?: rt->enable_flash, RT8515_ON);

[Severity: High]
If a transition is made from LED_OFF directly to max torch brightness, the optional vin regulator remains disabled because rt8515_gpio_brightness_commit() is completely bypassed here. Does this mean the max torch state might silently fail to power the IC?

Quoted 11 lines
drivers/leds/flash/leds-rt8515.c +2 -1
 	}
 
+out:
 	mutex_unlock(&rt->lock);
 
-	return 0;
+	return ret;
 }
 
 static int rt8515_led_flash_strobe_set(struct led_classdev_flash *fled,

@@ -116,27 +157,35 @@ static int rt8515_led_flash_strobe_set(struct led_classdev_flash *fled,

[ ... ]

Quoted 8 lines
drivers/leds/flash/leds-rt8515.c +0 -8
-	if (state) {
-		/* Enable LED flash mode and set brightness */
-		rt8515_gpio_brightness_commit(rt->enable_flash, brightness);
-		/* Set timeout */
-		mod_timer(&rt->powerdown_timer,
-			  jiffies + usecs_to_jiffies(timeout->val));
-	} else {
-		timer_delete_sync(&rt->powerdown_timer);

drivers/leds/flash/leds-rt8515.c static int rt8515_led_flash_strobe_set(struct led_classdev_flash *fled, · line 180

Quoted 10 lines
drivers/leds/flash/leds-rt8515.c +7 -3
-		/* Turn the LED off */
-		rt8515_gpio_led_off(rt);
-	}
+	/* Enable LED flash mode and set brightness */
+	ret = rt8515_gpio_brightness_commit(rt, rt->enable_flash, brightness);
+	if (ret)
+		goto out;
+
+	/* Set timeout */
+	schedule_delayed_work(&rt->powerdown_work, usecs_to_jiffies(timeout->val));

[Severity: High]
Does schedule_delayed_work() correctly handle consecutive flash strobe commands? Unlike mod_timer(), schedule_delayed_work() silently ignores updates if the work is already pending. This could cause consecutive V4L2 flash strobes to fail to extend the timeout, shutting down the flash prematurely.

Additionally, if the delayed work is actively executing and blocked waiting on rt->lock, its pending bit is cleared. In that scenario, could schedule_delayed_work() queue a duplicate instance of the work that immediately shuts down the LED as soon as the lock is released? Would using mod_delayed_work() resolve these state management issues?

Quoted 10 lines
drivers/leds/flash/leds-rt8515.c +2 -1
 	fled->led_cdev.brightness = LED_OFF;
 	/* After this the torch LED will be disabled */
 
+out:
 	mutex_unlock(&rt->lock);
 
-	return 0;
+	return ret;
 }

[ ... ]

Quoted 1 line
drivers/leds/flash/leds-rt8515.c +0 -0
@@ -327,12 +389,17 @@ static int rt8515_probe(struct platform_device *pdev)

[ ... ]

drivers/leds/flash/leds-rt8515.c

Quoted 3 lines
drivers/leds/flash/leds-rt8515.c +0 -0
 	platform_set_drvdata(pdev, rt);
 
 	ret = devm_led_classdev_flash_register_ext(dev, fled, &init_data);

[Severity: Critical]
This is a pre-existing issue, but registering the LED class device via the devm API schedules its sysfs interfaces to be unregistered after rt8515_remove() returns.

[ ... ]

drivers/leds/flash/leds-rt8515.c static void rt8515_remove(struct platform_device *pdev) · line 442

Quoted 7 lines
drivers/leds/flash/leds-rt8515.c +1 -1
@@ -371,7 +438,7 @@ static void rt8515_remove(struct platform_device *pdev)
 	struct rt8515 *rt = platform_get_drvdata(pdev);
 
 	rt8515_v4l2_flash_release(rt);
-	timer_delete_sync(&rt->powerdown_timer);
+	cancel_delayed_work_sync(&rt->powerdown_work);
 	mutex_destroy(&rt->lock);

Because devres destroys the sysfs interfaces after this function returns, could a concurrent user-space write to sysfs (like triggering a flash strobe) occur during this window? This would cause the sysfs handler to attempt to lock the destroyed mutex and queue work on the freed device structure, leading to a use-after-free. Should devm_add_action_or_reset() be used here instead to ensure the work and mutex are only destroyed after the devm-managed LED registration is cleanly torn down?

Quoted 1 line
drivers/leds/flash/leds-rt8515.c +0 -0
 }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
maintainer
#7 lore raw

Re: [PATCH v7 2/3] leds: flash: rt8515: Support single-GPIO flash ICs with vin supply

Quoted 6 lines · On Sun, 13 Sep 2026, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Regulator is not enabled when requesting maximum torch brightness, preventing the LED from turning on.
- [High] Replacing mod_timer with schedule_delayed_work silently drops updates to the strobe timeout and causes premature LED power down.
- [Medium] Unbalanced regulator disable due to reliance on the global/physical regulator state instead of the driver's consumer enable count.

Please review, explain or fix the Sashiko review(s).

If this is a set, this request includes the other patches too.

Quoted 177 lines

Pre-existing issues:
- [Critical] Premature destruction of mutex and cancellation of workqueue during device removal while the LED class device is still registered, leading to a Use-After-Free. --

commit 6a964f82a818f57f5a1a31366f614609bc9c5008
Author: Rudraksha Gupta <[email protected]>

leds: flash: rt8515: Support single-GPIO flash ICs with vin supply

This commit extends the RT8515 driver to support flash ICs that use only a single GPIO for both flash and torch modes, alongside an optional vin regulator that gates power to the flash IC.

Quoted 4 lines
drivers/leds/flash/leds-rt8515.c +0 -0
index 00904cc90ed6e..541f61e13f018 100644

[ ... ]

Quoted 6 lines
drivers/leds/flash/leds-rt8515.c +1 -1
@@ -62,27 +66,50 @@ static struct rt8515 *to_rt8515(struct led_classdev_flash *fled)
 	return container_of(fled, struct rt8515, fled);
 }
 
-static void rt8515_gpio_led_off(struct rt8515 *rt)
+static int rt8515_gpio_led_off(struct rt8515 *rt)

drivers/leds/flash/leds-rt8515.c static struct rt8515 *to_rt8515(struct led_classdev_flash *fled) · line 77

Quoted 10 lines
drivers/leds/flash/leds-rt8515.c +7 -2
 {
-	gpiod_set_value(rt->enable_flash, 0);
-	gpiod_set_value(rt->enable_torch, 0);
+	gpiod_set_value(rt->enable_flash, RT8515_OFF);
+	gpiod_set_value(rt->enable_torch, RT8515_OFF);
+
+	if (!rt->reg)
+		return 0;
+
+	return regulator_is_enabled(rt->reg) > 0 ? regulator_disable(rt->reg) : 0;

[Severity: Medium]
Since regulator_is_enabled() returns the global physical state rather than the per-consumer enable count, could this lead to an unbalanced disable if the regulator is marked always-on or shared with another consumer?

Because rt8515_gpio_led_off() is reachable from user-controllable sysfs attributes via rt8515_led_brightness_set(), repeated calls when the driver has not enabled the regulator might cause the regulator core to throw a warning and return -EIO. Should the driver maintain its own boolean state to track whether it has enabled the regulator?

Quoted 1 line
drivers/leds/flash/leds-rt8515.c +0 -0
 }

[ ... ]

Quoted 25 lines
drivers/leds/flash/leds-rt8515.c +12 -4
@@ -91,23 +118,37 @@ static int rt8515_led_brightness_set(struct led_classdev *led,
 {
 	struct led_classdev_flash *fled = lcdev_to_flcdev(led);
 	struct rt8515 *rt = to_rt8515(fled);
+	int ret = 0;
 
 	mutex_lock(&rt->lock);
 
 	if (brightness == LED_OFF) {
-		/* Off */
-		rt8515_gpio_led_off(rt);
+		ret = rt8515_gpio_led_off(rt);
+		if (ret)
+			goto out;
 	} else if (brightness < RT8515_TORCH_MAX) {
-		/* Step it up to movie mode brightness using the flash pin */
-		rt8515_gpio_brightness_commit(rt->enable_torch, brightness);
+		/*
+		 * Step it up to movie mode brightness.
+		 * If there is no separate torch pin, use the flash pin
+		 * for torch as well.
+		 */
+		ret = rt8515_gpio_brightness_commit(rt,
+				rt->enable_torch ?: rt->enable_flash, brightness);
+		if (ret)

drivers/leds/flash/leds-rt8515.c static int rt8515_led_brightness_set(struct led_classdev *led, · line 145

Quoted 10 lines
drivers/leds/flash/leds-rt8515.c +7 -2
+			goto out;
 	} else {
-		/* Max torch brightness requested */
-		gpiod_set_value(rt->enable_torch, 1);
+		/*
+		 * Max torch brightness requested.
+		 * If there is no separate torch pin, use the flash pin
+		 * for torch as well.
+		 */
+		gpiod_set_value(rt->enable_torch ?: rt->enable_flash, RT8515_ON);

[Severity: High]
If a transition is made from LED_OFF directly to max torch brightness, the optional vin regulator remains disabled because rt8515_gpio_brightness_commit() is completely bypassed here. Does this mean the max torch state might silently fail to power the IC?

Quoted 11 lines
drivers/leds/flash/leds-rt8515.c +2 -1
 	}
 
+out:
 	mutex_unlock(&rt->lock);
 
-	return 0;
+	return ret;
 }
 
 static int rt8515_led_flash_strobe_set(struct led_classdev_flash *fled,

@@ -116,27 +157,35 @@ static int rt8515_led_flash_strobe_set(struct led_classdev_flash *fled,

[ ... ]

Quoted 8 lines
drivers/leds/flash/leds-rt8515.c +0 -8
-	if (state) {
-		/* Enable LED flash mode and set brightness */
-		rt8515_gpio_brightness_commit(rt->enable_flash, brightness);
-		/* Set timeout */
-		mod_timer(&rt->powerdown_timer,
-			  jiffies + usecs_to_jiffies(timeout->val));
-	} else {
-		timer_delete_sync(&rt->powerdown_timer);

drivers/leds/flash/leds-rt8515.c static int rt8515_led_flash_strobe_set(struct led_classdev_flash *fled, · line 180

Quoted 10 lines
drivers/leds/flash/leds-rt8515.c +7 -3
-		/* Turn the LED off */
-		rt8515_gpio_led_off(rt);
-	}
+	/* Enable LED flash mode and set brightness */
+	ret = rt8515_gpio_brightness_commit(rt, rt->enable_flash, brightness);
+	if (ret)
+		goto out;
+
+	/* Set timeout */
+	schedule_delayed_work(&rt->powerdown_work, usecs_to_jiffies(timeout->val));

[Severity: High]
Does schedule_delayed_work() correctly handle consecutive flash strobe commands? Unlike mod_timer(), schedule_delayed_work() silently ignores updates if the work is already pending. This could cause consecutive V4L2 flash strobes to fail to extend the timeout, shutting down the flash prematurely.

Additionally, if the delayed work is actively executing and blocked waiting on rt->lock, its pending bit is cleared. In that scenario, could schedule_delayed_work() queue a duplicate instance of the work that immediately shuts down the LED as soon as the lock is released? Would using mod_delayed_work() resolve these state management issues?

Quoted 10 lines
drivers/leds/flash/leds-rt8515.c +2 -1
 	fled->led_cdev.brightness = LED_OFF;
 	/* After this the torch LED will be disabled */
 
+out:
 	mutex_unlock(&rt->lock);
 
-	return 0;
+	return ret;
 }

[ ... ]

Quoted 1 line
drivers/leds/flash/leds-rt8515.c +0 -0
@@ -327,12 +389,17 @@ static int rt8515_probe(struct platform_device *pdev)

[ ... ]

drivers/leds/flash/leds-rt8515.c

Quoted 3 lines
drivers/leds/flash/leds-rt8515.c +0 -0
 	platform_set_drvdata(pdev, rt);
 
 	ret = devm_led_classdev_flash_register_ext(dev, fled, &init_data);

[Severity: Critical]
This is a pre-existing issue, but registering the LED class device via the devm API schedules its sysfs interfaces to be unregistered after rt8515_remove() returns.

[ ... ]

drivers/leds/flash/leds-rt8515.c static void rt8515_remove(struct platform_device *pdev) · line 442

Quoted 7 lines
drivers/leds/flash/leds-rt8515.c +1 -1
@@ -371,7 +438,7 @@ static void rt8515_remove(struct platform_device *pdev)
 	struct rt8515 *rt = platform_get_drvdata(pdev);
 
 	rt8515_v4l2_flash_release(rt);
-	timer_delete_sync(&rt->powerdown_timer);
+	cancel_delayed_work_sync(&rt->powerdown_work);
 	mutex_destroy(&rt->lock);

Because devres destroys the sysfs interfaces after this function returns, could a concurrent user-space write to sysfs (like triggering a flash strobe) occur during this window? This would cause the sysfs handler to attempt to lock the destroyed mutex and queue work on the freed device structure, leading to a use-after-free. Should devm_add_action_or_reset() be used here instead to ensure the work and mutex are only destroyed after the devm-managed LED registration is cleanly torn down?

Quoted 1 line
drivers/leds/flash/leds-rt8515.c +0 -0
 }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Signature · 1 line
Lee Jones
author
#3 lore raw

Re: [PATCH v7 1/3] dt-bindings: leds: rt8515: Support single-GPIO flash ICs with vin supply

In reply to [email protected]
Quoted 3 lines · On 9/12/26 19:45, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The oneOf constraint incorrectly makes ent-gpios and vin-supply mutually exclusive and strictly requires exactly one to be present, contradicting the claim that vin-supply is optional and artificially restricting valid hardware configurations. --

make ARCH=arm CHECK_DTBS=y DT_SCHEMA_FILES=richtek,rt8515.yaml \
      qcom/qcom-msm8960-samsung-expressatt.dtb \
      st/ste-ux500-samsung-codina.dtb \
      st/ste-ux500-samsung-codina-tmo.dtb \
      st/ste-ux500-samsung-gavini.dtb \
      st/ste-ux500-samsung-golden.dtb \
      st/ste-ux500-samsung-janice.dtb \
      st/ste-ux500-samsung-kyle.dtb \
      st/ste-ux500-samsung-skomer.dtb

all 8 in-tree users of this binding. 0 warnings.

existing hw passes. Probably not useful to account for hw not in the tree

Quoted 47 lines

commit 72463777ee1c18e7913b350d5d2ad857810ece89
Author: Rudraksha Gupta <[email protected]>
Subject: dt-bindings: leds: rt8515: Support single-GPIO flash ICs with vin supply

This commit adds support for single-channel variants of the RT8515 flash ICs. It makes ent-gpios optional and introduces a vin-supply property, adding a oneOf constraint to manage their presence.

Quoted 4 lines
Documentation/devicetree/bindings/leds/richtek,rt8515.yaml +0 -0
index 0356371a6b014..ab3c5139132cc 100644

[ ... ]

Quoted 4 lines
Documentation/devicetree/bindings/leds/richtek,rt8515.yaml +0 -0
@@ -81,10 +90,15 @@ properties:
 
  required:
    - compatible

Documentation/devicetree/bindings/leds/richtek,rt8515.yaml properties: · line 101

Quoted 10 lines
Documentation/devicetree/bindings/leds/richtek,rt8515.yaml +6 -1
-  - ent-gpios
    - enf-gpios
    - led
 
+oneOf:
+  - required:
+      - ent-gpios
+  - required:
+      - vin-supply
+

[Severity: Medium]
Does this oneOf constraint incorrectly restrict valid hardware configurations?

The oneOf constraint strictly requires exactly one of its subschemas to match. If a single-channel IC (which naturally lacks ent-gpios) does not use the explicitly optional vin-supply regulator, zero subschemas will match, causing schema validation to fail.

Conversely, if a standard dual-channel IC (which requires ent-gpios) is also powered by a vin-supply regulator, two subschemas will match. This will also fail validation due to the strict exclusivity of oneOf.

Should this be modeled differently to allow vin-supply to be truly optional while safely permitting single-channel configurations?

Quoted 3 lines
Documentation/devicetree/bindings/leds/richtek,rt8515.yaml +0 -0
  additionalProperties: false
 
  examples:
author
#8 lore raw

Re: [PATCH v7 2/3] leds: flash: rt8515: Support single-GPIO flash ICs with vin supply

In reply to [email protected]
Quoted 10 lines · On 9/12/26 19:50, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Regulator is not enabled when requesting maximum torch brightness, preventing the LED from turning on.
- [High] Replacing mod_timer with schedule_delayed_work silently drops updates to the strobe timeout and causes premature LED power down.
- [Medium] Unbalanced regulator disable due to reliance on the global/physical regulator state instead of the driver's consumer enable count.

Pre-existing issues:
- [Critical] Premature destruction of mutex and cancellation of workqueue during device removal while the LED class device is still registered, leading to a Use-After-Free. --

1. Max torch regulator [High] - not reachable. The else branch needs brightness >= RT8515_TORCH_MAX (100). expressatt max_brightness is 16, ux500 boards reach it but have no vin-supply so rt->reg is NULL.

2. schedule_delayed_work [High] - shouldn't matter. max timeout is 250ms on every in-tree board. Imperceptible. Happy to change it though

3. Unbalanced disable [Medium] - was in v2, dropped in v3 at Linus's request. happy to reinstate.

4. remove() UAF [Critical] - pre-existing. not sure if i should address in this series.

Quoted 166 lines

commit 6a964f82a818f57f5a1a31366f614609bc9c5008
Author: Rudraksha Gupta <[email protected]>

leds: flash: rt8515: Support single-GPIO flash ICs with vin supply

This commit extends the RT8515 driver to support flash ICs that use only a single GPIO for both flash and torch modes, alongside an optional vin regulator that gates power to the flash IC.

Quoted 4 lines
drivers/leds/flash/leds-rt8515.c +0 -0
index 00904cc90ed6e..541f61e13f018 100644

[ ... ]

Quoted 6 lines
drivers/leds/flash/leds-rt8515.c +1 -1
@@ -62,27 +66,50 @@ static struct rt8515 *to_rt8515(struct led_classdev_flash *fled)
  	return container_of(fled, struct rt8515, fled);
  }
 
-static void rt8515_gpio_led_off(struct rt8515 *rt)
+static int rt8515_gpio_led_off(struct rt8515 *rt)

drivers/leds/flash/leds-rt8515.c static struct rt8515 *to_rt8515(struct led_classdev_flash *fled) · line 77

Quoted 10 lines
drivers/leds/flash/leds-rt8515.c +7 -2
  {
-	gpiod_set_value(rt->enable_flash, 0);
-	gpiod_set_value(rt->enable_torch, 0);
+	gpiod_set_value(rt->enable_flash, RT8515_OFF);
+	gpiod_set_value(rt->enable_torch, RT8515_OFF);
+
+	if (!rt->reg)
+		return 0;
+
+	return regulator_is_enabled(rt->reg) > 0 ? regulator_disable(rt->reg) : 0;

[Severity: Medium]
Since regulator_is_enabled() returns the global physical state rather than the per-consumer enable count, could this lead to an unbalanced disable if the regulator is marked always-on or shared with another consumer?

Because rt8515_gpio_led_off() is reachable from user-controllable sysfs attributes via rt8515_led_brightness_set(), repeated calls when the driver has not enabled the regulator might cause the regulator core to throw a warning and return -EIO. Should the driver maintain its own boolean state to track whether it has enabled the regulator?

Quoted 1 line
drivers/leds/flash/leds-rt8515.c +0 -0
  }

[ ... ]

Quoted 25 lines
drivers/leds/flash/leds-rt8515.c +12 -4
@@ -91,23 +118,37 @@ static int rt8515_led_brightness_set(struct led_classdev *led,
  {
  	struct led_classdev_flash *fled = lcdev_to_flcdev(led);
  	struct rt8515 *rt = to_rt8515(fled);
+	int ret = 0;
 
  	mutex_lock(&rt->lock);
 
  	if (brightness == LED_OFF) {
-		/* Off */
-		rt8515_gpio_led_off(rt);
+		ret = rt8515_gpio_led_off(rt);
+		if (ret)
+			goto out;
  	} else if (brightness < RT8515_TORCH_MAX) {
-		/* Step it up to movie mode brightness using the flash pin */
-		rt8515_gpio_brightness_commit(rt->enable_torch, brightness);
+		/*
+		 * Step it up to movie mode brightness.
+		 * If there is no separate torch pin, use the flash pin
+		 * for torch as well.
+		 */
+		ret = rt8515_gpio_brightness_commit(rt,
+				rt->enable_torch ?: rt->enable_flash, brightness);
+		if (ret)

drivers/leds/flash/leds-rt8515.c static int rt8515_led_brightness_set(struct led_classdev *led, · line 145

Quoted 10 lines
drivers/leds/flash/leds-rt8515.c +7 -2
+			goto out;
  	} else {
-		/* Max torch brightness requested */
-		gpiod_set_value(rt->enable_torch, 1);
+		/*
+		 * Max torch brightness requested.
+		 * If there is no separate torch pin, use the flash pin
+		 * for torch as well.
+		 */
+		gpiod_set_value(rt->enable_torch ?: rt->enable_flash, RT8515_ON);

[Severity: High]
If a transition is made from LED_OFF directly to max torch brightness, the optional vin regulator remains disabled because rt8515_gpio_brightness_commit() is completely bypassed here. Does this mean the max torch state might silently fail to power the IC?

Quoted 11 lines
drivers/leds/flash/leds-rt8515.c +2 -1
  	}
 
+out:
  	mutex_unlock(&rt->lock);
 
-	return 0;
+	return ret;
  }
 
  static int rt8515_led_flash_strobe_set(struct led_classdev_flash *fled,

@@ -116,27 +157,35 @@ static int rt8515_led_flash_strobe_set(struct led_classdev_flash *fled,

[ ... ]

Quoted 8 lines
drivers/leds/flash/leds-rt8515.c +0 -8
-	if (state) {
-		/* Enable LED flash mode and set brightness */
-		rt8515_gpio_brightness_commit(rt->enable_flash, brightness);
-		/* Set timeout */
-		mod_timer(&rt->powerdown_timer,
-			  jiffies + usecs_to_jiffies(timeout->val));
-	} else {
-		timer_delete_sync(&rt->powerdown_timer);

drivers/leds/flash/leds-rt8515.c static int rt8515_led_flash_strobe_set(struct led_classdev_flash *fled, · line 180

Quoted 10 lines
drivers/leds/flash/leds-rt8515.c +7 -3
-		/* Turn the LED off */
-		rt8515_gpio_led_off(rt);
-	}
+	/* Enable LED flash mode and set brightness */
+	ret = rt8515_gpio_brightness_commit(rt, rt->enable_flash, brightness);
+	if (ret)
+		goto out;
+
+	/* Set timeout */
+	schedule_delayed_work(&rt->powerdown_work, usecs_to_jiffies(timeout->val));

[Severity: High]
Does schedule_delayed_work() correctly handle consecutive flash strobe commands? Unlike mod_timer(), schedule_delayed_work() silently ignores updates if the work is already pending. This could cause consecutive V4L2 flash strobes to fail to extend the timeout, shutting down the flash prematurely.

Additionally, if the delayed work is actively executing and blocked waiting on rt->lock, its pending bit is cleared. In that scenario, could schedule_delayed_work() queue a duplicate instance of the work that immediately shuts down the LED as soon as the lock is released? Would using mod_delayed_work() resolve these state management issues?

Quoted 10 lines

fled->led_cdev.brightness = LED_OFF; /* After this the torch LED will be disabled */

drivers/leds/flash/leds-rt8515.c +2 -1
+out:
  	mutex_unlock(&rt->lock);
 
-	return 0;
+	return ret;
  }

[ ... ]

Quoted 1 line
drivers/leds/flash/leds-rt8515.c +0 -0
@@ -327,12 +389,17 @@ static int rt8515_probe(struct platform_device *pdev)

[ ... ]

Quoted 3 lines
  	platform_set_drvdata(pdev, rt);
  
  	ret = devm_led_classdev_flash_register_ext(dev, fled, &init_data);

[Severity: Critical]
This is a pre-existing issue, but registering the LED class device via the devm API schedules its sysfs interfaces to be unregistered after rt8515_remove() returns.

[ ... ]

drivers/leds/flash/leds-rt8515.c static void rt8515_remove(struct platform_device *pdev) · line 442

Quoted 7 lines
drivers/leds/flash/leds-rt8515.c +1 -1
@@ -371,7 +438,7 @@ static void rt8515_remove(struct platform_device *pdev)
  	struct rt8515 *rt = platform_get_drvdata(pdev);
 
  	rt8515_v4l2_flash_release(rt);
-	timer_delete_sync(&rt->powerdown_timer);
+	cancel_delayed_work_sync(&rt->powerdown_work);
  	mutex_destroy(&rt->lock);

Because devres destroys the sysfs interfaces after this function returns, could a concurrent user-space write to sysfs (like triggering a flash strobe) occur during this window? This would cause the sysfs handler to attempt to lock the destroyed mutex and queue work on the freed device structure, leading to a use-after-free. Should devm_add_action_or_reset() be used here instead to ensure the work and mutex are only destroyed after the devm-managed LED registration is cleanly torn down?

Quoted 1 line
drivers/leds/flash/leds-rt8515.c +0 -0
  }
maintainer
#4 lore raw

Re: [PATCH v7 1/3] dt-bindings: leds: rt8515: Support single-GPIO flash ICs with vin supply

In reply to Rudraksha Gupta
Quoted 15 lines · Rudraksha Gupta
Quoted 3 lines · On 9/12/26 19:45, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The oneOf constraint incorrectly makes ent-gpios and vin-supply mutually exclusive and strictly requires exactly one to be present, contradicting the claim that vin-supply is optional and artificially restricting valid hardware configurations. --

make ARCH=arm CHECK_DTBS=y DT_SCHEMA_FILES=richtek,rt8515.yaml \
      qcom/qcom-msm8960-samsung-expressatt.dtb \
      st/ste-ux500-samsung-codina.dtb \
      st/ste-ux500-samsung-codina-tmo.dtb \
      st/ste-ux500-samsung-gavini.dtb \
      st/ste-ux500-samsung-golden.dtb \
      st/ste-ux500-samsung-janice.dtb \
      st/ste-ux500-samsung-kyle.dtb \
      st/ste-ux500-samsung-skomer.dtb

Quoted 3 lines

all 8 in-tree users of this binding. 0 warnings.

existing hw passes.

So?

Quoted 1 line

Probably not useful to account for hw not in the tree

Oh, yes it is.

Quoted 29 lines
Quoted 29 lines

commit 72463777ee1c18e7913b350d5d2ad857810ece89
Author: Rudraksha Gupta <[email protected]>
Subject: dt-bindings: leds: rt8515: Support single-GPIO flash ICs with vin supply

This commit adds support for single-channel variants of the RT8515 flash ICs. It makes ent-gpios optional and introduces a vin-supply property, adding a oneOf constraint to manage their presence.

Quoted 4 lines
Documentation/devicetree/bindings/leds/richtek,rt8515.yaml +0 -0
index 0356371a6b014..ab3c5139132cc 100644

[ ... ]

Documentation/devicetree/bindings/leds/richtek,rt8515.yaml properties: · line 99

Quoted 12 lines
Documentation/devicetree/bindings/leds/richtek,rt8515.yaml +6 -1
@@ -81,10 +90,15 @@ properties:
  required:
    - compatible
-  - ent-gpios
    - enf-gpios
    - led
+oneOf:
+  - required:
+      - ent-gpios
+  - required:
+      - vin-supply
+

[Severity: Medium]
Does this oneOf constraint incorrectly restrict valid hardware configurations?

Quoted 8 lines
Quoted 8 lines

The oneOf constraint strictly requires exactly one of its subschemas to match. If a single-channel IC (which naturally lacks ent-gpios) does not use the explicitly optional vin-supply regulator, zero subschemas will match, causing schema validation to fail.

Conversely, if a standard dual-channel IC (which requires ent-gpios) is also powered by a vin-supply regulator, two subschemas will match. This will also fail validation due to the strict exclusivity of oneOf.

I agree. It makes little sense that you can only have either a supply or a GPIO. If that does make sense, the commit message better explain why.

Rob