[RESEND,v5,1/2] leds: flash: add driver to support flash LED module in QCOM PMICs

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>
Tested-by: Luca Weiss <luca.weiss@fairphone.com> # sm7225-fairphone-fp4 + pm6150l
---
 drivers/leds/flash/Kconfig           |  15 +
 drivers/leds/flash/Makefile          |   1 +
 drivers/leds/flash/leds-qcom-flash.c | 701 +++++++++++++++++++++++++++
 3 files changed, 717 insertions(+)
 create mode 100644 drivers/leds/flash/leds-qcom-flash.c

On Tue, 27 Dec 2022, 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>
> Tested-by: Luca Weiss <luca.weiss@fairphone.com> # sm7225-fairphone-fp4 + pm6150l
> ---
>  drivers/leds/flash/Kconfig           |  15 +
>  drivers/leds/flash/Makefile          |   1 +
>  drivers/leds/flash/leds-qcom-flash.c | 701 +++++++++++++++++++++++++++
>  3 files changed, 717 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..f36a60409290 100644
> --- a/drivers/leds/flash/Kconfig
> +++ b/drivers/leds/flash/Kconfig
> @@ -61,6 +61,21 @@ 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
> +	depends on LEDS_CLASS && OF
> +	depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
> +	select REGMAP
> +	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..3735282b77e9
> --- /dev/null
> +++ b/drivers/leds/flash/leds-qcom-flash.c
> @@ -0,0 +1,701 @@
> +// 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_TIMER_EN_BIT		BIT(7)
> +#define FLASH_TIMER_VAL_MASK		GENMASK(6, 0)
> +#define FLASH_TIMER_STEP_MS		10
> +
> +#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,
> +};
> +
> +enum {
> +	REG_STATUS1,
> +	REG_STATUS2,
> +	REG_STATUS3,
> +	REG_CHAN_TIMER,
> +	REG_ITARGET,
> +	REG_MODULE_EN,
> +	REG_IRESOLUTION,
> +	REG_CHAN_STROBE,
> +	REG_CHAN_EN,
> +	REG_MAX_COUNT,
> +};
> +
> +struct reg_field mvflash_3ch_regs[REG_MAX_COUNT] = {
> +	REG_FIELD(0x08, 0, 7),			/* status1	*/
> +	REG_FIELD(0x09, 0, 7),                  /* status2	*/
> +	REG_FIELD(0x0a, 0, 7),                  /* status3	*/
> +	REG_FIELD_ID(0x40, 0, 7, 3, 1),         /* chan_timer	*/
> +	REG_FIELD_ID(0x43, 0, 6, 3, 1),         /* itarget	*/
> +	REG_FIELD(0x46, 7, 7),                  /* module_en	*/
> +	REG_FIELD(0x47, 0, 5),                  /* iresolution	*/
> +	REG_FIELD_ID(0x49, 0, 2, 3, 1),         /* chan_strobe	*/
> +	REG_FIELD(0x4c, 0, 2),                  /* chan_en	*/
> +};
> +
> +struct reg_field mvflash_4ch_regs[REG_MAX_COUNT] = {
> +	REG_FIELD(0x06, 0, 7),			/* status1	*/
> +	REG_FIELD(0x07, 0, 6),			/* status2	*/
> +	REG_FIELD(0x09, 0, 7),			/* status3	*/
> +	REG_FIELD_ID(0x3e, 0, 7, 4, 1),		/* chan_timer	*/
> +	REG_FIELD_ID(0x42, 0, 6, 4, 1),		/* itarget	*/
> +	REG_FIELD(0x46, 7, 7),			/* module_en	*/
> +	REG_FIELD(0x49, 0, 3),			/* iresolution	*/
> +	REG_FIELD_ID(0x4a, 0, 6, 4, 1),		/* chan_strobe	*/
> +	REG_FIELD(0x4e, 0, 3),			/* chan_en	*/
> +};
> +
> +struct qcom_flash_led {
> +	struct qcom_flash_chip		*chip;

Not a fan of these interwoven references.  Where 'chip' has a reverence
to 'led' and 'led' has a reference to 'chip'.

  chip->leds[0]->chip->leds[0]->chip ...

Either re-work your API (pass 'chip' and an LED index for example [this
may very well not be the correct solution) or use something akin to
container_of().

> +	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;
> +	struct regmap_field     *r_fields[REG_MAX_COUNT];
> +	struct device		*dev;
> +	struct mutex		lock;
> +	enum hw_type		hw_type;
> +	u8			leds_count;
> +	u8			max_channels;
> +	u8			chan_en_bits;
> +};
> +
> +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;
> +	rc = regmap_field_write(chip->r_fields[REG_MODULE_EN], val);

I'm a little confused by this.

So we go through the process of flipping individual enable bits, then
trash all of that knowledge and end up writing an individual 0 or 1.

Why not replace the whole function with:

    regmap_field_write(chip->r_fields[REG_MODULE_EN], en);

> +	if (rc < 0)
> +		dev_err(chip->dev, "write module_en failed, rc=%d\n", rc);
> +	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;

Please make it clear what you're doing here.

I suggest you separate the allocation and the assignment and add a
comment to the latter.

> +	u32 ires_ua = (mode == FLASH_MODE) ? FLASH_IRES_UA : TORCH_IRES_UA;
> +	u8 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;

If you allocate 'val' in here and pre-assign it, you can negate the
logic and omit this branch.  Unless this is going to be multi-use, 'val'
is not a great name for a variable.

> +		else
> +			val = itarg_ua / ires_ua - 1;
> +
> +		rc = regmap_fields_write(chip->r_fields[REG_ITARGET], chan_id - 1, val);

Instead of '-1'ing every time you wish to use the value, why don't you
start the numbering off at 0 instead?  If that's not possible, take 1
off when you initially assign it.

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

... else do something unexpected?

> +	}
> +
> +	return regmap_field_update_bits(chip->r_fields[REG_IRESOLUTION], 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 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];
> +		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;

