* [PATCH v2 0/2] Add LED driver for flash module in QCOM PMICs
@ 2022-09-29 12:15 Fenglin Wu
2022-09-29 12:15 ` [PATCH v2 1/2] leds: flash: add driver to support flash LED " Fenglin Wu
2022-09-29 12:15 ` [PATCH v2 2/2] dt-bindings: add bindings for QCOM flash LED Fenglin Wu
0 siblings, 2 replies; 11+ messages in thread
From: Fenglin Wu @ 2022-09-29 12:15 UTC (permalink / raw)
To: linux-arm-msm, linux-kernel, krzysztof.kozlowski
Cc: collinsd, subbaram, quic_fenglinw
Initial driver and binding document changes for supporting flash LED
module in Qualcomm Technologies, Inc. PMICs.
Changes in V2:
1. Addressed review comments in binding change, thanks Krzysztof!
2. Updated driver to address the compilation issue reported by
kernel test robot.
Fenglin Wu (2):
leds: flash: add driver to support flash LED module in QCOM PMICs
dt-bindings: add bindings for QCOM flash LED
.../bindings/leds/qcom,spmi-flash-led.yaml | 120 +++
drivers/leds/flash/Kconfig | 14 +
drivers/leds/flash/Makefile | 1 +
drivers/leds/flash/leds-qcom-flash.c | 707 ++++++++++++++++++
4 files changed, 842 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/qcom,spmi-flash-led.yaml
create mode 100644 drivers/leds/flash/leds-qcom-flash.c
--
2.25.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] leds: flash: add driver to support flash LED module in QCOM PMICs
2022-09-29 12:15 [PATCH v2 0/2] Add LED driver for flash module in QCOM PMICs Fenglin Wu
@ 2022-09-29 12:15 ` Fenglin Wu
2022-09-29 12:23 ` Krzysztof Kozlowski
2022-09-29 12:15 ` [PATCH v2 2/2] dt-bindings: add bindings for QCOM flash LED Fenglin Wu
1 sibling, 1 reply; 11+ messages in thread
From: Fenglin Wu @ 2022-09-29 12:15 UTC (permalink / raw)
To: linux-arm-msm, linux-kernel, krzysztof.kozlowski, Pavel Machek,
Gene Chen, Jacek Anaszewski, Fenglin Wu, linux-leds
Cc: collinsd, subbaram
Add initial driver to support flash LED module found in Qualcomm
Technologies, Inc. PMICs. The flash module can have 3 or 4 channels
and each channel can be controlled indepedently and support full scale
current up to 1.5 A. It also supports connecting two channels together
to supply one LED component with full scale current up to 2 A. In that
case, the current will be split on each channel symmetrically and the
channels will be enabled and disabled at the same time.
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
drivers/leds/flash/Kconfig | 14 +
drivers/leds/flash/Makefile | 1 +
drivers/leds/flash/leds-qcom-flash.c | 707 +++++++++++++++++++++++++++
3 files changed, 722 insertions(+)
create mode 100644 drivers/leds/flash/leds-qcom-flash.c
diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
index d3eb689b193c..92773fa872dc 100644
--- a/drivers/leds/flash/Kconfig
+++ b/drivers/leds/flash/Kconfig
@@ -61,6 +61,20 @@ config LEDS_MT6360
Independent current sources supply for each flash LED support torch
and strobe mode.
+config LEDS_QCOM_FLASH
+ tristate "LED support for flash module inside Qualcomm Technologies, Inc. PMIC"
+ depends on MFD_SPMI_PMIC
+ depends on LEDS_CLASS && OF
+ depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
+ help
+ This option enables support for the flash module found in Qualcomm
+ Technologies, Inc. PMICs. The flash module can have 3 or 4 flash LED
+ channels and each channel is programmable to support up to 1.5 A full
+ scale current. It also supports connecting two channels' output together
+ to supply one LED component to achieve current up to 2 A. In such case,
+ the total LED current will be split symmetrically on each channel and
+ they will be enabled/disabled at the same time.
+
config LEDS_RT4505
tristate "LED support for RT4505 flashlight controller"
depends on I2C && OF
diff --git a/drivers/leds/flash/Makefile b/drivers/leds/flash/Makefile
index 0acbddc0b91b..8a60993f1a25 100644
--- a/drivers/leds/flash/Makefile
+++ b/drivers/leds/flash/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_LEDS_AS3645A) += leds-as3645a.o
obj-$(CONFIG_LEDS_KTD2692) += leds-ktd2692.o
obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o
obj-$(CONFIG_LEDS_MAX77693) += leds-max77693.o
+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
diff --git a/drivers/leds/flash/leds-qcom-flash.c b/drivers/leds/flash/leds-qcom-flash.c
new file mode 100644
index 000000000000..7b941eb769fe
--- /dev/null
+++ b/drivers/leds/flash/leds-qcom-flash.c
@@ -0,0 +1,707 @@
+//SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/leds.h>
+#include <linux/led-class-flash.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <media/v4l2-flash-led-class.h>
+
+/* registers definitions */
+#define FLASH_TYPE_REG 0x04
+#define FLASH_TYPE_VAL 0x18
+
+#define FLASH_SUBTYPE_REG 0x05
+#define FLASH_SUBTYPE_3CH_VAL 0x04
+#define FLASH_SUBTYPE_4CH_VAL 0x07
+
+#define FLASH_MODULE_EN_BIT BIT(7)
+
+#define FLASH_TIMER_EN_BIT BIT(7)
+#define FLASH_TIMER_VAL_MASK GENMASK(6, 0)
+#define FLASH_TIMER_STEP_MS 10
+
+#define FLASH_ITARGET_CURRENT_MASK GENMASK(6, 0)
+
+#define FLASH_STROBE_HW_SW_SEL_BIT BIT(2)
+#define SW_STROBE_VAL 0
+#define HW_STROBE_VAL 1
+#define FLASH_HW_STROBE_TRIGGER_SEL_BIT BIT(1)
+#define STROBE_LEVEL_TRIGGER_VAL 0
+#define STROBE_EDGE_TRIGGER_VAL 1
+#define FLASH_STROBE_POLARITY_BIT BIT(0)
+#define STROBE_ACTIVE_HIGH_VAL 1
+
+#define FLASH_IRES_MASK_4CH BIT(0)
+#define FLASH_IRES_MASK_3CH GENMASK(1, 0)
+#define FLASH_IRES_12P5MA_VAL 0
+#define FLASH_IRES_5MA_VAL_4CH 1
+#define FLASH_IRES_5MA_VAL_3CH 3
+
+/* constants */
+#define FLASH_CURRENT_MAX_UA 1500000
+#define TORCH_CURRENT_MAX_UA 500000
+#define FLASH_TOTAL_CURRENT_MAX_UA 2000000
+#define FLASH_CURRENT_DEFAULT_UA 1000000
+#define TORCH_CURRENT_DEFAULT_UA 200000
+
+#define TORCH_IRES_UA 5000
+#define FLASH_IRES_UA 12500
+
+#define FLASH_TIMEOUT_MAX_US 1280000
+#define FLASH_TIMEOUT_STEP_US 10000
+
+enum hw_type {
+ QCOM_MVFLASH_3CH,
+ QCOM_MVFLASH_4CH,
+};
+
+enum led_mode {
+ FLASH_MODE,
+ TORCH_MODE,
+};
+
+enum led_strobe {
+ SW_STROBE,
+ HW_STROBE,
+};
+
+struct qcom_flash_reg {
+ u8 module_en;
+ u8 chan_timer;
+ u8 itarget;
+ u8 iresolution;
+ u8 chan_strobe;
+ u8 chan_en;
+ u8 status1;
+ u8 status2;
+ u8 status3;
+};
+
+struct qcom_flash_led {
+ struct qcom_flash_chip *chip;
+ struct led_classdev_flash flash;
+ struct v4l2_flash *v4l2_flash;
+ u32 max_flash_current_ma;
+ u32 max_torch_current_ma;
+ u32 max_timeout_ms;
+ u32 flash_current_ma;
+ u32 flash_timeout_ms;
+ u8 *chan_id;
+ u8 chan_count;
+ bool enabled;
+};
+
+struct qcom_flash_chip {
+ struct qcom_flash_led *leds;
+ const struct qcom_flash_reg *reg;
+ struct device *dev;
+ struct regmap *regmap;
+ struct mutex lock;
+ enum hw_type hw_type;
+ u32 reg_base;
+ u8 leds_count;
+ u8 max_channels;
+ u8 chan_en_bits;
+};
+
+static const struct qcom_flash_reg mvflash_3ch_reg = {
+ .chan_timer = 0x40,
+ .itarget = 0x43,
+ .module_en = 0x46,
+ .iresolution = 0x47,
+ .chan_strobe = 0x49,
+ .chan_en = 0x4c,
+ .status1 = 0x08,
+ .status2 = 0x09,
+ .status3 = 0x0a,
+};
+
+static const struct qcom_flash_reg mvflash_4ch_reg = {
+ .chan_timer = 0x3e,
+ .itarget = 0x42,
+ .module_en = 0x46,
+ .iresolution = 0x49,
+ .chan_strobe = 0x4a,
+ .chan_en = 0x4e,
+ .status1 = 0x06,
+ .status2 = 0x07,
+ .status3 = 0x09,
+};
+
+static int __set_flash_module_en(struct qcom_flash_led *led, bool en)
+{
+ struct qcom_flash_chip *chip = led->chip;
+ u8 led_mask = 0, val;
+ int i, rc;
+
+ for (i = 0; i < led->chan_count; i++)
+ led_mask |= BIT(led->chan_id[i] - 1);
+
+ mutex_lock(&chip->lock);
+ if (en)
+ chip->chan_en_bits |= led_mask;
+ else
+ chip->chan_en_bits &= ~led_mask;
+
+ val = (!!chip->chan_en_bits) ? FLASH_MODULE_EN_BIT : 0;
+ rc = regmap_update_bits(chip->regmap, chip->reg_base + chip->reg->module_en,
+ FLASH_MODULE_EN_BIT, val);
+ mutex_unlock(&chip->lock);
+
+ return rc;
+}
+
+static int __set_flash_current(struct qcom_flash_led *led, u32 current_ma, enum led_mode mode)
+{
+ struct qcom_flash_chip *chip = led->chip;
+ u32 itarg_ua = current_ma * 1000 / led->chan_count + 1;
+ u32 ires_ua = (mode == FLASH_MODE) ? FLASH_IRES_UA : TORCH_IRES_UA;
+ u8 reg, val, shift, ires_mask = 0, ires_val = 0, chan_id;
+ int i, rc;
+
+ /*
+ * Split the current across the channels and set the
+ * IRESOLUTION and ITARGET registers accordingly.
+ */
+ for (i = 0; i < led->chan_count; i++) {
+ chan_id = led->chan_id[i];
+ if (itarg_ua < ires_ua)
+ val = 0;
+ else
+ val = itarg_ua / ires_ua - 1;
+
+ reg = chip->reg->itarget + chan_id - 1;
+ rc = regmap_update_bits(chip->regmap, chip->reg_base + reg,
+ FLASH_ITARGET_CURRENT_MASK, val);
+ if (rc < 0)
+ return rc;
+
+ if (chip->hw_type == QCOM_MVFLASH_3CH) {
+ shift = (chan_id - 1) * 2;
+ ires_mask |= FLASH_IRES_MASK_3CH << shift;
+ ires_val |= ((mode == FLASH_MODE) ?
+ (FLASH_IRES_12P5MA_VAL << shift) :
+ (FLASH_IRES_5MA_VAL_3CH << shift));
+ } else if (chip->hw_type == QCOM_MVFLASH_4CH) {
+ shift = chan_id - 1;
+ ires_mask |= FLASH_IRES_MASK_4CH << shift;
+ ires_val |= ((mode == FLASH_MODE) ?
+ (FLASH_IRES_12P5MA_VAL << shift) :
+ (FLASH_IRES_5MA_VAL_4CH << shift));
+ }
+ }
+
+ reg = chip->reg->iresolution;
+
+ return regmap_update_bits(chip->regmap, chip->reg_base + reg, ires_mask, ires_val);
+}
+
+static int __set_flash_timeout(struct qcom_flash_led *led, u32 timeout_ms)
+{
+ struct qcom_flash_chip *chip = led->chip;
+ u8 reg, mask, val, chan_id;
+ int rc, i;
+
+ /* set SAFETY_TIMER for all the channels connected to the same LED */
+ timeout_ms = min_t(u32, timeout_ms, led->max_timeout_ms);
+ for (i = 0; i < led->chan_count; i++) {
+ chan_id = led->chan_id[i];
+ mask = 0xff;
+ val = timeout_ms / FLASH_TIMER_STEP_MS;
+ val = clamp_t(u8, val, 0, FLASH_TIMER_VAL_MASK);
+ if (timeout_ms)
+ val |= FLASH_TIMER_EN_BIT;
+
+ reg = chip->reg->chan_timer + chan_id - 1;
+ rc = regmap_update_bits(chip->regmap, chip->reg_base + reg, mask, val);
+ if (rc < 0)
+ return rc;
+
+ }
+
+ return 0;
+}
+
+static int __set_flash_strobe(struct qcom_flash_led *led, enum led_strobe strobe, bool state)
+{
+ struct qcom_flash_chip *chip = led->chip;
+ u8 reg, mask, val, chan_id = 0, chan_mask = 0;
+ int rc, i;
+
+ /* Set SW strobe config for all channels connected to the LED */
+ for (i = 0; i < led->chan_count; i++) {
+ chan_id = led->chan_id[i];
+ mask = FLASH_STROBE_HW_SW_SEL_BIT | FLASH_HW_STROBE_TRIGGER_SEL_BIT
+ | FLASH_STROBE_POLARITY_BIT;
+ if (strobe == SW_STROBE)
+ val = FIELD_PREP(FLASH_STROBE_HW_SW_SEL_BIT, SW_STROBE_VAL);
+ else
+ val = FIELD_PREP(FLASH_STROBE_HW_SW_SEL_BIT, HW_STROBE_VAL);
+
+ val |= FIELD_PREP(FLASH_HW_STROBE_TRIGGER_SEL_BIT, STROBE_LEVEL_TRIGGER_VAL) |
+ FIELD_PREP(FLASH_STROBE_POLARITY_BIT, STROBE_ACTIVE_HIGH_VAL);
+ reg = chip->reg->chan_strobe + chan_id - 1;
+ rc = regmap_update_bits(chip->regmap, chip->reg_base + reg, mask, val);
+ if (rc < 0)
+ return rc;
+
+ chan_mask |= BIT(chan_id - 1);
+ }
+
+ /* enable/disable flash channels */
+ mask = chan_mask;
+ val = state ? mask : 0;
+ reg = chip->reg->chan_en;
+ rc = regmap_update_bits(chip->regmap, chip->reg_base + reg, mask, val);
+ if (rc < 0)
+ return rc;
+
+ led->enabled = state;
+ return 0;
+}
+
+static int qcom_flash_brightness_set(struct led_classdev_flash *fled_cdev, u32 brightness)
+{
+ struct qcom_flash_led *led = container_of(fled_cdev, struct qcom_flash_led, flash);
+
+ led->flash_current_ma = min_t(u32, led->max_flash_current_ma, brightness / 1000);
+ return 0;
+}
+
+static int qcom_flash_timeout_set(struct led_classdev_flash *fled_cdev, u32 timeout)
+{
+ struct qcom_flash_led *led = container_of(fled_cdev, struct qcom_flash_led, flash);
+
+ led->flash_timeout_ms = timeout / 1000;
+ return 0;
+}
+
+static int qcom_flash_strobe_set(struct led_classdev_flash *fled_cdev, bool state)
+{
+ struct qcom_flash_led *led = container_of(fled_cdev, struct qcom_flash_led, flash);
+ int rc;
+
+ rc = __set_flash_current(led, led->flash_current_ma, FLASH_MODE);
+ if (rc < 0)
+ return rc;
+
+ rc = __set_flash_timeout(led, led->flash_timeout_ms);
+ if (rc < 0)
+ return rc;
+
+ rc = __set_flash_module_en(led, state);
+ if (rc < 0)
+ return rc;
+
+ return __set_flash_strobe(led, SW_STROBE, state);
+}
+
+static int qcom_flash_strobe_get(struct led_classdev_flash *fled_cdev, bool *state)
+{
+ struct qcom_flash_led *led = container_of(fled_cdev, struct qcom_flash_led, flash);
+
+ *state = led->enabled;
+ return 0;
+}
+
+static int qcom_flash_fault_get(struct led_classdev_flash *fled_cdev, u32 *fault)
+{
+ struct qcom_flash_led *led = container_of(fled_cdev, struct qcom_flash_led, flash);
+ struct qcom_flash_chip *chip = led->chip;
+ u8 shift, chan_id = 0, chan_mask = 0;
+ u8 ot_mask = 0, oc_mask = 0, uv_mask = 0;
+ u32 val, fault_sts = 0;
+ int i, rc;
+
+ rc = regmap_read(chip->regmap, chip->reg_base + chip->reg->status1, &val);
+ if (rc < 0)
+ return rc;
+
+ for (i = 0; i < led->chan_count; i++) {
+ chan_id = led->chan_id[i];
+ shift = (chan_id - 1) * 2;
+ if (val & BIT(shift))
+ fault_sts |= LED_FAULT_SHORT_CIRCUIT;
+ chan_mask |= BIT(chan_id - 1);
+ }
+
+ rc = regmap_read(chip->regmap, chip->reg_base + chip->reg->status2, &val);
+ if (rc < 0)
+ return rc;
+
+ if (chip->hw_type == QCOM_MVFLASH_3CH) {
+ ot_mask = 0x0f;
+ oc_mask = 0xe0;
+ uv_mask = 0x10;
+ } else if (chip->hw_type == QCOM_MVFLASH_4CH) {
+ ot_mask = 0x70;
+ oc_mask = 0x0e;
+ uv_mask = 0x01;
+ }
+
+ if (val & ot_mask)
+ fault_sts |= LED_FAULT_OVER_TEMPERATURE;
+ if (val & oc_mask)
+ fault_sts |= LED_FAULT_OVER_CURRENT;
+ if (val & uv_mask)
+ fault_sts |= LED_FAULT_INPUT_VOLTAGE;
+
+ rc = regmap_read(chip->regmap, chip->reg_base + chip->reg->status3, &val);
+ if (rc < 0)
+ return rc;
+
+ if (chip->hw_type == QCOM_MVFLASH_3CH) {
+ if (val & chan_mask)
+ fault_sts |= LED_FAULT_TIMEOUT;
+ } else if (chip->hw_type == QCOM_MVFLASH_4CH) {
+ for (i = 0; i < led->chan_count; i++) {
+ chan_id = led->chan_id[i];
+ shift = (chan_id - 1) * 2;
+ if (val & BIT(shift))
+ fault_sts |= LED_FAULT_TIMEOUT;
+ }
+ }
+
+ *fault = fault_sts;
+ return 0;
+}
+
+static int qcom_flash_led_brightness_set(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ struct led_classdev_flash *fled_cdev =
+ container_of(led_cdev, struct led_classdev_flash, led_cdev);
+ struct qcom_flash_led *led =
+ container_of(fled_cdev, struct qcom_flash_led, flash);
+ u32 current_ma = brightness * led->max_torch_current_ma / LED_FULL;
+ bool enable = !!brightness;
+ int rc;
+
+ rc = __set_flash_current(led, current_ma, TORCH_MODE);
+ if (rc < 0)
+ return rc;
+
+ /* disable flash timeout for torch LED */
+ rc = __set_flash_timeout(led, 0);
+ if (rc < 0)
+ return rc;
+
+ rc = __set_flash_module_en(led, enable);
+ if (rc < 0)
+ return rc;
+
+ return __set_flash_strobe(led, SW_STROBE, enable);
+}
+
+static const struct led_flash_ops qcom_flash_ops = {
+ .flash_brightness_set = qcom_flash_brightness_set,
+ .strobe_set = qcom_flash_strobe_set,
+ .strobe_get = qcom_flash_strobe_get,
+ .timeout_set = qcom_flash_timeout_set,
+ .fault_get = qcom_flash_fault_get,
+};
+
+#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
+static int qcom_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
+{
+ struct led_classdev_flash *flash = v4l2_flash->fled_cdev;
+ struct qcom_flash_led *led = container_of(flash, struct qcom_flash_led, flash);
+ int rc;
+
+ rc = __set_flash_module_en(led, enable);
+ if (rc < 0)
+ return rc;
+
+ if (enable)
+ return __set_flash_strobe(led, HW_STROBE, true);
+ else
+ return __set_flash_strobe(led, SW_STROBE, false);
+}
+
+static enum led_brightness qcom_flash_intensity_to_led_brightness(
+ struct v4l2_flash *v4l2_flash, s32 intensity)
+{
+ struct led_classdev_flash *flash = v4l2_flash->fled_cdev;
+ struct qcom_flash_led *led = container_of(flash, struct qcom_flash_led, flash);
+ u32 current_ma = intensity / 1000;
+
+ current_ma = min_t(u32, current_ma, led->max_torch_current_ma);
+ if (!current_ma)
+ return LED_OFF;
+
+ return current_ma * LED_FULL / led->max_torch_current_ma;
+}
+
+static s32 qcom_flash_brightness_to_led_intensity(struct v4l2_flash *v4l2_flash,
+ enum led_brightness brightness)
+{
+ struct led_classdev_flash *flash = v4l2_flash->fled_cdev;
+ struct qcom_flash_led *led = container_of(flash, struct qcom_flash_led, flash);
+
+ return (brightness * led->max_torch_current_ma * 1000) / LED_FULL;
+}
+
+static const struct v4l2_flash_ops qcom_v4l2_flash_ops = {
+ .external_strobe_set = qcom_flash_external_strobe_set,
+ .intensity_to_led_brightness = qcom_flash_intensity_to_led_brightness,
+ .led_brightness_to_intensity = qcom_flash_brightness_to_led_intensity,
+};
+
+static int qcom_flash_v4l2_init(struct qcom_flash_led *led, struct fwnode_handle *fwnode)
+{
+ struct v4l2_flash_config v4l2_cfg = {0};
+ struct led_flash_setting *s = &v4l2_cfg.intensity;
+
+ if (!(led->flash.led_cdev.flags & LED_DEV_CAP_FLASH))
+ return 0;
+
+ s->min = s->step = TORCH_IRES_UA * led->chan_count;
+ s->max = led->max_torch_current_ma * 1000;
+ s->val = min_t(u32, s->max, TORCH_CURRENT_DEFAULT_UA);
+
+ strscpy(v4l2_cfg.dev_name, led->flash.led_cdev.dev->kobj.name,
+ sizeof(v4l2_cfg.dev_name));
+ v4l2_cfg.has_external_strobe = 1;
+ v4l2_cfg.flash_faults = LED_FAULT_INPUT_VOLTAGE | LED_FAULT_OVER_CURRENT |
+ LED_FAULT_SHORT_CIRCUIT | LED_FAULT_OVER_TEMPERATURE | LED_FAULT_TIMEOUT;
+
+ led->v4l2_flash = v4l2_flash_init(led->chip->dev, fwnode, &led->flash,
+ &qcom_v4l2_flash_ops, &v4l2_cfg);
+ return PTR_ERR_OR_ZERO(led->v4l2_flash);
+}
+# else
+static int qcom_flash_v4l2_init(struct qcom_flash_led *led, struct fwnode_handle *fwnode)
+{
+ return 0;
+}
+#endif
+
+static int qcom_flash_register_led_device(struct device *parent,
+ struct fwnode_handle *node, struct qcom_flash_led *led)
+{
+ struct qcom_flash_chip *chip = led->chip;
+ struct led_init_data init_data;
+ struct led_classdev_flash *flash;
+ struct led_flash_setting *s;
+ u32 count, val;
+ u32 channels[4];
+ int i, rc;
+
+ flash = &led->flash;
+ count = fwnode_property_count_u32(node, "led-sources");
+ if (count <= 0) {
+ dev_err(chip->dev, "No led-sources specified\n");
+ return -ENODEV;
+ }
+
+ if (count > chip->max_channels) {
+ dev_err(chip->dev, "led-sources count %u exceeds maximum channel count %u\n",
+ count, chip->max_channels);
+ return -EINVAL;
+ }
+
+ rc = fwnode_property_read_u32_array(node, "led-sources", channels, count);
+ if (rc < 0) {
+ dev_err(chip->dev, "get led-sources failed, rc=%d\n", rc);
+ return rc;
+ }
+
+ led->chan_count = count;
+ led->chan_id = devm_kcalloc(chip->dev, count, sizeof(u8), GFP_KERNEL);
+ if (!led->chan_id)
+ return -ENOMEM;
+
+ for (i = 0; i < count; i++) {
+ if (channels[i] > chip->max_channels) {
+ dev_err(chip->dev, "led-source out of HW support range [1-%u]\n",
+ chip->max_channels);
+ return -EINVAL;
+ }
+
+ led->chan_id[i] = channels[i];
+ }
+
+ rc = fwnode_property_read_u32(node, "led-max-microamp", &val);
+ if (rc < 0) {
+ dev_err(chip->dev, "Get led-max-microamp failed, rc=%d\n", rc);
+ return rc;
+ }
+
+ if (!val) {
+ dev_err(chip->dev, "led-max-microamp shouldn't be 0\n");
+ return -EINVAL;
+ }
+
+ val = min_t(u32, val, TORCH_CURRENT_MAX_UA * led->chan_count);
+ led->max_torch_current_ma = val / 1000;
+
+ if (fwnode_property_present(node, "flash-max-microamp")) {
+ flash->led_cdev.flags |= LED_DEV_CAP_FLASH;
+ rc = fwnode_property_read_u32(node, "flash-max-microamp", &val);
+ if (rc < 0) {
+ dev_err(chip->dev, "Get flash-max-microamp failed, rc=%d\n", rc);
+ return rc;
+ }
+
+ val = min_t(u32, val, FLASH_CURRENT_MAX_UA * led->chan_count);
+ val = min_t(u32, val, FLASH_TOTAL_CURRENT_MAX_UA);
+ s = &flash->brightness;
+ s->min = s->step = FLASH_IRES_UA * led->chan_count;
+ s->max = val;
+ s->val = min_t(u32, val, FLASH_CURRENT_DEFAULT_UA);
+ led->max_flash_current_ma = val / 1000;
+ led->flash_current_ma = s->val / 1000;
+
+ rc = fwnode_property_read_u32(node, "flash-max-timeout-us", &val);
+ if (rc < 0) {
+ dev_err(chip->dev, "Get flash-max-timeout-us failed, rc=%d\n", rc);
+ return rc;
+ }
+
+ val = min_t(u32, val, FLASH_TIMEOUT_MAX_US);
+ s = &flash->timeout;
+ s->min = s->step = FLASH_TIMEOUT_STEP_US;
+ s->val = s->max = val;
+ led->max_timeout_ms = led->flash_timeout_ms = val / 1000;
+
+ flash->ops = &qcom_flash_ops;
+ }
+
+ flash->led_cdev.brightness_set_blocking = qcom_flash_led_brightness_set;
+ init_data.fwnode = node;
+ init_data.devicename = NULL;
+ init_data.default_label = NULL;
+ init_data.devname_mandatory = false;
+ rc = devm_led_classdev_flash_register_ext(parent, flash, &init_data);
+ if (rc < 0) {
+ dev_err(chip->dev, "Register flash LED classdev failed, rc=%d\n", rc);
+ return rc;
+ }
+
+ return qcom_flash_v4l2_init(led, node);
+}
+
+static int qcom_flash_led_probe(struct platform_device *pdev)
+{
+ struct qcom_flash_chip *chip;
+ struct qcom_flash_led *led;
+ struct fwnode_handle *child;
+ struct device *dev = &pdev->dev;
+ int count, rc;
+ u32 val;
+
+ chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+
+ chip->regmap = dev_get_regmap(dev->parent, NULL);
+ if (!chip->regmap) {
+ dev_err(dev, "Failed to get parent regmap\n");
+ return -EINVAL;
+ }
+
+ rc = fwnode_property_read_u32(dev->fwnode, "reg", &chip->reg_base);
+ if (rc < 0) {
+ dev_err(dev, "Failed to get register base address, rc=%d\n", rc);
+ return rc;
+ }
+
+ rc = regmap_read(chip->regmap, chip->reg_base + FLASH_TYPE_REG, &val);
+ if (rc < 0) {
+ dev_err(dev, "Read flash module type failed, rc=%d\n", rc);
+ return rc;
+ }
+
+ if (val != FLASH_TYPE_VAL) {
+ dev_err(dev, "type %#x is not a flash module\n", val);
+ return -ENODEV;
+ }
+
+ rc = regmap_read(chip->regmap, chip->reg_base + FLASH_SUBTYPE_REG, &val);
+ if (rc < 0) {
+ dev_err(dev, "Read flash module subtype failed, rc=%d\n", rc);
+ return rc;
+ }
+
+ if (val == FLASH_SUBTYPE_3CH_VAL) {
+ chip->hw_type = QCOM_MVFLASH_3CH;
+ chip->max_channels = 3;
+ chip->reg = &mvflash_3ch_reg;
+ } else if (val == FLASH_SUBTYPE_4CH_VAL) {
+ chip->hw_type = QCOM_MVFLASH_4CH;
+ chip->max_channels = 4;
+ chip->reg = &mvflash_4ch_reg;
+ } else {
+ dev_err(dev, "flash subtype %#x is not yet supported\n", val);
+ return -ENODEV;
+ }
+
+ chip->dev = dev;
+ platform_set_drvdata(pdev, chip);
+ mutex_init(&chip->lock);
+ count = device_get_child_node_count(dev);
+ if (count == 0 || count > chip->max_channels) {
+ dev_err(dev, "No child or child count exceeds %d\n", chip->max_channels);
+ return -EINVAL;
+ }
+
+ chip->leds = devm_kcalloc(dev, count, sizeof(*chip->leds), GFP_KERNEL);
+ if (!chip->leds)
+ return -ENOMEM;
+
+ device_for_each_child_node(dev, child) {
+ led = &chip->leds[chip->leds_count];
+ led->chip = chip;
+ rc = qcom_flash_register_led_device(dev, child, led);
+ if (rc < 0)
+ goto release;
+
+ chip->leds_count++;
+ }
+
+ return 0;
+release:
+ while (chip->leds && chip->leds_count--)
+ v4l2_flash_release(chip->leds[chip->leds_count].v4l2_flash);
+ return rc;
+}
+
+static int qcom_flash_led_remove(struct platform_device *pdev)
+{
+ struct qcom_flash_chip *chip = platform_get_drvdata(pdev);
+
+ while (chip->leds_count--)
+ v4l2_flash_release(chip->leds[chip->leds_count].v4l2_flash);
+
+ mutex_destroy(&chip->lock);
+ return 0;
+}
+
+static const struct of_device_id qcom_flash_led_match_table[] = {
+ { .compatible = "qcom,spmi-flash-led" },
+ { .compatible = "qcom,pm8150c-flash-led" },
+ { .compatible = "qcom,pm8150l-flash-led" },
+ { .compatible = "qcom,pm8350c-flash-led" },
+ { }
+};
+
+static struct platform_driver qcom_flash_led_driver = {
+ .driver = {
+ .name = "leds-qcom-flash",
+ .of_match_table = qcom_flash_led_match_table,
+ },
+ .probe = qcom_flash_led_probe,
+ .remove = qcom_flash_led_remove,
+};
+
+module_platform_driver(qcom_flash_led_driver);
+
+MODULE_DESCRIPTION("QCOM Flash LED driver");
+MODULE_LICENSE("GPL");
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] dt-bindings: add bindings for QCOM flash LED
2022-09-29 12:15 [PATCH v2 0/2] Add LED driver for flash module in QCOM PMICs Fenglin Wu
2022-09-29 12:15 ` [PATCH v2 1/2] leds: flash: add driver to support flash LED " Fenglin Wu
@ 2022-09-29 12:15 ` Fenglin Wu
2022-09-29 12:40 ` Krzysztof Kozlowski
1 sibling, 1 reply; 11+ messages in thread
From: Fenglin Wu @ 2022-09-29 12:15 UTC (permalink / raw)
To: linux-arm-msm, linux-kernel, krzysztof.kozlowski, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Fenglin Wu, linux-leds, devicetree
Cc: collinsd, subbaram
Add binding document for flash LED module inside Qualcomm Technologies,
Inc. PMICs.
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
.../bindings/leds/qcom,spmi-flash-led.yaml | 120 ++++++++++++++++++
1 file changed, 120 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/qcom,spmi-flash-led.yaml
diff --git a/Documentation/devicetree/bindings/leds/qcom,spmi-flash-led.yaml b/Documentation/devicetree/bindings/leds/qcom,spmi-flash-led.yaml
new file mode 100644
index 000000000000..3ab1113a7b28
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/qcom,spmi-flash-led.yaml
@@ -0,0 +1,120 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/qcom,spmi-flash-led.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Flash LED device inside Qualcomm Technologies, Inc. PMICs
+
+maintainers:
+ - Fenglin Wu <quic_fenglinw@quicinc.com>
+
+description: |
+ Flash LED controller is present inside some Qualcomm Technologies, Inc. PMICs.
+ The flash LED module can have different number of LED channels supported
+ e.g. 3 or 4. There are some different registers between them but they can
+ both support maximum current up to 1.5 A per channel and they can also support
+ ganging 2 channels together to supply maximum current up to 2 A. The current
+ will be split symmetrically on each channel and they will be enabled and
+ disabled at the same time.
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - qcom,pm8150c-flash-led
+ - qcom,pm8150l-flash-led
+ - qcom,pm8350c-flash-led
+ - const: qcom,spmi-flash-led
+ reg:
+ description: address offset of the flash LED controller
+ maxItems: 1
+
+patternProperties:
+ "^led[0-3]$":
+ type: object
+ $ref: common.yaml#
+ unevaluatedProperties: false
+ description: |
+ Represents the physical LED components which are connected to the
+ flash LED channels' output.
+
+ properties:
+ led-sources:
+ description: |
+ The HW indices of the flash LED channels that connect to the
+ physical LED
+ allOf:
+ - minItems: 1
+ maxItems: 2
+ items:
+ enum: [1, 2, 3, 4]
+
+ led-max-microamp:
+ description: |
+ The maximum current value when LED is not operating in flash mode (i.e. torch mode)
+ Valid values when an LED is connected to one flash LED channel:
+ 5000 - 500000, step by 5000
+ Valid values when an LED is connected to two flash LED channels:
+ 10000 - 1000000, step by 10000
+ minimum: 5000
+ maximum: 1000000
+
+ flash-max-microamp:
+ description: |
+ The maximum current value when LED is operating in flash mode.
+ Valid values when an LED is connected to one flash LED channel:
+ 12500 - 1500000, step by 12500
+ Valid values when an LED is connected to two flash LED channels:
+ 25000 - 2000000, step by 12500
+ minimum: 12500
+ maximum: 2000000
+
+ flash-max-timeout-us:
+ description: |
+ The maximum timeout value when LED is operating in flash mode.
+ Valid values: 10000 - 1280000, step by 10000
+ minimum: 10000
+ maximum: 1280000
+
+ required:
+ - led-sources
+ - led-max-microamp
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/leds/common.h>
+ spmi_bus {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ led-controller@ee00 {
+ compatible = "qcom,pm8350c-flash-led", "qcom,spmi-flash-led";
+ reg = <0xee00>;
+
+ led0 {
+ function = LED_FUNCTION_FLASH;
+ color = <LED_COLOR_ID_WHITE>;
+ led-sources = <1>, <4>;
+ led-max-microamp = <300000>;
+ flash-max-microamp = <2000000>;
+ flash-max-timeout-us = <1280000>;
+ function-enumerator = <0>;
+ };
+
+ led1 {
+ function = LED_FUNCTION_FLASH;
+ color = <LED_COLOR_ID_YELLOW>;
+ led-sources = <2>, <3>;
+ led-max-microamp = <300000>;
+ flash-max-microamp = <2000000>;
+ flash-max-timeout-us = <1280000>;
+ function-enumerator = <1>;
+ };
+ };
+ };
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] leds: flash: add driver to support flash LED module in QCOM PMICs
2022-09-29 12:15 ` [PATCH v2 1/2] leds: flash: add driver to support flash LED " Fenglin Wu
@ 2022-09-29 12:23 ` Krzysztof Kozlowski
2022-10-10 10:00 ` Fenglin Wu
0 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Kozlowski @ 2022-09-29 12:23 UTC (permalink / raw)
To: Fenglin Wu, linux-arm-msm, linux-kernel, Pavel Machek, Gene Chen,
Jacek Anaszewski, linux-leds
Cc: collinsd, subbaram
On 29/09/2022 14:15, Fenglin Wu wrote:
> Add initial driver to support flash LED module found in Qualcomm
> Technologies, Inc. PMICs. The flash module can have 3 or 4 channels
> and each channel can be controlled indepedently and support full scale
> current up to 1.5 A. It also supports connecting two channels together
> to supply one LED component with full scale current up to 2 A. In that
> case, the current will be split on each channel symmetrically and the
> channels will be enabled and disabled at the same time.
>
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> ---
> drivers/leds/flash/Kconfig | 14 +
> drivers/leds/flash/Makefile | 1 +
> drivers/leds/flash/leds-qcom-flash.c | 707 +++++++++++++++++++++++++++
> 3 files changed, 722 insertions(+)
> create mode 100644 drivers/leds/flash/leds-qcom-flash.c
>
> diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
> index d3eb689b193c..92773fa872dc 100644
> --- a/drivers/leds/flash/Kconfig
> +++ b/drivers/leds/flash/Kconfig
> @@ -61,6 +61,20 @@ config LEDS_MT6360
> Independent current sources supply for each flash LED support torch
> and strobe mode.
>
> +config LEDS_QCOM_FLASH
> + tristate "LED support for flash module inside Qualcomm Technologies, Inc. PMIC"
> + depends on MFD_SPMI_PMIC
|| COMPILE_TEST
(and actually test it, e.g. you might need here "select REGMAP")
> + depends on LEDS_CLASS && OF
> + depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
> + help
> + This option enables support for the flash module found in Qualcomm
> + Technologies, Inc. PMICs. The flash module can have 3 or 4 flash LED
> + channels and each channel is programmable to support up to 1.5 A full
> + scale current. It also supports connecting two channels' output together
> + to supply one LED component to achieve current up to 2 A. In such case,
> + the total LED current will be split symmetrically on each channel and
> + they will be enabled/disabled at the same time.
> +
> config LEDS_RT4505
> tristate "LED support for RT4505 flashlight controller"
> depends on I2C && OF
> diff --git a/drivers/leds/flash/Makefile b/drivers/leds/flash/Makefile
> index 0acbddc0b91b..8a60993f1a25 100644
> --- a/drivers/leds/flash/Makefile
> +++ b/drivers/leds/flash/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_LEDS_AS3645A) += leds-as3645a.o
> obj-$(CONFIG_LEDS_KTD2692) += leds-ktd2692.o
> obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o
> obj-$(CONFIG_LEDS_MAX77693) += leds-max77693.o
> +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
> diff --git a/drivers/leds/flash/leds-qcom-flash.c b/drivers/leds/flash/leds-qcom-flash.c
> new file mode 100644
> index 000000000000..7b941eb769fe
> --- /dev/null
> +++ b/drivers/leds/flash/leds-qcom-flash.c
> @@ -0,0 +1,707 @@
> +//SPDX-License-Identifier: GPL-2.0-only
Missing space after //
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/leds.h>
> +#include <linux/led-class-flash.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <media/v4l2-flash-led-class.h>
> +
> +/* registers definitions */
> +#define FLASH_TYPE_REG 0x04
> +#define FLASH_TYPE_VAL 0x18
> +
> +#define FLASH_SUBTYPE_REG 0x05
> +#define FLASH_SUBTYPE_3CH_VAL 0x04
> +#define FLASH_SUBTYPE_4CH_VAL 0x07
> +
> +#define FLASH_MODULE_EN_BIT BIT(7)
> +
> +#define FLASH_TIMER_EN_BIT BIT(7)
> +#define FLASH_TIMER_VAL_MASK GENMASK(6, 0)
> +#define FLASH_TIMER_STEP_MS 10
> +
> +#define FLASH_ITARGET_CURRENT_MASK GENMASK(6, 0)
> +
> +#define FLASH_STROBE_HW_SW_SEL_BIT BIT(2)
> +#define SW_STROBE_VAL 0
> +#define HW_STROBE_VAL 1
> +#define FLASH_HW_STROBE_TRIGGER_SEL_BIT BIT(1)
> +#define STROBE_LEVEL_TRIGGER_VAL 0
> +#define STROBE_EDGE_TRIGGER_VAL 1
> +#define FLASH_STROBE_POLARITY_BIT BIT(0)
> +#define STROBE_ACTIVE_HIGH_VAL 1
> +
> +#define FLASH_IRES_MASK_4CH BIT(0)
> +#define FLASH_IRES_MASK_3CH GENMASK(1, 0)
> +#define FLASH_IRES_12P5MA_VAL 0
> +#define FLASH_IRES_5MA_VAL_4CH 1
> +#define FLASH_IRES_5MA_VAL_3CH 3
> +
> +/* constants */
> +#define FLASH_CURRENT_MAX_UA 1500000
> +#define TORCH_CURRENT_MAX_UA 500000
> +#define FLASH_TOTAL_CURRENT_MAX_UA 2000000
> +#define FLASH_CURRENT_DEFAULT_UA 1000000
> +#define TORCH_CURRENT_DEFAULT_UA 200000
> +
> +#define TORCH_IRES_UA 5000
> +#define FLASH_IRES_UA 12500
> +
> +#define FLASH_TIMEOUT_MAX_US 1280000
> +#define FLASH_TIMEOUT_STEP_US 10000
> +
> +enum hw_type {
> + QCOM_MVFLASH_3CH,
> + QCOM_MVFLASH_4CH,
> +};
> +
> +enum led_mode {
> + FLASH_MODE,
> + TORCH_MODE,
> +};
> +
> +enum led_strobe {
> + SW_STROBE,
> + HW_STROBE,
> +};
> +
> +struct qcom_flash_reg {
> + u8 module_en;
> + u8 chan_timer;
> + u8 itarget;
> + u8 iresolution;
> + u8 chan_strobe;
> + u8 chan_en;
> + u8 status1;
> + u8 status2;
> + u8 status3;
> +};
> +
> +struct qcom_flash_led {
> + struct qcom_flash_chip *chip;
> + struct led_classdev_flash flash;
> + struct v4l2_flash *v4l2_flash;
> + u32 max_flash_current_ma;
> + u32 max_torch_current_ma;
> + u32 max_timeout_ms;
> + u32 flash_current_ma;
> + u32 flash_timeout_ms;
> + u8 *chan_id;
> + u8 chan_count;
> + bool enabled;
> +};
> +
> +struct qcom_flash_chip {
> + struct qcom_flash_led *leds;
> + const struct qcom_flash_reg *reg;
> + struct device *dev;
> + struct regmap *regmap;
> + struct mutex lock;
> + enum hw_type hw_type;
> + u32 reg_base;
> + u8 leds_count;
> + u8 max_channels;
> + u8 chan_en_bits;
> +};
> +
> +static const struct qcom_flash_reg mvflash_3ch_reg = {
> + .chan_timer = 0x40,
> + .itarget = 0x43,
> + .module_en = 0x46,
> + .iresolution = 0x47,
> + .chan_strobe = 0x49,
> + .chan_en = 0x4c,
> + .status1 = 0x08,
> + .status2 = 0x09,
> + .status3 = 0x0a,
> +};
> +
> +static const struct qcom_flash_reg mvflash_4ch_reg = {
> + .chan_timer = 0x3e,
> + .itarget = 0x42,
> + .module_en = 0x46,
> + .iresolution = 0x49,
> + .chan_strobe = 0x4a,
> + .chan_en = 0x4e,
> + .status1 = 0x06,
> + .status2 = 0x07,
> + .status3 = 0x09,
Don't reinvent the wheel. Use regmap fields.
> +};
> +
> +static int __set_flash_module_en(struct qcom_flash_led *led, bool en)
Drop __ prefix here and in other functions.
(...)
> +
> +static int qcom_flash_led_remove(struct platform_device *pdev)
> +{
> + struct qcom_flash_chip *chip = platform_get_drvdata(pdev);
> +
> + while (chip->leds_count--)
> + v4l2_flash_release(chip->leds[chip->leds_count].v4l2_flash);
> +
> + mutex_destroy(&chip->lock);
> + return 0;
> +}
> +
> +static const struct of_device_id qcom_flash_led_match_table[] = {
> + { .compatible = "qcom,spmi-flash-led" },
Only this one is needed. Remove the rest:
> + { .compatible = "qcom,pm8150c-flash-led" },
> + { .compatible = "qcom,pm8150l-flash-led" },
> + { .compatible = "qcom,pm8350c-flash-led" },
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] dt-bindings: add bindings for QCOM flash LED
2022-09-29 12:15 ` [PATCH v2 2/2] dt-bindings: add bindings for QCOM flash LED Fenglin Wu
@ 2022-09-29 12:40 ` Krzysztof Kozlowski
2022-09-30 1:10 ` Fenglin Wu
2022-09-30 19:33 ` Rob Herring
0 siblings, 2 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2022-09-29 12:40 UTC (permalink / raw)
To: Fenglin Wu, linux-arm-msm, linux-kernel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, linux-leds, devicetree
Cc: collinsd, subbaram
On 29/09/2022 14:15, Fenglin Wu wrote:
> Add binding document for flash LED module inside Qualcomm Technologies,
> Inc. PMICs.
>
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
Thank you for your patch. There is something to discuss/improve.
> + reg:
> + description: address offset of the flash LED controller
> + maxItems: 1
> +
> +patternProperties:
> + "^led[0-3]$":
In such case: ^led-[0-9]$"
> + type: object
> + $ref: common.yaml#
> + unevaluatedProperties: false
> + description: |
> + Represents the physical LED components which are connected to the
> + flash LED channels' output.
> +
> + properties:
> + led-sources:
> + description: |
> + The HW indices of the flash LED channels that connect to the
> + physical LED
> + allOf:
> + - minItems: 1
> + maxItems: 2
> + items:
> + enum: [1, 2, 3, 4]
> +
> + led-max-microamp:
> + description: |
> + The maximum current value when LED is not operating in flash mode (i.e. torch mode)
> + Valid values when an LED is connected to one flash LED channel:
> + 5000 - 500000, step by 5000
> + Valid values when an LED is connected to two flash LED channels:
> + 10000 - 1000000, step by 10000
> + minimum: 5000
> + maximum: 1000000
> +
> + flash-max-microamp:
> + description: |
> + The maximum current value when LED is operating in flash mode.
> + Valid values when an LED is connected to one flash LED channel:
> + 12500 - 1500000, step by 12500
> + Valid values when an LED is connected to two flash LED channels:
> + 25000 - 2000000, step by 12500
> + minimum: 12500
> + maximum: 2000000
> +
> + flash-max-timeout-us:
> + description: |
> + The maximum timeout value when LED is operating in flash mode.
> + Valid values: 10000 - 1280000, step by 10000
> + minimum: 10000
> + maximum: 1280000
> +
> + required:
> + - led-sources
> + - led-max-microamp
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/leds/common.h>
> + spmi_bus {
No underscores in node names, so just "bus"
> + #address-cells = <1>;
> + #size-cells = <0>;
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] dt-bindings: add bindings for QCOM flash LED
2022-09-29 12:40 ` Krzysztof Kozlowski
@ 2022-09-30 1:10 ` Fenglin Wu
2022-09-30 19:33 ` Rob Herring
1 sibling, 0 replies; 11+ messages in thread
From: Fenglin Wu @ 2022-09-30 1:10 UTC (permalink / raw)
To: Krzysztof Kozlowski, linux-arm-msm, linux-kernel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, linux-leds, devicetree
Cc: collinsd, subbaram
On 2022/9/29 20:40, Krzysztof Kozlowski wrote:
> On 29/09/2022 14:15, Fenglin Wu wrote:
>> Add binding document for flash LED module inside Qualcomm Technologies,
>> Inc. PMICs.
>>
>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>
> Thank you for your patch. There is something to discuss/improve.
>
>> + reg:
>> + description: address offset of the flash LED controller
>> + maxItems: 1
>> +
>> +patternProperties:
>> + "^led[0-3]$":
>
> In such case: ^led-[0-9]$"
Sure, I will update in next patchset
>
>> + type: object
>> + $ref: common.yaml#
>> + unevaluatedProperties: false
>> + description: |
>> + Represents the physical LED components which are connected to the
>> + flash LED channels' output.
>> +
>> + properties:
>> + led-sources:
>> + description: |
>> + The HW indices of the flash LED channels that connect to the
>> + physical LED
>> + allOf:
>> + - minItems: 1
>> + maxItems: 2
>> + items:
>> + enum: [1, 2, 3, 4]
>> +
>> + led-max-microamp:
>> + description: |
>> + The maximum current value when LED is not operating in flash mode (i.e. torch mode)
>> + Valid values when an LED is connected to one flash LED channel:
>> + 5000 - 500000, step by 5000
>> + Valid values when an LED is connected to two flash LED channels:
>> + 10000 - 1000000, step by 10000
>> + minimum: 5000
>> + maximum: 1000000
>> +
>> + flash-max-microamp:
>> + description: |
>> + The maximum current value when LED is operating in flash mode.
>> + Valid values when an LED is connected to one flash LED channel:
>> + 12500 - 1500000, step by 12500
>> + Valid values when an LED is connected to two flash LED channels:
>> + 25000 - 2000000, step by 12500
>> + minimum: 12500
>> + maximum: 2000000
>> +
>> + flash-max-timeout-us:
>> + description: |
>> + The maximum timeout value when LED is operating in flash mode.
>> + Valid values: 10000 - 1280000, step by 10000
>> + minimum: 10000
>> + maximum: 1280000
>> +
>> + required:
>> + - led-sources
>> + - led-max-microamp
>> +
>> +required:
>> + - compatible
>> + - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/leds/common.h>
>> + spmi_bus {
>
> No underscores in node names, so just "bus"
Sure, I will update in next patchset
>
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] dt-bindings: add bindings for QCOM flash LED
2022-09-29 12:40 ` Krzysztof Kozlowski
2022-09-30 1:10 ` Fenglin Wu
@ 2022-09-30 19:33 ` Rob Herring
2022-10-03 12:49 ` Krzysztof Kozlowski
2022-10-10 9:47 ` Fenglin Wu
1 sibling, 2 replies; 11+ messages in thread
From: Rob Herring @ 2022-09-30 19:33 UTC (permalink / raw)
To: Krzysztof Kozlowski, Fenglin Wu
Cc: linux-arm-msm, linux-kernel, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Pavel Machek, Krzysztof Kozlowski, linux-leds,
devicetree, collinsd, subbaram
On Thu, Sep 29, 2022 at 02:40:05PM +0200, Krzysztof Kozlowski wrote:
> On 29/09/2022 14:15, Fenglin Wu wrote:
> > Add binding document for flash LED module inside Qualcomm Technologies,
> > Inc. PMICs.
> >
> > Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>
> Thank you for your patch. There is something to discuss/improve.
>
> > + reg:
> > + description: address offset of the flash LED controller
> > + maxItems: 1
> > +
> > +patternProperties:
> > + "^led[0-3]$":
>
> In such case: ^led-[0-9]$"
>
> > + type: object
> > + $ref: common.yaml#
> > + unevaluatedProperties: false
> > + description: |
> > + Represents the physical LED components which are connected to the
> > + flash LED channels' output.
> > +
> > + properties:
> > + led-sources:
This is for when the power source and LED connection are programmable.
IOW, when 'reg' is not enough to describe the configuration. If you only
have LED channels 1-4 with a fixed connection to LED pins/output 1-4,
then use 'reg'.
> > + description: |
> > + The HW indices of the flash LED channels that connect to the
> > + physical LED
> > + allOf:
> > + - minItems: 1
> > + maxItems: 2
> > + items:
> > + enum: [1, 2, 3, 4]
> > +
> > + led-max-microamp:
> > + description: |
> > + The maximum current value when LED is not operating in flash mode (i.e. torch mode)
> > + Valid values when an LED is connected to one flash LED channel:
> > + 5000 - 500000, step by 5000
> > + Valid values when an LED is connected to two flash LED channels:
> > + 10000 - 1000000, step by 10000
> > + minimum: 5000
> > + maximum: 1000000
anyOf:
- minimum: 5000
maximum: 500000
multipleOf: 5000
- minimum: 10000
maximum: 1000000
multipleOf: 10000
Drop any description that's captured by the constraints.
> > +
> > + flash-max-microamp:
> > + description: |
> > + The maximum current value when LED is operating in flash mode.
> > + Valid values when an LED is connected to one flash LED channel:
> > + 12500 - 1500000, step by 12500
> > + Valid values when an LED is connected to two flash LED channels:
> > + 25000 - 2000000, step by 12500
> > + minimum: 12500
> > + maximum: 2000000
> > +
> > + flash-max-timeout-us:
> > + description: |
> > + The maximum timeout value when LED is operating in flash mode.
> > + Valid values: 10000 - 1280000, step by 10000
> > + minimum: 10000
> > + maximum: 1280000
Similar comment for these 2.
> > +
> > + required:
> > + - led-sources
> > + - led-max-microamp
> > +
> > +required:
> > + - compatible
> > + - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/leds/common.h>
> > + spmi_bus {
>
> No underscores in node names, so just "bus"
SPMI is something else though...
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] dt-bindings: add bindings for QCOM flash LED
2022-09-30 19:33 ` Rob Herring
@ 2022-10-03 12:49 ` Krzysztof Kozlowski
2022-10-10 9:47 ` Fenglin Wu
1 sibling, 0 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2022-10-03 12:49 UTC (permalink / raw)
To: Rob Herring, Fenglin Wu
Cc: linux-arm-msm, linux-kernel, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Pavel Machek, Krzysztof Kozlowski, linux-leds,
devicetree, collinsd, subbaram
On 30/09/2022 21:33, Rob Herring wrote:
>>> +
>>> + required:
>>> + - led-sources
>>> + - led-max-microamp
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + #include <dt-bindings/leds/common.h>
>>> + spmi_bus {
>>
>> No underscores in node names, so just "bus"
>
> SPMI is something else though...
True, then rather spmi - already used in DTS.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] dt-bindings: add bindings for QCOM flash LED
2022-09-30 19:33 ` Rob Herring
2022-10-03 12:49 ` Krzysztof Kozlowski
@ 2022-10-10 9:47 ` Fenglin Wu
1 sibling, 0 replies; 11+ messages in thread
From: Fenglin Wu @ 2022-10-10 9:47 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski
Cc: linux-arm-msm, linux-kernel, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Pavel Machek, Krzysztof Kozlowski, linux-leds,
devicetree, Subbaraman Narayanamurthy, quic_collinsd
On 2022/10/1 3:33, Rob Herring wrote:
> On Thu, Sep 29, 2022 at 02:40:05PM +0200, Krzysztof Kozlowski wrote:
>> On 29/09/2022 14:15, Fenglin Wu wrote:
>>> Add binding document for flash LED module inside Qualcomm Technologies,
>>> Inc. PMICs.
>>>
>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>> + reg:
>>> + description: address offset of the flash LED controller
>>> + maxItems: 1
>>> +
>>> +patternProperties:
>>> + "^led[0-3]$":
>>
>> In such case: ^led-[0-9]$"
>>
>>> + type: object
>>> + $ref: common.yaml#
>>> + unevaluatedProperties: false
>>> + description: |
>>> + Represents the physical LED components which are connected to the
>>> + flash LED channels' output.
>>> +
>>> + properties:
>>> + led-sources:
>
> This is for when the power source and LED connection are programmable.
> IOW, when 'reg' is not enough to describe the configuration. If you only
> have LED channels 1-4 with a fixed connection to LED pins/output 1-4,
> then use 'reg'.
>
I think using led-sources here is more appropriate. The LED connection
can be programmable to match with the board design. The flash module has
4 channels (current outputs, indexed from 1 to 4) and the LEDs can be
connected to either one or two of them depends on the design. Such as,
if the design only requires LEDs with 1.5 A maximum current, then the HW
just connects one channel to each LED and specify the corresponding
channel index in the led-sources. Or if the design requires LEDs
supporting up to 2 A maximum current, then the HW needs to gang 2
channels' output together and specify the HW indices in led-sources
accordingly.
>>> + description: |
>>> + The HW indices of the flash LED channels that connect to the
>>> + physical LED
>>> + allOf:
>>> + - minItems: 1
>>> + maxItems: 2
>>> + items:
>>> + enum: [1, 2, 3, 4]
>>> +
>>> + led-max-microamp:
>>> + description: |
>>> + The maximum current value when LED is not operating in flash mode (i.e. torch mode)
>>> + Valid values when an LED is connected to one flash LED channel:
>>> + 5000 - 500000, step by 5000
>>> + Valid values when an LED is connected to two flash LED channels:
>>> + 10000 - 1000000, step by 10000
>>> + minimum: 5000
>>> + maximum: 1000000
>
> anyOf:
> - minimum: 5000
> maximum: 500000
> multipleOf: 5000
> - minimum: 10000
> maximum: 1000000
> multipleOf: 10000
>
> Drop any description that's captured by the constraints.
Thanks for the suggestion. I will update it accordingly.
>
>>> +
>>> + flash-max-microamp:
>>> + description: |
>>> + The maximum current value when LED is operating in flash mode.
>>> + Valid values when an LED is connected to one flash LED channel:
>>> + 12500 - 1500000, step by 12500
>>> + Valid values when an LED is connected to two flash LED channels:
>>> + 25000 - 2000000, step by 12500
>>> + minimum: 12500
>>> + maximum: 2000000
>>> +
>>> + flash-max-timeout-us:
>>> + description: |
>>> + The maximum timeout value when LED is operating in flash mode.
>>> + Valid values: 10000 - 1280000, step by 10000
>>> + minimum: 10000
>>> + maximum: 1280000
>
> Similar comment for these 2.
Done
>
>>> +
>>> + required:
>>> + - led-sources
>>> + - led-max-microamp
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + #include <dt-bindings/leds/common.h>
>>> + spmi_bus {
>>
>> No underscores in node names, so just "bus"
>
> SPMI is something else though...
Sure, will update it to spmi
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] leds: flash: add driver to support flash LED module in QCOM PMICs
2022-09-29 12:23 ` Krzysztof Kozlowski
@ 2022-10-10 10:00 ` Fenglin Wu
2022-10-10 10:20 ` Krzysztof Kozlowski
0 siblings, 1 reply; 11+ messages in thread
From: Fenglin Wu @ 2022-10-10 10:00 UTC (permalink / raw)
To: Krzysztof Kozlowski, linux-arm-msm, linux-kernel, Pavel Machek,
Gene Chen, Jacek Anaszewski, linux-leds
Cc: collinsd, subbaram
On 2022/9/29 20:23, Krzysztof Kozlowski wrote:
> On 29/09/2022 14:15, Fenglin Wu wrote:
>> Add initial driver to support flash LED module found in Qualcomm
>> Technologies, Inc. PMICs. The flash module can have 3 or 4 channels
>> and each channel can be controlled indepedently and support full scale
>> current up to 1.5 A. It also supports connecting two channels together
>> to supply one LED component with full scale current up to 2 A. In that
>> case, the current will be split on each channel symmetrically and the
>> channels will be enabled and disabled at the same time.
>>
>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>> ---
>> drivers/leds/flash/Kconfig | 14 +
>> drivers/leds/flash/Makefile | 1 +
>> drivers/leds/flash/leds-qcom-flash.c | 707 +++++++++++++++++++++++++++
>> 3 files changed, 722 insertions(+)
>> create mode 100644 drivers/leds/flash/leds-qcom-flash.c
>>
>> diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
>> index d3eb689b193c..92773fa872dc 100644
>> --- a/drivers/leds/flash/Kconfig
>> +++ b/drivers/leds/flash/Kconfig
>> @@ -61,6 +61,20 @@ config LEDS_MT6360
>> Independent current sources supply for each flash LED support torch
>> and strobe mode.
>>
>> +config LEDS_QCOM_FLASH
>> + tristate "LED support for flash module inside Qualcomm Technologies, Inc. PMIC"
>> + depends on MFD_SPMI_PMIC
>
> || COMPILE_TEST
>
> (and actually test it, e.g. you might need here "select REGMAP")
>
I will add "COMPILE_TEST" here and compile with with "COMPILE_TEST" enabled.
>> + depends on LEDS_CLASS && OF
>> + depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
>> + help
>> + This option enables support for the flash module found in Qualcomm
>> + Technologies, Inc. PMICs. The flash module can have 3 or 4 flash LED
>> + channels and each channel is programmable to support up to 1.5 A full
>> + scale current. It also supports connecting two channels' output together
>> + to supply one LED component to achieve current up to 2 A. In such case,
>> + the total LED current will be split symmetrically on each channel and
>> + they will be enabled/disabled at the same time.
>> +
>
>> config LEDS_RT4505
>> tristate "LED support for RT4505 flashlight controller"
>> depends on I2C && OF
>> diff --git a/drivers/leds/flash/Makefile b/drivers/leds/flash/Makefile
>> index 0acbddc0b91b..8a60993f1a25 100644
>> --- a/drivers/leds/flash/Makefile
>> +++ b/drivers/leds/flash/Makefile
>> @@ -6,6 +6,7 @@ obj-$(CONFIG_LEDS_AS3645A) += leds-as3645a.o
>> obj-$(CONFIG_LEDS_KTD2692) += leds-ktd2692.o
>> obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o
>> obj-$(CONFIG_LEDS_MAX77693) += leds-max77693.o
>> +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
>> diff --git a/drivers/leds/flash/leds-qcom-flash.c b/drivers/leds/flash/leds-qcom-flash.c
>> new file mode 100644
>> index 000000000000..7b941eb769fe
>> --- /dev/null
>> +++ b/drivers/leds/flash/leds-qcom-flash.c
>> @@ -0,0 +1,707 @@
>> +//SPDX-License-Identifier: GPL-2.0-only
>
> Missing space after //
Thanks for catching it, will fix it.
>
>> +/*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/bits.h>
>> +#include <linux/leds.h>
>> +#include <linux/led-class-flash.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/property.h>
>> +#include <linux/regmap.h>
>> +#include <media/v4l2-flash-led-class.h>
>> +
>> +/* registers definitions */
>> +#define FLASH_TYPE_REG 0x04
>> +#define FLASH_TYPE_VAL 0x18
>> +
>> +#define FLASH_SUBTYPE_REG 0x05
>> +#define FLASH_SUBTYPE_3CH_VAL 0x04
>> +#define FLASH_SUBTYPE_4CH_VAL 0x07
>> +
>> +#define FLASH_MODULE_EN_BIT BIT(7)
>> +
>> +#define FLASH_TIMER_EN_BIT BIT(7)
>> +#define FLASH_TIMER_VAL_MASK GENMASK(6, 0)
>> +#define FLASH_TIMER_STEP_MS 10
>> +
>> +#define FLASH_ITARGET_CURRENT_MASK GENMASK(6, 0)
>> +
>> +#define FLASH_STROBE_HW_SW_SEL_BIT BIT(2)
>> +#define SW_STROBE_VAL 0
>> +#define HW_STROBE_VAL 1
>> +#define FLASH_HW_STROBE_TRIGGER_SEL_BIT BIT(1)
>> +#define STROBE_LEVEL_TRIGGER_VAL 0
>> +#define STROBE_EDGE_TRIGGER_VAL 1
>> +#define FLASH_STROBE_POLARITY_BIT BIT(0)
>> +#define STROBE_ACTIVE_HIGH_VAL 1
>> +
>> +#define FLASH_IRES_MASK_4CH BIT(0)
>> +#define FLASH_IRES_MASK_3CH GENMASK(1, 0)
>> +#define FLASH_IRES_12P5MA_VAL 0
>> +#define FLASH_IRES_5MA_VAL_4CH 1
>> +#define FLASH_IRES_5MA_VAL_3CH 3
>> +
>> +/* constants */
>> +#define FLASH_CURRENT_MAX_UA 1500000
>> +#define TORCH_CURRENT_MAX_UA 500000
>> +#define FLASH_TOTAL_CURRENT_MAX_UA 2000000
>> +#define FLASH_CURRENT_DEFAULT_UA 1000000
>> +#define TORCH_CURRENT_DEFAULT_UA 200000
>> +
>> +#define TORCH_IRES_UA 5000
>> +#define FLASH_IRES_UA 12500
>> +
>> +#define FLASH_TIMEOUT_MAX_US 1280000
>> +#define FLASH_TIMEOUT_STEP_US 10000
>> +
>> +enum hw_type {
>> + QCOM_MVFLASH_3CH,
>> + QCOM_MVFLASH_4CH,
>> +};
>> +
>> +enum led_mode {
>> + FLASH_MODE,
>> + TORCH_MODE,
>> +};
>> +
>> +enum led_strobe {
>> + SW_STROBE,
>> + HW_STROBE,
>> +};
>> +
>> +struct qcom_flash_reg {
>> + u8 module_en;
>> + u8 chan_timer;
>> + u8 itarget;
>> + u8 iresolution;
>> + u8 chan_strobe;
>> + u8 chan_en;
>> + u8 status1;
>> + u8 status2;
>> + u8 status3;
>> +};
>> +
>> +struct qcom_flash_led {
>> + struct qcom_flash_chip *chip;
>> + struct led_classdev_flash flash;
>> + struct v4l2_flash *v4l2_flash;
>> + u32 max_flash_current_ma;
>> + u32 max_torch_current_ma;
>> + u32 max_timeout_ms;
>> + u32 flash_current_ma;
>> + u32 flash_timeout_ms;
>> + u8 *chan_id;
>> + u8 chan_count;
>> + bool enabled;
>> +};
>> +
>> +struct qcom_flash_chip {
>> + struct qcom_flash_led *leds;
>> + const struct qcom_flash_reg *reg;
>> + struct device *dev;
>> + struct regmap *regmap;
>> + struct mutex lock;
>> + enum hw_type hw_type;
>> + u32 reg_base;
>> + u8 leds_count;
>> + u8 max_channels;
>> + u8 chan_en_bits;
>> +};
>> +
>> +static const struct qcom_flash_reg mvflash_3ch_reg = {
>> + .chan_timer = 0x40,
>> + .itarget = 0x43,
>> + .module_en = 0x46,
>> + .iresolution = 0x47,
>> + .chan_strobe = 0x49,
>> + .chan_en = 0x4c,
>> + .status1 = 0x08,
>> + .status2 = 0x09,
>> + .status3 = 0x0a,
>> +};
>> +
>> +static const struct qcom_flash_reg mvflash_4ch_reg = {
>> + .chan_timer = 0x3e,
>> + .itarget = 0x42,
>> + .module_en = 0x46,
>> + .iresolution = 0x49,
>> + .chan_strobe = 0x4a,
>> + .chan_en = 0x4e,
>> + .status1 = 0x06,
>> + .status2 = 0x07,
>> + .status3 = 0x09,
>
> Don't reinvent the wheel. Use regmap fields.
>
Do you mean defining regmap_filed pointer in struct qcom_flash_chip
for all these registers instead of creating the data structure
qcom_flash_reg to include all the registers? Similar like this
struct qcom_flash_chip {
....
struct regmap_field *chan_timer;
struct regmap_field *chan_timer;
struct regmap_field *itarget;
struct regmap_field *iresolution;
struct regmap_field *chan_strobe;
struct regmap_field *chan_en;
struct regmap_field *status1;
struct regmap_field *status2;
struct regmap_field *status3;
....
};
This won't make the code cleaner actually. If this is the preferred way,
I will update it accordingly.
Thanks
>> +};
>> +
>> +static int __set_flash_module_en(struct qcom_flash_led *led, bool en)
>
> Drop __ prefix here and in other functions.
>
> (...)
Done
>
>> +
>> +static int qcom_flash_led_remove(struct platform_device *pdev)
>> +{
>> + struct qcom_flash_chip *chip = platform_get_drvdata(pdev);
>> +
>> + while (chip->leds_count--)
>> + v4l2_flash_release(chip->leds[chip->leds_count].v4l2_flash);
>> +
>> + mutex_destroy(&chip->lock);
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id qcom_flash_led_match_table[] = {
>> + { .compatible = "qcom,spmi-flash-led" },
>
> Only this one is needed. Remove the rest:
Done
>
>> + { .compatible = "qcom,pm8150c-flash-led" },
>> + { .compatible = "qcom,pm8150l-flash-led" },
>> + { .compatible = "qcom,pm8350c-flash-led" },
>
>
>
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] leds: flash: add driver to support flash LED module in QCOM PMICs
2022-10-10 10:00 ` Fenglin Wu
@ 2022-10-10 10:20 ` Krzysztof Kozlowski
0 siblings, 0 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2022-10-10 10:20 UTC (permalink / raw)
To: Fenglin Wu, linux-arm-msm, linux-kernel, Pavel Machek, Gene Chen,
Jacek Anaszewski, linux-leds
Cc: collinsd, subbaram
On 10/10/2022 06:00, Fenglin Wu wrote:
>>> +
>>> +static const struct qcom_flash_reg mvflash_3ch_reg = {
>>> + .chan_timer = 0x40,
>>> + .itarget = 0x43,
>>> + .module_en = 0x46,
>>> + .iresolution = 0x47,
>>> + .chan_strobe = 0x49,
>>> + .chan_en = 0x4c,
>>> + .status1 = 0x08,
>>> + .status2 = 0x09,
>>> + .status3 = 0x0a,
>>> +};
>>> +
>>> +static const struct qcom_flash_reg mvflash_4ch_reg = {
>>> + .chan_timer = 0x3e,
>>> + .itarget = 0x42,
>>> + .module_en = 0x46,
>>> + .iresolution = 0x49,
>>> + .chan_strobe = 0x4a,
>>> + .chan_en = 0x4e,
>>> + .status1 = 0x06,
>>> + .status2 = 0x07,
>>> + .status3 = 0x09,
>>
>> Don't reinvent the wheel. Use regmap fields.
>>
>
> Do you mean defining regmap_filed pointer in struct qcom_flash_chip
> for all these registers instead of creating the data structure
> qcom_flash_reg to include all the registers? Similar like this
>
> struct qcom_flash_chip {
> ....
> struct regmap_field *chan_timer;
> struct regmap_field *chan_timer;
> struct regmap_field *itarget;
> struct regmap_field *iresolution;
> struct regmap_field *chan_strobe;
> struct regmap_field *chan_en;
> struct regmap_field *status1;
> struct regmap_field *status2;
> struct regmap_field *status3;
> ....
> };
This is one way, other is with allocated array and indexing by some enum.
>
> This won't make the code cleaner actually. If this is the preferred way,
> I will update it accordingly.
A bit it will, because regmap fields will replace most of your shifts
and masks. The point is rather not doing something by your own, but
using frameworks which are exactly for this purpose.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-10-10 10:22 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-29 12:15 [PATCH v2 0/2] Add LED driver for flash module in QCOM PMICs Fenglin Wu
2022-09-29 12:15 ` [PATCH v2 1/2] leds: flash: add driver to support flash LED " Fenglin Wu
2022-09-29 12:23 ` Krzysztof Kozlowski
2022-10-10 10:00 ` Fenglin Wu
2022-10-10 10:20 ` Krzysztof Kozlowski
2022-09-29 12:15 ` [PATCH v2 2/2] dt-bindings: add bindings for QCOM flash LED Fenglin Wu
2022-09-29 12:40 ` Krzysztof Kozlowski
2022-09-30 1:10 ` Fenglin Wu
2022-09-30 19:33 ` Rob Herring
2022-10-03 12:49 ` Krzysztof Kozlowski
2022-10-10 9:47 ` Fenglin Wu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).