[PATCH v2 0/3] Add sy7802 flash led driver

[PATCH v2 0/3] Add sy7802 flash led driver

[André Apitzsch via B4 Relay]

This series introduces a driver for the Silergy SY7802 charge pump used
in the BQ Aquaris M5 and X5 smartphones.

The implementation is based on information extracted from downstream as
the datasheet provided by a distributor of the hardware didn't include
any information about the i2c register description.

Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
Changes in v2:
- bindings: remove unneeded allOf
- bindings: example: move flash-led-controller under i2c node to fix
  check error
- Cc to phone-devel
- Link to v1: https://lore.kernel.org/r/20240327-sy7802-v1-0-db74ab32faaf@apitzsch.eu

---
André Apitzsch (3):
      dt-bindings: leds: Add Silergy SY7802 flash LED
      leds: sy7802: Add support for Silergy SY7802 flash LED controller
      arm64: dts: qcom: msm8939-longcheer-l9100: Add rear flash

 .../devicetree/bindings/leds/silergy,sy7802.yaml   | 100 ++++
 .../boot/dts/qcom/msm8939-longcheer-l9100.dts      |  26 +
 drivers/leds/flash/Kconfig                         |  11 +
 drivers/leds/flash/Makefile                        |   1 +
 drivers/leds/flash/leds-sy7802.c                   | 532 +++++++++++++++++++++
 5 files changed, 670 insertions(+)
---
base-commit: a6bd6c9333397f5a0e2667d4d82fef8c970108f2
change-id: 20240325-sy7802-f40fc6f56525

Best regards,
-- 
André Apitzsch <git@apitzsch.eu>

[PATCH v2 1/3] dt-bindings: leds: Add Silergy SY7802 flash LED

[André Apitzsch via B4 Relay]

From: André Apitzsch <git@apitzsch.eu>

Document Silergy SY7802 flash LED driver devicetree bindings.

Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
 .../devicetree/bindings/leds/silergy,sy7802.yaml   | 100 +++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/silergy,sy7802.yaml b/Documentation/devicetree/bindings/leds/silergy,sy7802.yaml
new file mode 100644
index 000000000000..46b8e5452b62
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/silergy,sy7802.yaml
@@ -0,0 +1,100 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/silergy,sy7802.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Silergy SY7802 1800mA Boost Charge Pump LED Driver
+
+maintainers:
+  - André Apitzsch <git@apitzsch.eu>
+
+description: |
+  The SY7802 is a current-regulated charge pump which can regulate two current
+  levels for Flash and Torch modes.
+
+  The SY7802 is a high-current synchronous boost converter with 2-channel
+  high side current sources. Each channel is able to deliver 900mA current.
+
+properties:
+  compatible:
+    enum:
+      - silergy,sy7802
+
+  reg:
+    maxItems: 1
+
+  enable-gpios:
+    maxItems: 1
+    description: A connection to the 'EN' pin.
+
+  flash-gpios:
+    maxItems: 1
+    description: A connection to the 'FLEN' pin.
+
+  vin-supply:
+    description: Regulator providing power to the 'VIN' pin.
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^led@[0-1]$":
+    type: object
+    $ref: common.yaml#
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        description: Index of the LED.
+        minimum: 0
+        maximum: 1
+
+      led-sources:
+        minItems: 1
+        maxItems: 2
+        items:
+          minimum: 0
+          maximum: 1
+
+    required:
+      - reg
+      - led-sources
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+  - enable-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/leds/common.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        flash-led-controller@53 {
+            compatible = "silergy,sy7802";
+            reg = <0x53>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            enable-gpios = <&tlmm 16 GPIO_ACTIVE_HIGH>;
+
+            led@0 {
+                reg = <0>;
+                function = LED_FUNCTION_FLASH;
+                color = <LED_COLOR_ID_WHITE>;
+                led-sources = <0>, <1>;
+            };
+        };
+    };

-- 
2.44.0

Re: [PATCH v2 1/3] dt-bindings: leds: Add Silergy SY7802 flash LED

[Rob Herring]

On Mon, 01 Apr 2024 23:23:55 +0200, André Apitzsch wrote:
> Document Silergy SY7802 flash LED driver devicetree bindings.
> 
> Signed-off-by: André Apitzsch <git@apitzsch.eu>
> ---
>  .../devicetree/bindings/leds/silergy,sy7802.yaml   | 100 +++++++++++++++++++++
>  1 file changed, 100 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v2 2/3] leds: sy7802: Add support for Silergy SY7802 flash LED controller

[André Apitzsch via B4 Relay]

From: André Apitzsch <git@apitzsch.eu>

Add support for SY7802 flash LED controller. It can support up to 1.8A
flash current.

Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
 drivers/leds/flash/Kconfig       |  11 +
 drivers/leds/flash/Makefile      |   1 +
 drivers/leds/flash/leds-sy7802.c | 532 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 544 insertions(+)

diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
index 809b6d98bb3e..f39f0bfe6eef 100644
--- a/drivers/leds/flash/Kconfig
+++ b/drivers/leds/flash/Kconfig
@@ -121,4 +121,15 @@ config LEDS_SGM3140
 	  This option enables support for the SGM3140 500mA Buck/Boost Charge
 	  Pump LED Driver.
 
+config LEDS_SY7802
+	tristate "LED support for the Silergy SY7802"
+	depends on I2C && OF
+	depends on GPIOLIB
+	select REGMAP_I2C
+	help
+	  This option enables support for the SY7802 flash LED controller.
+	  SY7802 includes torch and flash functions with programmable current.
+
+	  This driver can be built as a module, it will be called "leds-sy7802".
+
 endif # LEDS_CLASS_FLASH
diff --git a/drivers/leds/flash/Makefile b/drivers/leds/flash/Makefile
index 91d60a4b7952..48860eeced79 100644
--- a/drivers/leds/flash/Makefile
+++ b/drivers/leds/flash/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_LEDS_QCOM_FLASH)	+= leds-qcom-flash.o
 obj-$(CONFIG_LEDS_RT4505)	+= leds-rt4505.o
 obj-$(CONFIG_LEDS_RT8515)	+= leds-rt8515.o
 obj-$(CONFIG_LEDS_SGM3140)	+= leds-sgm3140.o
