[PATCH 0/3] leds: lp55xx: Fix the LP5569 LED mapping table placement

[PATCH 0/3] leds: lp55xx: Fix the LP5569 LED mapping table placement

[Arthur Crepin Leblond]

The LP5569 and LP5523 keep a per-engine LED mapping table in the SRAM.
The table is read by the engine's map_* instructions.

On LP5569, the instruction loads a table from a 7-bits absolute SRAM
address (0-127), therefore the mapping table address must be in the
lower-half of the SRAM.

This series makes the layout configurable and fixes the LP5569:

  - lp55xx: Add program and mux base pages to the device config
    - Add prog_page_base and mux_page_base to struct
      lp55xx_device_config and use them for the engine program pages
      and for the mapping tables written by the engineN_leds attribute.
    - lp5569/lp5523/lp8501: set values matching their current layout,
      so their behavior is unchanged.

  - lp5569: Place the LED mapping table in the lower half of the SRAM
    - Place the mapping table at pages 0-2 (address 0x00/0x10/0x20)
    - Place the engine programs at addresses 0x30/0x70/0xb0 (4 pages
      per engine)

Tested on a Raspberry Pi Zero 2W.

Arthur Crepin Leblond

Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>
---
Arthur Crepin Leblond (3):
      leds: lp55xx: Add program and mux base pages to the device config
      leds: leds-lp5569: Place the LED mapping table in the lower half of the SRAM
      Documentation: leds: lp55xx: Add an LP5569 example

 Documentation/leds/leds-lp55xx.rst | 24 +++++++++++++++++
 drivers/leds/leds-lp5523.c         |  4 +++
 drivers/leds/leds-lp5569.c         | 53 ++++++++++++++++++++++++++------------
 drivers/leds/leds-lp55xx-common.c  | 14 +++++-----
 drivers/leds/leds-lp55xx-common.h  |  6 +++++
 drivers/leds/leds-lp8501.c         |  2 ++
 6 files changed, 80 insertions(+), 23 deletions(-)
---
base-commit: c190ff288cb5c09e1b30db9e51a11d979d27f5b9
change-id: 20261026-lp55xx-1bdb2c9d8fc1

[PATCH 1/3] leds: lp55xx: Add program and mux base pages to the device config

[Arthur Crepin Leblond]

Add program and mux base page to struct lp55xx_device_config to
set the first page of the engine programs and of the LED mapping table.

Until now the mapping tables were placed at the end of the programs.
On the LP5569 this puts them in the upper half of the SRAM, while the
map_* instructions can only address words 0-127.
This change allows placing the mapping tables before the programs instead.

All the chips set the values matching their current layout, so their
behavior is unchanged:
  - LP5523 has the 3 engine programs on the first 3 pages and their
    LED mapping tables on the next 3 pages.
  - LP5569 has the 3 engine programs of 4 pages each, followed by the
    LED mapping tables.
  - LP8501 stores its program at address 0.

Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>
---
 drivers/leds/leds-lp5523.c        |  4 ++++
 drivers/leds/leds-lp5569.c        |  4 ++++
 drivers/leds/leds-lp55xx-common.c | 14 ++++++++------
 drivers/leds/leds-lp55xx-common.h |  6 ++++++
 drivers/leds/leds-lp8501.c        |  2 ++
 5 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index 39cb26d343c1..3ec9f3f31708 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -31,6 +31,8 @@
  * 0x50 engine 3 muxing info
  */
 #define LP5523_PAGES_PER_ENGINE		1
+#define LP5523_PROG_PAGE_BASE		0
+#define LP5523_MUX_PAGE_BASE		3
 #define LP5523_MAX_LEDS			9
 
 /* Registers */
@@ -335,6 +337,8 @@ static struct lp55xx_device_config lp5523_cfg = {
 		.addr = LP5523_REG_LED_CTRL_BASE,
 	},
 	.pages_per_engine   = LP5523_PAGES_PER_ENGINE,
+	.prog_page_base     = LP5523_PROG_PAGE_BASE,
+	.mux_page_base      = LP5523_MUX_PAGE_BASE,
 	.max_channel  = LP5523_MAX_LEDS,
 	.post_init_device   = lp5523_post_init_device,
 	.brightness_fn      = lp55xx_led_brightness,
