Re: [PATCH v6 08/11] leds: rgb: add support for Samsung S2M series PMIC RGB LED device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Uninitialized boolean variables `ramp_up_en` and `ramp_dn_en` are evaluated in conditional branches.
- [High] Global state mutation and data race via shared static array `s2mu005_rgb_subled_info`.
- [High] Setting a solid brightness fails to clear active hardware pattern timings.
- [High] Hardware pattern rejection due to missing duration clamping in LUT lookup.
- [Medium] Redundant read-modify-write on a volatile register risks re-asserting the hardware reset.
- [Medium] Missing OF match table registration prevents native Device Tree matching.
- [Low] Unnecessary Kconfig dependency on `REGMAP_IRQ`.
--

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

leds: rgb: add support for Samsung S2M series PMIC RGB LED device

Add support for the RGB LEDs found in certain Samsung S2M series PMICs.
The device has three LED channels, controlled as a single device. These
LEDs are typically used as status indicators in mobile phones.

> diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
> index 28ef4c487367c..cf57d85e574ec 100644
> --- a/drivers/leds/rgb/Kconfig
> +++ b/drivers/leds/rgb/Kconfig
> @@ -75,6 +75,17 @@ config LEDS_QCOM_LPG
>  
>  	  If compiled as a module, the module will be named leds-qcom-lpg.
>  
> +config LEDS_S2M_RGB
> +	tristate "Samsung S2M series PMICs RGB LED support"
> +	depends on LEDS_CLASS
> +	depends on MFD_SEC_CORE
> +	select REGMAP_IRQ

Is it necessary to select REGMAP_IRQ here? The driver appears to only
perform basic regmap I/O and does not seem to utilize any IRQ-related
regmap functionality.

> +	help
> +	  This option enables support for the S2MU005 RGB LEDs. These devices

[ ... ]

> diff --git a/drivers/leds/rgb/leds-s2m-rgb.c b/drivers/leds/rgb/leds-s2m-rgb.c
> new file mode 100644
> index 0000000000000..e7295d0b150da
> --- /dev/null
> +++ b/drivers/leds/rgb/leds-s2m-rgb.c
> @@ -0,0 +1,416 @@

[ ... ]