Nit: This if doesn't have anything to do with the line above.  If you
were to space them out (with a '\n') some sections would be easier to
read.  Please also apply this logic to the rest of the file.

> +
> +		rc = regmap_fields_write(chip->r_fields[REG_CHAN_TIMER], chan_id - 1, 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 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];
> +		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);
> +		rc = regmap_fields_write(chip->r_fields[REG_CHAN_STROBE], chan_id - 1, val);
> +		if (rc < 0)
> +			return rc;
> +
> +		chan_mask |= BIT(chan_id - 1);
> +	}
> +
> +	/* enable/disable flash channels */

Nit: "Enable"

> +	mask = chan_mask;

Why can't you just use chan_mask?

> +	val = state ? mask : 0;
> +	rc = regmap_field_update_bits(chip->r_fields[REG_CHAN_EN], 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);

No magic numbers please.

Can you define the 1000 or use a MACRO instead please?

Same throughout.

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

These can all be simplified to 'if (rc)'?

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

Why do you use the qcom_ prefix sometimes and not others?

> +{
> +	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_field_read(chip->r_fields[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_field_read(chip->r_fields[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;
> +	}

No magic numbers please.

> +	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_field_read(chip->r_fields[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 */

Nit: "Disable"

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

This is an ugly way to break a line.  Suggest this:

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;

Suggest some additional bracketing here.

> +}
> +
> +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};

Nit: Spaces around the 0 please.

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

This is a bool.

> +	v4l2_cfg.flash_faults = LED_FAULT_INPUT_VOLTAGE | LED_FAULT_OVER_CURRENT |
> +		LED_FAULT_SHORT_CIRCUIT | LED_FAULT_OVER_TEMPERATURE | LED_FAULT_TIMEOUT;

	v4l2_cfg.flash_faults = LED_FAULT_INPUT_VOLTAGE |
				LED_FAULT_OVER_CURRENT |
				LED_FAULT_SHORT_CIRCUIT |
				LED_FAULT_OVER_TEMPERATURE |
				LED_FAULT_TIMEOUT;

Don't you think that's nicer to read?

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

a) this has nothing to do with the following line, so you should
separate it and b) I don't see any reason not to place the assignment
on the allocate line.

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

"Failed to read led-max-microamp property"

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

As above.

> +			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' is not a good variable name.

> +		s->min = s->step = FLASH_IRES_UA * led->chan_count;

These get over-written before they are used.

Scrap that, I see that 's' gets reassigned.

Even more reason to change 's' for something else and use 2 variables
instead of one to make this point clear.

A few comments spread around the complex areas wouldn't go amiss either.

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

'\n'

> +	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;
> +	struct regmap *map;
> +	struct reg_field *regs;
> +	int count, i, rc;
> +	u32 val, reg_base;
> +
> +	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	map = dev_get_regmap(dev->parent, NULL);

Can you use 'regmap' here please.

> +	if (!map) {
> +		dev_err(dev, "Failed to get parent regmap\n");
> +		return -EINVAL;
> +	}
> +
> +	rc = fwnode_property_read_u32(dev->fwnode, "reg", &reg_base);
> +	if (rc < 0) {
> +		dev_err(dev, "Failed to get register base address, rc=%d\n", rc);
> +		return rc;
> +	}
> +
> +	rc = regmap_read(map, 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);

Can we be more specific?  Flash to me is a memory devices.

> +		return -ENODEV;
> +	}
> +
> +	rc = regmap_read(map, 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;
> +		regs = mvflash_3ch_regs;
> +	} else if (val == FLASH_SUBTYPE_4CH_VAL) {
> +		chip->hw_type = QCOM_MVFLASH_4CH;
> +		chip->max_channels = 4;
> +		regs = mvflash_4ch_regs;
> +	} else {
> +		dev_err(dev, "flash subtype %#x is not yet supported\n", val);
> +		return -ENODEV;
> +	}
> +
> +	for (i = 0; i < REG_MAX_COUNT; i++)
> +		regs[i].reg += reg_base;
> +
> +	rc = devm_regmap_field_bulk_alloc(dev, map, chip->r_fields, regs, REG_MAX_COUNT);
> +	if (rc < 0) {
> +		dev_err(dev, "failed to alloc regmap filed, rc=%d\n", rc);

"Failed to allocate ..."

"field"

> +		return rc;
> +	}
> +
> +	chip->dev = dev;
> +	platform_set_drvdata(pdev, chip);
> +	mutex_init(&chip->lock);

'\n'

Lots of other places too - please refrain from smooshing code together.

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

'\n'

> +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" },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, qcom_flash_led_match_table);
> +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
>

Hi Jones,

Thanks for reviewing the driver!
Replies inline.


On 2023/1/26 21:23, Lee Jones wrote:
> On Tue, 27 Dec 2022, 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>
>> Tested-by: Luca Weiss <luca.weiss@fairphone.com> # sm7225-fairphone-fp4 + pm6150l
>> ---
>>   drivers/leds/flash/Kconfig           |  15 +
>>   drivers/leds/flash/Makefile          |   1 +
>>   drivers/leds/flash/leds-qcom-flash.c | 701 +++++++++++++++++++++++++++
>>   3 files changed, 717 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..f36a60409290 100644
>> --- a/drivers/leds/flash/Kconfig
>> +++ b/drivers/leds/flash/Kconfig
>> @@ -61,6 +61,21 @@ 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
>> +	depends on LEDS_CLASS && OF
>> +	depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
>> +	select REGMAP
>> +	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..3735282b77e9
>> --- /dev/null
>> +++ b/drivers/leds/flash/leds-qcom-flash.c
>> @@ -0,0 +1,701 @@
>> +// 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_TIMER_EN_BIT		BIT(7)
>> +#define FLASH_TIMER_VAL_MASK		GENMASK(6, 0)
>> +#define FLASH_TIMER_STEP_MS		10
>> +
>> +#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,
>> +};
>> +
>> +enum {
>> +	REG_STATUS1,
>> +	REG_STATUS2,
>> +	REG_STATUS3,
>> +	REG_CHAN_TIMER,
>> +	REG_ITARGET,
>> +	REG_MODULE_EN,
>> +	REG_IRESOLUTION,
>> +	REG_CHAN_STROBE,
>> +	REG_CHAN_EN,
>> +	REG_MAX_COUNT,
>> +};
>> +
>> +struct reg_field mvflash_3ch_regs[REG_MAX_COUNT] = {
>> +	REG_FIELD(0x08, 0, 7),			/* status1	*/
>> +	REG_FIELD(0x09, 0, 7),                  /* status2	*/
>> +	REG_FIELD(0x0a, 0, 7),                  /* status3	*/
>> +	REG_FIELD_ID(0x40, 0, 7, 3, 1),         /* chan_timer	*/
>> +	REG_FIELD_ID(0x43, 0, 6, 3, 1),         /* itarget	*/
>> +	REG_FIELD(0x46, 7, 7),                  /* module_en	*/
>> +	REG_FIELD(0x47, 0, 5),                  /* iresolution	*/
>> +	REG_FIELD_ID(0x49, 0, 2, 3, 1),         /* chan_strobe	*/
>> +	REG_FIELD(0x4c, 0, 2),                  /* chan_en	*/
>> +};
>> +
>> +struct reg_field mvflash_4ch_regs[REG_MAX_COUNT] = {
>> +	REG_FIELD(0x06, 0, 7),			/* status1	*/
>> +	REG_FIELD(0x07, 0, 6),			/* status2	*/
>> +	REG_FIELD(0x09, 0, 7),			/* status3	*/
>> +	REG_FIELD_ID(0x3e, 0, 7, 4, 1),		/* chan_timer	*/
>> +	REG_FIELD_ID(0x42, 0, 6, 4, 1),		/* itarget	*/
>> +	REG_FIELD(0x46, 7, 7),			/* module_en	*/
>> +	REG_FIELD(0x49, 0, 3),			/* iresolution	*/
>> +	REG_FIELD_ID(0x4a, 0, 6, 4, 1),		/* chan_strobe	*/
>> +	REG_FIELD(0x4e, 0, 3),			/* chan_en	*/
>> +};
>> +
>> +struct qcom_flash_led {
>> +	struct qcom_flash_chip		*chip;
> 
> Not a fan of these interwoven references.  Where 'chip' has a reverence
> to 'led' and 'led' has a reference to 'chip'.
> 
>    chip->leds[0]->chip->leds[0]->chip ...
> 
> Either re-work your API (pass 'chip' and an LED index for example [this
> may very well not be the correct solution) or use something akin to
> container_of().
> 

Done.
'container_of()' won't work here since the 'leds' is a pointer and the 
buffer is allocated dynamically according to the number of the flas LED 
device-tree sub-nodes. I will figure out how to avoid the interwoven 
reference and update.

>> +	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;
>> +	struct regmap_field     *r_fields[REG_MAX_COUNT];
>> +	struct device		*dev;
>> +	struct mutex		lock;
>> +	enum hw_type		hw_type;
>> +	u8			leds_count;
>> +	u8			max_channels;
>> +	u8			chan_en_bits;
>> +};
>> +
>> +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;
>> +	rc = regmap_field_write(chip->r_fields[REG_MODULE_EN], val);
> 
> I'm a little confused by this.
> 
> So we go through the process of flipping individual enable bits, then
> trash all of that knowledge and end up writing an individual 0 or 1.
> 
> Why not replace the whole function with:
> 
>      regmap_field_write(chip->r_fields[REG_MODULE_EN], en);
> 

'REG_MODULE_EN" is basically an overall gating register for all 
channels. It needs to be enabled when any channel is enabled and it can 
only be disabled after all channels are disabled.

>> +	if (rc < 0)
>> +		dev_err(chip->dev, "write module_en failed, rc=%d\n", rc);
>> +	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;
> 
> Please make it clear what you're doing here.
> 
> I suggest you separate the allocation and the assignment and add a
> comment to the latter.
> 

Done.

>> +	u32 ires_ua = (mode == FLASH_MODE) ? FLASH_IRES_UA : TORCH_IRES_UA;
>> +	u8 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;
> 
> If you allocate 'val' in here and pre-assign it, you can negate the
> logic and omit this branch.  Unless this is going to be multi-use, 'val'
> is not a great name for a variable.
> 

Done.
I use 'val' to represent the variable is for caching the register value, 
I can update it to 'reg_val' or 'value'.

>> +		else
>> +			val = itarg_ua / ires_ua - 1;
>> +
>> +		rc = regmap_fields_write(chip->r_fields[REG_ITARGET], chan_id - 1, val);
> 
> Instead of '-1'ing every time you wish to use the value, why don't you
> start the numbering off at 0 instead?  If that's not possible, take 1
> off when you initially assign it.
> 

Done.

>> +		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));
>> +		}
> 
> ... else do something unexpected?
> 