diff --git a/drivers/leds/leds-lp5569.c b/drivers/leds/leds-lp5569.c
index 199db3efca65..15beac7b34ea 100644
--- a/drivers/leds/leds-lp5569.c
+++ b/drivers/leds/leds-lp5569.c
@@ -30,6 +30,8 @@
  * 0xe0 engine 3 muxing info (1 page)
  */
 #define LP5569_PAGES_PER_ENGINE		4
+#define LP5569_PROG_PAGE_BASE		0
+#define LP5569_MUX_PAGE_BASE		12
 
 #define LP5569_REG_ENABLE		0x00
 #define   LP5569_ENABLE			BIT(6)
@@ -503,6 +505,8 @@ static struct lp55xx_device_config lp5569_cfg = {
 		.addr = LP5569_REG_LED_CTRL_BASE,
 	},
 	.pages_per_engine   = LP5569_PAGES_PER_ENGINE,
+	.prog_page_base     = LP5569_PROG_PAGE_BASE,
+	.mux_page_base      = LP5569_MUX_PAGE_BASE,
 	.max_channel  = LP5569_MAX_LEDS,
 	.post_init_device   = lp5569_post_init_device,
 	.brightness_fn      = lp55xx_led_brightness,
diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
index ea131177de96..e2c55ce5d92b 100644
--- a/drivers/leds/leds-lp55xx-common.c
+++ b/drivers/leds/leds-lp55xx-common.c
@@ -61,8 +61,8 @@
 
 /* Memory Page Selection */
 #define LP55xx_REG_PROG_PAGE_SEL	0x4f
-/* If supported, each ENGINE have an equal amount of pages offset from page 0 */
-#define LP55xx_PAGE_OFFSET(n, pages)	(((n) - 1) * (pages))
+/* If supported, each ENGINE have an equal amount of pages offset from base */
+#define LP55xx_PAGE_OFFSET(n, pages, base)	((base) + ((n) - 1) * (pages))
 
 #define LED_ACTIVE(mux, led)		(!!((mux) & (0x0001 << (led))))
 
@@ -131,7 +131,8 @@ void lp55xx_load_engine(struct lp55xx_chip *chip)
 	/* Setup PAGE if supported (pages_per_engine not 0)*/
 	if (cfg->pages_per_engine)
 		lp55xx_write(chip, LP55xx_REG_PROG_PAGE_SEL,
-			     LP55xx_PAGE_OFFSET(idx, cfg->pages_per_engine));
+			     LP55xx_PAGE_OFFSET(idx, cfg->pages_per_engine,
+						cfg->prog_page_base));
 }
 EXPORT_SYMBOL_GPL(lp55xx_load_engine);
 