+obj-$(CONFIG_LEDS_SY7802)	+= leds-sy7802.o
diff --git a/drivers/leds/flash/leds-sy7802.c b/drivers/leds/flash/leds-sy7802.c
new file mode 100644
index 000000000000..c03a571b0e08
--- /dev/null
+++ b/drivers/leds/flash/leds-sy7802.c
@@ -0,0 +1,532 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Silergy SY7802 flash LED driver with I2C interface
+ *
+ * Copyright 2024 André Apitzsch <git@apitzsch.eu>
+ */
+
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/led-class-flash.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+#define SY7802_MAX_LEDS 2
+#define SY7802_LED_JOINT 2
+
+#define SY7802_REG_ENABLE		0x10
+#define SY7802_REG_TORCH_BRIGHTNESS	0xa0
+#define SY7802_REG_FLASH_BRIGHTNESS	0xb0
+#define SY7802_REG_FLASH_DURATION	0xc0
+#define SY7802_REG_FLAGS		0xd0
+#define SY7802_REG_CONFIG_1		0xe0
+#define SY7802_REG_CONFIG_2		0xf0
+#define SY7802_REG_VIN_MONITOR		0x80
+#define SY7802_REG_LAST_FLASH		0x81
+#define SY7802_REG_VLED_MONITOR		0x30
+#define SY7802_REG_ADC_DELAY		0x31
+#define SY7802_REG_DEV_ID		0xff
+
+#define SY7802_MODE_OFF		0
+#define SY7802_MODE_TORCH	2
+#define SY7802_MODE_FLASH	3
+#define SY7802_MODE_MASK	GENMASK(1, 0)
+
+#define SY7802_LEDS_SHIFT	3
+#define SY7802_LEDS_MASK(_id)	(BIT(_id) << SY7802_LEDS_SHIFT)
+#define SY7802_LEDS_MASK_ALL	(SY7802_LEDS_MASK(0) | SY7802_LEDS_MASK(1))
+
+#define SY7802_TORCH_CURRENT_SHIFT	3
+#define SY7802_TORCH_CURRENT_MASK(_id) \
+	(GENMASK(2, 0) << (SY7802_TORCH_CURRENT_SHIFT * (_id)))
+#define SY7802_TORCH_CURRENT_MASK_ALL \
+	(SY7802_TORCH_CURRENT_MASK(0) | SY7802_TORCH_CURRENT_MASK(1))
+
+#define SY7802_FLASH_CURRENT_SHIFT	4
+#define SY7802_FLASH_CURRENT_MASK(_id) \
+	(GENMASK(3, 0) << (SY7802_FLASH_CURRENT_SHIFT * (_id)))
+#define SY7802_FLASH_CURRENT_MASK_ALL \
+	(SY7802_FLASH_CURRENT_MASK(0) | SY7802_FLASH_CURRENT_MASK(1))
+
+#define SY7802_TIMEOUT_DEFAULT_US	512000U
+#define SY7802_TIMEOUT_MIN_US		32000U
+#define SY7802_TIMEOUT_MAX_US		1024000U
+#define SY7802_TIMEOUT_STEPSIZE_US	32000U
+
+#define SY7802_TORCH_BRIGHTNESS_MAX 8
+
+#define SY7802_FLASH_BRIGHTNESS_DEFAULT	14
+#define SY7802_FLASH_BRIGHTNESS_MIN	0
+#define SY7802_FLASH_BRIGHTNESS_MAX	15
+#define SY7802_FLASH_BRIGHTNESS_STEP	1
+
+#define SY7802_FLAG_TIMEOUT			(1 << 0)
+#define SY7802_FLAG_THERMAL_SHUTDOWN		(1 << 1)
+#define SY7802_FLAG_LED_FAULT			(1 << 2)
+#define SY7802_FLAG_TX1_INTERRUPT		(1 << 3)
+#define SY7802_FLAG_TX2_INTERRUPT		(1 << 4)
+#define SY7802_FLAG_LED_THERMAL_FAULT		(1 << 5)
+#define SY7802_FLAG_FLASH_INPUT_VOLTAGE_LOW	(1 << 6)
+#define SY7802_FLAG_INPUT_VOLTAGE_LOW		(1 << 7)
+
+#define SY7802_CHIP_ID	0x51
+
+static const struct reg_default sy7802_regmap_defs[] = {
+	{ SY7802_REG_ENABLE, SY7802_LEDS_MASK_ALL },
+	{ SY7802_REG_TORCH_BRIGHTNESS, 0x92 },
+	{ SY7802_REG_FLASH_BRIGHTNESS, SY7802_FLASH_BRIGHTNESS_DEFAULT |
+		SY7802_FLASH_BRIGHTNESS_DEFAULT << SY7802_FLASH_CURRENT_SHIFT },
+	{ SY7802_REG_FLASH_DURATION, 0x6f },
+	{ SY7802_REG_FLAGS, 0x0 },
+	{ SY7802_REG_CONFIG_1, 0x68 },
+	{ SY7802_REG_CONFIG_2, 0xf0 },
+};
+
+struct sy7802_led {
+	struct led_classdev_flash flash;
+	struct sy7802 *chip;
+	u8 led_no;
+};
+
+struct sy7802 {
+	struct mutex mutex;
+	struct device *dev;
+	struct regmap *regmap;
+
+	struct gpio_desc *enable_gpio;
+	struct regulator *vin_regulator;
+
+	unsigned int fled_strobe_used;
+	unsigned int fled_torch_used;
+	unsigned int leds_active;
+	int num_leds;
+	struct sy7802_led leds[] __counted_by(num_leds);
+};
+
+static int sy7802_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
+{
+	struct sy7802_led *led = container_of(lcdev, struct sy7802_led, flash.led_cdev);
+	u32 led_enable_mask = led->led_no == SY7802_LED_JOINT ? SY7802_LEDS_MASK_ALL :
+			      SY7802_LEDS_MASK(led->led_no);
+	u32 enable_mask = SY7802_MODE_MASK | led_enable_mask;
+	u32 val = level ? led_enable_mask : SY7802_MODE_OFF;
+	struct sy7802 *chip = led->chip;
+	u32 curr;
+	u32 mask;
+	int ret;
+
+	mutex_lock(&chip->mutex);
+
+	/*
+	 * There is only one set of flash control logic, and this flag is used to check if 'strobe'
+	 * is currently being used.
+	 */
+	if (chip->fled_strobe_used) {
+		dev_warn(chip->dev, "Please disable strobe first [%d]\n", chip->fled_strobe_used);
+		ret = -EBUSY;
+		goto unlock;
+	}
+
+	if (level)
+		curr = chip->fled_torch_used | BIT(led->led_no);
+	else
+		curr = chip->fled_torch_used & ~BIT(led->led_no);
+
+	if (curr)
+		val |= SY7802_MODE_TORCH;
+
+	/* Torch needs to be disabled first to apply new brightness */
+	ret = regmap_update_bits(chip->regmap, SY7802_REG_ENABLE, SY7802_MODE_MASK,
+				 SY7802_MODE_OFF);
+	if (ret)
+		goto unlock;
+
+	mask = led->led_no == SY7802_LED_JOINT ? SY7802_TORCH_CURRENT_MASK_ALL :
+	       SY7802_TORCH_CURRENT_MASK(led->led_no);
+
+	/* Register expects brightness between 0 and MAX_BRIGHTNESS - 1 */
+	if (level)
+		level -= 1;
+
+	level |= (level << SY7802_TORCH_CURRENT_SHIFT);
+
+	ret = regmap_update_bits(chip->regmap, SY7802_REG_TORCH_BRIGHTNESS, mask, level);
+	if (ret)
+		goto unlock;
+
+	ret = regmap_update_bits(chip->regmap, SY7802_REG_ENABLE, enable_mask, val);
+	if (ret)
+		goto unlock;
+
+	chip->fled_torch_used = curr;
+
+unlock:
+	mutex_unlock(&chip->mutex);
+	return ret;
+}
+
+static int sy7802_flash_brightness_set(struct led_classdev_flash *fl_cdev, u32 brightness)
+{
+	struct sy7802_led *led = container_of(fl_cdev, struct sy7802_led, flash);
+	struct led_flash_setting *s = &fl_cdev->brightness;
+	u32 val = (brightness - s->min) / s->step;
+	struct sy7802 *chip = led->chip;
+	u32 mask;
+	int ret;
+
+	val |= (val << SY7802_FLASH_CURRENT_SHIFT);
+	mask = led->led_no == SY7802_LED_JOINT ? SY7802_FLASH_CURRENT_MASK_ALL :
+	       SY7802_FLASH_CURRENT_MASK(led->led_no);
+
+	mutex_lock(&chip->mutex);
+	ret = regmap_update_bits(chip->regmap, SY7802_REG_FLASH_BRIGHTNESS, mask, val);
+	mutex_unlock(&chip->mutex);
+
+	return ret;
+}
+
+static int sy7802_strobe_set(struct led_classdev_flash *fl_cdev, bool state)
+{
+	struct sy7802_led *led = container_of(fl_cdev, struct sy7802_led, flash);
+	u32 led_enable_mask = led->led_no == SY7802_LED_JOINT ? SY7802_LEDS_MASK_ALL :
+			      SY7802_LEDS_MASK(led->led_no);
+	u32 enable_mask = SY7802_MODE_MASK | led_enable_mask;
+	u32 val = state ? led_enable_mask : SY7802_MODE_OFF;
+	struct sy7802 *chip = led->chip;
+	u32 curr;
+	int ret;
+
+	mutex_lock(&chip->mutex);
+
+	/*
+	 * There is only one set of flash control logic, and this flag is used to check if 'torch'
+	 * is currently being used.
+	 */
+	if (chip->fled_torch_used) {
+		dev_warn(chip->dev, "Please disable torch first [0x%x]\n", chip->fled_torch_used);
+		ret = -EBUSY;
+		goto unlock;
+	}
+
+	if (state)
+		curr = chip->fled_strobe_used | BIT(led->led_no);
+	else
+		curr = chip->fled_strobe_used & ~BIT(led->led_no);
+
+	if (curr)
+		val |= SY7802_MODE_FLASH;
+
+	ret = regmap_update_bits(chip->regmap, SY7802_REG_ENABLE, enable_mask, val);
+
+	if (ret)
+		goto unlock;
+
+	chip->fled_strobe_used = curr;
+
+unlock:
+	mutex_unlock(&chip->mutex);
+	return ret;
+}
+
+static int sy7802_strobe_get(struct led_classdev_flash *fl_cdev, bool *state)
+{
+	struct sy7802_led *led = container_of(fl_cdev, struct sy7802_led, flash);
+	struct sy7802 *chip = led->chip;
+
+	mutex_lock(&chip->mutex);
+	*state = !!(chip->fled_strobe_used & BIT(led->led_no));
+	mutex_unlock(&chip->mutex);
+
+	return 0;
+}
+
+static int sy7802_timeout_set(struct led_classdev_flash *fl_cdev,
+			      u32 timeout)
+{
+	struct sy7802_led *led = container_of(fl_cdev, struct sy7802_led, flash);
+	struct led_flash_setting *s = &fl_cdev->timeout;
+	u32 val = (timeout - s->min) / s->step;
+	struct sy7802 *chip = led->chip;
+
+	return regmap_write(chip->regmap, SY7802_REG_FLASH_DURATION, val);
+}
+
+static int sy7802_fault_get(struct led_classdev_flash *fl_cdev, u32 *fault)
+{
+	struct sy7802_led *led = container_of(fl_cdev, struct sy7802_led, flash);
+	struct sy7802 *chip = led->chip;
+	u32 val, led_faults = 0;
+	int ret;
+
+	/* NOTE: reading register clears fault status */
+	ret = regmap_read(chip->regmap, SY7802_REG_FLAGS, &val);
+	if (ret)
+		return ret;
+
+	if (val & (SY7802_FLAG_FLASH_INPUT_VOLTAGE_LOW | SY7802_FLAG_INPUT_VOLTAGE_LOW))
+		led_faults |= LED_FAULT_INPUT_VOLTAGE;
+
+	if (val & SY7802_FLAG_THERMAL_SHUTDOWN)
+		led_faults |= LED_FAULT_OVER_TEMPERATURE;
+
+	if (val & SY7802_FLAG_TIMEOUT)
+		led_faults |= LED_FAULT_TIMEOUT;
+
+	*fault = led_faults;
+	return 0;
+}
+
+static const struct led_flash_ops sy7802_flash_ops = {
+	.flash_brightness_set = sy7802_flash_brightness_set,
+	.strobe_set = sy7802_strobe_set,
+	.strobe_get = sy7802_strobe_get,
+	.timeout_set = sy7802_timeout_set,
+	.fault_get = sy7802_fault_get,
+};
+
+static void sy7802_init_flash_brightness(struct led_classdev_flash *fl_cdev)
+{
+	struct led_flash_setting *s;
+
+	/* Init flash brightness setting */
+	s = &fl_cdev->brightness;
+	s->min = SY7802_FLASH_BRIGHTNESS_MIN;
+	s->max = SY7802_FLASH_BRIGHTNESS_MAX;
+	s->step = SY7802_FLASH_BRIGHTNESS_STEP;
+	s->val = SY7802_FLASH_BRIGHTNESS_DEFAULT;
+}
+
+static void sy7802_init_flash_timeout(struct led_classdev_flash *fl_cdev)
+{
+	struct led_flash_setting *s;
+
+	/* Init flash timeout setting */
+	s = &fl_cdev->timeout;
+	s->min = SY7802_TIMEOUT_MIN_US;
+	s->max = SY7802_TIMEOUT_MAX_US;
+	s->step = SY7802_TIMEOUT_STEPSIZE_US;
+	s->val = SY7802_TIMEOUT_DEFAULT_US;
+}
+
+static int sy7802_led_register(struct device *dev, struct sy7802_led *led,
+			       struct device_node *np)
+{
+	struct led_init_data init_data = {};
+	int ret;
+
+	init_data.fwnode = of_fwnode_handle(np);
+
+	ret = devm_led_classdev_flash_register_ext(dev, &led->flash, &init_data);
+	if (ret) {
+		dev_err(dev, "Couldn't register flash %d\n", led->led_no);
+		return ret;
+	}
+
+	return ret;
+}
+
+static int sy7802_init_flash_properties(struct device *dev, struct sy7802_led *led,
+					struct device_node *np)
+{
+	struct led_classdev_flash *flash = &led->flash;
+	struct led_classdev *lcdev = &flash->led_cdev;
+	u32 sources[SY7802_MAX_LEDS];
+	int i, num, ret;
+
+	num = of_property_count_u32_elems(np, "led-sources");
+	if (num < 1) {
+		dev_err(dev, "Not specified or wrong number of led-sources\n");
+		return -EINVAL;
+	}
+
+	ret = of_property_read_u32_array(np, "led-sources", sources, num);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < num; i++) {
+		if (sources[i] >= SY7802_MAX_LEDS)
+			return -EINVAL;
+		if (led->chip->leds_active & BIT(sources[i]))
+			return -EINVAL;
+		led->chip->leds_active |= BIT(sources[i]);
+	}
+
+	/* If both channels are specified in 'led-sources', joint flash output mode is used */
+	led->led_no = num == 2 ? SY7802_LED_JOINT : sources[0];
+
+	lcdev->max_brightness = SY7802_TORCH_BRIGHTNESS_MAX;
+	lcdev->brightness_set_blocking = sy7802_torch_brightness_set;
+	lcdev->flags |= LED_DEV_CAP_FLASH;
+
+	flash->ops = &sy7802_flash_ops;
+
+	sy7802_init_flash_brightness(flash);
+	sy7802_init_flash_timeout(flash);
+
+	return 0;
+}
+
+static int sy7802_chip_check(struct sy7802 *chip)
+{
+	struct device *dev = chip->dev;
+	u32 chipid;
+	int ret;
+
+	ret = regmap_read(chip->regmap, SY7802_REG_DEV_ID, &chipid);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to read chip ID\n");
+
+	if (chipid != SY7802_CHIP_ID)
+		return dev_err_probe(dev, -ENODEV, "Chip reported wrong ID: %x\n", chipid);
+
+	return 0;
+}
+
+static void sy7802_enable(struct sy7802 *chip)
+{
+	gpiod_set_value_cansleep(chip->enable_gpio, 1);
+	usleep_range(200, 300);
+}
+
+static void sy7802_disable(struct sy7802 *chip)
+{
+	gpiod_set_value_cansleep(chip->enable_gpio, 0);
+}
+
+static int sy7802_probe_dt(struct sy7802 *chip)
+{
+	struct device_node *np = dev_of_node(chip->dev), *child;
+	int i = 0;
+	int ret;
+
+	regmap_write(chip->regmap, SY7802_REG_ENABLE, SY7802_MODE_OFF);
+	regmap_write(chip->regmap, SY7802_REG_TORCH_BRIGHTNESS, LED_OFF);
+
+	for_each_available_child_of_node(np, child) {
+		struct sy7802_led *led = chip->leds + i;
+
+		led->chip = chip;
+		led->led_no = i;
+
+		ret = sy7802_init_flash_properties(chip->dev, led, child);
+		if (ret) {
+			of_node_put(child);
+			return ret;
+		}
+
+		ret = sy7802_led_register(chip->dev, led, child);
+		if (ret) {
+			of_node_put(child);
+			return ret;
+		}
+
+		i++;
+	}
+	return 0;
+}
+
+static const struct regmap_config sy7802_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0xff,
+	.cache_type = REGCACHE_MAPLE,
+	.reg_defaults = sy7802_regmap_defs,
+	.num_reg_defaults = ARRAY_SIZE(sy7802_regmap_defs),
+};
+
+static int sy7802_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct sy7802 *chip;
+	size_t count;
+	int ret;
+
+	count = device_get_child_node_count(dev);
+	if (!count || count > SY7802_MAX_LEDS)
+		return dev_err_probe(dev, -EINVAL,
+		       "No child node or node count over max led number %zu\n", count);
+
+	chip = devm_kzalloc(dev, struct_size(chip, leds, count), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->num_leds = count;
+
+	chip->dev = dev;
+	i2c_set_clientdata(client, chip);
+
+	chip->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
+	ret = PTR_ERR_OR_ZERO(chip->enable_gpio);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to request enable gpio\n");
+
+	chip->vin_regulator = devm_regulator_get(dev, "vin");
+	ret = PTR_ERR_OR_ZERO(chip->vin_regulator);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to request regulator\n");
+
+	ret = regulator_enable(chip->vin_regulator);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable regulator\n");
+
+	chip->regmap = devm_regmap_init_i2c(client, &sy7802_regmap_config);
+	if (IS_ERR(chip->regmap)) {
+		regulator_disable(chip->vin_regulator);
+		return dev_err_probe(dev, PTR_ERR(chip->regmap),
+				    "Failed to allocate register map.\n");
+	}
+
+	ret = sy7802_probe_dt(chip);
+	if (ret < 0) {
+		regulator_disable(chip->vin_regulator);
+		return ret;
+	}
+
+	sy7802_enable(chip);
+
+	ret = sy7802_chip_check(chip);
+	if (ret) {
+		sy7802_disable(chip);
+		regulator_disable(chip->vin_regulator);
+		return ret;
+	}
+
+	mutex_init(&chip->mutex);
+
+	return ret;
+}
+
+static void sy7802_remove(struct i2c_client *client)
+{
+	struct sy7802 *chip = i2c_get_clientdata(client);
+
+	sy7802_disable(chip);
+
+	mutex_destroy(&chip->mutex);
+	regulator_disable(chip->vin_regulator);
+}
+
+static const struct of_device_id __maybe_unused sy7802_leds_match[] = {
+	{ .compatible = "silergy,sy7802", },
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, sy7802_leds_match);
+
+static struct i2c_driver sy7802_driver = {
+	.driver = {
+		.name = "sy7802",
+		.of_match_table = of_match_ptr(sy7802_leds_match),
+	},
+	.probe = sy7802_probe,
+	.remove = sy7802_remove,
+};
+
+module_i2c_driver(sy7802_driver);
+
+MODULE_AUTHOR("André Apitzsch <git@apitzsch.eu>");
+MODULE_DESCRIPTION("Silergy SY7802 flash LED driver");
+MODULE_LICENSE("GPL");