Done, will print an error message here.

>> +	}
>> +
>> +	return regmap_field_update_bits(chip->r_fields[REG_IRESOLUTION], 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 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];
>> +		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;
> 
> Nit: This if doesn't have anything to do with the line above.  If you
> were to space them out (with a '\n') some sections would be easier to
> read.  Please also apply this logic to the rest of the file.
> 

Done.

>> +
>> +		rc = regmap_fields_write(chip->r_fields[REG_CHAN_TIMER], chan_id - 1, 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 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];
>> +		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);
>> +		rc = regmap_fields_write(chip->r_fields[REG_CHAN_STROBE], chan_id - 1, val);
>> +		if (rc < 0)
>> +			return rc;
>> +
>> +		chan_mask |= BIT(chan_id - 1);
>> +	}
>> +
>> +	/* enable/disable flash channels */
> 
> Nit: "Enable"
> 

Done.

>> +	mask = chan_mask;
> 
> Why can't you just use chan_mask?
> 

Done.

>> +	val = state ? mask : 0;
>> +	rc = regmap_field_update_bits(chip->r_fields[REG_CHAN_EN], 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);
> 
> No magic numbers please.
> 
> Can you define the 1000 or use a MACRO instead please?
> 
> Same throughout.
> 

Done.

>> +	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)
> 
> These can all be simplified to 'if (rc)'?
> 

