Re: [PATCH/RFC v10 08/19] leds: Add support for max77693 mfd flash cell

[Jacek Anaszewski <j.anaszewski@samsung.com>]

Hi Sakari,

Thanks for the review.

On 02/05/2015 04:34 PM, Sakari Ailus wrote:
> Hi Jacek,
>
> A few comments below.
>
> On Fri, Jan 09, 2015 at 04:22:58PM +0100, Jacek Anaszewski wrote:
>> This patch adds led-flash support to Maxim max77693 chipset.
>> A device can be exposed to user space through LED subsystem
>> sysfs interface. Device supports up to two leds which can
>> work in flash and torch mode. The leds can be triggered
>> externally or by software.
>>
>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> Cc: Bryan Wu <cooloney@gmail.com>
>> Cc: Richard Purdie <rpurdie@rpsys.net>
>> Cc: Lee Jones <lee.jones@linaro.org>
>> Cc: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>   drivers/leds/Kconfig         |   10 +
>>   drivers/leds/Makefile        |    1 +
>>   drivers/leds/leds-max77693.c | 1045 ++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 1056 insertions(+)
>>   create mode 100644 drivers/leds/leds-max77693.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 95029df..ff9c21b 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -464,6 +464,16 @@ config LEDS_TCA6507
>>   	  LED driver chips accessed via the I2C bus.
>>   	  Driver support brightness control and hardware-assisted blinking.
>>
>> +config LEDS_MAX77693
>> +	tristate "LED support for MAX77693 Flash"
>> +	depends on LEDS_CLASS_FLASH
>> +	depends on MFD_MAX77693
>> +	depends on OF
>> +	help
>> +	  This option enables support for the flash part of the MAX77693
>> +	  multifunction device. It has build in control for two leds in flash
>> +	  and torch mode.
>> +
>>   config LEDS_MAX8997
>>   	tristate "LED support for MAX8997 PMIC"
>>   	depends on LEDS_CLASS && MFD_MAX8997
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index cbba921..57ca62b 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -52,6 +52,7 @@ obj-$(CONFIG_LEDS_MC13783)		+= leds-mc13783.o
>>   obj-$(CONFIG_LEDS_NS2)			+= leds-ns2.o
>>   obj-$(CONFIG_LEDS_NETXBIG)		+= leds-netxbig.o
>>   obj-$(CONFIG_LEDS_ASIC3)		+= leds-asic3.o
>> +obj-$(CONFIG_LEDS_MAX77693)		+= leds-max77693.o
>>   obj-$(CONFIG_LEDS_MAX8997)		+= leds-max8997.o
>>   obj-$(CONFIG_LEDS_LM355x)		+= leds-lm355x.o
>>   obj-$(CONFIG_LEDS_BLINKM)		+= leds-blinkm.o
>> diff --git a/drivers/leds/leds-max77693.c b/drivers/leds/leds-max77693.c
>> new file mode 100644
>> index 0000000..3ba07c4
>> --- /dev/null
>> +++ b/drivers/leds/leds-max77693.c
>> @@ -0,0 +1,1045 @@
>> +/*
>> + * LED Flash class driver for the flash cell of max77693 mfd.
>> + *
>> + *	Copyright (C) 2015, Samsung Electronics Co., Ltd.
>> + *
>> + *	Authors: Jacek Anaszewski <j.anaszewski@samsung.com>
>> + *		 Andrzej Hajda <a.hajda@samsung.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + */
>> +
>> +#include <asm/div64.h>
>> +#include <linux/led-class-flash.h>
>> +#include <linux/mfd/max77693.h>
>> +#include <linux/mfd/max77693-private.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/workqueue.h>
>> +
>> +#define MODE_OFF		0
>> +#define MODE_FLASH(a)		(1 << (a))
>> +#define MODE_TORCH(a)		(1 << (2 + (a)))
>> +#define MODE_FLASH_EXTERNAL(a)	(1 << (4 + (a)))
>> +
>> +#define MODE_FLASH_MASK		(MODE_FLASH(FLED1) | MODE_FLASH(FLED2) | \
>> +				 MODE_FLASH_EXTERNAL(FLED1) | \
>> +				 MODE_FLASH_EXTERNAL(FLED2))
>> +#define MODE_TORCH_MASK		(MODE_TORCH(FLED1) | MODE_TORCH(FLED2))
>> +
>> +#define FLED1_IOUT		(1 << 0)
>> +#define FLED2_IOUT		(1 << 1)
>> +
>> +enum max77693_fled {
>> +	FLED1,
>> +	FLED2,
>> +};
>> +
>> +enum max77693_led_mode {
>> +	FLASH,
>> +	TORCH,
>> +};
>> +
>> +struct max77693_sub_led {
>> +	/* related FLED output identifier */
>> +	int fled_id;
>> +	/* related LED Flash class device */
>> +	struct led_classdev_flash fled_cdev;
>> +	/* assures led-triggers compatibility */
>> +	struct work_struct work_brightness_set;
>> +
>> +	/* brightness cache */
>> +	unsigned int torch_brightness;
>> +	/* flash timeout cache */
>> +	unsigned int flash_timeout;
>> +};
>> +
>> +struct max77693_led_device {
>> +	/* parent mfd regmap */
>> +	struct regmap *regmap;
>> +	/* platform device data */
>> +	struct platform_device *pdev;
>> +	/* configuration data for the device */
>> +	struct max77693_led_config_data *cfg_data;
>
> Where is this defined?

In the file include/linux/mfd/max77693.h. The original
struct max77693_led_platform_data was defined there
and one of the preceding patches in this patch set renames it.

>> +	/* secures access to the device */
>> +	struct mutex lock;
>> +
>> +	/* sub led data */
>> +	struct max77693_sub_led sub_leds[2];
>> +
>> +	/* current flash timeout cache */
>> +	unsigned int current_flash_timeout;
>> +	/* ITORCH register cache */
>> +	u8 torch_iout_reg;
>> +	/* mode of fled outputs */
>> +	unsigned int mode_flags;
>> +	/* recently strobed fled */
>> +	int strobing_sub_led_id;
>> +	/* bitmask of fled outputs use state (bit 0. - FLED1, bit 1. - FLED2) */
>> +	u8 fled_mask;
>> +	/* fled modes that can be set */
>> +	u8 allowed_modes;
>> +
>> +	/* arrangement of current outputs */
>> +	bool iout_joint;
>> +};
>> +
>> +struct max77693_led_settings {
>> +	struct led_flash_setting torch_brightness;
>> +	struct led_flash_setting flash_brightness;
>> +	struct led_flash_setting flash_timeout;
>> +};
>> +
>> +static u8 max77693_led_iout_to_reg(u32 ua)
>> +{
>> +	if (ua < FLASH_IOUT_MIN)
>> +		ua = FLASH_IOUT_MIN;
>> +	return (ua - FLASH_IOUT_MIN) / FLASH_IOUT_STEP;
>> +}
>> +
>> +static u8 max77693_flash_timeout_to_reg(u32 us)
>> +{
>> +	return (us - FLASH_TIMEOUT_MIN) / FLASH_TIMEOUT_STEP;
>> +}
>> +
>> +static inline struct max77693_sub_led *flcdev_to_sub_led(
>> +					struct led_classdev_flash *fled_cdev)
>> +{
>> +	return container_of(fled_cdev, struct max77693_sub_led, fled_cdev);
>> +}
>> +
>> +static inline struct max77693_led_device *sub_led_to_led(
>> +					struct max77693_sub_led *sub_led)
>> +{
>> +	return container_of(sub_led, struct max77693_led_device,
>> +				sub_leds[sub_led->fled_id]);
>> +}
>> +
>> +static inline u8 max77693_led_vsys_to_reg(u32 mv)
>> +{
>> +	return ((mv - MAX_FLASH1_VSYS_MIN) / MAX_FLASH1_VSYS_STEP) << 2;
>> +}
>> +
>> +static inline u8 max77693_led_vout_to_reg(u32 mv)
>> +{
>> +	return (mv - FLASH_VOUT_MIN) / FLASH_VOUT_STEP + FLASH_VOUT_RMIN;
>> +}
>> +
>> +static inline bool max77693_fled_used(struct max77693_led_device *led,
>> +					 int fled_id)
>> +{
>> +	u8 fled_bit = (fled_id == FLED1) ? FLED1_IOUT : FLED2_IOUT;
>> +
>> +	return led->fled_mask & fled_bit;
>> +}
>> +
>> +/* split composite current @i into two @iout according to @imax weights */
>
> Do you still consider this as the right thing to do?
>
> If there's a single LED driven by multiple outputs, what is the value
> provided by board-specific current distribution between these outputs?

