Re: [PATCH v5 3/4] led: sony-cronos-smc: Add RGB LED driver for Sony Cronos SMC

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

Le 04/12/2025 à 19:50, Timothy Pearson a écrit :
> The Sony Cronos Platform Controller is a multi-purpose platform controller with
> an integrated multi-channel RGB LED controller.  The LED controller is a
> pseudo-RGB device with only two states for each of the RGB subcomponents of
> each LED, but is exposed as a full RGB device for ease of integration with
> userspace software.  Internal thresholding is used to convert the color values
> to the required on/off RGB subcomponent controls.
> 
> Signed-off-by: Timothy Pearson <tpearson-z0qzliK6Om0mgXJStvpl+vpXobYPEAuW@public.gmane.org>
> ---
>   drivers/leds/Kconfig            |  19 ++
>   drivers/leds/Makefile           |   1 +
>   drivers/leds/leds-sony-cronos.c | 378 ++++++++++++++++++++++++++++++++
>   3 files changed, 398 insertions(+)
>   create mode 100644 drivers/leds/leds-sony-cronos.c

Hi,

kernel test robot has complained, so it is still time to suggest some 
small clean-ups, if of any interest.

...

> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 9a0333ec1a86..6dbcf747cab6 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -84,6 +84,7 @@ obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
>   obj-$(CONFIG_LEDS_PWM)			+= leds-pwm.o
>   obj-$(CONFIG_LEDS_QNAP_MCU)		+= leds-qnap-mcu.o
>   obj-$(CONFIG_LEDS_REGULATOR)		+= leds-regulator.o
> +obj-$(CONFIG_LEDS_SONY_CRONOS)		+= leds-sony-cronos.o

Keep alphabetical order?

>   obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
>   obj-$(CONFIG_LEDS_ST1202)		+= leds-st1202.o
>   obj-$(CONFIG_LEDS_SUN50I_A100)		+= leds-sun50i-a100.o
> diff --git a/drivers/leds/leds-sony-cronos.c b/drivers/leds/leds-sony-cronos.c
> new file mode 100644
> index 000000000000..ce71a8b6ce94
> --- /dev/null
> +++ b/drivers/leds/leds-sony-cronos.c
> @@ -0,0 +1,378 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LED driver for Sony Cronos SMCs
> + * Copyright (C) 2012 Dialog Semiconductor Ltd.
> + * Copyright (C) 2023 Sony Interactive Entertainment
> + * Copyright (C) 2025 Raptor Engineering, LLC
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/mfd/sony-cronos.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>

Sometimes, it is preferred to keep #include in alphabetic order.

> +
> +/* Masks and Bit shifts */
> +#define CRONOS_LEDS_STATUS_FLASHING_MASK	0x40
> +#define CRONOS_LEDS_STATUS_FLASHING_SHIFT	6
> +#define CRONOS_LEDS_STATUS_COLOR_MASK		0x07
> +#define CRONOS_LEDS_STATUS_COLOR_SHIFT		0

...

> +enum sony_cronos_led_id {
> +	LED_ID_CCM1_STATUS = 0x00,
> +	LED_ID_CCM2_STATUS,
> +	LED_ID_CCM3_STATUS,
> +	LED_ID_CCM4_STATUS,
> +	LED_ID_SWITCH_STATUS,
> +	LED_ID_SMC_STATUS,
> +
> +	LED_ID_CCM1_LINK,
> +	LED_ID_CCM2_LINK,
> +	LED_ID_CCM3_LINK,
> +	LED_ID_CCM4_LINK,
> +	LED_ID_SWITCH_LINK,
> +
> +	LED_ID_CCM1_POWER,
> +	LED_ID_CCM2_POWER,
> +	LED_ID_CCM3_POWER,
> +	LED_ID_CCM4_POWER,
> +
> +	LED_ID_COUNT,

Unneeded trailing comma after a terminator.
Moreover, LED_ID_COUNT looks unused. Is it needed?

> +};
> +
> +enum sony_cronos_led_type {
> +	LED_TYPE_STATUS,
> +	LED_TYPE_LINK,
> +	LED_TYPE_POWER,
> +};

...

> +static int cronos_led_color_store(struct sony_cronos_smc *chip, struct sony_cronos_led *led)
> +{
> +	u8 byte;
> +	u8 color_mask;
> +	u8 color_shift;
> +	u8 color_key_red;
> +	u8 color_key_green;
> +	u8 color_key_blue;
> +	int ret;
> +
> +	if (led->led_type == LED_TYPE_STATUS) {
> +		color_mask = CRONOS_LEDS_STATUS_COLOR_MASK;
> +		color_shift = CRONOS_LEDS_STATUS_COLOR_SHIFT;
> +	} else if (led->led_type == LED_TYPE_LINK) {
> +		color_mask = CRONOS_LEDS_LINK_COLOR_MASK;
> +		color_shift = CRONOS_LEDS_LINK_COLOR_SHIFT;
> +	} else if (led->led_id == LED_ID_CCM1_POWER) {
> +		color_mask = CRONOS_LEDS_CCM1_POWER_COLOR_MASK;
> +		color_shift = CRONOS_LEDS_CCM1_POWER_COLOR_SHIFT;
> +	} else if (led->led_id == LED_ID_CCM2_POWER) {
> +		color_mask = CRONOS_LEDS_CCM2_POWER_COLOR_MASK;
> +		color_shift = CRONOS_LEDS_CCM2_POWER_COLOR_SHIFT;
> +	} else if (led->led_id == LED_ID_CCM3_POWER) {
> +		color_mask = CRONOS_LEDS_CCM3_POWER_COLOR_MASK;
> +		color_shift = CRONOS_LEDS_CCM3_POWER_COLOR_SHIFT;
> +	} else if (led->led_id == LED_ID_CCM4_POWER) {
> +		color_mask = CRONOS_LEDS_CCM4_POWER_COLOR_MASK;
> +		color_shift = CRONOS_LEDS_CCM4_POWER_COLOR_SHIFT;
> +	} else
> +		return ret;
> +
> +	switch (led->led_type) {
> +	case LED_TYPE_POWER:
> +		color_key_red = LED_COLOR_POWER_RED;
> +		color_key_green = LED_COLOR_POWER_GREEN;
> +		/* Blue channel does not exist for CCM power LEDs */
> +		color_key_blue = LED_COLOR_POWER_OFF;
> +		break;
> +	default:
> +		color_key_red = LED_COLOR_RED;
> +		color_key_green = LED_COLOR_GREEN;
> +		color_key_blue = LED_COLOR_BLUE;
> +	}
> +
> +	/* Assemble SMC color command code */
> +	byte = LED_COLOR_POWER_OFF;
> +	if (led->subled_info[0].brightness > 128)
> +		byte |= color_key_red;
> +	if (led->subled_info[1].brightness > 128)
> +		byte |= color_key_green;
> +	if (led->subled_info[2].brightness > 128)
> +		byte |= color_key_blue;
> +
> +	ret = regmap_update_bits(chip->regmap, led->led_register, color_mask, byte << color_shift);
> +	if (ret) {
> +		dev_err(chip->dev, "Failed to set color value 0x%02x to LED register 0x%02x", byte,

Missing \n at the end of the message.

> +			led->led_register);
> +		return ret;
> +	}
> +	return 0;
> +}

...

> +static const struct of_device_id sony_cronos_led_of_id_table[] = {
> +	{ .compatible = "sie,cronos-led", },
> +	{},

Unneeded trailing comma after a terminator.

> +};
> +MODULE_DEVICE_TABLE(of, sony_cronos_led_of_id_table);

...

CJ