Done.

>> +		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)
> 
> Why do you use the qcom_ prefix sometimes and not others?
> 

The functions with 'qcom_' prefix are mainly the function implementation 
of the hooks required by the framework/core driver.

The functions without 'qcom_' prefix are 'local' functions for HW 
operation and they are normally called inside the 'qcom_' prefix functions.

>> +{
>> +	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_field_read(chip->r_fields[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_field_read(chip->r_fields[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;
>> +	}
> 
> No magic numbers please.
> 

Done.

>> +	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_field_read(chip->r_fields[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 */
> 
> Nit: "Disable"
>

Done.


>> +	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)
> 
> This is an ugly way to break a line.  Suggest this:
> 
> static enum led_brightness
> qcom_flash_intensity_to_led_brightness(struct v4l2_flash *v4l2_flash, s32 intensity)
> 

Done.

>> +{
>> +	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;
> 
> Suggest some additional bracketing here.
> 

Done.

>> +}
>> +
>> +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};
> 
> Nit: Spaces around the 0 please.
> 

Done.

>> +	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;
> 
> This is a bool.
> 

Done.

>> +	v4l2_cfg.flash_faults = LED_FAULT_INPUT_VOLTAGE | LED_FAULT_OVER_CURRENT |
>> +		LED_FAULT_SHORT_CIRCUIT | LED_FAULT_OVER_TEMPERATURE | LED_FAULT_TIMEOUT;
> 
> 	v4l2_cfg.flash_faults = LED_FAULT_INPUT_VOLTAGE |
> 				LED_FAULT_OVER_CURRENT |
> 				LED_FAULT_SHORT_CIRCUIT |
> 				LED_FAULT_OVER_TEMPERATURE |
> 				LED_FAULT_TIMEOUT;
> 
> Don't you think that's nicer to read?
> 

Done.

>> +	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;
> 
> a) this has nothing to do with the following line, so you should
> separate it and b) I don't see any reason not to place the assignment
> on the allocate line.
> 
Done.

>> +	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);
> 
> "Failed to read led-max-microamp property"
> 

Done.

>> +		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);
> 
> As above.
> 

