Skip to contentSkip to conversation

leds: leds-lp55xx: overhaul driver

PATCH Applied Christian Marangi 3 participants 29 messages Jun 20, 2024 – Jun 26, 2024

Subsystems (2)

v7 of 8 Applied Latest is v8 →

lore
Share & tools
lore
#0 lore raw

v7 cover leds: leds-lp55xx: overhaul driver

This long series is (as requested) a big overhaul of the lp55xx based LED driver.

As notice for these kind of LED chip there was the bad habit of copy the old driver and just modify it enough to make it work with the new model. Till v4 I was also doing the same by following the pattern and the code format of previous driver.

Since Lee didn't like this, here is the BIG series that generalize pretty much anything in the 4 model currently supported.

Indeed, although the LED chip have fundamental difference (page support), things can be generalized and produce slimmer drivers by putting everything in the lp55xx-common shared module.

This result in the new model lp5569 being very small with only the selftest portion to be custom.

Lee also wasn't clear by the meaning of ENGINE in these LED driver, so here some simple explaination. This is very common on these TI LED chip. The ENGINE (there are always 3) is just some kind of processor that execute a program (precompiled code ASM like) loaded in the SRAM. Sysfs is used to load the pattern, and to start and stop the engine.

These pattern can do all kind of complex thing with LEDs. Old LED chip had 32bytes of space for the pattern but newer one (like lp5569) have pages and if correctly configured can have massive pattern. These pattern can do all kind of magic like loops that make the LED pulse, change color and all kind of stuff.

(For Lee, sorry if you will have to repeat some review that I might
have missed in the lp5569 driver)

Changes v7:
- Add Suggested-by tag
- Fix checkpatch error for complex macro (rework define)
- Add missing values for fader conversion
- Align some function with redundant new line
- Capitalize every commit title
Changes v6:
- Fix compilation warning for ret unused in read_poll_timeout (no idea why this is flagged only on some particular arch...)
- Fix missing bitfield.h in lp55x-common.c (again it seems this header gets included in the flow if the arch use them or not...)
Changes v5:
- Big generalization patch
- Rework lp5569 driver with new generalized functions
- Drop all copyright header in lp5569 as the driver got reworked entirely and it's not based on previous one anymore.
Changes v4:
- Fix reported buffer overflow due to a copypaste error
- Add comments to describe fw size logic
Changes v3:
- Add ACK tag to DT patch
- Enlarge and support program size up to 128bytes Changes v2:
- Add ACK tag to DT patch
- Fix compilation error with target that doesn't include bitfield.h

Christian Marangi (20):
dt-bindings: leds-lp55xx: Limit pwr-sel property to ti,lp8501
dt-bindings: leds-lp55xx: Add new ti,lp5569 compatible
leds: leds-lp55xx: Generalize stop_all_engine OP
leds: leds-lp55xx: Generalize probe/remove functions
leds: leds-lp55xx: Generalize load_engine function
leds: leds-lp55xx: Generalize load_engine_and_select_page function
leds: leds-lp55xx: Generalize run_engine function
leds: leds-lp55xx: Generalize update_program_memory function
leds: leds-lp55xx: Generalize firmware_loaded function
leds: leds-lp55xx: Generalize led_brightness function
leds: leds-lp55xx: Generalize multicolor_brightness function
leds: leds-lp55xx: Generalize set_led_current function
leds: leds-lp55xx: Generalize turn_off_channels function
leds: leds-lp55xx: Generalize stop_engine function
leds: leds-lp55xx: Generalize sysfs engine_load and engine_mode
leds: leds-lp55xx: Generalize sysfs engine_leds
leds: leds-lp55xx: Generalize sysfs master_fader
leds: leds-lp55xx: Support ENGINE program up to 128 bytes
leds: leds-lp55xx: Drop deprecated defines
leds: leds-lp5569: Add support for Texas Instruments LP5569

 .../devicetree/bindings/leds/leds-lp55xx.yaml |  11 +
 drivers/leds/Kconfig                          |  16 +-
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-lp5521.c                    | 405 +---------
 drivers/leds/leds-lp5523.c                    | 734 ++---------------
 drivers/leds/leds-lp5562.c                    | 261 +-----
 drivers/leds/leds-lp5569.c                    | 544 +++++++++++++
 drivers/leds/leds-lp55xx-common.c             | 743 +++++++++++++++++-
 drivers/leds/leds-lp55xx-common.h             | 163 ++--
 drivers/leds/leds-lp8501.c                    | 313 +-------
 10 files changed, 1540 insertions(+), 1651 deletions(-)
 create mode 100644 drivers/leds/leds-lp5569.c
Signature · 1 line
2.45.1
#1 lore raw

PATCH v7 1/20 dt-bindings: leds-lp55xx: Limit pwr-sel property to ti,lp8501

pwr-sel property is specific to ti,lp8501, make it conditional of the related compatible.

---
 .../devicetree/bindings/leds/leds-lp55xx.yaml          | 10 ++++++++++
 1 file changed, 10 insertions(+)
Documentation/devicetree/bindings/leds/leds-lp55xx.yaml +10 -0
index e9d4514d0166..77828dedbb9f 100644
@@ -151,6 +151,16 @@ patternProperties:
         $ref: /schemas/types.yaml#/definitions/string
         description: name of channel
 
+if:
+  not:
+    properties:
+      compatible:
+        contains:
+          const: ti,lp8501
+then:
+  properties:
+    pwr-sel: false
+
 required:
   - compatible
   - reg
Signature · 1 line
2.45.1
#2 lore raw

PATCH v7 2/20 dt-bindings: leds-lp55xx: Add new ti,lp5569 compatible

In reply to Christian Marangi

Add new ti,lp5569 compatible, this is similar to national,lp5523 with slight change to reg order and reg type and advanced way for LED fault.

---
 Documentation/devicetree/bindings/leds/leds-lp55xx.yaml | 1 +
 1 file changed, 1 insertion(+)
Documentation/devicetree/bindings/leds/leds-lp55xx.yaml +1 -0
index 77828dedbb9f..fe8aaecf3010 100644
@@ -28,6 +28,7 @@ properties:
       - national,lp5523
       - ti,lp55231
       - ti,lp5562
+      - ti,lp5569
       - ti,lp8501
 
   reg:
Signature · 1 line
2.45.1
#3 lore raw

PATCH v7 3/20 leds: leds-lp55xx: Generalize stop_all_engine OP

In reply to Christian Marangi

In all the lp55xx based driver, we have a similar implementation of the stop_all_engine function with the only difference of the required sleep for the OP MODE change.

The main difference is legacy LEDs require a min of 152 us while new one use a generic 1-2ms. The new one use a 1-2ms sleep as suggested in the datasheet IN ALTERNATIVE to a much more robust approach by using the newly introduced ENGINE_BUSY bit in the STATUS reg.

To better handle sleep after OP MODE change, add support for polling the ENGINE_BUSY bit and use the legacy sleep for old LEDs.

With this change, stop_all_engine can be generalized and moved to lp55xx-common.

To make more clear the double usage of lp55xx_reg, define a union for additional scope of mask and shift.

Update all lp55xx based driver to use the new generalized function and define the required bits in the device_config struct.

---
 drivers/leds/leds-lp5521.c        | 11 ++++------
 drivers/leds/leds-lp5523.c        | 20 ++++++++++-------
 drivers/leds/leds-lp5562.c        | 15 ++++++-------
 drivers/leds/leds-lp55xx-common.c | 36 +++++++++++++++++++++++++++++++
 drivers/leds/leds-lp55xx-common.h | 16 ++++++++++++--
 drivers/leds/leds-lp8501.c        | 20 ++++++++++-------
 6 files changed, 84 insertions(+), 34 deletions(-)
drivers/leds/leds-lp5521.c +4 -7
index f9c8b568b652..15ef5ae58623 100644
@@ -135,12 +135,6 @@ static void lp5521_load_engine(struct lp55xx_chip *chip)
 	lp5521_wait_opmode_done();
 }
 
