Re: [PATCH v3 09/13] leds: flash: add support for Samsung S2M series PMIC flash LED device

[Lee Jones <lee@kernel.org>]

On Wed, 25 Feb 2026, Kaustabh Chakraborty wrote:

> Add support for flash LEDs found in certain Samsung S2M series PMICs.
> The device has two channels for LEDs, typically for the back and front
> cameras in mobile devices. Both channels can be independently
> controlled, and can be operated in torch or flash modes.
> 
> The driver includes initial support for the S2MU005 PMIC flash LEDs.
> 
> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
> ---
>  drivers/leds/flash/Kconfig          |  12 +
>  drivers/leds/flash/Makefile         |   1 +
>  drivers/leds/flash/leds-s2m-flash.c | 429 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 442 insertions(+)
> 
> diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
> index 5e08102a67841..be62e05277429 100644
> --- a/drivers/leds/flash/Kconfig
> +++ b/drivers/leds/flash/Kconfig
> @@ -114,6 +114,18 @@ config LEDS_RT8515
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called leds-rt8515.
>  
> +config LEDS_S2M_FLASH
> +	tristate "Samsung S2M series PMICs flash/torch LED support"
> +	depends on LEDS_CLASS
> +	depends on MFD_SEC_CORE
> +	depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
> +	select REGMAP_IRQ
> +	help
> +	  This option enables support for the flash/torch LEDs found in
> +	  certain Samsung S2M series PMICs, such as the S2MU005. It has
> +	  a LED channel dedicated for every physical LED. The LEDs can
> +	  be controlled in flash and torch modes.
> +
>  config LEDS_SGM3140
>  	tristate "LED support for the SGM3140"
>  	depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
> diff --git a/drivers/leds/flash/Makefile b/drivers/leds/flash/Makefile
> index 712fb737a428e..44e6c1b4beb37 100644
> --- a/drivers/leds/flash/Makefile
> +++ b/drivers/leds/flash/Makefile
> @@ -10,6 +10,7 @@ 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_S2M_FLASH)	+= leds-s2m-flash.o
>  obj-$(CONFIG_LEDS_SGM3140)	+= leds-sgm3140.o
>  obj-$(CONFIG_LEDS_SY7802)	+= leds-sy7802.o
>  obj-$(CONFIG_LEDS_TPS6131X)	+= leds-tps6131x.o
> diff --git a/drivers/leds/flash/leds-s2m-flash.c b/drivers/leds/flash/leds-s2m-flash.c
> new file mode 100644
> index 0000000000000..536a529889a9c
> --- /dev/null
> +++ b/drivers/leds/flash/leds-s2m-flash.c
> @@ -0,0 +1,429 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Flash and Torch LED Driver for Samsung S2M series PMICs.
> + *
> + * Copyright (c) 2015 Samsung Electronics Co., Ltd
> + * Copyright (c) 2025 Kaustabh Chakraborty <kauschluss@disroot.org>
> + */
> +
> +#include <linux/container_of.h>
> +#include <linux/led-class-flash.h>
> +#include <linux/mfd/samsung/core.h>
> +#include <linux/mfd/samsung/s2mu005.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <media/v4l2-flash-led-class.h>
> +
> +#define MAX_CHANNELS	2
> +
> +struct s2m_fled {

This is not the fled, change this to _led and call pointers to it 'led'.

> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct led_classdev_flash cdev;

This is the 'fled'.  Please change all pointer to it to 'fled'.

> +	struct v4l2_flash *v4l2_flash;
> +	/*
> +	 * The mutex object prevents the concurrent access of flash control
> +	 * registers by the LED and V4L2 subsystems.
> +	 */
> +	struct mutex lock;
> +	const struct s2m_fled_spec *spec;

This should not be in here.

> +	unsigned int reg_enable;
> +	u8 channel;
> +	u8 flash_brightness;
> +	u8 flash_timeout;
> +};
> +
> +struct s2m_fled_spec {
> +	u8 nr_channels;
> +	u32 torch_max_brightness;
> +	u32 flash_min_current_ua;
> +	u32 flash_max_current_ua;
> +	u32 flash_min_timeout_us;
> +	u32 flash_max_timeout_us;
> +	int (*torch_brightness_set_blocking)(struct led_classdev *led_cdev,
> +					     enum led_brightness brightness);

I'm really not a fan of pointers to functions at the driver level.

If there are differences between devices, which I cannot see here, store
an identifier and use that to choose which call-back function is the
most appropriate.

> +	const struct led_flash_ops *flash_ops;

Why do we need to store these?

The same principles apply to the function pointer above.

> +};
> +
> +static struct led_classdev_flash *to_cdev_flash(struct led_classdev *cdev)
> +{
> +	return container_of(cdev, struct led_classdev_flash, led_cdev);
> +}
> +
> +static struct s2m_fled *to_led_priv(struct led_classdev_flash *cdev)