Done.

>> +			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' is not a good variable name.
> 
>> +		s->min = s->step = FLASH_IRES_UA * led->chan_count;
> 
> These get over-written before they are used.
> 
> Scrap that, I see that 's' gets reassigned.
> 
> Even more reason to change 's' for something else and use 2 variables
> instead of one to make this point clear.
> 
> A few comments spread around the complex areas wouldn't go amiss either.
> 

The 's' here is just a temporary pointer to simply the initial 
assignments to data members in 'flash->brightness' and 'flash->timeout' 
and both of them are 'struct flash_led_setting' type, so there won't be 
any places really use it.
I can update it and use 'brightness' and 'timeout' if you suggest to use 
2 variables.

>> +		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;
> 
> '\n'
> 

Done.

>> +	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;
>> +	struct regmap *map;
>> +	struct reg_field *regs;
>> +	int count, i, rc;
>> +	u32 val, reg_base;
>> +
>> +	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
>> +	if (!chip)
>> +		return -ENOMEM;
>> +
>> +	map = dev_get_regmap(dev->parent, NULL);
> 
> Can you use 'regmap' here please.
> 

Done.

>> +	if (!map) {
>> +		dev_err(dev, "Failed to get parent regmap\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	rc = fwnode_property_read_u32(dev->fwnode, "reg", &reg_base);
>> +	if (rc < 0) {
>> +		dev_err(dev, "Failed to get register base address, rc=%d\n", rc);
>> +		return rc;
>> +	}
>> +
>> +	rc = regmap_read(map, 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);
> 
> Can we be more specific?  Flash to me is a memory devices.
> 

Done.

>> +		return -ENODEV;
>> +	}
>> +
>> +	rc = regmap_read(map, 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;
>> +		regs = mvflash_3ch_regs;
>> +	} else if (val == FLASH_SUBTYPE_4CH_VAL) {
>> +		chip->hw_type = QCOM_MVFLASH_4CH;
>> +		chip->max_channels = 4;
>> +		regs = mvflash_4ch_regs;
>> +	} else {
>> +		dev_err(dev, "flash subtype %#x is not yet supported\n", val);
>> +		return -ENODEV;
>> +	}
>> +
>> +	for (i = 0; i < REG_MAX_COUNT; i++)
>> +		regs[i].reg += reg_base;
>> +
>> +	rc = devm_regmap_field_bulk_alloc(dev, map, chip->r_fields, regs, REG_MAX_COUNT);
>> +	if (rc < 0) {
>> +		dev_err(dev, "failed to alloc regmap filed, rc=%d\n", rc);
> 
> "Failed to allocate ..."
> 
> "field"
>

Done.

>> +		return rc;
>> +	}
>> +
>> +	chip->dev = dev;
>> +	platform_set_drvdata(pdev, chip);
>> +	mutex_init(&chip->lock);
> 
> '\n'
> 
> Lots of other places too - please refrain from smooshing code together.
> 

Done.

>> +	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;
> 
> '\n'
>

Done.

>> +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" },
>> +	{ }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, qcom_flash_led_match_table);
>> +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
>>
>

On Mon, 30 Jan 2023, Fenglin Wu wrote:

> Hi Jones,
> 
> Thanks for reviewing the driver!
> Replies inline.
> 
> 
> On 2023/1/26 21:23, Lee Jones wrote:
> > On Tue, 27 Dec 2022, 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>
> > > Tested-by: Luca Weiss <luca.weiss@fairphone.com> # sm7225-fairphone-fp4 + pm6150l
> > > ---
> > >   drivers/leds/flash/Kconfig           |  15 +
> > >   drivers/leds/flash/Makefile          |   1 +
> > >   drivers/leds/flash/leds-qcom-flash.c | 701 +++++++++++++++++++++++++++
> > >   3 files changed, 717 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..f36a60409290 100644
> > > --- a/drivers/leds/flash/Kconfig
> > > +++ b/drivers/leds/flash/Kconfig
> > > @@ -61,6 +61,21 @@ 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
> > > +	depends on LEDS_CLASS && OF
> > > +	depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
> > > +	select REGMAP
> > > +	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..3735282b77e9
> > > --- /dev/null
> > > +++ b/drivers/leds/flash/leds-qcom-flash.c
> > > @@ -0,0 +1,701 @@
> > > +// 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_TIMER_EN_BIT		BIT(7)
> > > +#define FLASH_TIMER_VAL_MASK		GENMASK(6, 0)
> > > +#define FLASH_TIMER_STEP_MS		10
> > > +
> > > +#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,
> > > +};
> > > +
> > > +enum {
> > > +	REG_STATUS1,
> > > +	REG_STATUS2,
> > > +	REG_STATUS3,
> > > +	REG_CHAN_TIMER,
> > > +	REG_ITARGET,
> > > +	REG_MODULE_EN,
> > > +	REG_IRESOLUTION,
> > > +	REG_CHAN_STROBE,
> > > +	REG_CHAN_EN,
> > > +	REG_MAX_COUNT,
> > > +};
> > > +
> > > +struct reg_field mvflash_3ch_regs[REG_MAX_COUNT] = {
> > > +	REG_FIELD(0x08, 0, 7),			/* status1	*/
> > > +	REG_FIELD(0x09, 0, 7),                  /* status2	*/
> > > +	REG_FIELD(0x0a, 0, 7),                  /* status3	*/
> > > +	REG_FIELD_ID(0x40, 0, 7, 3, 1),         /* chan_timer	*/
> > > +	REG_FIELD_ID(0x43, 0, 6, 3, 1),         /* itarget	*/
> > > +	REG_FIELD(0x46, 7, 7),                  /* module_en	*/
> > > +	REG_FIELD(0x47, 0, 5),                  /* iresolution	*/
> > > +	REG_FIELD_ID(0x49, 0, 2, 3, 1),         /* chan_strobe	*/
> > > +	REG_FIELD(0x4c, 0, 2),                  /* chan_en	*/
> > > +};
> > > +
> > > +struct reg_field mvflash_4ch_regs[REG_MAX_COUNT] = {
> > > +	REG_FIELD(0x06, 0, 7),			/* status1	*/
> > > +	REG_FIELD(0x07, 0, 6),			/* status2	*/
> > > +	REG_FIELD(0x09, 0, 7),			/* status3	*/
> > > +	REG_FIELD_ID(0x3e, 0, 7, 4, 1),		/* chan_timer	*/
> > > +	REG_FIELD_ID(0x42, 0, 6, 4, 1),		/* itarget	*/
> > > +	REG_FIELD(0x46, 7, 7),			/* module_en	*/
> > > +	REG_FIELD(0x49, 0, 3),			/* iresolution	*/
> > > +	REG_FIELD_ID(0x4a, 0, 6, 4, 1),		/* chan_strobe	*/
> > > +	REG_FIELD(0x4e, 0, 3),			/* chan_en	*/
> > > +};
> > > +
> > > +struct qcom_flash_led {
> > > +	struct qcom_flash_chip		*chip;
> > 
> > Not a fan of these interwoven references.  Where 'chip' has a reverence
> > to 'led' and 'led' has a reference to 'chip'.
> > 
> >    chip->leds[0]->chip->leds[0]->chip ...
> > 
> > Either re-work your API (pass 'chip' and an LED index for example [this
> > may very well not be the correct solution) or use something akin to
> > container_of().
> > 
> 
> Done.
> 'container_of()' won't work here since the 'leds' is a pointer and the
> buffer is allocated dynamically according to the number of the flas LED
> device-tree sub-nodes. I will figure out how to avoid the interwoven
> reference and update.

Which is why I said "akin to".

> > > +	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;
> > > +	struct regmap_field     *r_fields[REG_MAX_COUNT];
> > > +	struct device		*dev;
> > > +	struct mutex		lock;
> > > +	enum hw_type		hw_type;
> > > +	u8			leds_count;
> > > +	u8			max_channels;
> > > +	u8			chan_en_bits;
> > > +};
> > > +
> > > +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;
> > > +	rc = regmap_field_write(chip->r_fields[REG_MODULE_EN], val);
> > 
> > I'm a little confused by this.
> > 
> > So we go through the process of flipping individual enable bits, then
> > trash all of that knowledge and end up writing an individual 0 or 1.
> > 
> > Why not replace the whole function with:
> > 
> >      regmap_field_write(chip->r_fields[REG_MODULE_EN], en);

I see, so chan_en_bits is the cache to all enabled channels.

> 'REG_MODULE_EN" is basically an overall gating register for all channels. It
> needs to be enabled when any channel is enabled and it can only be disabled
> after all channels are disabled.
> 
> > > +	if (rc < 0)
> > > +		dev_err(chip->dev, "write module_en failed, rc=%d\n", rc);
> > > +	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;
> > 
> > Please make it clear what you're doing here.
> > 
> > I suggest you separate the allocation and the assignment and add a
> > comment to the latter.
> > 
> 
> Done.

Please snip replies.  You only need to provide responses for items
you're providing more info about.  It takes time to go read all of the
'done' comments that do not add value.

> > > +	/*
> > > +	 * 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;
> > 
> > If you allocate 'val' in here and pre-assign it, you can negate the
> > logic and omit this branch.  Unless this is going to be multi-use, 'val'
> > is not a great name for a variable.
> > 
> 
> Done.
> I use 'val' to represent the variable is for caching the register value, I
> can update it to 'reg_val' or 'value'.

Those are equally terrible variable names.

 What is the name of the register you're writing to?

 What does it actually 'do'?

 What happens when that is written?

 What *is* the value?

'shared_ua` perhaps?  Not data, reg, or value please.

> > > +		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' is not a good variable name.
> > 
> > > +		s->min = s->step = FLASH_IRES_UA * led->chan_count;
> > 
> > These get over-written before they are used.
> > 
> > Scrap that, I see that 's' gets reassigned.
> > 
> > Even more reason to change 's' for something else and use 2 variables
> > instead of one to make this point clear.
> > 
> > A few comments spread around the complex areas wouldn't go amiss either.
> > 
> 
> The 's' here is just a temporary pointer to simply the initial assignments
> to data members in 'flash->brightness' and 'flash->timeout' and both of them
> are 'struct flash_led_setting' type, so there won't be any places really use
> it.
> I can update it and use 'brightness' and 'timeout' if you suggest to use 2
> variables.

It just makes everything so much nicer to read if you use proper
human-friendly names for things.

On 2023/1/30 19:56, Lee Jones wrote:
> On Mon, 30 Jan 2023, Fenglin Wu wrote:
> 
>> Hi Jones,
>>
>> Thanks for reviewing the driver!
>> Replies inline.
>>
>>
>> On 2023/1/26 21:23, Lee Jones wrote:
>>> On Tue, 27 Dec 2022, 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>
>>>> Tested-by: Luca Weiss <luca.weiss@fairphone.com> # sm7225-fairphone-fp4 + pm6150l
>>>> ---
>>>>    drivers/leds/flash/Kconfig           |  15 +
>>>>    drivers/leds/flash/Makefile          |   1 +
>>>>    drivers/leds/flash/leds-qcom-flash.c | 701 +++++++++++++++++++++++++++
>>>>    3 files changed, 717 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..f36a60409290 100644
>>>> --- a/drivers/leds/flash/Kconfig
>>>> +++ b/drivers/leds/flash/Kconfig
>>>> @@ -61,6 +61,21 @@ 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
>>>> +	depends on LEDS_CLASS && OF
>>>> +	depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
>>>> +	select REGMAP
>>>> +	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..3735282b77e9
>>>> --- /dev/null
>>>> +++ b/drivers/leds/flash/leds-qcom-flash.c
>>>> @@ -0,0 +1,701 @@
>>>> +// 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_TIMER_EN_BIT		BIT(7)
>>>> +#define FLASH_TIMER_VAL_MASK		GENMASK(6, 0)
>>>> +#define FLASH_TIMER_STEP_MS		10
>>>> +
>>>> +#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,
>>>> +};
>>>> +
>>>> +enum {
>>>> +	REG_STATUS1,
>>>> +	REG_STATUS2,
>>>> +	REG_STATUS3,
>>>> +	REG_CHAN_TIMER,
>>>> +	REG_ITARGET,
>>>> +	REG_MODULE_EN,
>>>> +	REG_IRESOLUTION,
>>>> +	REG_CHAN_STROBE,
>>>> +	REG_CHAN_EN,
>>>> +	REG_MAX_COUNT,
>>>> +};
>>>> +
>>>> +struct reg_field mvflash_3ch_regs[REG_MAX_COUNT] = {
>>>> +	REG_FIELD(0x08, 0, 7),			/* status1	*/
>>>> +	REG_FIELD(0x09, 0, 7),                  /* status2	*/
>>>> +	REG_FIELD(0x0a, 0, 7),                  /* status3	*/
>>>> +	REG_FIELD_ID(0x40, 0, 7, 3, 1),         /* chan_timer	*/
>>>> +	REG_FIELD_ID(0x43, 0, 6, 3, 1),         /* itarget	*/
>>>> +	REG_FIELD(0x46, 7, 7),                  /* module_en	*/
>>>> +	REG_FIELD(0x47, 0, 5),                  /* iresolution	*/
>>>> +	REG_FIELD_ID(0x49, 0, 2, 3, 1),         /* chan_strobe	*/
>>>> +	REG_FIELD(0x4c, 0, 2),                  /* chan_en	*/
>>>> +};
>>>> +
>>>> +struct reg_field mvflash_4ch_regs[REG_MAX_COUNT] = {
>>>> +	REG_FIELD(0x06, 0, 7),			/* status1	*/
>>>> +	REG_FIELD(0x07, 0, 6),			/* status2	*/
>>>> +	REG_FIELD(0x09, 0, 7),			/* status3	*/
>>>> +	REG_FIELD_ID(0x3e, 0, 7, 4, 1),		/* chan_timer	*/
>>>> +	REG_FIELD_ID(0x42, 0, 6, 4, 1),		/* itarget	*/
>>>> +	REG_FIELD(0x46, 7, 7),			/* module_en	*/
>>>> +	REG_FIELD(0x49, 0, 3),			/* iresolution	*/
>>>> +	REG_FIELD_ID(0x4a, 0, 6, 4, 1),		/* chan_strobe	*/
>>>> +	REG_FIELD(0x4e, 0, 3),			/* chan_en	*/
>>>> +};
>>>> +
>>>> +struct qcom_flash_led {
>>>> +	struct qcom_flash_chip		*chip;
>>>
>>> Not a fan of these interwoven references.  Where 'chip' has a reverence
>>> to 'led' and 'led' has a reference to 'chip'.
>>>
>>>     chip->leds[0]->chip->leds[0]->chip ...
>>>
>>> Either re-work your API (pass 'chip' and an LED index for example [this
>>> may very well not be the correct solution) or use something akin to
>>> container_of().
>>>
>>
>> Done.
>> 'container_of()' won't work here since the 'leds' is a pointer and the
>> buffer is allocated dynamically according to the number of the flas LED
>> device-tree sub-nodes. I will figure out how to avoid the interwoven
>> reference and update.
> 
> Which is why I said "akin to".
> 

I thought about other options but I couldn't find a very good approach 
to hand this.

Updating API couldn't work either because the "chip" was needed mainly 
in the "led_flash_ops" function hooks and all of them are function 
pointers with fixed parameters. The only parameter can be potentially 
useful is "led_classdev_flash" or "led_classdev" type, but there is no 
private "data"  section in "struct led_classdev_flash" or "struct 
led_classdev" can be used to cache the "chip" pointer, and it's not 
possible to get the "chip" data by using "container_of".

An alternate way not referring "chip" directly in "struct 
qcom_flash_led" is, add a "void" data pointer in "struct qcom_flash_led" 
and name it as "priv_data", and assign "chip" to it during the 
initialization, and cast it back to "struct qcom_flash_chip" when using 
it. How do you think about this way? Or do you have any better idea?
Thanks

>>>> +	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;
>>>> +	struct regmap_field     *r_fields[REG_MAX_COUNT];
>>>> +	struct device		*dev;
>>>> +	struct mutex		lock;
>>>> +	enum hw_type		hw_type;
>>>> +	u8			leds_count;
>>>> +	u8			max_channels;
>>>> +	u8			chan_en_bits;
>>>> +};
>>>> +
>>>> +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;
>>>> +	rc = regmap_field_write(chip->r_fields[REG_MODULE_EN], val);
>>>
>>> I'm a little confused by this.
>>>
>>> So we go through the process of flipping individual enable bits, then
>>> trash all of that knowledge and end up writing an individual 0 or 1.
>>>
>>> Why not replace the whole function with:
>>>
>>>       regmap_field_write(chip->r_fields[REG_MODULE_EN], en);
> 
> I see, so chan_en_bits is the cache to all enabled channels.
> 
>> 'REG_MODULE_EN" is basically an overall gating register for all channels. It
>> needs to be enabled when any channel is enabled and it can only be disabled
>> after all channels are disabled.
>>
>>>> +	if (rc < 0)
>>>> +		dev_err(chip->dev, "write module_en failed, rc=%d\n", rc);
>>>> +	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;
>>>
>>> Please make it clear what you're doing here.
>>>
>>> I suggest you separate the allocation and the assignment and add a
>>> comment to the latter.
>>>
>>
>> Done.
> 
> Please snip replies.  You only need to provide responses for items
> you're providing more info about.  It takes time to go read all of the
> 'done' comments that do not add value.
> 
>>>> +	/*
>>>> +	 * 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;
>>>
>>> If you allocate 'val' in here and pre-assign it, you can negate the
>>> logic and omit this branch.  Unless this is going to be multi-use, 'val'
>>> is not a great name for a variable.
>>>
>>
>> Done.
>> I use 'val' to represent the variable is for caching the register value, I
>> can update it to 'reg_val' or 'value'.
> 
> Those are equally terrible variable names.
> 
>   What is the name of the register you're writing to?
> 
>   What does it actually 'do'?
> 
>   What happens when that is written?
> 
>   What *is* the value?
> 
> 'shared_ua` perhaps?  Not data, reg, or value please.
> 
>>>> +		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' is not a good variable name.
>>>
>>>> +		s->min = s->step = FLASH_IRES_UA * led->chan_count;
>>>
>>> These get over-written before they are used.
>>>
>>> Scrap that, I see that 's' gets reassigned.
>>>
>>> Even more reason to change 's' for something else and use 2 variables
>>> instead of one to make this point clear.
>>>
>>> A few comments spread around the complex areas wouldn't go amiss either.
>>>
>>
>> The 's' here is just a temporary pointer to simply the initial assignments
>> to data members in 'flash->brightness' and 'flash->timeout' and both of them
>> are 'struct flash_led_setting' type, so there won't be any places really use
>> it.
>> I can update it and use 'brightness' and 'timeout' if you suggest to use 2
>> variables.
> 
> It just makes everything so much nicer to read if you use proper
> human-friendly names for things.
>