-static void lp5521_stop_all_engines(struct lp55xx_chip *chip)
-{
-	lp55xx_write(chip, LP5521_REG_OP_MODE, 0);
-	lp5521_wait_opmode_done();
-}
-
 static void lp5521_stop_engine(struct lp55xx_chip *chip)
 {
 	enum lp55xx_engine_index idx = chip->engine_idx;
@@ -499,6 +493,9 @@ static const struct attribute_group lp5521_group = {
 
 /* Chip specific configurations */
 static struct lp55xx_device_config lp5521_cfg = {
+	.reg_op_mode = {
+		.addr = LP5521_REG_OP_MODE,
+	},
 	.reset = {
 		.addr = LP5521_REG_RESET,
 		.val  = LP5521_RESET,
@@ -585,7 +582,7 @@ static void lp5521_remove(struct i2c_client *client)
 	struct lp55xx_led *led = i2c_get_clientdata(client);
 	struct lp55xx_chip *chip = led->chip;
 
-	lp5521_stop_all_engines(chip);
+	lp55xx_stop_all_engine(chip);
 	lp55xx_unregister_sysfs(chip);
 	lp55xx_deinit_device(chip);
 }
drivers/leds/leds-lp5523.c +12 -8
index 38de853f9939..79931555eddd 100644
@@ -41,7 +41,10 @@
 #define LP5523_REG_LED_PWM_BASE		0x16
 #define LP5523_REG_LED_CURRENT_BASE	0x26
 #define LP5523_REG_CONFIG		0x36
+
 #define LP5523_REG_STATUS		0x3A
+#define LP5523_ENGINE_BUSY		BIT(4)
+
 #define LP5523_REG_RESET		0x3D
 #define LP5523_REG_LED_TEST_CTRL	0x41
 #define LP5523_REG_LED_TEST_ADC		0x42
@@ -190,12 +193,6 @@ static void lp5523_load_engine_and_select_page(struct lp55xx_chip *chip)
 	lp55xx_write(chip, LP5523_REG_PROG_PAGE_SEL, page_sel[idx]);
 }
 
-static void lp5523_stop_all_engines(struct lp55xx_chip *chip)
-{
-	lp55xx_write(chip, LP5523_REG_OP_MODE, 0);
-	lp5523_wait_opmode_done();
-}
-
 static void lp5523_stop_engine(struct lp55xx_chip *chip)
 {
 	enum lp55xx_engine_index idx = chip->engine_idx;
@@ -322,7 +319,7 @@ static int lp5523_init_program_engine(struct lp55xx_chip *chip)
 	}
 
 out:
-	lp5523_stop_all_engines(chip);
+	lp55xx_stop_all_engine(chip);
 	return ret;
 }
 
@@ -873,6 +870,13 @@ static const struct attribute_group lp5523_group = {
 
 /* Chip specific configurations */
 static struct lp55xx_device_config lp5523_cfg = {
+	.reg_op_mode = {
+		.addr = LP5523_REG_OP_MODE,
+	},
+	.engine_busy = {
+		.addr = LP5523_REG_STATUS,
+		.mask  = LP5523_ENGINE_BUSY,
+	},
 	.reset = {
 		.addr = LP5523_REG_RESET,
 		.val  = LP5523_RESET,
@@ -959,7 +963,7 @@ static void lp5523_remove(struct i2c_client *client)
 	struct lp55xx_led *led = i2c_get_clientdata(client);
 	struct lp55xx_chip *chip = led->chip;
 
-	lp5523_stop_all_engines(chip);
+	lp55xx_stop_all_engine(chip);
 	lp55xx_unregister_sysfs(chip);
 	lp55xx_deinit_device(chip);
 }
drivers/leds/leds-lp5562.c +6 -9
index 39db9aeb67c5..28469bd2c75b 100644
@@ -144,12 +144,6 @@ static void lp5562_load_engine(struct lp55xx_chip *chip)
 	lp5562_wait_opmode_done();
 }
 
-static void lp5562_stop_engine(struct lp55xx_chip *chip)
-{
-	lp55xx_write(chip, LP5562_REG_OP_MODE, LP5562_CMD_DISABLE);
-	lp5562_wait_opmode_done();
-}
-
 static void lp5562_run_engine(struct lp55xx_chip *chip, bool start)
 {
 	int ret;
@@ -160,7 +154,7 @@ static void lp5562_run_engine(struct lp55xx_chip *chip, bool start)
 	if (!start) {
 		lp55xx_write(chip, LP5562_REG_ENABLE, LP5562_ENABLE_DEFAULT);
 		lp5562_wait_enable_done();
-		lp5562_stop_engine(chip);
+		lp55xx_stop_all_engine(chip);
 		lp55xx_write(chip, LP5562_REG_ENG_SEL, LP5562_ENG_SEL_PWM);
 		lp55xx_write(chip, LP5562_REG_OP_MODE, LP5562_CMD_DIRECT);
 		lp5562_wait_opmode_done();
@@ -369,7 +363,7 @@ static int lp5562_run_predef_led_pattern(struct lp55xx_chip *chip, int mode)
 		return -EINVAL;
 	}
 
-	lp5562_stop_engine(chip);
+	lp55xx_stop_all_engine(chip);
 
 	/* Set LED map as RGB */
 	lp55xx_write(chip, LP5562_REG_ENG_SEL, LP5562_ENG_SEL_RGB);
@@ -495,6 +489,9 @@ static const struct attribute_group lp5562_group = {
 /* Chip specific configurations */
 static struct lp55xx_device_config lp5562_cfg = {
 	.max_channel  = LP5562_MAX_LEDS,
+	.reg_op_mode = {
+		.addr = LP5562_REG_OP_MODE,
+	},
 	.reset = {
 		.addr = LP5562_REG_RESET,
 		.val  = LP5562_RESET,
@@ -577,7 +574,7 @@ static void lp5562_remove(struct i2c_client *client)
 	struct lp55xx_led *led = i2c_get_clientdata(client);
 	struct lp55xx_chip *chip = led->chip;
 
-	lp5562_stop_engine(chip);
+	lp55xx_stop_all_engine(chip);
 
 	lp55xx_unregister_sysfs(chip);
 	lp55xx_deinit_device(chip);
drivers/leds/leds-lp55xx-common.c +36 -0
index 8e7074f0fee0..2cbc5b302fd4 100644
@@ -13,6 +13,7 @@
 #include <linux/delay.h>
 #include <linux/firmware.h>
 #include <linux/i2c.h>
+#include <linux/iopoll.h>
 #include <linux/leds.h>
 #include <linux/module.h>
 #include <linux/platform_data/leds-lp55xx.h>
@@ -22,6 +23,12 @@
 
 #include "leds-lp55xx-common.h"
 
+/* OP MODE require at least 153 us to clear regs */
+#define LP55XX_CMD_SLEEP		200
+
+/* Program Commands */
+#define LP55xx_MODE_DISABLE_ALL_ENG	0x0
+
 /* External clock rate */
 #define LP55XX_CLK_32K			32768
 
@@ -40,6 +47,35 @@ static struct lp55xx_led *mcled_cdev_to_led(struct led_classdev_mc *mc_cdev)
 	return container_of(mc_cdev, struct lp55xx_led, mc_cdev);
 }
 
+static void lp55xx_wait_opmode_done(struct lp55xx_chip *chip)
+{
+	struct lp55xx_device_config *cfg = chip->cfg;
+	int __always_unused ret;
+	u8 val;
+
+	/*
+	 * Recent chip supports BUSY bit for engine.
+	 * Check support by checking if val is not 0.
+	 * For legacy device, sleep at least 153 us.
+	 */
+	if (cfg->engine_busy.val) {
+		read_poll_timeout(lp55xx_read, ret, !(val & cfg->engine_busy.mask),
+				  LP55XX_CMD_SLEEP, LP55XX_CMD_SLEEP * 10, false,
+				  chip, cfg->engine_busy.addr, &val);
+	} else {
+		usleep_range(LP55XX_CMD_SLEEP, LP55XX_CMD_SLEEP * 2);
+	}
+}
+
+void lp55xx_stop_all_engine(struct lp55xx_chip *chip)
+{
+	struct lp55xx_device_config *cfg = chip->cfg;
+
+	lp55xx_write(chip, cfg->reg_op_mode.addr, LP55xx_MODE_DISABLE_ALL_ENG);
+	lp55xx_wait_opmode_done(chip);
+}
+EXPORT_SYMBOL_GPL(lp55xx_stop_all_engine);
+
 static void lp55xx_reset_device(struct lp55xx_chip *chip)
 {
 	struct lp55xx_device_config *cfg = chip->cfg;
drivers/leds/leds-lp55xx-common.h +14 -2
index 2f38c5b33830..c7c8a77ddb1a 100644
@@ -81,15 +81,22 @@ struct lp55xx_chip;
 /*
  * struct lp55xx_reg
  * @addr : Register address
- * @val  : Register value
+ * @val  : Register value (can also used as mask or shift)
  */
 struct lp55xx_reg {
 	u8 addr;
-	u8 val;
+	union {
+		u8 val;
+		u8 mask;
+		u8 shift;
+	};
 };
 
 /*
  * struct lp55xx_device_config
+ * @reg_op_mode        : Chip specific OP MODE reg addr
+ * @engine_busy        : Chip specific engine busy
+ *			 (if not supported 153 us sleep)
  * @reset              : Chip specific reset command
  * @enable             : Chip specific enable command
  * @max_channel        : Maximum number of channels
@@ -102,6 +109,8 @@ struct lp55xx_reg {
  * @dev_attr_group     : Device specific attributes
  */
 struct lp55xx_device_config {
+	const struct lp55xx_reg reg_op_mode; /* addr, shift */
+	const struct lp55xx_reg engine_busy; /* addr, mask */
 	const struct lp55xx_reg reset;
 	const struct lp55xx_reg enable;
 	const int max_channel;
@@ -191,6 +200,9 @@ extern int lp55xx_update_bits(struct lp55xx_chip *chip, u8 reg,
 /* external clock detection */
 extern bool lp55xx_is_extclk_used(struct lp55xx_chip *chip);
 
+/* common chip functions */
+extern void lp55xx_stop_all_engine(struct lp55xx_chip *chip);
+
 /* common device init/deinit functions */
 extern int lp55xx_init_device(struct lp55xx_chip *chip);
 extern void lp55xx_deinit_device(struct lp55xx_chip *chip);
drivers/leds/leds-lp8501.c +12 -8
index ac50aa88939a..57e184dda490 100644
@@ -58,6 +58,9 @@
 #define LP8501_INT_CLK			BIT(0)
 #define LP8501_DEFAULT_CFG (LP8501_PWM_PSAVE | LP8501_AUTO_INC | LP8501_PWR_SAVE)
 
+#define LP8501_REG_STATUS		0x3A
+#define LP8501_ENGINE_BUSY		BIT(4)
+
 #define LP8501_REG_RESET		0x3D
 #define LP8501_RESET			0xFF
 
@@ -141,12 +144,6 @@ static void lp8501_load_engine(struct lp55xx_chip *chip)
 	lp55xx_write(chip, LP8501_REG_PROG_PAGE_SEL, page_sel[idx]);
 }
 
-static void lp8501_stop_engine(struct lp55xx_chip *chip)
-{
-	lp55xx_write(chip, LP8501_REG_OP_MODE, 0);
-	lp8501_wait_opmode_done();
-}
-
 static void lp8501_turn_off_channels(struct lp55xx_chip *chip)
 {
 	int i;
@@ -163,7 +160,7 @@ static void lp8501_run_engine(struct lp55xx_chip *chip, bool start)
 
 	/* stop engine */
 	if (!start) {
-		lp8501_stop_engine(chip);
+		lp55xx_stop_all_engine(chip);
 		lp8501_turn_off_channels(chip);
 		return;
 	}
@@ -285,6 +282,13 @@ static int lp8501_led_brightness(struct lp55xx_led *led)
 
 /* Chip specific configurations */
 static struct lp55xx_device_config lp8501_cfg = {
+	.reg_op_mode = {
+		.addr = LP8501_REG_OP_MODE,
+	},
+	.engine_busy = {
+		.addr = LP8501_REG_STATUS,
+		.maks = LP8501_ENGINE_BUSY,
+	},
 	.reset = {
 		.addr = LP8501_REG_RESET,
 		.val  = LP8501_RESET,
@@ -369,7 +373,7 @@ static void lp8501_remove(struct i2c_client *client)
 	struct lp55xx_led *led = i2c_get_clientdata(client);
 	struct lp55xx_chip *chip = led->chip;
 
-	lp8501_stop_engine(chip);
+	lp55xx_stop_all_engine(chip);
 	lp55xx_unregister_sysfs(chip);
 	lp55xx_deinit_device(chip);
 }
Signature · 1 line
2.45.1
#4 lore raw

PATCH v7 4/20 leds: leds-lp55xx: Generalize probe/remove functions

In reply to Christian Marangi

Now that stop_all_engine is generalized, probe and remove function are the same across every lp55xx based LED driver and can be generalized.

To permit to use a common probe, make use of the OF match_data and i2c driver_data value to store the device_config struct specific for the LED.

Also drop the now unused exported symbol in lp55xx-common and make them static.

Update any lp55xx based LED driver to use the new generic probe/remove.

---
 drivers/leds/leds-lp5521.c        |  81 +----------------
 drivers/leds/leds-lp5523.c        |  85 ++----------------
 drivers/leds/leds-lp5562.c        |  80 +----------------
 drivers/leds/leds-lp55xx-common.c | 141 ++++++++++++++++++++++--------
 drivers/leds/leds-lp55xx-common.h |  21 +----
 drivers/leds/leds-lp8501.c        |  81 +----------------
 6 files changed, 127 insertions(+), 362 deletions(-)
drivers/leds/leds-lp5521.c +4 -77
index 15ef5ae58623..5015f385cc17 100644
@@ -514,87 +514,14 @@ static struct lp55xx_device_config lp5521_cfg = {
 	.dev_attr_group     = &lp5521_group,
 };
 
-static int lp5521_probe(struct i2c_client *client)
-{
-	const struct i2c_device_id *id = i2c_client_get_device_id(client);
-	int ret;
-	struct lp55xx_chip *chip;
-	struct lp55xx_led *led;
-	struct lp55xx_platform_data *pdata = dev_get_platdata(&client->dev);
-	struct device_node *np = dev_of_node(&client->dev);
-
-	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
-	if (!chip)
-		return -ENOMEM;
-
-	chip->cfg = &lp5521_cfg;
-
-	if (!pdata) {
-		if (np) {
-			pdata = lp55xx_of_populate_pdata(&client->dev, np,
-							 chip);
-			if (IS_ERR(pdata))
-				return PTR_ERR(pdata);
-		} else {
-			dev_err(&client->dev, "no platform data\n");
-			return -EINVAL;
-		}
-	}
-
-	led = devm_kcalloc(&client->dev,
-			pdata->num_channels, sizeof(*led), GFP_KERNEL);
-	if (!led)
-		return -ENOMEM;
-
-	chip->cl = client;
-	chip->pdata = pdata;
-
-	mutex_init(&chip->lock);
-
-	i2c_set_clientdata(client, led);
-
-	ret = lp55xx_init_device(chip);
-	if (ret)
-		goto err_init;
-
-	dev_info(&client->dev, "%s programmable led chip found\n", id->name);
-
-	ret = lp55xx_register_leds(led, chip);
-	if (ret)
-		goto err_out;
-
-	ret = lp55xx_register_sysfs(chip);
-	if (ret) {
-		dev_err(&client->dev, "registering sysfs failed\n");
-		goto err_out;
-	}
-
-	return 0;
-
-err_out:
-	lp55xx_deinit_device(chip);
-err_init:
-	return ret;
-}
-
-static void lp5521_remove(struct i2c_client *client)
-{
-	struct lp55xx_led *led = i2c_get_clientdata(client);
-	struct lp55xx_chip *chip = led->chip;
-
-	lp55xx_stop_all_engine(chip);
-	lp55xx_unregister_sysfs(chip);
-	lp55xx_deinit_device(chip);
-}
-
 static const struct i2c_device_id lp5521_id[] = {
-	{ "lp5521", 0 }, /* Three channel chip */
+	{ "lp5521", .driver_data = (kernel_ulong_t)&lp5521_cfg, }, /* Three channel chip */
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, lp5521_id);
 
 static const struct of_device_id of_lp5521_leds_match[] = {
-	{ .compatible = "national,lp5521", },
+	{ .compatible = "national,lp5521", .data = &lp5521_cfg, },
 	{},
 };
 
@@ -605,8 +532,8 @@ static struct i2c_driver lp5521_driver = {
 		.name	= "lp5521",
 		.of_match_table = of_lp5521_leds_match,
 	},
-	.probe		= lp5521_probe,
-	.remove		= lp5521_remove,
+	.probe		= lp55xx_probe,
+	.remove		= lp55xx_remove,
 	.id_table	= lp5521_id,
 };
 
drivers/leds/leds-lp5523.c +6 -79

104 lines not on this page · raw message

drivers/leds/leds-lp5562.c +4 -76

98 lines not on this page · raw message

drivers/leds/leds-lp55xx-common.c +105 -36

242 lines not on this page · raw message

drivers/leds/leds-lp55xx-common.h +4 -17
index c7c8a77ddb1a..26a724acac16 100644
@@ -164,7 +164,7 @@ struct lp55xx_chip {
 	struct lp55xx_platform_data *pdata;
 	struct mutex lock;	/* lock for user-space interface */
 	int num_leds;
-	struct lp55xx_device_config *cfg;
+	const struct lp55xx_device_config *cfg;
 	enum lp55xx_engine_index engine_idx;
 	struct lp55xx_engine engines[LP55XX_ENGINE_MAX];
 	const struct firmware *fw;
@@ -203,21 +203,8 @@ extern bool lp55xx_is_extclk_used(struct lp55xx_chip *chip);
 /* common chip functions */
 extern void lp55xx_stop_all_engine(struct lp55xx_chip *chip);
 
-/* common device init/deinit functions */
-extern int lp55xx_init_device(struct lp55xx_chip *chip);
-extern void lp55xx_deinit_device(struct lp55xx_chip *chip);
-
-/* common LED class device functions */
-extern int lp55xx_register_leds(struct lp55xx_led *led,
-				struct lp55xx_chip *chip);
-
-/* common device attributes functions */
-extern int lp55xx_register_sysfs(struct lp55xx_chip *chip);
-extern void lp55xx_unregister_sysfs(struct lp55xx_chip *chip);
-
-/* common device tree population function */
-extern struct lp55xx_platform_data
-*lp55xx_of_populate_pdata(struct device *dev, struct device_node *np,
-			  struct lp55xx_chip *chip);
+/* common probe/remove function */
+extern int lp55xx_probe(struct i2c_client *client);
+extern void lp55xx_remove(struct i2c_client *client);
 
 #endif /* _LEDS_LP55XX_COMMON_H */
drivers/leds/leds-lp8501.c +4 -77

99 lines not on this page · raw message

Signature · 1 line
2.45.1
#5 lore raw

PATCH v7 5/20 leds: leds-lp55xx: Generalize load_engine function

In reply to Christian Marangi

LED driver based on lp55xx have all a very similar implementation for load_engine function. Move the function to lp55xx-common and rework the define to be more dynamic instead of having to declare a temp array for them.

Engine mask are the same for every LED based on lp55xx.

---
 drivers/leds/leds-lp5521.c        | 26 +++---------------------
 drivers/leds/leds-lp5523.c        | 26 +++---------------------
 drivers/leds/leds-lp5562.c        | 24 ++--------------------
 drivers/leds/leds-lp55xx-common.c | 33 ++++++++++++++++++++++++++++++-
 drivers/leds/leds-lp55xx-common.h |  1 +
 drivers/leds/leds-lp8501.c        | 17 ++--------------
 6 files changed, 43 insertions(+), 84 deletions(-)
drivers/leds/leds-lp5521.c +3 -23
index 5015f385cc17..08db470fff6c 100644
@@ -115,26 +115,6 @@ static void lp5521_set_led_current(struct lp55xx_led *led, u8 led_current)
 		led_current);
 }
 
-static void lp5521_load_engine(struct lp55xx_chip *chip)
-{
-	enum lp55xx_engine_index idx = chip->engine_idx;
-	static const u8 mask[] = {
-		[LP55XX_ENGINE_1] = LP5521_MODE_R_M,
-		[LP55XX_ENGINE_2] = LP5521_MODE_G_M,
-		[LP55XX_ENGINE_3] = LP5521_MODE_B_M,
-	};
-
-	static const u8 val[] = {
-		[LP55XX_ENGINE_1] = LP5521_LOAD_R,
-		[LP55XX_ENGINE_2] = LP5521_LOAD_G,
-		[LP55XX_ENGINE_3] = LP5521_LOAD_B,
-	};
-
-	lp55xx_update_bits(chip, LP5521_REG_OP_MODE, mask[idx], val[idx]);
-
-	lp5521_wait_opmode_done();
-}
-
 static void lp5521_stop_engine(struct lp55xx_chip *chip)
 {
 	enum lp55xx_engine_index idx = chip->engine_idx;
@@ -264,7 +244,7 @@ static void lp5521_firmware_loaded(struct lp55xx_chip *chip)
 	 *  2) write firmware data into program memory
 	 */
 
-	lp5521_load_engine(chip);
+	lp55xx_load_engine(chip);
 	lp5521_update_program_memory(chip, fw->data, fw->size);
 }
 
@@ -415,7 +395,7 @@ static ssize_t store_engine_mode(struct device *dev,
 		engine->mode = LP55XX_ENGINE_RUN;
 	} else if (!strncmp(buf, "load", 4)) {
 		lp5521_stop_engine(chip);
-		lp5521_load_engine(chip);
+		lp55xx_load_engine(chip);
 		engine->mode = LP55XX_ENGINE_LOAD;
 	} else if (!strncmp(buf, "disabled", 8)) {
 		lp5521_stop_engine(chip);
@@ -441,7 +421,7 @@ static ssize_t store_engine_load(struct device *dev,
 	mutex_lock(&chip->lock);
 
 	chip->engine_idx = nr;
-	lp5521_load_engine(chip);
+	lp55xx_load_engine(chip);
 	ret = lp5521_update_program_memory(chip, buf, len);
 
 	mutex_unlock(&chip->lock);
drivers/leds/leds-lp5523.c +3 -23
index bd0209e2ee42..086b4d8975a4 100644
@@ -159,26 +159,6 @@ static int lp5523_post_init_device(struct lp55xx_chip *chip)
 	return lp5523_init_program_engine(chip);
 }
 
-static void lp5523_load_engine(struct lp55xx_chip *chip)
-{
-	enum lp55xx_engine_index idx = chip->engine_idx;
-	static const u8 mask[] = {
-		[LP55XX_ENGINE_1] = LP5523_MODE_ENG1_M,
-		[LP55XX_ENGINE_2] = LP5523_MODE_ENG2_M,
-		[LP55XX_ENGINE_3] = LP5523_MODE_ENG3_M,
-	};
-
-	static const u8 val[] = {
-		[LP55XX_ENGINE_1] = LP5523_LOAD_ENG1,
-		[LP55XX_ENGINE_2] = LP5523_LOAD_ENG2,
-		[LP55XX_ENGINE_3] = LP5523_LOAD_ENG3,
-	};
-
-	lp55xx_update_bits(chip, LP5523_REG_OP_MODE, mask[idx], val[idx]);
-
-	lp5523_wait_opmode_done();
-}
-
 static void lp5523_load_engine_and_select_page(struct lp55xx_chip *chip)
 {
 	enum lp55xx_engine_index idx = chip->engine_idx;
@@ -188,7 +168,7 @@ static void lp5523_load_engine_and_select_page(struct lp55xx_chip *chip)
 		[LP55XX_ENGINE_3] = LP5523_PAGE_ENG3,
 	};
 
-	lp5523_load_engine(chip);
+	lp55xx_load_engine(chip);
 
 	lp55xx_write(chip, LP5523_REG_PROG_PAGE_SEL, page_sel[idx]);
 }
@@ -425,7 +405,7 @@ static ssize_t store_engine_mode(struct device *dev,
 		engine->mode = LP55XX_ENGINE_RUN;
 	} else if (!strncmp(buf, "load", 4)) {
 		lp5523_stop_engine(chip);
-		lp5523_load_engine(chip);
+		lp55xx_load_engine(chip);
 		engine->mode = LP55XX_ENGINE_LOAD;
 	} else if (!strncmp(buf, "disabled", 8)) {
 		lp5523_stop_engine(chip);
@@ -502,7 +482,7 @@ static int lp5523_load_mux(struct lp55xx_chip *chip, u16 mux, int nr)
 		[LP55XX_ENGINE_3] = LP5523_PAGE_MUX3,
 	};
 
-	lp5523_load_engine(chip);
+	lp55xx_load_engine(chip);
 
 	ret = lp55xx_write(chip, LP5523_REG_PROG_PAGE_SEL, mux_page[nr]);
 	if (ret)
drivers/leds/leds-lp5562.c +2 -22
index 65a6a05c3848..5e26a52f534f 100644
@@ -124,26 +124,6 @@ static void lp5562_set_led_current(struct lp55xx_led *led, u8 led_current)
 	lp55xx_write(led->chip, addr[led->chan_nr], led_current);
 }
 
-static void lp5562_load_engine(struct lp55xx_chip *chip)
-{
-	enum lp55xx_engine_index idx = chip->engine_idx;
-	static const u8 mask[] = {
-		[LP55XX_ENGINE_1] = LP5562_MODE_ENG1_M,
-		[LP55XX_ENGINE_2] = LP5562_MODE_ENG2_M,
-		[LP55XX_ENGINE_3] = LP5562_MODE_ENG3_M,
-	};
-
-	static const u8 val[] = {
-		[LP55XX_ENGINE_1] = LP5562_LOAD_ENG1,
-		[LP55XX_ENGINE_2] = LP5562_LOAD_ENG2,
-		[LP55XX_ENGINE_3] = LP5562_LOAD_ENG3,
-	};
-
-	lp55xx_update_bits(chip, LP5562_REG_OP_MODE, mask[idx], val[idx]);
-
-	lp5562_wait_opmode_done();
-}
-
 static void lp5562_run_engine(struct lp55xx_chip *chip, bool start)
 {
 	int ret;
@@ -270,7 +250,7 @@ static void lp5562_firmware_loaded(struct lp55xx_chip *chip)
 	 *  2) write firmware data into program memory
 	 */
 
-	lp5562_load_engine(chip);
+	lp55xx_load_engine(chip);
 	lp5562_update_firmware(chip, fw->data, fw->size);
 }
 
@@ -371,7 +351,7 @@ static int lp5562_run_predef_led_pattern(struct lp55xx_chip *chip, int mode)
 	/* Load engines */
 	for (i = LP55XX_ENGINE_1; i <= LP55XX_ENGINE_3; i++) {
 		chip->engine_idx = i;
-		lp5562_load_engine(chip);
+		lp55xx_load_engine(chip);
 	}
 
 	/* Clear program registers */
drivers/leds/leds-lp55xx-common.c +32 -1
index 2949ea56a170..3461158ad372 100644
@@ -9,6 +9,7 @@
  * Derived from leds-lp5521.c, leds-lp5523.c
  */
 
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/firmware.h>
@@ -26,8 +27,24 @@
 /* OP MODE require at least 153 us to clear regs */
 #define LP55XX_CMD_SLEEP		200
 
-/* Program Commands */
+/*
+ * Program Memory Operations
+ * Same Mask for each engine for both mode and exec
+ * ENG1        GENMASK(3, 2)
+ * ENG2        GENMASK(5, 4)
+ * ENG3        GENMASK(7, 6)
+ */
 #define LP55xx_MODE_DISABLE_ALL_ENG	0x0
+#define LP55xx_MODE_ENG_MASK           GENMASK(1, 0)
+#define   LP55xx_MODE_DISABLE_ENG      FIELD_PREP_CONST(LP55xx_MODE_ENG_MASK, 0x0)
+#define   LP55xx_MODE_LOAD_ENG         FIELD_PREP_CONST(LP55xx_MODE_ENG_MASK, 0x1)
+#define   LP55xx_MODE_RUN_ENG          FIELD_PREP_CONST(LP55xx_MODE_ENG_MASK, 0x2)
+#define   LP55xx_MODE_HALT_ENG         FIELD_PREP_CONST(LP55xx_MODE_ENG_MASK, 0x3)
+
+#define   LP55xx_MODE_ENGn_SHIFT(n, shift)	((shift) + (2 * (3 - (n))))
+#define   LP55xx_MODE_ENGn_MASK(n, shift)     (LP55xx_MODE_ENG_MASK << LP55xx_MODE_ENGn_SHIFT(n, shift))
+#define   LP55xx_MODE_ENGn_GET(n, mode, shift)        \
+	(((mode) >> LP55xx_MODE_ENGn_SHIFT(n, shift)) & LP55xx_MODE_ENG_MASK)
 
 /* External clock rate */
 #define LP55XX_CLK_32K			32768
@@ -76,6 +93,20 @@ void lp55xx_stop_all_engine(struct lp55xx_chip *chip)
 }
 EXPORT_SYMBOL_GPL(lp55xx_stop_all_engine);
 
+void lp55xx_load_engine(struct lp55xx_chip *chip)
+{
+	enum lp55xx_engine_index idx = chip->engine_idx;
+	const struct lp55xx_device_config *cfg = chip->cfg;
+	u8 mask, val;
+
+	mask = LP55xx_MODE_ENGn_MASK(idx, cfg->reg_op_mode.shift);
+	val = LP55xx_MODE_LOAD_ENG << LP55xx_MODE_ENGn_SHIFT(idx, cfg->reg_op_mode.shift);
+
+	lp55xx_update_bits(chip, cfg->reg_op_mode.addr, mask, val);
+	lp55xx_wait_opmode_done(chip);
+}
+EXPORT_SYMBOL_GPL(lp55xx_load_engine);
+
 static void lp55xx_reset_device(struct lp55xx_chip *chip)
 {
 	const struct lp55xx_device_config *cfg = chip->cfg;
drivers/leds/leds-lp55xx-common.h +1 -0
index 26a724acac16..bb146bcecdcf 100644
@@ -202,6 +202,7 @@ extern bool lp55xx_is_extclk_used(struct lp55xx_chip *chip);
 
 /* common chip functions */
 extern void lp55xx_stop_all_engine(struct lp55xx_chip *chip);
+extern void lp55xx_load_engine(struct lp55xx_chip *chip);
 
 /* common probe/remove function */
 extern int lp55xx_probe(struct i2c_client *client);
drivers/leds/leds-lp8501.c +2 -15
index d3c718bb8275..04173d6875af 100644
@@ -119,17 +119,6 @@ static int lp8501_post_init_device(struct lp55xx_chip *chip)
 static void lp8501_load_engine(struct lp55xx_chip *chip)
 {
 	enum lp55xx_engine_index idx = chip->engine_idx;
-	static const u8 mask[] = {
-		[LP55XX_ENGINE_1] = LP8501_MODE_ENG1_M,
-		[LP55XX_ENGINE_2] = LP8501_MODE_ENG2_M,
-		[LP55XX_ENGINE_3] = LP8501_MODE_ENG3_M,
-	};
-
-	static const u8 val[] = {
-		[LP55XX_ENGINE_1] = LP8501_LOAD_ENG1,
-		[LP55XX_ENGINE_2] = LP8501_LOAD_ENG2,
-		[LP55XX_ENGINE_3] = LP8501_LOAD_ENG3,
-	};
 
 	static const u8 page_sel[] = {
 		[LP55XX_ENGINE_1] = LP8501_PAGE_ENG1,
@@ -137,9 +126,7 @@ static void lp8501_load_engine(struct lp55xx_chip *chip)
 		[LP55XX_ENGINE_3] = LP8501_PAGE_ENG3,
 	};
 
-	lp55xx_update_bits(chip, LP8501_REG_OP_MODE, mask[idx], val[idx]);
-
-	lp8501_wait_opmode_done();
+	lp55xx_load_engine(chip);
 
 	lp55xx_write(chip, LP8501_REG_PROG_PAGE_SEL, page_sel[idx]);
 }
@@ -287,7 +274,7 @@ static struct lp55xx_device_config lp8501_cfg = {
 	},
 	.engine_busy = {
 		.addr = LP8501_REG_STATUS,
-		.maks = LP8501_ENGINE_BUSY,
+		.mask = LP8501_ENGINE_BUSY,
 	},
 	.reset = {
 		.addr = LP8501_REG_RESET,
Signature · 1 line
2.45.1
#6 lore raw

PATCH v7 6/20 leds: leds-lp55xx: Generalize load_engine_and_select_page function

In reply to Christian Marangi

Generalize load_engine_and_select_page by reworking the implementation and making it part of the generic load_engine function.

Add a new option in device_config, pages_per_engine used to define pages assigned to each engine. With this option set, it's assumed LED chip supports pages and load_engine will correctly setup the write page.

An equal amount of pages is assigned to each engine and they are assigned from page 0.

Update any lp55xx based LED driver to define the option and use the new function.

---
 drivers/leds/leds-lp5523.c        | 22 +++++-----------------
 drivers/leds/leds-lp55xx-common.c | 10 ++++++++++
 drivers/leds/leds-lp55xx-common.h |  3 +++
 drivers/leds/leds-lp8501.c        | 19 +++----------------
 4 files changed, 21 insertions(+), 33 deletions(-)
drivers/leds/leds-lp5523.c +5 -17
index 086b4d8975a4..8dabd5814110 100644
@@ -30,6 +30,7 @@
  * 0x40 engine 2 muxing info
  * 0x50 engine 3 muxing info
  */
+#define LP5523_PAGES_PER_ENGINE		1
 #define LP5523_MAX_LEDS			9
 
 /* Registers */
@@ -159,20 +160,6 @@ static int lp5523_post_init_device(struct lp55xx_chip *chip)
 	return lp5523_init_program_engine(chip);
 }
 
-static void lp5523_load_engine_and_select_page(struct lp55xx_chip *chip)
-{
-	enum lp55xx_engine_index idx = chip->engine_idx;
-	static const u8 page_sel[] = {
-		[LP55XX_ENGINE_1] = LP5523_PAGE_ENG1,
-		[LP55XX_ENGINE_2] = LP5523_PAGE_ENG2,
-		[LP55XX_ENGINE_3] = LP5523_PAGE_ENG3,
-	};
-
-	lp55xx_load_engine(chip);
-
-	lp55xx_write(chip, LP5523_REG_PROG_PAGE_SEL, page_sel[idx]);
-}
-
 static void lp5523_stop_engine(struct lp55xx_chip *chip)
 {
 	enum lp55xx_engine_index idx = chip->engine_idx;
@@ -272,7 +259,7 @@ static int lp5523_init_program_engine(struct lp55xx_chip *chip)
 	/* write LED MUX address space for each engine */
 	for (i = LP55XX_ENGINE_1; i <= LP55XX_ENGINE_3; i++) {
 		chip->engine_idx = i;
-		lp5523_load_engine_and_select_page(chip);
+		lp55xx_load_engine(chip);
 
 		for (j = 0; j < LP5523_PROGRAM_LENGTH; j++) {
 			ret = lp55xx_write(chip, LP5523_REG_PROG_MEM + j,
@@ -362,7 +349,7 @@ static void lp5523_firmware_loaded(struct lp55xx_chip *chip)
 	 *  2) write firmware data into program memory
 	 */
 
-	lp5523_load_engine_and_select_page(chip);
+	lp55xx_load_engine(chip);
 	lp5523_update_program_memory(chip, fw->data, fw->size);
 }
 
@@ -544,7 +531,7 @@ static ssize_t store_engine_load(struct device *dev,
 	mutex_lock(&chip->lock);
 
 	chip->engine_idx = nr;
-	lp5523_load_engine_and_select_page(chip);
+	lp55xx_load_engine(chip);
 	ret = lp5523_update_program_memory(chip, buf, len);
 
 	mutex_unlock(&chip->lock);
@@ -865,6 +852,7 @@ static struct lp55xx_device_config lp5523_cfg = {
 		.addr = LP5523_REG_ENABLE,
 		.val  = LP5523_ENABLE,
 	},
+	.pages_per_engine   = LP5523_PAGES_PER_ENGINE,
 	.max_channel  = LP5523_MAX_LEDS,
 	.post_init_device   = lp5523_post_init_device,
 	.brightness_fn      = lp5523_led_brightness,
drivers/leds/leds-lp55xx-common.c +10 -0
index 3461158ad372..38564b9cda0d 100644
@@ -46,6 +46,11 @@
 #define   LP55xx_MODE_ENGn_GET(n, mode, shift)        \
 	(((mode) >> LP55xx_MODE_ENGn_SHIFT(n, shift)) & LP55xx_MODE_ENG_MASK)
 
+/* 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))
+
 /* External clock rate */
 #define LP55XX_CLK_32K			32768
 
@@ -104,6 +109,11 @@ void lp55xx_load_engine(struct lp55xx_chip *chip)
 
 	lp55xx_update_bits(chip, cfg->reg_op_mode.addr, mask, val);
 	lp55xx_wait_opmode_done(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));
 }
 EXPORT_SYMBOL_GPL(lp55xx_load_engine);
 
drivers/leds/leds-lp55xx-common.h +3 -0
index bb146bcecdcf..015ac2ef4e4d 100644
@@ -99,6 +99,8 @@ struct lp55xx_reg {
  *			 (if not supported 153 us sleep)
  * @reset              : Chip specific reset command
  * @enable             : Chip specific enable command
+ * @pages_per_engine   : Assigned pages for each engine
+ *                       (if not set chip doesn't support pages)
  * @max_channel        : Maximum number of channels
  * @post_init_device   : Chip specific initialization code
  * @brightness_fn      : Brightness function
@@ -113,6 +115,7 @@ struct lp55xx_device_config {
 	const struct lp55xx_reg engine_busy; /* addr, mask */
 	const struct lp55xx_reg reset;
 	const struct lp55xx_reg enable;
+	const int pages_per_engine;
 	const int max_channel;
 
 	/* define if the device has specific initialization process */
drivers/leds/leds-lp8501.c +3 -16
index 04173d6875af..abe2c4b213d7 100644
@@ -21,6 +21,7 @@
 #include "leds-lp55xx-common.h"
 
 #define LP8501_PROGRAM_LENGTH		32
+#define LP8501_PAGES_PER_ENGINE		1
 #define LP8501_MAX_LEDS			9
 
 /* Registers */
@@ -116,21 +117,6 @@ static int lp8501_post_init_device(struct lp55xx_chip *chip)
 				LP8501_PWR_CONFIG_M, chip->pdata->pwr_sel);
 }
 
-static void lp8501_load_engine(struct lp55xx_chip *chip)
-{
-	enum lp55xx_engine_index idx = chip->engine_idx;
-
-	static const u8 page_sel[] = {
-		[LP55XX_ENGINE_1] = LP8501_PAGE_ENG1,
-		[LP55XX_ENGINE_2] = LP8501_PAGE_ENG2,
-		[LP55XX_ENGINE_3] = LP8501_PAGE_ENG3,
-	};
-
-	lp55xx_load_engine(chip);
-
-	lp55xx_write(chip, LP8501_REG_PROG_PAGE_SEL, page_sel[idx]);
-}
-
 static void lp8501_turn_off_channels(struct lp55xx_chip *chip)
 {
 	int i;
@@ -250,7 +236,7 @@ static void lp8501_firmware_loaded(struct lp55xx_chip *chip)
 	 *  2) write firmware data into program memory
 	 */
 
-	lp8501_load_engine(chip);
+	lp55xx_load_engine(chip);
 	lp8501_update_program_memory(chip, fw->data, fw->size);
 }
 
@@ -284,6 +270,7 @@ static struct lp55xx_device_config lp8501_cfg = {
 		.addr = LP8501_REG_ENABLE,
 		.val  = LP8501_ENABLE,
 	},
+	.pages_per_engine   = LP8501_PAGES_PER_ENGINE,
 	.max_channel  = LP8501_MAX_LEDS,
 	.post_init_device   = lp8501_post_init_device,
 	.brightness_fn      = lp8501_led_brightness,
Signature · 1 line
2.45.1
#7 lore raw

PATCH v7 7/20 leds: leds-lp55xx: Generalize run_engine function

In reply to Christian Marangi

Generalize run_engine function for lp55xx based LED driver. The logic is similar to every LED driver, rework it with more macro magic and account for LED model that might have OP MODE and EXEC at base offset in the reg.

Update any lp55xx based LED driver to use this generalized function and declare required bits.

---
 drivers/leds/leds-lp5521.c        | 42 +++++-------------------------
 drivers/leds/leds-lp5523.c        | 41 +++--------------------------
 drivers/leds/leds-lp5562.c        | 42 +++++-------------------------
 drivers/leds/leds-lp55xx-common.c | 43 +++++++++++++++++++++++++++++++
 drivers/leds/leds-lp55xx-common.h |  2 ++
 drivers/leds/leds-lp8501.c        | 41 +++--------------------------
 6 files changed, 65 insertions(+), 146 deletions(-)
drivers/leds/leds-lp5521.c +6 -36
index 08db470fff6c..0b9f99f4fff2 100644
@@ -132,8 +132,6 @@ static void lp5521_stop_engine(struct lp55xx_chip *chip)
 static void lp5521_run_engine(struct lp55xx_chip *chip, bool start)
 {
 	int ret;
-	u8 mode;
-	u8 exec;
 
 	/* stop engine */
 	if (!start) {
@@ -143,40 +141,9 @@ static void lp5521_run_engine(struct lp55xx_chip *chip, bool start)
 		return;
 	}
 
-	/*
-	 * To run the engine,
-	 * operation mode and enable register should updated at the same time
-	 */
-
-	ret = lp55xx_read(chip, LP5521_REG_OP_MODE, &mode);
-	if (ret)
-		return;
-
-	ret = lp55xx_read(chip, LP5521_REG_ENABLE, &exec);
-	if (ret)
-		return;
-
-	/* change operation mode to RUN only when each engine is loading */
-	if (LP5521_R_IS_LOADING(mode)) {
-		mode = (mode & ~LP5521_MODE_R_M) | LP5521_RUN_R;
-		exec = (exec & ~LP5521_EXEC_R_M) | LP5521_RUN_R;
-	}
-
-	if (LP5521_G_IS_LOADING(mode)) {
-		mode = (mode & ~LP5521_MODE_G_M) | LP5521_RUN_G;
-		exec = (exec & ~LP5521_EXEC_G_M) | LP5521_RUN_G;
-	}
-
-	if (LP5521_B_IS_LOADING(mode)) {
-		mode = (mode & ~LP5521_MODE_B_M) | LP5521_RUN_B;
-		exec = (exec & ~LP5521_EXEC_B_M) | LP5521_RUN_B;
-	}
-
-	lp55xx_write(chip, LP5521_REG_OP_MODE, mode);
-	lp5521_wait_opmode_done();
-
-	lp55xx_update_bits(chip, LP5521_REG_ENABLE, LP5521_EXEC_M, exec);
-	lp5521_wait_enable_done();
+	ret = lp55xx_run_engine_common(chip);
+	if (!ret)
+		lp5521_wait_enable_done();
 }
 
 static int lp5521_update_program_memory(struct lp55xx_chip *chip,
@@ -476,6 +443,9 @@ static struct lp55xx_device_config lp5521_cfg = {
 	.reg_op_mode = {
 		.addr = LP5521_REG_OP_MODE,
 	},
+	.reg_exec = {
+		.addr = LP5521_REG_ENABLE,
+	},
 	.reset = {
 		.addr = LP5521_REG_RESET,
 		.val  = LP5521_RESET,
drivers/leds/leds-lp5523.c +4 -37

59 lines not on this page · raw message

drivers/leds/leds-lp5562.c +6 -36

60 lines not on this page · raw message

drivers/leds/leds-lp55xx-common.c +43 -0

55 lines not on this page · raw message

drivers/leds/leds-lp55xx-common.h +2 -0
index 015ac2ef4e4d..dd74b214ec74 100644
@@ -112,6 +112,7 @@ struct lp55xx_reg {
  */
 struct lp55xx_device_config {
 	const struct lp55xx_reg reg_op_mode; /* addr, shift */
+	const struct lp55xx_reg reg_exec; /* addr, shift */
 	const struct lp55xx_reg engine_busy; /* addr, mask */
 	const struct lp55xx_reg reset;
 	const struct lp55xx_reg enable;
@@ -206,6 +207,7 @@ extern bool lp55xx_is_extclk_used(struct lp55xx_chip *chip);
 /* common chip functions */
 extern void lp55xx_stop_all_engine(struct lp55xx_chip *chip);
 extern void lp55xx_load_engine(struct lp55xx_chip *chip);
+extern int lp55xx_run_engine_common(struct lp55xx_chip *chip);
 
 /* common probe/remove function */
 extern int lp55xx_probe(struct i2c_client *client);
drivers/leds/leds-lp8501.c +4 -37

59 lines not on this page · raw message

Signature · 1 line
2.45.1
#8 lore raw

PATCH v7 8/20 leds: leds-lp55xx: Generalize update_program_memory function

In reply to Christian Marangi

LED Driver based on lp55xx all use the same logic to write memory in SMEM. The only difference is that legacy chip doesn't support pages and have the engine regs one after another.

To handle this apply the same logic used for load_engine also for update_program_memory.

Introduce a new config in device_config, base_prog. For LED chip that doesn't support pages, offset this values of 32 for each engine.

Update all lp55xx based LED driver to use this new function and define all the required bits.

---
 drivers/leds/leds-lp5521.c        | 56 +++--------------------------
 drivers/leds/leds-lp5523.c        | 50 +++-----------------------
 drivers/leds/leds-lp5562.c        | 58 +++----------------------------
 drivers/leds/leds-lp55xx-common.c | 57 ++++++++++++++++++++++++++++++
 drivers/leds/leds-lp55xx-common.h |  4 +++
 drivers/leds/leds-lp8501.c        | 52 +++------------------------
 6 files changed, 79 insertions(+), 198 deletions(-)
drivers/leds/leds-lp5521.c +5 -51
index 0b9f99f4fff2..7ea3e5715f59 100644
@@ -146,55 +146,6 @@ static void lp5521_run_engine(struct lp55xx_chip *chip, bool start)
 		lp5521_wait_enable_done();
 }
 
-static int lp5521_update_program_memory(struct lp55xx_chip *chip,
-					const u8 *data, size_t size)
-{
-	enum lp55xx_engine_index idx = chip->engine_idx;
-	u8 pattern[LP5521_PROGRAM_LENGTH] = {0};
-	static const u8 addr[] = {
-		[LP55XX_ENGINE_1] = LP5521_REG_R_PROG_MEM,
-		[LP55XX_ENGINE_2] = LP5521_REG_G_PROG_MEM,
-		[LP55XX_ENGINE_3] = LP5521_REG_B_PROG_MEM,
-	};
-	unsigned cmd;
-	char c[3];
-	int nrchars;
-	int ret;
-	int offset = 0;
-	int i = 0;
-
-	while ((offset < size - 1) && (i < LP5521_PROGRAM_LENGTH)) {
-		/* separate sscanfs because length is working only for %s */
-		ret = sscanf(data + offset, "%2s%n ", c, &nrchars);
-		if (ret != 1)
-			goto err;
-
-		ret = sscanf(c, "%2x", &cmd);
-		if (ret != 1)
-			goto err;
-
-		pattern[i] = (u8)cmd;
-		offset += nrchars;
-		i++;
-	}
-
-	/* Each instruction is 16bit long. Check that length is even */
-	if (i % 2)
-		goto err;
-
-	for (i = 0; i < LP5521_PROGRAM_LENGTH; i++) {
-		ret = lp55xx_write(chip, addr[idx] + i, pattern[i]);
-		if (ret)
-			return -EINVAL;
-	}
-
-	return size;
-
-err:
-	dev_err(&chip->cl->dev, "wrong pattern format\n");
-	return -EINVAL;
-}
-
 static void lp5521_firmware_loaded(struct lp55xx_chip *chip)
 {
 	const struct firmware *fw = chip->fw;
@@ -212,7 +163,7 @@ static void lp5521_firmware_loaded(struct lp55xx_chip *chip)
 	 */
 
 	lp55xx_load_engine(chip);
-	lp5521_update_program_memory(chip, fw->data, fw->size);
+	lp55xx_update_program_memory(chip, fw->data, fw->size);
 }
 
 static int lp5521_post_init_device(struct lp55xx_chip *chip)
@@ -389,7 +340,7 @@ static ssize_t store_engine_load(struct device *dev,
 
 	chip->engine_idx = nr;
 	lp55xx_load_engine(chip);
-	ret = lp5521_update_program_memory(chip, buf, len);
+	ret = lp55xx_update_program_memory(chip, buf, len);
 
 	mutex_unlock(&chip->lock);
 
@@ -454,6 +405,9 @@ static struct lp55xx_device_config lp5521_cfg = {
 		.addr = LP5521_REG_ENABLE,
 		.val  = LP5521_ENABLE_DEFAULT,
 	},
+	.prog_mem_base = {
+		.addr = LP5521_REG_R_PROG_MEM,
+	},
 	.max_channel  = LP5521_MAX_LEDS,
 	.post_init_device   = lp5521_post_init_device,
 	.brightness_fn      = lp5521_led_brightness,
drivers/leds/leds-lp5523.c +5 -45

74 lines not on this page · raw message

drivers/leds/leds-lp5562.c +4 -54

76 lines not on this page · raw message

drivers/leds/leds-lp55xx-common.c +57 -0

69 lines not on this page · raw message

drivers/leds/leds-lp55xx-common.h +4 -0
index dd74b214ec74..f0bbd41fdab3 100644
@@ -99,6 +99,7 @@ struct lp55xx_reg {
  *			 (if not supported 153 us sleep)
  * @reset              : Chip specific reset command
  * @enable             : Chip specific enable command
+ * @prog_mem_base      : Chip specific base reg address for chip SMEM programming
  * @pages_per_engine   : Assigned pages for each engine
  *                       (if not set chip doesn't support pages)
  * @max_channel        : Maximum number of channels
@@ -116,6 +117,7 @@ struct lp55xx_device_config {
 	const struct lp55xx_reg engine_busy; /* addr, mask */
 	const struct lp55xx_reg reset;
 	const struct lp55xx_reg enable;
+	const struct lp55xx_reg prog_mem_base;
 	const int pages_per_engine;
 	const int max_channel;
 
@@ -208,6 +210,8 @@ extern bool lp55xx_is_extclk_used(struct lp55xx_chip *chip);
 extern void lp55xx_stop_all_engine(struct lp55xx_chip *chip);
 extern void lp55xx_load_engine(struct lp55xx_chip *chip);
 extern int lp55xx_run_engine_common(struct lp55xx_chip *chip);
+extern int lp55xx_update_program_memory(struct lp55xx_chip *chip,
+					const u8 *data, size_t size);
 
 /* common probe/remove function */
 extern int lp55xx_probe(struct i2c_client *client);
drivers/leds/leds-lp8501.c +4 -48

70 lines not on this page · raw message

Signature · 1 line
2.45.1
#9 lore raw

PATCH v7 9/20 leds: leds-lp55xx: Generalize firmware_loaded function

In reply to Christian Marangi

Generalize firmware_loaded function as lp55xx based LED driver all share the same logic.

---
 drivers/leds/leds-lp5521.c        | 22 +---------------------
 drivers/leds/leds-lp5523.c        | 22 +---------------------
 drivers/leds/leds-lp5562.c        | 26 +-------------------------
 drivers/leds/leds-lp55xx-common.c | 25 +++++++++++++++++++++++++
 drivers/leds/leds-lp55xx-common.h |  1 +
 drivers/leds/leds-lp8501.c        | 22 +---------------------
 6 files changed, 30 insertions(+), 88 deletions(-)
drivers/leds/leds-lp5521.c +1 -21
index 7ea3e5715f59..722b5cd9236e 100644
@@ -146,26 +146,6 @@ static void lp5521_run_engine(struct lp55xx_chip *chip, bool start)
 		lp5521_wait_enable_done();
 }
 
-static void lp5521_firmware_loaded(struct lp55xx_chip *chip)
-{
-	const struct firmware *fw = chip->fw;
-
-	if (fw->size > LP5521_PROGRAM_LENGTH) {
-		dev_err(&chip->cl->dev, "firmware data size overflow: %zu\n",
-			fw->size);
-		return;
-	}
-
-	/*
-	 * Program memory sequence
-	 *  1) set engine mode to "LOAD"
-	 *  2) write firmware data into program memory
-	 */
-
-	lp55xx_load_engine(chip);
-	lp55xx_update_program_memory(chip, fw->data, fw->size);
-}
-
 static int lp5521_post_init_device(struct lp55xx_chip *chip)
 {
 	int ret;
@@ -413,7 +393,7 @@ static struct lp55xx_device_config lp5521_cfg = {
 	.brightness_fn      = lp5521_led_brightness,
 	.multicolor_brightness_fn = lp5521_multicolor_brightness,
 	.set_led_current    = lp5521_set_led_current,
-	.firmware_cb        = lp5521_firmware_loaded,
+	.firmware_cb        = lp55xx_firmware_loaded_cb,
 	.run_engine         = lp5521_run_engine,
 	.dev_attr_group     = &lp5521_group,
 };
drivers/leds/leds-lp5523.c +1 -21
index 395c57330484..5525d60c342c 100644
@@ -254,26 +254,6 @@ static int lp5523_init_program_engine(struct lp55xx_chip *chip)
 	return ret;
 }
 
-static void lp5523_firmware_loaded(struct lp55xx_chip *chip)
-{
-	const struct firmware *fw = chip->fw;
-
-	if (fw->size > LP5523_PROGRAM_LENGTH) {
-		dev_err(&chip->cl->dev, "firmware data size overflow: %zu\n",
-			fw->size);
-		return;
-	}
-
-	/*
-	 * Program memory sequence
-	 *  1) set engine mode to "LOAD"
-	 *  2) write firmware data into program memory
-	 */
-
-	lp55xx_load_engine(chip);
-	lp55xx_update_program_memory(chip, fw->data, fw->size);
-}
-
 static ssize_t show_engine_mode(struct device *dev,
 				struct device_attribute *attr,
 				char *buf, int nr)
@@ -785,7 +765,7 @@ static struct lp55xx_device_config lp5523_cfg = {
 	.brightness_fn      = lp5523_led_brightness,
 	.multicolor_brightness_fn = lp5523_multicolor_brightness,
 	.set_led_current    = lp5523_set_led_current,
-	.firmware_cb        = lp5523_firmware_loaded,
+	.firmware_cb        = lp55xx_firmware_loaded_cb,
 	.run_engine         = lp5523_run_engine,
 	.dev_attr_group     = &lp5523_group,
 };
drivers/leds/leds-lp5562.c +1 -25
index 7f3733fc446e..e50b68c9ccf3 100644
@@ -144,30 +144,6 @@ static void lp5562_run_engine(struct lp55xx_chip *chip, bool start)
 		lp5562_wait_enable_done();
 }
 
-static void lp5562_firmware_loaded(struct lp55xx_chip *chip)
-{
-	const struct firmware *fw = chip->fw;
-
-	/*
-	 * the firmware is encoded in ascii hex character, with 2 chars
-	 * per byte
-	 */
-	if (fw->size > (LP5562_PROGRAM_LENGTH * 2)) {
-		dev_err(&chip->cl->dev, "firmware data size overflow: %zu\n",
-			fw->size);
-		return;
-	}
-
-	/*
-	 * Program memory sequence
-	 *  1) set engine mode to "LOAD"
-	 *  2) write firmware data into program memory
-	 */
-
-	lp55xx_load_engine(chip);
-	lp55xx_update_program_memory(chip, fw->data, fw->size);
-}
-
 static int lp5562_post_init_device(struct lp55xx_chip *chip)
 {
 	int ret;
@@ -404,7 +380,7 @@ static struct lp55xx_device_config lp5562_cfg = {
 	.set_led_current    = lp5562_set_led_current,
 	.brightness_fn      = lp5562_led_brightness,
 	.run_engine         = lp5562_run_engine,
-	.firmware_cb        = lp5562_firmware_loaded,
+	.firmware_cb        = lp55xx_firmware_loaded_cb,
 	.dev_attr_group     = &lp5562_group,
 };
 
drivers/leds/leds-lp55xx-common.c +25 -0
index 8c20d7b4bc82..1c9348ed3edc 100644
@@ -217,6 +217,31 @@ int lp55xx_update_program_memory(struct lp55xx_chip *chip,
 }
 EXPORT_SYMBOL_GPL(lp55xx_update_program_memory);
 
+void lp55xx_firmware_loaded_cb(struct lp55xx_chip *chip)
+{
+	const struct firmware *fw = chip->fw;
+
+	/*
+	 * the firmware is encoded in ascii hex character, with 2 chars
+	 * per byte
+	 */
+	if (fw->size > LP55xx_PROGRAM_LENGTH * 2) {
+		dev_err(&chip->cl->dev, "firmware data size overflow: %zu\n",
+			fw->size);
+		return;
+	}
+
+	/*
+	 * Program memory sequence
+	 *  1) set engine mode to "LOAD"
+	 *  2) write firmware data into program memory
+	 */
+
+	lp55xx_load_engine(chip);
+	lp55xx_update_program_memory(chip, fw->data, fw->size);
+}
+EXPORT_SYMBOL_GPL(lp55xx_firmware_loaded_cb);
+
 static void lp55xx_reset_device(struct lp55xx_chip *chip)
 {
 	const struct lp55xx_device_config *cfg = chip->cfg;
drivers/leds/leds-lp55xx-common.h +1 -0
index f0bbd41fdab3..cbc122c56828 100644
@@ -212,6 +212,7 @@ extern void lp55xx_load_engine(struct lp55xx_chip *chip);
 extern int lp55xx_run_engine_common(struct lp55xx_chip *chip);
 extern int lp55xx_update_program_memory(struct lp55xx_chip *chip,
 					const u8 *data, size_t size);
+extern void lp55xx_firmware_loaded_cb(struct lp55xx_chip *chip);
 
 /* common probe/remove function */
 extern int lp55xx_probe(struct i2c_client *client);
drivers/leds/leds-lp8501.c +1 -21
index d4094d20bdc1..1ea7bb73cd22 100644
@@ -137,26 +137,6 @@ static void lp8501_run_engine(struct lp55xx_chip *chip, bool start)
 	lp55xx_run_engine_common(chip);
 }
 
-static void lp8501_firmware_loaded(struct lp55xx_chip *chip)
-{
-	const struct firmware *fw = chip->fw;
-
-	if (fw->size > LP8501_PROGRAM_LENGTH) {
-		dev_err(&chip->cl->dev, "firmware data size overflow: %zu\n",
-			fw->size);
-		return;
-	}
-
-	/*
-	 * Program memory sequence
-	 *  1) set engine mode to "LOAD"
-	 *  2) write firmware data into program memory
-	 */
-
-	lp55xx_load_engine(chip);
-	lp55xx_update_program_memory(chip, fw->data, fw->size);
-}
-
 static int lp8501_led_brightness(struct lp55xx_led *led)
 {
 	struct lp55xx_chip *chip = led->chip;
@@ -198,7 +178,7 @@ static struct lp55xx_device_config lp8501_cfg = {
 	.post_init_device   = lp8501_post_init_device,
 	.brightness_fn      = lp8501_led_brightness,
 	.set_led_current    = lp8501_set_led_current,
-	.firmware_cb        = lp8501_firmware_loaded,
+	.firmware_cb        = lp55xx_firmware_loaded_cb,
 	.run_engine         = lp8501_run_engine,
 };
 
Signature · 1 line
2.45.1
#10 lore raw

PATCH v7 10/20 leds: leds-lp55xx: Generalize led_brightness function

In reply to Christian Marangi

Generalize led_brightness function as the implementation is the same for most of the lp55xx based LED driver.

Introduce a new option in device_config, reg_led_pwm_base since the reg value is not the same for every LED chip.

---
 drivers/leds/leds-lp5521.c        | 18 ++++--------------
 drivers/leds/leds-lp5523.c        | 17 ++++-------------
 drivers/leds/leds-lp55xx-common.c | 14 ++++++++++++++
 drivers/leds/leds-lp55xx-common.h |  3 +++
 drivers/leds/leds-lp8501.c        | 18 ++++--------------
 5 files changed, 29 insertions(+), 41 deletions(-)
drivers/leds/leds-lp5521.c +4 -14
index 722b5cd9236e..e4d0dcdbf11b 100644
@@ -241,19 +241,6 @@ static int lp5521_multicolor_brightness(struct lp55xx_led *led)
 	return ret;
 }
 
-static int lp5521_led_brightness(struct lp55xx_led *led)
-{
-	struct lp55xx_chip *chip = led->chip;
-	int ret;
-
-	mutex_lock(&chip->lock);
-	ret = lp55xx_write(chip, LP5521_REG_LED_PWM_BASE + led->chan_nr,
-		led->brightness);
-	mutex_unlock(&chip->lock);
-
-	return ret;
-}
-
 static ssize_t show_engine_mode(struct device *dev,
 				struct device_attribute *attr,
 				char *buf, int nr)
@@ -388,9 +375,12 @@ static struct lp55xx_device_config lp5521_cfg = {
 	.prog_mem_base = {
 		.addr = LP5521_REG_R_PROG_MEM,
 	},
+	.reg_led_pwm_base = {
+		.addr = LP5521_REG_LED_PWM_BASE,
+	},
 	.max_channel  = LP5521_MAX_LEDS,
 	.post_init_device   = lp5521_post_init_device,
-	.brightness_fn      = lp5521_led_brightness,
+	.brightness_fn      = lp55xx_led_brightness,
 	.multicolor_brightness_fn = lp5521_multicolor_brightness,
 	.set_led_current    = lp5521_set_led_current,
 	.firmware_cb        = lp55xx_firmware_loaded_cb,
drivers/leds/leds-lp5523.c +4 -13
index 5525d60c342c..a3b4063d504e 100644
@@ -683,18 +683,6 @@ static int lp5523_multicolor_brightness(struct lp55xx_led *led)
 	return ret;
 }
 
-static int lp5523_led_brightness(struct lp55xx_led *led)
-{
-	struct lp55xx_chip *chip = led->chip;
-	int ret;
-
-	mutex_lock(&chip->lock);
-	ret = lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + led->chan_nr,
-		     led->brightness);
-	mutex_unlock(&chip->lock);
-	return ret;
-}
-
 static LP55XX_DEV_ATTR_RW(engine1_mode, show_engine1_mode, store_engine1_mode);
 static LP55XX_DEV_ATTR_RW(engine2_mode, show_engine2_mode, store_engine2_mode);
 static LP55XX_DEV_ATTR_RW(engine3_mode, show_engine3_mode, store_engine3_mode);
@@ -759,10 +747,13 @@ static struct lp55xx_device_config lp5523_cfg = {
 	.prog_mem_base = {
 		.addr = LP5523_REG_PROG_MEM,
 	},
+	.reg_led_pwm_base = {
+		.addr = LP5523_REG_LED_PWM_BASE,
+	},
 	.pages_per_engine   = LP5523_PAGES_PER_ENGINE,
 	.max_channel  = LP5523_MAX_LEDS,
 	.post_init_device   = lp5523_post_init_device,
-	.brightness_fn      = lp5523_led_brightness,
+	.brightness_fn      = lp55xx_led_brightness,
 	.multicolor_brightness_fn = lp5523_multicolor_brightness,
 	.set_led_current    = lp5523_set_led_current,
 	.firmware_cb        = lp55xx_firmware_loaded_cb,
drivers/leds/leds-lp55xx-common.c +14 -0
index 1c9348ed3edc..cc393e407837 100644
@@ -242,6 +242,20 @@ void lp55xx_firmware_loaded_cb(struct lp55xx_chip *chip)
 }
 EXPORT_SYMBOL_GPL(lp55xx_firmware_loaded_cb);
 
+int lp55xx_led_brightness(struct lp55xx_led *led)
+{
+	struct lp55xx_chip *chip = led->chip;
+	const struct lp55xx_device_config *cfg = chip->cfg;
+	int ret;
+
+	mutex_lock(&chip->lock);
+	ret = lp55xx_write(chip, cfg->reg_led_pwm_base.addr + led->chan_nr,
+			   led->brightness);
+	mutex_unlock(&chip->lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(lp55xx_led_brightness);
+
 static void lp55xx_reset_device(struct lp55xx_chip *chip)
 {
 	const struct lp55xx_device_config *cfg = chip->cfg;
drivers/leds/leds-lp55xx-common.h +3 -0
index cbc122c56828..4578418e5011 100644
@@ -100,6 +100,7 @@ struct lp55xx_reg {
  * @reset              : Chip specific reset command
  * @enable             : Chip specific enable command
  * @prog_mem_base      : Chip specific base reg address for chip SMEM programming
+ * @reg_led_pwm_base   : Chip specific base reg address for LED PWM conf
  * @pages_per_engine   : Assigned pages for each engine
  *                       (if not set chip doesn't support pages)
  * @max_channel        : Maximum number of channels
@@ -118,6 +119,7 @@ struct lp55xx_device_config {
 	const struct lp55xx_reg reset;
 	const struct lp55xx_reg enable;
 	const struct lp55xx_reg prog_mem_base;
+	const struct lp55xx_reg reg_led_pwm_base;
 	const int pages_per_engine;
 	const int max_channel;
 
@@ -213,6 +215,7 @@ extern int lp55xx_run_engine_common(struct lp55xx_chip *chip);
 extern int lp55xx_update_program_memory(struct lp55xx_chip *chip,
 					const u8 *data, size_t size);
 extern void lp55xx_firmware_loaded_cb(struct lp55xx_chip *chip);
+extern int lp55xx_led_brightness(struct lp55xx_led *led);
 
 /* common probe/remove function */
 extern int lp55xx_probe(struct i2c_client *client);
drivers/leds/leds-lp8501.c +4 -14
index 1ea7bb73cd22..8f1fd9525e9a 100644
@@ -137,19 +137,6 @@ static void lp8501_run_engine(struct lp55xx_chip *chip, bool start)
 	lp55xx_run_engine_common(chip);
 }
 
-static int lp8501_led_brightness(struct lp55xx_led *led)
-{
-	struct lp55xx_chip *chip = led->chip;
-	int ret;
-
-	mutex_lock(&chip->lock);
-	ret = lp55xx_write(chip, LP8501_REG_LED_PWM_BASE + led->chan_nr,
-		     led->brightness);
-	mutex_unlock(&chip->lock);
-
-	return ret;
-}
-
 /* Chip specific configurations */
 static struct lp55xx_device_config lp8501_cfg = {
 	.reg_op_mode = {
@@ -173,10 +160,13 @@ static struct lp55xx_device_config lp8501_cfg = {
 	.prog_mem_base = {
 		.addr = LP8501_REG_PROG_MEM,
 	},
+	.reg_led_pwm_base = {
+		.addr = LP8501_REG_LED_PWM_BASE,
+	},
 	.pages_per_engine   = LP8501_PAGES_PER_ENGINE,
 	.max_channel  = LP8501_MAX_LEDS,
 	.post_init_device   = lp8501_post_init_device,
-	.brightness_fn      = lp8501_led_brightness,
+	.brightness_fn      = lp55xx_led_brightness,
 	.set_led_current    = lp8501_set_led_current,
 	.firmware_cb        = lp55xx_firmware_loaded_cb,
 	.run_engine         = lp8501_run_engine,
Signature · 1 line
2.45.1
#11 lore raw

Re: [PATCH v7 10/20] leds: leds-lp55xx: Generalize led_brightness function

…

Quoted 2 lines

+++ b/drivers/leds/leds-lp55xx-common.c

(quoted diff) +0 -0
@@ -242,6 +242,20 @@ void lp55xx_firmware_loaded_cb(struct lp55xx_chip *chip)

…

Quoted 2 lines
drivers/leds/leds-lp55xx-common.c +2 -0
+int lp55xx_led_brightness(struct lp55xx_led *led)
+{

…

drivers/leds/leds-lp55xx-common.c void lp55xx_firmware_loaded_cb(struct lp55xx_chip *chip) · line 256

Quoted 6 lines
drivers/leds/leds-lp55xx-common.c +6 -0
+	mutex_lock(&chip->lock);
+	ret = lp55xx_write(chip, cfg->reg_led_pwm_base.addr + led->chan_nr,
+			   led->brightness);
+	mutex_unlock(&chip->lock);
+	return ret;
+}

…

Would you become interested to apply a statement like “guard(mutex)(&chip->lock);”?
https://elixir.bootlin.com/linux/v6.10-rc5/source/include/linux/mutex.h#L196

Regards,
Markus

#12 lore raw

PATCH v7 11/20 leds: leds-lp55xx: Generalize multicolor_brightness function

In reply to Christian Marangi

Generalize multicolor_brightness function as the implementation is the same for most of the lp55xx based LED driver.

---
 drivers/leds/leds-lp5521.c        | 21 +--------------------
 drivers/leds/leds-lp5523.c        | 21 +--------------------
 drivers/leds/leds-lp55xx-common.c | 21 +++++++++++++++++++++
 drivers/leds/leds-lp55xx-common.h |  1 +
 4 files changed, 24 insertions(+), 40 deletions(-)
drivers/leds/leds-lp5521.c +1 -20
index e4d0dcdbf11b..dd7e996f22f9 100644
@@ -222,25 +222,6 @@ static int lp5521_run_selftest(struct lp55xx_chip *chip, char *buf)
 	return 0;
 }
 
-static int lp5521_multicolor_brightness(struct lp55xx_led *led)
-{
-	struct lp55xx_chip *chip = led->chip;
-	int ret;
-	int i;
-
-	mutex_lock(&chip->lock);
-	for (i = 0; i < led->mc_cdev.num_colors; i++) {
-		ret = lp55xx_write(chip,
-				   LP5521_REG_LED_PWM_BASE +
-				   led->mc_cdev.subled_info[i].channel,
-				   led->mc_cdev.subled_info[i].brightness);
-		if (ret)
-			break;
-	}
-	mutex_unlock(&chip->lock);
-	return ret;
-}
-
 static ssize_t show_engine_mode(struct device *dev,
 				struct device_attribute *attr,
 				char *buf, int nr)
@@ -381,7 +362,7 @@ static struct lp55xx_device_config lp5521_cfg = {
 	.max_channel  = LP5521_MAX_LEDS,
 	.post_init_device   = lp5521_post_init_device,
 	.brightness_fn      = lp55xx_led_brightness,
-	.multicolor_brightness_fn = lp5521_multicolor_brightness,
+	.multicolor_brightness_fn = lp55xx_multicolor_brightness,
 	.set_led_current    = lp5521_set_led_current,
 	.firmware_cb        = lp55xx_firmware_loaded_cb,
 	.run_engine         = lp5521_run_engine,
drivers/leds/leds-lp5523.c +1 -20
index a3b4063d504e..bfa0c4431ede 100644
@@ -664,25 +664,6 @@ static ssize_t store_master_fader_leds(struct device *dev,
 	return ret;
 }
 
-static int lp5523_multicolor_brightness(struct lp55xx_led *led)
-{
-	struct lp55xx_chip *chip = led->chip;
-	int ret;
-	int i;
-
-	mutex_lock(&chip->lock);
-	for (i = 0; i < led->mc_cdev.num_colors; i++) {
-		ret = lp55xx_write(chip,
-				   LP5523_REG_LED_PWM_BASE +
-				   led->mc_cdev.subled_info[i].channel,
-				   led->mc_cdev.subled_info[i].brightness);
-		if (ret)
-			break;
-	}
-	mutex_unlock(&chip->lock);
-	return ret;
-}
-
 static LP55XX_DEV_ATTR_RW(engine1_mode, show_engine1_mode, store_engine1_mode);
 static LP55XX_DEV_ATTR_RW(engine2_mode, show_engine2_mode, store_engine2_mode);
 static LP55XX_DEV_ATTR_RW(engine3_mode, show_engine3_mode, store_engine3_mode);
@@ -754,7 +735,7 @@ static struct lp55xx_device_config lp5523_cfg = {
 	.max_channel  = LP5523_MAX_LEDS,
 	.post_init_device   = lp5523_post_init_device,
 	.brightness_fn      = lp55xx_led_brightness,
-	.multicolor_brightness_fn = lp5523_multicolor_brightness,
+	.multicolor_brightness_fn = lp55xx_multicolor_brightness,
 	.set_led_current    = lp5523_set_led_current,
 	.firmware_cb        = lp55xx_firmware_loaded_cb,
 	.run_engine         = lp5523_run_engine,
drivers/leds/leds-lp55xx-common.c +21 -0
index cc393e407837..27008b6a4757 100644
@@ -256,6 +256,27 @@ int lp55xx_led_brightness(struct lp55xx_led *led)
 }
 EXPORT_SYMBOL_GPL(lp55xx_led_brightness);
 
+int lp55xx_multicolor_brightness(struct lp55xx_led *led)
+{
+	struct lp55xx_chip *chip = led->chip;
+	const struct lp55xx_device_config *cfg = chip->cfg;
+	int ret;
+	int i;
+
+	mutex_lock(&chip->lock);
+	for (i = 0; i < led->mc_cdev.num_colors; i++) {
+		ret = lp55xx_write(chip,
+				   cfg->reg_led_pwm_base.addr +
+				   led->mc_cdev.subled_info[i].channel,
+				   led->mc_cdev.subled_info[i].brightness);
+		if (ret)
+			break;
+	}
+	mutex_unlock(&chip->lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(lp55xx_multicolor_brightness);
+
 static void lp55xx_reset_device(struct lp55xx_chip *chip)
 {
 	const struct lp55xx_device_config *cfg = chip->cfg;
drivers/leds/leds-lp55xx-common.h +1 -0
index 4578418e5011..021dd17bc5d7 100644
@@ -216,6 +216,7 @@ extern int lp55xx_update_program_memory(struct lp55xx_chip *chip,
 					const u8 *data, size_t size);
 extern void lp55xx_firmware_loaded_cb(struct lp55xx_chip *chip);
 extern int lp55xx_led_brightness(struct lp55xx_led *led);
+extern int lp55xx_multicolor_brightness(struct lp55xx_led *led);
 
 /* common probe/remove function */
 extern int lp55xx_probe(struct i2c_client *client);
Signature · 1 line
2.45.1
#13 lore raw

PATCH v7 12/20 leds: leds-lp55xx: Generalize set_led_current function

In reply to Christian Marangi

Generalize set_led_current function as the implementation is the same for most of the lp55xx based LED driver.

---
 drivers/leds/leds-lp5521.c        | 12 ++++--------
 drivers/leds/leds-lp5523.c        | 12 ++++--------
 drivers/leds/leds-lp55xx-common.c | 11 +++++++++++
 drivers/leds/leds-lp55xx-common.h |  3 +++
 drivers/leds/leds-lp8501.c        | 12 ++++--------
 5 files changed, 26 insertions(+), 24 deletions(-)
drivers/leds/leds-lp5521.c +4 -8
index dd7e996f22f9..a1a3bf0ff703 100644
@@ -108,13 +108,6 @@ static inline void lp5521_wait_enable_done(void)
 	usleep_range(500, 600);
 }
 
-static void lp5521_set_led_current(struct lp55xx_led *led, u8 led_current)
-{
-	led->led_current = led_current;
-	lp55xx_write(led->chip, LP5521_REG_LED_CURRENT_BASE + led->chan_nr,
-		led_current);
-}
-
 static void lp5521_stop_engine(struct lp55xx_chip *chip)
 {
 	enum lp55xx_engine_index idx = chip->engine_idx;
@@ -359,11 +352,14 @@ static struct lp55xx_device_config lp5521_cfg = {
 	.reg_led_pwm_base = {
 		.addr = LP5521_REG_LED_PWM_BASE,
 	},
+	.reg_led_current_base = {
+		.addr = LP5521_REG_LED_CURRENT_BASE,
+	},
 	.max_channel  = LP5521_MAX_LEDS,
 	.post_init_device   = lp5521_post_init_device,
 	.brightness_fn      = lp55xx_led_brightness,
 	.multicolor_brightness_fn = lp55xx_multicolor_brightness,
-	.set_led_current    = lp5521_set_led_current,
+	.set_led_current    = lp55xx_set_led_current,
 	.firmware_cb        = lp55xx_firmware_loaded_cb,
 	.run_engine         = lp5521_run_engine,
 	.dev_attr_group     = &lp5521_group,
drivers/leds/leds-lp5523.c +4 -8
index bfa0c4431ede..3030a4495808 100644
@@ -122,13 +122,6 @@ static inline void lp5523_wait_opmode_done(void)
 	usleep_range(1000, 2000);
 }
 
-static void lp5523_set_led_current(struct lp55xx_led *led, u8 led_current)
-{
-	led->led_current = led_current;
-	lp55xx_write(led->chip, LP5523_REG_LED_CURRENT_BASE + led->chan_nr,
-		led_current);
-}
-
 static int lp5523_post_init_device(struct lp55xx_chip *chip)
 {
 	int ret;
@@ -731,12 +724,15 @@ static struct lp55xx_device_config lp5523_cfg = {
 	.reg_led_pwm_base = {
 		.addr = LP5523_REG_LED_PWM_BASE,
 	},
+	.reg_led_current_base = {
+		.addr = LP5523_REG_LED_CURRENT_BASE,
+	},
 	.pages_per_engine   = LP5523_PAGES_PER_ENGINE,
 	.max_channel  = LP5523_MAX_LEDS,
 	.post_init_device   = lp5523_post_init_device,
 	.brightness_fn      = lp55xx_led_brightness,
 	.multicolor_brightness_fn = lp55xx_multicolor_brightness,
-	.set_led_current    = lp5523_set_led_current,
+	.set_led_current    = lp55xx_set_led_current,
 	.firmware_cb        = lp55xx_firmware_loaded_cb,
 	.run_engine         = lp5523_run_engine,
 	.dev_attr_group     = &lp5523_group,
drivers/leds/leds-lp55xx-common.c +11 -0
index 27008b6a4757..d17538ebfcf9 100644
@@ -277,6 +277,17 @@ int lp55xx_multicolor_brightness(struct lp55xx_led *led)
 }
 EXPORT_SYMBOL_GPL(lp55xx_multicolor_brightness);
 
+void lp55xx_set_led_current(struct lp55xx_led *led, u8 led_current)
+{
+	struct lp55xx_chip *chip = led->chip;
+	const struct lp55xx_device_config *cfg = chip->cfg;
+
+	led->led_current = led_current;
+	lp55xx_write(led->chip, cfg->reg_led_current_base.addr + led->chan_nr,
+		     led_current);
+}
+EXPORT_SYMBOL_GPL(lp55xx_set_led_current);
+
 static void lp55xx_reset_device(struct lp55xx_chip *chip)
 {
 	const struct lp55xx_device_config *cfg = chip->cfg;
drivers/leds/leds-lp55xx-common.h +3 -0
index 021dd17bc5d7..e638049d9297 100644
@@ -101,6 +101,7 @@ struct lp55xx_reg {
  * @enable             : Chip specific enable command
  * @prog_mem_base      : Chip specific base reg address for chip SMEM programming
  * @reg_led_pwm_base   : Chip specific base reg address for LED PWM conf
+ * @reg_led_current_base : Chip specific base reg address for LED current conf
  * @pages_per_engine   : Assigned pages for each engine
  *                       (if not set chip doesn't support pages)
  * @max_channel        : Maximum number of channels
@@ -120,6 +121,7 @@ struct lp55xx_device_config {
 	const struct lp55xx_reg enable;
 	const struct lp55xx_reg prog_mem_base;
 	const struct lp55xx_reg reg_led_pwm_base;
+	const struct lp55xx_reg reg_led_current_base;
 	const int pages_per_engine;
 	const int max_channel;
 
@@ -217,6 +219,7 @@ extern int lp55xx_update_program_memory(struct lp55xx_chip *chip,
 extern void lp55xx_firmware_loaded_cb(struct lp55xx_chip *chip);
 extern int lp55xx_led_brightness(struct lp55xx_led *led);
 extern int lp55xx_multicolor_brightness(struct lp55xx_led *led);
+extern void lp55xx_set_led_current(struct lp55xx_led *led, u8 led_current);
 
 /* common probe/remove function */
 extern int lp55xx_probe(struct i2c_client *client);
drivers/leds/leds-lp8501.c +4 -8
index 8f1fd9525e9a..d924572e4533 100644
@@ -84,13 +84,6 @@ static inline void lp8501_wait_opmode_done(void)
 	usleep_range(1000, 2000);
 }
 
-static void lp8501_set_led_current(struct lp55xx_led *led, u8 led_current)
-{
-	led->led_current = led_current;
-	lp55xx_write(led->chip, LP8501_REG_LED_CURRENT_BASE + led->chan_nr,
-		led_current);
-}
-
 static int lp8501_post_init_device(struct lp55xx_chip *chip)
 {
 	int ret;
@@ -163,11 +156,14 @@ static struct lp55xx_device_config lp8501_cfg = {
 	.reg_led_pwm_base = {
 		.addr = LP8501_REG_LED_PWM_BASE,
 	},
+	.reg_led_current_base = {
+		.addr = LP8501_REG_LED_CURRENT_BASE,
+	},
 	.pages_per_engine   = LP8501_PAGES_PER_ENGINE,
 	.max_channel  = LP8501_MAX_LEDS,
 	.post_init_device   = lp8501_post_init_device,
 	.brightness_fn      = lp55xx_led_brightness,
-	.set_led_current    = lp8501_set_led_current,
+	.set_led_current    = lp55xx_set_led_current,
 	.firmware_cb        = lp55xx_firmware_loaded_cb,
 	.run_engine         = lp8501_run_engine,
 };
Signature · 1 line
2.45.1
#14 lore raw

PATCH v7 13/20 leds: leds-lp55xx: Generalize turn_off_channels function

In reply to Christian Marangi

Generalize turn_off_channels function as the implementation is the same for most of the lp55xx based LED driver.

---
 drivers/leds/leds-lp5523.c        | 10 +---------
 drivers/leds/leds-lp55xx-common.c | 10 ++++++++++
 drivers/leds/leds-lp55xx-common.h |  1 +
 drivers/leds/leds-lp8501.c        | 10 +---------
 4 files changed, 13 insertions(+), 18 deletions(-)
drivers/leds/leds-lp5523.c +1 -9
index 3030a4495808..4a4463cb44a4 100644
@@ -167,20 +167,12 @@ static void lp5523_stop_engine(struct lp55xx_chip *chip)
 	lp5523_wait_opmode_done();
 }
 
-static void lp5523_turn_off_channels(struct lp55xx_chip *chip)
-{
-	int i;
-
-	for (i = 0; i < LP5523_MAX_LEDS; i++)
-		lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + i, 0);
-}
-
 static void lp5523_run_engine(struct lp55xx_chip *chip, bool start)
 {
 	/* stop engine */
 	if (!start) {
 		lp5523_stop_engine(chip);
-		lp5523_turn_off_channels(chip);
+		lp55xx_turn_off_channels(chip);
 		return;
 	}
 
drivers/leds/leds-lp55xx-common.c +10 -0
index d17538ebfcf9..58121f27f17b 100644
@@ -288,6 +288,16 @@ void lp55xx_set_led_current(struct lp55xx_led *led, u8 led_current)
 }
 EXPORT_SYMBOL_GPL(lp55xx_set_led_current);
 
+void lp55xx_turn_off_channels(struct lp55xx_chip *chip)
+{
+	const struct lp55xx_device_config *cfg = chip->cfg;
+	int i;
+
+	for (i = 0; i < cfg->max_channel; i++)
+		lp55xx_write(chip, cfg->reg_led_pwm_base.addr + i, 0);
+}
+EXPORT_SYMBOL_GPL(lp55xx_turn_off_channels);
+
 static void lp55xx_reset_device(struct lp55xx_chip *chip)
 {
 	const struct lp55xx_device_config *cfg = chip->cfg;
drivers/leds/leds-lp55xx-common.h +1 -0
index e638049d9297..531fbb0acb2e 100644
@@ -220,6 +220,7 @@ extern void lp55xx_firmware_loaded_cb(struct lp55xx_chip *chip);
 extern int lp55xx_led_brightness(struct lp55xx_led *led);
 extern int lp55xx_multicolor_brightness(struct lp55xx_led *led);
 extern void lp55xx_set_led_current(struct lp55xx_led *led, u8 led_current);
+extern void lp55xx_turn_off_channels(struct lp55xx_chip *chip);
 
 /* common probe/remove function */
 extern int lp55xx_probe(struct i2c_client *client);
drivers/leds/leds-lp8501.c +1 -9
index d924572e4533..1fb876f64cb7 100644
@@ -110,20 +110,12 @@ static int lp8501_post_init_device(struct lp55xx_chip *chip)
 				LP8501_PWR_CONFIG_M, chip->pdata->pwr_sel);
 }
 
-static void lp8501_turn_off_channels(struct lp55xx_chip *chip)
-{
-	int i;
-
-	for (i = 0; i < LP8501_MAX_LEDS; i++)
-		lp55xx_write(chip, LP8501_REG_LED_PWM_BASE + i, 0);
-}
-
 static void lp8501_run_engine(struct lp55xx_chip *chip, bool start)
 {
 	/* stop engine */
 	if (!start) {
 		lp55xx_stop_all_engine(chip);
-		lp8501_turn_off_channels(chip);
+		lp55xx_turn_off_channels(chip);
 		return;
 	}
 
Signature · 1 line
2.45.1
#15 lore raw

PATCH v7 14/20 leds: leds-lp55xx: Generalize stop_engine function

In reply to Christian Marangi

Generalize stop_engine function as the implementation is the same for most of the lp55xx based LED driver.

---
 drivers/leds/leds-lp5521.c        | 20 +++-----------------
 drivers/leds/leds-lp5523.c        | 20 +++-----------------
 drivers/leds/leds-lp55xx-common.c | 13 +++++++++++++
 drivers/leds/leds-lp55xx-common.h |  1 +
 4 files changed, 20 insertions(+), 34 deletions(-)
drivers/leds/leds-lp5521.c +3 -17
index a1a3bf0ff703..4afae0c70d19 100644
@@ -108,27 +108,13 @@ static inline void lp5521_wait_enable_done(void)
 	usleep_range(500, 600);
 }
 
-static void lp5521_stop_engine(struct lp55xx_chip *chip)
-{
-	enum lp55xx_engine_index idx = chip->engine_idx;
-	static const u8 mask[] = {
-		[LP55XX_ENGINE_1] = LP5521_MODE_R_M,
-		[LP55XX_ENGINE_2] = LP5521_MODE_G_M,
-		[LP55XX_ENGINE_3] = LP5521_MODE_B_M,
-	};
-
-	lp55xx_update_bits(chip, LP5521_REG_OP_MODE, mask[idx], 0);
-
-	lp5521_wait_opmode_done();
-}
-
 static void lp5521_run_engine(struct lp55xx_chip *chip, bool start)
 {
 	int ret;
 
 	/* stop engine */
 	if (!start) {
-		lp5521_stop_engine(chip);
+		lp55xx_stop_engine(chip);
 		lp55xx_write(chip, LP5521_REG_OP_MODE, LP5521_CMD_DIRECT);
 		lp5521_wait_opmode_done();
 		return;
@@ -253,11 +239,11 @@ static ssize_t store_engine_mode(struct device *dev,
 		lp5521_run_engine(chip, true);
 		engine->mode = LP55XX_ENGINE_RUN;
 	} else if (!strncmp(buf, "load", 4)) {
-		lp5521_stop_engine(chip);
+		lp55xx_stop_engine(chip);
 		lp55xx_load_engine(chip);
 		engine->mode = LP55XX_ENGINE_LOAD;
 	} else if (!strncmp(buf, "disabled", 8)) {
-		lp5521_stop_engine(chip);
+		lp55xx_stop_engine(chip);
 		engine->mode = LP55XX_ENGINE_DISABLED;
 	}
 
drivers/leds/leds-lp5523.c +3 -17
index 4a4463cb44a4..1dd909a0fff5 100644
@@ -153,25 +153,11 @@ static int lp5523_post_init_device(struct lp55xx_chip *chip)
 	return lp5523_init_program_engine(chip);
 }
 
-static void lp5523_stop_engine(struct lp55xx_chip *chip)
-{
-	enum lp55xx_engine_index idx = chip->engine_idx;
-	static const u8 mask[] = {
-		[LP55XX_ENGINE_1] = LP5523_MODE_ENG1_M,
-		[LP55XX_ENGINE_2] = LP5523_MODE_ENG2_M,
-		[LP55XX_ENGINE_3] = LP5523_MODE_ENG3_M,
-	};
-
-	lp55xx_update_bits(chip, LP5523_REG_OP_MODE, mask[idx], 0);
-
-	lp5523_wait_opmode_done();
-}
-
 static void lp5523_run_engine(struct lp55xx_chip *chip, bool start)
 {
 	/* stop engine */
 	if (!start) {
-		lp5523_stop_engine(chip);
+		lp55xx_stop_engine(chip);
 		lp55xx_turn_off_channels(chip);
 		return;
 	}
@@ -277,11 +263,11 @@ static ssize_t store_engine_mode(struct device *dev,
 		lp5523_run_engine(chip, true);
 		engine->mode = LP55XX_ENGINE_RUN;
 	} else if (!strncmp(buf, "load", 4)) {
-		lp5523_stop_engine(chip);
+		lp55xx_stop_engine(chip);
 		lp55xx_load_engine(chip);
 		engine->mode = LP55XX_ENGINE_LOAD;
 	} else if (!strncmp(buf, "disabled", 8)) {
-		lp5523_stop_engine(chip);
+		lp55xx_stop_engine(chip);
 		engine->mode = LP55XX_ENGINE_DISABLED;
 	}
 
drivers/leds/leds-lp55xx-common.c +13 -0
index 58121f27f17b..cecff34e6e15 100644
@@ -298,6 +298,19 @@ void lp55xx_turn_off_channels(struct lp55xx_chip *chip)
 }
 EXPORT_SYMBOL_GPL(lp55xx_turn_off_channels);
 
+void lp55xx_stop_engine(struct lp55xx_chip *chip)
+{
+	enum lp55xx_engine_index idx = chip->engine_idx;
+	const struct lp55xx_device_config *cfg = chip->cfg;
+	u8 mask;
+
+	mask = LP55xx_MODE_ENGn_MASK(idx, cfg->reg_op_mode.shift);
+	lp55xx_update_bits(chip, cfg->reg_op_mode.addr, mask, 0);
+
+	lp55xx_wait_opmode_done(chip);
+}
+EXPORT_SYMBOL_GPL(lp55xx_stop_engine);
+
 static void lp55xx_reset_device(struct lp55xx_chip *chip)
 {
 	const struct lp55xx_device_config *cfg = chip->cfg;
drivers/leds/leds-lp55xx-common.h +1 -0
index 531fbb0acb2e..0aba6955a3af 100644
@@ -221,6 +221,7 @@ extern int lp55xx_led_brightness(struct lp55xx_led *led);
 extern int lp55xx_multicolor_brightness(struct lp55xx_led *led);
 extern void lp55xx_set_led_current(struct lp55xx_led *led, u8 led_current);
 extern void lp55xx_turn_off_channels(struct lp55xx_chip *chip);
+extern void lp55xx_stop_engine(struct lp55xx_chip *chip);
 
 /* common probe/remove function */
 extern int lp55xx_probe(struct i2c_client *client);
Signature · 1 line
2.45.1
#16 lore raw

PATCH v7 15/20 leds: leds-lp55xx: Generalize sysfs engine_load and engine_mode

In reply to Christian Marangi

Generalize sysfs engine_load and engine_mode since their implementation is the same acrosso some lp55xx based LED driver.

---
 drivers/leds/leds-lp5521.c        | 88 +++----------------------------
 drivers/leds/leds-lp5523.c        | 88 +++----------------------------
 drivers/leds/leds-lp55xx-common.c | 83 ++++++++++++++++++++++++++---
 drivers/leds/leds-lp55xx-common.h | 44 ++++++++++------
 4 files changed, 117 insertions(+), 186 deletions(-)
drivers/leds/leds-lp5521.c +6 -82
index 4afae0c70d19..519e7627ac22 100644
@@ -201,82 +201,6 @@ static int lp5521_run_selftest(struct lp55xx_chip *chip, char *buf)
 	return 0;
 }
 
-static ssize_t show_engine_mode(struct device *dev,
-				struct device_attribute *attr,
-				char *buf, int nr)
-{
-	struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
-	struct lp55xx_chip *chip = led->chip;
-	enum lp55xx_engine_mode mode = chip->engines[nr - 1].mode;
-
-	switch (mode) {
-	case LP55XX_ENGINE_RUN:
-		return sprintf(buf, "run\n");
-	case LP55XX_ENGINE_LOAD:
-		return sprintf(buf, "load\n");
-	case LP55XX_ENGINE_DISABLED:
-	default:
-		return sprintf(buf, "disabled\n");
-	}
-}
-show_mode(1)
-show_mode(2)
-show_mode(3)
-
-static ssize_t store_engine_mode(struct device *dev,
-				 struct device_attribute *attr,
-				 const char *buf, size_t len, int nr)
-{
-	struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
-	struct lp55xx_chip *chip = led->chip;
-	struct lp55xx_engine *engine = &chip->engines[nr - 1];
-
-	mutex_lock(&chip->lock);
-
-	chip->engine_idx = nr;
-
-	if (!strncmp(buf, "run", 3)) {
-		lp5521_run_engine(chip, true);
-		engine->mode = LP55XX_ENGINE_RUN;
-	} else if (!strncmp(buf, "load", 4)) {
-		lp55xx_stop_engine(chip);
-		lp55xx_load_engine(chip);
-		engine->mode = LP55XX_ENGINE_LOAD;
-	} else if (!strncmp(buf, "disabled", 8)) {
-		lp55xx_stop_engine(chip);
-		engine->mode = LP55XX_ENGINE_DISABLED;
-	}
-
-	mutex_unlock(&chip->lock);
-
-	return len;
-}
-store_mode(1)
-store_mode(2)
-store_mode(3)
-
-static ssize_t store_engine_load(struct device *dev,
-			     struct device_attribute *attr,
-			     const char *buf, size_t len, int nr)
-{
-	struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
-	struct lp55xx_chip *chip = led->chip;
-	int ret;
-
-	mutex_lock(&chip->lock);
-
-	chip->engine_idx = nr;
-	lp55xx_load_engine(chip);
-	ret = lp55xx_update_program_memory(chip, buf, len);
-
-	mutex_unlock(&chip->lock);
-
-	return ret;
-}
-store_load(1)
-store_load(2)
-store_load(3)
-
 static ssize_t lp5521_selftest(struct device *dev,
 			       struct device_attribute *attr,
 			       char *buf)
@@ -293,12 +217,12 @@ static ssize_t lp5521_selftest(struct device *dev,
 }
 
 /* device attributes */
-static LP55XX_DEV_ATTR_RW(engine1_mode, show_engine1_mode, store_engine1_mode);
-static LP55XX_DEV_ATTR_RW(engine2_mode, show_engine2_mode, store_engine2_mode);
-static LP55XX_DEV_ATTR_RW(engine3_mode, show_engine3_mode, store_engine3_mode);
-static LP55XX_DEV_ATTR_WO(engine1_load, store_engine1_load);
-static LP55XX_DEV_ATTR_WO(engine2_load, store_engine2_load);
-static LP55XX_DEV_ATTR_WO(engine3_load, store_engine3_load);
+LP55XX_DEV_ATTR_ENGINE_MODE(1);
+LP55XX_DEV_ATTR_ENGINE_MODE(2);
+LP55XX_DEV_ATTR_ENGINE_MODE(3);
+LP55XX_DEV_ATTR_ENGINE_LOAD(1);
+LP55XX_DEV_ATTR_ENGINE_LOAD(2);
+LP55XX_DEV_ATTR_ENGINE_LOAD(3);
 static LP55XX_DEV_ATTR_RO(selftest, lp5521_selftest);
 
 static struct attribute *lp5521_attributes[] = {
drivers/leds/leds-lp5523.c +6 -82

109 lines not on this page · raw message

drivers/leds/leds-lp55xx-common.c +77 -6

107 lines not on this page · raw message

drivers/leds/leds-lp55xx-common.h +28 -16

66 lines not on this page · raw message

Signature · 1 line
2.45.1
#17 lore raw

PATCH v7 16/20 leds: leds-lp55xx: Generalize sysfs engine_leds

In reply to Christian Marangi

Generalize sysfs engine_leds since their implementation is the same across some lp55xx based LED driver.

While at it simplify the implementation for show_engine_leds.

---
 drivers/leds/leds-lp5523.c        | 119 +-----------------------------
 drivers/leds/leds-lp55xx-common.c | 109 +++++++++++++++++++++++++++
 drivers/leds/leds-lp55xx-common.h |  32 ++++----
 3 files changed, 131 insertions(+), 129 deletions(-)
drivers/leds/leds-lp5523.c +3 -116
index 19b119a2b256..9d91c2c5a3eb 100644
@@ -225,119 +225,6 @@ static int lp5523_init_program_engine(struct lp55xx_chip *chip)
 	return ret;
 }
 
-static int lp5523_mux_parse(const char *buf, u16 *mux, size_t len)
-{
-	u16 tmp_mux = 0;
-	int i;
-
-	len = min_t(int, len, LP5523_MAX_LEDS);
-
-	for (i = 0; i < len; i++) {
-		switch (buf[i]) {
-		case '1':
-			tmp_mux |= (1 << i);
-			break;
-		case '0':
-			break;
-		case '\n':
-			i = len;
-			break;
-		default:
-			return -1;
-		}
-	}
-	*mux = tmp_mux;
-
-	return 0;
-}
-
-static void lp5523_mux_to_array(u16 led_mux, char *array)
-{
-	int i, pos = 0;
-
-	for (i = 0; i < LP5523_MAX_LEDS; i++)
-		pos += sprintf(array + pos, "%x", LED_ACTIVE(led_mux, i));
-
-	array[pos] = '\0';
-}
-
-static ssize_t show_engine_leds(struct device *dev,
-			    struct device_attribute *attr,
-			    char *buf, int nr)
-{
-	struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
-	struct lp55xx_chip *chip = led->chip;
-	char mux[LP5523_MAX_LEDS + 1];
-
-	lp5523_mux_to_array(chip->engines[nr - 1].led_mux, mux);
-
-	return sprintf(buf, "%s\n", mux);
-}
-show_leds(1)
-show_leds(2)
-show_leds(3)
-
-static int lp5523_load_mux(struct lp55xx_chip *chip, u16 mux, int nr)
-{
-	struct lp55xx_engine *engine = &chip->engines[nr - 1];
-	int ret;
-	static const u8 mux_page[] = {
-		[LP55XX_ENGINE_1] = LP5523_PAGE_MUX1,
-		[LP55XX_ENGINE_2] = LP5523_PAGE_MUX2,
-		[LP55XX_ENGINE_3] = LP5523_PAGE_MUX3,
-	};
-
-	lp55xx_load_engine(chip);
-
-	ret = lp55xx_write(chip, LP5523_REG_PROG_PAGE_SEL, mux_page[nr]);
-	if (ret)
-		return ret;
-
-	ret = lp55xx_write(chip, LP5523_REG_PROG_MEM, (u8)(mux >> 8));
-	if (ret)
-		return ret;
-
-	ret = lp55xx_write(chip, LP5523_REG_PROG_MEM + 1, (u8)(mux));
-	if (ret)
-		return ret;
-
-	engine->led_mux = mux;
-	return 0;
-}
-
-static ssize_t store_engine_leds(struct device *dev,
-			     struct device_attribute *attr,
-			     const char *buf, size_t len, int nr)
-{
-	struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
-	struct lp55xx_chip *chip = led->chip;
-	struct lp55xx_engine *engine = &chip->engines[nr - 1];
-	u16 mux = 0;
-	ssize_t ret;
-
-	if (lp5523_mux_parse(buf, &mux, len))
-		return -EINVAL;
-
-	mutex_lock(&chip->lock);
-
-	chip->engine_idx = nr;
-	ret = -EINVAL;
-
-	if (engine->mode != LP55XX_ENGINE_LOAD)
-		goto leave;
-
-	if (lp5523_load_mux(chip, mux, nr))
-		goto leave;
-
-	ret = len;
-leave:
-	mutex_unlock(&chip->lock);
-	return ret;
-}
-store_leds(1)
-store_leds(2)
-store_leds(3)
-
 static ssize_t lp5523_selftest(struct device *dev,
 			       struct device_attribute *attr,
 			       char *buf)
@@ -562,9 +449,9 @@ static ssize_t store_master_fader_leds(struct device *dev,
 LP55XX_DEV_ATTR_ENGINE_MODE(1);
 LP55XX_DEV_ATTR_ENGINE_MODE(2);
 LP55XX_DEV_ATTR_ENGINE_MODE(3);
-static LP55XX_DEV_ATTR_RW(engine1_leds, show_engine1_leds, store_engine1_leds);
-static LP55XX_DEV_ATTR_RW(engine2_leds, show_engine2_leds, store_engine2_leds);
-static LP55XX_DEV_ATTR_RW(engine3_leds, show_engine3_leds, store_engine3_leds);
+LP55XX_DEV_ATTR_ENGINE_LEDS(1);
+LP55XX_DEV_ATTR_ENGINE_LEDS(2);
+LP55XX_DEV_ATTR_ENGINE_LEDS(3);
 LP55XX_DEV_ATTR_ENGINE_LOAD(1);
 LP55XX_DEV_ATTR_ENGINE_LOAD(2);
 LP55XX_DEV_ATTR_ENGINE_LOAD(3);
drivers/leds/leds-lp55xx-common.c +109 -0

121 lines not on this page · raw message

drivers/leds/leds-lp55xx-common.h +19 -13
index 5b3e9473cadc..5f2394a6de15 100644
@@ -51,21 +51,21 @@ static ssize_t store_engine##nr##_mode(struct device *dev,		\
 static LP55XX_DEV_ATTR_RW(engine##nr##_mode, show_engine##nr##_mode,	\
 			  store_engine##nr##_mode)
 
-#define show_leds(nr)							\
+#define LP55XX_DEV_ATTR_ENGINE_LEDS(nr)					\
 static ssize_t show_engine##nr##_leds(struct device *dev,		\
-			    struct device_attribute *attr,		\
-			    char *buf)					\
+				      struct device_attribute *attr,	\
+				      char *buf)			\
 {									\
-	return show_engine_leds(dev, attr, buf, nr);			\
-}
-
-#define store_leds(nr)						\
-static ssize_t store_engine##nr##_leds(struct device *dev,	\
-			     struct device_attribute *attr,	\
-			     const char *buf, size_t len)	\
-{								\
-	return store_engine_leds(dev, attr, buf, len, nr);	\
-}
+	return lp55xx_show_engine_leds(dev, attr, buf, nr);		\
+}									\
+static ssize_t store_engine##nr##_leds(struct device *dev,		\
+				       struct device_attribute *attr,	\
+				       const char *buf, size_t len)	\
+{									\
+	return lp55xx_store_engine_leds(dev, attr, buf, len, nr);	\
+}									\
+static LP55XX_DEV_ATTR_RW(engine##nr##_leds, show_engine##nr##_leds,	\
+			  store_engine##nr##_leds)
 
 #define LP55XX_DEV_ATTR_ENGINE_LOAD(nr)					\
 static ssize_t store_engine##nr##_load(struct device *dev,		\
@@ -238,5 +238,11 @@ extern ssize_t lp55xx_store_engine_mode(struct device *dev,
 extern ssize_t lp55xx_store_engine_load(struct device *dev,
 					struct device_attribute *attr,
 					const char *buf, size_t len, int nr);
+extern ssize_t lp55xx_show_engine_leds(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf, int nr);
+extern ssize_t lp55xx_store_engine_leds(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t len, int nr);
 
 #endif /* _LEDS_LP55XX_COMMON_H */
Signature · 1 line
2.45.1
#18 lore raw

PATCH v7 17/20 leds: leds-lp55xx: Generalize sysfs master_fader

In reply to Christian Marangi

Generalize sysfs master_fader since their implementation is the same across some lp55xx based LED driver.

---
 drivers/leds/leds-lp5523.c        | 150 +++---------------------------
 drivers/leds/leds-lp55xx-common.c | 113 ++++++++++++++++++++++
 drivers/leds/leds-lp55xx-common.h |  32 +++++++
 3 files changed, 156 insertions(+), 139 deletions(-)
drivers/leds/leds-lp5523.c +11 -139
index 9d91c2c5a3eb..1b3ffdc3dfa3 100644
@@ -315,137 +315,6 @@ static ssize_t lp5523_selftest(struct device *dev,
 	return pos;
 }
 
-#define show_fader(nr)						\
-static ssize_t show_master_fader##nr(struct device *dev,	\
-			    struct device_attribute *attr,	\
-			    char *buf)				\
-{								\
-	return show_master_fader(dev, attr, buf, nr);		\
-}
-
-#define store_fader(nr)						\
-static ssize_t store_master_fader##nr(struct device *dev,	\
-			     struct device_attribute *attr,	\
-			     const char *buf, size_t len)	\
-{								\
-	return store_master_fader(dev, attr, buf, len, nr);	\
-}
-
-static ssize_t show_master_fader(struct device *dev,
-				 struct device_attribute *attr,
-				 char *buf, int nr)
-{
-	struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
-	struct lp55xx_chip *chip = led->chip;
-	int ret;
-	u8 val;
-
-	mutex_lock(&chip->lock);
-	ret = lp55xx_read(chip, LP5523_REG_MASTER_FADER_BASE + nr - 1, &val);
-	mutex_unlock(&chip->lock);
-
-	if (ret == 0)
-		ret = sprintf(buf, "%u\n", val);
-
-	return ret;
-}
-show_fader(1)
-show_fader(2)
-show_fader(3)
-
-static ssize_t store_master_fader(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf, size_t len, int nr)
-{
-	struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
-	struct lp55xx_chip *chip = led->chip;
-	int ret;
-	unsigned long val;
-
-	if (kstrtoul(buf, 0, &val))
-		return -EINVAL;
-
-	if (val > 0xff)
-		return -EINVAL;
-
-	mutex_lock(&chip->lock);
-	ret = lp55xx_write(chip, LP5523_REG_MASTER_FADER_BASE + nr - 1,
-			   (u8)val);
-	mutex_unlock(&chip->lock);
-
-	if (ret == 0)
-		ret = len;
-
-	return ret;
-}
-store_fader(1)
-store_fader(2)
-store_fader(3)
-
-static ssize_t show_master_fader_leds(struct device *dev,
-				      struct device_attribute *attr,
-				      char *buf)
-{
-	struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
-	struct lp55xx_chip *chip = led->chip;
-	int i, ret, pos = 0;
-	u8 val;
-
-	mutex_lock(&chip->lock);
-
-	for (i = 0; i < LP5523_MAX_LEDS; i++) {
-		ret = lp55xx_read(chip, LP5523_REG_LED_CTRL_BASE + i, &val);
-		if (ret)
-			goto leave;
-
-		val = (val & LP5523_FADER_MAPPING_MASK)
-			>> LP5523_FADER_MAPPING_SHIFT;
-		if (val > 3) {
-			ret = -EINVAL;
-			goto leave;
-		}
-		buf[pos++] = val + '0';
-	}
-	buf[pos++] = '\n';
-	ret = pos;
-leave:
-	mutex_unlock(&chip->lock);
-	return ret;
-}
-
-static ssize_t store_master_fader_leds(struct device *dev,
-				       struct device_attribute *attr,
-				       const char *buf, size_t len)
-{
-	struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
-	struct lp55xx_chip *chip = led->chip;
-	int i, n, ret;
-	u8 val;
-
-	n = min_t(int, len, LP5523_MAX_LEDS);
-
-	mutex_lock(&chip->lock);
-
-	for (i = 0; i < n; i++) {
-		if (buf[i] >= '0' && buf[i] <= '3') {
-			val = (buf[i] - '0') << LP5523_FADER_MAPPING_SHIFT;
-			ret = lp55xx_update_bits(chip,
-						 LP5523_REG_LED_CTRL_BASE + i,
-						 LP5523_FADER_MAPPING_MASK,
-						 val);
-			if (ret)
-				goto leave;
-		} else {
-			ret = -EINVAL;
-			goto leave;
-		}
-	}
-	ret = len;
-leave:
-	mutex_unlock(&chip->lock);
-	return ret;
-}
-
 LP55XX_DEV_ATTR_ENGINE_MODE(1);
 LP55XX_DEV_ATTR_ENGINE_MODE(2);
 LP55XX_DEV_ATTR_ENGINE_MODE(3);
@@ -456,14 +325,11 @@ LP55XX_DEV_ATTR_ENGINE_LOAD(1);
 LP55XX_DEV_ATTR_ENGINE_LOAD(2);
 LP55XX_DEV_ATTR_ENGINE_LOAD(3);
 static LP55XX_DEV_ATTR_RO(selftest, lp5523_selftest);
-static LP55XX_DEV_ATTR_RW(master_fader1, show_master_fader1,
-			  store_master_fader1);
-static LP55XX_DEV_ATTR_RW(master_fader2, show_master_fader2,
-			  store_master_fader2);
-static LP55XX_DEV_ATTR_RW(master_fader3, show_master_fader3,
-			  store_master_fader3);
-static LP55XX_DEV_ATTR_RW(master_fader_leds, show_master_fader_leds,
-			  store_master_fader_leds);
+LP55XX_DEV_ATTR_MASTER_FADER(1);
+LP55XX_DEV_ATTR_MASTER_FADER(2);
+LP55XX_DEV_ATTR_MASTER_FADER(3);
+static LP55XX_DEV_ATTR_RW(master_fader_leds, lp55xx_show_master_fader_leds,
+			  lp55xx_store_master_fader_leds);
 
 static struct attribute *lp5523_attributes[] = {
 	&dev_attr_engine1_mode.attr,
@@ -516,6 +382,12 @@ static struct lp55xx_device_config lp5523_cfg = {
 	.reg_led_current_base = {
 		.addr = LP5523_REG_LED_CURRENT_BASE,
 	},
+	.reg_master_fader_base = {
+		.addr = LP5523_REG_MASTER_FADER_BASE,
+	},
+	.reg_led_ctrl_base = {
+		.addr = LP5523_REG_LED_CTRL_BASE,
+	},
 	.pages_per_engine   = LP5523_PAGES_PER_ENGINE,
 	.max_channel  = LP5523_MAX_LEDS,
 	.post_init_device   = lp5523_post_init_device,
drivers/leds/leds-lp55xx-common.c +113 -0

125 lines not on this page · raw message

drivers/leds/leds-lp55xx-common.h +32 -0
index 5f2394a6de15..6dcffa0db647 100644
@@ -76,6 +76,22 @@ static ssize_t store_engine##nr##_load(struct device *dev,		\
 }									\
 static LP55XX_DEV_ATTR_WO(engine##nr##_load, store_engine##nr##_load)
 
+#define LP55XX_DEV_ATTR_MASTER_FADER(nr)				\
+static ssize_t show_master_fader##nr(struct device *dev,		\
+				     struct device_attribute *attr,	\
+				     char *buf)				\
+{									\
+	return lp55xx_show_master_fader(dev, attr, buf, nr);		\
+}									\
+static ssize_t store_master_fader##nr(struct device *dev,		\
+				      struct device_attribute *attr,	\
+				      const char *buf, size_t len)	\
+{									\
+	return lp55xx_store_master_fader(dev, attr, buf, len, nr);	\
+}									\
+static LP55XX_DEV_ATTR_RW(master_fader##nr, show_master_fader##nr,	\
+			  store_master_fader##nr)
+
 struct lp55xx_led;
 struct lp55xx_chip;
 
@@ -103,6 +119,8 @@ struct lp55xx_reg {
  * @prog_mem_base      : Chip specific base reg address for chip SMEM programming
  * @reg_led_pwm_base   : Chip specific base reg address for LED PWM conf
  * @reg_led_current_base : Chip specific base reg address for LED current conf
+ * @reg_master_fader_base : Chip specific base reg address for master fader base
+ * @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)
  * @max_channel        : Maximum number of channels
@@ -123,6 +141,8 @@ struct lp55xx_device_config {
 	const struct lp55xx_reg prog_mem_base;
 	const struct lp55xx_reg reg_led_pwm_base;
 	const struct lp55xx_reg reg_led_current_base;
+	const struct lp55xx_reg reg_master_fader_base;
+	const struct lp55xx_reg reg_led_ctrl_base;
 	const int pages_per_engine;
 	const int max_channel;
 
@@ -244,5 +264,17 @@ extern ssize_t lp55xx_show_engine_leds(struct device *dev,
 extern ssize_t lp55xx_store_engine_leds(struct device *dev,
 					struct device_attribute *attr,
 					const char *buf, size_t len, int nr);
+extern ssize_t lp55xx_show_master_fader(struct device *dev,
+					struct device_attribute *attr,
+					char *buf, int nr);
+extern ssize_t lp55xx_store_master_fader(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t len, int nr);
+extern ssize_t lp55xx_show_master_fader_leds(struct device *dev,
+					     struct device_attribute *attr,
+					     char *buf);
+extern ssize_t lp55xx_store_master_fader_leds(struct device *dev,
+					      struct device_attribute *attr,
+					      const char *buf, size_t len);
 
 #endif /* _LEDS_LP55XX_COMMON_H */
Signature · 1 line
2.45.1
#19 lore raw

PATCH v7 18/20 leds: leds-lp55xx: Support ENGINE program up to 128 bytes

In reply to Christian Marangi

Some LED chip supports up to 16 pages and with some magic they can be divided in 4 page for each ENGINE + 1 for each MUX. Following this we can support bigger programs up to 128 bytes.

Rework the update_program_memory function to support program of multiple pages instead of hardcoding it to one page per programs.

---
 drivers/leds/leds-lp5523.c        |  5 ++-
 drivers/leds/leds-lp5562.c        |  7 ++--
 drivers/leds/leds-lp55xx-common.c | 54 ++++++++++++++++++++++++-------
 drivers/leds/leds-lp55xx-common.h |  2 ++
 4 files changed, 49 insertions(+), 19 deletions(-)
drivers/leds/leds-lp5523.c +2 -3
index 1b3ffdc3dfa3..54ead09488a5 100644
@@ -21,7 +21,6 @@
 
 #include "leds-lp55xx-common.h"
 
-#define LP5523_PROGRAM_LENGTH		32	/* bytes */
 /* Memory is used like this:
  * 0x00 engine 1 program
  * 0x10 engine 2 program
@@ -172,7 +171,7 @@ static int lp5523_init_program_engine(struct lp55xx_chip *chip)
 	int ret;
 	u8 status;
 	/* one pattern per engine setting LED MUX start and stop addresses */
-	static const u8 pattern[][LP5523_PROGRAM_LENGTH] =  {
+	static const u8 pattern[][LP55xx_BYTES_PER_PAGE] =  {
 		{ 0x9c, 0x30, 0x9c, 0xb0, 0x9d, 0x80, 0xd8, 0x00, 0},
 		{ 0x9c, 0x40, 0x9c, 0xc0, 0x9d, 0x80, 0xd8, 0x00, 0},
 		{ 0x9c, 0x50, 0x9c, 0xd0, 0x9d, 0x80, 0xd8, 0x00, 0},
@@ -196,7 +195,7 @@ static int lp5523_init_program_engine(struct lp55xx_chip *chip)
 		chip->engine_idx = i;
 		lp55xx_load_engine(chip);
 
-		for (j = 0; j < LP5523_PROGRAM_LENGTH; j++) {
+		for (j = 0; j < LP55xx_BYTES_PER_PAGE; j++) {
 			ret = lp55xx_write(chip, LP5523_REG_PROG_MEM + j,
 					pattern[i - 1][j]);
 			if (ret)
drivers/leds/leds-lp5562.c +3 -4
index e50b68c9ccf3..109162f1720f 100644
@@ -19,7 +19,6 @@
 
 #include "leds-lp55xx-common.h"
 
-#define LP5562_PROGRAM_LENGTH		32
 #define LP5562_MAX_LEDS			4
 
 /* ENABLE Register 00h */
@@ -212,9 +211,9 @@ static void lp5562_write_program_memory(struct lp55xx_chip *chip,
 /* check the size of program count */
 static inline bool _is_pc_overflow(struct lp55xx_predef_pattern *ptn)
 {
-	return ptn->size_r >= LP5562_PROGRAM_LENGTH ||
-	       ptn->size_g >= LP5562_PROGRAM_LENGTH ||
-	       ptn->size_b >= LP5562_PROGRAM_LENGTH;
+	return ptn->size_r >= LP55xx_BYTES_PER_PAGE ||
+	       ptn->size_g >= LP55xx_BYTES_PER_PAGE ||
+	       ptn->size_b >= LP55xx_BYTES_PER_PAGE;
 }
 
 static int lp5562_run_predef_led_pattern(struct lp55xx_chip *chip, int mode)
drivers/leds/leds-lp55xx-common.c +42 -12

99 lines not on this page · raw message

drivers/leds/leds-lp55xx-common.h +2 -0
index 6dcffa0db647..1bb7c559662c 100644
@@ -14,6 +14,8 @@
 
 #include <linux/led-class-multicolor.h>
 
+#define LP55xx_BYTES_PER_PAGE		32      /* bytes */
+
 enum lp55xx_engine_index {
 	LP55XX_ENGINE_INVALID,
 	LP55XX_ENGINE_1,
Signature · 1 line
2.45.1
#20 lore raw

PATCH v7 19/20 leds: leds-lp55xx: Drop deprecated defines

In reply to Christian Marangi

Drop deprecated defines not used anymore as the related function got moved to lp55xx-common.

---
 drivers/leds/leds-lp5521.c | 24 --------------------
 drivers/leds/leds-lp5523.c | 46 --------------------------------------
 drivers/leds/leds-lp5562.c | 15 -------------
 drivers/leds/leds-lp8501.c | 31 -------------------------
 4 files changed, 116 deletions(-)
drivers/leds/leds-lp5521.c +0 -24
index 519e7627ac22..de0f8ea48eba 100644
@@ -21,7 +21,6 @@
 
 #include "leds-lp55xx-common.h"
 
-#define LP5521_PROGRAM_LENGTH		32
 #define LP5521_MAX_LEDS			3
 #define LP5521_CMD_DIRECT		0x3F
 
@@ -73,29 +72,6 @@
 /* Reset register value */
 #define LP5521_RESET			0xFF
 
-/* Program Memory Operations */
-#define LP5521_MODE_R_M			0x30	/* Operation Mode Register */
-#define LP5521_MODE_G_M			0x0C
-#define LP5521_MODE_B_M			0x03
-#define LP5521_LOAD_R			0x10
-#define LP5521_LOAD_G			0x04
-#define LP5521_LOAD_B			0x01
-
-#define LP5521_R_IS_LOADING(mode)	\
-	((mode & LP5521_MODE_R_M) == LP5521_LOAD_R)
-#define LP5521_G_IS_LOADING(mode)	\
-	((mode & LP5521_MODE_G_M) == LP5521_LOAD_G)
-#define LP5521_B_IS_LOADING(mode)	\
-	((mode & LP5521_MODE_B_M) == LP5521_LOAD_B)
-
-#define LP5521_EXEC_R_M			0x30	/* Enable Register */
-#define LP5521_EXEC_G_M			0x0C
-#define LP5521_EXEC_B_M			0x03
-#define LP5521_EXEC_M			0x3F
-#define LP5521_RUN_R			0x20
-#define LP5521_RUN_G			0x08
-#define LP5521_RUN_B			0x02
-
 static inline void lp5521_wait_opmode_done(void)
 {
 	/* operation mode change needs to be longer than 153 us */
drivers/leds/leds-lp5523.c +0 -46

54 lines not on this page · raw message

drivers/leds/leds-lp5562.c +0 -15
index 109162f1720f..6ba5dbb9cace 100644
@@ -37,21 +37,6 @@
 
 /* OPMODE Register 01h */
 #define LP5562_REG_OP_MODE		0x01
-#define LP5562_MODE_ENG1_M		0x30
-#define LP5562_MODE_ENG2_M		0x0C
-#define LP5562_MODE_ENG3_M		0x03
-#define LP5562_LOAD_ENG1		0x10
-#define LP5562_LOAD_ENG2		0x04
-#define LP5562_LOAD_ENG3		0x01
-#define LP5562_RUN_ENG1			0x20
-#define LP5562_RUN_ENG2			0x08
-#define LP5562_RUN_ENG3			0x02
-#define LP5562_ENG1_IS_LOADING(mode)	\
-	((mode & LP5562_MODE_ENG1_M) == LP5562_LOAD_ENG1)
-#define LP5562_ENG2_IS_LOADING(mode)	\
-	((mode & LP5562_MODE_ENG2_M) == LP5562_LOAD_ENG2)
-#define LP5562_ENG3_IS_LOADING(mode)	\
-	((mode & LP5562_MODE_ENG3_M) == LP5562_LOAD_ENG3)
 
 /* BRIGHTNESS Registers */
 #define LP5562_REG_R_PWM		0x04
drivers/leds/leds-lp8501.c +0 -31
index 1fb876f64cb7..ee4ff4586bc0 100644
@@ -20,28 +20,14 @@
 
 #include "leds-lp55xx-common.h"
 
-#define LP8501_PROGRAM_LENGTH		32
 #define LP8501_PAGES_PER_ENGINE		1
 #define LP8501_MAX_LEDS			9
 
 /* Registers */
 #define LP8501_REG_ENABLE		0x00
 #define LP8501_ENABLE			BIT(6)
-#define LP8501_EXEC_M			0x3F
-#define LP8501_EXEC_ENG1_M		0x30
-#define LP8501_EXEC_ENG2_M		0x0C
-#define LP8501_EXEC_ENG3_M		0x03
-#define LP8501_RUN_ENG1			0x20
-#define LP8501_RUN_ENG2			0x08
-#define LP8501_RUN_ENG3			0x02
 
 #define LP8501_REG_OP_MODE		0x01
-#define LP8501_MODE_ENG1_M		0x30
-#define LP8501_MODE_ENG2_M		0x0C
-#define LP8501_MODE_ENG3_M		0x03
-#define LP8501_LOAD_ENG1		0x10
-#define LP8501_LOAD_ENG2		0x04
-#define LP8501_LOAD_ENG3		0x01
 
 #define LP8501_REG_PWR_CONFIG		0x05
 #define LP8501_PWR_CONFIG_M		0x03
@@ -65,25 +51,8 @@
 #define LP8501_REG_RESET		0x3D
 #define LP8501_RESET			0xFF
 
-#define LP8501_REG_PROG_PAGE_SEL	0x4F
-#define LP8501_PAGE_ENG1		0
-#define LP8501_PAGE_ENG2		1
-#define LP8501_PAGE_ENG3		2
-
 #define LP8501_REG_PROG_MEM		0x50
 
-#define LP8501_ENG1_IS_LOADING(mode)	\
-	((mode & LP8501_MODE_ENG1_M) == LP8501_LOAD_ENG1)
-#define LP8501_ENG2_IS_LOADING(mode)	\
-	((mode & LP8501_MODE_ENG2_M) == LP8501_LOAD_ENG2)
-#define LP8501_ENG3_IS_LOADING(mode)	\
-	((mode & LP8501_MODE_ENG3_M) == LP8501_LOAD_ENG3)
-
-static inline void lp8501_wait_opmode_done(void)
-{
-	usleep_range(1000, 2000);
-}
-
 static int lp8501_post_init_device(struct lp55xx_chip *chip)
 {
 	int ret;
Signature · 1 line
2.45.1
#21 lore raw

PATCH v7 20/20 leds: leds-lp5569: Add support for Texas Instruments LP5569

In reply to Christian Marangi

Add support for Texas Instruments LP5569 LED driver.

Texas Instruments LP5569 is 9 channels chip with programmable engines.

It almost a copy of LP5523 with fundamental changes to regs order and regs content.

Has difference in how the clock is handled and doesn't support detecting clock time automatically, different handling for selftest and different scheme for the status regs.

LED chip supports ENGINE and MUX to group LED and run precompiled code with magic values to run patterns. This is loaded via the sysfs entry and it's passed as a string of ASCII HEX char.

One some devices using this LED Controller (a NBG7815 Router) it was found loading big precompiled pattern with up to 96 bytes of code. To have support for this "extended" scenario, hardcode each engine to support 4 pages of precompiled pattern (128 bytes of code) and 1 page for each MUX. This gives plenty of space for any kind precompiled pattern keeping simple logic for page handling of each engine and mux.

---
 drivers/leds/Kconfig       |  16 +-
 drivers/leds/Makefile      |   1 +
 drivers/leds/leds-lp5569.c | 544 +++++++++++++++++++++++++++++++++++++
 3 files changed, 558 insertions(+), 3 deletions(-)
 create mode 100644 drivers/leds/leds-lp5569.c
drivers/leds/Kconfig +13 -3
index 05e6af88b88c..b1d7d94317b3 100644
@@ -414,7 +414,7 @@ config LEDS_LP50XX
 	  module will be called leds-lp50xx.
 
 config LEDS_LP55XX_COMMON
-	tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501"
+	tristate "Common Driver for TI/National LP5521/5523/55231/5562/5569/8501"
 	depends on LEDS_CLASS
 	depends on LEDS_CLASS_MULTICOLOR
 	depends on OF
@@ -422,8 +422,8 @@ config LEDS_LP55XX_COMMON
 	select FW_LOADER
 	select FW_LOADER_USER_HELPER
 	help
-	  This option supports common operations for LP5521/5523/55231/5562/8501
-	  devices.
+	  This option supports common operations for LP5521/5523/55231/5562/5569/
+	  8501 devices.
 
 config LEDS_LP5521
 	tristate "LED Support for N.S. LP5521 LED driver chip"
@@ -456,6 +456,16 @@ config LEDS_LP5562
 	  Driver provides direct control via LED class and interface for
 	  programming the engines.
 
+config LEDS_LP5569
+	tristate "LED Support for TI LP5569 LED driver chip"
+	depends on LEDS_CLASS && I2C
+	depends on LEDS_LP55XX_COMMON
+	help
+	  If you say yes here you get support for TI LP5569 LED driver.
+	  It is 9 channels chip with programmable engines.
+	  Driver provides direct control via LED class and interface for
+	  programming the engines.
+
 config LEDS_LP8501
 	tristate "LED Support for TI LP8501 LED driver chip"
 	depends on LEDS_CLASS && I2C
drivers/leds/Makefile +1 -0
index effdfc6f1e95..ad21941efa19 100644
@@ -51,6 +51,7 @@ obj-$(CONFIG_LEDS_LP50XX)		+= leds-lp50xx.o
 obj-$(CONFIG_LEDS_LP5521)		+= leds-lp5521.o
 obj-$(CONFIG_LEDS_LP5523)		+= leds-lp5523.o
 obj-$(CONFIG_LEDS_LP5562)		+= leds-lp5562.o
+obj-$(CONFIG_LEDS_LP5569)		+= leds-lp5569.o
 obj-$(CONFIG_LEDS_LP55XX_COMMON)	+= leds-lp55xx-common.o
 obj-$(CONFIG_LEDS_LP8501)		+= leds-lp8501.o
 obj-$(CONFIG_LEDS_LP8788)		+= leds-lp8788.o
drivers/leds/leds-lp5569.c new file +544 -0

544 lines not on this page · raw message

Signature · 1 line
2.45.1
maintainer
#22 lore raw
In reply to Christian Marangi
Quoted 82 lines · Christian Marangi

This long series is (as requested) a big overhaul of the lp55xx based LED driver.

As notice for these kind of LED chip there was the bad habit of copy the old driver and just modify it enough to make it work with the new model. Till v4 I was also doing the same by following the pattern and the code format of previous driver.

Since Lee didn't like this, here is the BIG series that generalize pretty much anything in the 4 model currently supported.

Indeed, although the LED chip have fundamental difference (page support), things can be generalized and produce slimmer drivers by putting everything in the lp55xx-common shared module.

This result in the new model lp5569 being very small with only the selftest portion to be custom.

Lee also wasn't clear by the meaning of ENGINE in these LED driver, so here some simple explaination. This is very common on these TI LED chip. The ENGINE (there are always 3) is just some kind of processor that execute a program (precompiled code ASM like) loaded in the SRAM. Sysfs is used to load the pattern, and to start and stop the engine.

These pattern can do all kind of complex thing with LEDs. Old LED chip had 32bytes of space for the pattern but newer one (like lp5569) have pages and if correctly configured can have massive pattern. These pattern can do all kind of magic like loops that make the LED pulse, change color and all kind of stuff.

(For Lee, sorry if you will have to repeat some review that I might
have missed in the lp5569 driver)

Changes v7:
- Add Suggested-by tag
- Fix checkpatch error for complex macro (rework define)
- Add missing values for fader conversion
- Align some function with redundant new line
- Capitalize every commit title
Changes v6:
- Fix compilation warning for ret unused in read_poll_timeout (no idea why this is flagged only on some particular arch...)
- Fix missing bitfield.h in lp55x-common.c (again it seems this header gets included in the flow if the arch use them or not...)
Changes v5:
- Big generalization patch
- Rework lp5569 driver with new generalized functions
- Drop all copyright header in lp5569 as the driver got reworked entirely and it's not based on previous one anymore.
Changes v4:
- Fix reported buffer overflow due to a copypaste error
- Add comments to describe fw size logic
Changes v3:
- Add ACK tag to DT patch
- Enlarge and support program size up to 128bytes Changes v2:
- Add ACK tag to DT patch
- Fix compilation error with target that doesn't include bitfield.h

Christian Marangi (20):
dt-bindings: leds-lp55xx: Limit pwr-sel property to ti,lp8501
dt-bindings: leds-lp55xx: Add new ti,lp5569 compatible
leds: leds-lp55xx: Generalize stop_all_engine OP
leds: leds-lp55xx: Generalize probe/remove functions
leds: leds-lp55xx: Generalize load_engine function
leds: leds-lp55xx: Generalize load_engine_and_select_page function
leds: leds-lp55xx: Generalize run_engine function
leds: leds-lp55xx: Generalize update_program_memory function
leds: leds-lp55xx: Generalize firmware_loaded function
leds: leds-lp55xx: Generalize led_brightness function
leds: leds-lp55xx: Generalize multicolor_brightness function
leds: leds-lp55xx: Generalize set_led_current function
leds: leds-lp55xx: Generalize turn_off_channels function
leds: leds-lp55xx: Generalize stop_engine function
leds: leds-lp55xx: Generalize sysfs engine_load and engine_mode
leds: leds-lp55xx: Generalize sysfs engine_leds
leds: leds-lp55xx: Generalize sysfs master_fader
leds: leds-lp55xx: Support ENGINE program up to 128 bytes
leds: leds-lp55xx: Drop deprecated defines
leds: leds-lp5569: Add support for Texas Instruments LP5569

Quoted 12 lines
 .../devicetree/bindings/leds/leds-lp55xx.yaml |  11 +
 drivers/leds/Kconfig                          |  16 +-
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-lp5521.c                    | 405 +---------
 drivers/leds/leds-lp5523.c                    | 734 ++---------------
 drivers/leds/leds-lp5562.c                    | 261 +-----
 drivers/leds/leds-lp5569.c                    | 544 +++++++++++++
 drivers/leds/leds-lp55xx-common.c             | 743 +++++++++++++++++-
 drivers/leds/leds-lp55xx-common.h             | 163 ++--
 drivers/leds/leds-lp8501.c                    | 313 +-------
 10 files changed, 1540 insertions(+), 1651 deletions(-)
 create mode 100644 drivers/leds/leds-lp5569.c

For whatever reason, this no longer applies.

Please rebase it on Linux -next or for-leds-next and resubmit.

Signature · 1 line
Lee Jones [李琼斯]
#23 lore raw
Quoted 97 lines · Lee Jones
Quoted 82 lines · Christian Marangi

This long series is (as requested) a big overhaul of the lp55xx based LED driver.

As notice for these kind of LED chip there was the bad habit of copy the old driver and just modify it enough to make it work with the new model. Till v4 I was also doing the same by following the pattern and the code format of previous driver.

Since Lee didn't like this, here is the BIG series that generalize pretty much anything in the 4 model currently supported.

Indeed, although the LED chip have fundamental difference (page support), things can be generalized and produce slimmer drivers by putting everything in the lp55xx-common shared module.

This result in the new model lp5569 being very small with only the selftest portion to be custom.

Lee also wasn't clear by the meaning of ENGINE in these LED driver, so here some simple explaination. This is very common on these TI LED chip. The ENGINE (there are always 3) is just some kind of processor that execute a program (precompiled code ASM like) loaded in the SRAM. Sysfs is used to load the pattern, and to start and stop the engine.

These pattern can do all kind of complex thing with LEDs. Old LED chip had 32bytes of space for the pattern but newer one (like lp5569) have pages and if correctly configured can have massive pattern. These pattern can do all kind of magic like loops that make the LED pulse, change color and all kind of stuff.

(For Lee, sorry if you will have to repeat some review that I might
have missed in the lp5569 driver)

Changes v7:
- Add Suggested-by tag
- Fix checkpatch error for complex macro (rework define)
- Add missing values for fader conversion
- Align some function with redundant new line
- Capitalize every commit title
Changes v6:
- Fix compilation warning for ret unused in read_poll_timeout (no idea why this is flagged only on some particular arch...)
- Fix missing bitfield.h in lp55x-common.c (again it seems this header gets included in the flow if the arch use them or not...)
Changes v5:
- Big generalization patch
- Rework lp5569 driver with new generalized functions
- Drop all copyright header in lp5569 as the driver got reworked entirely and it's not based on previous one anymore.
Changes v4:
- Fix reported buffer overflow due to a copypaste error
- Add comments to describe fw size logic
Changes v3:
- Add ACK tag to DT patch
- Enlarge and support program size up to 128bytes Changes v2:
- Add ACK tag to DT patch
- Fix compilation error with target that doesn't include bitfield.h

Christian Marangi (20):
dt-bindings: leds-lp55xx: Limit pwr-sel property to ti,lp8501
dt-bindings: leds-lp55xx: Add new ti,lp5569 compatible
leds: leds-lp55xx: Generalize stop_all_engine OP
leds: leds-lp55xx: Generalize probe/remove functions
leds: leds-lp55xx: Generalize load_engine function
leds: leds-lp55xx: Generalize load_engine_and_select_page function
leds: leds-lp55xx: Generalize run_engine function
leds: leds-lp55xx: Generalize update_program_memory function
leds: leds-lp55xx: Generalize firmware_loaded function
leds: leds-lp55xx: Generalize led_brightness function
leds: leds-lp55xx: Generalize multicolor_brightness function
leds: leds-lp55xx: Generalize set_led_current function
leds: leds-lp55xx: Generalize turn_off_channels function
leds: leds-lp55xx: Generalize stop_engine function
leds: leds-lp55xx: Generalize sysfs engine_load and engine_mode
leds: leds-lp55xx: Generalize sysfs engine_leds
leds: leds-lp55xx: Generalize sysfs master_fader
leds: leds-lp55xx: Support ENGINE program up to 128 bytes
leds: leds-lp55xx: Drop deprecated defines
leds: leds-lp5569: Add support for Texas Instruments LP5569

Quoted 12 lines
 .../devicetree/bindings/leds/leds-lp55xx.yaml |  11 +
 drivers/leds/Kconfig                          |  16 +-
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-lp5521.c                    | 405 +---------
 drivers/leds/leds-lp5523.c                    | 734 ++---------------
 drivers/leds/leds-lp5562.c                    | 261 +-----
 drivers/leds/leds-lp5569.c                    | 544 +++++++++++++
 drivers/leds/leds-lp55xx-common.c             | 743 +++++++++++++++++-
 drivers/leds/leds-lp55xx-common.h             | 163 ++--
 drivers/leds/leds-lp8501.c                    | 313 +-------
 10 files changed, 1540 insertions(+), 1651 deletions(-)
 create mode 100644 drivers/leds/leds-lp5569.c
Quoted 4 lines

For whatever reason, this no longer applies.

Please rebase it on Linux -next or for-leds-next and resubmit.

Hi Lee.,

I just checked and yes there is to rebase the series due to c0e3d2beeb03 ("leds: Drop explicit initialization of struct i2c_device_id::driver_data to 0")

But I notice that something went bad in reverting some commits from the v7 series and in for-leds-next there are still 3 commit to drop

mainly
- 3bed6364ffd0057bd10c081d2e24aa7e8555e0a8
- 3bcc6d6ad7821aa459826592f830d76a3dc6f79f
- 5a402b40c651baf758ad884aed36c0ea70cd8b1e

I'm sending v8 with everything rebased but please keep in mind that those commits needs to be reverted as the leds one also somehow got half applied (no idea what happen)

Signature · 1 line
	Ansuel
maintainer
#24 lore raw
Quoted 112 lines · Christian Marangi
Quoted 97 lines · Lee Jones
Quoted 82 lines · Christian Marangi

This long series is (as requested) a big overhaul of the lp55xx based LED driver.

As notice for these kind of LED chip there was the bad habit of copy the old driver and just modify it enough to make it work with the new model. Till v4 I was also doing the same by following the pattern and the code format of previous driver.

Since Lee didn't like this, here is the BIG series that generalize pretty much anything in the 4 model currently supported.

Indeed, although the LED chip have fundamental difference (page support), things can be generalized and produce slimmer drivers by putting everything in the lp55xx-common shared module.

This result in the new model lp5569 being very small with only the selftest portion to be custom.

Lee also wasn't clear by the meaning of ENGINE in these LED driver, so here some simple explaination. This is very common on these TI LED chip. The ENGINE (there are always 3) is just some kind of processor that execute a program (precompiled code ASM like) loaded in the SRAM. Sysfs is used to load the pattern, and to start and stop the engine.

These pattern can do all kind of complex thing with LEDs. Old LED chip had 32bytes of space for the pattern but newer one (like lp5569) have pages and if correctly configured can have massive pattern. These pattern can do all kind of magic like loops that make the LED pulse, change color and all kind of stuff.

(For Lee, sorry if you will have to repeat some review that I might
have missed in the lp5569 driver)

Changes v7:
- Add Suggested-by tag
- Fix checkpatch error for complex macro (rework define)
- Add missing values for fader conversion
- Align some function with redundant new line
- Capitalize every commit title
Changes v6:
- Fix compilation warning for ret unused in read_poll_timeout (no idea why this is flagged only on some particular arch...)
- Fix missing bitfield.h in lp55x-common.c (again it seems this header gets included in the flow if the arch use them or not...)
Changes v5:
- Big generalization patch
- Rework lp5569 driver with new generalized functions
- Drop all copyright header in lp5569 as the driver got reworked entirely and it's not based on previous one anymore.
Changes v4:
- Fix reported buffer overflow due to a copypaste error
- Add comments to describe fw size logic
Changes v3:
- Add ACK tag to DT patch
- Enlarge and support program size up to 128bytes Changes v2:
- Add ACK tag to DT patch
- Fix compilation error with target that doesn't include bitfield.h

Christian Marangi (20):
dt-bindings: leds-lp55xx: Limit pwr-sel property to ti,lp8501
dt-bindings: leds-lp55xx: Add new ti,lp5569 compatible
leds: leds-lp55xx: Generalize stop_all_engine OP
leds: leds-lp55xx: Generalize probe/remove functions
leds: leds-lp55xx: Generalize load_engine function
leds: leds-lp55xx: Generalize load_engine_and_select_page function
leds: leds-lp55xx: Generalize run_engine function
leds: leds-lp55xx: Generalize update_program_memory function
leds: leds-lp55xx: Generalize firmware_loaded function
leds: leds-lp55xx: Generalize led_brightness function
leds: leds-lp55xx: Generalize multicolor_brightness function
leds: leds-lp55xx: Generalize set_led_current function
leds: leds-lp55xx: Generalize turn_off_channels function
leds: leds-lp55xx: Generalize stop_engine function
leds: leds-lp55xx: Generalize sysfs engine_load and engine_mode
leds: leds-lp55xx: Generalize sysfs engine_leds
leds: leds-lp55xx: Generalize sysfs master_fader
leds: leds-lp55xx: Support ENGINE program up to 128 bytes
leds: leds-lp55xx: Drop deprecated defines
leds: leds-lp5569: Add support for Texas Instruments LP5569

Quoted 12 lines
 .../devicetree/bindings/leds/leds-lp55xx.yaml |  11 +
 drivers/leds/Kconfig                          |  16 +-
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-lp5521.c                    | 405 +---------
 drivers/leds/leds-lp5523.c                    | 734 ++---------------
 drivers/leds/leds-lp5562.c                    | 261 +-----
 drivers/leds/leds-lp5569.c                    | 544 +++++++++++++
 drivers/leds/leds-lp55xx-common.c             | 743 +++++++++++++++++-
 drivers/leds/leds-lp55xx-common.h             | 163 ++--
 drivers/leds/leds-lp8501.c                    | 313 +-------
 10 files changed, 1540 insertions(+), 1651 deletions(-)
 create mode 100644 drivers/leds/leds-lp5569.c
Quoted 4 lines

For whatever reason, this no longer applies.

Please rebase it on Linux -next or for-leds-next and resubmit.

Hi Lee.,

I just checked and yes there is to rebase the series due to c0e3d2beeb03 ("leds: Drop explicit initialization of struct i2c_device_id::driver_data to 0")

But I notice that something went bad in reverting some commits from the v7 series and in for-leds-next there are still 3 commit to drop

Quoted 8 lines

mainly
- 3bed6364ffd0057bd10c081d2e24aa7e8555e0a8
- 3bcc6d6ad7821aa459826592f830d76a3dc6f79f
- 5a402b40c651baf758ad884aed36c0ea70cd8b1e

I'm sending v8 with everything rebased but please keep in mind that those commits needs to be reverted as the leds one also somehow got half applied (no idea what happen)

This must be related to the tooling error that happened recently.

I have removed those 3 patches from the tree and re-pushed the branch.

If you need to rebase again for-leds-next is now up-to-date.

Signature · 1 line
Lee Jones [李琼斯]
maintainer
#25 lore raw
Quoted 123 lines · Lee Jones
Quoted 112 lines · Christian Marangi
Quoted 97 lines · Lee Jones
Quoted 82 lines · Christian Marangi

This long series is (as requested) a big overhaul of the lp55xx based LED driver.

As notice for these kind of LED chip there was the bad habit of copy the old driver and just modify it enough to make it work with the new model. Till v4 I was also doing the same by following the pattern and the code format of previous driver.

Since Lee didn't like this, here is the BIG series that generalize pretty much anything in the 4 model currently supported.

Indeed, although the LED chip have fundamental difference (page support), things can be generalized and produce slimmer drivers by putting everything in the lp55xx-common shared module.

This result in the new model lp5569 being very small with only the selftest portion to be custom.

Lee also wasn't clear by the meaning of ENGINE in these LED driver, so here some simple explaination. This is very common on these TI LED chip. The ENGINE (there are always 3) is just some kind of processor that execute a program (precompiled code ASM like) loaded in the SRAM. Sysfs is used to load the pattern, and to start and stop the engine.

These pattern can do all kind of complex thing with LEDs. Old LED chip had 32bytes of space for the pattern but newer one (like lp5569) have pages and if correctly configured can have massive pattern. These pattern can do all kind of magic like loops that make the LED pulse, change color and all kind of stuff.

(For Lee, sorry if you will have to repeat some review that I might
have missed in the lp5569 driver)

Changes v7:
- Add Suggested-by tag
- Fix checkpatch error for complex macro (rework define)
- Add missing values for fader conversion
- Align some function with redundant new line
- Capitalize every commit title
Changes v6:
- Fix compilation warning for ret unused in read_poll_timeout (no idea why this is flagged only on some particular arch...)
- Fix missing bitfield.h in lp55x-common.c (again it seems this header gets included in the flow if the arch use them or not...)
Changes v5:
- Big generalization patch
- Rework lp5569 driver with new generalized functions
- Drop all copyright header in lp5569 as the driver got reworked entirely and it's not based on previous one anymore.
Changes v4:
- Fix reported buffer overflow due to a copypaste error
- Add comments to describe fw size logic
Changes v3:
- Add ACK tag to DT patch
- Enlarge and support program size up to 128bytes Changes v2:
- Add ACK tag to DT patch
- Fix compilation error with target that doesn't include bitfield.h

Christian Marangi (20):
dt-bindings: leds-lp55xx: Limit pwr-sel property to ti,lp8501
dt-bindings: leds-lp55xx: Add new ti,lp5569 compatible
leds: leds-lp55xx: Generalize stop_all_engine OP
leds: leds-lp55xx: Generalize probe/remove functions
leds: leds-lp55xx: Generalize load_engine function
leds: leds-lp55xx: Generalize load_engine_and_select_page function
leds: leds-lp55xx: Generalize run_engine function
leds: leds-lp55xx: Generalize update_program_memory function
leds: leds-lp55xx: Generalize firmware_loaded function
leds: leds-lp55xx: Generalize led_brightness function
leds: leds-lp55xx: Generalize multicolor_brightness function
leds: leds-lp55xx: Generalize set_led_current function
leds: leds-lp55xx: Generalize turn_off_channels function
leds: leds-lp55xx: Generalize stop_engine function
leds: leds-lp55xx: Generalize sysfs engine_load and engine_mode
leds: leds-lp55xx: Generalize sysfs engine_leds
leds: leds-lp55xx: Generalize sysfs master_fader
leds: leds-lp55xx: Support ENGINE program up to 128 bytes
leds: leds-lp55xx: Drop deprecated defines
leds: leds-lp5569: Add support for Texas Instruments LP5569

Quoted 12 lines
 .../devicetree/bindings/leds/leds-lp55xx.yaml |  11 +
 drivers/leds/Kconfig                          |  16 +-
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-lp5521.c                    | 405 +---------
 drivers/leds/leds-lp5523.c                    | 734 ++---------------
 drivers/leds/leds-lp5562.c                    | 261 +-----
 drivers/leds/leds-lp5569.c                    | 544 +++++++++++++
 drivers/leds/leds-lp55xx-common.c             | 743 +++++++++++++++++-
 drivers/leds/leds-lp55xx-common.h             | 163 ++--
 drivers/leds/leds-lp8501.c                    | 313 +-------
 10 files changed, 1540 insertions(+), 1651 deletions(-)
 create mode 100644 drivers/leds/leds-lp5569.c
Quoted 4 lines

For whatever reason, this no longer applies.

Please rebase it on Linux -next or for-leds-next and resubmit.

Hi Lee.,

I just checked and yes there is to rebase the series due to c0e3d2beeb03 ("leds: Drop explicit initialization of struct i2c_device_id::driver_data to 0")

But I notice that something went bad in reverting some commits from the v7 series and in for-leds-next there are still 3 commit to drop

Quoted 8 lines

mainly
- 3bed6364ffd0057bd10c081d2e24aa7e8555e0a8
- 3bcc6d6ad7821aa459826592f830d76a3dc6f79f
- 5a402b40c651baf758ad884aed36c0ea70cd8b1e

I'm sending v8 with everything rebased but please keep in mind that those commits needs to be reverted as the leds one also somehow got half applied (no idea what happen)

Quoted 5 lines

This must be related to the tooling error that happened recently.

I have removed those 3 patches from the tree and re-pushed the branch.

If you need to rebase again for-leds-next is now up-to-date.

Okay v8 applied without issues.

Thank you.

Signature · 1 line
Lee Jones [李琼斯]
#26 lore raw
Quoted 127 lines · Lee Jones
Quoted 125 lines · Lee Jones
Quoted 112 lines · Christian Marangi
Quoted 97 lines · Lee Jones
Quoted 82 lines · Christian Marangi

This long series is (as requested) a big overhaul of the lp55xx based LED driver.

As notice for these kind of LED chip there was the bad habit of copy the old driver and just modify it enough to make it work with the new model. Till v4 I was also doing the same by following the pattern and the code format of previous driver.

Since Lee didn't like this, here is the BIG series that generalize pretty much anything in the 4 model currently supported.

Indeed, although the LED chip have fundamental difference (page support), things can be generalized and produce slimmer drivers by putting everything in the lp55xx-common shared module.

This result in the new model lp5569 being very small with only the selftest portion to be custom.

Lee also wasn't clear by the meaning of ENGINE in these LED driver, so here some simple explaination. This is very common on these TI LED chip. The ENGINE (there are always 3) is just some kind of processor that execute a program (precompiled code ASM like) loaded in the SRAM. Sysfs is used to load the pattern, and to start and stop the engine.

These pattern can do all kind of complex thing with LEDs. Old LED chip had 32bytes of space for the pattern but newer one (like lp5569) have pages and if correctly configured can have massive pattern. These pattern can do all kind of magic like loops that make the LED pulse, change color and all kind of stuff.

(For Lee, sorry if you will have to repeat some review that I might
have missed in the lp5569 driver)

Changes v7:
- Add Suggested-by tag
- Fix checkpatch error for complex macro (rework define)
- Add missing values for fader conversion
- Align some function with redundant new line
- Capitalize every commit title
Changes v6:
- Fix compilation warning for ret unused in read_poll_timeout (no idea why this is flagged only on some particular arch...)
- Fix missing bitfield.h in lp55x-common.c (again it seems this header gets included in the flow if the arch use them or not...)
Changes v5:
- Big generalization patch
- Rework lp5569 driver with new generalized functions
- Drop all copyright header in lp5569 as the driver got reworked entirely and it's not based on previous one anymore.
Changes v4:
- Fix reported buffer overflow due to a copypaste error
- Add comments to describe fw size logic
Changes v3:
- Add ACK tag to DT patch
- Enlarge and support program size up to 128bytes Changes v2:
- Add ACK tag to DT patch
- Fix compilation error with target that doesn't include bitfield.h

Christian Marangi (20):
dt-bindings: leds-lp55xx: Limit pwr-sel property to ti,lp8501
dt-bindings: leds-lp55xx: Add new ti,lp5569 compatible
leds: leds-lp55xx: Generalize stop_all_engine OP
leds: leds-lp55xx: Generalize probe/remove functions
leds: leds-lp55xx: Generalize load_engine function
leds: leds-lp55xx: Generalize load_engine_and_select_page function
leds: leds-lp55xx: Generalize run_engine function
leds: leds-lp55xx: Generalize update_program_memory function
leds: leds-lp55xx: Generalize firmware_loaded function
leds: leds-lp55xx: Generalize led_brightness function
leds: leds-lp55xx: Generalize multicolor_brightness function
leds: leds-lp55xx: Generalize set_led_current function
leds: leds-lp55xx: Generalize turn_off_channels function
leds: leds-lp55xx: Generalize stop_engine function
leds: leds-lp55xx: Generalize sysfs engine_load and engine_mode
leds: leds-lp55xx: Generalize sysfs engine_leds
leds: leds-lp55xx: Generalize sysfs master_fader
leds: leds-lp55xx: Support ENGINE program up to 128 bytes
leds: leds-lp55xx: Drop deprecated defines
leds: leds-lp5569: Add support for Texas Instruments LP5569

Quoted 12 lines
 .../devicetree/bindings/leds/leds-lp55xx.yaml |  11 +
 drivers/leds/Kconfig                          |  16 +-
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-lp5521.c                    | 405 +---------
 drivers/leds/leds-lp5523.c                    | 734 ++---------------
 drivers/leds/leds-lp5562.c                    | 261 +-----
 drivers/leds/leds-lp5569.c                    | 544 +++++++++++++
 drivers/leds/leds-lp55xx-common.c             | 743 +++++++++++++++++-
 drivers/leds/leds-lp55xx-common.h             | 163 ++--
 drivers/leds/leds-lp8501.c                    | 313 +-------
 10 files changed, 1540 insertions(+), 1651 deletions(-)
 create mode 100644 drivers/leds/leds-lp5569.c
Quoted 4 lines

For whatever reason, this no longer applies.

Please rebase it on Linux -next or for-leds-next and resubmit.

Hi Lee.,

I just checked and yes there is to rebase the series due to c0e3d2beeb03 ("leds: Drop explicit initialization of struct i2c_device_id::driver_data to 0")

But I notice that something went bad in reverting some commits from the v7 series and in for-leds-next there are still 3 commit to drop

Quoted 8 lines

mainly
- 3bed6364ffd0057bd10c081d2e24aa7e8555e0a8
- 3bcc6d6ad7821aa459826592f830d76a3dc6f79f
- 5a402b40c651baf758ad884aed36c0ea70cd8b1e

I'm sending v8 with everything rebased but please keep in mind that those commits needs to be reverted as the leds one also somehow got half applied (no idea what happen)

This must be related to the tooling error that happened recently.

Quoted 8 lines
Quoted 3 lines

I have removed those 3 patches from the tree and re-pushed the branch.

If you need to rebase again for-leds-next is now up-to-date.

Okay v8 applied without issues.

Thank you.

Happy to have this finally merged and sorry for the big series and for the mess.

Again thank to you for review and accept the changes!

Signature · 1 line
	Ansuel
maintainer Applied
#27 lore raw
In reply to Christian Marangi
Quoted 9 lines · Christian Marangi

This long series is (as requested) a big overhaul of the lp55xx based LED driver.

As notice for these kind of LED chip there was the bad habit of copy the old driver and just modify it enough to make it work with the new model. Till v4 I was also doing the same by following the pattern and the code format of previous driver.

[...]

Applied, thanks!

[01/20] dt-bindings: leds-lp55xx: Limit pwr-sel property to ti,lp8501
(no commit info)
[02/20] dt-bindings: leds-lp55xx: Add new ti,lp5569 compatible

        commit: c4bd4feece4103388d15c99461741604fec0ebbc

[03/20] leds: leds-lp55xx: Generalize stop_all_engine OP
(no commit info)
[04/20] leds: leds-lp55xx: Generalize probe/remove functions
(no commit info)
[05/20] leds: leds-lp55xx: Generalize load_engine function
(no commit info)
[06/20] leds: leds-lp55xx: Generalize load_engine_and_select_page function
(no commit info)
[07/20] leds: leds-lp55xx: Generalize run_engine function
(no commit info)
[08/20] leds: leds-lp55xx: Generalize update_program_memory function
(no commit info)
[09/20] leds: leds-lp55xx: Generalize firmware_loaded function
(no commit info)
[10/20] leds: leds-lp55xx: Generalize led_brightness function
(no commit info)
[11/20] leds: leds-lp55xx: Generalize multicolor_brightness function
(no commit info)
[12/20] leds: leds-lp55xx: Generalize set_led_current function
(no commit info)
[13/20] leds: leds-lp55xx: Generalize turn_off_channels function
(no commit info)
[14/20] leds: leds-lp55xx: Generalize stop_engine function
(no commit info)
[15/20] leds: leds-lp55xx: Generalize sysfs engine_load and engine_mode
(no commit info)
[16/20] leds: leds-lp55xx: Generalize sysfs engine_leds
(no commit info)
[17/20] leds: leds-lp55xx: Generalize sysfs master_fader
(no commit info)
[18/20] leds: leds-lp55xx: Support ENGINE program up to 128 bytes
(no commit info)
[19/20] leds: leds-lp55xx: Drop deprecated defines
(no commit info)
[20/20] leds: leds-lp5569: Add support for Texas Instruments LP5569
(no commit info)

--
Lee Jones [李琼斯]

maintainer Applied
#28 lore raw
In reply to Christian Marangi
Quoted 9 lines · Christian Marangi

This long series is (as requested) a big overhaul of the lp55xx based LED driver.

As notice for these kind of LED chip there was the bad habit of copy the old driver and just modify it enough to make it work with the new model. Till v4 I was also doing the same by following the pattern and the code format of previous driver.

[...]

Applied, thanks!

[01/20] dt-bindings: leds-lp55xx: Limit pwr-sel property to ti,lp8501

        commit: 468434a059a7d1fad4b98c2ca080817b1520cbdc
[02/20] dt-bindings: leds-lp55xx: Add new ti,lp5569 compatible
        commit: a6ca48430de6e87644203bdca03f4065f5b9df7a
[03/20] leds: leds-lp55xx: Generalize stop_all_engine OP
        commit: a9b202b9cf0e817be756a720920ad4b522e6f6aa
[04/20] leds: leds-lp55xx: Generalize probe/remove functions
        commit: db30c2891bfc74acb8823edee5f39cbc36bd9a4d
[05/20] leds: leds-lp55xx: Generalize load_engine function
        commit: 4d310b96f2db602830c40f82a75ede799b243cce
[06/20] leds: leds-lp55xx: Generalize load_engine_and_select_page function
        commit: 409a9dc53682b9f02793584d17721ab3e1b9c86f
[07/20] leds: leds-lp55xx: Generalize run_engine function
        commit: 42a9eaac9784e9b3df56f1947526d7d4d0ed9b26
[08/20] leds: leds-lp55xx: Generalize update_program_memory function
        commit: 31379a57cf2f155eb147ace86547b7143592945a
[09/20] leds: leds-lp55xx: Generalize firmware_loaded function
        commit: a3df1906fb9aa9ff45149e0a3c6434b2cef4f6e7
[10/20] leds: leds-lp55xx: Generalize led_brightness function
        commit: c63580b27a2c638cbae2fc26484b0bf29f303134
[11/20] leds: leds-lp55xx: Generalize multicolor_brightness function
        commit: 794826b2d87538a0fa5429957439f82bb7f32b53
[12/20] leds: leds-lp55xx: Generalize set_led_current function
        commit: 01e0290d17b2fb9717ee80fed512b32e0460b14c
[13/20] leds: leds-lp55xx: Generalize turn_off_channels function
        commit: e35bc5d8a023a55a5f895d6648a455ed83dc0db2
[14/20] leds: leds-lp55xx: Generalize stop_engine function
        commit: 43e91e5eb9c8b36ddd1dc239e0d8c36cc034e8ca
[15/20] leds: leds-lp55xx: Generalize sysfs engine_load and engine_mode
        commit: 082a4d3f068734eb242e38892d0977ef271c0143
[16/20] leds: leds-lp55xx: Generalize sysfs engine_leds
        commit: 8913c2c14728851f110e0d439d5bb2360c767cd2
[17/20] leds: leds-lp55xx: Generalize sysfs master_fader
        commit: 5a15b2ab57095a7c8597d42efbfe452844578785
[18/20] leds: leds-lp55xx: Support ENGINE program up to 128 bytes
        commit: b9d55087dfa950aecece1cf864d3918a12694c25
[19/20] leds: leds-lp55xx: Drop deprecated defines
        commit: 49d943a426d1e2c034ff2f132f65590dbdc01fbd
[20/20] leds: leds-lp5569: Add support for Texas Instruments LP5569
        commit: 30c6743cc89cdb357d1f8a98978da0f7c138130b

--
Lee Jones [李琼斯]