@@ -176,6 +177,8 @@ int lp55xx_update_program_memory(struct lp55xx_chip *chip,
 	const struct lp55xx_device_config *cfg = chip->cfg;
 	u8 pattern[LP55xx_MAX_PROGRAM_LENGTH] = { };
 	u8 start_addr = cfg->prog_mem_base.addr;
+	int first_page = LP55xx_PAGE_OFFSET(idx, cfg->pages_per_engine,
+					    cfg->prog_page_base);
 	int page, i = 0, offset = 0;
 	int program_length, ret;
 
@@ -218,7 +221,7 @@ int lp55xx_update_program_memory(struct lp55xx_chip *chip,
 		/* Write to the next page each 32 bytes (if supported) */
 		if (cfg->pages_per_engine)
 			lp55xx_write(chip, LP55xx_REG_PROG_PAGE_SEL,
-				     LP55xx_PAGE_OFFSET(idx, cfg->pages_per_engine) + page);
+				     first_page + page);
 
 		for (i = 0; i < LP55xx_BYTES_PER_PAGE; i++) {
 			ret = lp55xx_write(chip, start_addr + i,
@@ -769,8 +772,7 @@ static int lp55xx_load_mux(struct lp55xx_chip *chip, u16 mux, int nr)
 
 	lp55xx_load_engine(chip);
 
-	/* Derive the MUX page offset by starting at the end of the ENGINE pages */
-	mux_page = cfg->pages_per_engine * LP55XX_ENGINE_MAX + (nr - 1);
+	mux_page = cfg->mux_page_base + (nr - 1);
 	ret = lp55xx_write(chip, LP55xx_REG_PROG_PAGE_SEL, mux_page);
 	if (ret)
 		return ret;
diff --git a/drivers/leds/leds-lp55xx-common.h b/drivers/leds/leds-lp55xx-common.h
index 8fd64ec40919..7ef657047933 100644
--- a/drivers/leds/leds-lp55xx-common.h
+++ b/drivers/leds/leds-lp55xx-common.h
@@ -125,6 +125,10 @@ struct lp55xx_reg {
  * @reg_led_ctrl_base  : Chip specific base reg address for LED ctrl base
  * @pages_per_engine   : Assigned pages for each engine
  *                       (if not set chip doesn't support pages)
+ * @prog_page_base     : First page of the engine 1 program, the programs of
+ *                       the other engines follow one after another
+ * @mux_page_base      : First page of the engine 1 LED mapping row, the rows
+ *                       of the other engines are in the next pages
  * @max_channel        : Maximum number of channels
  * @post_init_device   : Chip specific initialization code
  * @brightness_fn      : Brightness function
@@ -146,6 +150,8 @@ struct lp55xx_device_config {
 	const struct lp55xx_reg reg_master_fader_base;
 	const struct lp55xx_reg reg_led_ctrl_base;
 	const int pages_per_engine;
+	const u8 prog_page_base;
+	const u8 mux_page_base;
 	const int max_channel;
 
 	/* define if the device has specific initialization process */
diff --git a/drivers/leds/leds-lp8501.c b/drivers/leds/leds-lp8501.c
index 8009689db7b4..1c05839cd4c2 100644
--- a/drivers/leds/leds-lp8501.c
+++ b/drivers/leds/leds-lp8501.c
@@ -21,6 +21,7 @@
 #include "leds-lp55xx-common.h"
 
 #define LP8501_PAGES_PER_ENGINE		1
+#define LP8501_PROG_PAGE_BASE		0
 #define LP8501_MAX_LEDS			9
 
 /* Registers */
@@ -121,6 +122,7 @@ static struct lp55xx_device_config lp8501_cfg = {
 		.addr = LP8501_REG_LED_CURRENT_BASE,
 	},
 	.pages_per_engine   = LP8501_PAGES_PER_ENGINE,
+	.prog_page_base     = LP8501_PROG_PAGE_BASE,
 	.max_channel  = LP8501_MAX_LEDS,
 	.post_init_device   = lp8501_post_init_device,
 	.brightness_fn      = lp55xx_led_brightness,

-- 
2.56.0

[PATCH 2/3] leds: leds-lp5569: Place the LED mapping table in the lower half of the SRAM

[Arthur Crepin Leblond]

The LED mapping instructions (map_start, load_end, map_addr, ...) take
a 7-bits absolute SRAM address, so the mapping table must be in the
lower half of the SRAM (0-127).
The driver placed the muxing info after the engine programs, which
these instructions cannot reach.

Place the mapping tables in pages 0-2 and the engine programs after
them.

This changes the SRAM layout seen by userspace:

  - The program of engines 1/2/3 now starts at address 0x30/0x70/0xb0
    instead of 0x00/0x40/0x80 (ENGINEx_PROG_START).
  - The mapping of engines 1/2/3 is at address 0x00/0x10/0x20.

Programs loaded through engineN_load or the firmware interface are not
affected, since they are written relative to the start of the engine.
Programs that use absolute SRAM addresses (map_start, load_end,
load_addr, map_addr) must be updated to the new layout.

Fixes: 30c6743cc89c ("leds: leds-lp5569: Add support for Texas Instruments LP5569")
Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>
---
 drivers/leds/leds-lp5569.c | 53 +++++++++++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 19 deletions(-)

diff --git a/drivers/leds/leds-lp5569.c b/drivers/leds/leds-lp5569.c
index 15beac7b34ea..a4d9787c4158 100644
--- a/drivers/leds/leds-lp5569.c
+++ b/drivers/leds/leds-lp5569.c
@@ -21,17 +21,21 @@
 
 #define LP5569_MAX_LEDS			9
 
-/* Memory is used like this:
- * 0x00 engine 1 program (4 pages)
- * 0x40 engine 2 program (4 pages)
- * 0x80 engine 3 program (4 pages)
- * 0xc0 engine 1 muxing info (1 page)
- * 0xd0 engine 2 muxing info (1 page)
- * 0xe0 engine 3 muxing info (1 page)
+/*
+ * Memory is used like this (a page is 16 instructions).
+ * The LED mapping rows must be in the lower half of the SRAM (0x00-0x7f) to be
+ * reachable by the map_* instructions, so they are placed before the engine
+ * programs:
+ * 0x00 engine 1 muxing info (1 page)
+ * 0x10 engine 2 muxing info (1 page)
+ * 0x20 engine 3 muxing info (1 page)
+ * 0x30 engine 1 program (4 pages)
+ * 0x70 engine 2 program (4 pages)
+ * 0xb0 engine 3 program (4 pages)
  */
 #define LP5569_PAGES_PER_ENGINE		4
-#define LP5569_PROG_PAGE_BASE		0
-#define LP5569_MUX_PAGE_BASE		12
+#define LP5569_MUX_PAGE_BASE		0
+#define LP5569_PROG_PAGE_BASE		3
 
 #define LP5569_REG_ENABLE		0x00
 #define   LP5569_ENABLE			BIT(6)
@@ -88,12 +92,12 @@
 #define   LP5569_LED_FAULT1		BIT(1)
 #define   LP5569_LED_FAULT0		BIT(0)
 
-#define LP5569_ENG1_PROG_ADDR		0x0
-#define LP5569_ENG2_PROG_ADDR		0x40
-#define LP5569_ENG3_PROG_ADDR		0x80
-#define LP5569_ENG1_MUX_ADDR		0xc0
-#define LP5569_ENG2_MUX_ADDR		0xd0
-#define LP5569_ENG3_MUX_ADDR		0xe0
+#define LP5569_ENG1_MUX_ADDR		0x00
+#define LP5569_ENG2_MUX_ADDR		0x10
+#define LP5569_ENG3_MUX_ADDR		0x20
+#define LP5569_ENG1_PROG_ADDR		0x30
+#define LP5569_ENG2_PROG_ADDR		0x70
+#define LP5569_ENG3_PROG_ADDR		0xb0
 
 #define LP5569_STARTUP_SLEEP		500
 
@@ -119,11 +123,22 @@ static int lp5569_init_program_engine(struct lp55xx_chip *chip)
 	int j;
 	int ret;
 	u8 status;
-	/* Precompiled pattern per ENGINE setting LED MUX start and stop addresses */
+	/*
+	 * Precompiled program run by each ENGINE, setting the start and end of
+	 * the LED mapping table to the row of the engine (bit 7 of the operand
+	 * selects load_end instead of map_start):
+	 *   map_start MUX_ADDR
+	 *   load_end  MUX_ADDR
+	 *   map_next
+	 *   end
+	 */
 	static const u8 pattern[][LP55xx_BYTES_PER_PAGE] =  {
-		{ 0x9c, LP5569_ENG1_MUX_ADDR, 0x9c, 0xb0, 0x9d, 0x80, 0xd8, 0x00, 0},
-		{ 0x9c, LP5569_ENG2_MUX_ADDR, 0x9c, 0xc0, 0x9d, 0x80, 0xd8, 0x00, 0},
-		{ 0x9c, LP5569_ENG3_MUX_ADDR, 0x9c, 0xd0, 0x9d, 0x80, 0xd8, 0x00, 0},
+		{ 0x9c, LP5569_ENG1_MUX_ADDR, 0x9c, 0x80 | LP5569_ENG1_MUX_ADDR,
+		  0x9d, 0x80, 0xd8, 0x00, 0},
+		{ 0x9c, LP5569_ENG2_MUX_ADDR, 0x9c, 0x80 | LP5569_ENG2_MUX_ADDR,
+		  0x9d, 0x80, 0xd8, 0x00, 0},
+		{ 0x9c, LP5569_ENG3_MUX_ADDR, 0x9c, 0x80 | LP5569_ENG3_MUX_ADDR,
+		  0x9d, 0x80, 0xd8, 0x00, 0},
 	};
 
 	/* Setup each ENGINE program start address */

-- 
2.56.0

[PATCH 3/3] Documentation: leds: lp55xx: Add an LP5569 example

[Arthur Crepin Leblond]

Add an example for the LP5569 that drives three LED channels at the
same time.

Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>
---
 Documentation/leds/leds-lp55xx.rst | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/leds/leds-lp55xx.rst b/Documentation/leds/leds-lp55xx.rst
index f60c7ec39816..549bdb0c890f 100644
--- a/Documentation/leds/leds-lp55xx.rst
+++ b/Documentation/leds/leds-lp55xx.rst
@@ -161,6 +161,30 @@ Ex 2) Engine 2 and 3 are used at the same time::
 	sleep 1
 	echo 1 > /sys/class/leds/d1/device/run_engine
 
+Here is another example for LP5569.
+
+Three engines run at the same time on different channels:
+
+  - channel 0 blinking every ~1 second
+  - channel 4 pulsing from 0 to 255
+  - channel 8 blinking every ~500ms
+
+Load a program and select the LED(s) of each engine, then run them::
+
+	cd /sys/bus/i2c/devices/xxxx
+	echo "load" > engine1_mode
+	echo "load" > engine2_mode
+	echo "load" > engine3_mode
+	echo "100000000" > engine1_leds
+	echo "000010000" > engine2_leds
+	echo "000000001" > engine3_leds
+	echo "9D8040FF7E007E0040007E007E00A001C000" > engine1_load
+	echo "9D80400010FF11FFA002C000" > engine2_load
+	echo "9D8040FF7E0040007E00A001C000" > engine3_load
+	echo "run" > engine1_mode
+	echo "run" > engine2_mode
+	echo "run" > engine3_mode
+
 ( 'run_engine' and 'firmware_cb' )
 
 The sequence of running the program data is common.

-- 
2.56.0

Re: [PATCH 1/3] leds: lp55xx: Add program and mux base pages to the device config

[sashiko-bot]

> Add program and mux base page to struct lp55xx_device_config to
> set the first page of the engine programs and of the LED mapping table.
> 
> Until now the mapping tables were placed at the end of the programs.
> On the LP5569 this puts them in the upper half of the SRAM, while the
> map_* instructions can only address words 0-127.
> This change allows placing the mapping tables before the programs instead.
> 
> All the chips set the values matching their current layout, so their
> behavior is unchanged:
>   - LP5523 has the 3 engine programs on the first 3 pages and their
>     LED mapping tables on the next 3 pages.
>   - LP5569 has the 3 engine programs of 4 pages each, followed by the
>     LED mapping tables.
>   - LP8501 stores its program at address 0.
> 
> Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-lp55xx-v1-0-6eaaf530115c@marmottus.net?part=1

Re: [PATCH 3/3] Documentation: leds: lp55xx: Add an LP5569 example

[sashiko-bot]

> Add an example for the LP5569 that drives three LED channels at the
> same time.
> 
> Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-lp55xx-v1-0-6eaaf530115c@marmottus.net?part=3

Re: [PATCH 2/3] leds: leds-lp5569: Place the LED mapping table in the lower half of the SRAM

[sashiko-bot]

> The LED mapping instructions (map_start, load_end, map_addr, ...) take
> a 7-bits absolute SRAM address, so the mapping table must be in the
> lower half of the SRAM (0-127).
> The driver placed the muxing info after the engine programs, which
> these instructions cannot reach.
> 
> Place the mapping tables in pages 0-2 and the engine programs after
> them.
> 
> This changes the SRAM layout seen by userspace:
> 
>   - The program of engines 1/2/3 now starts at address 0x30/0x70/0xb0
>     instead of 0x00/0x40/0x80 (ENGINEx_PROG_START).
>   - The mapping of engines 1/2/3 is at address 0x00/0x10/0x20.
> 
> Programs loaded through engineN_load or the firmware interface are not
> affected, since they are written relative to the start of the engine.
> Programs that use absolute SRAM addresses (map_start, load_end,
> load_addr, map_addr) must be updated to the new layout.
> 
> Fixes: 30c6743cc89c ("leds: leds-lp5569: Add support for Texas Instruments LP5569")
> Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-lp55xx-v1-0-6eaaf530115c@marmottus.net?part=2