> +static int s2mu005_rgb_apply_params(struct s2m_rgb *rgb)
> +{
> +	struct regmap *regmap = rgb->regmap;
> +	unsigned int ramp_val = 0;
> +	unsigned int stay_val = 0;
> +	int ret;
> +
> +	ramp_val |= FIELD_PREP(S2MU005_RGB_CH_RAMP_UP, rgb->ramp_up);
> +	ramp_val |= FIELD_PREP(S2MU005_RGB_CH_RAMP_DN, rgb->ramp_dn);
> +
> +	stay_val |= FIELD_PREP(S2MU005_RGB_CH_STAY_HI, rgb->stay_hi);
> +	stay_val |= FIELD_PREP(S2MU005_RGB_CH_STAY_LO, rgb->stay_lo);
> +
> +	ret = regmap_write(regmap, S2MU005_REG_RGB_EN, S2MU005_RGB_RESET);
> +	if (ret) {

[ ... ]

> +		ret = regmap_write(regmap, S2MU005_REG_RGB_CH_STAY(i), stay_val);
> +		if (ret) {
> +			dev_err(rgb->dev, "failed to set stay timings\n");
> +			return ret;
> +		}
> +	}
> +
> +	ret = regmap_update_bits(regmap, S2MU005_REG_RGB_EN, S2MU005_RGB_SLOPE,
> +				 S2MU005_RGB_SLOPE_SMOOTH);

Could this read-modify-write re-assert the hardware reset?

If the register defaults to volatile, the read part of regmap_update_bits()
might fetch the reset bit as 1 if the hardware doesn't auto-clear it
immediately. Since the previous regmap_write() wiped the register's previous
state, would a single regmap_write() be safer here to apply the slope
configuration?

> +	if (ret) {
> +		dev_err(rgb->dev, "failed to set ramp slope\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int s2m_rgb_lut_get_closest_duration(const u32 *lut, const size_t len, const u32 duration)
> +{
> +	u32 closest_distance = abs(duration - lut[0]);
> +	int closest_index = 0;
> +	int lo = 0;
> +	int hi = len - 1;
> +
> +	/* Bounds checking */
> +	if (duration < lut[0] || duration > lut[len - 1])
> +		return -EINVAL;

Should this clamp the out-of-bounds duration to the closest hardware index
(0 or len - 1) instead of rejecting it?

In s2m_rgb_pattern_set(), the stay_hi duration is calculated by subtracting
the ramp timings from the total time_hi. If the remaining duration falls
below lut_stay_hi[0] (100ms), the lookup function returns -EINVAL and the
pattern application fails entirely.

> +
> +	while (lo <= hi) {

[ ... ]

> +static int s2m_rgb_pattern_set(struct led_classdev *cdev, struct led_pattern *pattern, u32 len,
> +			       int repeat)
> +{
> +	struct s2m_rgb *rgb = to_s2m_rgb(to_s2m_mc(cdev));
> +	const u32 *lut_ramp_up, *lut_ramp_dn, *lut_stay_hi, *lut_stay_lo;
> +	size_t lut_ramp_up_len, lut_ramp_dn_len, lut_stay_hi_len, lut_stay_lo_len;
> +	int brightness_peak = 0;
> +	u32 time_hi = 0, time_lo = 0;
> +	bool ramp_up_en, ramp_dn_en;
> +	int ret;

[ ... ]

> +	for (int i = 0; i < len; i++) {
> +		int brightness = pattern[i].brightness;
> +		u32 delta_t = pattern[i].delta_t;
> +
> +		if (brightness) {
> +			/*
> +			 * The pattern shold define only one non-zero
> +			 * brightness in the HIGH state. The device doesn't
> +			 * have any provisions to handle multiple peak
> +			 * brightness levels.
> +			 */
> +			if (brightness_peak && brightness_peak != brightness)
> +				return -EINVAL;
> +
> +			brightness_peak = brightness;
> +			time_hi += delta_t;
> +			ramp_dn_en = !!delta_t;
> +		} else {
> +			time_lo += delta_t;
> +			ramp_up_en = !!delta_t;
> +		}
> +	}
> +
> +	/* LUTs are specific to device variant. */
> +	lut_ramp_up = s2mu005_rgb_lut_ramp;
> +	lut_ramp_up_len = ARRAY_SIZE(s2mu005_rgb_lut_ramp);
> +	lut_ramp_dn = s2mu005_rgb_lut_ramp;
> +	lut_ramp_dn_len = ARRAY_SIZE(s2mu005_rgb_lut_ramp);
> +	lut_stay_hi = s2mu005_rgb_lut_stay_hi;
> +	lut_stay_hi_len = ARRAY_SIZE(s2mu005_rgb_lut_stay_hi);
> +	lut_stay_lo = s2mu005_rgb_lut_stay_lo;
> +	lut_stay_lo_len = ARRAY_SIZE(s2mu005_rgb_lut_stay_lo);
> +
> +	mutex_lock(&rgb->lock);
> +
> +	/*
> +	 * The timings ramp_up, stay_hi, and ramp_dn of the HIGH state are
> +	 * roughly equal. Firstly, calculate and set timings for ramp_up and
> +	 * ramp_dn (making sure they're exactly equal).
> +	 */
> +	rgb->ramp_up = 0;
> +	rgb->ramp_dn = 0;
> +
> +	if (ramp_up_en) {

Can ramp_up_en or ramp_dn_en be read uninitialized here?

If a user provides a pattern consisting only of non-zero brightness values,
ramp_up_en is never assigned. Conversely, a pattern of all zero brightness
leaves ramp_dn_en uninitialized, which could result in arbitrary timing
values.

> +		ret = s2m_rgb_lut_get_closest_duration(lut_ramp_up, lut_ramp_up_len, time_hi / 3);

[ ... ]

> +static int s2m_rgb_brightness_set(struct led_classdev *cdev, enum led_brightness value)
> +{
> +	struct s2m_rgb *rgb = to_s2m_rgb(to_s2m_mc(cdev));
> +	int ret = 0;
> +
> +	if (!value)
> +		return s2m_rgb_pattern_clear(cdev);
> +
> +	mutex_lock(&rgb->lock);
> +
> +	led_mc_calc_color_components(&rgb->mc, value);
> +	/* Apply params with variant-specific implementation. */
> +	ret = s2mu005_rgb_apply_params(rgb);

Does this need to clear the internal pattern timing variables (rgb->ramp_up,
rgb->ramp_dn, etc.) before calling s2mu005_rgb_apply_params()?

If a hardware pattern was previously running, it looks like setting a solid
brightness will re-apply the old pattern's timings with the new peak
brightness, causing the LED to continue blinking instead of emitting a
constant light.

> +
> +	mutex_unlock(&rgb->lock);
> +
> +	return ret;
> +}
> +
> +static struct mc_subled s2mu005_rgb_subled_info[] = {
> +	{ .channel = 0, .color_index = LED_COLOR_ID_BLUE },
> +	{ .channel = 1, .color_index = LED_COLOR_ID_GREEN },
> +	{ .channel = 2, .color_index = LED_COLOR_ID_RED },
> +};
> +
> +static int s2m_rgb_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sec_pmic_dev *pmic_drvdata = dev_get_drvdata(dev->parent);
> +	struct s2m_rgb *rgb;
> +	struct led_init_data init_data = {};
> +	int ret;
> +
> +	rgb = devm_kzalloc(dev, sizeof(*rgb), GFP_KERNEL);
> +	if (!rgb)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, rgb);
> +	rgb->dev = dev;
> +	rgb->regmap = pmic_drvdata->regmap_pmic;
> +
> +	/* Configure variant-specific details. */
> +	rgb->mc.subled_info = s2mu005_rgb_subled_info;

Could this lead to a data race if there are multiple device instances?

The multicolor LED core function led_mc_calc_color_components() mutates
the .brightness fields of the subled_info array elements. Since this assigns
a pointer to a static global array, multiple devices might concurrently modify
the shared array.

Would it be better to dynamically allocate a per-device copy using
devm_kmemdup()?

> +	rgb->mc.num_colors = ARRAY_SIZE(s2mu005_rgb_subled_info);

[ ... ]

> +static const struct of_device_id s2m_rgb_of_match_table[] = {
> +	{ .compatible = "samsung,s2mu005-rgb", .data = (void *)S2MU005 },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, s2m_rgb_of_match_table);
> +
> +static struct platform_driver s2m_rgb_driver = {
> +	.driver = {
> +		.name = "s2m-rgb",
> +	},
> +	.probe = s2m_rgb_probe,
> +	.id_table = s2m_rgb_id_table,
> +};

Is the .of_match_table field missing here?

Without assigning s2m_rgb_of_match_table to the driver struct, native Device
Tree matching might fail and prevent the of_node from being correctly
associated with the platform device.

> +module_platform_driver(s2m_rgb_driver);

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