From: Lee Jones <lee@kernel.org>
To: Daniel Mack <daniel.mack@holoplot.com>
Cc: linux-leds@vger.kernel.org, pavel@kernel.org,
Daniel Mack <daniel@zonque.org>
Subject: Re: [PATCH] leds: is31f132xx: add support for is31fl3293
Date: Thu, 13 Nov 2025 15:49:43 +0000 [thread overview]
Message-ID: <20251113154943.GL1949330@google.com> (raw)
In-Reply-To: <20251104142136.649551-1-daniel.mack@holoplot.com>
On Tue, 04 Nov 2025, Daniel Mack wrote:
> From: Daniel Mack <daniel@zonque.org>
>
> This chip supports 3 LED channels with 4096 possible PWM values.
>
> Extend the driver to support this variant:
>
> * Make brightness steps configurable per device type
> * Handle dual-register brightness updates
> * Allow to specify values to write into the PWM update register
> * Add custom init and shutdown function for 3293 variant
> * Init registers after parsing DT properties
>
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
> drivers/leds/leds-is31fl32xx.c | 138 +++++++++++++++++++++++++++++++--
> 1 file changed, 130 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index 8793330dd414..8630ca068624 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c
> @@ -32,10 +32,24 @@
> #define IS31FL3216_CONFIG_SSD_ENABLE BIT(7)
> #define IS31FL3216_CONFIG_SSD_DISABLE 0
>
> +/* Registers for IS31FL3293 */
> +#define IS31FL3293_SHUTDOWN_REG 0x01
> +#define IS31FL3293_SHUTDOWN_SSD_DISABLE BIT(0)
> +#define IS31FL3293_SHUTDOWN_EN1 BIT(4)
> +#define IS31FL3293_SHUTDOWN_EN2 BIT(5)
> +#define IS31FL3293_SHUTDOWN_EN3 BIT(6)
> +#define IS31FL3293_GCC_REG 0x03
> +#define IS31FL3293_CL_REG 0x10
> +#define IS31FL3293_COLOR_UPDATE_REG 0x27
> +#define IS31FL3293_COLOR_UPDATE_MAGIC 0xc5
> +#define IS31FL3293_RESET_REG 0x3c
> +#define IS31FL3293_RESET_MAGIC 0xc5
> +
> struct is31fl32xx_priv;
> struct is31fl32xx_led_data {
> struct led_classdev cdev;
> u8 channel; /* 1-based, max priv->cdef->channels */
> + u32 max_microamp;
> struct is31fl32xx_priv *priv;
> };
>
> @@ -51,12 +65,14 @@ struct is31fl32xx_priv {
> * @channels : Number of LED channels
> * @shutdown_reg : address of Shutdown register (optional)
> * @pwm_update_reg : address of PWM Update register
> + * @pwm_update_value : value to write to PWM Update register
> * @global_control_reg : address of Global Control register (optional)
> * @reset_reg : address of Reset register (optional)
> * @pwm_register_base : address of first PWM register
> * @pwm_registers_reversed: : true if PWM registers count down instead of up
> * @led_control_register_base : address of first LED control register (optional)
> * @enable_bits_per_led_control_register: number of LEDs enable bits in each
> + * @brightness_steps : number of brightness steps supported by the chip
> * @reset_func : pointer to reset function
> * @sw_shutdown_func : pointer to software shutdown function
> *
> @@ -74,12 +90,14 @@ struct is31fl32xx_chipdef {
> u8 channels;
> u8 shutdown_reg;
> u8 pwm_update_reg;
> + u8 pwm_update_value;
> u8 global_control_reg;
> u8 reset_reg;
> u8 pwm_register_base;
> bool pwm_registers_reversed;
> u8 led_control_register_base;
> u8 enable_bits_per_led_control_register;
> + u16 brightness_steps;
> int (*reset_func)(struct is31fl32xx_priv *priv);
> int (*sw_shutdown_func)(struct is31fl32xx_priv *priv, bool enable);
Grim!
> };
> @@ -93,6 +111,7 @@ static const struct is31fl32xx_chipdef is31fl3236_cdef = {
> .pwm_register_base = 0x01,
> .led_control_register_base = 0x26,
> .enable_bits_per_led_control_register = 1,
> + .brightness_steps = 256,
> };
>
> static const struct is31fl32xx_chipdef is31fl3235_cdef = {
> @@ -104,6 +123,7 @@ static const struct is31fl32xx_chipdef is31fl3235_cdef = {
> .pwm_register_base = 0x05,
> .led_control_register_base = 0x2a,
> .enable_bits_per_led_control_register = 1,
> + .brightness_steps = 256,
> };
>
> static const struct is31fl32xx_chipdef is31fl3218_cdef = {
> @@ -115,6 +135,7 @@ static const struct is31fl32xx_chipdef is31fl3218_cdef = {
> .pwm_register_base = 0x01,
> .led_control_register_base = 0x13,
> .enable_bits_per_led_control_register = 6,
> + .brightness_steps = 256,
> };
>
> static int is31fl3216_reset(struct is31fl32xx_priv *priv);
> @@ -132,6 +153,24 @@ static const struct is31fl32xx_chipdef is31fl3216_cdef = {
> .enable_bits_per_led_control_register = 8,
> .reset_func = is31fl3216_reset,
> .sw_shutdown_func = is31fl3216_software_shutdown,
> + .brightness_steps = 256,
> +};
> +
> +static int is31fl3293_reset(struct is31fl32xx_priv *priv);
> +static int is31fl3293_software_shutdown(struct is31fl32xx_priv *priv,
> + bool enable);
Forward declarations are bad.
> +static const struct is31fl32xx_chipdef is31fl3293_cdef = {
> + .channels = 3,
> + .shutdown_reg = IS31FL32XX_REG_NONE,
> + .pwm_update_reg = 0x28,
> + .pwm_update_value = 0xc5,
> + .global_control_reg = IS31FL32XX_REG_NONE,
> + .reset_reg = IS31FL32XX_REG_NONE,
> + .pwm_register_base = 0x19,
> + .led_control_register_base = IS31FL32XX_REG_NONE,
> + .brightness_steps = 4096,
> + .reset_func = is31fl3293_reset,
> + .sw_shutdown_func = is31fl3293_software_shutdown,
> };
>
> static int is31fl32xx_write(struct is31fl32xx_priv *priv, u8 reg, u8 val)
> @@ -198,6 +237,61 @@ static int is31fl3216_software_shutdown(struct is31fl32xx_priv *priv,
> return is31fl32xx_write(priv, IS31FL3216_CONFIG_REG, value);
> }
>
> +/*
> + * Custom Reset function for IS31FL3293. We need to set the global current limit
> + * and write to the color update register once.
> + */
> +static int is31fl3293_reset(struct is31fl32xx_priv *priv)
> +{
> + int i, ret;
> +
> + ret = is31fl32xx_write(priv, IS31FL3293_RESET_REG,
> + IS31FL3293_RESET_MAGIC);
> + if (ret)
> + return ret;
> +
> + /* Set the global current limit to maximum */
> + ret = is31fl32xx_write(priv, IS31FL3293_GCC_REG, 0x3f);
Nit: No magic numbers. Please define it.
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < priv->num_leds; i++) {
> + struct is31fl32xx_led_data *led_data = &priv->leds[i];
> + int cl_reg = IS31FL3293_CL_REG + led_data->channel - 1;
What's cl? Help the reader out with clear nomenclature throughout.
> + int ma = max(led_data->max_microamp, 20000);
And here.
> + int cl = (ma * 0xff) / 20000;
And here.
Also, no magic numbers, throughout.
> + ret = is31fl32xx_write(priv, cl_reg, cl);
> + if (ret)
> + return ret;
> + }
> +
> + ret = is31fl32xx_write(priv, IS31FL3293_COLOR_UPDATE_REG,
> + IS31FL3293_COLOR_UPDATE_MAGIC);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +/*
> + * Custom Software-Shutdown function for IS31FL3293 because the SHUTDOWN
> + * register of this device also has bits to enable the channels.
> + */
> +static int is31fl3293_software_shutdown(struct is31fl32xx_priv *priv,
> + bool enable)
> +{
> + u8 value = 0;
> +
> + if (!enable)
> + value = IS31FL3293_SHUTDOWN_SSD_DISABLE |
> + IS31FL3293_SHUTDOWN_EN1 |
> + IS31FL3293_SHUTDOWN_EN2 |
> + IS31FL3293_SHUTDOWN_EN3;
> +
> + return is31fl32xx_write(priv, IS31FL3293_SHUTDOWN_REG, value);
> +}
> +
> /*
> * NOTE: A mutex is not needed in this function because:
> * - All referenced data is read-only after probe()
> @@ -236,13 +330,36 @@ static int is31fl32xx_brightness_set(struct led_classdev *led_cdev,
> else
> pwm_register_offset = led_data->channel - 1;
>
> - ret = is31fl32xx_write(led_data->priv,
> - cdef->pwm_register_base + pwm_register_offset,
> - brightness);
> - if (ret)
> - return ret;
> + switch (cdef->brightness_steps) {
> + case 256:
> + ret = is31fl32xx_write(led_data->priv,
> + cdef->pwm_register_base + pwm_register_offset,
> + brightness);
> + if (ret)
> + return ret;
> +
> + break;
> + case 4096:
> + /* IS31FL329x devices use two registers to store 12 bits of brightness */
> + pwm_register_offset *= 2;
>
> - return is31fl32xx_write(led_data->priv, cdef->pwm_update_reg, 0);
> + ret = is31fl32xx_write(led_data->priv,
> + cdef->pwm_register_base + pwm_register_offset,
> + brightness & 0xff);
> + if (ret)
> + return ret;
> +
> + ret = is31fl32xx_write(led_data->priv,
> + cdef->pwm_register_base + pwm_register_offset + 1,
> + (brightness >> 8) & 0xf);
> + if (ret)
> + return ret;
> +
> + break;
> + }
> +
> + return is31fl32xx_write(led_data->priv, cdef->pwm_update_reg,
> + cdef->pwm_update_value);
> }
>
> static int is31fl32xx_reset_regs(struct is31fl32xx_priv *priv)
> @@ -341,6 +458,8 @@ static int is31fl32xx_parse_child_dt(const struct device *dev,
> }
> led_data->channel = reg;
>
> + of_property_read_u32(child, "led-max-microamp", &led_data->max_microamp);
> +
> cdev->brightness_set_blocking = is31fl32xx_brightness_set;
>
> return 0;
> @@ -372,6 +491,7 @@ static int is31fl32xx_parse_dt(struct device *dev,
> const struct is31fl32xx_led_data *other_led_data;
>
> led_data->priv = priv;
> + led_data->cdev.max_brightness = priv->cdef->brightness_steps-1;
Spaces around the '-'. Did you run checkpatch.pl?
> ret = is31fl32xx_parse_child_dt(dev, child, led_data);
> if (ret)
> @@ -404,6 +524,7 @@ static int is31fl32xx_parse_dt(struct device *dev,
> }
>
> static const struct of_device_id of_is31fl32xx_match[] = {
> + { .compatible = "issi,is31fl3293", .data = &is31fl3293_cdef, },
> { .compatible = "issi,is31fl3236", .data = &is31fl3236_cdef, },
> { .compatible = "issi,is31fl3235", .data = &is31fl3235_cdef, },
> { .compatible = "issi,is31fl3218", .data = &is31fl3218_cdef, },
> @@ -438,11 +559,11 @@ static int is31fl32xx_probe(struct i2c_client *client)
> priv->cdef = cdef;
> i2c_set_clientdata(client, priv);
>
> - ret = is31fl32xx_init_regs(priv);
> + ret = is31fl32xx_parse_dt(dev, priv);
> if (ret)
> return ret;
>
> - ret = is31fl32xx_parse_dt(dev, priv);
> + ret = is31fl32xx_init_regs(priv);
> if (ret)
> return ret;
>
> @@ -465,6 +586,7 @@ static void is31fl32xx_remove(struct i2c_client *client)
> * even though it is not used for DeviceTree based instantiation.
> */
> static const struct i2c_device_id is31fl32xx_id[] = {
> + { "is31fl3293" },
> { "is31fl3236" },
> { "is31fl3235" },
> { "is31fl3218" },
> --
> 2.51.1
>
--
Lee Jones [李琼斯]
prev parent reply other threads:[~2025-11-13 15:49 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-04 14:21 [PATCH] leds: is31f132xx: add support for is31fl3293 Daniel Mack
2025-11-13 15:49 ` Lee Jones [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20251113154943.GL1949330@google.com \
--to=lee@kernel.org \
--cc=daniel.mack@holoplot.com \
--cc=daniel@zonque.org \
--cc=linux-leds@vger.kernel.org \
--cc=pavel@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.