-- 
2.44.0

Re: [PATCH v2 2/3] leds: sy7802: Add support for Silergy SY7802 flash LED controller

[Lee Jones]

On Mon, 01 Apr 2024, André Apitzsch via B4 Relay wrote:

> From: André Apitzsch <git@apitzsch.eu>
> 
> Add support for SY7802 flash LED controller. It can support up to 1.8A
> flash current.

This is a very small commit message for a 500+ line change!

Please, tell us more.

> Signed-off-by: André Apitzsch <git@apitzsch.eu>
> ---
>  drivers/leds/flash/Kconfig       |  11 +
>  drivers/leds/flash/Makefile      |   1 +
>  drivers/leds/flash/leds-sy7802.c | 532 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 544 insertions(+)
> 
> diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
> index 809b6d98bb3e..f39f0bfe6eef 100644
> --- a/drivers/leds/flash/Kconfig
> +++ b/drivers/leds/flash/Kconfig
> @@ -121,4 +121,15 @@ config LEDS_SGM3140
>  	  This option enables support for the SGM3140 500mA Buck/Boost Charge
>  	  Pump LED Driver.
>  
> +config LEDS_SY7802
> +	tristate "LED support for the Silergy SY7802"
> +	depends on I2C && OF
> +	depends on GPIOLIB
> +	select REGMAP_I2C
> +	help
> +	  This option enables support for the SY7802 flash LED controller.
> +	  SY7802 includes torch and flash functions with programmable current.
> +
> +	  This driver can be built as a module, it will be called "leds-sy7802".
> +
>  endif # LEDS_CLASS_FLASH
> diff --git a/drivers/leds/flash/Makefile b/drivers/leds/flash/Makefile
> index 91d60a4b7952..48860eeced79 100644
> --- a/drivers/leds/flash/Makefile
> +++ b/drivers/leds/flash/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_LEDS_QCOM_FLASH)	+= leds-qcom-flash.o
>  obj-$(CONFIG_LEDS_RT4505)	+= leds-rt4505.o
>  obj-$(CONFIG_LEDS_RT8515)	+= leds-rt8515.o
>  obj-$(CONFIG_LEDS_SGM3140)	+= leds-sgm3140.o
> +obj-$(CONFIG_LEDS_SY7802)	+= leds-sy7802.o
> diff --git a/drivers/leds/flash/leds-sy7802.c b/drivers/leds/flash/leds-sy7802.c
> new file mode 100644
> index 000000000000..c03a571b0e08
> --- /dev/null
> +++ b/drivers/leds/flash/leds-sy7802.c
> @@ -0,0 +1,532 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Silergy SY7802 flash LED driver with I2C interface
> + *
> + * Copyright 2024 André Apitzsch <git@apitzsch.eu>
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/led-class-flash.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define SY7802_MAX_LEDS 2
> +#define SY7802_LED_JOINT 2
> +
> +#define SY7802_REG_ENABLE		0x10
> +#define SY7802_REG_TORCH_BRIGHTNESS	0xa0
> +#define SY7802_REG_FLASH_BRIGHTNESS	0xb0
> +#define SY7802_REG_FLASH_DURATION	0xc0
> +#define SY7802_REG_FLAGS		0xd0
> +#define SY7802_REG_CONFIG_1		0xe0
> +#define SY7802_REG_CONFIG_2		0xf0
> +#define SY7802_REG_VIN_MONITOR		0x80
> +#define SY7802_REG_LAST_FLASH		0x81
> +#define SY7802_REG_VLED_MONITOR		0x30
> +#define SY7802_REG_ADC_DELAY		0x31
> +#define SY7802_REG_DEV_ID		0xff
> +
> +#define SY7802_MODE_OFF		0
> +#define SY7802_MODE_TORCH	2
> +#define SY7802_MODE_FLASH	3
> +#define SY7802_MODE_MASK	GENMASK(1, 0)
> +
> +#define SY7802_LEDS_SHIFT	3
> +#define SY7802_LEDS_MASK(_id)	(BIT(_id) << SY7802_LEDS_SHIFT)
> +#define SY7802_LEDS_MASK_ALL	(SY7802_LEDS_MASK(0) | SY7802_LEDS_MASK(1))
> +
> +#define SY7802_TORCH_CURRENT_SHIFT	3
> +#define SY7802_TORCH_CURRENT_MASK(_id) \
> +	(GENMASK(2, 0) << (SY7802_TORCH_CURRENT_SHIFT * (_id)))
> +#define SY7802_TORCH_CURRENT_MASK_ALL \
> +	(SY7802_TORCH_CURRENT_MASK(0) | SY7802_TORCH_CURRENT_MASK(1))
> +
> +#define SY7802_FLASH_CURRENT_SHIFT	4
> +#define SY7802_FLASH_CURRENT_MASK(_id) \
> +	(GENMASK(3, 0) << (SY7802_FLASH_CURRENT_SHIFT * (_id)))
> +#define SY7802_FLASH_CURRENT_MASK_ALL \
> +	(SY7802_FLASH_CURRENT_MASK(0) | SY7802_FLASH_CURRENT_MASK(1))
> +
> +#define SY7802_TIMEOUT_DEFAULT_US	512000U
> +#define SY7802_TIMEOUT_MIN_US		32000U
> +#define SY7802_TIMEOUT_MAX_US		1024000U
> +#define SY7802_TIMEOUT_STEPSIZE_US	32000U
> +
> +#define SY7802_TORCH_BRIGHTNESS_MAX 8
> +
> +#define SY7802_FLASH_BRIGHTNESS_DEFAULT	14
> +#define SY7802_FLASH_BRIGHTNESS_MIN	0
> +#define SY7802_FLASH_BRIGHTNESS_MAX	15
> +#define SY7802_FLASH_BRIGHTNESS_STEP	1

Much nicer to read if everything was aligned.

> +#define SY7802_FLAG_TIMEOUT			(1 << 0)
> +#define SY7802_FLAG_THERMAL_SHUTDOWN		(1 << 1)
> +#define SY7802_FLAG_LED_FAULT			(1 << 2)
> +#define SY7802_FLAG_TX1_INTERRUPT		(1 << 3)
> +#define SY7802_FLAG_TX2_INTERRUPT		(1 << 4)
> +#define SY7802_FLAG_LED_THERMAL_FAULT		(1 << 5)
> +#define SY7802_FLAG_FLASH_INPUT_VOLTAGE_LOW	(1 << 6)
> +#define SY7802_FLAG_INPUT_VOLTAGE_LOW		(1 << 7)

Why not BIT()?

> +#define SY7802_CHIP_ID	0x51
> +
> +static const struct reg_default sy7802_regmap_defs[] = {
> +	{ SY7802_REG_ENABLE, SY7802_LEDS_MASK_ALL },
> +	{ SY7802_REG_TORCH_BRIGHTNESS, 0x92 },
> +	{ SY7802_REG_FLASH_BRIGHTNESS, SY7802_FLASH_BRIGHTNESS_DEFAULT |
> +		SY7802_FLASH_BRIGHTNESS_DEFAULT << SY7802_FLASH_CURRENT_SHIFT },
> +	{ SY7802_REG_FLASH_DURATION, 0x6f },
> +	{ SY7802_REG_FLAGS, 0x0 },
> +	{ SY7802_REG_CONFIG_1, 0x68 },
> +	{ SY7802_REG_CONFIG_2, 0xf0 },
> +};
> +
> +struct sy7802_led {
> +	struct led_classdev_flash flash;
> +	struct sy7802 *chip;
> +	u8 led_no;

Is this LED number or no LED?

How about led_num or led_id?

> +};
> +
> +struct sy7802 {
> +	struct mutex mutex;

Place the big stuff (like struct device and regmap at the top).

> +	struct device *dev;
> +	struct regmap *regmap;
> +
> +	struct gpio_desc *enable_gpio;
> +	struct regulator *vin_regulator;
> +
> +	unsigned int fled_strobe_used;
> +	unsigned int fled_torch_used;
> +	unsigned int leds_active;
> +	int num_leds;
> +	struct sy7802_led leds[] __counted_by(num_leds);
> +};
> +
> +static int sy7802_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level)

