| Message ID | 20250610174319.183375-1-trannamatk@gmail.com |
|---|---|
| Headers | show |
| Series | leds: add new LED driver for TI LP5812 | expand |
Please disregard the mistakenly sent patch named "[PATCH v5] test" — it was sent in error and is not part of this series. Apologies for the noise. -- Nam Tran
On 6/10/25 10:43 AM, Nam Tran wrote: > diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig > index 222d943d826a..becee5c1d21c 100644 > --- a/drivers/leds/rgb/Kconfig > +++ b/drivers/leds/rgb/Kconfig > @@ -26,6 +26,19 @@ config LEDS_KTD202X > To compile this driver as a module, choose M here: the module > will be called leds-ktd202x. > > +config LEDS_LP5812 > + tristate "LED support for Texas Instruments LP5812" > + depends on I2C > + help > + If you say Y here you get support for TI LP5812 LED driver. > + The LP5812 is a 4 × 3 matrix RGB LED driver with autonomous The '×' character does not display well (not at all) in menuconfig or nconfig. The graphical configs (gconfig, xconfig) can display it. I would change it to 4x3 (letter 'x') but it's up to you. > + animation engine control. > + > + To compile this driver as a module, choose M here: the > + module will be called leds-lp5812. > + > + If unsure, say N.
Le 10/06/2025 à 19:43, Nam Tran a écrit : > The LP5812 is a 4×3 matrix RGB LED driver with an autonomous animation > engine and time-cross-multiplexing (TCM) support for up to 12 LEDs or > 4 RGB LEDs. Each LED can be configured through the related registers > to realize vivid and fancy lighting effects. Hi, ... > +static struct lp5812_data *lp5812_of_populate_pdata(struct device *dev, > + struct device_node *np, > + struct lp5812_chip *chip) > +{ > + struct device_node *child; > + struct lp5812_data *pdata; > + struct lp5812_led_config *cfg; > + int num_channels, i = 0, ret; > + > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); > + if (!pdata) > + return ERR_PTR(-ENOMEM); > + > + num_channels = of_get_available_child_count(np); > + if (num_channels == 0) { > + dev_err(dev, "no LED channels\n"); > + return ERR_PTR(-EINVAL); > + } > + > + cfg = devm_kcalloc(dev, num_channels, sizeof(*cfg), GFP_KERNEL); > + if (!cfg) > + return ERR_PTR(-ENOMEM); > + > + pdata->led_config = &cfg[0]; > + pdata->num_channels = num_channels; > + > + for_each_available_child_of_node(np, child) { Maybe for_each_available_child_of_node_scoped() to slihtly simplify the code? > + ret = lp5812_parse_logical_led(child, cfg, i); > + if (ret) { > + of_node_put(child); > + return ERR_PTR(-EINVAL); > + } > + i++; > + } > + > + of_property_read_string(np, "label", &pdata->label); > + > + return pdata; > +} ... > +static ssize_t lp5812_aeu_slope_time(struct device *dev, > + struct device_attribute *attr, > + enum slope_time_num slope_chan, > + const char *buf, size_t len) > +{ > + struct lp5812_led *led; > + struct lp5812_chip *chip; > + struct lp5812_led_config *led_cfg; > + const char *name = dev->platform_data; > + int val[LED_COLOR_ID_MAX]; > + u8 chan_nr = 0; > + char *sub_str, *str = (char *)buf; > + int i, ret, aeu; > + union slope_time slope_time_val; > + u16 reg; > + > + if (strcmp(name, LP5812_SC_LED) == 0) > + led = dev_to_lp5812_led(dev); > + else > + led = dev_to_lp5812_led_mc(dev); > + > + chan_nr = led->chan_nr; > + chip = led->chip; > + led_cfg = &chip->pdata->led_config[chan_nr]; > + > + sub_str = strsep(&str, ":"); > + if (!sub_str) > + return -EINVAL; > + if (kstrtoint(&sub_str[3], 0, &aeu)) > + return -EINVAL; > + > + pr_info("AEU = %d", aeu); > + > + guard(mutex)(&chip->lock); > + for (i = 0; i < led_cfg->num_colors; i++) { > + sub_str = strsep(&str, " "); > + if (!sub_str) > + return -EINVAL; > + if (kstrtoint(sub_str, 0, &val[i])) > + return -EINVAL; > + if (val[i] < 0 || val[i] > 15) > + return -EINVAL; > + > + reg = LP5812_AEU_SLOPE_TIME_ADDR(led_cfg->led_id[i], aeu, slope_chan); > + > + /* get original value of slope time */ > + ret = lp5812_read(chip, reg, &slope_time_val.time_val); > + if (ret) > + return ret; > + > + /* Update new value for slope time*/ > + if (slope_chan == LP5812_SLOPE_TIME_T1 || slope_chan == LP5812_SLOPE_TIME_T3) > + slope_time_val.s_time.first = val[i]; > + if (slope_chan == LP5812_SLOPE_TIME_T2 || slope_chan == LP5812_SLOPE_TIME_T4) > + slope_time_val.s_time.second = val[i]; > + > + /* Save updated value to hardware */ > + ret = lp5812_write(chip, reg, slope_time_val.time_val); Should we do something if ret != 0? > + } > + > + return len; > +} ... > +static struct attribute *lp5812_led_attrs[] = { > + &dev_attr_led_current.attr, > + &dev_attr_max_current.attr, > + &dev_attr_mode.attr, > + &dev_attr_activate.attr, > + &dev_attr_pwm_dimming_scale.attr, > + &dev_attr_pwm_phase_align.attr, > + &dev_attr_auto_time_pause_at_start.attr, > + &dev_attr_auto_time_pause_at_stop.attr, > + &dev_attr_auto_playback_eau_number.attr, > + &dev_attr_auto_playback_time.attr, > + &dev_attr_aeu_playback_time.attr, > + &dev_attr_aeu_pwm1.attr, > + &dev_attr_aeu_pwm2.attr, > + &dev_attr_aeu_pwm3.attr, > + &dev_attr_aeu_pwm4.attr, > + &dev_attr_aeu_pwm5.attr, > + &dev_attr_aeu_slop_time_t1.attr, > + &dev_attr_aeu_slop_time_t2.attr, > + &dev_attr_aeu_slop_time_t3.attr, > + &dev_attr_aeu_slop_time_t4.attr, > + &dev_attr_lod_lsd.attr, > + NULL, Unneeded trailing comma after a terminator. > +}; > +ATTRIBUTE_GROUPS(lp5812_led); ... > +static int lp5812_init_led(struct lp5812_led *led, struct lp5812_chip *chip, int chan) > +{ > + struct lp5812_data *pdata = chip->pdata; > + struct device *dev = &chip->i2c_cl->dev; > + struct mc_subled *mc_led_info; > + struct led_classdev *led_cdev; > + char name[32]; > + int i, ret = 0; > + > + if (pdata->led_config[chan].name) { > + led->cdev.name = pdata->led_config[chan].name; > + } else { > + snprintf(name, sizeof(name), "%s:channel%d", > + pdata->label ? : chip->i2c_cl->name, chan); > + led->cdev.name = name; Is it fine below when 'name' is defined on the stack and is used... > + } > + > + if (pdata->led_config[chan].is_sc_led == 0) { > + mc_led_info = devm_kcalloc(dev, > + pdata->led_config[chan].num_colors, > + sizeof(*mc_led_info), GFP_KERNEL); > + if (!mc_led_info) > + return -ENOMEM; > + > + led_cdev = &led->mc_cdev.led_cdev; > + led_cdev->name = led->cdev.name; ...here? > + led_cdev->brightness_set_blocking = lp5812_set_mc_brightness; > + led->mc_cdev.num_colors = pdata->led_config[chan].num_colors; > + for (i = 0; i < led->mc_cdev.num_colors; i++) { > + mc_led_info[i].color_index = > + pdata->led_config[chan].color_id[i]; > + mc_led_info[i].channel = > + pdata->led_config[chan].led_id[i]; > + } > + > + led->mc_cdev.subled_info = mc_led_info; > + } else { > + led->cdev.brightness_set_blocking = lp5812_set_brightness; > + } > + > + led->cdev.groups = lp5812_led_groups; > + led->chan_nr = chan; > + > + if (pdata->led_config[chan].is_sc_led) { > + ret = devm_led_classdev_register(dev, &led->cdev); > + if (ret == 0) { > + led->cdev.dev->platform_data = devm_kstrdup(dev, LP5812_SC_LED, GFP_KERNEL); > + if (!led->cdev.dev->platform_data) > + return -ENOMEM; > + } > + } else { > + ret = devm_led_classdev_multicolor_register(dev, &led->mc_cdev); > + if (ret == 0) { > + led->mc_cdev.led_cdev.dev->platform_data = > + devm_kstrdup(dev, LP5812_MC_LED, GFP_KERNEL); > + if (!led->mc_cdev.led_cdev.dev->platform_data) > + return -ENOMEM; > + > + ret = sysfs_create_groups(&led->mc_cdev.led_cdev.dev->kobj, > + lp5812_led_groups); > + if (ret) > + dev_err(dev, "sysfs_create_groups failed\n"); > + } > + } > + > + if (ret) { > + dev_err(dev, "led register err: %d\n", ret); > + return ret; > + } > + > + return 0; > +} ... CJ
On 10/06/2025 19:43, Nam Tran wrote: > This patch series adds support for the TI/National Semiconductor LP5812 > 4x3 matrix RGB LED driver. The driver supports features such as autonomous > animation and time-cross-multiplexing (TCM) for dynamic LED effects. > > Following feedback from both the LED and auxdisplay subsystem maintainers, > the driver has been moved back to the LED subsystem, under drivers/leds/rgb/. > This version integrates with the existing multicolor LED APIs, avoiding custom > sysfs where standard LED interfaces are sufficient. > > Signed-off-by: Nam Tran <trannamatk@gmail.com> > --- > Changes in v9: > - Move driver back to drivers/leds/rgb/ > - Integrate with LED multicolor framework > - Refactor and simplify custom sysfs handling > - Extend Device Tree binding to support multi-led@ nodes using leds-class-multicolor.yaml You need to provide reason why you dropped reviews. Best regards, Krzysztof
On Wed, Jun 11, 2025 at 12:43:15AM GMT, Nam Tran wrote: > +patternProperties: > + "^led@[0-3]$": > + type: object > + $ref: common.yaml# > + unevaluatedProperties: false > + > + properties: > + led-cur: > + $ref: /schemas/types.yaml#/definitions/uint8 > + description: | > + LED current in 0.1 mA steps (e.g., 150 = 15.0 mA; 0 if not connected) > + minimum: 0 > + maximum: 255 > + > + max-cur: > + $ref: /schemas/types.yaml#/definitions/uint8 > + description: Maximum allowed current in 0.1 mA steps > + > + reg: > + minimum: 0 > + maximum: 3 Place properties according to DTS coding style. > + > + '^multi-led@[4-7]$': > + type: object > + $ref: leds-class-multicolor.yaml# > + unevaluatedProperties: false > + > + properties: > + reg: > + minimum: 4 > + maximum: 7 > + > + '#address-cells': Don't mix quotes. Either ' or " > + const: 1 > + > + '#size-cells': > + const: 0 > + > + patternProperties: > + "^led@[4-9a-f]$": > + type: object > + $ref: common.yaml# > + unevaluatedProperties: false > + > + properties: > + led-cur: > + $ref: /schemas/types.yaml#/definitions/uint8 No, use existing led common properties. Also observe the units - this is not uint8 but a defined type for microamp, see property-units in dtschema. > + description: | > + LED current in 0.1 mA steps (e.g., 150 = 15.0 mA; 0 if not connected) > + minimum: 0 > + maximum: 255 > + > + max-cur: > + $ref: /schemas/types.yaml#/definitions/uint8 No, use existing led common properties. Same everywhere. > + description: Maximum allowed current in 0.1 mA steps > + > + reg: > + minimum: 4 > + maximum: 15 > + > + required: > + - reg > + > +required: > + - compatible > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/leds/common.h> > + > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + led-controller@1b { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "ti,lp5812"; > + reg = <0x1b>; > + vcc-supply = <&vdd_3v3_reg>; > + > + led@0 { > + reg = <0x0>; Messed/mixed indentation. BTW, such significant binding change at v9, invalidting reviews and rewriting the binding completely, is surprising. Best regards, Krzysztof
This patch series adds support for the TI/National Semiconductor LP5812 4x3 matrix RGB LED driver. The driver supports features such as autonomous animation and time-cross-multiplexing (TCM) for dynamic LED effects. Following feedback from both the LED and auxdisplay subsystem maintainers, the driver has been moved back to the LED subsystem, under drivers/leds/rgb/. This version integrates with the existing multicolor LED APIs, avoiding custom sysfs where standard LED interfaces are sufficient. Signed-off-by: Nam Tran <trannamatk@gmail.com> --- Changes in v9: - Move driver back to drivers/leds/rgb/ - Integrate with LED multicolor framework - Refactor and simplify custom sysfs handling - Extend Device Tree binding to support multi-led@ nodes using leds-class-multicolor.yaml - Update documentation to reflect the updated sysfs - Link to v8: https://lore.kernel.org/lkml/20250427082447.138359-1-trannamatk@gmail.com/ Changes in v8: - Move driver to drivers/auxdisplay/ instead of drivers/leds/. - Rename files from leds-lp5812.c/.h to lp5812.c/.h. - Move ti,lp5812.yaml binding to auxdisplay/ directory, and update the title and $id to match new path. - No functional changes to the binding itself (keep Reviewed-by). - Update commit messages and patch titles to reflect the move. - Link to v7: https://lore.kernel.org/linux-leds/20250422190121.46839-1-trannamatk@gmail.com/ Changes in v7: - Mark `chip_leds_map` as const. - Use consistent `ret` initialization. - Simplify the function `set_mix_sel_led()`. - Refactor `dev_config_show()` and `led_auto_animation_show()` to avoid temp buffer, malloc/free. - Simplify the code and ensure consistent use of mutex lock/unlock in show/store functions. - Remove `total_leds` and `total_aeu`. - Link to v6: https://lore.kernel.org/linux-leds/20250419184333.56617-1-trannamatk@gmail.com/ Changes in v6: - Add `vcc-supply` property to describe the LP5812 power supply. - Remove `chan-name` property and entire LED subnodes, as they are not needed. - Update LP5812 LED driver node to Raspberry Pi 4 B Device Tree, based on updated binding. - Link to v5: https://lore.kernel.org/linux-leds/20250414145742.35713-1-trannamatk@gmail.com/ Changes in v5: - Rebase on v6.15-rc2 - Removed unused functions (lp5812_dump_regs, lp5812_update_bit). - Address Krzysztof's review comments - Link to v4: https://lore.kernel.org/linux-leds/20250405183246.198568-1-trannamatk@gmail.com/ --- Nam Tran (4): dt-bindings: leds: add TI/National Semiconductor LP5812 LED Driver leds: add TI/National Semiconductor LP5812 LED Driver docs: ABI: Document LP5812 LED sysfs interfaces docs: leds: Document TI LP5812 LED driver .../ABI/testing/sysfs-bus-i2c-devices-lp5812 | 40 + .../ABI/testing/sysfs-class-led-lp5812 | 120 + .../devicetree/bindings/leds/ti,lp5812.yaml | 264 +++ Documentation/leds/index.rst | 1 + Documentation/leds/leds-lp5812.rst | 84 + MAINTAINERS | 13 + drivers/leds/rgb/Kconfig | 13 + drivers/leds/rgb/Makefile | 1 + drivers/leds/rgb/leds-lp5812.c | 1946 +++++++++++++++++ drivers/leds/rgb/leds-lp5812.h | 228 ++ 10 files changed, 2710 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-lp5812 create mode 100644 Documentation/ABI/testing/sysfs-class-led-lp5812 create mode 100644 Documentation/devicetree/bindings/leds/ti,lp5812.yaml create mode 100644 Documentation/leds/leds-lp5812.rst create mode 100644 drivers/leds/rgb/leds-lp5812.c create mode 100644 drivers/leds/rgb/leds-lp5812.h base-commit: f09079bd04a924c72d555cd97942d5f8d7eca98c