There is nothing private about s2m_{f}led.

'led' is the most common nomenclature.  So: led->channel, etc.

Remove 'priv' throughout.

> +{
> +	return container_of(cdev, struct s2m_fled, cdev);

	return container_of(fled, struct s2m_led, fled);

> +}
> +
> +static int s2m_fled_flash_brightness_set(struct led_classdev_flash *cdev,
> +					 u32 brightness)
> +{
> +	struct s2m_fled *priv = to_led_priv(cdev);
> +	struct led_flash_setting *setting = &cdev->brightness;
> +
> +	priv->flash_brightness = (brightness - setting->min) / setting->step;
> +
> +	return 0;
> +}
> +
> +static int s2m_fled_flash_timeout_set(struct led_classdev_flash *cdev,
> +				      u32 timeout)
> +{
> +	struct s2m_fled *priv = to_led_priv(cdev);
> +	struct led_flash_setting *setting = &cdev->timeout;
> +
> +	priv->flash_timeout = (timeout - setting->min) / setting->step;
> +
> +	return 0;
> +}
> +
> +#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
> +static int s2m_fled_flash_external_strobe_set(struct v4l2_flash *v4l2_flash,
> +					      bool enable)
> +{
> +	struct s2m_fled *priv = to_led_priv(v4l2_flash->fled_cdev);
> +
> +	mutex_lock(&priv->lock);
> +
> +	priv->cdev.ops->strobe_set(&priv->cdev, enable);
> +
> +	mutex_unlock(&priv->lock);
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_flash_ops s2m_fled_v4l2_flash_ops = {
> +	.external_strobe_set = s2m_fled_flash_external_strobe_set,
> +};
> +#else
> +static const struct v4l2_flash_ops s2m_fled_v4l2_flash_ops;
> +#endif
> +
> +static void s2m_fled_v4l2_flash_release(void *v4l2_flash)
> +{
> +	v4l2_flash_release(v4l2_flash);
> +}
> +
> +static int s2mu005_fled_torch_brightness_set(struct led_classdev *cdev,
> +					     enum led_brightness value)
> +{
> +	struct s2m_fled *priv = to_led_priv(to_cdev_flash(cdev));
> +	struct regmap *regmap = priv->regmap;
> +	int ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	if (value == LED_OFF) {

These defines are deprecated.

From include/linux/leds.h:

/* This is obsolete/useless. We now support variable maximum brightness. */
enum led_brightness {
        LED_OFF         = 0,
        LED_ON          = 1,
        LED_HALF        = 127,
        LED_FULL        = 255,
};

> +		ret = regmap_clear_bits(regmap, priv->reg_enable,
> +					S2MU005_FLED_TORCH_EN(priv->channel));
> +		if (ret < 0)
> +			dev_err(priv->dev, "failed to disable torch LED\n");
> +		goto unlock;
> +	}
> +
> +	ret = regmap_update_bits(regmap, S2MU005_REG_FLED_CH_CTRL1(priv->channel),
j> +				 S2MU005_FLED_TORCH_IOUT,
> +				 FIELD_PREP(S2MU005_FLED_TORCH_IOUT, value - 1));
> +	if (ret < 0) {
> +		dev_err(priv->dev, "failed to set torch current\n");
> +		goto unlock;
> +	}
> +
> +	ret = regmap_set_bits(regmap, priv->reg_enable,
> +			      S2MU005_FLED_TORCH_EN(priv->channel));
> +	if (ret < 0) {
> +		dev_err(priv->dev, "failed to enable torch LED\n");
> +		goto unlock;
> +	}
> +
> +unlock:
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}
> +
> +static int s2mu005_fled_flash_strobe_set(struct led_classdev_flash *cdev,
> +					 bool state)
> +{
> +	struct s2m_fled *priv = to_led_priv(cdev);
> +	struct regmap *regmap = priv->regmap;
> +	int ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	ret = regmap_clear_bits(regmap, priv->reg_enable,
> +				S2MU005_FLED_FLASH_EN(priv->channel));
> +	if (ret < 0) {
> +		dev_err(priv->dev, "failed to disable flash LED\n");
> +		goto unlock;
> +	}
> +
> +	if (!state)
> +		goto unlock;
> +
> +	ret = regmap_update_bits(regmap, S2MU005_REG_FLED_CH_CTRL0(priv->channel),
> +				 S2MU005_FLED_FLASH_IOUT,
> +				 FIELD_PREP(S2MU005_FLED_FLASH_IOUT,
> +					    priv->flash_brightness));
> +	if (ret < 0) {
> +		dev_err(priv->dev, "failed to set flash brightness\n");
> +		goto unlock;
> +	}
> +
> +	ret = regmap_update_bits(regmap, S2MU005_REG_FLED_CH_CTRL3(priv->channel),
> +				 S2MU005_FLED_FLASH_TIMEOUT,
> +				 FIELD_PREP(S2MU005_FLED_FLASH_TIMEOUT,
> +					    priv->flash_timeout));
> +	if (ret < 0) {
> +		dev_err(priv->dev, "failed to set flash timeout\n");
> +		goto unlock;
> +	}
> +
> +	ret = regmap_set_bits(regmap, priv->reg_enable,
> +			      S2MU005_FLED_FLASH_EN(priv->channel));
> +	if (ret < 0) {
> +		dev_err(priv->dev, "failed to enable flash LED\n");
> +		goto unlock;
> +	}
> +
> +unlock:
> +	mutex_unlock(&priv->lock);
> +
> +	return 0;
> +}
> +
> +static int s2mu005_fled_flash_strobe_get(struct led_classdev_flash *cdev,
> +					 bool *state)
> +{
> +	struct s2m_fled *priv = to_led_priv(cdev);
> +	struct regmap *regmap = priv->regmap;

Since this is only used once, I don't see anything to gain from
utilising a local variable here.

> +	u8 channel = priv->channel;

Same here.

> +	u32 val;
> +	int ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	ret = regmap_read(regmap, S2MU005_REG_FLED_STATUS, &val);
> +	if (ret < 0) {
> +		dev_err(priv->dev, "failed to fetch LED status");
> +		goto unlock;
> +	}
> +
> +	*state = !!(val & S2MU005_FLED_FLASH_STATUS(channel));
> +
> +unlock:
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}
> +
> +static const struct led_flash_ops s2mu005_fled_flash_ops = {
> +	.flash_brightness_set = s2m_fled_flash_brightness_set,
> +	.timeout_set = s2m_fled_flash_timeout_set,
> +	.strobe_set = s2mu005_fled_flash_strobe_set,
> +	.strobe_get = s2mu005_fled_flash_strobe_get,
> +};
> +
> +static int s2mu005_fled_init(struct s2m_fled *priv)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	/* Enable the LED channels. */
> +	ret = regmap_set_bits(priv->regmap, S2MU005_REG_FLED_CTRL1,
> +			      S2MU005_FLED_CH_EN);

Feel free to use up to 100-char to eradicate these wraps.

> +	if (ret < 0)
> +		return dev_err_probe(priv->dev, ret, "failed to enable LED channels\n");
> +
> +	/*
> +	 * Get the LED enable register address. Revision EVT0 has the
> +	 * register at CTRL4, while EVT1 and higher have it at CTRL6.
> +	 */
> +	ret = regmap_read(priv->regmap, S2MU005_REG_ID, &val);
> +	if (ret < 0)
> +		return dev_err_probe(priv->dev, ret, "failed to read revision\n");
> +
> +	if (FIELD_GET(S2MU005_ID_MASK, val) == 0)

A comment above to explain what this means please.

And/or define the zero to something obvious.

> +		priv->reg_enable = S2MU005_REG_FLED_CTRL4;
> +	else
> +		priv->reg_enable = S2MU005_REG_FLED_CTRL6;
> +
> +	return 0;
> +}
> +
> +static const struct s2m_fled_spec s2mu005_fled_spec = {
> +	.nr_channels = 2,
> +	.torch_max_brightness = 16,
> +	.flash_min_current_ua = 25000,
> +	.flash_max_current_ua = 375000, /* 400000 causes flickering */
> +	.flash_min_timeout_us = 62000,
> +	.flash_max_timeout_us = 992000,
> +	.torch_brightness_set_blocking = s2mu005_fled_torch_brightness_set,
> +	.flash_ops = &s2mu005_fled_flash_ops,
> +};
> +
> +static int s2m_fled_init_channel(struct s2m_fled *priv,
> +				 struct fwnode_handle *fwnp)