s/level/brightness/

> +{
> +	struct sy7802_led *led = container_of(lcdev, struct sy7802_led, flash.led_cdev);
> +	u32 led_enable_mask = led->led_no == SY7802_LED_JOINT ? SY7802_LEDS_MASK_ALL :
> +			      SY7802_LEDS_MASK(led->led_no);

Do all of the fancy multi-line assignment outside of the declaration block.

> +	u32 enable_mask = SY7802_MODE_MASK | led_enable_mask;
> +	u32 val = level ? led_enable_mask : SY7802_MODE_OFF;
> +	struct sy7802 *chip = led->chip;
> +	u32 curr;

This is a temporary placeholder for fled_torch_used, right?

fled_torch_used_tmp?  Sometimes abbreviated to tmp.

> +	u32 mask;

That's a lot of masks.  Which one is this?

> +	int ret;
> +
> +	mutex_lock(&chip->mutex);
> +
> +	/*
> +	 * There is only one set of flash control logic, and this flag is used to check if 'strobe'

The ',' before 'and' is superfluous.

> +	 * is currently being used.
> +	 */

Doesn't the variable name kind of imply this?

> +	if (chip->fled_strobe_used) {
> +		dev_warn(chip->dev, "Please disable strobe first [%d]\n", chip->fled_strobe_used);

"Cannot set torch brightness whilst strobe is enabled"

> +		ret = -EBUSY;
> +		goto unlock;
> +	}
> +
> +	if (level)
> +		curr = chip->fled_torch_used | BIT(led->led_no);
> +	else
> +		curr = chip->fled_torch_used & ~BIT(led->led_no);
> +
> +	if (curr)
> +		val |= SY7802_MODE_TORCH;
> +
> +	/* Torch needs to be disabled first to apply new brightness */

"Disable touch to apply brightness"

> +	ret = regmap_update_bits(chip->regmap, SY7802_REG_ENABLE, SY7802_MODE_MASK,
> +				 SY7802_MODE_OFF);
> +	if (ret)
> +		goto unlock;
> +
> +	mask = led->led_no == SY7802_LED_JOINT ? SY7802_TORCH_CURRENT_MASK_ALL :

Why not just use led->led_no in place of mask?

Easier to read if you drop SY7802_TORCH_CURRENT_MASK_ALL to its own line.

> +	       SY7802_TORCH_CURRENT_MASK(led->led_no);
> +
> +	/* Register expects brightness between 0 and MAX_BRIGHTNESS - 1 */
> +	if (level)
> +		level -= 1;
> +
> +	level |= (level << SY7802_TORCH_CURRENT_SHIFT);
> +
> +	ret = regmap_update_bits(chip->regmap, SY7802_REG_TORCH_BRIGHTNESS, mask, level);
> +	if (ret)
> +		goto unlock;
> +
> +	ret = regmap_update_bits(chip->regmap, SY7802_REG_ENABLE, enable_mask, val);
> +	if (ret)
> +		goto unlock;
> +
> +	chip->fled_torch_used = curr;
> +
> +unlock:
> +	mutex_unlock(&chip->mutex);
> +	return ret;
> +}
> +
> +static int sy7802_flash_brightness_set(struct led_classdev_flash *fl_cdev, u32 brightness)
> +{
> +	struct sy7802_led *led = container_of(fl_cdev, struct sy7802_led, flash);
> +	struct led_flash_setting *s = &fl_cdev->brightness;
> +	u32 val = (brightness - s->min) / s->step;
> +	struct sy7802 *chip = led->chip;
> +	u32 mask;
> +	int ret;
> +
> +	val |= (val << SY7802_FLASH_CURRENT_SHIFT);
> +	mask = led->led_no == SY7802_LED_JOINT ? SY7802_FLASH_CURRENT_MASK_ALL :
> +	       SY7802_FLASH_CURRENT_MASK(led->led_no);
> +
> +	mutex_lock(&chip->mutex);
> +	ret = regmap_update_bits(chip->regmap, SY7802_REG_FLASH_BRIGHTNESS, mask, val);
> +	mutex_unlock(&chip->mutex);
> +
> +	return ret;
> +}
> +
> +static int sy7802_strobe_set(struct led_classdev_flash *fl_cdev, bool state)

Same comments as above apply throughout the driver.

