* [PATCH v17 0/3] leds: add new LED driver for TI LP5812
@ 2025-10-21 15:59 Nam Tran
2025-10-21 15:59 ` [PATCH v17 1/3] dt-bindings: leds: add TI/National Semiconductor LP5812 LED Driver Nam Tran
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Nam Tran @ 2025-10-21 15:59 UTC (permalink / raw)
To: lee
Cc: pavel, gregkh, rdunlap, christophe.jaillet, krzk+dt, robh,
conor+dt, corbet, linux-leds, linux-kernel, devicetree, linux-doc,
Nam Tran
This patch series adds initial support for the TI LP5812,
a 4x3 matrix RGB LED driver with autonomous engine control.
This version provides a minimal, clean implementation focused
on core functionality only. The goal is to upstream a solid
foundation, with the expectation that additional features can
be added incrementally in future patches.
The driver integrates with the LED multicolor framework and
supports a set of basic sysfs interfaces for LED control and
chip management.
Signed-off-by: Nam Tran <trannamatk@gmail.com>
---
Changes in v17:
- Dropped direct_mode from ti,scan-mode property.
- Updated logic so that omitting ti,scan-mode now implies direct-drive mode by default.
- Refactor lp5812_parse_led_channel() to simplify function arguments.
- Mark lp5812_cfg as const since it contains only static configuration data.
- Link to v16: https://lore.kernel.org/linux-leds/20251013173551.108205-1-trannamatk@gmail.com/
Changes in v16:
- Renamed property 'ti,operation-mode' to 'ti,scan-mode'.
- Simplified allowed values using regex pattern instead of long enum list.
- Updated description accordingly and noted 'direct_mode' as default.
- Updated core driver to match the binding change.
- Link to v15: https://lore.kernel.org/linux-leds/20251005153337.94025-1-trannamatk@gmail.com/
Changes in v15:
- Removed all custom sysfs attributes; driver now fully relies on standard LED multicolor class interfaces.
- Added new device tree property `ti,operation-mode` to configure direct, TCM, and mix scan modes.
- Dropped previous Reviewed-by tag from the binding patch due to property addition.
- Removed ABI documentation since no new sysfs entries are created.
- Updated lp5812.rst documentation accordingly.
- Link to v14: https://lore.kernel.org/linux-leds/20250907160944.149104-1-trannamatk@gmail.com/
Changes in v14:
- Replaced inline constants with proper macros for readability and maintainability.
- Refactored lp5812_read() and lp5812_write() to simplify logic and improve clarity.
- Updated lp5812_fault_clear() to use switch() instead of if/else chain.
- Refactored parse_drive_mode() for cleaner logic, removed string parsing of concatenated data.
- Updated activate_store() and led_current_store() to replace strsep()/kstrtoint() parsing with sscanf().
- Removed redundant comments and renamed variables for better clarity.
- Link to v13: https://lore.kernel.org/lkml/20250818012654.143058-1-trannamatk@gmail.com/
Changes in v13:
- Fixes build warnings reported by kernel test robot:
- Inconsistent indent in lp5812_probe()
- Uninitialized variable 'ret' in lp5812_multicolor_brightness()
- Drop of_match_ptr() and directly assign of_match_table, as the driver is DT-only.
- Link to v12: https://lore.kernel.org/lkml/20250728065814.120769-1-trannamatk@gmail.com/
Changes in v12:
- Reordered helper functions above lp5812_probe() for better structure.
- Clarified DT-only support by removing fallback paths and i2c_device_id table.
- Directly assign platform_data to the correct pointer instead of relying on
string comparisons (LP5812_SC_LED, LP5812_MC_LED) and container_of() casting.
This simplifies the logic and avoids unnecessary type checks.
- Removed redundant messages.
- Update ABI documentation to reflect reduced feature set.
- Link to v11: https://lore.kernel.org/lkml/20250714172355.84609-1-trannamatk@gmail.com/
Changes in v11:
- Drop autonomous animation and other advanced features; reduce driver to core functionality only.
- Simplify LED parsing to use a unified path.
- Clean up and streamline code
- Use alphabetically ordered includes
- Remove redundant comments
- Fix style issues (e.g., comment capitalization, code placement)
- Update ABI documentation to reflect reduced feature set.
- Link to v10: https://lore.kernel.org/lkml/20250618183205.113344-1-trannamatk@gmail.com/
Changes in v10:
- Address feedback on v9 regarding missing Reviewed-by tag
- Added explanation: binding structure changed significantly to integrate
with the standard leds-class-multicolor.yaml schema and support multi-led@
nodes with nested led@ subnodes. This change introduced a new patternProperties
hierarchy and removed the previous flat led@ layout used in the earlier versions.
So the Reviewed-by tag was dropped out of caution.
- Address binding document feedback
- Use consistent quotes
- Replace 'max-cur' with the standard 'led-max-microamp'
- Remove 'led-cur' property
- Fix mixed indentation
- Updated core driver to align with the updated binding schema.
- Address core driver feedback
- Use for_each_available_child_of_node_scoped() to simplify the code
- Add a return checks for lp5812_write() and lp5812_read()
- Remove unneeded trailing commas
- Fix unsafe usage of stack-allocated strings
- Link to v9: https://lore.kernel.org/lkml/20250617154020.7785-1-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 (3):
dt-bindings: leds: add TI/National Semiconductor LP5812 LED Driver
leds: add basic support for TI/National Semiconductor LP5812 LED
Driver
docs: leds: Document TI LP5812 LED driver
.../devicetree/bindings/leds/ti,lp5812.yaml | 246 ++++++
Documentation/leds/index.rst | 1 +
Documentation/leds/leds-lp5812.rst | 50 ++
MAINTAINERS | 11 +
drivers/leds/rgb/Kconfig | 13 +
drivers/leds/rgb/Makefile | 1 +
drivers/leds/rgb/leds-lp5812.c | 730 ++++++++++++++++++
drivers/leds/rgb/leds-lp5812.h | 197 +++++
8 files changed, 1249 insertions(+)
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: 6548d364a3e850326831799d7e3ea2d7bb97ba08
--
2.25.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v17 1/3] dt-bindings: leds: add TI/National Semiconductor LP5812 LED Driver 2025-10-21 15:59 [PATCH v17 0/3] leds: add new LED driver for TI LP5812 Nam Tran @ 2025-10-21 15:59 ` Nam Tran 2025-10-26 22:06 ` Rob Herring (Arm) 2025-10-21 15:59 ` [PATCH v17 2/3] leds: add basic support for " Nam Tran 2025-10-21 15:59 ` [PATCH v17 3/3] docs: leds: Document TI LP5812 LED driver Nam Tran 2 siblings, 1 reply; 8+ messages in thread From: Nam Tran @ 2025-10-21 15:59 UTC (permalink / raw) To: lee Cc: pavel, gregkh, rdunlap, christophe.jaillet, krzk+dt, robh, conor+dt, corbet, linux-leds, linux-kernel, devicetree, linux-doc, Nam Tran The LP5812 is a 4x3 RGB LED driver with an autonomous animation engine and time-cross-multiplexing (TCM) support for up to 12 LEDs or 4 RGB LEDs. It supports both analog (256 levels) and PWM (8-bit) dimming, including exponential PWM for smooth brightness control. Signed-off-by: Nam Tran <trannamatk@gmail.com> --- .../devicetree/bindings/leds/ti,lp5812.yaml | 246 ++++++++++++++++++ MAINTAINERS | 6 + 2 files changed, 252 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/ti,lp5812.yaml diff --git a/Documentation/devicetree/bindings/leds/ti,lp5812.yaml b/Documentation/devicetree/bindings/leds/ti,lp5812.yaml new file mode 100644 index 000000000000..de34bff441c7 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/ti,lp5812.yaml @@ -0,0 +1,246 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/leds/ti,lp5812.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: TI LP5812 4x3 Matrix RGB LED Driver with Autonomous Control + +maintainers: + - Nam Tran <trannamatk@gmail.com> + +description: | + The LP5812 is a 4x3 matrix RGB LED driver with I2C interface + and autonomous animation engine control. + For more product information please see the link below: + https://www.ti.com/product/LP5812#tech-docs + +properties: + compatible: + const: ti,lp5812 + + reg: + maxItems: 1 + + ti,scan-mode: + description: | + Selects the LED scan mode of the LP5812. The device supports + three modes: + - Direct-drive mode (by default if 'ti,scan-mode' is omitted) + drives up to 4 LEDs directly by internal current sinks (LED0-LED3). + - TCM-drive mode ("tcm:<n>:<order...>") drives up to 12 LEDs + (4 RGB) using 1-4 scan multiplexing. The <n> specifies the number + of scans (1-4), and <order...> defines the scan order of the outputs. + - Mix-drive mode ("mix:<n>:<direct>:<order...>") combines + direct-drive and TCM-drive outputs. The <n> specifies the number + of scans, <direct> selects the direct-drive outputs, and <order...> + defines the scan order. + $ref: /schemas/types.yaml#/definitions/string + pattern: '^(tcm|mix):[1-4](:[0-3]){1,4}$' + + vcc-supply: + description: Regulator providing power to the 'VCC' pin. + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + +patternProperties: + "^led@[0-3]$": + type: object + $ref: common.yaml# + unevaluatedProperties: false + + properties: + reg: + minimum: 0 + maximum: 3 + + required: + - reg + + "^multi-led@[4-7]$": + type: object + $ref: leds-class-multicolor.yaml# + unevaluatedProperties: false + + properties: + reg: + minimum: 4 + maximum: 7 + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + + patternProperties: + "^led@[4-9a-f]$": + type: object + $ref: common.yaml# + unevaluatedProperties: false + + properties: + 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>; + ti,scan-mode = "tcm:4:0:1:2:3"; + vcc-supply = <&vdd_3v3_reg>; + + led@0 { + reg = <0x0>; + label = "LED0"; + led-max-microamp = <25500>; + }; + + led@1 { + reg = <0x1>; + label = "LED1"; + led-max-microamp = <25500>; + }; + + led@2 { + reg = <0x2>; + label = "LED2"; + led-max-microamp = <25500>; + }; + + led@3 { + reg = <0x3>; + label = "LED3"; + led-max-microamp = <25500>; + }; + + multi-led@4 { + #address-cells = <1>; + #size-cells = <0>; + reg = <0x4>; + color = <LED_COLOR_ID_RGB>; + label = "LED_A"; + + led@4 { + reg = <0x4>; + color = <LED_COLOR_ID_GREEN>; + led-max-microamp = <25500>; + }; + + led@5 { + reg = <0x5>; + color = <LED_COLOR_ID_RED>; + led-max-microamp = <25500>; + }; + + led@6 { + reg = <0x6>; + color = <LED_COLOR_ID_BLUE>; + led-max-microamp = <25500>; + }; + }; + + multi-led@5 { + #address-cells = <1>; + #size-cells = <0>; + reg = <0x5>; + color = <LED_COLOR_ID_RGB>; + label = "LED_B"; + + led@7 { + reg = <0x7>; + color = <LED_COLOR_ID_GREEN>; + led-max-microamp = <25500>; + }; + + led@8 { + reg = <0x8>; + color = <LED_COLOR_ID_RED>; + led-max-microamp = <25500>; + }; + + led@9 { + reg = <0x9>; + color = <LED_COLOR_ID_BLUE>; + led-max-microamp = <25500>; + }; + }; + + multi-led@6 { + #address-cells = <1>; + #size-cells = <0>; + reg = <0x6>; + color = <LED_COLOR_ID_RGB>; + label = "LED_C"; + + led@a { + reg = <0xa>; + color = <LED_COLOR_ID_GREEN>; + led-max-microamp = <25500>; + }; + + led@b { + reg = <0xb>; + color = <LED_COLOR_ID_RED>; + led-max-microamp = <25500>; + }; + + led@c { + reg = <0xc>; + color = <LED_COLOR_ID_BLUE>; + led-max-microamp = <25500>; + }; + }; + + multi-led@7 { + #address-cells = <1>; + #size-cells = <0>; + reg = <0x7>; + color = <LED_COLOR_ID_RGB>; + label = "LED_D"; + + led@d { + reg = <0xd>; + color = <LED_COLOR_ID_GREEN>; + led-max-microamp = <25500>; + }; + + led@e { + reg = <0xe>; + color = <LED_COLOR_ID_RED>; + led-max-microamp = <25500>; + }; + + led@f { + reg = <0xf>; + color = <LED_COLOR_ID_BLUE>; + led-max-microamp = <25500>; + }; + }; + }; + }; + +... diff --git a/MAINTAINERS b/MAINTAINERS index 545a4776795e..6a84f999ef73 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -25424,6 +25424,12 @@ S: Supported F: Documentation/devicetree/bindings/iio/dac/ti,dac7612.yaml F: drivers/iio/dac/ti-dac7612.c +TEXAS INSTRUMENTS' LP5812 RGB LED DRIVER +M: Nam Tran <trannamatk@gmail.com> +L: linux-leds@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/leds/ti,lp5812.yaml + TEXAS INSTRUMENTS' LB8864 LED BACKLIGHT DRIVER M: Alexander Sverdlin <alexander.sverdlin@siemens.com> L: linux-leds@vger.kernel.org -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v17 1/3] dt-bindings: leds: add TI/National Semiconductor LP5812 LED Driver 2025-10-21 15:59 ` [PATCH v17 1/3] dt-bindings: leds: add TI/National Semiconductor LP5812 LED Driver Nam Tran @ 2025-10-26 22:06 ` Rob Herring (Arm) 0 siblings, 0 replies; 8+ messages in thread From: Rob Herring (Arm) @ 2025-10-26 22:06 UTC (permalink / raw) To: Nam Tran Cc: gregkh, christophe.jaillet, linux-doc, conor+dt, krzk+dt, rdunlap, pavel, devicetree, corbet, linux-leds, linux-kernel, lee On Tue, 21 Oct 2025 22:59:25 +0700, Nam Tran wrote: > The LP5812 is a 4x3 RGB LED driver with an autonomous animation > engine and time-cross-multiplexing (TCM) support for up to 12 LEDs > or 4 RGB LEDs. It supports both analog (256 levels) and PWM (8-bit) > dimming, including exponential PWM for smooth brightness control. > > Signed-off-by: Nam Tran <trannamatk@gmail.com> > --- > .../devicetree/bindings/leds/ti,lp5812.yaml | 246 ++++++++++++++++++ > MAINTAINERS | 6 + > 2 files changed, 252 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/ti,lp5812.yaml > Reviewed-by: Rob Herring (Arm) <robh@kernel.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v17 2/3] leds: add basic support for TI/National Semiconductor LP5812 LED Driver 2025-10-21 15:59 [PATCH v17 0/3] leds: add new LED driver for TI LP5812 Nam Tran 2025-10-21 15:59 ` [PATCH v17 1/3] dt-bindings: leds: add TI/National Semiconductor LP5812 LED Driver Nam Tran @ 2025-10-21 15:59 ` Nam Tran 2025-11-06 15:59 ` Lee Jones 2025-10-21 15:59 ` [PATCH v17 3/3] docs: leds: Document TI LP5812 LED driver Nam Tran 2 siblings, 1 reply; 8+ messages in thread From: Nam Tran @ 2025-10-21 15:59 UTC (permalink / raw) To: lee Cc: pavel, gregkh, rdunlap, christophe.jaillet, krzk+dt, robh, conor+dt, corbet, linux-leds, linux-kernel, devicetree, linux-doc, Nam Tran The LP5812 is a 4x3 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. This patch adds minimal driver support for the LP5812, implementing only the essential functionality: I2C communication with the device, LED registration, brightness control in manual mode, and basic sysfs interfaces for LED configuration and fault monitoring. Signed-off-by: Nam Tran <trannamatk@gmail.com> --- MAINTAINERS | 4 + drivers/leds/rgb/Kconfig | 13 + drivers/leds/rgb/Makefile | 1 + drivers/leds/rgb/leds-lp5812.c | 730 +++++++++++++++++++++++++++++++++ drivers/leds/rgb/leds-lp5812.h | 197 +++++++++ 5 files changed, 945 insertions(+) create mode 100644 drivers/leds/rgb/leds-lp5812.c create mode 100644 drivers/leds/rgb/leds-lp5812.h diff --git a/MAINTAINERS b/MAINTAINERS index 6a84f999ef73..ee56b68e54cd 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -25429,6 +25429,10 @@ M: Nam Tran <trannamatk@gmail.com> L: linux-leds@vger.kernel.org S: Maintained F: Documentation/devicetree/bindings/leds/ti,lp5812.yaml +F: drivers/leds/rgb/Kconfig +F: drivers/leds/rgb/Makefile +F: drivers/leds/rgb/leds-lp5812.c +F: drivers/leds/rgb/leds-lp5812.h TEXAS INSTRUMENTS' LB8864 LED BACKLIGHT DRIVER M: Alexander Sverdlin <alexander.sverdlin@siemens.com> diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig index 222d943d826a..28ef4c487367 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 4x3 matrix RGB LED driver with autonomous + animation engine control. + + To compile this driver as a module, choose M here: the + module will be called leds-lp5812. + + If unsure, say N. + config LEDS_NCP5623 tristate "LED support for NCP5623" depends on I2C diff --git a/drivers/leds/rgb/Makefile b/drivers/leds/rgb/Makefile index a501fd27f179..be45991f63f5 100644 --- a/drivers/leds/rgb/Makefile +++ b/drivers/leds/rgb/Makefile @@ -2,6 +2,7 @@ obj-$(CONFIG_LEDS_GROUP_MULTICOLOR) += leds-group-multicolor.o obj-$(CONFIG_LEDS_KTD202X) += leds-ktd202x.o +obj-$(CONFIG_LEDS_LP5812) += leds-lp5812.o obj-$(CONFIG_LEDS_NCP5623) += leds-ncp5623.o obj-$(CONFIG_LEDS_PWM_MULTICOLOR) += leds-pwm-multicolor.o obj-$(CONFIG_LEDS_QCOM_LPG) += leds-qcom-lpg.o diff --git a/drivers/leds/rgb/leds-lp5812.c b/drivers/leds/rgb/leds-lp5812.c new file mode 100644 index 000000000000..d3657ba9d987 --- /dev/null +++ b/drivers/leds/rgb/leds-lp5812.c @@ -0,0 +1,730 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * LP5812 LED driver + * + * Copyright (C) 2025 Texas Instruments + * + * Author: Jared Zhou <jared-zhou@ti.com> + */ + +#include <linux/delay.h> +#include <linux/i2c.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/led-class-multicolor.h> +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/sysfs.h> +#include <linux/types.h> + +#include "leds-lp5812.h" + +static const struct lp5812_mode_mapping chip_mode_map[] = { + {"direct_mode", 0, 0, 0, 0, 0, 0}, + {"tcm:1:0", 1, 0, 0, 0, 0, 0}, + {"tcm:1:1", 1, 1, 0, 0, 0, 0}, + {"tcm:1:2", 1, 2, 0, 0, 0, 0}, + {"tcm:1:3", 1, 3, 0, 0, 0, 0}, + {"tcm:2:0:1", 2, 0, 1, 0, 0, 0}, + {"tcm:2:0:2", 2, 0, 2, 0, 0, 0}, + {"tcm:2:0:3", 2, 0, 3, 0, 0, 0}, + {"tcm:2:1:2", 2, 1, 2, 0, 0, 0}, + {"tcm:2:1:3", 2, 1, 3, 0, 0, 0}, + {"tcm:2:2:3", 2, 2, 3, 0, 0, 0}, + {"tcm:3:0:1:2", 3, 0, 1, 2, 0, 0}, + {"tcm:3:0:1:3", 3, 0, 1, 3, 0, 0}, + {"tcm:3:0:2:3", 3, 0, 2, 3, 0, 0}, + {"tcm:4:0:1:2:3", 4, 0, 1, 2, 3, 0}, + {"mix:1:0:1", 5, 1, 0, 0, 0, 0}, + {"mix:1:0:2", 5, 2, 0, 0, 0, 0}, + {"mix:1:0:3", 5, 3, 0, 0, 0, 0}, + {"mix:1:1:0", 5, 0, 0, 0, 0, 1}, + {"mix:1:1:2", 5, 2, 0, 0, 0, 1}, + {"mix:1:1:3", 5, 3, 0, 0, 0, 1}, + {"mix:1:2:0", 5, 0, 0, 0, 0, 2}, + {"mix:1:2:1", 5, 1, 0, 0, 0, 2}, + {"mix:1:2:3", 5, 3, 0, 0, 0, 2}, + {"mix:1:3:0", 5, 0, 0, 0, 0, 3}, + {"mix:1:3:1", 5, 1, 0, 0, 0, 3}, + {"mix:1:3:2", 5, 2, 0, 0, 0, 3}, + {"mix:2:0:1:2", 6, 1, 2, 0, 0, 0}, + {"mix:2:0:1:3", 6, 1, 3, 0, 0, 0}, + {"mix:2:0:2:3", 6, 2, 3, 0, 0, 0}, + {"mix:2:1:0:2", 6, 0, 2, 0, 0, 1}, + {"mix:2:1:0:3", 6, 0, 3, 0, 0, 1}, + {"mix:2:1:2:3", 6, 2, 3, 0, 0, 1}, + {"mix:2:2:0:1", 6, 0, 1, 0, 0, 2}, + {"mix:2:2:0:3", 6, 0, 3, 0, 0, 2}, + {"mix:2:2:1:3", 6, 1, 3, 0, 0, 2}, + {"mix:2:3:0:1", 6, 0, 1, 0, 0, 3}, + {"mix:2:3:0:2", 6, 0, 2, 0, 0, 3}, + {"mix:2:3:1:2", 6, 1, 2, 0, 0, 3}, + {"mix:3:0:1:2:3", 7, 1, 2, 3, 0, 0}, + {"mix:3:1:0:2:3", 7, 0, 2, 3, 0, 1}, + {"mix:3:2:0:1:3", 7, 0, 1, 3, 0, 2}, + {"mix:3:3:0:1:2", 7, 0, 1, 2, 0, 3} +}; + +static int lp5812_write(struct lp5812_chip *chip, u16 reg, u8 val) +{ + struct device *dev = &chip->client->dev; + struct i2c_msg msg; + u8 buf[LP5812_DATA_LENGTH]; + u8 reg_addr_bit8_9; + int ret; + + /* Extract register address bits 9 and 8 for Address Byte 1 */ + reg_addr_bit8_9 = (reg >> LP5812_REG_ADDR_HIGH_SHIFT) & LP5812_REG_ADDR_BIT_8_9_MASK; + + /* Prepare payload: Address Byte 2 (bits [7:0]) and value to write */ + buf[LP5812_DATA_BYTE_0_IDX] = (u8)(reg & LP5812_REG_ADDR_LOW_MASK); + buf[LP5812_DATA_BYTE_1_IDX] = val; + + /* Construct I2C message for a write operation */ + msg.addr = (chip->client->addr << LP5812_CHIP_ADDR_SHIFT) | reg_addr_bit8_9; + msg.flags = 0; + msg.len = sizeof(buf); + msg.buf = buf; + + ret = i2c_transfer(chip->client->adapter, &msg, 1); + if (ret == 1) + return 0; + + dev_err(dev, "I2C write error, ret=%d\n", ret); + return ret < 0 ? ret : -EIO; +} + +static int lp5812_read(struct lp5812_chip *chip, u16 reg, u8 *val) +{ + struct device *dev = &chip->client->dev; + struct i2c_msg msgs[LP5812_READ_MSG_LENGTH]; + u8 ret_val; + u8 reg_addr_bit8_9; + u8 converted_reg; + int ret; + + /* Extract register address bits 9 and 8 for Address Byte 1 */ + reg_addr_bit8_9 = (reg >> LP5812_REG_ADDR_HIGH_SHIFT) & LP5812_REG_ADDR_BIT_8_9_MASK; + + /* Lower 8 bits go in Address Byte 2 */ + converted_reg = (u8)(reg & LP5812_REG_ADDR_LOW_MASK); + + /* Prepare I2C write message to set register address */ + msgs[LP5812_MSG_0_IDX].addr = + (chip->client->addr << LP5812_CHIP_ADDR_SHIFT) | reg_addr_bit8_9; + msgs[LP5812_MSG_0_IDX].flags = 0; + msgs[LP5812_MSG_0_IDX].len = 1; + msgs[LP5812_MSG_0_IDX].buf = &converted_reg; + + /* Prepare I2C read message to retrieve register value */ + msgs[LP5812_MSG_1_IDX].addr = + (chip->client->addr << LP5812_CHIP_ADDR_SHIFT) | reg_addr_bit8_9; + msgs[LP5812_MSG_1_IDX].flags = I2C_M_RD; + msgs[LP5812_MSG_1_IDX].len = 1; + msgs[LP5812_MSG_1_IDX].buf = &ret_val; + + ret = i2c_transfer(chip->client->adapter, msgs, LP5812_READ_MSG_LENGTH); + if (ret == LP5812_READ_MSG_LENGTH) { + *val = ret_val; + return 0; + } + + dev_err(dev, "I2C read error, ret=%d\n", ret); + *val = 0; + return ret < 0 ? ret : -EIO; +} + +static int lp5812_read_tsd_config_status(struct lp5812_chip *chip, u8 *reg_val) +{ + return lp5812_read(chip, chip->cfg->reg_tsd_config_status.addr, reg_val); +} + +static int lp5812_update_regs_config(struct lp5812_chip *chip) +{ + u8 reg_val; + int ret; + + ret = lp5812_write(chip, chip->cfg->reg_cmd_update.addr, LP5812_UPDATE_CMD_VAL); + if (ret) + return ret; + + ret = lp5812_read_tsd_config_status(chip, ®_val); + if (ret) + return ret; + + return reg_val & LP5812_CFG_ERR_STATUS_MASK; +} + +static ssize_t parse_drive_mode(struct lp5812_chip *chip, const char *str) +{ + int i; + + chip->u_drive_mode.s_drive_mode.mix_sel_led_0 = false; + chip->u_drive_mode.s_drive_mode.mix_sel_led_1 = false; + chip->u_drive_mode.s_drive_mode.mix_sel_led_2 = false; + chip->u_drive_mode.s_drive_mode.mix_sel_led_3 = false; + + if (sysfs_streq(str, LP5812_MODE_DIRECT_NAME)) { + chip->u_drive_mode.s_drive_mode.led_mode = LP5812_MODE_DIRECT_VALUE; + return 0; + } + + for (i = 0; i < ARRAY_SIZE(chip_mode_map); i++) { + if (sysfs_streq(str, chip_mode_map[i].mode_name)) { + chip->u_drive_mode.s_drive_mode.led_mode = chip_mode_map[i].mode; + chip->u_scan_order.s_scan_order.scan_order_0 = + chip_mode_map[i].scan_order_0; + chip->u_scan_order.s_scan_order.scan_order_1 = + chip_mode_map[i].scan_order_1; + chip->u_scan_order.s_scan_order.scan_order_2 = + chip_mode_map[i].scan_order_2; + chip->u_scan_order.s_scan_order.scan_order_3 = + chip_mode_map[i].scan_order_3; + + switch (chip_mode_map[i].selection_led) { + case LP5812_MODE_MIX_SELECT_LED_0: + chip->u_drive_mode.s_drive_mode.mix_sel_led_0 = true; + break; + case LP5812_MODE_MIX_SELECT_LED_1: + chip->u_drive_mode.s_drive_mode.mix_sel_led_1 = true; + break; + case LP5812_MODE_MIX_SELECT_LED_2: + chip->u_drive_mode.s_drive_mode.mix_sel_led_2 = true; + break; + case LP5812_MODE_MIX_SELECT_LED_3: + chip->u_drive_mode.s_drive_mode.mix_sel_led_3 = true; + break; + default: + return -EINVAL; + } + + return 0; + } + } + + return -EINVAL; +} + +static int lp5812_set_drive_mode_scan_order(struct lp5812_chip *chip) +{ + u8 val; + int ret; + + /* Set led mode */ + val = chip->u_drive_mode.drive_mode_val; + ret = lp5812_write(chip, chip->cfg->reg_dev_config_1.addr, val); + if (ret) + return ret; + + /* Setup scan order */ + val = chip->u_scan_order.scan_order_val; + ret = lp5812_write(chip, chip->cfg->reg_dev_config_2.addr, val); + + return ret; +} + +static int lp5812_set_led_mode(struct lp5812_chip *chip, int led_number, + enum control_mode mode) +{ + u8 reg_val; + u16 reg; + int ret; + + if (led_number < LP5812_NUMBER_LED_IN_REG) + reg = chip->cfg->reg_dev_config_3.addr; + else + reg = chip->cfg->reg_dev_config_4.addr; + + ret = lp5812_read(chip, reg, ®_val); + if (ret) + return ret; + + if (mode == LP5812_MODE_MANUAL) + reg_val &= ~(1 << (led_number % LP5812_NUMBER_LED_IN_REG)); + else + reg_val |= (1 << (led_number % LP5812_NUMBER_LED_IN_REG)); + + ret = lp5812_write(chip, reg, reg_val); + if (ret) + return ret; + + ret = lp5812_update_regs_config(chip); + + return ret; +} + +static int lp5812_manual_dc_pwm_control(struct lp5812_chip *chip, int led_number, + u8 val, enum dimming_type dimming_type) +{ + u16 led_base_reg; + int ret; + + if (dimming_type == LP5812_DIMMING_ANALOG) + led_base_reg = chip->cfg->reg_manual_dc_base.addr; + else + led_base_reg = chip->cfg->reg_manual_pwm_base.addr; + ret = lp5812_write(chip, led_base_reg + led_number, val); + + return ret; +} + +static int lp5812_auto_dc(struct lp5812_chip *chip, + int led_number, u8 val) +{ + return lp5812_write(chip, chip->cfg->reg_auto_dc_base.addr + led_number, val); +} + +static int lp5812_multicolor_brightness(struct lp5812_led *led) +{ + int ret, i; + struct lp5812_chip *chip = led->chip; + + guard(mutex)(&chip->lock); + for (i = 0; i < led->mc_cdev.num_colors; i++) { + ret = lp5812_manual_dc_pwm_control(chip, led->mc_cdev.subled_info[i].channel, + led->mc_cdev.subled_info[i].brightness, + LP5812_DIMMING_PWM); + if (ret) + return ret; + } + + return 0; +} + +static int lp5812_led_brightness(struct lp5812_led *led) +{ + struct lp5812_chip *chip = led->chip; + struct lp5812_led_config *led_cfg; + int ret; + + led_cfg = &chip->led_config[led->chan_nr]; + + guard(mutex)(&chip->lock); + ret = lp5812_manual_dc_pwm_control(chip, led_cfg->led_id[0], + led->brightness, LP5812_DIMMING_PWM); + + return ret; +} + +static int lp5812_set_brightness(struct led_classdev *cdev, + enum led_brightness brightness) +{ + struct lp5812_led *led = container_of(cdev, struct lp5812_led, cdev); + + led->brightness = (u8)brightness; + return lp5812_led_brightness(led); +} + +static int lp5812_set_mc_brightness(struct led_classdev *cdev, + enum led_brightness brightness) +{ + struct led_classdev_mc *mc_dev = lcdev_to_mccdev(cdev); + struct lp5812_led *led = container_of(mc_dev, struct lp5812_led, mc_cdev); + + led_mc_calc_color_components(&led->mc_cdev, brightness); + return lp5812_multicolor_brightness(led); +} + +static int lp5812_init_led(struct lp5812_led *led, struct lp5812_chip *chip, int chan) +{ + struct device *dev = &chip->client->dev; + struct mc_subled *mc_led_info; + struct led_classdev *led_cdev; + int i, ret; + + if (chip->led_config[chan].name) { + led->cdev.name = chip->led_config[chan].name; + } else { + led->cdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s:channel%d", + chip->label ? : chip->client->name, chan); + if (!led->cdev.name) + return -ENOMEM; + } + + if (!chip->led_config[chan].is_sc_led) { + mc_led_info = devm_kcalloc(dev, + chip->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; + led_cdev->brightness_set_blocking = lp5812_set_mc_brightness; + led->mc_cdev.num_colors = chip->led_config[chan].num_colors; + for (i = 0; i < led->mc_cdev.num_colors; i++) { + mc_led_info[i].color_index = + chip->led_config[chan].color_id[i]; + mc_led_info[i].channel = + chip->led_config[chan].led_id[i]; + } + + led->mc_cdev.subled_info = mc_led_info; + } else { + led->cdev.brightness_set_blocking = lp5812_set_brightness; + } + + led->chan_nr = chan; + + if (chip->led_config[chan].is_sc_led) { + ret = devm_led_classdev_register(dev, &led->cdev); + if (ret == 0) + led->cdev.dev->platform_data = led; + } else { + ret = devm_led_classdev_multicolor_register(dev, &led->mc_cdev); + if (ret == 0) + led->mc_cdev.led_cdev.dev->platform_data = led; + } + + return ret; +} + +static int lp5812_register_leds(struct lp5812_led *led, struct lp5812_chip *chip) +{ + struct lp5812_led *each; + int num_channels = chip->num_channels; + u8 reg_val; + u16 reg; + int ret, i, j; + + for (i = 0; i < num_channels; i++) { + each = led + i; + ret = lp5812_init_led(each, chip, i); + if (ret) + goto err_init_led; + + each->chip = chip; + + for (j = 0; j < chip->led_config[i].num_colors; j++) { + ret = lp5812_auto_dc(chip, chip->led_config[i].led_id[j], + chip->led_config[i].max_current[j]); + if (ret) + goto err_init_led; + + ret = lp5812_manual_dc_pwm_control(chip, chip->led_config[i].led_id[j], + chip->led_config[i].max_current[j], + LP5812_DIMMING_ANALOG); + if (ret) + goto err_init_led; + + ret = lp5812_set_led_mode(chip, chip->led_config[i].led_id[j], + LP5812_MODE_MANUAL); + if (ret) + goto err_init_led; + + reg = (chip->led_config[i].led_id[j] < LP5812_NUMBER_LED_IN_REG) ? + chip->cfg->reg_led_en_1.addr : + chip->cfg->reg_led_en_2.addr; + + ret = lp5812_read(chip, reg, ®_val); + if (ret) + goto err_init_led; + + reg_val |= (1 << (chip->led_config[i].led_id[j] % + LP5812_NUMBER_LED_IN_REG)); + + ret = lp5812_write(chip, reg, reg_val); + if (ret) + goto err_init_led; + } + } + + return 0; + +err_init_led: + return ret; +} + +static int lp5812_init_device(struct lp5812_chip *chip) +{ + int ret; + + usleep_range(LP5812_WAIT_DEVICE_STABLE_MIN, LP5812_WAIT_DEVICE_STABLE_MAX); + + ret = lp5812_write(chip, chip->cfg->reg_chip_en.addr, (u8)1); + if (ret) { + dev_err(&chip->client->dev, "lp5812_enable_disable failed\n"); + return ret; + } + + ret = lp5812_write(chip, chip->cfg->reg_dev_config_12.addr, LP5812_LSD_LOD_START_UP); + if (ret) { + dev_err(&chip->client->dev, "write 0x0B to DEV_CONFIG12 failed\n"); + return ret; + } + + ret = parse_drive_mode(chip, chip->scan_mode); + if (ret) + return ret; + + ret = lp5812_set_drive_mode_scan_order(chip); + if (ret) + return ret; + + ret = lp5812_update_regs_config(chip); + if (ret) { + dev_err(&chip->client->dev, "lp5812_update_regs_config failed\n"); + return ret; + } + + return 0; +} + +static void lp5812_deinit_device(struct lp5812_chip *chip) +{ + lp5812_write(chip, chip->cfg->reg_led_en_1.addr, 0); + lp5812_write(chip, chip->cfg->reg_led_en_2.addr, 0); + lp5812_write(chip, chip->cfg->reg_chip_en.addr, 0); +} + +static int lp5812_parse_led_channel(struct device_node *np, + struct lp5812_led_config *cfg, + int color_number) +{ + int color_id = 0, reg, ret; + u32 max_cur = 0; + + ret = of_property_read_u32(np, "reg", ®); + if (ret) + return ret; + + cfg->led_id[color_number] = reg; + + of_property_read_u32(np, "led-max-microamp", &max_cur); + cfg->max_current[color_number] = max_cur / 100; + + of_property_read_u32(np, "color", &color_id); + cfg->color_id[color_number] = color_id; + + return 0; +} + +static int lp5812_parse_led(struct device_node *np, + struct lp5812_led_config *cfg, + int led_index) +{ + int num_colors = 0, ret; + + of_property_read_string(np, "label", &cfg[led_index].name); + + ret = of_property_read_u32(np, "reg", &cfg[led_index].chan_nr); + if (ret) + return ret; + + for_each_available_child_of_node_scoped(np, child) { + ret = lp5812_parse_led_channel(child, &cfg[led_index], num_colors); + if (ret) + return ret; + num_colors++; + } + + if (num_colors == 0) { + ret = lp5812_parse_led_channel(np, &cfg[led_index], 0); + if (ret) + return ret; + num_colors = 1; + cfg[led_index].is_sc_led = true; + } else { + cfg[led_index].is_sc_led = false; + } + + cfg[led_index].num_colors = num_colors; + + return 0; +} + +static int lp5812_of_populate_pdata(struct device *dev, + struct device_node *np, + struct lp5812_chip *chip) +{ + struct lp5812_led_config *cfg; + int num_channels, i = 0, ret; + + num_channels = of_get_available_child_count(np); + if (num_channels == 0) { + dev_err(dev, "no LED channels\n"); + return -EINVAL; + } + + cfg = devm_kcalloc(dev, num_channels, sizeof(*cfg), GFP_KERNEL); + if (!cfg) + return -ENOMEM; + + chip->led_config = &cfg[0]; + chip->num_channels = num_channels; + + for_each_available_child_of_node_scoped(np, child) { + ret = lp5812_parse_led(child, cfg, i); + if (ret) + return -EINVAL; + i++; + } + + ret = of_property_read_string(np, "ti,scan-mode", &chip->scan_mode); + if (ret) + chip->scan_mode = LP5812_MODE_DIRECT_NAME; + + of_property_read_string(np, "label", &chip->label); + + return 0; +} + +static int lp5812_probe(struct i2c_client *client) +{ + struct lp5812_chip *chip; + struct device_node *np = dev_of_node(&client->dev); + struct lp5812_led *led; + int ret; + + if (!np) + return -EINVAL; + + chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL); + if (!chip) + return -ENOMEM; + + chip->cfg = i2c_get_match_data(client); + ret = lp5812_of_populate_pdata(&client->dev, np, chip); + if (ret) + return ret; + + led = devm_kcalloc(&client->dev, chip->num_channels, sizeof(*led), GFP_KERNEL); + if (!led) + return -ENOMEM; + + chip->client = client; + mutex_init(&chip->lock); + i2c_set_clientdata(client, led); + + ret = lp5812_init_device(chip); + if (ret) + return ret; + + ret = lp5812_register_leds(led, chip); + if (ret) + goto err_out; + + return 0; + +err_out: + lp5812_deinit_device(chip); + return ret; +} + +static void lp5812_remove(struct i2c_client *client) +{ + struct lp5812_led *led = i2c_get_clientdata(client); + + lp5812_deinit_device(led->chip); +} + +/* Chip specific configurations */ +static const struct lp5812_device_config lp5812_cfg = { + .reg_reset = { + .addr = LP5812_REG_RESET, + .val = LP5812_RESET + }, + .reg_chip_en = { + .addr = LP5812_REG_ENABLE, + .val = LP5812_ENABLE_DEFAULT + }, + .reg_dev_config_0 = { + .addr = LP5812_DEV_CONFIG0, + .val = 0 + }, + .reg_dev_config_1 = { + .addr = LP5812_DEV_CONFIG1, + .val = 0 + }, + .reg_dev_config_2 = { + .addr = LP5812_DEV_CONFIG2, + .val = 0 + }, + .reg_dev_config_3 = { + .addr = LP5812_DEV_CONFIG3, + .val = 0 + }, + .reg_dev_config_4 = { + .addr = LP5812_DEV_CONFIG4, + .val = 0 + }, + .reg_dev_config_5 = { + .addr = LP5812_DEV_CONFIG5, + .val = 0 + }, + .reg_dev_config_6 = { + .addr = LP5812_DEV_CONFIG6, + .val = 0 + }, + .reg_dev_config_7 = { + .addr = LP5812_DEV_CONFIG7, + .val = 0 + }, + .reg_dev_config_12 = { + .addr = LP5812_DEV_CONFIG12, + .val = LP5812_DEV_CONFIG12_DEFAULT + }, + .reg_cmd_update = { + .addr = LP5812_CMD_UPDATE, + .val = 0 + }, + .reg_tsd_config_status = { + .addr = LP5812_TSD_CONFIG_STATUS, + .val = 0 + }, + .reg_led_en_1 = { + .addr = LP5812_LED_EN_1, + .val = 0 + }, + .reg_led_en_2 = { + .addr = LP5812_LED_EN_2, + .val = 0 + }, + .reg_fault_clear = { + .addr = LP5812_FAULT_CLEAR, + .val = 0 + }, + .reg_manual_dc_base = { + .addr = LP5812_MANUAL_DC_BASE, + .val = 0 + }, + .reg_auto_dc_base = { + .addr = LP5812_AUTO_DC_BASE, + .val = 0 + }, + .reg_manual_pwm_base = { + .addr = LP5812_MANUAL_PWM_BASE, + .val = 0 + }, + .reg_lod_status_base = { + .addr = LP5812_LOD_STATUS, + .val = 0 + }, + .reg_lsd_status_base = { + .addr = LP5812_LSD_STATUS, + .val = 0 + } +}; + +static const struct of_device_id of_lp5812_match[] = { + { .compatible = "ti,lp5812", .data = &lp5812_cfg }, + {/* NULL */} +}; + +MODULE_DEVICE_TABLE(of, of_lp5812_match); + +static struct i2c_driver lp5812_driver = { + .driver = { + .name = "lp5812", + .of_match_table = of_lp5812_match, + }, + .probe = lp5812_probe, + .remove = lp5812_remove, +}; + +module_i2c_driver(lp5812_driver); + +MODULE_DESCRIPTION("Texas Instruments LP5812 LED Driver"); +MODULE_AUTHOR("Jared Zhou"); +MODULE_LICENSE("GPL"); diff --git a/drivers/leds/rgb/leds-lp5812.h b/drivers/leds/rgb/leds-lp5812.h new file mode 100644 index 000000000000..bb0a9549c151 --- /dev/null +++ b/drivers/leds/rgb/leds-lp5812.h @@ -0,0 +1,197 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * LP5812 Driver Header + * + * Copyright (C) 2025 Texas Instruments + * + * Author: Jared Zhou <jared-zhou@ti.com> + */ + +#ifndef _LP5812_H_ +#define _LP5812_H_ + +#include <linux/delay.h> +#include <linux/i2c.h> +#include <linux/kernel.h> +#include <linux/led-class-multicolor.h> +#include <linux/leds.h> +#include <linux/mutex.h> +#include <linux/sysfs.h> +#include <linux/types.h> + +#define LP5812_REG_ENABLE 0x0000 +#define LP5812_REG_RESET 0x0023 +#define LP5812_DEV_CONFIG0 0x0001 +#define LP5812_DEV_CONFIG1 0x0002 +#define LP5812_DEV_CONFIG2 0x0003 +#define LP5812_DEV_CONFIG3 0x0004 +#define LP5812_DEV_CONFIG4 0x0005 +#define LP5812_DEV_CONFIG5 0x0006 +#define LP5812_DEV_CONFIG6 0x0007 +#define LP5812_DEV_CONFIG7 0x0008 +#define LP5812_DEV_CONFIG8 0x0009 +#define LP5812_DEV_CONFIG9 0x000A +#define LP5812_DEV_CONFIG10 0x000B +#define LP5812_DEV_CONFIG11 0x000c +#define LP5812_DEV_CONFIG12 0x000D +#define LP5812_CMD_UPDATE 0x0010 +#define LP5812_LED_EN_1 0x0020 +#define LP5812_LED_EN_2 0x0021 +#define LP5812_FAULT_CLEAR 0x0022 +#define LP5812_MANUAL_DC_BASE 0x0030 +#define LP5812_AUTO_DC_BASE 0x0050 +#define LP5812_MANUAL_PWM_BASE 0x0040 + +#define LP5812_TSD_CONFIG_STATUS 0x0300 +#define LP5812_LOD_STATUS 0x0301 +#define LP5812_LSD_STATUS 0x0303 + +#define LP5812_ENABLE_DEFAULT 0x01 +#define FAULT_CLEAR_ALL 0x07 +#define TSD_CLEAR_VAL 0x04 +#define LSD_CLEAR_VAL 0x02 +#define LOD_CLEAR_VAL 0x01 +#define LP5812_RESET 0x66 +#define LP5812_DEV_CONFIG12_DEFAULT 0x08 + +#define LP5812_UPDATE_CMD_VAL 0x55 +#define LP5812_REG_ADDR_HIGH_SHIFT 8 +#define LP5812_REG_ADDR_BIT_8_9_MASK 0x03 +#define LP5812_REG_ADDR_LOW_MASK 0xFF +#define LP5812_CHIP_ADDR_SHIFT 2 +#define LP5812_DATA_LENGTH 2 +#define LP5812_DATA_BYTE_0_IDX 0 +#define LP5812_DATA_BYTE_1_IDX 1 + +#define LP5812_READ_MSG_LENGTH 2 +#define LP5812_MSG_0_IDX 0 +#define LP5812_MSG_1_IDX 1 +#define LP5812_CFG_ERR_STATUS_MASK 0x01 +#define LP5812_CFG_TSD_STATUS_SHIFT 1 +#define LP5812_CFG_TSD_STATUS_MASK 0x01 + +#define LP5812_FAULT_CLEAR_LOD 0 +#define LP5812_FAULT_CLEAR_LSD 1 +#define LP5812_FAULT_CLEAR_TSD 2 +#define LP5812_FAULT_CLEAR_ALL 3 +#define LP5812_NUMBER_LED_IN_REG 8 + +#define LP5812_WAIT_DEVICE_STABLE_MIN 1000 +#define LP5812_WAIT_DEVICE_STABLE_MAX 1100 + +#define LP5812_LSD_LOD_START_UP 0x0B +#define LP5812_MODE_NAME_MAX_LEN 20 +#define LP5812_MODE_DIRECT_NAME "direct_mode" +#define LP5812_MODE_DIRECT_VALUE 0 +#define LP5812_MODE_MIX_SELECT_LED_0 0 +#define LP5812_MODE_MIX_SELECT_LED_1 1 +#define LP5812_MODE_MIX_SELECT_LED_2 2 +#define LP5812_MODE_MIX_SELECT_LED_3 3 + +enum control_mode { + LP5812_MODE_MANUAL = 0, + LP5812_MODE_AUTONOMOUS +}; + +enum dimming_type { + LP5812_DIMMING_ANALOG, + LP5812_DIMMING_PWM +}; + +union u_scan_order { + struct { + u8 scan_order_0:2; + u8 scan_order_1:2; + u8 scan_order_2:2; + u8 scan_order_3:2; + } s_scan_order; + u8 scan_order_val; +}; + +union u_drive_mode { + struct { + u8 mix_sel_led_0:1; + u8 mix_sel_led_1:1; + u8 mix_sel_led_2:1; + u8 mix_sel_led_3:1; + u8 led_mode:3; + u8 pwm_fre:1; + } s_drive_mode; + u8 drive_mode_val; +}; + +struct lp5812_reg { + u16 addr; + union { + u8 val; + u8 mask; + u8 shift; + }; +}; + +struct lp5812_mode_mapping { + char mode_name[LP5812_MODE_NAME_MAX_LEN]; + u8 mode; + u8 scan_order_0; + u8 scan_order_1; + u8 scan_order_2; + u8 scan_order_3; + u8 selection_led; +}; + +struct lp5812_led_config { + bool is_sc_led; + const char *name; + u8 color_id[LED_COLOR_ID_MAX]; + u32 max_current[LED_COLOR_ID_MAX]; + int chan_nr; + int num_colors; + int led_id[LED_COLOR_ID_MAX]; +}; + +struct lp5812_chip { + u8 num_channels; + struct i2c_client *client; + struct mutex lock; /* Protects register access */ + struct lp5812_led_config *led_config; + const char *label; + const char *scan_mode; + const struct lp5812_device_config *cfg; + union u_scan_order u_scan_order; + union u_drive_mode u_drive_mode; +}; + +struct lp5812_led { + u8 brightness; + int chan_nr; + struct led_classdev cdev; + struct led_classdev_mc mc_cdev; + struct lp5812_chip *chip; +}; + +struct lp5812_device_config { + const struct lp5812_reg reg_reset; + const struct lp5812_reg reg_chip_en; + const struct lp5812_reg reg_dev_config_0; + const struct lp5812_reg reg_dev_config_1; + const struct lp5812_reg reg_dev_config_2; + const struct lp5812_reg reg_dev_config_3; + const struct lp5812_reg reg_dev_config_4; + const struct lp5812_reg reg_dev_config_5; + const struct lp5812_reg reg_dev_config_6; + const struct lp5812_reg reg_dev_config_7; + const struct lp5812_reg reg_dev_config_12; + const struct lp5812_reg reg_cmd_update; + + const struct lp5812_reg reg_led_en_1; + const struct lp5812_reg reg_led_en_2; + const struct lp5812_reg reg_fault_clear; + const struct lp5812_reg reg_manual_dc_base; + const struct lp5812_reg reg_auto_dc_base; + const struct lp5812_reg reg_manual_pwm_base; + const struct lp5812_reg reg_tsd_config_status; + const struct lp5812_reg reg_lod_status_base; + const struct lp5812_reg reg_lsd_status_base; +}; + +#endif /*_LP5812_H_*/ -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v17 2/3] leds: add basic support for TI/National Semiconductor LP5812 LED Driver 2025-10-21 15:59 ` [PATCH v17 2/3] leds: add basic support for " Nam Tran @ 2025-11-06 15:59 ` Lee Jones 2025-11-11 17:07 ` Nam Tran 0 siblings, 1 reply; 8+ messages in thread From: Lee Jones @ 2025-11-06 15:59 UTC (permalink / raw) To: Nam Tran Cc: pavel, gregkh, rdunlap, christophe.jaillet, krzk+dt, robh, conor+dt, corbet, linux-leds, linux-kernel, devicetree, linux-doc On Tue, 21 Oct 2025, Nam Tran wrote: > The LP5812 is a 4x3 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. > > This patch adds minimal driver support for the LP5812, implementing > only the essential functionality: I2C communication with the device, > LED registration, brightness control in manual mode, and basic sysfs > interfaces for LED configuration and fault monitoring. > > Signed-off-by: Nam Tran <trannamatk@gmail.com> > --- > MAINTAINERS | 4 + > drivers/leds/rgb/Kconfig | 13 + > drivers/leds/rgb/Makefile | 1 + > drivers/leds/rgb/leds-lp5812.c | 730 +++++++++++++++++++++++++++++++++ > drivers/leds/rgb/leds-lp5812.h | 197 +++++++++ > 5 files changed, 945 insertions(+) > create mode 100644 drivers/leds/rgb/leds-lp5812.c > create mode 100644 drivers/leds/rgb/leds-lp5812.h Last go - just a few nits to fix-up. > diff --git a/MAINTAINERS b/MAINTAINERS > index 6a84f999ef73..ee56b68e54cd 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -25429,6 +25429,10 @@ M: Nam Tran <trannamatk@gmail.com> > L: linux-leds@vger.kernel.org > S: Maintained > F: Documentation/devicetree/bindings/leds/ti,lp5812.yaml > +F: drivers/leds/rgb/Kconfig > +F: drivers/leds/rgb/Makefile > +F: drivers/leds/rgb/leds-lp5812.c > +F: drivers/leds/rgb/leds-lp5812.h > > TEXAS INSTRUMENTS' LB8864 LED BACKLIGHT DRIVER > M: Alexander Sverdlin <alexander.sverdlin@siemens.com> > diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig > index 222d943d826a..28ef4c487367 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 4x3 matrix RGB LED driver with autonomous > + animation engine control. > + > + To compile this driver as a module, choose M here: the > + module will be called leds-lp5812. > + > + If unsure, say N. > + > config LEDS_NCP5623 > tristate "LED support for NCP5623" > depends on I2C > diff --git a/drivers/leds/rgb/Makefile b/drivers/leds/rgb/Makefile > index a501fd27f179..be45991f63f5 100644 > --- a/drivers/leds/rgb/Makefile > +++ b/drivers/leds/rgb/Makefile > @@ -2,6 +2,7 @@ > > obj-$(CONFIG_LEDS_GROUP_MULTICOLOR) += leds-group-multicolor.o > obj-$(CONFIG_LEDS_KTD202X) += leds-ktd202x.o > +obj-$(CONFIG_LEDS_LP5812) += leds-lp5812.o > obj-$(CONFIG_LEDS_NCP5623) += leds-ncp5623.o > obj-$(CONFIG_LEDS_PWM_MULTICOLOR) += leds-pwm-multicolor.o > obj-$(CONFIG_LEDS_QCOM_LPG) += leds-qcom-lpg.o > diff --git a/drivers/leds/rgb/leds-lp5812.c b/drivers/leds/rgb/leds-lp5812.c > new file mode 100644 > index 000000000000..d3657ba9d987 > --- /dev/null > +++ b/drivers/leds/rgb/leds-lp5812.c > @@ -0,0 +1,730 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * LP5812 LED driver > + * > + * Copyright (C) 2025 Texas Instruments > + * > + * Author: Jared Zhou <jared-zhou@ti.com> > + */ > + > +#include <linux/delay.h> > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/led-class-multicolor.h> > +#include <linux/leds.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/sysfs.h> > +#include <linux/types.h> > + > +#include "leds-lp5812.h" > + > +static const struct lp5812_mode_mapping chip_mode_map[] = { > + {"direct_mode", 0, 0, 0, 0, 0, 0}, > + {"tcm:1:0", 1, 0, 0, 0, 0, 0}, > + {"tcm:1:1", 1, 1, 0, 0, 0, 0}, > + {"tcm:1:2", 1, 2, 0, 0, 0, 0}, > + {"tcm:1:3", 1, 3, 0, 0, 0, 0}, > + {"tcm:2:0:1", 2, 0, 1, 0, 0, 0}, > + {"tcm:2:0:2", 2, 0, 2, 0, 0, 0}, > + {"tcm:2:0:3", 2, 0, 3, 0, 0, 0}, > + {"tcm:2:1:2", 2, 1, 2, 0, 0, 0}, > + {"tcm:2:1:3", 2, 1, 3, 0, 0, 0}, > + {"tcm:2:2:3", 2, 2, 3, 0, 0, 0}, > + {"tcm:3:0:1:2", 3, 0, 1, 2, 0, 0}, > + {"tcm:3:0:1:3", 3, 0, 1, 3, 0, 0}, > + {"tcm:3:0:2:3", 3, 0, 2, 3, 0, 0}, > + {"tcm:4:0:1:2:3", 4, 0, 1, 2, 3, 0}, > + {"mix:1:0:1", 5, 1, 0, 0, 0, 0}, > + {"mix:1:0:2", 5, 2, 0, 0, 0, 0}, > + {"mix:1:0:3", 5, 3, 0, 0, 0, 0}, > + {"mix:1:1:0", 5, 0, 0, 0, 0, 1}, > + {"mix:1:1:2", 5, 2, 0, 0, 0, 1}, > + {"mix:1:1:3", 5, 3, 0, 0, 0, 1}, > + {"mix:1:2:0", 5, 0, 0, 0, 0, 2}, > + {"mix:1:2:1", 5, 1, 0, 0, 0, 2}, > + {"mix:1:2:3", 5, 3, 0, 0, 0, 2}, > + {"mix:1:3:0", 5, 0, 0, 0, 0, 3}, > + {"mix:1:3:1", 5, 1, 0, 0, 0, 3}, > + {"mix:1:3:2", 5, 2, 0, 0, 0, 3}, > + {"mix:2:0:1:2", 6, 1, 2, 0, 0, 0}, > + {"mix:2:0:1:3", 6, 1, 3, 0, 0, 0}, > + {"mix:2:0:2:3", 6, 2, 3, 0, 0, 0}, > + {"mix:2:1:0:2", 6, 0, 2, 0, 0, 1}, > + {"mix:2:1:0:3", 6, 0, 3, 0, 0, 1}, > + {"mix:2:1:2:3", 6, 2, 3, 0, 0, 1}, > + {"mix:2:2:0:1", 6, 0, 1, 0, 0, 2}, > + {"mix:2:2:0:3", 6, 0, 3, 0, 0, 2}, > + {"mix:2:2:1:3", 6, 1, 3, 0, 0, 2}, > + {"mix:2:3:0:1", 6, 0, 1, 0, 0, 3}, > + {"mix:2:3:0:2", 6, 0, 2, 0, 0, 3}, > + {"mix:2:3:1:2", 6, 1, 2, 0, 0, 3}, > + {"mix:3:0:1:2:3", 7, 1, 2, 3, 0, 0}, > + {"mix:3:1:0:2:3", 7, 0, 2, 3, 0, 1}, > + {"mix:3:2:0:1:3", 7, 0, 1, 3, 0, 2}, > + {"mix:3:3:0:1:2", 7, 0, 1, 2, 0, 3} > +}; > + > +static int lp5812_write(struct lp5812_chip *chip, u16 reg, u8 val) > +{ > + struct device *dev = &chip->client->dev; > + struct i2c_msg msg; > + u8 buf[LP5812_DATA_LENGTH]; > + u8 reg_addr_bit8_9; > + int ret; > + > + /* Extract register address bits 9 and 8 for Address Byte 1 */ > + reg_addr_bit8_9 = (reg >> LP5812_REG_ADDR_HIGH_SHIFT) & LP5812_REG_ADDR_BIT_8_9_MASK; > + > + /* Prepare payload: Address Byte 2 (bits [7:0]) and value to write */ > + buf[LP5812_DATA_BYTE_0_IDX] = (u8)(reg & LP5812_REG_ADDR_LOW_MASK); > + buf[LP5812_DATA_BYTE_1_IDX] = val; > + > + /* Construct I2C message for a write operation */ > + msg.addr = (chip->client->addr << LP5812_CHIP_ADDR_SHIFT) | reg_addr_bit8_9; > + msg.flags = 0; > + msg.len = sizeof(buf); > + msg.buf = buf; > + > + ret = i2c_transfer(chip->client->adapter, &msg, 1); > + if (ret == 1) > + return 0; > + > + dev_err(dev, "I2C write error, ret=%d\n", ret); > + return ret < 0 ? ret : -EIO; > +} > + > +static int lp5812_read(struct lp5812_chip *chip, u16 reg, u8 *val) > +{ > + struct device *dev = &chip->client->dev; > + struct i2c_msg msgs[LP5812_READ_MSG_LENGTH]; > + u8 ret_val; > + u8 reg_addr_bit8_9; > + u8 converted_reg; > + int ret; > + > + /* Extract register address bits 9 and 8 for Address Byte 1 */ > + reg_addr_bit8_9 = (reg >> LP5812_REG_ADDR_HIGH_SHIFT) & LP5812_REG_ADDR_BIT_8_9_MASK; > + > + /* Lower 8 bits go in Address Byte 2 */ > + converted_reg = (u8)(reg & LP5812_REG_ADDR_LOW_MASK); > + > + /* Prepare I2C write message to set register address */ > + msgs[LP5812_MSG_0_IDX].addr = > + (chip->client->addr << LP5812_CHIP_ADDR_SHIFT) | reg_addr_bit8_9; > + msgs[LP5812_MSG_0_IDX].flags = 0; > + msgs[LP5812_MSG_0_IDX].len = 1; > + msgs[LP5812_MSG_0_IDX].buf = &converted_reg; > + > + /* Prepare I2C read message to retrieve register value */ > + msgs[LP5812_MSG_1_IDX].addr = > + (chip->client->addr << LP5812_CHIP_ADDR_SHIFT) | reg_addr_bit8_9; > + msgs[LP5812_MSG_1_IDX].flags = I2C_M_RD; > + msgs[LP5812_MSG_1_IDX].len = 1; > + msgs[LP5812_MSG_1_IDX].buf = &ret_val; > + > + ret = i2c_transfer(chip->client->adapter, msgs, LP5812_READ_MSG_LENGTH); > + if (ret == LP5812_READ_MSG_LENGTH) { > + *val = ret_val; > + return 0; > + } > + > + dev_err(dev, "I2C read error, ret=%d\n", ret); > + *val = 0; > + return ret < 0 ? ret : -EIO; > +} > + > +static int lp5812_read_tsd_config_status(struct lp5812_chip *chip, u8 *reg_val) > +{ > + return lp5812_read(chip, chip->cfg->reg_tsd_config_status.addr, reg_val); > +} > + > +static int lp5812_update_regs_config(struct lp5812_chip *chip) > +{ > + u8 reg_val; > + int ret; > + > + ret = lp5812_write(chip, chip->cfg->reg_cmd_update.addr, LP5812_UPDATE_CMD_VAL); > + if (ret) > + return ret; > + > + ret = lp5812_read_tsd_config_status(chip, ®_val); > + if (ret) > + return ret; > + > + return reg_val & LP5812_CFG_ERR_STATUS_MASK; > +} > + > +static ssize_t parse_drive_mode(struct lp5812_chip *chip, const char *str) > +{ > + int i; > + > + chip->u_drive_mode.s_drive_mode.mix_sel_led_0 = false; > + chip->u_drive_mode.s_drive_mode.mix_sel_led_1 = false; > + chip->u_drive_mode.s_drive_mode.mix_sel_led_2 = false; > + chip->u_drive_mode.s_drive_mode.mix_sel_led_3 = false; > + > + if (sysfs_streq(str, LP5812_MODE_DIRECT_NAME)) { > + chip->u_drive_mode.s_drive_mode.led_mode = LP5812_MODE_DIRECT_VALUE; > + return 0; > + } > + > + for (i = 0; i < ARRAY_SIZE(chip_mode_map); i++) { > + if (sysfs_streq(str, chip_mode_map[i].mode_name)) { > + chip->u_drive_mode.s_drive_mode.led_mode = chip_mode_map[i].mode; > + chip->u_scan_order.s_scan_order.scan_order_0 = > + chip_mode_map[i].scan_order_0; > + chip->u_scan_order.s_scan_order.scan_order_1 = > + chip_mode_map[i].scan_order_1; > + chip->u_scan_order.s_scan_order.scan_order_2 = > + chip_mode_map[i].scan_order_2; > + chip->u_scan_order.s_scan_order.scan_order_3 = > + chip_mode_map[i].scan_order_3; > + > + switch (chip_mode_map[i].selection_led) { > + case LP5812_MODE_MIX_SELECT_LED_0: > + chip->u_drive_mode.s_drive_mode.mix_sel_led_0 = true; > + break; > + case LP5812_MODE_MIX_SELECT_LED_1: > + chip->u_drive_mode.s_drive_mode.mix_sel_led_1 = true; > + break; > + case LP5812_MODE_MIX_SELECT_LED_2: > + chip->u_drive_mode.s_drive_mode.mix_sel_led_2 = true; > + break; > + case LP5812_MODE_MIX_SELECT_LED_3: > + chip->u_drive_mode.s_drive_mode.mix_sel_led_3 = true; > + break; > + default: > + return -EINVAL; > + } > + > + return 0; > + } Please change the logic to: if (Qsysfs_streq(str, chip_mode_map[i].mode_name)) continue; Then remove one level of tabbing for the remainder. > + } > + > + return -EINVAL; > +} > + > +static int lp5812_set_drive_mode_scan_order(struct lp5812_chip *chip) > +{ > + u8 val; > + int ret; > + > + /* Set led mode */ "LED" "LED mode" doesn't make much sense in LED driver. What exactly do the modes configure? This should probably be "Drive Mode", no? > + val = chip->u_drive_mode.drive_mode_val; > + ret = lp5812_write(chip, chip->cfg->reg_dev_config_1.addr, val); > + if (ret) > + return ret; > + > + /* Setup scan order */ Both of these comments reflect the variable nomenclature, so are they really required? > + val = chip->u_scan_order.scan_order_val; > + ret = lp5812_write(chip, chip->cfg->reg_dev_config_2.addr, val); > + > + return ret; > +} > + > +static int lp5812_set_led_mode(struct lp5812_chip *chip, int led_number, > + enum control_mode mode) > +{ > + u8 reg_val; > + u16 reg; > + int ret; > + > + if (led_number < LP5812_NUMBER_LED_IN_REG) > + reg = chip->cfg->reg_dev_config_3.addr; > + else > + reg = chip->cfg->reg_dev_config_4.addr; This part needs more explanation than the comment parts above. > + ret = lp5812_read(chip, reg, ®_val); > + if (ret) > + return ret; > + > + if (mode == LP5812_MODE_MANUAL) > + reg_val &= ~(1 << (led_number % LP5812_NUMBER_LED_IN_REG)); > + else > + reg_val |= (1 << (led_number % LP5812_NUMBER_LED_IN_REG)); > + > + ret = lp5812_write(chip, reg, reg_val); > + if (ret) > + return ret; > + > + ret = lp5812_update_regs_config(chip); > + > + return ret; > +} > + > +static int lp5812_manual_dc_pwm_control(struct lp5812_chip *chip, int led_number, > + u8 val, enum dimming_type dimming_type) > +{ > + u16 led_base_reg; > + int ret; > + > + if (dimming_type == LP5812_DIMMING_ANALOG) > + led_base_reg = chip->cfg->reg_manual_dc_base.addr; > + else > + led_base_reg = chip->cfg->reg_manual_pwm_base.addr; '\n' here. > + ret = lp5812_write(chip, led_base_reg + led_number, val); > + > + return ret; > +} > + > +static int lp5812_auto_dc(struct lp5812_chip *chip, > + int led_number, u8 val) This doesn't need wrapping. > +{ > + return lp5812_write(chip, chip->cfg->reg_auto_dc_base.addr + led_number, val); > +} > + > +static int lp5812_multicolor_brightness(struct lp5812_led *led) > +{ > + int ret, i; > + struct lp5812_chip *chip = led->chip; Swap these lines around please. > + > + guard(mutex)(&chip->lock); > + for (i = 0; i < led->mc_cdev.num_colors; i++) { > + ret = lp5812_manual_dc_pwm_control(chip, led->mc_cdev.subled_info[i].channel, > + led->mc_cdev.subled_info[i].brightness, > + LP5812_DIMMING_PWM); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +static int lp5812_led_brightness(struct lp5812_led *led) > +{ > + struct lp5812_chip *chip = led->chip; > + struct lp5812_led_config *led_cfg; > + int ret; > + > + led_cfg = &chip->led_config[led->chan_nr]; > + > + guard(mutex)(&chip->lock); > + ret = lp5812_manual_dc_pwm_control(chip, led_cfg->led_id[0], > + led->brightness, LP5812_DIMMING_PWM); > + > + return ret; > +} > + > +static int lp5812_set_brightness(struct led_classdev *cdev, > + enum led_brightness brightness) > +{ > + struct lp5812_led *led = container_of(cdev, struct lp5812_led, cdev); > + > + led->brightness = (u8)brightness; '\n' here. > + return lp5812_led_brightness(led); > +} > + > +static int lp5812_set_mc_brightness(struct led_classdev *cdev, > + enum led_brightness brightness) > +{ > + struct led_classdev_mc *mc_dev = lcdev_to_mccdev(cdev); > + struct lp5812_led *led = container_of(mc_dev, struct lp5812_led, mc_cdev); > + > + led_mc_calc_color_components(&led->mc_cdev, brightness); '\n' here. > + return lp5812_multicolor_brightness(led); > +} > + > +static int lp5812_init_led(struct lp5812_led *led, struct lp5812_chip *chip, int chan) > +{ > + struct device *dev = &chip->client->dev; > + struct mc_subled *mc_led_info; > + struct led_classdev *led_cdev; > + int i, ret; > + > + if (chip->led_config[chan].name) { > + led->cdev.name = chip->led_config[chan].name; > + } else { > + led->cdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s:channel%d", > + chip->label ? : chip->client->name, chan); > + if (!led->cdev.name) > + return -ENOMEM; > + } > + > + if (!chip->led_config[chan].is_sc_led) { > + mc_led_info = devm_kcalloc(dev, > + chip->led_config[chan].num_colors, Put this on the line above. > + 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; > + led_cdev->brightness_set_blocking = lp5812_set_mc_brightness; > + led->mc_cdev.num_colors = chip->led_config[chan].num_colors; '\n' here. > + for (i = 0; i < led->mc_cdev.num_colors; i++) { > + mc_led_info[i].color_index = > + chip->led_config[chan].color_id[i]; > + mc_led_info[i].channel = > + chip->led_config[chan].led_id[i]; > + } > + > + led->mc_cdev.subled_info = mc_led_info; > + } else { > + led->cdev.brightness_set_blocking = lp5812_set_brightness; > + } > + > + led->chan_nr = chan; > + > + if (chip->led_config[chan].is_sc_led) { > + ret = devm_led_classdev_register(dev, &led->cdev); > + if (ret == 0) > + led->cdev.dev->platform_data = led; > + } else { > + ret = devm_led_classdev_multicolor_register(dev, &led->mc_cdev); > + if (ret == 0) > + led->mc_cdev.led_cdev.dev->platform_data = led; > + } > + > + return ret; > +} > + > +static int lp5812_register_leds(struct lp5812_led *led, struct lp5812_chip *chip) > +{ > + struct lp5812_led *each; > + int num_channels = chip->num_channels; > + u8 reg_val; > + u16 reg; > + int ret, i, j; > + > + for (i = 0; i < num_channels; i++) { > + each = led + i; I'm not overly keen on doing this kind of opaque pointer asthmatic on an array of structs. Can you change the formatting here to make it clear that this is happening. Also "leds" and "led" is better than "led" and "each". struct lp5812_led *led = leds[i]; Also '\n' here. > + ret = lp5812_init_led(each, chip, i); > + if (ret) > + goto err_init_led; > + > + each->chip = chip; > + > + for (j = 0; j < chip->led_config[i].num_colors; j++) { > + ret = lp5812_auto_dc(chip, chip->led_config[i].led_id[j], > + chip->led_config[i].max_current[j]); > + if (ret) > + goto err_init_led; > + > + ret = lp5812_manual_dc_pwm_control(chip, chip->led_config[i].led_id[j], > + chip->led_config[i].max_current[j], > + LP5812_DIMMING_ANALOG); > + if (ret) > + goto err_init_led; > + > + ret = lp5812_set_led_mode(chip, chip->led_config[i].led_id[j], > + LP5812_MODE_MANUAL); > + if (ret) > + goto err_init_led; > + > + reg = (chip->led_config[i].led_id[j] < LP5812_NUMBER_LED_IN_REG) ? > + chip->cfg->reg_led_en_1.addr : > + chip->cfg->reg_led_en_2.addr; > + > + ret = lp5812_read(chip, reg, ®_val); > + if (ret) > + goto err_init_led; > + > + reg_val |= (1 << (chip->led_config[i].led_id[j] % What does 1 do here? Please define all of them. #define ENABLE 1 #define MANUAL_MODE_OFF 1 Etc. > + LP5812_NUMBER_LED_IN_REG)); > + > + ret = lp5812_write(chip, reg, reg_val); > + if (ret) > + goto err_init_led; > + } > + } > + > + return 0; > + > +err_init_led: > + return ret; > +} > + > +static int lp5812_init_device(struct lp5812_chip *chip) > +{ > + int ret; > + > + usleep_range(LP5812_WAIT_DEVICE_STABLE_MIN, LP5812_WAIT_DEVICE_STABLE_MAX); > + > + ret = lp5812_write(chip, chip->cfg->reg_chip_en.addr, (u8)1); Define the 1 > + if (ret) { > + dev_err(&chip->client->dev, "lp5812_enable_disable failed\n"); What even is lp5812_enable_disable? Plain English please. "Failed to enable the device"? > + return ret; > + } > + > + ret = lp5812_write(chip, chip->cfg->reg_dev_config_12.addr, LP5812_LSD_LOD_START_UP); > + if (ret) { > + dev_err(&chip->client->dev, "write 0x0B to DEV_CONFIG12 failed\n"); The user doesn't care about registers and values. Swap it out for something in plain English please. > + return ret; > + } > + > + ret = parse_drive_mode(chip, chip->scan_mode); > + if (ret) > + return ret; > + > + ret = lp5812_set_drive_mode_scan_order(chip); > + if (ret) > + return ret; > + > + ret = lp5812_update_regs_config(chip); > + if (ret) { > + dev_err(&chip->client->dev, "lp5812_update_regs_config failed\n"); As above and in all cases. > + return ret; > + } > + > + return 0; > +} > + > +static void lp5812_deinit_device(struct lp5812_chip *chip) > +{ > + lp5812_write(chip, chip->cfg->reg_led_en_1.addr, 0); > + lp5812_write(chip, chip->cfg->reg_led_en_2.addr, 0); > + lp5812_write(chip, chip->cfg->reg_chip_en.addr, 0); These need defining too. > +} > + > +static int lp5812_parse_led_channel(struct device_node *np, > + struct lp5812_led_config *cfg, > + int color_number) > +{ > + int color_id = 0, reg, ret; Why does this need to be pre-initialised? > + u32 max_cur = 0; Or this? > + > + ret = of_property_read_u32(np, "reg", ®); > + if (ret) > + return ret; > + > + cfg->led_id[color_number] = reg; > + > + of_property_read_u32(np, "led-max-microamp", &max_cur); > + cfg->max_current[color_number] = max_cur / 100; Did you just divide by zero? > + > + of_property_read_u32(np, "color", &color_id); > + cfg->color_id[color_number] = color_id; > + > + return 0; > +} > + > +static int lp5812_parse_led(struct device_node *np, > + struct lp5812_led_config *cfg, > + int led_index) > +{ > + int num_colors = 0, ret; As above. > + > + of_property_read_string(np, "label", &cfg[led_index].name); Is this optional? > + ret = of_property_read_u32(np, "reg", &cfg[led_index].chan_nr); > + if (ret) > + return ret; > + > + for_each_available_child_of_node_scoped(np, child) { > + ret = lp5812_parse_led_channel(child, &cfg[led_index], num_colors); > + if (ret) > + return ret; '\n' here. > + num_colors++; > + } > + > + if (num_colors == 0) { > + ret = lp5812_parse_led_channel(np, &cfg[led_index], 0); > + if (ret) > + return ret; '\n' here. > + num_colors = 1; > + cfg[led_index].is_sc_led = true; > + } else { > + cfg[led_index].is_sc_led = false; > + } > + > + cfg[led_index].num_colors = num_colors; > + > + return 0; > +} > + > +static int lp5812_of_populate_pdata(struct device *dev, > + struct device_node *np, > + struct lp5812_chip *chip) > +{ > + struct lp5812_led_config *cfg; > + int num_channels, i = 0, ret; > + > + num_channels = of_get_available_child_count(np); > + if (num_channels == 0) { > + dev_err(dev, "no LED channels\n"); > + return -EINVAL; > + } > + > + cfg = devm_kcalloc(dev, num_channels, sizeof(*cfg), GFP_KERNEL); > + if (!cfg) > + return -ENOMEM; > + > + chip->led_config = &cfg[0]; > + chip->num_channels = num_channels; > + > + for_each_available_child_of_node_scoped(np, child) { > + ret = lp5812_parse_led(child, cfg, i); > + if (ret) > + return -EINVAL; > + i++; > + } > + > + ret = of_property_read_string(np, "ti,scan-mode", &chip->scan_mode); > + if (ret) > + chip->scan_mode = LP5812_MODE_DIRECT_NAME; > + > + of_property_read_string(np, "label", &chip->label); > + > + return 0; > +} > + > +static int lp5812_probe(struct i2c_client *client) > +{ > + struct lp5812_chip *chip; > + struct device_node *np = dev_of_node(&client->dev); > + struct lp5812_led *led; This is all of the LEDs though, right. So "leds" would be better. > + int ret; > + > + if (!np) > + return -EINVAL; > + > + chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL); > + if (!chip) > + return -ENOMEM; > + > + chip->cfg = i2c_get_match_data(client); > + ret = lp5812_of_populate_pdata(&client->dev, np, chip); That's not all this function does though. And it's not pdata. lp5812_of_probe() would probably be better. > + if (ret) > + return ret; > + > + led = devm_kcalloc(&client->dev, chip->num_channels, sizeof(*led), GFP_KERNEL); > + if (!led) > + return -ENOMEM; > + > + chip->client = client; > + mutex_init(&chip->lock); > + i2c_set_clientdata(client, led); If you're only using the chip, why not just save the chip? > + ret = lp5812_init_device(chip); > + if (ret) > + return ret; > + > + ret = lp5812_register_leds(led, chip); > + if (ret) > + goto err_out; > + > + return 0; > + > +err_out: > + lp5812_deinit_device(chip); > + return ret; > +} > + > +static void lp5812_remove(struct i2c_client *client) > +{ > + struct lp5812_led *led = i2c_get_clientdata(client); > + > + lp5812_deinit_device(led->chip); > +} > + > +/* Chip specific configurations */ > +static const struct lp5812_device_config lp5812_cfg = { > + .reg_reset = { > + .addr = LP5812_REG_RESET, > + .val = LP5812_RESET > + }, > + .reg_chip_en = { > + .addr = LP5812_REG_ENABLE, > + .val = LP5812_ENABLE_DEFAULT > + }, > + .reg_dev_config_0 = { > + .addr = LP5812_DEV_CONFIG0, > + .val = 0 > + }, > + .reg_dev_config_1 = { > + .addr = LP5812_DEV_CONFIG1, > + .val = 0 > + }, > + .reg_dev_config_2 = { > + .addr = LP5812_DEV_CONFIG2, > + .val = 0 > + }, > + .reg_dev_config_3 = { > + .addr = LP5812_DEV_CONFIG3, > + .val = 0 > + }, > + .reg_dev_config_4 = { > + .addr = LP5812_DEV_CONFIG4, > + .val = 0 > + }, > + .reg_dev_config_5 = { > + .addr = LP5812_DEV_CONFIG5, > + .val = 0 > + }, > + .reg_dev_config_6 = { > + .addr = LP5812_DEV_CONFIG6, > + .val = 0 > + }, > + .reg_dev_config_7 = { > + .addr = LP5812_DEV_CONFIG7, > + .val = 0 > + }, > + .reg_dev_config_12 = { > + .addr = LP5812_DEV_CONFIG12, > + .val = LP5812_DEV_CONFIG12_DEFAULT > + }, > + .reg_cmd_update = { > + .addr = LP5812_CMD_UPDATE, > + .val = 0 > + }, > + .reg_tsd_config_status = { > + .addr = LP5812_TSD_CONFIG_STATUS, > + .val = 0 > + }, > + .reg_led_en_1 = { > + .addr = LP5812_LED_EN_1, > + .val = 0 > + }, > + .reg_led_en_2 = { > + .addr = LP5812_LED_EN_2, > + .val = 0 > + }, > + .reg_fault_clear = { > + .addr = LP5812_FAULT_CLEAR, > + .val = 0 > + }, > + .reg_manual_dc_base = { > + .addr = LP5812_MANUAL_DC_BASE, > + .val = 0 > + }, > + .reg_auto_dc_base = { > + .addr = LP5812_AUTO_DC_BASE, > + .val = 0 > + }, > + .reg_manual_pwm_base = { > + .addr = LP5812_MANUAL_PWM_BASE, > + .val = 0 > + }, > + .reg_lod_status_base = { > + .addr = LP5812_LOD_STATUS, > + .val = 0 > + }, > + .reg_lsd_status_base = { > + .addr = LP5812_LSD_STATUS, > + .val = 0 > + } > +}; This is an unusual way to set out a register map. Where have you seen this done before? > +static const struct of_device_id of_lp5812_match[] = { > + { .compatible = "ti,lp5812", .data = &lp5812_cfg }, Seems odd to populate .data when you only have a single device. > + {/* NULL */} Remove this comment or replace it with: /* sentinel */ > +}; > + Remove this line. > +MODULE_DEVICE_TABLE(of, of_lp5812_match); > + > +static struct i2c_driver lp5812_driver = { > + .driver = { > + .name = "lp5812", > + .of_match_table = of_lp5812_match, > + }, > + .probe = lp5812_probe, > + .remove = lp5812_remove, > +}; > + And this one. > +module_i2c_driver(lp5812_driver); > + > +MODULE_DESCRIPTION("Texas Instruments LP5812 LED Driver"); > +MODULE_AUTHOR("Jared Zhou"); Email? > +MODULE_LICENSE("GPL"); > diff --git a/drivers/leds/rgb/leds-lp5812.h b/drivers/leds/rgb/leds-lp5812.h > new file mode 100644 > index 000000000000..bb0a9549c151 > --- /dev/null > +++ b/drivers/leds/rgb/leds-lp5812.h > @@ -0,0 +1,197 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * LP5812 Driver Header > + * > + * Copyright (C) 2025 Texas Instruments > + * > + * Author: Jared Zhou <jared-zhou@ti.com> > + */ > + > +#ifndef _LP5812_H_ > +#define _LP5812_H_ > + > +#include <linux/delay.h> > +#include <linux/i2c.h> > +#include <linux/kernel.h> > +#include <linux/led-class-multicolor.h> > +#include <linux/leds.h> > +#include <linux/mutex.h> > +#include <linux/sysfs.h> > +#include <linux/types.h> > + > +#define LP5812_REG_ENABLE 0x0000 > +#define LP5812_REG_RESET 0x0023 > +#define LP5812_DEV_CONFIG0 0x0001 > +#define LP5812_DEV_CONFIG1 0x0002 > +#define LP5812_DEV_CONFIG2 0x0003 > +#define LP5812_DEV_CONFIG3 0x0004 > +#define LP5812_DEV_CONFIG4 0x0005 > +#define LP5812_DEV_CONFIG5 0x0006 > +#define LP5812_DEV_CONFIG6 0x0007 > +#define LP5812_DEV_CONFIG7 0x0008 > +#define LP5812_DEV_CONFIG8 0x0009 > +#define LP5812_DEV_CONFIG9 0x000A > +#define LP5812_DEV_CONFIG10 0x000B > +#define LP5812_DEV_CONFIG11 0x000c > +#define LP5812_DEV_CONFIG12 0x000D > +#define LP5812_CMD_UPDATE 0x0010 > +#define LP5812_LED_EN_1 0x0020 > +#define LP5812_LED_EN_2 0x0021 > +#define LP5812_FAULT_CLEAR 0x0022 > +#define LP5812_MANUAL_DC_BASE 0x0030 > +#define LP5812_AUTO_DC_BASE 0x0050 > +#define LP5812_MANUAL_PWM_BASE 0x0040 > + > +#define LP5812_TSD_CONFIG_STATUS 0x0300 > +#define LP5812_LOD_STATUS 0x0301 > +#define LP5812_LSD_STATUS 0x0303 > + > +#define LP5812_ENABLE_DEFAULT 0x01 > +#define FAULT_CLEAR_ALL 0x07 > +#define TSD_CLEAR_VAL 0x04 > +#define LSD_CLEAR_VAL 0x02 > +#define LOD_CLEAR_VAL 0x01 > +#define LP5812_RESET 0x66 > +#define LP5812_DEV_CONFIG12_DEFAULT 0x08 > + > +#define LP5812_UPDATE_CMD_VAL 0x55 > +#define LP5812_REG_ADDR_HIGH_SHIFT 8 > +#define LP5812_REG_ADDR_BIT_8_9_MASK 0x03 > +#define LP5812_REG_ADDR_LOW_MASK 0xFF > +#define LP5812_CHIP_ADDR_SHIFT 2 > +#define LP5812_DATA_LENGTH 2 > +#define LP5812_DATA_BYTE_0_IDX 0 > +#define LP5812_DATA_BYTE_1_IDX 1 > + > +#define LP5812_READ_MSG_LENGTH 2 > +#define LP5812_MSG_0_IDX 0 > +#define LP5812_MSG_1_IDX 1 > +#define LP5812_CFG_ERR_STATUS_MASK 0x01 > +#define LP5812_CFG_TSD_STATUS_SHIFT 1 > +#define LP5812_CFG_TSD_STATUS_MASK 0x01 > + > +#define LP5812_FAULT_CLEAR_LOD 0 > +#define LP5812_FAULT_CLEAR_LSD 1 > +#define LP5812_FAULT_CLEAR_TSD 2 > +#define LP5812_FAULT_CLEAR_ALL 3 > +#define LP5812_NUMBER_LED_IN_REG 8 > + > +#define LP5812_WAIT_DEVICE_STABLE_MIN 1000 > +#define LP5812_WAIT_DEVICE_STABLE_MAX 1100 > + > +#define LP5812_LSD_LOD_START_UP 0x0B > +#define LP5812_MODE_NAME_MAX_LEN 20 > +#define LP5812_MODE_DIRECT_NAME "direct_mode" > +#define LP5812_MODE_DIRECT_VALUE 0 > +#define LP5812_MODE_MIX_SELECT_LED_0 0 > +#define LP5812_MODE_MIX_SELECT_LED_1 1 > +#define LP5812_MODE_MIX_SELECT_LED_2 2 > +#define LP5812_MODE_MIX_SELECT_LED_3 3 > + > +enum control_mode { > + LP5812_MODE_MANUAL = 0, > + LP5812_MODE_AUTONOMOUS > +}; > + > +enum dimming_type { > + LP5812_DIMMING_ANALOG, > + LP5812_DIMMING_PWM > +}; > + > +union u_scan_order { > + struct { > + u8 scan_order_0:2; > + u8 scan_order_1:2; > + u8 scan_order_2:2; > + u8 scan_order_3:2; > + } s_scan_order; > + u8 scan_order_val; > +}; > + > +union u_drive_mode { > + struct { > + u8 mix_sel_led_0:1; > + u8 mix_sel_led_1:1; > + u8 mix_sel_led_2:1; > + u8 mix_sel_led_3:1; > + u8 led_mode:3; > + u8 pwm_fre:1; > + } s_drive_mode; > + u8 drive_mode_val; > +}; > + > +struct lp5812_reg { > + u16 addr; > + union { > + u8 val; > + u8 mask; > + u8 shift; > + }; > +}; > + > +struct lp5812_mode_mapping { > + char mode_name[LP5812_MODE_NAME_MAX_LEN]; > + u8 mode; > + u8 scan_order_0; > + u8 scan_order_1; > + u8 scan_order_2; > + u8 scan_order_3; > + u8 selection_led; > +}; > + > +struct lp5812_led_config { > + bool is_sc_led; > + const char *name; > + u8 color_id[LED_COLOR_ID_MAX]; > + u32 max_current[LED_COLOR_ID_MAX]; > + int chan_nr; > + int num_colors; > + int led_id[LED_COLOR_ID_MAX]; > +}; > + > +struct lp5812_chip { > + u8 num_channels; > + struct i2c_client *client; > + struct mutex lock; /* Protects register access */ > + struct lp5812_led_config *led_config; > + const char *label; > + const char *scan_mode; > + const struct lp5812_device_config *cfg; > + union u_scan_order u_scan_order; > + union u_drive_mode u_drive_mode; > +}; > + > +struct lp5812_led { > + u8 brightness; > + int chan_nr; > + struct led_classdev cdev; > + struct led_classdev_mc mc_cdev; > + struct lp5812_chip *chip; > +}; > + > +struct lp5812_device_config { > + const struct lp5812_reg reg_reset; > + const struct lp5812_reg reg_chip_en; > + const struct lp5812_reg reg_dev_config_0; > + const struct lp5812_reg reg_dev_config_1; > + const struct lp5812_reg reg_dev_config_2; > + const struct lp5812_reg reg_dev_config_3; > + const struct lp5812_reg reg_dev_config_4; > + const struct lp5812_reg reg_dev_config_5; > + const struct lp5812_reg reg_dev_config_6; > + const struct lp5812_reg reg_dev_config_7; > + const struct lp5812_reg reg_dev_config_12; > + const struct lp5812_reg reg_cmd_update; > + > + const struct lp5812_reg reg_led_en_1; > + const struct lp5812_reg reg_led_en_2; > + const struct lp5812_reg reg_fault_clear; > + const struct lp5812_reg reg_manual_dc_base; > + const struct lp5812_reg reg_auto_dc_base; > + const struct lp5812_reg reg_manual_pwm_base; > + const struct lp5812_reg reg_tsd_config_status; > + const struct lp5812_reg reg_lod_status_base; > + const struct lp5812_reg reg_lsd_status_base; > +}; > + > +#endif /*_LP5812_H_*/ > -- > 2.25.1 > > -- Lee Jones [李琼斯] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v17 2/3] leds: add basic support for TI/National Semiconductor LP5812 LED Driver 2025-11-06 15:59 ` Lee Jones @ 2025-11-11 17:07 ` Nam Tran 2025-11-13 11:29 ` Lee Jones 0 siblings, 1 reply; 8+ messages in thread From: Nam Tran @ 2025-11-11 17:07 UTC (permalink / raw) To: lee Cc: gregkh, pavel, rdunlap, christophe.jaillet, krzk+dt, robh, conor+dt, corbet, linux-leds, linux-kernel, devicetree, linux-doc On Thu, 6 Nov 2025, Lee Jones wrote: > On Tue, 21 Oct 2025, Nam Tran wrote: > > > The LP5812 is a 4x3 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. > > > > This patch adds minimal driver support for the LP5812, implementing > > only the essential functionality: I2C communication with the device, > > LED registration, brightness control in manual mode, and basic sysfs > > interfaces for LED configuration and fault monitoring. > > > > Signed-off-by: Nam Tran <trannamatk@gmail.com> > > --- > > MAINTAINERS | 4 + > > drivers/leds/rgb/Kconfig | 13 + > > drivers/leds/rgb/Makefile | 1 + > > drivers/leds/rgb/leds-lp5812.c | 730 +++++++++++++++++++++++++++++++++ > > drivers/leds/rgb/leds-lp5812.h | 197 +++++++++ > > 5 files changed, 945 insertions(+) > > create mode 100644 drivers/leds/rgb/leds-lp5812.c > > create mode 100644 drivers/leds/rgb/leds-lp5812.h > > Last go - just a few nits to fix-up. Thank you for the feedback. I'll address these minor issues and include the fixes in the next revision. But I have a few concerns about some of the nits. > > +static int lp5812_parse_led(struct device_node *np, > > + struct lp5812_led_config *cfg, > > + int led_index) > > +{ > > + int num_colors = 0, ret; > > As above. > > > + > > + of_property_read_string(np, "label", &cfg[led_index].name); > > Is this optional? The 'label' property is required for proper sysfs naming. Should I update the DT binding to mark it mandatory and adjust the driver accordingly? I'd like to confirm if this aligns with usual conventions for such properties. > > +static int lp5812_probe(struct i2c_client *client) > > +{ > > + struct lp5812_chip *chip; > > + struct device_node *np = dev_of_node(&client->dev); > > + struct lp5812_led *led; > > This is all of the LEDs though, right. > > So "leds" would be better. > > > + int ret; > > + > > + if (!np) > > + return -EINVAL; > > + > > + chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL); > > + if (!chip) > > + return -ENOMEM; > > + > > + chip->cfg = i2c_get_match_data(client); > > + ret = lp5812_of_populate_pdata(&client->dev, np, chip); > > That's not all this function does though. > > And it's not pdata. > > lp5812_of_probe() would probably be better. > > > + if (ret) > > + return ret; > > + > > + led = devm_kcalloc(&client->dev, chip->num_channels, sizeof(*led), GFP_KERNEL); > > + if (!led) > > + return -ENOMEM; > > + > > + chip->client = client; > > + mutex_init(&chip->lock); > > + i2c_set_clientdata(client, led); > > If you're only using the chip, why not just save the chip? Just to confirm, you mean to store all LED instances inside the lp5812_chip struct and only save the chip in i2c_set_clientdata(), instead of allocating a separate leds array in probe()? I can update the code accordingly if that's the preferred approach. > > +/* Chip specific configurations */ > > +static const struct lp5812_device_config lp5812_cfg = { > > + .reg_reset = { > > + .addr = LP5812_REG_RESET, > > + .val = LP5812_RESET > > + }, > > + .reg_chip_en = { > > + .addr = LP5812_REG_ENABLE, > > + .val = LP5812_ENABLE_DEFAULT > > + }, > > + .reg_dev_config_0 = { > > + .addr = LP5812_DEV_CONFIG0, > > + .val = 0 > > + }, > > + .reg_dev_config_1 = { > > + .addr = LP5812_DEV_CONFIG1, > > + .val = 0 > > + }, > > + .reg_dev_config_2 = { > > + .addr = LP5812_DEV_CONFIG2, > > + .val = 0 > > + }, > > + .reg_dev_config_3 = { > > + .addr = LP5812_DEV_CONFIG3, > > + .val = 0 > > + }, > > + .reg_dev_config_4 = { > > + .addr = LP5812_DEV_CONFIG4, > > + .val = 0 > > + }, > > + .reg_dev_config_5 = { > > + .addr = LP5812_DEV_CONFIG5, > > + .val = 0 > > + }, > > + .reg_dev_config_6 = { > > + .addr = LP5812_DEV_CONFIG6, > > + .val = 0 > > + }, > > + .reg_dev_config_7 = { > > + .addr = LP5812_DEV_CONFIG7, > > + .val = 0 > > + }, > > + .reg_dev_config_12 = { > > + .addr = LP5812_DEV_CONFIG12, > > + .val = LP5812_DEV_CONFIG12_DEFAULT > > + }, > > + .reg_cmd_update = { > > + .addr = LP5812_CMD_UPDATE, > > + .val = 0 > > + }, > > + .reg_tsd_config_status = { > > + .addr = LP5812_TSD_CONFIG_STATUS, > > + .val = 0 > > + }, > > + .reg_led_en_1 = { > > + .addr = LP5812_LED_EN_1, > > + .val = 0 > > + }, > > + .reg_led_en_2 = { > > + .addr = LP5812_LED_EN_2, > > + .val = 0 > > + }, > > + .reg_fault_clear = { > > + .addr = LP5812_FAULT_CLEAR, > > + .val = 0 > > + }, > > + .reg_manual_dc_base = { > > + .addr = LP5812_MANUAL_DC_BASE, > > + .val = 0 > > + }, > > + .reg_auto_dc_base = { > > + .addr = LP5812_AUTO_DC_BASE, > > + .val = 0 > > + }, > > + .reg_manual_pwm_base = { > > + .addr = LP5812_MANUAL_PWM_BASE, > > + .val = 0 > > + }, > > + .reg_lod_status_base = { > > + .addr = LP5812_LOD_STATUS, > > + .val = 0 > > + }, > > + .reg_lsd_status_base = { > > + .addr = LP5812_LSD_STATUS, > > + .val = 0 > > + } > > +}; > > This is an unusual way to set out a register map. > > Where have you seen this done before? > > > +static const struct of_device_id of_lp5812_match[] = { > > + { .compatible = "ti,lp5812", .data = &lp5812_cfg }, > > Seems odd to populate .data when you only have a single device. I followed the style used in the lp55xx series drivers for the register map and device config. I thought it makes sense to keep the same pattern to allow easier upgrade and maintenance in the future. But you expect a more typical approach, right? Thanks again for your review and support! Best regards, Nam Tran ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v17 2/3] leds: add basic support for TI/National Semiconductor LP5812 LED Driver 2025-11-11 17:07 ` Nam Tran @ 2025-11-13 11:29 ` Lee Jones 0 siblings, 0 replies; 8+ messages in thread From: Lee Jones @ 2025-11-13 11:29 UTC (permalink / raw) To: Nam Tran Cc: gregkh, pavel, rdunlap, christophe.jaillet, krzk+dt, robh, conor+dt, corbet, linux-leds, linux-kernel, devicetree, linux-doc On Wed, 12 Nov 2025, Nam Tran wrote: > On Thu, 6 Nov 2025, Lee Jones wrote: > > > On Tue, 21 Oct 2025, Nam Tran wrote: > > > > > The LP5812 is a 4x3 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. > > > > > > This patch adds minimal driver support for the LP5812, implementing > > > only the essential functionality: I2C communication with the device, > > > LED registration, brightness control in manual mode, and basic sysfs > > > interfaces for LED configuration and fault monitoring. > > > > > > Signed-off-by: Nam Tran <trannamatk@gmail.com> > > > --- > > > MAINTAINERS | 4 + > > > drivers/leds/rgb/Kconfig | 13 + > > > drivers/leds/rgb/Makefile | 1 + > > > drivers/leds/rgb/leds-lp5812.c | 730 +++++++++++++++++++++++++++++++++ > > > drivers/leds/rgb/leds-lp5812.h | 197 +++++++++ > > > 5 files changed, 945 insertions(+) > > > create mode 100644 drivers/leds/rgb/leds-lp5812.c > > > create mode 100644 drivers/leds/rgb/leds-lp5812.h > > > > Last go - just a few nits to fix-up. > > Thank you for the feedback. > I'll address these minor issues and include the fixes in the next revision. > But I have a few concerns about some of the nits. > > > > +static int lp5812_parse_led(struct device_node *np, > > > + struct lp5812_led_config *cfg, > > > + int led_index) > > > +{ > > > + int num_colors = 0, ret; > > > > As above. > > > > > + > > > + of_property_read_string(np, "label", &cfg[led_index].name); > > > > Is this optional? > > The 'label' property is required for proper sysfs naming. Should I update the DT binding > to mark it mandatory and adjust the driver accordingly? I'd like to confirm if this aligns > with usual conventions for such properties. I'll let you look around and decide for yourself. If this is not optional, you should check this call for errors. > > > +static int lp5812_probe(struct i2c_client *client) > > > +{ > > > + struct lp5812_chip *chip; > > > + struct device_node *np = dev_of_node(&client->dev); > > > + struct lp5812_led *led; > > > > This is all of the LEDs though, right. > > > > So "leds" would be better. > > > > > + int ret; > > > + > > > + if (!np) > > > + return -EINVAL; > > > + > > > + chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL); > > > + if (!chip) > > > + return -ENOMEM; > > > + > > > + chip->cfg = i2c_get_match_data(client); > > > + ret = lp5812_of_populate_pdata(&client->dev, np, chip); > > > > That's not all this function does though. > > > > And it's not pdata. > > > > lp5812_of_probe() would probably be better. > > > > > + if (ret) > > > + return ret; > > > + > > > + led = devm_kcalloc(&client->dev, chip->num_channels, sizeof(*led), GFP_KERNEL); > > > + if (!led) > > > + return -ENOMEM; > > > + > > > + chip->client = client; > > > + mutex_init(&chip->lock); > > > + i2c_set_clientdata(client, led); > > > > If you're only using the chip, why not just save the chip? > > Just to confirm, you mean to store all LED instances inside the lp5812_chip struct and > only save the chip in i2c_set_clientdata(), instead of allocating a separate leds array > in probe()? At the moment, it looks as though you save the array of `led`s and pull out the `chip` pointer from the first one (in .remove() below). Why not just store the `chip` in clientdata in the first place? > I can update the code accordingly if that's the preferred approach. > > > > +/* Chip specific configurations */ > > > +static const struct lp5812_device_config lp5812_cfg = { > > > + .reg_reset = { > > > + .addr = LP5812_REG_RESET, > > > + .val = LP5812_RESET > > > + }, > > > + .reg_chip_en = { > > > + .addr = LP5812_REG_ENABLE, > > > + .val = LP5812_ENABLE_DEFAULT > > > + }, > > > + .reg_dev_config_0 = { > > > + .addr = LP5812_DEV_CONFIG0, > > > + .val = 0 > > > + }, > > > + .reg_dev_config_1 = { > > > + .addr = LP5812_DEV_CONFIG1, > > > + .val = 0 > > > + }, > > > + .reg_dev_config_2 = { > > > + .addr = LP5812_DEV_CONFIG2, > > > + .val = 0 > > > + }, > > > + .reg_dev_config_3 = { > > > + .addr = LP5812_DEV_CONFIG3, > > > + .val = 0 > > > + }, > > > + .reg_dev_config_4 = { > > > + .addr = LP5812_DEV_CONFIG4, > > > + .val = 0 > > > + }, > > > + .reg_dev_config_5 = { > > > + .addr = LP5812_DEV_CONFIG5, > > > + .val = 0 > > > + }, > > > + .reg_dev_config_6 = { > > > + .addr = LP5812_DEV_CONFIG6, > > > + .val = 0 > > > + }, > > > + .reg_dev_config_7 = { > > > + .addr = LP5812_DEV_CONFIG7, > > > + .val = 0 > > > + }, > > > + .reg_dev_config_12 = { > > > + .addr = LP5812_DEV_CONFIG12, > > > + .val = LP5812_DEV_CONFIG12_DEFAULT > > > + }, > > > + .reg_cmd_update = { > > > + .addr = LP5812_CMD_UPDATE, > > > + .val = 0 > > > + }, > > > + .reg_tsd_config_status = { > > > + .addr = LP5812_TSD_CONFIG_STATUS, > > > + .val = 0 > > > + }, > > > + .reg_led_en_1 = { > > > + .addr = LP5812_LED_EN_1, > > > + .val = 0 > > > + }, > > > + .reg_led_en_2 = { > > > + .addr = LP5812_LED_EN_2, > > > + .val = 0 > > > + }, > > > + .reg_fault_clear = { > > > + .addr = LP5812_FAULT_CLEAR, > > > + .val = 0 > > > + }, > > > + .reg_manual_dc_base = { > > > + .addr = LP5812_MANUAL_DC_BASE, > > > + .val = 0 > > > + }, > > > + .reg_auto_dc_base = { > > > + .addr = LP5812_AUTO_DC_BASE, > > > + .val = 0 > > > + }, > > > + .reg_manual_pwm_base = { > > > + .addr = LP5812_MANUAL_PWM_BASE, > > > + .val = 0 > > > + }, > > > + .reg_lod_status_base = { > > > + .addr = LP5812_LOD_STATUS, > > > + .val = 0 > > > + }, > > > + .reg_lsd_status_base = { > > > + .addr = LP5812_LSD_STATUS, > > > + .val = 0 > > > + } > > > +}; > > > > This is an unusual way to set out a register map. > > > > Where have you seen this done before? > > > > > +static const struct of_device_id of_lp5812_match[] = { > > > + { .compatible = "ti,lp5812", .data = &lp5812_cfg }, > > > > Seems odd to populate .data when you only have a single device. > > I followed the style used in the lp55xx series drivers for the register map and device > config. I thought it makes sense to keep the same pattern to allow easier upgrade and > maintenance in the future. But you expect a more typical approach, right? You only need to provide differentiation when you support more than one device. -- Lee Jones [李琼斯] ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v17 3/3] docs: leds: Document TI LP5812 LED driver 2025-10-21 15:59 [PATCH v17 0/3] leds: add new LED driver for TI LP5812 Nam Tran 2025-10-21 15:59 ` [PATCH v17 1/3] dt-bindings: leds: add TI/National Semiconductor LP5812 LED Driver Nam Tran 2025-10-21 15:59 ` [PATCH v17 2/3] leds: add basic support for " Nam Tran @ 2025-10-21 15:59 ` Nam Tran 2 siblings, 0 replies; 8+ messages in thread From: Nam Tran @ 2025-10-21 15:59 UTC (permalink / raw) To: lee Cc: pavel, gregkh, rdunlap, christophe.jaillet, krzk+dt, robh, conor+dt, corbet, linux-leds, linux-kernel, devicetree, linux-doc, Nam Tran The driver provides sysfs interfaces to control and configure the LP5812 device and its LED channels. The documetation describes the chip's capabilities, sysfs interface, and usage examples. Signed-off-by: Nam Tran <trannamatk@gmail.com> --- Documentation/leds/index.rst | 1 + Documentation/leds/leds-lp5812.rst | 50 ++++++++++++++++++++++++++++++ MAINTAINERS | 1 + 3 files changed, 52 insertions(+) create mode 100644 Documentation/leds/leds-lp5812.rst diff --git a/Documentation/leds/index.rst b/Documentation/leds/index.rst index 76fae171039c..bebf44004278 100644 --- a/Documentation/leds/index.rst +++ b/Documentation/leds/index.rst @@ -25,6 +25,7 @@ LEDs leds-lp5523 leds-lp5562 leds-lp55xx + leds-lp5812 leds-mlxcpld leds-mt6370-rgb leds-sc27xx diff --git a/Documentation/leds/leds-lp5812.rst b/Documentation/leds/leds-lp5812.rst new file mode 100644 index 000000000000..4c22d9a79d14 --- /dev/null +++ b/Documentation/leds/leds-lp5812.rst @@ -0,0 +1,50 @@ +.. SPDX-License-Identifier: GPL-2.0 + +======================== +Kernel driver for lp5812 +======================== + +* TI/National Semiconductor LP5812 LED Driver +* Datasheet: https://www.ti.com/product/LP5812#tech-docs + +Authors: Jared Zhou <jared-zhou@ti.com> + +Description +=========== + +The LP5812 is a 4x3 matrix LED driver with support for both manual and +autonomous animation control. This driver provides sysfs interfaces to +control and configure the LP5812 device and its LED channels. + +Sysfs Interface +=============== + +This driver uses the standard multicolor LED class interfaces defined +in `Documentation/ABI/testing/sysfs-class-led-multicolor`. + +Each LP5812 LED output appears under ``/sys/class/leds/`` with its +assigned label (for example ``LED_A``). + +The following attributes are exposed: + - multi_intensity: Per-channel RGB intensity control. + - brightness: Standard brightness control (0-255) + +Autonomous Control Modes +======================== + +The driver also supports autonomous control through pattern configuration +(e.g., direct, tcmscan, or mixscan modes) defined in the device tree. +When configured, the LP5812 can generate transitions and color effects +without CPU intervention. + +Refer to the device tree binding document for valid mode strings and +configuration examples. + +Example Usage +============= + +To control LED_A:: + # Set RGB intensity (R=50, G=50, B=50) + echo 50 50 50 > /sys/class/leds/LED_A/multi_intensity + # Set overall brightness to maximum + echo 255 > /sys/class/leds/LED_A/brightness diff --git a/MAINTAINERS b/MAINTAINERS index ee56b68e54cd..e35ae62b7c1a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -25429,6 +25429,7 @@ M: Nam Tran <trannamatk@gmail.com> L: linux-leds@vger.kernel.org S: Maintained F: Documentation/devicetree/bindings/leds/ti,lp5812.yaml +F: Documentation/leds/leds-lp5812.rst F: drivers/leds/rgb/Kconfig F: drivers/leds/rgb/Makefile F: drivers/leds/rgb/leds-lp5812.c -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-11-13 11:29 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-21 15:59 [PATCH v17 0/3] leds: add new LED driver for TI LP5812 Nam Tran 2025-10-21 15:59 ` [PATCH v17 1/3] dt-bindings: leds: add TI/National Semiconductor LP5812 LED Driver Nam Tran 2025-10-26 22:06 ` Rob Herring (Arm) 2025-10-21 15:59 ` [PATCH v17 2/3] leds: add basic support for " Nam Tran 2025-11-06 15:59 ` Lee Jones 2025-11-11 17:07 ` Nam Tran 2025-11-13 11:29 ` Lee Jones 2025-10-21 15:59 ` [PATCH v17 3/3] docs: leds: Document TI LP5812 LED driver Nam Tran
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.