Re: [PATCH v7 07/10] 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 8 potential issue(s) to consider:
- [High] The driver uses the `abs()` macro on unsigned integer subtractions, causing a build failure and integer underflow.
- [High] Unconditional dereference of `dev->parent` without validation causes a kernel panic if instantiated via a standalone DT node.
- [High] Setting a solid brightness does not clear previously configured hardware pattern timings, causing the LED to continue blinking.
- [High] Missing Kconfig dependency on `LEDS_CLASS_MULTICOLOR`.
- [Medium] The driver silently aggregates complex multi-pulse patterns and forcibly alters blink semantics instead of rejecting unsupported patterns.
- [Medium] Software and hardware pattern states become desynchronized on error paths.
- [Medium] The driver incorrectly uses the array loop index instead of the `channel` field for hardware register offsets.
- [Low] The `of_match_table` is exported but not assigned to the platform driver.
--

commit d2d65a1923271678db5e8b3090e9b23768e600c4
Author: Kaustabh Chakraborty <kauschluss@disroot.org>
leds: rgb: add support for Samsung S2M series PMIC RGB LED device

This commit adds support for the RGB LEDs found in Samsung S2M series PMICs,
starting with the S2MU005. It controls three LED channels as a single
device, providing 8-bit brightness control for each channel.

> diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
> index 28ef4c487367c..b16144b48b8f8 100644
> --- a/drivers/leds/rgb/Kconfig
> +++ b/drivers/leds/rgb/Kconfig
> @@ -75,6 +75,16 @@ 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

Should this also depend on LEDS_CLASS_MULTICOLOR?

The driver uses multicolor LED APIs like led_mc_calc_color_components and
devm_led_classdev_multicolor_register_ext. Without a Kconfig dependency on
CONFIG_LEDS_CLASS_MULTICOLOR, building the kernel with the multicolor class
disabled could result in silent functional failures or link errors.

> diff --git a/drivers/leds/rgb/leds-s2m-rgb.c b/drivers/leds/rgb/leds-s2m-rgb.c
> new file mode 100644
> index 0000000000000..d239f54eee901
> --- /dev/null
> +++ b/drivers/leds/rgb/leds-s2m-rgb.c
[ ... ]
> +static int s2mu005_rgb_apply_params(struct s2m_rgb *rgb)
> +{
[ ... ]
> +	for (int i = 0; i < rgb->mc.num_colors; i++) {
> +		ret = regmap_write(regmap, S2MU005_REG_RGB_CH_CTRL(i),
> +				   rgb->mc.subled_info[i].brightness);

Is it safe to use the loop index i to calculate the hardware channel?

While i currently matches the channel sequentially, any reordering of the
subled_info array might cause the index to map to the wrong physical channel.

Would it be better to extract and use rgb->mc.subled_info[i].channel instead?

[ ... ]
> +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]);

Can this cause a compile-time error and integer underflow?

Both duration and lut[0] are u32 variables, and subtracting a larger unsigned
integer from a smaller one will wrap around. Additionally, the abs macro in
linux/math.h is designed to reject unsigned types and evaluates to void.

Could abs_diff be used here instead?

[ ... ]
> +static int s2m_rgb_pattern_set(struct led_classdev *cdev, struct led_pattern *pattern, u32 len,
> +			       int repeat)
> +{
[ ... ]
> +	for (int i = 0; i < len; i++) {
> +		int brightness = pattern[i].brightness;
> +		u32 delta_t = pattern[i].delta_t;
> +
> +		if (brightness) {
> +			/*
> +			 * The pattern should 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;
> +		}
> +	}

Instead of accumulating durations to force a breathing effect, should the
driver reject unsupported patterns by returning -EINVAL?

If a sequence with more than two states or sharp step transitions is requested,
rejecting it would allow the LED core to correctly fall back to
software-driven blinking instead of silently distorting the sequence.

[ ... ]
> +param_fail:
> +	rgb->ramp_up = 0;
> +	rgb->ramp_dn = 0;
> +	rgb->stay_hi = 0;
> +	rgb->stay_lo = 0;
> +
> +	mutex_unlock(&rgb->lock);
> +
> +	return ret;
> +}

Could this error path leave the software and hardware pattern states
desynchronized?

Zeroing out the timing state variables without clearing the hardware pattern
means the hardware continues blinking. A subsequent brightness update might
then unintentionally alter the running pattern by writing this zeroed state.

[ ... ]
> +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 previously configured hardware pattern timings when
setting a solid brightness?

If a hardware blink pattern was previously active, the old non-zero timings
will be written back to the registers along with the new peak brightness,
causing the LED to continue blinking instead of emitting a solid color.

[ ... ]
> +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);

What happens if this driver is instantiated via a standalone device tree
node rather than as an MFD child?

If dev->parent is NULL, this unconditional dereference will cause a kernel
panic.

Should dev->parent be validated before use?

[ ... ]
> +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 assignment missing from the driver struct?

s2m_rgb_of_match_table is declared earlier in the file to support explicit
device tree matching but is not assigned here, preventing direct Open Firmware
matching.

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