Re: [PATCH v2 1/2] leds: flash: add driver to support flash LED module in QCOM PMICs

[Fenglin Wu <quic_fenglinw@quicinc.com>]

On 2022/9/29 20:23, Krzysztof Kozlowski wrote:
> On 29/09/2022 14:15, Fenglin Wu wrote:
>> Add initial driver to support flash LED module found in Qualcomm
>> Technologies, Inc. PMICs. The flash module can have 3 or 4 channels
>> and each channel can be controlled indepedently and support full scale
>> current up to 1.5 A. It also supports connecting two channels together
>> to supply one LED component with full scale current up to 2 A. In that
>> case, the current will be split on each channel symmetrically and the
>> channels will be enabled and disabled at the same time.
>>
>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>> ---
>>   drivers/leds/flash/Kconfig           |  14 +
>>   drivers/leds/flash/Makefile          |   1 +
>>   drivers/leds/flash/leds-qcom-flash.c | 707 +++++++++++++++++++++++++++
>>   3 files changed, 722 insertions(+)
>>   create mode 100644 drivers/leds/flash/leds-qcom-flash.c
>>
>> diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
>> index d3eb689b193c..92773fa872dc 100644
>> --- a/drivers/leds/flash/Kconfig
>> +++ b/drivers/leds/flash/Kconfig
>> @@ -61,6 +61,20 @@ config LEDS_MT6360
>>   	  Independent current sources supply for each flash LED support torch
>>   	  and strobe mode.
>>   
>> +config LEDS_QCOM_FLASH
>> +	tristate "LED support for flash module inside Qualcomm Technologies, Inc. PMIC"
>> +	depends on MFD_SPMI_PMIC
> 
> || COMPILE_TEST
> 
> (and actually test it, e.g. you might need here "select REGMAP")
> 
I will add "COMPILE_TEST" here and compile with with "COMPILE_TEST" enabled.
>> +	depends on LEDS_CLASS && OF
>> +	depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
>> +	help
>> +	  This option enables support for the flash module found in Qualcomm
>> +	  Technologies, Inc. PMICs. The flash module can have 3 or 4 flash LED
>> +	  channels and each channel is programmable to support up to 1.5 A full
>> +	  scale current. It also supports connecting two channels' output together
>> +	  to supply one LED component to achieve current up to 2 A. In such case,
>> +	  the total LED current will be split symmetrically on each channel and
>> +	  they will be enabled/disabled at the same time.
>> +
> 
>>   config LEDS_RT4505
>>   	tristate "LED support for RT4505 flashlight controller"
>>   	depends on I2C && OF
>> diff --git a/drivers/leds/flash/Makefile b/drivers/leds/flash/Makefile
>> index 0acbddc0b91b..8a60993f1a25 100644
>> --- a/drivers/leds/flash/Makefile
>> +++ b/drivers/leds/flash/Makefile
>> @@ -6,6 +6,7 @@ obj-$(CONFIG_LEDS_AS3645A)	+= leds-as3645a.o
>>   obj-$(CONFIG_LEDS_KTD2692)	+= leds-ktd2692.o
>>   obj-$(CONFIG_LEDS_LM3601X)	+= leds-lm3601x.o
>>   obj-$(CONFIG_LEDS_MAX77693)	+= leds-max77693.o
>> +obj-$(CONFIG_LEDS_QCOM_FLASH)	+= leds-qcom-flash.o
>>   obj-$(CONFIG_LEDS_RT4505)	+= leds-rt4505.o
>>   obj-$(CONFIG_LEDS_RT8515)	+= leds-rt8515.o
>>   obj-$(CONFIG_LEDS_SGM3140)	+= leds-sgm3140.o
>> diff --git a/drivers/leds/flash/leds-qcom-flash.c b/drivers/leds/flash/leds-qcom-flash.c
>> new file mode 100644
>> index 000000000000..7b941eb769fe
>> --- /dev/null
>> +++ b/drivers/leds/flash/leds-qcom-flash.c
>> @@ -0,0 +1,707 @@
>> +//SPDX-License-Identifier: GPL-2.0-only
> 
> Missing space after //
Thanks for catching it, will fix it.
> 
>> +/*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/bits.h>
>> +#include <linux/leds.h>
>> +#include <linux/led-class-flash.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/property.h>
>> +#include <linux/regmap.h>
>> +#include <media/v4l2-flash-led-class.h>
>> +
>> +/* registers definitions */
>> +#define FLASH_TYPE_REG			0x04
>> +#define FLASH_TYPE_VAL			0x18
>> +
>> +#define FLASH_SUBTYPE_REG		0x05
>> +#define FLASH_SUBTYPE_3CH_VAL		0x04
>> +#define FLASH_SUBTYPE_4CH_VAL		0x07
>> +
>> +#define FLASH_MODULE_EN_BIT		BIT(7)
>> +
>> +#define FLASH_TIMER_EN_BIT		BIT(7)
>> +#define FLASH_TIMER_VAL_MASK		GENMASK(6, 0)
>> +#define FLASH_TIMER_STEP_MS		10
>> +
>> +#define FLASH_ITARGET_CURRENT_MASK	GENMASK(6, 0)
>> +
>> +#define FLASH_STROBE_HW_SW_SEL_BIT	BIT(2)
>> +#define SW_STROBE_VAL			0
>> +#define HW_STROBE_VAL			1
>> +#define FLASH_HW_STROBE_TRIGGER_SEL_BIT	BIT(1)
>> +#define STROBE_LEVEL_TRIGGER_VAL	0
>> +#define STROBE_EDGE_TRIGGER_VAL		1
>> +#define FLASH_STROBE_POLARITY_BIT	BIT(0)
>> +#define STROBE_ACTIVE_HIGH_VAL		1
>> +
>> +#define FLASH_IRES_MASK_4CH		BIT(0)
>> +#define FLASH_IRES_MASK_3CH		GENMASK(1, 0)
>> +#define FLASH_IRES_12P5MA_VAL		0
>> +#define FLASH_IRES_5MA_VAL_4CH		1
>> +#define FLASH_IRES_5MA_VAL_3CH		3
>> +
>> +/* constants */
>> +#define FLASH_CURRENT_MAX_UA		1500000
>> +#define TORCH_CURRENT_MAX_UA		500000
>> +#define FLASH_TOTAL_CURRENT_MAX_UA	2000000
>> +#define FLASH_CURRENT_DEFAULT_UA	1000000
>> +#define TORCH_CURRENT_DEFAULT_UA	200000
>> +
>> +#define TORCH_IRES_UA			5000
>> +#define FLASH_IRES_UA			12500
>> +
>> +#define FLASH_TIMEOUT_MAX_US		1280000
>> +#define FLASH_TIMEOUT_STEP_US		10000
>> +
>> +enum hw_type {
>> +	QCOM_MVFLASH_3CH,
>> +	QCOM_MVFLASH_4CH,
>> +};
>> +
>> +enum led_mode {
>> +	FLASH_MODE,
>> +	TORCH_MODE,
>> +};
>> +
>> +enum led_strobe {
>> +	SW_STROBE,
>> +	HW_STROBE,
>> +};
>> +
>> +struct qcom_flash_reg {
>> +	u8 module_en;
>> +	u8 chan_timer;
>> +	u8 itarget;
>> +	u8 iresolution;
>> +	u8 chan_strobe;
>> +	u8 chan_en;
>> +	u8 status1;
>> +	u8 status2;
>> +	u8 status3;
>> +};
>> +
>> +struct qcom_flash_led {
>> +	struct qcom_flash_chip		*chip;
>> +	struct led_classdev_flash	flash;
>> +	struct v4l2_flash		*v4l2_flash;
>> +	u32				max_flash_current_ma;
>> +	u32				max_torch_current_ma;
>> +	u32				max_timeout_ms;
>> +	u32				flash_current_ma;
>> +	u32				flash_timeout_ms;
>> +	u8				*chan_id;
>> +	u8				chan_count;
>> +	bool				enabled;
>> +};
>> +
>> +struct qcom_flash_chip {
>> +	struct qcom_flash_led		*leds;
>> +	const struct qcom_flash_reg	*reg;
>> +	struct device			*dev;
>> +	struct regmap			*regmap;
>> +	struct mutex			lock;
>> +	enum hw_type			hw_type;
>> +	u32				reg_base;
>> +	u8				leds_count;
>> +	u8				max_channels;
>> +	u8				chan_en_bits;
>> +};
>> +
>> +static const struct qcom_flash_reg mvflash_3ch_reg = {
>> +	.chan_timer	= 0x40,
>> +	.itarget	= 0x43,
>> +	.module_en	= 0x46,
>> +	.iresolution	= 0x47,
>> +	.chan_strobe	= 0x49,
>> +	.chan_en	= 0x4c,
>> +	.status1	= 0x08,
>> +	.status2	= 0x09,
>> +	.status3	= 0x0a,
>> +};
>> +
>> +static const struct qcom_flash_reg mvflash_4ch_reg = {
>> +	.chan_timer	= 0x3e,
>> +	.itarget	= 0x42,
>> +	.module_en	= 0x46,
>> +	.iresolution	= 0x49,
>> +	.chan_strobe	= 0x4a,
>> +	.chan_en	= 0x4e,
>> +	.status1	= 0x06,
>> +	.status2	= 0x07,
>> +	.status3	= 0x09,
> 
> Don't reinvent the wheel. Use regmap fields.
> 

Do you mean defining regmap_filed pointer in struct qcom_flash_chip
for all these registers instead of creating the data structure 
qcom_flash_reg to include all the registers? Similar like this

struct qcom_flash_chip {
	....
         struct regmap_field     *chan_timer;
         struct regmap_field     *chan_timer;
         struct regmap_field     *itarget;
         struct regmap_field     *iresolution;
         struct regmap_field     *chan_strobe;
         struct regmap_field     *chan_en;
         struct regmap_field     *status1;
         struct regmap_field     *status2;
         struct regmap_field     *status3;
	....
};

This won't make the code cleaner actually. If this is the preferred way, 
I will update it accordingly.
Thanks


>> +};
>> +
>> +static int __set_flash_module_en(struct qcom_flash_led *led, bool en)
> 
> Drop __ prefix here and in other functions.
> 
> (...)
Done
> 
>> +
>> +static int qcom_flash_led_remove(struct platform_device *pdev)
>> +{
>> +	struct qcom_flash_chip *chip = platform_get_drvdata(pdev);
>> +
>> +	while (chip->leds_count--)
>> +		v4l2_flash_release(chip->leds[chip->leds_count].v4l2_flash);
>> +
>> +	mutex_destroy(&chip->lock);
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id qcom_flash_led_match_table[] = {
>> +	{ .compatible = "qcom,spmi-flash-led" },
> 
> Only this one is needed. Remove the rest:
Done
> 
>> +	{ .compatible = "qcom,pm8150c-flash-led" },
>> +	{ .compatible = "qcom,pm8150l-flash-led" },
>> +	{ .compatible = "qcom,pm8350c-flash-led" },
> 
> 
> 
> Best regards,
> Krzysztof
>