Unwrap all of these that fit into 100-chars.

> +{
> +	struct led_classdev *led = &priv->cdev.led_cdev;
> +	struct led_init_data init_data = {};
> +	struct v4l2_flash_config v4l2_cfg = {};
> +	int ret;
> +
> +	led->max_brightness = priv->spec->torch_max_brightness;
> +	led->brightness_set_blocking = priv->spec->torch_brightness_set_blocking;
> +	led->flags |= LED_DEV_CAP_FLASH;
> +
> +	priv->cdev.timeout.min = priv->spec->flash_min_timeout_us;
> +	priv->cdev.timeout.step = priv->spec->flash_min_timeout_us;
> +	priv->cdev.timeout.max = priv->spec->flash_max_timeout_us;
> +	priv->cdev.timeout.val = priv->spec->flash_max_timeout_us;
> +
> +	priv->cdev.brightness.min = priv->spec->flash_min_current_ua;
> +	priv->cdev.brightness.step = priv->spec->flash_min_current_ua;
> +	priv->cdev.brightness.max = priv->spec->flash_max_current_ua;
> +	priv->cdev.brightness.val = priv->spec->flash_max_current_ua;

Moving things around in priv/data tells me that something isn't right.

I think the spec should be removed from what is currently called priv.

> +	s2m_fled_flash_timeout_set(&priv->cdev, priv->cdev.timeout.val);
> +	s2m_fled_flash_brightness_set(&priv->cdev, priv->cdev.brightness.val);
> +
> +	priv->cdev.ops = priv->spec->flash_ops;
> +
> +	init_data.fwnode = fwnp;
> +	ret = devm_led_classdev_flash_register_ext(priv->dev, &priv->cdev,
> +						   &init_data);
> +	if (ret < 0)
> +		return dev_err_probe(priv->dev, ret, "failed to create LED flash device\n");
> +
> +	v4l2_cfg.intensity.min = priv->spec->flash_min_current_ua;
> +	v4l2_cfg.intensity.step = priv->spec->flash_min_current_ua;
> +	v4l2_cfg.intensity.max = priv->spec->flash_max_current_ua;
> +	v4l2_cfg.intensity.val = priv->spec->flash_max_current_ua;
> +
> +	v4l2_cfg.has_external_strobe = true;
> +
> +	priv->v4l2_flash = v4l2_flash_init(priv->dev, fwnp, &priv->cdev,
> +					   &s2m_fled_v4l2_flash_ops, &v4l2_cfg);
> +	if (IS_ERR(priv->v4l2_flash)) {
> +		v4l2_flash_release(priv->v4l2_flash);
> +		return dev_err_probe(priv->dev, PTR_ERR(priv->v4l2_flash),
> +				     "failed to create V4L2 flash device\n");
> +	}
> +
> +	ret = devm_add_action_or_reset(priv->dev, (void *)s2m_fled_v4l2_flash_release,
> +				       priv->v4l2_flash);
> +	if (ret < 0)
> +		return dev_err_probe(priv->dev, ret, "failed to add cleanup action\n");

v4l2_flash_release()?

> +	return 0;
> +}
> +
> +static int s2m_fled_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sec_pmic_dev *pmic_drvdata = dev_get_drvdata(dev->parent);

