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

[Kaustabh Chakraborty]

On 2026-05-07 20:00 +01:00, Lee Jones wrote:
> On Fri, 24 Apr 2026, Kaustabh Chakraborty wrote:

[...]

>> +
>> +	switch (rgb->device_type) {
>> +	case S2MU005:
>> +		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);
>> +		break;
>> +	default:
>> +		/* execution shouldn't reach here */
>
> Instead of a comment, perhaps a WARN_ON_ONCE(1); or similar would be
> more robust here to catch unexpected device types?
>

[...]

>> +static int s2m_rgb_pattern_clear(struct led_classdev *cdev)
>> +{
>> +	struct s2m_rgb *rgb = to_s2m_rgb(to_s2m_mc(cdev));
>> +	int ret = 0;
>> +
>> +	mutex_lock(&rgb->lock);
>> +
>> +	switch (rgb->device_type) {
>> +	case S2MU005:
>> +		ret = s2mu005_rgb_reset_params(rgb);
>> +		break;
>> +	default:
>> +		/* execution shouldn't reach here */
>> +		break;
>
> As above.
>
> And a single branch switch () makes little sense.

Even with an `if`, since only one variant is supported we're sure that
the control would never go to `else` anyway. I will flatten this block,
and expect the switch to be added when another variant is added.

>> +static struct mc_subled s2mu005_rgb_subled_info[] = {
>
> const?

No, this is fed to (struct led_classdev_mc)::subled_info, which is not a
const pointer. Relevant snip is marked below.

"Assigning to 'struct mc_subled *' from const struct mc_subled[3]
discards qualifiers."


>> +	{ .channel = 0, .color_index = LED_COLOR_ID_BLUE },
>> +	{ .channel = 1, .color_index = LED_COLOR_ID_GREEN },
>> +	{ .channel = 2, .color_index = LED_COLOR_ID_RED },
>> +};

[...]

>> +	switch (rgb->device_type) {
>> +	case S2MU005:
>> +		rgb->mc.subled_info = s2mu005_rgb_subled_info;

Here.

>> +		rgb->mc.num_colors = ARRAY_SIZE(s2mu005_rgb_subled_info);
>> +		break;
>> +	default:
>> +		return dev_err_probe(dev, -ENODEV, "device type %d is not supported by driver\n",
>> +				     pmic_drvdata->device_type);