Re: [PATCH v2 08/12] leds: flash: add support for Samsung S2M series PMIC flash LED device

[André Draszik]

On Thu, 2026-02-05 at 21:46 +0530, Kaustabh Chakraborty wrote:
> On 2026-02-04 16:55 +00:00, André Draszik wrote:
> > Hi,
> > 
> > On Mon, 2026-01-26 at 00:37 +0530, 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@xxxxxxxxxxx>
> > > ---
> > >  drivers/leds/flash/Kconfig          |  12 ++
> > >  drivers/leds/flash/Makefile         |   1 +
> > >  drivers/leds/flash/leds-s2m-flash.c | 410 ++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 423 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..1be2745c475bf
> > > --- /dev/null
> > > +++ b/drivers/leds/flash/leds-s2m-flash.c
> > > @@ -0,0 +1,410 @@
> > > +// 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@xxxxxxxxxxx>
> > > + */
> > > +
> > > +#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 {
> > > +	struct device *dev;
> > > +	struct regmap *regmap;
> > > +	struct led_classdev_flash cdev;
> > > +	struct v4l2_flash *v4l2_flash;
> > > +	struct mutex lock;
> > 
> > Please add a (brief) comment describing what the mutex protects.
> 
> The mutex object prevents the concurrent access of flash control
> registers by the LED and V4L2 subsystems. -- will add this.
> 
> > > +
> > > +	/*
> > > +	 * Get the LED enable register address. Revision EVT0 has the
> > > +	 * register at CTRL4, while EVT1 and higher have it at CTRL6.
> > > +	 */
> > > +	if (priv->pmic_revision == 0)
> > > +		reg_enable = S2MU005_REG_FLED_CTRL4;
> > > +	else
> > > +		reg_enable = S2MU005_REG_FLED_CTRL6;
> > 
> > You could REG_FIELD() and friends for this and everywhere else with
> > similar if / else.
> > 
> 
> REG_FIELD(), from what I understood, is for selecting a bit field inside
> a single register. However this code chooses between two separate
> registers. I believe your interpretation was incorrect? Please clarify.

The first argument to REG_FIELD is the register itself, so reg fields can
be used to describe this difference. See e.g. drivers/leds/rgb/leds-mt6370-rgb.c
Of course, you could have a member variable instead to hold the register
index if all bits are the same in both revisions. Either way would avoid
having to constantly check the revision during runtime.

Cheers,
Andre'