Date:   Tue, 31 Jan 2023 15:38:26 +0800
From:   Fenglin Wu <quic_fenglinw@...cinc.com>
To:     Lee Jones <lee@...nel.org>
CC:     <linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <pavel@....cz>, <krzysztof.kozlowski@...aro.org>,
        Gene Chen <gene_chen@...htek.com>,
        Jacek Anaszewski <jacek.anaszewski@...il.com>,
        <linux-leds@...r.kernel.org>, <quic_collinsd@...cinc.com>,
        <quic_subbaram@...cinc.com>, Luca Weiss <luca.weiss@...rphone.com>
Subject: Re: [RESEND PATCH v5 1/2] leds: flash: add driver to support flash
 LED module in QCOM PMICs



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@...cinc.com>
>>>> Tested-by: Luca Weiss <luca.weiss@...rphone.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.
> 

Powered by blists - more mailing lists