s/pmic_drvdata/ddata/

> +	struct s2m_fled *priv;
> +	bool channel_initialized[MAX_CHANNELS] = { false };
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv) * MAX_CHANNELS, GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);

Where is this used?

> +	priv->dev = dev;

This is already available to you in what is currently called:

  priv->cdev->led_cdev->dev

Also, isn't this an array?

> +	priv->regmap = pmic_drvdata->regmap_pmic;
> +
> +	switch (platform_get_device_id(pdev)->driver_data) {
> +	case S2MU005:
> +		priv->spec = &s2mu005_fled_spec;
> +		ret = s2mu005_fled_init(priv);
> +		if (ret)
> +			return ret;
> +		break;
> +	default:
> +		return dev_err_probe(dev, -ENODEV,
> +				     "device type %d is not supported by driver\n",
> +				     pmic_drvdata->device_type);
> +	}
> +
> +	if (priv->spec->nr_channels > MAX_CHANNELS)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "number of channels specified (%u) exceeds the limit (%u)\n",
> +				     priv->spec->nr_channels, MAX_CHANNELS);
> +
> +	device_for_each_child_node_scoped(dev, child) {
> +		u32 reg;
> +
> +		if (fwnode_property_read_u32(child, "reg", &reg))
> +			continue;
> +
> +		if (reg >= priv->spec->nr_channels) {
> +			dev_warn(dev, "channel %d is non-existent\n", reg);
> +			continue;
> +		}
> +
> +		if (channel_initialized[reg]) {

Do you really need a whole variable just for this?

If this a risk?  If it really is, why not check (priv[reg].dev)?

> +			dev_warn(dev, "duplicate node for channel %d\n", reg);
> +			continue;
> +		}
> +
> +		priv[reg].dev = priv->dev;

What on earth is going on here?

> +		priv[reg].regmap = priv->regmap;
> +		priv[reg].spec = priv->spec;
> +		priv[reg].reg_enable = priv->reg_enable;

And why do you need ${reg} copies of the same data?

Sounds like a waste of resources, no?

> +		priv[reg].channel = (u8)reg;
> +
> +		ret = devm_mutex_init(dev, &priv[reg].lock);

Why not just look the whole device?  Rather than per channel?

To channels have no shared resources?

> +		if (ret)
> +			return dev_err_probe(dev, ret, "failed to create mutex lock\n");

You can drop the ' lock' part.

> +
> +		ret = s2m_fled_init_channel(priv + reg, child);
> +		if (ret < 0)
> +			return ret;
> +
> +		channel_initialized[reg] = true;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct platform_device_id s2m_fled_id_table[] = {
> +	{ "s2mu005-flash", S2MU005 },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(platform, s2m_fled_id_table);
> +
> +/*
> + * Device is instantiated through parent MFD device and device matching
> + * is done through platform_device_id.
> + *
> + * However if device's DT node contains proper compatible and driver is
> + * built as a module, then the *module* matching will be done through DT
> + * aliases. This requires of_device_id table. In the same time this will
> + * not change the actual *device* matching so do not add .of_match_table.
> + */

All of this is already implied.

No need to have this comment on all MFD sub-devices.

> +static const struct of_device_id s2m_fled_of_match_table[] = {
> +	{
> +		.compatible = "samsung,s2mu005-flash",
> +		.data = (void *)S2MU005,

These usually fit on a single line.

> +	}, {
> +		/* sentinel */
> +	},
> +};
> +MODULE_DEVICE_TABLE(of, s2m_fled_of_match_table);
> +
> +static struct platform_driver s2m_fled_driver = {
> +	.driver = {
> +		.name = "s2m-flash",
> +	},
> +	.probe = s2m_fled_probe,
> +	.id_table = s2m_fled_id_table,
> +};
> +module_platform_driver(s2m_fled_driver);
> +
> +MODULE_DESCRIPTION("Flash/Torch LED Driver For Samsung S2M Series PMICs");
> +MODULE_AUTHOR("Kaustabh Chakraborty <kauschluss@disroot.org>");
> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.52.0
> 

-- 
Lee Jones [李琼斯]