This is required for being able to set odd current levels, in case
one led is connected to both outputs. The currents are still being set
separately for both outputs. For example, if we want to set LED
subsystem brightness level == 3, then we have to set e.g. register
MAX77693_LED_REG_IFLASH1 to 0 and MAX77693_LED_REG_IFLASH1 to 1
(where levels start from 0 (15.625mA) in the max77693-led device).

I think that we've agreed about this on #v4l.

>> +static void __max77693_calc_iout(u32 iout[2], u32 i, u32 imax[2])
>> +{
>> +	u64 t = i;
>> +
>> +	t *= imax[1];
>> +	do_div(t, imax[0] + imax[1]);
>> +
>> +	iout[1] = (u32)t / FLASH_IOUT_STEP * FLASH_IOUT_STEP;
>> +	iout[0] = i - iout[1];
>> +}
>> +
>> +static int max77693_set_mode(struct max77693_led_device *led, u8 mode)
>> +{
>> +	struct regmap *rmap = led->regmap;
>> +	int ret, v = 0, i;
>> +
>> +	for (i = FLED1; i <= FLED2; ++i) {
>> +		if (mode & MODE_TORCH(i))
>> +			v |= FLASH_EN_ON << TORCH_EN_SHIFT(i);
>> +
>> +		if (mode & MODE_FLASH(i)) {
>> +			v |= FLASH_EN_ON << FLASH_EN_SHIFT(i);
>> +		} else if (mode & MODE_FLASH_EXTERNAL(i)) {
>> +			v |= FLASH_EN_FLASH << FLASH_EN_SHIFT(i);
>> +			/*
>> +			 * Enable hw triggering also for torch mode, as some
>> +			 * camera sensors use torch led to fathom ambient light
>> +			 * conditions before strobing the flash.
>> +			 */
>> +			v |= FLASH_EN_TORCH << TORCH_EN_SHIFT(i);
>> +		}
>> +	}
>> +
>> +	/* Reset the register only prior setting flash modes */
>> +	if (mode & ~(MODE_TORCH(FLED1) | MODE_TORCH(FLED2))) {
>> +		ret = regmap_write(rmap, MAX77693_LED_REG_FLASH_EN, 0);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	return regmap_write(rmap, MAX77693_LED_REG_FLASH_EN, v);
>> +}
>> +
>> +static void max77693_set_sync_strobe(struct max77693_led_device *led,
>> +					u8 *mode)
>> +{
>> +	struct max77693_sub_led *sub_leds = led->sub_leds;
>> +	struct led_classdev_flash *fled_cdev;
>> +	u8 m = *mode;
>> +
>> +	if (led->iout_joint)
>> +		return;
>> +
>> +	/* Check if the other sub-led wants to be strobed simultaneously. */
>> +	if (m & (MODE_FLASH(FLED1) | MODE_FLASH_EXTERNAL(FLED1))) {
>> +		fled_cdev = &sub_leds[FLED2].fled_cdev;
>> +		if (fled_cdev->sync_led_id)
>> +			m |= m << 1;
>> +	} else if (m & (MODE_FLASH(FLED2) | MODE_FLASH_EXTERNAL(FLED2))) {
>> +		fled_cdev = &sub_leds[FLED1].fled_cdev;
>> +		if (fled_cdev->sync_led_id)
>> +			m |= m >> 1;
>> +	}
>> +
>> +	*mode = m;
>> +}
>> +
>> +static int max77693_add_mode(struct max77693_led_device *led, u8 mode)
>> +{
>> +	int i, ret;
>> +
>> +	mode &= led->allowed_modes;
>> +
>> +	/*
>> +	 * Torch mode once enabled remains active until turned off. If the FLED2
>> +	 * output isn't to be disabled check if the torch mode to be set isn't
>> +	 * already activated and avoid re-setting it.
>> +	 */
>> +	if ((!(mode ^ led->mode_flags)) & MODE_TORCH(FLED2)) {
>> +		for (i = FLED1; i <= FLED2; ++i)
>> +			if ((mode & MODE_TORCH(i)) &&
>> +			    (led->mode_flags & MODE_TORCH(i)))
>> +				return 0;
>> +	}
>> +
>> +	/* Span the mode on FLED2 for joint iouts case */
>> +	if (led->iout_joint)
>> +		mode |= (mode << 1);
>> +
>> +	/* Span the flash mode on the other led if it is to be synchronized */
>> +	max77693_set_sync_strobe(led, &mode);
>> +
>> +	/*
>> +	 * FLASH_EXTERNAL mode activates FLASHEN and TORCHEN pins in the device.
>> +	 * The related register bits fields interfere with SW triggerred modes,
>> +	 * thus clear them to ensure proper device configuration.
>> +	 */
>> +	for (i = FLED1; i <= FLED2; ++i)
>> +		if (mode & MODE_FLASH_EXTERNAL(i))
>> +			led->mode_flags &= (~MODE_TORCH(i) & ~MODE_FLASH(i));
>> +
>> +	led->mode_flags |= mode;
>> +	led->mode_flags &= led->allowed_modes;
>> +
>> +	ret = max77693_set_mode(led, led->mode_flags);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/*
>> +	 * Clear flash mode flag after setting the mode to avoid spurious flash
>> +	 * strobing on each subsequent torch mode setting.
>> +	 */
>> +	if (mode & MODE_FLASH_MASK)
>> +		led->mode_flags &= ~mode;
>> +
>> +	return ret;
>> +}
>> +
>> +static int max77693_clear_mode(struct max77693_led_device *led,
>> +				u8 mode)
>> +{
>> +	int ret;
>> +
>> +	if (led->iout_joint)
>> +		/* Clear mode also on FLED2 for joint iouts case */
>> +		mode |= (mode << 1);
>> +	else
>> +		/*
>> +		 * Clear a flash mode on the other led
>> +		 * if it is to be synchronized.
>> +		 */
>> +		max77693_set_sync_strobe(led, &mode);
>> +
>> +	led->mode_flags &= ~mode;
>> +
>> +	ret = max77693_set_mode(led, led->mode_flags);
>> +
>> +	return ret;
>> +}
>> +
>> +static void max77693_calc_iout(struct max77693_led_device *led, int fled_id,
>> +				u32 iout[2], u32 micro_amp, u32 iout_max[2],
>> +				enum max77693_led_mode mode)
>> +{
>> +	u8 blocked_modes;
>> +
>> +	if (fled_id == FLED1) {
>> +		if (led->iout_joint) {
>> +			/*
>> +			 * FLED2 must not be turned on to get
>> +			 * 15625 uA of total current.
>> +			 */
>> +			if (micro_amp == FLASH_IOUT_MIN) {
>> +				if (mode == FLASH)
>> +					blocked_modes =
>> +						MODE_FLASH(FLED2) |
>> +						MODE_FLASH_EXTERNAL(FLED2);
>> +				else
>> +					blocked_modes = MODE_TORCH(FLED2);
>> +
>> +				led->allowed_modes &= ~blocked_modes;
>> +			}
>> +		} else {
>> +			/*
>> +			 * Preclude splitting current to FLED2 if we
>> +			 * are driving two separate leds.
>> +			 */
>> +			iout_max[FLED2] = 0;
>> +		}
>> +		__max77693_calc_iout(iout, micro_amp, iout_max);
>> +	} else if (fled_id == FLED2) {
>> +		/*
>> +		 * Preclude splitting current to FLED1 if we
>> +		 * are driving two separate leds.
>> +		 */
>> +		iout_max[FLED1] = 0;
>> +		__max77693_calc_iout(iout, micro_amp, iout_max);
>> +	}
>> +}
>> +
>> +static int max77693_set_torch_current(struct max77693_led_device *led,
>> +				int fled_id, u32 micro_amp)
>> +{
>> +	struct max77693_led_config_data *cfg = led->cfg_data;
>> +	struct regmap *rmap = led->regmap;
>> +	u32 iout[2], iout_max[2];
>> +	u8 iout1_reg = 0, iout2_reg = 0;
>> +
>> +	iout_max[FLED1] = cfg->iout_torch[FLED1];
>> +	iout_max[FLED2] = cfg->iout_torch[FLED2];
>> +
>> +	led->allowed_modes |= MODE_TORCH_MASK;
>> +
>> +	max77693_calc_iout(led, fled_id, iout, micro_amp, iout_max,
>
> Could you use cfg->iout_torch directly?

Right, I can't recall why I added redundant iout_max array.

>> +				TORCH);
>> +
>> +	if (fled_id == FLED1 || led->iout_joint) {
>> +		iout1_reg = max77693_led_iout_to_reg(iout[FLED1]);
>> +		led->torch_iout_reg &= 0xf0;
>
> I think it'd be cleaner to use register bit names and led-specific shift
> here. Up to you.

Sure.

>> +	}
>> +	if (fled_id == FLED2 || led->iout_joint) {
>> +		iout2_reg = max77693_led_iout_to_reg(iout[FLED2]);
>> +		led->torch_iout_reg &= 0x0f;
>> +	}
>> +
>> +	led->torch_iout_reg |= ((iout1_reg << TORCH_IOUT1_SHIFT) |
>> +				(iout2_reg << TORCH_IOUT2_SHIFT));
>> +
>> +	return regmap_write(rmap, MAX77693_LED_REG_ITORCH,
>> +						led->torch_iout_reg);
>> +}
>> +
>> +static int max77693_set_flash_current(struct max77693_led_device *led,
>> +					int fled_id,
>> +					u32 micro_amp)
>> +{
>> +	struct max77693_led_config_data *cfg = led->cfg_data;
>> +	struct regmap *rmap = led->regmap;
>> +	u32 iout[2], iout_max[2];
>> +	u8 iout1_reg, iout2_reg;
>> +	int ret = -EINVAL;
>> +
>> +	iout_max[FLED1] = cfg->iout_flash[FLED1];
>> +	iout_max[FLED2] = cfg->iout_flash[FLED2];
>> +
>> +	led->allowed_modes |= MODE_FLASH_MASK;
>> +
>> +	max77693_calc_iout(led, fled_id, iout, micro_amp, iout_max,
>> +				FLASH);
>> +
>> +	if (fled_id == FLED1 || led->iout_joint) {
>> +		iout1_reg = max77693_led_iout_to_reg(iout[FLED1]);
>> +		ret = regmap_write(rmap, MAX77693_LED_REG_IFLASH1,
>> +							iout1_reg);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +	if (fled_id == FLED2 || led->iout_joint) {
>> +		iout2_reg = max77693_led_iout_to_reg(iout[FLED2]);
>> +		ret = regmap_write(rmap, MAX77693_LED_REG_IFLASH2,
>> +							iout2_reg);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int max77693_set_timeout(struct max77693_led_device *led, u32 microsec)
>> +{
>> +	struct max77693_led_config_data *cfg = led->cfg_data;
>> +	struct regmap *rmap = led->regmap;
>> +	u8 v;
>> +	int ret;
>> +
>> +	v = max77693_flash_timeout_to_reg(microsec);
>> +
>> +	if (cfg->trigger_type == MAX77693_LED_TRIG_TYPE_LEVEL)
>> +		v |= FLASH_TMR_LEVEL;
>> +
>> +	ret = regmap_write(rmap, MAX77693_LED_REG_FLASH_TIMER, v);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	led->current_flash_timeout = microsec;
>> +
>> +	return 0;
>> +}
>> +
>> +static int max77693_strobe_status_get(struct max77693_led_device *led,
>> +					bool *state)
>> +{
>> +	struct regmap *rmap = led->regmap;
>> +	unsigned int v;
>> +	int ret;
>> +
>> +	ret = regmap_read(rmap, MAX77693_LED_REG_FLASH_STATUS, &v);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	*state = v & FLASH_STATUS_FLASH_ON;
>> +
>> +	return ret;
>> +}
>> +
>> +static int max77693_int_flag_get(struct max77693_led_device *led,
>> +					unsigned int *v)
>> +{
>> +	struct regmap *rmap = led->regmap;
>> +
>> +	return regmap_read(rmap, MAX77693_LED_REG_FLASH_INT, v);
>> +}
>> +
>> +static int max77693_setup(struct max77693_led_device *led)
>> +{
>> +	struct max77693_led_config_data *cfg = led->cfg_data;
>> +	struct regmap *rmap = led->regmap;
>> +	int i, first_led, last_led, ret;
>> +	u32 max_flash_curr[2];
>> +	u8 v;
>> +
>> +	/*
>> +	 * Initialize only flash current. Torch current doesn't
>> +	 * require initialization as ITORCH register is written with
>> +	 * new value each time brightness_set op is called.
>> +	 */
>> +	if (led->iout_joint) {
>> +		first_led = FLED1;
>> +		last_led = FLED1;
>> +		max_flash_curr[FLED1] = cfg->iout_flash[FLED1] +
>> +					cfg->iout_flash[FLED2];
>> +	} else {
>> +		first_led = max77693_fled_used(led, FLED1) ? FLED1 : FLED2;
>> +		last_led = led->cfg_data->num_leds == 2 ? FLED2 : first_led;
>> +		max_flash_curr[FLED1] = cfg->iout_flash[FLED1];
>> +		max_flash_curr[FLED2] = cfg->iout_flash[FLED2];
>> +	}
>> +
>> +	for (i = first_led; i <= last_led; ++i) {
>> +		ret = max77693_set_flash_current(led, i,
>> +					max_flash_curr[i]);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	v = TORCH_TMR_NO_TIMER | MAX77693_LED_TRIG_TYPE_LEVEL;
>> +	ret = regmap_write(rmap, MAX77693_LED_REG_ITORCHTIMER, v);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* initially set FLED1 timeout */
>> +	ret = max77693_set_timeout(led, cfg->flash_timeout[FLED1]);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (cfg->low_vsys > 0)
>> +		v = max77693_led_vsys_to_reg(cfg->low_vsys) |
>> +						MAX_FLASH1_MAX_FL_EN;
>> +	else
>> +		v = 0;
>> +
>> +	ret = regmap_write(rmap, MAX77693_LED_REG_MAX_FLASH1, v);
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = regmap_write(rmap, MAX77693_LED_REG_MAX_FLASH2, 0);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (cfg->boost_mode == MAX77693_LED_BOOST_FIXED)
>> +		v = FLASH_BOOST_FIXED;
>> +	else
>> +		v = cfg->boost_mode | cfg->boost_mode << 1;
>> +
>> +	if (max77693_fled_used(led, FLED1) && max77693_fled_used(led, FLED2))
>> +		v |= FLASH_BOOST_LEDNUM_2;
>> +
>> +	ret = regmap_write(rmap, MAX77693_LED_REG_VOUT_CNTL, v);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	v = max77693_led_vout_to_reg(cfg->boost_vout);
>> +	ret = regmap_write(rmap, MAX77693_LED_REG_VOUT_FLASH1, v);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Allow all modes on both fled outputs */
>> +	led->allowed_modes = MODE_FLASH_MASK | MODE_TORCH_MASK;
>> +
>> +	return max77693_set_mode(led, MODE_OFF);
>> +}
>> +
>> +static int __max77693_led_brightness_set(struct max77693_led_device *led,
>> +					int fled_id, enum led_brightness value)
>> +{
>> +	int ret;
>> +
>> +	mutex_lock(&led->lock);
>> +
>> +	if (value == 0) {
>> +		ret = max77693_clear_mode(led, MODE_TORCH(fled_id));
>> +		if (ret < 0)
>> +			dev_dbg(&led->pdev->dev,
>> +				"Failed to clear torch mode (%d)\n",
>> +				ret);
>> +		goto unlock;
>> +	}
>> +
>> +	ret = max77693_set_torch_current(led, fled_id, value * TORCH_IOUT_STEP);
>> +	if (ret < 0) {
>> +		dev_dbg(&led->pdev->dev,
>> +			"Failed to set torch current (%d)\n",
>> +			ret);
>> +		goto unlock;
>> +	}
>> +
>> +	ret = max77693_add_mode(led, MODE_TORCH(fled_id));
>> +	if (ret < 0)
>> +		dev_dbg(&led->pdev->dev,
>> +			"Failed to set torch mode (%d)\n",
>> +			ret);
>> +unlock:
>> +	mutex_unlock(&led->lock);
>> +	return ret;
>> +}
>> +
>> +static void max77693_led_brightness_set_work(
>> +					struct work_struct *work)
>> +{
>> +	struct max77693_sub_led *sub_led =
>> +			container_of(work, struct max77693_sub_led,
>> +					work_brightness_set);
>> +	struct max77693_led_device *led = sub_led_to_led(sub_led);
>> +
>> +	__max77693_led_brightness_set(led, sub_led->fled_id,
>> +				sub_led->torch_brightness);
>> +}
>> +
>> +/* LED subsystem callbacks */
>> +
>> +static int max77693_led_brightness_set_sync(
>> +				struct led_classdev *led_cdev,
>> +				enum led_brightness value)
>> +{
>> +	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
>> +	struct max77693_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>> +	struct max77693_led_device *led = sub_led_to_led(sub_led);
>> +
>> +	return __max77693_led_brightness_set(led, sub_led->fled_id, value);
>> +}
>> +
>> +static void max77693_led_brightness_set(
>> +				struct led_classdev *led_cdev,
>> +				enum led_brightness value)
>> +{
>> +	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
>> +	struct max77693_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>> +
>> +	sub_led->torch_brightness = value;
>> +	schedule_work(&sub_led->work_brightness_set);
>> +}
>> +
>> +static int max77693_led_flash_brightness_set(
>> +				struct led_classdev_flash *fled_cdev,
>> +				u32 brightness)
>> +{
>> +	struct max77693_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>> +	struct max77693_led_device *led = sub_led_to_led(sub_led);
>> +	int ret;
>> +
>> +	mutex_lock(&led->lock);
>> +	ret = max77693_set_flash_current(led, sub_led->fled_id, brightness);
>> +	mutex_unlock(&led->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int max77693_led_flash_strobe_set(
>> +				struct led_classdev_flash *fled_cdev,
>> +				bool state)
>> +{
>> +	struct max77693_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>> +	struct max77693_led_device *led = sub_led_to_led(sub_led);
>> +	int fled_id = sub_led->fled_id;
>> +	int ret;
>> +
>> +	mutex_lock(&led->lock);
>> +
>> +	if (!state) {
>> +		ret = max77693_clear_mode(led, MODE_FLASH(fled_id));
>> +		goto unlock;
>> +	}
>> +
>> +	if (sub_led->flash_timeout != led->current_flash_timeout) {
>> +		ret = max77693_set_timeout(led, sub_led->flash_timeout);
>> +		if (ret < 0)
>> +			goto unlock;
>> +	}
>> +
>> +	led->strobing_sub_led_id = fled_id;
>> +
>> +	ret = max77693_add_mode(led, MODE_FLASH(fled_id));
>> +
>> +unlock:
>> +	mutex_unlock(&led->lock);
>> +	return ret;
>> +}
>> +
>> +static int max77693_led_flash_fault_get(
>> +				struct led_classdev_flash *fled_cdev,
>> +				u32 *fault)
>> +{
>> +	struct max77693_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>> +	struct max77693_led_device *led = sub_led_to_led(sub_led);
>> +	unsigned int v;
>> +	int ret;
>> +
>> +	ret = max77693_int_flag_get(led, &v);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	*fault = 0;
>> +
>> +	if (v & ((sub_led->fled_id == FLED1) ? FLASH_INT_FLED1_OPEN :
>> +					       FLASH_INT_FLED2_OPEN))
>> +		*fault |= LED_FAULT_OVER_VOLTAGE;
>> +	if (v & ((sub_led->fled_id == FLED1) ? FLASH_INT_FLED1_SHORT :
>> +					       FLASH_INT_FLED2_SHORT))
>> +		*fault |= LED_FAULT_SHORT_CIRCUIT;
>> +	if (v & FLASH_INT_OVER_CURRENT)
>> +		*fault |= LED_FAULT_OVER_CURRENT;
>> +
>> +	return 0;
>> +}
>> +
>> +static int max77693_led_flash_strobe_get(
>> +				struct led_classdev_flash *fled_cdev,
>> +				bool *state)
>> +{
>> +	struct max77693_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>> +	struct max77693_led_device *led = sub_led_to_led(sub_led);
>> +	int ret;
>> +
>> +	if (!state)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&led->lock);
>> +
>> +	ret = max77693_strobe_status_get(led, state);
>> +
>> +	*state = !!(*state && (led->strobing_sub_led_id == sub_led->fled_id));
>> +
>> +
>> +	mutex_unlock(&led->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int max77693_led_flash_timeout_set(
>> +				struct led_classdev_flash *fled_cdev,
>> +				u32 timeout)
>> +{
>> +	struct max77693_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>> +	struct max77693_led_device *led = sub_led_to_led(sub_led);
>> +
>> +	mutex_lock(&led->lock);
>> +	sub_led->flash_timeout = timeout;
>> +	mutex_unlock(&led->lock);
>> +
>> +	return 0;
>> +}
>> +
>> +static int max77693_led_parse_dt(struct max77693_led_device *led,
>> +				 struct device_node *node)
>> +{
>> +	struct max77693_led_config_data *cfg = led->cfg_data;
>> +	struct max77693_sub_led *sub_leds = led->sub_leds;
>> +	struct device *dev = &led->pdev->dev;
>> +	struct device_node *child_node;
>> +	u32 led_sources[2];
>> +	int fled_id, ret;
>> +
>> +	of_property_read_u32(node, "maxim,trigger-type", &cfg->trigger_type);
>> +	of_property_read_u32(node, "maxim,boost-mode", &cfg->boost_mode);
>> +	of_property_read_u32(node, "maxim,boost-vout", &cfg->boost_vout);
>> +	of_property_read_u32(node, "maxim,vsys-min", &cfg->low_vsys);
>> +
>> +	for_each_available_child_of_node(node, child_node) {
>> +		ret = of_property_read_u32_array(child_node, "led-sources",
>> +							led_sources, 2);
>> +		if (ret < 0) {
>> +			dev_err(dev,
>> +				"Error reading \"led-sources\" DT property\n");
>> +			return ret;
>> +		}
>> +
>> +		if (led_sources[0] && led_sources[1]) {
>> +			fled_id = FLED1;
>> +			led->fled_mask = FLED1_IOUT | FLED2_IOUT;
>> +		} else if (led_sources[0] && !led_sources[1]) {
>> +			fled_id = FLED1;
>> +			led->fled_mask |= FLED1_IOUT;
>> +		} else if (!led_sources[0] && led_sources[1]) {
>> +			fled_id = FLED2;
>> +			led->fled_mask |= FLED2_IOUT;
>> +		} else {
>> +			dev_err(dev,
>> +				"Wrong \"led-sources\" DT property value\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		sub_leds[fled_id].fled_id = fled_id;
>> +
>> +		ret = of_property_read_string(child_node, "label",
>> +					(const char **) &cfg->label[fled_id]);
>> +		if (ret < 0) {
>> +			dev_err(dev, "Error reading \"label\" DT property\n");
>> +			return ret;
>> +		}
>> +
>> +		of_property_read_u32(child_node, "max-microamp",
>> +						&cfg->iout_torch[fled_id]);
>> +		of_property_read_u32(child_node, "flash-max-microamp",
>> +						&cfg->iout_flash[fled_id]);
>> +		of_property_read_u32(child_node, "flash-timeout-us",
>> +						&cfg->flash_timeout[fled_id]);
>> +
>> +		if (++led->cfg_data->num_leds == 2 ||
>> +		    (max77693_fled_used(led, FLED1) &&
>> +		     max77693_fled_used(led, FLED2)))
>> +			break;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void clamp_align(u32 *v, u32 min, u32 max, u32 step)
>> +{
>> +	*v = clamp_val(*v, min, max);
>> +	if (step > 1)
>> +		*v = (*v - min) / step * step + min;
>> +}
>> +
>> +static void max77693_led_validate_configuration(struct max77693_led_device *led)
>> +{
>> +	struct max77693_led_config_data *cfg = led->cfg_data;
>> +	int i;
>> +
>> +	if (led->cfg_data->num_leds == 1 &&
>> +	    max77693_fled_used(led, FLED1) && max77693_fled_used(led, FLED2))
>> +		led->iout_joint = true;
>> +
>> +	cfg->boost_mode = clamp_val(cfg->boost_mode, MAX77693_LED_BOOST_NONE,
>> +			    MAX77693_LED_BOOST_FIXED);
>> +
>> +	/* Boost must be enabled if both current outputs are used */
>> +	if ((cfg->boost_mode == MAX77693_LED_BOOST_NONE) && led->iout_joint)
>> +		cfg->boost_mode = MAX77693_LED_BOOST_FIXED;
>> +
>> +	/* Split max current settings to both outputs in case of joint leds */
>> +	if (led->iout_joint) {
>> +		cfg->iout_torch[FLED1] /= 2;
>> +		cfg->iout_torch[FLED2] = cfg->iout_torch[FLED1];
>> +		cfg->iout_flash[FLED1] /= 2;
>> +		cfg->iout_flash[FLED2] = cfg->iout_flash[FLED1];
>> +	}
>> +
>> +	for (i = FLED1; i <= FLED2; ++i) {
>> +		if (max77693_fled_used(led, i)) {
>> +			clamp_align(&cfg->iout_torch[i], TORCH_IOUT_MIN,
>> +					TORCH_IOUT_MAX, TORCH_IOUT_STEP);
>> +			clamp_align(&cfg->iout_flash[i], FLASH_IOUT_MIN,
>> +					cfg->boost_mode ? FLASH_IOUT_MAX_2LEDS :
>> +							FLASH_IOUT_MAX_1LED,
>> +					FLASH_IOUT_STEP);
>> +		} else {
>> +			cfg->iout_torch[i] = cfg->iout_flash[i] = 0;
>> +		}
>> +	}
>> +
>> +	for (i = 0; i < ARRAY_SIZE(cfg->flash_timeout); ++i)
>> +		clamp_align(&cfg->flash_timeout[i], FLASH_TIMEOUT_MIN,
>> +				FLASH_TIMEOUT_MAX, FLASH_TIMEOUT_STEP);
>> +
>> +	cfg->trigger_type = clamp_val(cfg->trigger_type,
>> +					MAX77693_LED_TRIG_TYPE_EDGE,
>> +					MAX77693_LED_TRIG_TYPE_LEVEL);
>> +
>> +	clamp_align(&cfg->boost_vout, FLASH_VOUT_MIN, FLASH_VOUT_MAX,
>> +							FLASH_VOUT_STEP);
>> +
>> +	if (cfg->low_vsys)
>> +		clamp_align(&cfg->low_vsys, MAX_FLASH1_VSYS_MIN,
>> +				MAX_FLASH1_VSYS_MAX, MAX_FLASH1_VSYS_STEP);
>> +}
>> +
>> +static int max77693_led_get_configuration(struct max77693_led_device *led)
>> +{
>> +	struct device *dev = &led->pdev->dev;
>> +	int ret;
>> +
>> +	if (!dev->of_node)
>> +		return -EINVAL;
>> +
>> +	led->cfg_data = devm_kzalloc(dev, sizeof(*led->cfg_data), GFP_KERNEL);
>> +	if (!led->cfg_data)
>> +		return -ENOMEM;
>> +
>> +	ret = max77693_led_parse_dt(led, dev->of_node);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	max77693_led_validate_configuration(led);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct led_flash_ops flash_ops = {
>> +	.flash_brightness_set	= max77693_led_flash_brightness_set,
>> +	.strobe_set		= max77693_led_flash_strobe_set,
>> +	.strobe_get		= max77693_led_flash_strobe_get,
>> +	.timeout_set		= max77693_led_flash_timeout_set,
>> +	.fault_get		= max77693_led_flash_fault_get,
>> +};
>> +
>> +static void max77693_init_flash_settings(struct max77693_led_device *led,
>> +					 struct max77693_led_settings *s,
>> +					 int fled_id)
>> +{
>> +	struct max77693_led_config_data *cfg = led->cfg_data;
>> +	struct led_flash_setting *setting;
>> +
>> +	/* Init torch intensity setting */
>> +	setting = &s->torch_brightness;
>> +	setting->min = TORCH_IOUT_MIN;
>> +	setting->max = cfg->iout_torch[fled_id];
>> +	setting->max = led->iout_joint ?
>> +			cfg->iout_torch[FLED1] + cfg->iout_torch[FLED2] :
>> +			cfg->iout_torch[fled_id];
>> +	setting->step = TORCH_IOUT_STEP;
>> +	setting->val = setting->max;
>> +
>> +	/* Init flash intensity setting */
>> +	setting = &s->flash_brightness;
>> +	setting->min = FLASH_IOUT_MIN;
>> +	setting->max = led->iout_joint ?
>> +			cfg->iout_flash[FLED1] + cfg->iout_flash[FLED2] :
>> +			cfg->iout_flash[fled_id];
>> +	setting->step = FLASH_IOUT_STEP;
>> +	setting->val = setting->max;
>> +
>> +	/* Init flash timeout setting */
>> +	setting = &s->flash_timeout;
>> +	setting->min = FLASH_TIMEOUT_MIN;
>> +	setting->max = cfg->flash_timeout[fled_id];
>> +	setting->step = FLASH_TIMEOUT_STEP;
>> +	setting->val = setting->max;
>> +}
>> +
>> +static int max77693_set_available_sync_led(struct max77693_led_device *led,
>> +						int fled_id)
>> +{
>> +	struct max77693_sub_led *sub_led = &led->sub_leds[fled_id];
>> +	struct led_classdev_flash *fled_cdev = &sub_led->fled_cdev;
>> +
>> +	fled_cdev->sync_leds = devm_kzalloc(&led->pdev->dev, sizeof(fled_cdev),
>> +					GFP_KERNEL);
>> +	if (!fled_cdev->sync_leds)
>> +		return -ENOMEM;
>> +
>> +	fled_cdev->sync_leds[0] = &led->sub_leds[!fled_id].fled_cdev;
>> +	fled_cdev->num_sync_leds = 1;
>> +
>> +	return 0;
>> +}
>> +
>> +static void max77693_init_fled_cdev(struct max77693_led_device *led,
>> +					int fled_id)
>> +{
>> +	struct led_classdev_flash *fled_cdev;
>> +	struct led_classdev *led_cdev;
>> +	struct max77693_sub_led *sub_led = &led->sub_leds[fled_id];
>> +	struct max77693_led_settings settings;
>> +
>> +	/* Initialize flash settings */
>> +	max77693_init_flash_settings(led, &settings, fled_id);
>> +
>> +	/* Initialize LED Flash class device */
>> +	fled_cdev = &sub_led->fled_cdev;
>> +	fled_cdev->ops = &flash_ops;
>> +	led_cdev = &fled_cdev->led_cdev;
>> +	led_cdev->name = led->cfg_data->label[fled_id];
>> +	led_cdev->brightness_set = max77693_led_brightness_set;
>> +	led_cdev->brightness_set_sync = max77693_led_brightness_set_sync;
>> +	INIT_WORK(&sub_led->work_brightness_set,
>> +			max77693_led_brightness_set_work);
>> +
>> +	led_cdev->max_brightness = settings.torch_brightness.val /
>> +					TORCH_IOUT_STEP;
>> +	led_cdev->flags |= LED_DEV_CAP_FLASH;
>> +	if (led->cfg_data->num_leds == 2)
>> +		led_cdev->flags |= LED_DEV_CAP_SYNC_STROBE;
>> +
>> +	fled_cdev->brightness = settings.flash_brightness;
>> +	fled_cdev->timeout = settings.flash_timeout;
>> +	sub_led->flash_timeout = fled_cdev->timeout.val;
>> +}
>> +
>> +static int max77693_led_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct max77693_dev *iodev = dev_get_drvdata(dev->parent);
>> +	struct max77693_led_device *led;
>> +	struct max77693_sub_led *sub_leds;
>> +	int init_fled_cdev[2], i, ret;
>> +
>> +	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
>> +	if (!led)
>> +		return -ENOMEM;
>> +
>> +	led->pdev = pdev;
>> +	led->regmap = iodev->regmap;
>> +	sub_leds = led->sub_leds;
>> +
>> +	platform_set_drvdata(pdev, led);
>> +	ret = max77693_led_get_configuration(led);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = max77693_setup(led);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	init_fled_cdev[FLED1] =
>> +			led->iout_joint || max77693_fled_used(led, FLED1);
>> +	init_fled_cdev[FLED2] =
>> +			!led->iout_joint && max77693_fled_used(led, FLED2);
>> +
>> +	/* Initialize LED Flash class device(s) */
>> +	for (i = FLED1; i <= FLED2; ++i)
>> +		if (init_fled_cdev[i])
>> +			max77693_init_fled_cdev(led, i);
>> +
>> +	/* Setup sub-leds available for flash strobe synchronization */
>> +	if (led->cfg_data->num_leds == 2) {
>> +		for (i = FLED1; i <= FLED2; ++i) {
>> +			ret = max77693_set_available_sync_led(led, i);
>> +			if (ret < 0)
>> +				return ret;
>> +		}
>> +	}
>> +
>> +	mutex_init(&led->lock);
>> +
>> +	/* Register LED Flash class device(s) */
>> +	for (i = FLED1; i <= FLED2; ++i) {
>> +		if (init_fled_cdev[i]) {
>
> if (!...)
> 	continue;
>
> The rest of the loop would look nicer this way I think. Up to you.

Right.

>> +			ret = led_classdev_flash_register(dev,
>> +							&sub_leds[i].fled_cdev);
>> +			if (ret < 0) {
>> +				/*
>> +				 * At this moment FLED1 might have been already
>> +				 * registered and it needs to be released.
>> +				 */
>> +				if (i == FLED2)
>> +					goto err_register_led2;
>> +				else
>> +					goto err_register_led1;
>> +			}
>> +		}
>> +	}
>> +
>> +
>> +	return 0;
>> +
>> +err_register_led2:
>> +	/* It is possible than only FLED2 was to be registered */
>> +	if (!init_fled_cdev[FLED1])
>> +		goto err_register_led1;
>> +	led_classdev_flash_unregister(&sub_leds[FLED1].fled_cdev);
>> +err_register_led1:
>> +	mutex_destroy(&led->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int max77693_led_remove(struct platform_device *pdev)
>> +{
>> +	struct max77693_led_device *led = platform_get_drvdata(pdev);
>> +	struct max77693_sub_led *sub_leds = led->sub_leds;
>> +
>> +	if (led->iout_joint || max77693_fled_used(led, FLED1)) {
>> +		led_classdev_flash_unregister(&sub_leds[FLED1].fled_cdev);
>> +		cancel_work_sync(&sub_leds[FLED1].work_brightness_set);
>> +	}
>> +
>> +	if (!led->iout_joint && max77693_fled_used(led, FLED2)) {
>> +		led_classdev_flash_unregister(&sub_leds[FLED2].fled_cdev);
>> +		cancel_work_sync(&sub_leds[FLED2].work_brightness_set);
>> +	}
>> +
>> +	mutex_destroy(&led->lock);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct of_device_id max77693_led_dt_match[] = {
>> +	{.compatible = "maxim,max77693-led"},
>> +	{},
>> +};
>> +
>> +static struct platform_driver max77693_led_driver = {
>> +	.probe		= max77693_led_probe,
>> +	.remove		= max77693_led_remove,
>> +	.driver		= {
>> +		.name	= "max77693-led",
>> +		.owner	= THIS_MODULE,
>> +		.of_match_table = max77693_led_dt_match,
>> +	},
>> +};
>> +
>> +module_platform_driver(max77693_led_driver);
>> +
>> +MODULE_AUTHOR("Jacek Anaszewski <j.anaszewski@samsung.com>");
>> +MODULE_AUTHOR("Andrzej Hajda <a.hajda@samsung.com>");
>> +MODULE_DESCRIPTION("Maxim MAX77693 led flash driver");
>> +MODULE_LICENSE("GPL v2");
>


-- 
Best Regards,
Jacek Anaszewski