> +{
> +	struct sy7802_led *led = container_of(fl_cdev, struct sy7802_led, flash);
> +	u32 led_enable_mask = led->led_no == SY7802_LED_JOINT ? SY7802_LEDS_MASK_ALL :
> +			      SY7802_LEDS_MASK(led->led_no);
> +	u32 enable_mask = SY7802_MODE_MASK | led_enable_mask;
> +	u32 val = state ? led_enable_mask : SY7802_MODE_OFF;
> +	struct sy7802 *chip = led->chip;
> +	u32 curr;
> +	int ret;
> +
> +	mutex_lock(&chip->mutex);
> +
> +	/*
> +	 * There is only one set of flash control logic, and this flag is used to check if 'torch'
> +	 * is currently being used.
> +	 */
> +	if (chip->fled_torch_used) {
> +		dev_warn(chip->dev, "Please disable torch first [0x%x]\n", chip->fled_torch_used);
> +		ret = -EBUSY;
> +		goto unlock;
> +	}
> +
> +	if (state)
> +		curr = chip->fled_strobe_used | BIT(led->led_no);
> +	else
> +		curr = chip->fled_strobe_used & ~BIT(led->led_no);
> +
> +	if (curr)
> +		val |= SY7802_MODE_FLASH;
> +
> +	ret = regmap_update_bits(chip->regmap, SY7802_REG_ENABLE, enable_mask, val);
> +
> +	if (ret)
> +		goto unlock;
> +
> +	chip->fled_strobe_used = curr;
> +
> +unlock:
> +	mutex_unlock(&chip->mutex);
> +	return ret;
> +}
> +
> +static int sy7802_strobe_get(struct led_classdev_flash *fl_cdev, bool *state)
> +{
> +	struct sy7802_led *led = container_of(fl_cdev, struct sy7802_led, flash);
> +	struct sy7802 *chip = led->chip;
> +
> +	mutex_lock(&chip->mutex);
> +	*state = !!(chip->fled_strobe_used & BIT(led->led_no));
> +	mutex_unlock(&chip->mutex);
> +
> +	return 0;
> +}
> +
> +static int sy7802_timeout_set(struct led_classdev_flash *fl_cdev,
> +			      u32 timeout)
> +{
> +	struct sy7802_led *led = container_of(fl_cdev, struct sy7802_led, flash);
> +	struct led_flash_setting *s = &fl_cdev->timeout;
> +	u32 val = (timeout - s->min) / s->step;
> +	struct sy7802 *chip = led->chip;
> +
> +	return regmap_write(chip->regmap, SY7802_REG_FLASH_DURATION, val);
> +}
> +
> +static int sy7802_fault_get(struct led_classdev_flash *fl_cdev, u32 *fault)
> +{
> +	struct sy7802_led *led = container_of(fl_cdev, struct sy7802_led, flash);
> +	struct sy7802 *chip = led->chip;
> +	u32 val, led_faults = 0;
> +	int ret;
> +
> +	/* NOTE: reading register clears fault status */
> +	ret = regmap_read(chip->regmap, SY7802_REG_FLAGS, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val & (SY7802_FLAG_FLASH_INPUT_VOLTAGE_LOW | SY7802_FLAG_INPUT_VOLTAGE_LOW))
> +		led_faults |= LED_FAULT_INPUT_VOLTAGE;
> +
> +	if (val & SY7802_FLAG_THERMAL_SHUTDOWN)
> +		led_faults |= LED_FAULT_OVER_TEMPERATURE;
> +
> +	if (val & SY7802_FLAG_TIMEOUT)
> +		led_faults |= LED_FAULT_TIMEOUT;
> +
> +	*fault = led_faults;
> +	return 0;
> +}
> +
> +static const struct led_flash_ops sy7802_flash_ops = {
> +	.flash_brightness_set = sy7802_flash_brightness_set,
> +	.strobe_set = sy7802_strobe_set,
> +	.strobe_get = sy7802_strobe_get,
> +	.timeout_set = sy7802_timeout_set,
> +	.fault_get = sy7802_fault_get,
> +};
> +
> +static void sy7802_init_flash_brightness(struct led_classdev_flash *fl_cdev)
> +{
> +	struct led_flash_setting *s;
> +
> +	/* Init flash brightness setting */
> +	s = &fl_cdev->brightness;
> +	s->min = SY7802_FLASH_BRIGHTNESS_MIN;
> +	s->max = SY7802_FLASH_BRIGHTNESS_MAX;
> +	s->step = SY7802_FLASH_BRIGHTNESS_STEP;
> +	s->val = SY7802_FLASH_BRIGHTNESS_DEFAULT;
> +}
> +
> +static void sy7802_init_flash_timeout(struct led_classdev_flash *fl_cdev)
> +{
> +	struct led_flash_setting *s;
> +
> +	/* Init flash timeout setting */
> +	s = &fl_cdev->timeout;
> +	s->min = SY7802_TIMEOUT_MIN_US;
> +	s->max = SY7802_TIMEOUT_MAX_US;
> +	s->step = SY7802_TIMEOUT_STEPSIZE_US;
> +	s->val = SY7802_TIMEOUT_DEFAULT_US;
> +}
> +
> +static int sy7802_led_register(struct device *dev, struct sy7802_led *led,
> +			       struct device_node *np)
> +{
> +	struct led_init_data init_data = {};
> +	int ret;
> +
> +	init_data.fwnode = of_fwnode_handle(np);
> +
> +	ret = devm_led_classdev_flash_register_ext(dev, &led->flash, &init_data);
> +	if (ret) {
> +		dev_err(dev, "Couldn't register flash %d\n", led->led_no);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int sy7802_init_flash_properties(struct device *dev, struct sy7802_led *led,
> +					struct device_node *np)
> +{
> +	struct led_classdev_flash *flash = &led->flash;
> +	struct led_classdev *lcdev = &flash->led_cdev;
> +	u32 sources[SY7802_MAX_LEDS];
> +	int i, num, ret;
> +
> +	num = of_property_count_u32_elems(np, "led-sources");
> +	if (num < 1) {
> +		dev_err(dev, "Not specified or wrong number of led-sources\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = of_property_read_u32_array(np, "led-sources", sources, num);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < num; i++) {
> +		if (sources[i] >= SY7802_MAX_LEDS)
> +			return -EINVAL;
> +		if (led->chip->leds_active & BIT(sources[i]))
> +			return -EINVAL;
> +		led->chip->leds_active |= BIT(sources[i]);
> +	}
> +
> +	/* If both channels are specified in 'led-sources', joint flash output mode is used */
> +	led->led_no = num == 2 ? SY7802_LED_JOINT : sources[0];
> +
> +	lcdev->max_brightness = SY7802_TORCH_BRIGHTNESS_MAX;
> +	lcdev->brightness_set_blocking = sy7802_torch_brightness_set;
> +	lcdev->flags |= LED_DEV_CAP_FLASH;
> +
> +	flash->ops = &sy7802_flash_ops;
> +
> +	sy7802_init_flash_brightness(flash);
> +	sy7802_init_flash_timeout(flash);
> +
> +	return 0;
> +}
> +
> +static int sy7802_chip_check(struct sy7802 *chip)
> +{
> +	struct device *dev = chip->dev;
> +	u32 chipid;
> +	int ret;
> +
> +	ret = regmap_read(chip->regmap, SY7802_REG_DEV_ID, &chipid);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to read chip ID\n");
> +
> +	if (chipid != SY7802_CHIP_ID)
> +		return dev_err_probe(dev, -ENODEV, "Chip reported wrong ID: %x\n", chipid);

"Unsupported chip detected"

> +
> +	return 0;
> +}
> +
> +static void sy7802_enable(struct sy7802 *chip)
> +{
> +	gpiod_set_value_cansleep(chip->enable_gpio, 1);
> +	usleep_range(200, 300);
> +}
> +
> +static void sy7802_disable(struct sy7802 *chip)
> +{
> +	gpiod_set_value_cansleep(chip->enable_gpio, 0);
> +}
> +
> +static int sy7802_probe_dt(struct sy7802 *chip)
> +{
> +	struct device_node *np = dev_of_node(chip->dev), *child;

This is ugly.  Place 'child' on a separate line.

> +	int i = 0;

If you're going to use a variable like this, rename it.

'count' or 'child_num' perhaps.

And reset/assign it just before you're going to use it for clarity.

> +	int ret;
> +
> +	regmap_write(chip->regmap, SY7802_REG_ENABLE, SY7802_MODE_OFF);
> +	regmap_write(chip->regmap, SY7802_REG_TORCH_BRIGHTNESS, LED_OFF);
> +

        child_num = 0;
> +	for_each_available_child_of_node(np, child) {
> +		struct sy7802_led *led = chip->leds + i;
> +
> +		led->chip = chip;
> +		led->led_no = i;
> +
> +		ret = sy7802_init_flash_properties(chip->dev, led, child);
> +		if (ret) {
> +			of_node_put(child);
> +			return ret;
> +		}
> +
> +		ret = sy7802_led_register(chip->dev, led, child);
> +		if (ret) {
> +			of_node_put(child);
> +			return ret;
> +		}
> +
> +		i++;
> +	}
> +	return 0;
> +}
> +
> +static const struct regmap_config sy7802_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0xff,
> +	.cache_type = REGCACHE_MAPLE,
> +	.reg_defaults = sy7802_regmap_defs,
> +	.num_reg_defaults = ARRAY_SIZE(sy7802_regmap_defs),
> +};
> +
> +static int sy7802_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct sy7802 *chip;
> +	size_t count;
> +	int ret;
> +
> +	count = device_get_child_node_count(dev);
> +	if (!count || count > SY7802_MAX_LEDS)
> +		return dev_err_probe(dev, -EINVAL,
> +		       "No child node or node count over max led number %zu\n", count);

Split them up and report on them individually or combine the error message:

"Invalid amount of LED nodes"

> +	chip = devm_kzalloc(dev, struct_size(chip, leds, count), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	chip->num_leds = count;
> +
> +	chip->dev = dev;
> +	i2c_set_clientdata(client, chip);
> +
> +	chip->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> +	ret = PTR_ERR_OR_ZERO(chip->enable_gpio);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to request enable gpio\n");
> +
> +	chip->vin_regulator = devm_regulator_get(dev, "vin");
> +	ret = PTR_ERR_OR_ZERO(chip->vin_regulator);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to request regulator\n");
> +
> +	ret = regulator_enable(chip->vin_regulator);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable regulator\n");
> +
> +	chip->regmap = devm_regmap_init_i2c(client, &sy7802_regmap_config);
> +	if (IS_ERR(chip->regmap)) {
> +		regulator_disable(chip->vin_regulator);

Nicer to use gotos for the error path.

3 calls to regulator_disable() is avoidable.

> +		return dev_err_probe(dev, PTR_ERR(chip->regmap),
> +				    "Failed to allocate register map.\n");
> +	}
> +
> +	ret = sy7802_probe_dt(chip);
> +	if (ret < 0) {
> +		regulator_disable(chip->vin_regulator);
> +		return ret;
> +	}
> +
> +	sy7802_enable(chip);
> +
> +	ret = sy7802_chip_check(chip);
> +	if (ret) {
> +		sy7802_disable(chip);
> +		regulator_disable(chip->vin_regulator);
> +		return ret;
> +	}
> +
> +	mutex_init(&chip->mutex);
> +
> +	return ret;
> +}
> +
> +static void sy7802_remove(struct i2c_client *client)
> +{
> +	struct sy7802 *chip = i2c_get_clientdata(client);
> +
> +	sy7802_disable(chip);
> +
> +	mutex_destroy(&chip->mutex);
> +	regulator_disable(chip->vin_regulator);
> +}
> +
> +static const struct of_device_id __maybe_unused sy7802_leds_match[] = {
> +	{ .compatible = "silergy,sy7802", },
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(of, sy7802_leds_match);
> +
> +static struct i2c_driver sy7802_driver = {
> +	.driver = {
> +		.name = "sy7802",
> +		.of_match_table = of_match_ptr(sy7802_leds_match),
> +	},
> +	.probe = sy7802_probe,
> +	.remove = sy7802_remove,
> +};
> +
> +module_i2c_driver(sy7802_driver);
> +
> +MODULE_AUTHOR("André Apitzsch <git@apitzsch.eu>");
> +MODULE_DESCRIPTION("Silergy SY7802 flash LED driver");
> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.44.0
> 
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 2/3] leds: sy7802: Add support for Silergy SY7802 flash LED controller

[André Apitzsch]

Hi Lee Jones,

thanks for the feedback. I will address your comments in the next
version. I have a few comments/questions though, see below.

Best regards,
André

Am Donnerstag, dem 11.04.2024 um 13:48 +0100 schrieb Lee Jones:
> On Mon, 01 Apr 2024, André Apitzsch via B4 Relay wrote:
> > 
> > [..]
> > +
> > +#define SY7802_TIMEOUT_DEFAULT_US	512000U
> > +#define SY7802_TIMEOUT_MIN_US		32000U
> > +#define SY7802_TIMEOUT_MAX_US		1024000U
> > +#define SY7802_TIMEOUT_STEPSIZE_US	32000U
> > +
> > +#define SY7802_TORCH_BRIGHTNESS_MAX 8
> > +
> > +#define SY7802_FLASH_BRIGHTNESS_DEFAULT	14
> > +#define SY7802_FLASH_BRIGHTNESS_MIN	0
> > +#define SY7802_FLASH_BRIGHTNESS_MAX	15
> > +#define SY7802_FLASH_BRIGHTNESS_STEP	1
> 
> Much nicer to read if everything was aligned.

Using tab size 8, SY7802_FLASH_BRIGHTNESS_* look aligned to me. Do you
refer to SY7802_TORCH_BRIGHTNESS_MAX here? 

> 
> > [..]
> > +
> > +	/*
> > +	 * There is only one set of flash control logic, and this
> > flag is used to check if 'strobe'
> 
> The ',' before 'and' is superfluous.
> 
> > +	 * is currently being used.
> > +	 */
> 
> Doesn't the variable name kind of imply this?
> 
> > +	if (chip->fled_strobe_used) {
> > +		dev_warn(chip->dev, "Please disable strobe first
> > [%d]\n", chip->fled_strobe_used);
> 
> "Cannot set torch brightness whilst strobe is enabled"

The comment and the warn message are taken from 'leds-mt6370-flash.c'.
But I think using the warn message you suggested the comment can be
removed.

> 
> > +		ret = -EBUSY;
> > +		goto unlock;
> > +	}
> > +
> > +	if (level)
> > +		curr = chip->fled_torch_used | BIT(led->led_no);
> > +	else
> > +		curr = chip->fled_torch_used & ~BIT(led->led_no);
> > +
> > +	if (curr)
> > +		val |= SY7802_MODE_TORCH;
> > +
> > +	/* Torch needs to be disabled first to apply new
> > brightness */
> 
> "Disable touch to apply brightness"
> 
> > +	ret = regmap_update_bits(chip->regmap, SY7802_REG_ENABLE,
> > SY7802_MODE_MASK,
> > +				 SY7802_MODE_OFF);
> > +	if (ret)
> > +		goto unlock;
> > +
> > +	mask = led->led_no == SY7802_LED_JOINT ?
> > SY7802_TORCH_CURRENT_MASK_ALL :
> 
> Why not just use led->led_no in place of mask?

I might be missing something, but I don't know how to use led->led_no
in place of mask, when
led->led_no is in {0,1,2} and
mask is in {0x07, 0x38, 0x3f}.

> 
> Easier to read if you drop SY7802_TORCH_CURRENT_MASK_ALL to its own
> line.
> 
> > +	       SY7802_TORCH_CURRENT_MASK(led->led_no);
> > +
> > [..]
> > +
> > +static int sy7802_probe(struct i2c_client *client)
> > +{
> > +	struct device *dev = &client->dev;
> > +	struct sy7802 *chip;
> > +	size_t count;
> > +	int ret;
> > +
> > +	count = device_get_child_node_count(dev);
> > +	if (!count || count > SY7802_MAX_LEDS)
> > +		return dev_err_probe(dev, -EINVAL,
> > +		       "No child node or node count over max led
> > number %zu\n", count);
> 
> Split them up and report on them individually or combine the error
> message:
> 
> "Invalid amount of LED nodes"

This snippet was also taken from 'leds-mt6370-flash.c'.

> 

Re: [PATCH v2 2/3] leds: sy7802: Add support for Silergy SY7802 flash LED controller

[Lee Jones]

On Wed, 01 May 2024, André Apitzsch wrote:

> Hi Lee Jones,
> 
> thanks for the feedback. I will address your comments in the next
> version. I have a few comments/questions though, see below.
> 
> Best regards,
> André
> 
> Am Donnerstag, dem 11.04.2024 um 13:48 +0100 schrieb Lee Jones:
> > On Mon, 01 Apr 2024, André Apitzsch via B4 Relay wrote:
> > > 
> > > [..]
> > > +
> > > +#define SY7802_TIMEOUT_DEFAULT_US	512000U
> > > +#define SY7802_TIMEOUT_MIN_US		32000U
> > > +#define SY7802_TIMEOUT_MAX_US		1024000U
> > > +#define SY7802_TIMEOUT_STEPSIZE_US	32000U
> > > +
> > > +#define SY7802_TORCH_BRIGHTNESS_MAX 8
> > > +
> > > +#define SY7802_FLASH_BRIGHTNESS_DEFAULT	14
> > > +#define SY7802_FLASH_BRIGHTNESS_MIN	0
> > > +#define SY7802_FLASH_BRIGHTNESS_MAX	15
> > > +#define SY7802_FLASH_BRIGHTNESS_STEP	1
> > 
> > Much nicer to read if everything was aligned.
> 
> Using tab size 8, SY7802_FLASH_BRIGHTNESS_* look aligned to me. Do you
> refer to SY7802_TORCH_BRIGHTNESS_MAX here? 

These were not aligned in my mailer.  You can ignore.

> > > [..]
> > > +
> > > +	/*
> > > +	 * There is only one set of flash control logic, and this
> > > flag is used to check if 'strobe'
> > 
> > The ',' before 'and' is superfluous.
> > 
> > > +	 * is currently being used.
> > > +	 */
> > 
> > Doesn't the variable name kind of imply this?
> > 
> > > +	if (chip->fled_strobe_used) {
> > > +		dev_warn(chip->dev, "Please disable strobe first
> > > [%d]\n", chip->fled_strobe_used);
> > 
> > "Cannot set torch brightness whilst strobe is enabled"
> 
> The comment and the warn message are taken from 'leds-mt6370-flash.c'.
> But I think using the warn message you suggested the comment can be
> removed.

It's an improvement, right?

> > > +		ret = -EBUSY;
> > > +		goto unlock;
> > > +	}
> > > +
> > > +	if (level)
> > > +		curr = chip->fled_torch_used | BIT(led->led_no);
> > > +	else
> > > +		curr = chip->fled_torch_used & ~BIT(led->led_no);
> > > +
> > > +	if (curr)
> > > +		val |= SY7802_MODE_TORCH;
> > > +
> > > +	/* Torch needs to be disabled first to apply new
> > > brightness */
> > 
> > "Disable touch to apply brightness"
> > 
> > > +	ret = regmap_update_bits(chip->regmap, SY7802_REG_ENABLE,
> > > SY7802_MODE_MASK,
> > > +				 SY7802_MODE_OFF);
> > > +	if (ret)
> > > +		goto unlock;
> > > +
> > > +	mask = led->led_no == SY7802_LED_JOINT ?
> > > SY7802_TORCH_CURRENT_MASK_ALL :
> > 
> > Why not just use led->led_no in place of mask?
> 
> I might be missing something, but I don't know how to use led->led_no
> in place of mask, when
> led->led_no is in {0,1,2} and
> mask is in {0x07, 0x38, 0x3f}.

This doesn't make much sense.

I guess you mean that led_no is a u8 and mask is a u32.

What happens if you cast led_no to u32 in the call to regmap_update_bits()?

> > Easier to read if you drop SY7802_TORCH_CURRENT_MASK_ALL to its own
> > line.
> > 
> > > +	       SY7802_TORCH_CURRENT_MASK(led->led_no);
> > > +
> > > [..]
> > > +
> > > +static int sy7802_probe(struct i2c_client *client)
> > > +{
> > > +	struct device *dev = &client->dev;
> > > +	struct sy7802 *chip;
> > > +	size_t count;
> > > +	int ret;
> > > +
> > > +	count = device_get_child_node_count(dev);
> > > +	if (!count || count > SY7802_MAX_LEDS)
> > > +		return dev_err_probe(dev, -EINVAL,
> > > +		       "No child node or node count over max led
> > > number %zu\n", count);
> > 
> > Split them up and report on them individually or combine the error
> > message:
> > 
> > "Invalid amount of LED nodes"
> 
> This snippet was also taken from 'leds-mt6370-flash.c'.

Old mistakes should not be repeated. :)

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 2/3] leds: sy7802: Add support for Silergy SY7802 flash LED controller

[André Apitzsch]

Am Donnerstag, dem 02.05.2024 um 10:10 +0100 schrieb Lee Jones:
> On Wed, 01 May 2024, André Apitzsch wrote:
> 
> > > > +		ret = -EBUSY;
> > > > +		goto unlock;
> > > > +	}
> > > > +
> > > > +	if (level)
> > > > +		curr = chip->fled_torch_used | BIT(led-
> > > > >led_no);
> > > > +	else
> > > > +		curr = chip->fled_torch_used & ~BIT(led-
> > > > >led_no);
> > > > +
> > > > +	if (curr)
> > > > +		val |= SY7802_MODE_TORCH;
> > > > +
> > > > +	/* Torch needs to be disabled first to apply new
> > > > brightness */
> > > 
> > > "Disable touch to apply brightness"
> > > 
> > > > +	ret = regmap_update_bits(chip->regmap,
> > > > SY7802_REG_ENABLE,
> > > > SY7802_MODE_MASK,
> > > > +				 SY7802_MODE_OFF);
> > > > +	if (ret)
> > > > +		goto unlock;
> > > > +
> > > > +	mask = led->led_no == SY7802_LED_JOINT ?
> > > > SY7802_TORCH_CURRENT_MASK_ALL :
> > > 
> > > Why not just use led->led_no in place of mask?
> > 
> > I might be missing something, but I don't know how to use led-
> > >led_no
> > in place of mask, when
> > led->led_no is in {0,1,2} and
> > mask is in {0x07, 0x38, 0x3f}.
> 
> This doesn't make much sense.
> 
> I guess you mean that led_no is a u8 and mask is a u32.
> 
> What happens if you cast led_no to u32 in the call to
> regmap_update_bits()?

Sorry, I'm still confused. Could you elaborate your original question?

> 
> > > Easier to read if you drop SY7802_TORCH_CURRENT_MASK_ALL to its
> > > own
> > > line.
> > > 
> > > > +	       SY7802_TORCH_CURRENT_MASK(led->led_no);
> > > > +
> > > > [..]
> > > > 
> 

Re: [PATCH v2 2/3] leds: sy7802: Add support for Silergy SY7802 flash LED controller

[Lee Jones]

On Thu, 02 May 2024, André Apitzsch wrote:

> Am Donnerstag, dem 02.05.2024 um 10:10 +0100 schrieb Lee Jones:
> > On Wed, 01 May 2024, André Apitzsch wrote:
> > 
> > > > > +		ret = -EBUSY;
> > > > > +		goto unlock;
> > > > > +	}
> > > > > +
> > > > > +	if (level)
> > > > > +		curr = chip->fled_torch_used | BIT(led-
> > > > > >led_no);
> > > > > +	else
> > > > > +		curr = chip->fled_torch_used & ~BIT(led-
> > > > > >led_no);
> > > > > +
> > > > > +	if (curr)
> > > > > +		val |= SY7802_MODE_TORCH;
> > > > > +
> > > > > +	/* Torch needs to be disabled first to apply new
> > > > > brightness */
> > > > 
> > > > "Disable touch to apply brightness"
> > > > 
> > > > > +	ret = regmap_update_bits(chip->regmap,
> > > > > SY7802_REG_ENABLE,
> > > > > SY7802_MODE_MASK,
> > > > > +				 SY7802_MODE_OFF);
> > > > > +	if (ret)
> > > > > +		goto unlock;
> > > > > +
> > > > > +	mask = led->led_no == SY7802_LED_JOINT ?
> > > > > SY7802_TORCH_CURRENT_MASK_ALL :
> > > > 
> > > > Why not just use led->led_no in place of mask?
> > > 
> > > I might be missing something, but I don't know how to use led-
> > > >led_no
> > > in place of mask, when
> > > led->led_no is in {0,1,2} and
> > > mask is in {0x07, 0x38, 0x3f}.
> > 
> > This doesn't make much sense.
> > 
> > I guess you mean that led_no is a u8 and mask is a u32.
> > 
> > What happens if you cast led_no to u32 in the call to
> > regmap_update_bits()?
> 
> Sorry, I'm still confused. Could you elaborate your original question?

Replied where there is still context.

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 2/3] leds: sy7802: Add support for Silergy SY7802 flash LED controller

[Lee Jones]

On Thu, 11 Apr 2024, Lee Jones wrote:

> On Mon, 01 Apr 2024, André Apitzsch via B4 Relay wrote:
> 
> > From: André Apitzsch <git@apitzsch.eu>
> > 
> > Add support for SY7802 flash LED controller. It can support up to 1.8A
> > flash current.
> 
> This is a very small commit message for a 500+ line change!
> 
> Please, tell us more.
> 
> > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > ---
> >  drivers/leds/flash/Kconfig       |  11 +
> >  drivers/leds/flash/Makefile      |   1 +
> >  drivers/leds/flash/leds-sy7802.c | 532 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 544 insertions(+)
> > 
> > diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
> > index 809b6d98bb3e..f39f0bfe6eef 100644
> > --- a/drivers/leds/flash/Kconfig
> > +++ b/drivers/leds/flash/Kconfig
> > @@ -121,4 +121,15 @@ config LEDS_SGM3140
> >  	  This option enables support for the SGM3140 500mA Buck/Boost Charge
> >  	  Pump LED Driver.
> >  
> > +config LEDS_SY7802
> > +	tristate "LED support for the Silergy SY7802"
> > +	depends on I2C && OF
> > +	depends on GPIOLIB
> > +	select REGMAP_I2C
> > +	help
> > +	  This option enables support for the SY7802 flash LED controller.
> > +	  SY7802 includes torch and flash functions with programmable current.
> > +
> > +	  This driver can be built as a module, it will be called "leds-sy7802".
> > +
> >  endif # LEDS_CLASS_FLASH
> > diff --git a/drivers/leds/flash/Makefile b/drivers/leds/flash/Makefile
> > index 91d60a4b7952..48860eeced79 100644
> > --- a/drivers/leds/flash/Makefile
> > +++ b/drivers/leds/flash/Makefile
> > @@ -11,3 +11,4 @@ obj-$(CONFIG_LEDS_QCOM_FLASH)	+= leds-qcom-flash.o
> >  obj-$(CONFIG_LEDS_RT4505)	+= leds-rt4505.o
> >  obj-$(CONFIG_LEDS_RT8515)	+= leds-rt8515.o
> >  obj-$(CONFIG_LEDS_SGM3140)	+= leds-sgm3140.o
> > +obj-$(CONFIG_LEDS_SY7802)	+= leds-sy7802.o
> > diff --git a/drivers/leds/flash/leds-sy7802.c b/drivers/leds/flash/leds-sy7802.c
> > new file mode 100644
> > index 000000000000..c03a571b0e08
> > --- /dev/null
> > +++ b/drivers/leds/flash/leds-sy7802.c
> > @@ -0,0 +1,532 @@

[...]

> > +static int sy7802_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
> 
> s/level/brightness/
> 
> > +{
> > +	struct sy7802_led *led = container_of(lcdev, struct sy7802_led, flash.led_cdev);
> > +	u32 led_enable_mask = led->led_no == SY7802_LED_JOINT ? SY7802_LEDS_MASK_ALL :
> > +			      SY7802_LEDS_MASK(led->led_no);
> 
> Do all of the fancy multi-line assignment outside of the declaration block.
> 
> > +	u32 enable_mask = SY7802_MODE_MASK | led_enable_mask;
> > +	u32 val = level ? led_enable_mask : SY7802_MODE_OFF;
> > +	struct sy7802 *chip = led->chip;
> > +	u32 curr;
> 
> This is a temporary placeholder for fled_torch_used, right?
> 
> fled_torch_used_tmp?  Sometimes abbreviated to tmp.
> 
> > +	u32 mask;
> 
> That's a lot of masks.  Which one is this?
> 
> > +	int ret;
> > +
> > +	mutex_lock(&chip->mutex);
> > +
> > +	/*
> > +	 * There is only one set of flash control logic, and this flag is used to check if 'strobe'
> 
> The ',' before 'and' is superfluous.
> 
> > +	 * is currently being used.
> > +	 */
> 
> Doesn't the variable name kind of imply this?
> 
> > +	if (chip->fled_strobe_used) {
> > +		dev_warn(chip->dev, "Please disable strobe first [%d]\n", chip->fled_strobe_used);
> 
> "Cannot set torch brightness whilst strobe is enabled"
> 
> > +		ret = -EBUSY;
> > +		goto unlock;
> > +	}
> > +
> > +	if (level)
> > +		curr = chip->fled_torch_used | BIT(led->led_no);
> > +	else
> > +		curr = chip->fled_torch_used & ~BIT(led->led_no);
> > +
> > +	if (curr)
> > +		val |= SY7802_MODE_TORCH;
> > +
> > +	/* Torch needs to be disabled first to apply new brightness */
> 
> "Disable touch to apply brightness"
> 
> > +	ret = regmap_update_bits(chip->regmap, SY7802_REG_ENABLE, SY7802_MODE_MASK,
> > +				 SY7802_MODE_OFF);
> > +	if (ret)
> > +		goto unlock;
> > +
> > +	mask = led->led_no == SY7802_LED_JOINT ? SY7802_TORCH_CURRENT_MASK_ALL :
> 
> Why not just use led->led_no in place of mask?

mask and led->led_no are assigned the same value from this point on.

> Easier to read if you drop SY7802_TORCH_CURRENT_MASK_ALL to its own line.
> 
> > +	       SY7802_TORCH_CURRENT_MASK(led->led_no);
> > +
> > +	/* Register expects brightness between 0 and MAX_BRIGHTNESS - 1 */
> > +	if (level)
> > +		level -= 1;
> > +
> > +	level |= (level << SY7802_TORCH_CURRENT_SHIFT);
> > +
> > +	ret = regmap_update_bits(chip->regmap, SY7802_REG_TORCH_BRIGHTNESS, mask, level);

So why not kill the single-use 'mask' variable and use a cast version of
led->led_no here instead?

> > +	if (ret)
> > +		goto unlock;
> > +
> > +	ret = regmap_update_bits(chip->regmap, SY7802_REG_ENABLE, enable_mask, val);
> > +	if (ret)
> > +		goto unlock;
> > +
> > +	chip->fled_torch_used = curr;
> > +
> > +unlock:
> > +	mutex_unlock(&chip->mutex);
> > +	return ret;
> > +}

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 2/3] leds: sy7802: Add support for Silergy SY7802 flash LED controller

[André Apitzsch]

Am Freitag, dem 03.05.2024 um 08:19 +0100 schrieb Lee Jones:
> On Thu, 11 Apr 2024, Lee Jones wrote:
> 
> > On Mon, 01 Apr 2024, André Apitzsch via B4 Relay wrote:
> > 
> > > From: André Apitzsch <git@apitzsch.eu>
> > > 
> > > Add support for SY7802 flash LED controller. It can support up to
> > > 1.8A
> > > flash current.
> > 
> > This is a very small commit message for a 500+ line change!
> > 
> > Please, tell us more.
> > 
> > > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > > ---
> > >  drivers/leds/flash/Kconfig       |  11 +
> > >  drivers/leds/flash/Makefile      |   1 +
> > >  drivers/leds/flash/leds-sy7802.c | 532
> > > +++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 544 insertions(+)
> > > 
> > > diff --git a/drivers/leds/flash/Kconfig
> > > b/drivers/leds/flash/Kconfig
> > > index 809b6d98bb3e..f39f0bfe6eef 100644
> > > --- a/drivers/leds/flash/Kconfig
> > > +++ b/drivers/leds/flash/Kconfig
> > > @@ -121,4 +121,15 @@ config LEDS_SGM3140
> > >  	  This option enables support for the SGM3140 500mA
> > > Buck/Boost Charge
> > >  	  Pump LED Driver.
> > >  
> > > +config LEDS_SY7802
> > > +	tristate "LED support for the Silergy SY7802"
> > > +	depends on I2C && OF
> > > +	depends on GPIOLIB
> > > +	select REGMAP_I2C
> > > +	help
> > > +	  This option enables support for the SY7802 flash LED
> > > controller.
> > > +	  SY7802 includes torch and flash functions with
> > > programmable current.
> > > +
> > > +	  This driver can be built as a module, it will be
> > > called "leds-sy7802".
> > > +
> > >  endif # LEDS_CLASS_FLASH
> > > diff --git a/drivers/leds/flash/Makefile
> > > b/drivers/leds/flash/Makefile
> > > index 91d60a4b7952..48860eeced79 100644
> > > --- a/drivers/leds/flash/Makefile
> > > +++ b/drivers/leds/flash/Makefile
> > > @@ -11,3 +11,4 @@ obj-$(CONFIG_LEDS_QCOM_FLASH)	+= leds-qcom-
> > > flash.o
> > >  obj-$(CONFIG_LEDS_RT4505)	+= leds-rt4505.o
> > >  obj-$(CONFIG_LEDS_RT8515)	+= leds-rt8515.o
> > >  obj-$(CONFIG_LEDS_SGM3140)	+= leds-sgm3140.o
> > > +obj-$(CONFIG_LEDS_SY7802)	+= leds-sy7802.o
> > > diff --git a/drivers/leds/flash/leds-sy7802.c
> > > b/drivers/leds/flash/leds-sy7802.c
> > > new file mode 100644
> > > index 000000000000..c03a571b0e08
> > > --- /dev/null
> > > +++ b/drivers/leds/flash/leds-sy7802.c
> > > @@ -0,0 +1,532 @@
> 
> [...]
> 
> > > +static int sy7802_torch_brightness_set(struct led_classdev
> > > *lcdev, enum led_brightness level)
> > 
> > s/level/brightness/
> > 
> > > +{
> > > +	struct sy7802_led *led = container_of(lcdev, struct
> > > sy7802_led, flash.led_cdev);
> > > +	u32 led_enable_mask = led->led_no == SY7802_LED_JOINT ?
> > > SY7802_LEDS_MASK_ALL :
> > > +			      SY7802_LEDS_MASK(led->led_no);
> > 
> > Do all of the fancy multi-line assignment outside of the
> > declaration block.
> > 
> > > +	u32 enable_mask = SY7802_MODE_MASK | led_enable_mask;
> > > +	u32 val = level ? led_enable_mask : SY7802_MODE_OFF;
> > > +	struct sy7802 *chip = led->chip;
> > > +	u32 curr;
> > 
> > This is a temporary placeholder for fled_torch_used, right?
> > 
> > fled_torch_used_tmp?  Sometimes abbreviated to tmp.
> > 
> > > +	u32 mask;
> > 
> > That's a lot of masks.  Which one is this?
> > 
> > > +	int ret;
> > > +
> > > +	mutex_lock(&chip->mutex);
> > > +
> > > +	/*
> > > +	 * There is only one set of flash control logic, and
> > > this flag is used to check if 'strobe'
> > 
> > The ',' before 'and' is superfluous.
> > 
> > > +	 * is currently being used.
> > > +	 */
> > 
> > Doesn't the variable name kind of imply this?
> > 
> > > +	if (chip->fled_strobe_used) {
> > > +		dev_warn(chip->dev, "Please disable strobe first
> > > [%d]\n", chip->fled_strobe_used);
> > 
> > "Cannot set torch brightness whilst strobe is enabled"
> > 
> > > +		ret = -EBUSY;
> > > +		goto unlock;
> > > +	}
> > > +
> > > +	if (level)
> > > +		curr = chip->fled_torch_used | BIT(led->led_no);
> > > +	else
> > > +		curr = chip->fled_torch_used & ~BIT(led-
> > > >led_no);
> > > +
> > > +	if (curr)
> > > +		val |= SY7802_MODE_TORCH;
> > > +
> > > +	/* Torch needs to be disabled first to apply new
> > > brightness */
> > 
> > "Disable touch to apply brightness"
> > 
> > > +	ret = regmap_update_bits(chip->regmap,
> > > SY7802_REG_ENABLE, SY7802_MODE_MASK,
> > > +				 SY7802_MODE_OFF);
> > > +	if (ret)
> > > +		goto unlock;
> > > +
> > > +	mask = led->led_no == SY7802_LED_JOINT ?
> > > SY7802_TORCH_CURRENT_MASK_ALL :
> > 
> > Why not just use led->led_no in place of mask?
> 
> mask and led->led_no are assigned the same value from this point on.

Thanks for the clarification.
How to you come to the conclusion that mask and led->led_no are
assigned the same value from this point on?

The value of led->led_no is used here only as part of the if condition
(led->led_no == SY7802_LED_JOINT) and not assigned to mask.
> 
> > Easier to read if you drop SY7802_TORCH_CURRENT_MASK_ALL to its own
> > line.
> > 
> > > +	       SY7802_TORCH_CURRENT_MASK(led->led_no);
> > > +
> > > +	/* Register expects brightness between 0 and
> > > MAX_BRIGHTNESS - 1 */
> > > +	if (level)
> > > +		level -= 1;
> > > +
> > > +	level |= (level << SY7802_TORCH_CURRENT_SHIFT);
> > > +
> > > +	ret = regmap_update_bits(chip->regmap,
> > > SY7802_REG_TORCH_BRIGHTNESS, mask, level);
> 
> So why not kill the single-use 'mask' variable and use a cast version
> of led->led_no here instead?
> 
> > > +	if (ret)
> > > +		goto unlock;
> > > +
> > > +	ret = regmap_update_bits(chip->regmap,
> > > SY7802_REG_ENABLE, enable_mask, val);
> > > +	if (ret)
> > > +		goto unlock;
> > > +
> > > +	chip->fled_torch_used = curr;
> > > +
> > > +unlock:
> > > +	mutex_unlock(&chip->mutex);
> > > +	return ret;
> > > +}
> 

Re: [PATCH v2 2/3] leds: sy7802: Add support for Silergy SY7802 flash LED controller

[Lee Jones]

On Sat, 04 May 2024, André Apitzsch wrote:

> Am Freitag, dem 03.05.2024 um 08:19 +0100 schrieb Lee Jones:
> > On Thu, 11 Apr 2024, Lee Jones wrote:
> > 
> > > On Mon, 01 Apr 2024, André Apitzsch via B4 Relay wrote:
> > > 
> > > > From: André Apitzsch <git@apitzsch.eu>
> > > > 
> > > > Add support for SY7802 flash LED controller. It can support up to
> > > > 1.8A
> > > > flash current.
> > > 
> > > This is a very small commit message for a 500+ line change!
> > > 
> > > Please, tell us more.
> > > 
> > > > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > > > ---
> > > >  drivers/leds/flash/Kconfig       |  11 +
> > > >  drivers/leds/flash/Makefile      |   1 +
> > > >  drivers/leds/flash/leds-sy7802.c | 532
> > > > +++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 544 insertions(+)
> > > > 
> > > > diff --git a/drivers/leds/flash/Kconfig
> > > > b/drivers/leds/flash/Kconfig
> > > > index 809b6d98bb3e..f39f0bfe6eef 100644
> > > > --- a/drivers/leds/flash/Kconfig
> > > > +++ b/drivers/leds/flash/Kconfig
> > > > @@ -121,4 +121,15 @@ config LEDS_SGM3140
> > > >  	  This option enables support for the SGM3140 500mA
> > > > Buck/Boost Charge
> > > >  	  Pump LED Driver.
> > > >  
> > > > +config LEDS_SY7802
> > > > +	tristate "LED support for the Silergy SY7802"
> > > > +	depends on I2C && OF
> > > > +	depends on GPIOLIB
> > > > +	select REGMAP_I2C
> > > > +	help
> > > > +	  This option enables support for the SY7802 flash LED
> > > > controller.
> > > > +	  SY7802 includes torch and flash functions with
> > > > programmable current.
> > > > +
> > > > +	  This driver can be built as a module, it will be
> > > > called "leds-sy7802".
> > > > +
> > > >  endif # LEDS_CLASS_FLASH
> > > > diff --git a/drivers/leds/flash/Makefile
> > > > b/drivers/leds/flash/Makefile
> > > > index 91d60a4b7952..48860eeced79 100644
> > > > --- a/drivers/leds/flash/Makefile
> > > > +++ b/drivers/leds/flash/Makefile
> > > > @@ -11,3 +11,4 @@ obj-$(CONFIG_LEDS_QCOM_FLASH)	+= leds-qcom-
> > > > flash.o
> > > >  obj-$(CONFIG_LEDS_RT4505)	+= leds-rt4505.o
> > > >  obj-$(CONFIG_LEDS_RT8515)	+= leds-rt8515.o
> > > >  obj-$(CONFIG_LEDS_SGM3140)	+= leds-sgm3140.o
> > > > +obj-$(CONFIG_LEDS_SY7802)	+= leds-sy7802.o
> > > > diff --git a/drivers/leds/flash/leds-sy7802.c
> > > > b/drivers/leds/flash/leds-sy7802.c
> > > > new file mode 100644
> > > > index 000000000000..c03a571b0e08
> > > > --- /dev/null
> > > > +++ b/drivers/leds/flash/leds-sy7802.c
> > > > @@ -0,0 +1,532 @@
> > 
> > [...]
> > 
> > > > +static int sy7802_torch_brightness_set(struct led_classdev
> > > > *lcdev, enum led_brightness level)
> > > 
> > > s/level/brightness/
> > > 
> > > > +{
> > > > +	struct sy7802_led *led = container_of(lcdev, struct
> > > > sy7802_led, flash.led_cdev);
> > > > +	u32 led_enable_mask = led->led_no == SY7802_LED_JOINT ?
> > > > SY7802_LEDS_MASK_ALL :
> > > > +			      SY7802_LEDS_MASK(led->led_no);
> > > 
> > > Do all of the fancy multi-line assignment outside of the
> > > declaration block.
> > > 
> > > > +	u32 enable_mask = SY7802_MODE_MASK | led_enable_mask;
> > > > +	u32 val = level ? led_enable_mask : SY7802_MODE_OFF;
> > > > +	struct sy7802 *chip = led->chip;
> > > > +	u32 curr;
> > > 
> > > This is a temporary placeholder for fled_torch_used, right?
> > > 
> > > fled_torch_used_tmp?  Sometimes abbreviated to tmp.
> > > 
> > > > +	u32 mask;
> > > 
> > > That's a lot of masks.  Which one is this?
> > > 
> > > > +	int ret;
> > > > +
> > > > +	mutex_lock(&chip->mutex);
> > > > +
> > > > +	/*
> > > > +	 * There is only one set of flash control logic, and
> > > > this flag is used to check if 'strobe'
> > > 
> > > The ',' before 'and' is superfluous.
> > > 
> > > > +	 * is currently being used.
> > > > +	 */
> > > 
> > > Doesn't the variable name kind of imply this?
> > > 
> > > > +	if (chip->fled_strobe_used) {
> > > > +		dev_warn(chip->dev, "Please disable strobe first
> > > > [%d]\n", chip->fled_strobe_used);
> > > 
> > > "Cannot set torch brightness whilst strobe is enabled"
> > > 
> > > > +		ret = -EBUSY;
> > > > +		goto unlock;
> > > > +	}
> > > > +
> > > > +	if (level)
> > > > +		curr = chip->fled_torch_used | BIT(led->led_no);
> > > > +	else
> > > > +		curr = chip->fled_torch_used & ~BIT(led-
> > > > >led_no);
> > > > +
> > > > +	if (curr)
> > > > +		val |= SY7802_MODE_TORCH;
> > > > +
> > > > +	/* Torch needs to be disabled first to apply new
> > > > brightness */
> > > 
> > > "Disable touch to apply brightness"
> > > 
> > > > +	ret = regmap_update_bits(chip->regmap,
> > > > SY7802_REG_ENABLE, SY7802_MODE_MASK,
> > > > +				 SY7802_MODE_OFF);
> > > > +	if (ret)
> > > > +		goto unlock;
> > > > +
> > > > +	mask = led->led_no == SY7802_LED_JOINT ?
> > > > SY7802_TORCH_CURRENT_MASK_ALL :
> > > 
> > > Why not just use led->led_no in place of mask?
> > 
> > mask and led->led_no are assigned the same value from this point on.
> 
> Thanks for the clarification.
> How to you come to the conclusion that mask and led->led_no are
> assigned the same value from this point on?

Because I am blind and didn't see the double equals ("==").

#shouldhavegonetospecsavers

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 2/3] leds: sy7802: Add support for Silergy SY7802 flash LED controller

[Christophe JAILLET]

Le 01/04/2024 à 23:23, André Apitzsch via B4 Relay a écrit :
> From: André Apitzsch <git@apitzsch.eu>
> 
> Add support for SY7802 flash LED controller. It can support up to 1.8A
> flash current.
> 
> Signed-off-by: André Apitzsch <git@apitzsch.eu>
> ---
>   drivers/leds/flash/Kconfig       |  11 +
>   drivers/leds/flash/Makefile      |   1 +
>   drivers/leds/flash/leds-sy7802.c | 532 +++++++++++++++++++++++++++++++++++++++
>   3 files changed, 544 insertions(+)

...

> +static int sy7802_led_register(struct device *dev, struct sy7802_led *led,
> +			       struct device_node *np)
> +{
> +	struct led_init_data init_data = {};
> +	int ret;
> +
> +	init_data.fwnode = of_fwnode_handle(np);
> +
> +	ret = devm_led_classdev_flash_register_ext(dev, &led->flash, &init_data);
> +	if (ret) {
> +		dev_err(dev, "Couldn't register flash %d\n", led->led_no);
> +		return ret;
> +	}
> +
> +	return ret;

Hi,

Nitpick: return 0;

CJ

> +}

[PATCH v2 3/3] arm64: dts: qcom: msm8939-longcheer-l9100: Add rear flash

[André Apitzsch via B4 Relay]

From: André Apitzsch <git@apitzsch.eu>

The phone has a Silergy SY7802 flash LED controller.

Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
 .../boot/dts/qcom/msm8939-longcheer-l9100.dts      | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8939-longcheer-l9100.dts b/arch/arm64/boot/dts/qcom/msm8939-longcheer-l9100.dts
index e3404c4455cf..528737929274 100644
--- a/arch/arm64/boot/dts/qcom/msm8939-longcheer-l9100.dts
+++ b/arch/arm64/boot/dts/qcom/msm8939-longcheer-l9100.dts
@@ -159,6 +159,25 @@ led@2 {
 			};
 		};
 	};
+
+	flash-led-controller@53 {
+		compatible = "silergy,sy7802";
+		reg = <0x53>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		enable-gpios = <&tlmm 16 GPIO_ACTIVE_HIGH>;
+
+		pinctrl-0 = <&camera_rear_flash_default>;
+		pinctrl-names = "default";
+
+		led@0 {
+			reg = <0>;
+			function = LED_FUNCTION_FLASH;
+			color = <LED_COLOR_ID_WHITE>;
+			led-sources = <0>, <1>;
+		};
+	};
 };
 
 &blsp_i2c3 {
@@ -318,6 +337,13 @@ camera_front_flash_default: camera-front-flash-default-state {
 		bias-disable;
 	};
 
+	camera_rear_flash_default: camera-rear-flash-default-state {
+		pins = "gpio9", "gpio16", "gpio51";
+		function = "gpio";
+		drive-strength = <2>;
+		bias-disable;
+	};
+
 	gpio_hall_sensor_default: gpio-hall-sensor-default-state {
 		pins = "gpio20";
 		function = "gpio";

-- 
2.44.0

Re: [PATCH v2 0/3] Add sy7802 flash led driver

[Trilok Soni]

On 4/1/2024 2:23 PM, André Apitzsch via B4 Relay wrote:
> This series introduces a driver for the Silergy SY7802 charge pump used
> in the BQ Aquaris M5 and X5 smartphones.
> 
> The implementation is based on information extracted from downstream as
> the datasheet provided by a distributor of the hardware didn't include
> any information about the i2c register description.
> 
> Signed-off-by: André Apitzsch <git@apitzsch.eu>


Is this the right email address? "From" shows devnull+git.apitzsch.eu@kernel.org. 

> ---
> Changes in v2:
> - bindings: remove unneeded allOf
> - bindings: example: move flash-led-controller under i2c node to fix
>   check error
> - Cc to phone-devel
> - Link to v1: https://lore.kernel.org/r/20240327-sy7802-v1-0-db74ab32faaf@apitzsch.eu
> 
> ---
> André Apitzsch (3):
>       dt-bindings: leds: Add Silergy SY7802 flash LED
>       leds: sy7802: Add support for Silergy SY7802 flash LED controller
>       arm64: dts: qcom: msm8939-longcheer-l9100: Add rear flash
> 
>  .../devicetree/bindings/leds/silergy,sy7802.yaml   | 100 ++++
>  .../boot/dts/qcom/msm8939-longcheer-l9100.dts      |  26 +
>  drivers/leds/flash/Kconfig                         |  11 +
>  drivers/leds/flash/Makefile                        |   1 +
>  drivers/leds/flash/leds-sy7802.c                   | 532 +++++++++++++++++++++
>  5 files changed, 670 insertions(+)
> ---
> base-commit: a6bd6c9333397f5a0e2667d4d82fef8c970108f2
> change-id: 20240325-sy7802-f40fc6f56525
> 
> Best regards,
-- 
---Trilok Soni

Re: [PATCH v2 0/3] Add sy7802 flash led driver

[André Apitzsch]

Am Montag, dem 01.04.2024 um 14:45 -0700 schrieb Trilok Soni:
> On 4/1/2024 2:23 PM, André Apitzsch via B4 Relay wrote:
> > This series introduces a driver for the Silergy SY7802 charge pump
> > used
> > in the BQ Aquaris M5 and X5 smartphones.
> > 
> > The implementation is based on information extracted from
> > downstream as
> > the datasheet provided by a distributor of the hardware didn't
> > include
> > any information about the i2c register description.
> > 
> > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> 
> 
> Is this the right email address? "From" shows
> devnull+git.apitzsch.eu@kernel.org. 
> 
This was created by b4 using the web submission endpoint.

André

> > ---
> > Changes in v2:
> > - bindings: remove unneeded allOf
> > - bindings: example: move flash-led-controller under i2c node to
> > fix
> >   check error
> > - Cc to phone-devel
> > - Link to v1:
> > https://lore.kernel.org/r/20240327-sy7802-v1-0-db74ab32faaf@apitzsch.eu
> > 
> > ---
> > André Apitzsch (3):
> >       dt-bindings: leds: Add Silergy SY7802 flash LED
> >       leds: sy7802: Add support for Silergy SY7802 flash LED
> > controller
> >       arm64: dts: qcom: msm8939-longcheer-l9100: Add rear flash
> > 
> >  .../devicetree/bindings/leds/silergy,sy7802.yaml   | 100 ++++
> >  .../boot/dts/qcom/msm8939-longcheer-l9100.dts      |  26 +
> >  drivers/leds/flash/Kconfig                         |  11 +
> >  drivers/leds/flash/Makefile                        |   1 +
> >  drivers/leds/flash/leds-sy7802.c                   | 532
> > +++++++++++++++++++++
> >  5 files changed, 670 insertions(+)
> > ---
> > base-commit: a6bd6c9333397f5a0e2667d4d82fef8c970108f2
> > change-id: 20240325-sy7802-f40fc6f56525
> > 
> > Best regards,