Re: [PATCH v6 07/11] leds: flash: add support for Samsung S2M series PMIC flash LED device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Self-deadlock in `s2m_fled_flash_external_strobe_set` due to recursive mutex acquisition.
- [Medium] Incorrect implementation of V4L2 `external_strobe_set` callback and missing error handling.
- [Low] Invalid function pointer cast in `devm_add_action_or_reset()` causes compilation warnings/errors.
- [Low] Missing cleanup action for PMIC LED channel enable bit on probe failure.
- [Low] Unnecessary `select REGMAP_IRQ` in Kconfig.
- [Low] Unprotected updates to flash_brightness and flash_timeout state.
--

commit 901a86b5a2ab51ba67c0b914a1f2b70fb3100a1a
Author: Kaustabh Chakraborty <kauschluss@disroot.org>

leds: flash: add support for Samsung S2M series PMIC flash LED device

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.

> diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
> index 5e08102a67841..b6542ecc522dd 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

Is this select REGMAP_IRQ necessary? The driver appears to use basic regmap
read and write APIs without handling hardware interrupts or utilizing the
regmap IRQ framework functionalities.

> +	help
> +	  This option enables support for the flash/torch LEDs found in certain

[ ... ]

> diff --git a/drivers/leds/flash/leds-s2m-flash.c b/drivers/leds/flash/leds-s2m-flash.c
> new file mode 100644
> index 0000000000000..40cc5f8b8adc9
> --- /dev/null
> +++ b/drivers/leds/flash/leds-s2m-flash.c

[ ... ]

> +static int s2m_fled_flash_brightness_set(struct led_classdev_flash *fled, u32 brightness)
> +{
> +	struct s2m_led *led = to_s2m_led(fled);
> +	struct led_flash_setting *setting = &fled->brightness;
> +
> +	led->flash_brightness = (brightness - setting->min) / setting->step;

Should this update be protected by led->lock? The s2mu005_fled_flash_strobe_set()
function reads led->flash_brightness while holding led->lock. Concurrent
sysfs updates might be missed during an active strobe operation.

> +
> +	return 0;
> +}
> +
> +static int s2m_fled_flash_timeout_set(struct led_classdev_flash *fled, u32 timeout)
> +{
> +	struct s2m_led *led = to_s2m_led(fled);
> +	struct led_flash_setting *setting = &fled->timeout;
> +
> +	led->flash_timeout = (timeout - setting->min) / setting->step;

Similarly, should led->flash_timeout be protected by led->lock here?

> +
> +	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_led *led = to_s2m_led(v4l2_flash->fled_cdev);
> +
> +	mutex_lock(&led->lock);
> +
> +	led->fled.ops->strobe_set(&led->fled, enable);

Can this lead to a deadlock?

The external_strobe_set() wrapper acquires led->lock and then invokes
strobe_set() (which maps to s2mu005_fled_flash_strobe_set). However,
s2mu005_fled_flash_strobe_set() unconditionally attempts to acquire the
same led->lock.

Also, the V4L2 external_strobe_set callback is typically intended to
configure the hardware to listen to an external trigger pin. Does calling
the software strobe_set callback incorrectly turn on the flash via software
registers instead?

Lastly, the return value of strobe_set() is ignored here. Should underlying
regmap errors be propagated back to the V4L2 subsystem?

> +
> +	mutex_unlock(&led->lock);
> +
> +	return 0;
> +}

[ ... ]

> +static int s2mu005_fled_init(struct s2m_led *led, struct device *dev, struct regmap *regmap,
> +			     unsigned int nr_channels)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	/* Enable the LED channels. */
> +	ret = regmap_set_bits(regmap, S2MU005_REG_FLED_CTRL1, S2MU005_FLED_CH_EN);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to enable LED channels\n");

Is a cleanup action needed to clear S2MU005_FLED_CH_EN if the probe fails
later, or when the driver is unbound? Without it, the PMIC flash hardware
block might remain powered on.

> +
> +	ret = regmap_read(regmap, S2MU005_REG_ID, &val);

[ ... ]

> +static int s2mu005_fled_init_channel(struct s2m_led *led, struct device *dev,
> +				     struct fwnode_handle *fwnp)
> +{

[ ... ]

> +	led->v4l2_flash = v4l2_flash_init(dev, fwnp, &led->fled, &s2m_fled_v4l2_flash_ops,
> +					  &v4l2_cfg);
> +	if (IS_ERR(led->v4l2_flash))
> +		return dev_err_probe(dev, PTR_ERR(led->v4l2_flash),
> +				     "failed to create V4L2 flash device\n");
> +
> +	ret = devm_add_action_or_reset(dev, (void *)s2m_fled_v4l2_flash_release, led->v4l2_flash);

Is this (void *) cast necessary? The __devm_add_action_or_reset() API expects
a function pointer parameter of type void (*)(void *). The
s2m_fled_v4l2_flash_release function natively matches this signature. Casting
a function pointer to a generic data pointer might cause compilation errors
like -Werror=incompatible-pointer-types on modern compilers.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to add cleanup action\n");
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-s2mu005-pmic-v6-0-1979106992d4@disroot.org?part=7