Re: [PATCH v7 06/10] 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 5 potential issue(s) to consider:
- [Medium] The V4L2 external_strobe_set callback triggers a software strobe instead of configuring a hardware strobe.
- [Medium] The OF match table is declared but never assigned to the platform_driver struct.
- [Medium] Inconsistent Torch maximum brightness constraints between Sysfs and V4L2.
- [Medium] Lack of mutual exclusion between Torch and Flash modes.
- [Medium] Hardware state leak on probe failure or module unload.
--

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

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

This commit adds support for flash LEDs found in Samsung S2M series PMICs,
such as the S2MU005. The driver supports two independently controlled channels
that can operate in both torch and flash modes.

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

[ ... ]

> +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);
> +
> +	return led->fled.ops->strobe_set(&led->fled, enable);
> +}

Does this implementation fire the flash immediately instead of configuring
it to wait for a hardware strobe signal?

The V4L2 API contract expects external_strobe_set to merely arm the device
for an external hardware pin assertion. If the PMIC lacks true
hardware-based external strobe support, should has_external_strobe be
omitted instead?

[ ... ]

> +static int s2mu005_fled_torch_brightness_set(struct led_classdev *cdev, enum led_brightness value)
> +{
[ ... ]
> +	ret = regmap_set_bits(led->regmap, led->reg_enable, S2MU005_FLED_TORCH_EN(led->channel));
> +	if (ret) {
> +		dev_err(cdev->dev, "failed to enable torch LED\n");
> +		goto unlock;
> +	}
> +
> +unlock:
> +	mutex_unlock(&led->lock);
> +
> +	return ret;
> +}
> +
> +static int s2mu005_fled_flash_strobe_set(struct led_classdev_flash *fled, bool state)
> +{
[ ... ]
> +	ret = regmap_set_bits(led->regmap, led->reg_enable, S2MU005_FLED_FLASH_EN(led->channel));
> +	if (ret) {
> +		dev_err(fled->led_cdev.dev, "failed to enable flash LED\n");
> +		goto unlock;
> +	}
> +
> +unlock:
> +	mutex_unlock(&led->lock);
> +
> +	return ret;
> +}

Since the LED subsystem allows Sysfs to enable Torch and Flash
independently, is there a risk that both modes could be active
simultaneously in the PMIC registers?

Neither s2mu005_fled_torch_brightness_set() nor
s2mu005_fled_flash_strobe_set() appears to clear the enable bit for the
opposite mode.

[ ... ]

> +static int s2mu005_fled_init(struct s2m_led *led, struct device *dev, struct regmap *regmap,
> +			     unsigned int nr_channels)
> +{
[ ... ]
> +	/* 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");
> +
> +	return 0;
> +}

If the probe fails during child node iteration, or if the module is
unloaded, does this leave the PMIC LED block permanently powered on? 

It looks like S2MU005_FLED_CH_EN is set globally here but never cleared on
failure or removal, which could cause a resource and power leak.

[ ... ]

> +static int s2mu005_fled_init_channel(struct s2m_led *led, struct device *dev,
> +				     struct fwnode_handle *fwnp)
> +{
[ ... ]
> +	cdev->max_brightness = 16;
> +	cdev->brightness_set_blocking = s2mu005_fled_torch_brightness_set;
> +	cdev->flags |= LED_DEV_CAP_FLASH;
[ ... ]
> +	v4l2_cfg.intensity.min = led->fled.brightness.min;
> +	v4l2_cfg.intensity.step = led->fled.brightness.step;
> +	v4l2_cfg.intensity.max = led->fled.brightness.max;
> +	v4l2_cfg.intensity.val = led->fled.brightness.val;

Does this create an inconsistency between the Sysfs and V4L2 Torch maximum
brightness constraints?

The code sets cdev->max_brightness to 16, but v4l2_cfg.intensity is
initialized using the Flash constraints. Since led->fled.brightness limits
the brightness to 375,000 uA (effectively level 15), V4L2 users might be
artificially restricted to level 15 while Sysfs users can reach 16.

[ ... ]

> +static const struct of_device_id s2m_fled_of_match_table[] = {
> +	{ .compatible = "samsung,s2mu005-flash", .data = (void *)S2MU005 },
> +	{ /* 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);

Should s2m_fled_of_match_table be assigned to the .of_match_table field
within the driver structure here?

It appears to be declared and exported above, but never assigned to the
driver. This might prevent Device Tree compatible string matching.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-s2mu005-pmic-v7-0-73f9702fb461@disroot.org?part=6