2013-08-08 08:00:21

by Milo Kim

Subject: [PATCH 00/10] leds: lp5521,5523: restore device attributes for running LED patterns

This patch-set resolves the application conflict by restoring sysfs files.

For LP5521
engine1/2/3_mode
engine1/2/3_load

For LP5523
engine1/2/3_mode
engine1/2/3_load
engine1/2/3_leds

Those attributes are accessed when LED pattern is run by custom application.
Those were removed when LED pattern interface was changed to generic firmware
interface. Please refer to commits below.

git commit 9ce7cb170f97f83a78dc948bf7d25690f15e1328
(leds-lp5521: use generic firmware interface)

git commit db6eaf8388a413a5ee1b4547ce78506b9c6456b0
(leds-lp5523: use generic firmware interface)

Necessary attributes are restored in this patch-set.

(Other changes)
New data structure is added for handling values from/to an application.
Few code fixes for reducing writing I2C commands.
Add LP55xx common macros for code refactoring.
Documentation updates.

You can also pull from the location below
This branch is based on 'for-next' of linux-leds.

https://github.com/milokim/lp55xx.git resolve-missing-sysfs

Milo Kim (10):
leds: lp55xx: add common data structure for program
leds: lp55xx: add common macros for device attributes
leds: lp5521: restore legacy device attributes
leds: lp5521: remove unnecessary writing commands
leds: lp5523: make separate API for loading engine
leds: lp5523: LED MUX configuration on initializing
leds: lp5523: restore legacy device attributes
leds: lp5523: remove unnecessary writing commands
Documentation: leds-lp5521,lp5523: update device attribute
information
leds: lp5562: use LP55xx common macros for device attributes

Documentation/leds/leds-lp5521.txt | 20 ++-
Documentation/leds/leds-lp5523.txt | 21 ++-
drivers/leds/leds-lp5521.c | 114 +++++++++++--
drivers/leds/leds-lp5523.c | 321 ++++++++++++++++++++++++++++++++++--
drivers/leds/leds-lp5562.c | 4 +-
drivers/leds/leds-lp55xx-common.h | 66 ++++++++
6 files changed, 511 insertions(+), 35 deletions(-)

--
1.7.9.5

2013-08-08 08:00:33

by Milo Kim

Subject: [PATCH 02/10] leds: lp55xx: add common macros for device attributes

This patch provides common macros for LP5521 and LP5523 device attributes and
functions.

(Device attributes)
LP5521: 'mode', 'load' and 'selftest'
LP5523: 'mode', 'load', 'leds' and 'selftest'

(Permissions)
mode: R/W
load: Write-only
leds: R/W
selftest: Read-only

Couple of lines are duplicate, so use these macros for adding device attributes
in LP5521 and LP5523 drivers.

Signed-off-by: Milo Kim <[email protected]>
---
drivers/leds/leds-lp55xx-common.h | 47 +++++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)

diff --git a/drivers/leds/leds-lp55xx-common.h b/drivers/leds/leds-lp55xx-common.h
index 04c1d4f..cceab48 100644
--- a/drivers/leds/leds-lp55xx-common.h
+++ b/drivers/leds/leds-lp55xx-common.h
@@ -29,6 +29,53 @@ enum lp55xx_engine_mode {
LP55XX_ENGINE_RUN,
};

+#define LP55XX_DEV_ATTR_RW(name, show, store) \
+ DEVICE_ATTR(name, S_IRUGO | S_IWUSR, show, store)
+#define LP55XX_DEV_ATTR_RO(name, show) \
+ DEVICE_ATTR(name, S_IRUGO, show, NULL)
+#define LP55XX_DEV_ATTR_WO(name, store) \
+ DEVICE_ATTR(name, S_IWUSR, NULL, store)
+
+#define show_mode(nr) \
+static ssize_t show_engine##nr##_mode(struct device *dev, \
+ struct device_attribute *attr, \
+ char *buf) \
+{ \
+ return show_engine_mode(dev, attr, buf, nr); \
+}
+
+#define store_mode(nr) \
+static ssize_t store_engine##nr##_mode(struct device *dev, \
+ struct device_attribute *attr, \
+ const char *buf, size_t len) \
+{ \
+ return store_engine_mode(dev, attr, buf, len, nr); \
+}
+
+#define show_leds(nr) \
+static ssize_t show_engine##nr##_leds(struct device *dev, \
+ 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); \
+}
+
+#define store_load(nr) \
+static ssize_t store_engine##nr##_load(struct device *dev, \
+ struct device_attribute *attr, \
+ const char *buf, size_t len) \
+{ \
+ return store_engine_load(dev, attr, buf, len, nr); \
+}
+
struct lp55xx_led;
struct lp55xx_chip;

--
1.7.9.5

2013-08-08 08:00:42

by Milo Kim

Subject: [PATCH 06/10] leds: lp5523: LED MUX configuration on initializing

LED MUX start and stop address should be updated in the program memory
on LP5523 initialization.
LED pattern doesn't work without additional MUX address configuration.
This handling is done by new function, lp5523_init_program_engine().
Eventually, it's called during device initialization, lp5523_post_init_device().

This is a conflict after git commit 632418bf65503405df3f9a6a1616f5a95f91db85
(leds-lp5523: clean up lp5523_configure()).
So it should be fixed.

Cc: Pali Rohár <[email protected]>
Signed-off-by: Milo Kim <[email protected]>
---
drivers/leds/leds-lp5523.c | 70 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 69 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index b509480..29c8b19 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -49,6 +49,9 @@
#define LP5523_REG_RESET 0x3D
#define LP5523_REG_LED_TEST_CTRL 0x41
#define LP5523_REG_LED_TEST_ADC 0x42
+#define LP5523_REG_CH1_PROG_START 0x4C
+#define LP5523_REG_CH2_PROG_START 0x4D
+#define LP5523_REG_CH3_PROG_START 0x4E
#define LP5523_REG_PROG_PAGE_SEL 0x4F
#define LP5523_REG_PROG_MEM 0x50

@@ -65,6 +68,7 @@
#define LP5523_RESET 0xFF
#define LP5523_ADC_SHORTCIRC_LIM 80
#define LP5523_EXT_CLK_USED 0x08
+#define LP5523_ENG_STATUS_MASK 0x07

/* Memory Page Selection */
#define LP5523_PAGE_ENG1 0
@@ -99,6 +103,8 @@ enum lp5523_chip_id {
LP55231,
};

+static int lp5523_init_program_engine(struct lp55xx_chip *chip);
+
static inline void lp5523_wait_opmode_done(void)
{
usleep_range(1000, 2000);
@@ -134,7 +140,11 @@ static int lp5523_post_init_device(struct lp55xx_chip *chip)
if (ret)
return ret;

- return lp55xx_write(chip, LP5523_REG_ENABLE_LEDS_LSB, 0xff);
+ ret = lp55xx_write(chip, LP5523_REG_ENABLE_LEDS_LSB, 0xff);
+ if (ret)
+ return ret;
+
+ return lp5523_init_program_engine(chip);
}

static void lp5523_load_engine(struct lp55xx_chip *chip)
@@ -233,6 +243,64 @@ static void lp5523_run_engine(struct lp55xx_chip *chip, bool start)
lp55xx_update_bits(chip, LP5523_REG_ENABLE, LP5523_EXEC_M, exec);
}

+static int lp5523_init_program_engine(struct lp55xx_chip *chip)
+{
+ int i;
+ int j;
+ int ret;
+ u8 status;
+ /* one pattern per engine setting LED MUX start and stop addresses */
+ static const u8 pattern[][LP5523_PROGRAM_LENGTH] = {
+ { 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},
+ };
+
+ /* hardcode 32 bytes of memory for each engine from program memory */
+ ret = lp55xx_write(chip, LP5523_REG_CH1_PROG_START, 0x00);
+ if (ret)
+ return ret;
+
+ ret = lp55xx_write(chip, LP5523_REG_CH2_PROG_START, 0x10);
+ if (ret)
+ return ret;
+
+ ret = lp55xx_write(chip, LP5523_REG_CH3_PROG_START, 0x20);
+ if (ret)
+ return ret;
+
+ /* 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);
+
+ for (j = 0; j < LP5523_PROGRAM_LENGTH; j++) {
+ ret = lp55xx_write(chip, LP5523_REG_PROG_MEM + j,
+ pattern[i - 1][j]);
+ if (ret)
+ goto out;
+ }
+ }
+
+ lp5523_run_engine(chip, true);
+
+ /* Let the programs run for couple of ms and check the engine status */
+ usleep_range(3000, 6000);
+ lp55xx_read(chip, LP5523_REG_STATUS, &status);
+ status &= LP5523_ENG_STATUS_MASK;
+
+ if (status != LP5523_ENG_STATUS_MASK) {
+ dev_err(&chip->cl->dev,
+ "cound not configure LED engine, status = 0x%.2x\n",
+ status);
+ ret = -1;
+ }
+
+out:
+ lp5523_stop_engine(chip);
+ return ret;
+}
+
static int lp5523_update_program_memory(struct lp55xx_chip *chip,
const u8 *data, size_t size)
{
--
1.7.9.5

2013-08-08 08:00:38

by Milo Kim

Subject: [PATCH 05/10] leds: lp5523: make separate API for loading engine

lp5523_load_engine()
It is called whenever the operation mode is changed to 'load'.
It is used for simple operation mode change.
It will be used when engine mode and LED selection is updated in later patch.

lp5523_load_engine_and_select_page()
Change the operation mode to 'load' and select program page number.
This is used for programming a LED pattern at a time.
So load_engine() is replaced with new API, load_engine_and_select_page()
in lp5523_firmware_loaded().

Signed-off-by: Milo Kim <[email protected]>
---
drivers/leds/leds-lp5523.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index 72c10e2..b509480 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -152,15 +152,21 @@ static void lp5523_load_engine(struct lp55xx_chip *chip)
[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;
u8 page_sel[] = {
[LP55XX_ENGINE_1] = LP5523_PAGE_ENG1,
[LP55XX_ENGINE_2] = LP5523_PAGE_ENG2,
[LP55XX_ENGINE_3] = LP5523_PAGE_ENG3,
};

- lp55xx_update_bits(chip, LP5523_REG_OP_MODE, mask[idx], val[idx]);
-
- lp5523_wait_opmode_done();
+ lp5523_load_engine(chip);

lp55xx_write(chip, LP5523_REG_PROG_PAGE_SEL, page_sel[idx]);
}
@@ -290,7 +296,7 @@ static void lp5523_firmware_loaded(struct lp55xx_chip *chip)
* 2) write firmware data into program memory
*/

- lp5523_load_engine(chip);
+ lp5523_load_engine_and_select_page(chip);
lp5523_update_program_memory(chip, fw->data, fw->size);
}

--
1.7.9.5

2013-08-08 08:00:49

by Milo Kim

Subject: [PATCH 08/10] leds: lp5523: remove unnecessary writing commands

This patch reduces the number of programming commands.

(Count of sending commands)
Old code: 32 + program size (32 counts for clearing program memory)
New code: 32

Pattern buffer is initialized to 0 in this function.
Just update new program data and remaining buffers are filled with 0.
So it's needless to clear whole area.

Signed-off-by: Milo Kim <[email protected]>
---
drivers/leds/leds-lp5523.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index 9b8be6f6..fe3bcbb 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -312,17 +312,11 @@ static int lp5523_update_program_memory(struct lp55xx_chip *chip,
u8 pattern[LP5523_PROGRAM_LENGTH] = {0};
unsigned cmd;
char c[3];
- int update_size;
int nrchars;
- int offset = 0;
int ret;
- int i;
-
- /* clear program memory before updating */
- for (i = 0; i < LP5523_PROGRAM_LENGTH; i++)
- lp55xx_write(chip, LP5523_REG_PROG_MEM + i, 0);
+ int offset = 0;
+ int i = 0;

- i = 0;
while ((offset < size - 1) && (i < LP5523_PROGRAM_LENGTH)) {
/* separate sscanfs because length is working only for %s */
ret = sscanf(data + offset, "%2s%n ", c, &nrchars);
@@ -342,11 +336,9 @@ static int lp5523_update_program_memory(struct lp55xx_chip *chip,
if (i % 2)
goto err;

- update_size = i;
-
mutex_lock(&chip->lock);

- for (i = 0; i < update_size; i++) {
+ for (i = 0; i < LP5523_PROGRAM_LENGTH; i++) {
ret = lp55xx_write(chip, LP5523_REG_PROG_MEM + i, pattern[i]);
if (ret) {
mutex_unlock(&chip->lock);
--
1.7.9.5

2013-08-08 08:00:58

by Milo Kim

Subject: [PATCH 10/10] leds: lp5562: use LP55xx common macros for device attributes

Signed-off-by: Milo Kim <[email protected]>
---
drivers/leds/leds-lp5562.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/leds-lp5562.c b/drivers/leds/leds-lp5562.c
index a2c7398..2585cfd 100644
--- a/drivers/leds/leds-lp5562.c
+++ b/drivers/leds/leds-lp5562.c
@@ -477,8 +477,8 @@ static ssize_t lp5562_store_engine_mux(struct device *dev,
return len;
}

-static DEVICE_ATTR(led_pattern, S_IWUSR, NULL, lp5562_store_pattern);
-static DEVICE_ATTR(engine_mux, S_IWUSR, NULL, lp5562_store_engine_mux);
+static LP55XX_DEV_ATTR_WO(led_pattern, lp5562_store_pattern);
+static LP55XX_DEV_ATTR_WO(engine_mux, lp5562_store_engine_mux);

static struct attribute *lp5562_attributes[] = {
&dev_attr_led_pattern.attr,
--
1.7.9.5

2013-08-08 08:00:53

by Milo Kim

Subject: [PATCH 09/10] Documentation: leds-lp5521,lp5523: update device attribute information

Now, all legacy application interfaces are restored.
Each driver documentation is updated.

Cc: Pali Rohár <[email protected]>
Signed-off-by: Milo Kim <[email protected]>
---
Documentation/leds/leds-lp5521.txt | 20 +++++++++++++++++++-
Documentation/leds/leds-lp5523.txt | 21 ++++++++++++++++++++-
2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/Documentation/leds/leds-lp5521.txt b/Documentation/leds/leds-lp5521.txt
index 79e4c2e..d08d8c1 100644
--- a/Documentation/leds/leds-lp5521.txt
+++ b/Documentation/leds/leds-lp5521.txt
@@ -18,7 +18,25 @@ All three channels can be also controlled using the engine micro programs.
More details of the instructions can be found from the public data sheet.

LP5521 has the internal program memory for running various LED patterns.
-For the details, please refer to 'firmware' section in leds-lp55xx.txt
+There are two ways to run LED patterns.
+
+1) Legacy interface - enginex_mode and enginex_load
+ Control interface for the engines:
+ x is 1 .. 3
+ enginex_mode : disabled, load, run
+ enginex_load : store program (visible only in engine load mode)
+
+ Example (start to blink the channel 2 led):
+ cd /sys/class/leds/lp5521:channel2/device
+ echo "load" > engine3_mode
+ echo "037f4d0003ff6000" > engine3_load
+ echo "run" > engine3_mode
+
+ To stop the engine:
+ echo "disabled" > engine3_mode
+
+2) Firmware interface - LP55xx common interface
+ For the details, please refer to 'firmware' section in leds-lp55xx.txt

sysfs contains a selftest entry.
The test communicates with the chip and checks that
diff --git a/Documentation/leds/leds-lp5523.txt b/Documentation/leds/leds-lp5523.txt
index 899fdad..5b3e91d 100644
--- a/Documentation/leds/leds-lp5523.txt
+++ b/Documentation/leds/leds-lp5523.txt
@@ -28,7 +28,26 @@ If both fields are NULL, 'lp5523' is used by default.
/sys/class/leds/lp5523:channelN (N: 0 ~ 8)

LP5523 has the internal program memory for running various LED patterns.
-For the details, please refer to 'firmware' section in leds-lp55xx.txt
+There are two ways to run LED patterns.
+
+1) Legacy interface - enginex_mode, enginex_load and enginex_leds
+ Control interface for the engines:
+ x is 1 .. 3
+ enginex_mode : disabled, load, run
+ enginex_load : microcode load (visible only in load mode)
+ enginex_leds : led mux control (visible only in load mode)
+
+ cd /sys/class/leds/lp5523:channel2/device
+ echo "load" > engine3_mode
+ echo "9d80400004ff05ff437f0000" > engine3_load
+ echo "111111111" > engine3_leds
+ echo "run" > engine3_mode
+
+ To stop the engine:
+ echo "disabled" > engine3_mode
+
+2) Firmware interface - LP55xx common interface
+ For the details, please refer to 'firmware' section in leds-lp55xx.txt

Selftest uses always the current from the platform data.

--
1.7.9.5

2013-08-08 08:00:46

by Milo Kim

Subject: [PATCH 07/10] leds: lp5523: restore legacy device attributes

git commit db6eaf8388a413a5ee1b4547ce78506b9c6456b0
(leds-lp5523: use generic firmware interface) causes an application conflict.
This interface should be maintained for compatibility.

Restored device attributes are 'engineN_mode', 'engineN_load' and
'engineN_leds'. (N = 1, 2 or 3)
A 'selftest' attribute macro is replaced with LP55xx common macro.
Those are accessed when a LED pattern is run by an application.

Use a mutex in lp5523_update_program_memory()
: This function is called when an user-application writes a 'engineN_load' file
or pattern data is loaded from generic firmware interface.
So, writing program memory should be protected.
If an error occurs on accessing this area, just it returns as -EINVAL quickly.
This error code is exact same as old driver function, lp5523_do_store_load()
because it should be kept for an user-application compatibility.
Even the driver is changed, we can use the application without re-compiling
sources.

Reported-by: Pali Rohár <[email protected]>
Signed-off-by: Milo Kim <[email protected]>
---
drivers/leds/leds-lp5523.c | 227 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 223 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index 29c8b19..9b8be6f6 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -74,6 +74,9 @@
#define LP5523_PAGE_ENG1 0
#define LP5523_PAGE_ENG2 1
#define LP5523_PAGE_ENG3 2
+#define LP5523_PAGE_MUX1 3
+#define LP5523_PAGE_MUX2 4
+#define LP5523_PAGE_MUX3 5

/* Program Memory Operations */
#define LP5523_MODE_ENG1_M 0x30 /* Operation Mode Register */
@@ -98,6 +101,8 @@
#define LP5523_RUN_ENG2 0x08
#define LP5523_RUN_ENG3 0x02

+#define LED_ACTIVE(mux, led) (!!(mux & (0x0001 << led)))
+
enum lp5523_chip_id {
LP5523,
LP55231,
@@ -338,10 +343,20 @@ static int lp5523_update_program_memory(struct lp55xx_chip *chip,
goto err;

update_size = i;
- for (i = 0; i < update_size; i++)
- lp55xx_write(chip, LP5523_REG_PROG_MEM + i, pattern[i]);

- return 0;
+ mutex_lock(&chip->lock);
+
+ for (i = 0; i < update_size; i++) {
+ ret = lp55xx_write(chip, LP5523_REG_PROG_MEM + i, pattern[i]);
+ if (ret) {
+ mutex_unlock(&chip->lock);
+ return -EINVAL;
+ }
+ }
+
+ mutex_unlock(&chip->lock);
+
+ return size;

err:
dev_err(&chip->cl->dev, "wrong pattern format\n");
@@ -368,6 +383,192 @@ static void lp5523_firmware_loaded(struct lp55xx_chip *chip)
lp5523_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)
+{
+ 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)) {
+ lp5523_run_engine(chip, true);
+ engine->mode = LP55XX_ENGINE_RUN;
+ } else if (!strncmp(buf, "load", 4)) {
+ lp5523_stop_engine(chip);
+ lp5523_load_engine(chip);
+ engine->mode = LP55XX_ENGINE_LOAD;
+ } else if (!strncmp(buf, "disabled", 8)) {
+ lp5523_stop_engine(chip);
+ engine->mode = LP55XX_ENGINE_DISABLED;
+ }
+
+ mutex_unlock(&chip->lock);
+
+ return len;
+}
+store_mode(1)
+store_mode(2)
+store_mode(3)
+
+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;
+ u8 mux_page[] = {
+ [LP55XX_ENGINE_1] = LP5523_PAGE_MUX1,
+ [LP55XX_ENGINE_2] = LP5523_PAGE_MUX2,
+ [LP55XX_ENGINE_3] = LP5523_PAGE_MUX3,
+ };
+
+ lp5523_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 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;
+
+ mutex_lock(&chip->lock);
+
+ chip->engine_idx = nr;
+ lp5523_load_engine_and_select_page(chip);
+
+ mutex_unlock(&chip->lock);
+
+ return lp5523_update_program_memory(chip, buf, len);
+}
+store_load(1)
+store_load(2)
+store_load(3)
+
static ssize_t lp5523_selftest(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -467,9 +668,27 @@ static void lp5523_led_brightness_work(struct work_struct *work)
mutex_unlock(&chip->lock);
}

-static DEVICE_ATTR(selftest, S_IRUGO, lp5523_selftest, NULL);
+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_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);
+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);
+static LP55XX_DEV_ATTR_RO(selftest, lp5523_selftest);

static struct attribute *lp5523_attributes[] = {
+ &dev_attr_engine1_mode.attr,
+ &dev_attr_engine2_mode.attr,
+ &dev_attr_engine3_mode.attr,
+ &dev_attr_engine1_load.attr,
+ &dev_attr_engine2_load.attr,
+ &dev_attr_engine3_load.attr,
+ &dev_attr_engine1_leds.attr,
+ &dev_attr_engine2_leds.attr,
+ &dev_attr_engine3_leds.attr,
&dev_attr_selftest.attr,
NULL,
};
--
1.7.9.5

2013-08-08 08:02:31

by Milo Kim

Subject: [PATCH 04/10] leds: lp5521: remove unnecessary writing commands

This patch reduces the number of programming commands.

(Count of sending commands)
Old code: 32 + program size (32 counts for clearing program memory)
New code: 32

Pattern buffer is initialized to 0 in this function.
Just update new program data and remaining buffers are filled with 0.
So it's needless to clear whole area.

Signed-off-by: Milo Kim <[email protected]>
---
drivers/leds/leds-lp5521.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
index c00f922..0518835 100644
--- a/drivers/leds/leds-lp5521.c
+++ b/drivers/leds/leds-lp5521.c
@@ -220,17 +220,11 @@ static int lp5521_update_program_memory(struct lp55xx_chip *chip,
};
unsigned cmd;
char c[3];
- int program_size;
int nrchars;
- int offset = 0;
int ret;
- int i;
-
- /* clear program memory before updating */
- for (i = 0; i < LP5521_PROGRAM_LENGTH; i++)
- lp55xx_write(chip, addr[idx] + i, 0);
+ int offset = 0;
+ int i = 0;

- 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);
@@ -250,11 +244,9 @@ static int lp5521_update_program_memory(struct lp55xx_chip *chip,
if (i % 2)
goto err;

- program_size = i;
-
mutex_lock(&chip->lock);

- for (i = 0; i < program_size; i++) {
+ for (i = 0; i < LP5521_PROGRAM_LENGTH; i++) {
ret = lp55xx_write(chip, addr[idx] + i, pattern[i]);
if (ret) {
mutex_unlock(&chip->lock);
--
1.7.9.5

2013-08-08 08:00:31

by Milo Kim

Subject: [PATCH 01/10] leds: lp55xx: add common data structure for program

LP55xx family devices have internal three program engines which are used for
loading LED patterns.
To maintain legacy device attributes, specific data structure is used, 'mode'
and 'led_mux'.
The mode is used for showing/storing current engine mode such like disabled,
load and run.
Then led_mux is used for showing/storing current output LED selection.
This is only for LP5523/55231.

Signed-off-by: Milo Kim <[email protected]>
---
drivers/leds/leds-lp55xx-common.h | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)

diff --git a/drivers/leds/leds-lp55xx-common.h b/drivers/leds/leds-lp55xx-common.h
index dbbf86d..04c1d4f 100644
--- a/drivers/leds/leds-lp55xx-common.h
+++ b/drivers/leds/leds-lp55xx-common.h
@@ -20,6 +20,13 @@ enum lp55xx_engine_index {
LP55XX_ENGINE_1,
LP55XX_ENGINE_2,
LP55XX_ENGINE_3,
+ LP55XX_ENGINE_MAX = LP55XX_ENGINE_3,
+};
+
+enum lp55xx_engine_mode {
+ LP55XX_ENGINE_DISABLED,
+ LP55XX_ENGINE_LOAD,
+ LP55XX_ENGINE_RUN,
};

struct lp55xx_led;
@@ -72,6 +79,16 @@ struct lp55xx_device_config {
};

/*
+ * struct lp55xx_engine
+ * @mode : Engine mode
+ * @led_mux : Mux bits for LED selection. Only used in LP5523
+ */
+struct lp55xx_engine {
+ enum lp55xx_engine_mode mode;
+ u16 led_mux;
+};
+
+/*
* struct lp55xx_chip
* @cl : I2C communication for access registers
* @pdata : Platform specific data
@@ -79,6 +96,7 @@ struct lp55xx_device_config {
* @num_leds : Number of registered LEDs
* @cfg : Device specific configuration data
* @engine_idx : Selected engine number
+ * @engines : Engine structure for the device attribute R/W interface
* @fw : Firmware data for running a LED pattern
*/
struct lp55xx_chip {
@@ -89,6 +107,7 @@ struct lp55xx_chip {
int num_leds;
struct lp55xx_device_config *cfg;
enum lp55xx_engine_index engine_idx;
+ struct lp55xx_engine engines[LP55XX_ENGINE_MAX];
const struct firmware *fw;
};

--
1.7.9.5

2013-08-08 08:03:28

by Milo Kim

Subject: [PATCH 03/10] leds: lp5521: restore legacy device attributes

git commit 9ce7cb170f97f83a78dc948bf7d25690f15e1328
may cause an application confict, engineN_mode and engineN_load.
This interface should be maintained for compatibility.

Restored device attributes are 'engineN_mode' and 'engineN_load'.
A 'selftest' attribute macro is replaced with LP55xx common macro.

Use a mutex in lp5521_update_program_memory()
: This function is called when an user-application writes a 'engineN_load' file
or pattern data is loaded from generic firmware interface.
So, writing program memory should be protected.
If an error occurs on accessing this area, just it returns as -EINVAL quickly.
This error code is exact same as old driver function, lp5521_do_store_load()
because it should be kept for an user-application compatibility.
Even the driver is changed, we can use the application without re-compiling
sources.

'led_pattern' attribute is not included
: engineN_mode and _load were created for custom user-application.
'led_pattern' is an exception. I added this attribute not for custom application
but for simple test. Now it is used only in LP5562 driver, not LP5521.

Signed-off-by: Milo Kim <[email protected]>
---
drivers/leds/leds-lp5521.c | 104 ++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 100 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
index 9e28dd0..c00f922 100644
--- a/drivers/leds/leds-lp5521.c
+++ b/drivers/leds/leds-lp5521.c
@@ -251,10 +251,20 @@ static int lp5521_update_program_memory(struct lp55xx_chip *chip,
goto err;

program_size = i;
- for (i = 0; i < program_size; i++)
- lp55xx_write(chip, addr[idx] + i, pattern[i]);

- return 0;
+ mutex_lock(&chip->lock);
+
+ for (i = 0; i < program_size; i++) {
+ ret = lp55xx_write(chip, addr[idx] + i, pattern[i]);
+ if (ret) {
+ mutex_unlock(&chip->lock);
+ return -EINVAL;
+ }
+ }
+
+ mutex_unlock(&chip->lock);
+
+ return size;

err:
dev_err(&chip->cl->dev, "wrong pattern format\n");
@@ -365,6 +375,80 @@ static void lp5521_led_brightness_work(struct work_struct *work)
mutex_unlock(&chip->lock);
}

+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)) {
+ lp5521_stop_engine(chip);
+ lp5521_load_engine(chip);
+ engine->mode = LP55XX_ENGINE_LOAD;
+ } else if (!strncmp(buf, "disabled", 8)) {
+ lp5521_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;
+
+ mutex_lock(&chip->lock);
+
+ chip->engine_idx = nr;
+ lp5521_load_engine(chip);
+
+ mutex_unlock(&chip->lock);
+
+ return lp5521_update_program_memory(chip, buf, len);
+}
+store_load(1)
+store_load(2)
+store_load(3)
+
static ssize_t lp5521_selftest(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -381,9 +465,21 @@ static ssize_t lp5521_selftest(struct device *dev,
}

/* device attributes */
-static DEVICE_ATTR(selftest, S_IRUGO, lp5521_selftest, NULL);
+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);
+static LP55XX_DEV_ATTR_RO(selftest, lp5521_selftest);

static struct attribute *lp5521_attributes[] = {
+ &dev_attr_engine1_mode.attr,
+ &dev_attr_engine2_mode.attr,
+ &dev_attr_engine3_mode.attr,
+ &dev_attr_engine1_load.attr,
+ &dev_attr_engine2_load.attr,
+ &dev_attr_engine3_load.attr,
&dev_attr_selftest.attr,
NULL
};
--
1.7.9.5

2013-08-13 18:56:37

by Bryan Wu

Subject: Re: [PATCH 01/10] leds: lp55xx: add common data structure for program

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <[email protected]> wrote:
> LP55xx family devices have internal three program engines which are used for
> loading LED patterns.
> To maintain legacy device attributes, specific data structure is used, 'mode'
> and 'led_mux'.
> The mode is used for showing/storing current engine mode such like disabled,
> load and run.
> Then led_mux is used for showing/storing current output LED selection.
> This is only for LP5523/55231.
>

This patch looks good to me, but the commit message format is little
bit odd to me. I will fix that and merge into my tree.

Thanks,
-Bryan


> Signed-off-by: Milo Kim <[email protected]>
> ---
> drivers/leds/leds-lp55xx-common.h | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/drivers/leds/leds-lp55xx-common.h b/drivers/leds/leds-lp55xx-common.h
> index dbbf86d..04c1d4f 100644
> --- a/drivers/leds/leds-lp55xx-common.h
> +++ b/drivers/leds/leds-lp55xx-common.h
> @@ -20,6 +20,13 @@ enum lp55xx_engine_index {
> LP55XX_ENGINE_1,
> LP55XX_ENGINE_2,
> LP55XX_ENGINE_3,
> + LP55XX_ENGINE_MAX = LP55XX_ENGINE_3,
> +};
> +
> +enum lp55xx_engine_mode {
> + LP55XX_ENGINE_DISABLED,
> + LP55XX_ENGINE_LOAD,
> + LP55XX_ENGINE_RUN,
> };
>
> struct lp55xx_led;
> @@ -72,6 +79,16 @@ struct lp55xx_device_config {
> };
>
> /*
> + * struct lp55xx_engine
> + * @mode : Engine mode
> + * @led_mux : Mux bits for LED selection. Only used in LP5523
> + */
> +struct lp55xx_engine {
> + enum lp55xx_engine_mode mode;
> + u16 led_mux;
> +};
> +
> +/*
> * struct lp55xx_chip
> * @cl : I2C communication for access registers
> * @pdata : Platform specific data
> @@ -79,6 +96,7 @@ struct lp55xx_device_config {
> * @num_leds : Number of registered LEDs
> * @cfg : Device specific configuration data
> * @engine_idx : Selected engine number
> + * @engines : Engine structure for the device attribute R/W interface
> * @fw : Firmware data for running a LED pattern
> */
> struct lp55xx_chip {
> @@ -89,6 +107,7 @@ struct lp55xx_chip {
> int num_leds;
> struct lp55xx_device_config *cfg;
> enum lp55xx_engine_index engine_idx;
> + struct lp55xx_engine engines[LP55XX_ENGINE_MAX];
> const struct firmware *fw;
> };
>
> --
> 1.7.9.5
>

2013-08-13 19:12:51

by Bryan Wu

Subject: Re: [PATCH 02/10] leds: lp55xx: add common macros for device attributes

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <[email protected]> wrote:
> This patch provides common macros for LP5521 and LP5523 device attributes and
> functions.
>
> (Device attributes)
> LP5521: 'mode', 'load' and 'selftest'
> LP5523: 'mode', 'load', 'leds' and 'selftest'
>
> (Permissions)
> mode: R/W
> load: Write-only
> leds: R/W
> selftest: Read-only
>
> Couple of lines are duplicate, so use these macros for adding device attributes
> in LP5521 and LP5523 drivers.
>

Good, I will merge this.
-Bryan

> Signed-off-by: Milo Kim <[email protected]>
> ---
> drivers/leds/leds-lp55xx-common.h | 47 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
> diff --git a/drivers/leds/leds-lp55xx-common.h b/drivers/leds/leds-lp55xx-common.h
> index 04c1d4f..cceab48 100644
> --- a/drivers/leds/leds-lp55xx-common.h
> +++ b/drivers/leds/leds-lp55xx-common.h
> @@ -29,6 +29,53 @@ enum lp55xx_engine_mode {
> LP55XX_ENGINE_RUN,
> };
>
> +#define LP55XX_DEV_ATTR_RW(name, show, store) \
> + DEVICE_ATTR(name, S_IRUGO | S_IWUSR, show, store)
> +#define LP55XX_DEV_ATTR_RO(name, show) \
> + DEVICE_ATTR(name, S_IRUGO, show, NULL)
> +#define LP55XX_DEV_ATTR_WO(name, store) \
> + DEVICE_ATTR(name, S_IWUSR, NULL, store)
> +
> +#define show_mode(nr) \
> +static ssize_t show_engine##nr##_mode(struct device *dev, \
> + struct device_attribute *attr, \
> + char *buf) \
> +{ \
> + return show_engine_mode(dev, attr, buf, nr); \
> +}
> +
> +#define store_mode(nr) \
> +static ssize_t store_engine##nr##_mode(struct device *dev, \
> + struct device_attribute *attr, \
> + const char *buf, size_t len) \
> +{ \
> + return store_engine_mode(dev, attr, buf, len, nr); \
> +}
> +
> +#define show_leds(nr) \
> +static ssize_t show_engine##nr##_leds(struct device *dev, \
> + 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); \
> +}
> +
> +#define store_load(nr) \
> +static ssize_t store_engine##nr##_load(struct device *dev, \
> + struct device_attribute *attr, \
> + const char *buf, size_t len) \
> +{ \
> + return store_engine_load(dev, attr, buf, len, nr); \
> +}
> +
> struct lp55xx_led;
> struct lp55xx_chip;
>
> --
> 1.7.9.5
>

2013-08-13 20:40:34

by Bryan Wu

Subject: Re: [PATCH 03/10] leds: lp5521: restore legacy device attributes

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <[email protected]> wrote:
> git commit 9ce7cb170f97f83a78dc948bf7d25690f15e1328
> may cause an application confict, engineN_mode and engineN_load.
> This interface should be maintained for compatibility.
>
> Restored device attributes are 'engineN_mode' and 'engineN_load'.
> A 'selftest' attribute macro is replaced with LP55xx common macro.
>
> Use a mutex in lp5521_update_program_memory()
> : This function is called when an user-application writes a 'engineN_load' file
> or pattern data is loaded from generic firmware interface.
> So, writing program memory should be protected.
> If an error occurs on accessing this area, just it returns as -EINVAL quickly.
> This error code is exact same as old driver function, lp5521_do_store_load()
> because it should be kept for an user-application compatibility.
> Even the driver is changed, we can use the application without re-compiling
> sources.
>
> 'led_pattern' attribute is not included
> : engineN_mode and _load were created for custom user-application.
> 'led_pattern' is an exception. I added this attribute not for custom application
> but for simple test. Now it is used only in LP5562 driver, not LP5521.
>

Looks good to me. Thanks,
-Bryan

> Signed-off-by: Milo Kim <[email protected]>
> ---
> drivers/leds/leds-lp5521.c | 104 ++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 100 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
> index 9e28dd0..c00f922 100644
> --- a/drivers/leds/leds-lp5521.c
> +++ b/drivers/leds/leds-lp5521.c
> @@ -251,10 +251,20 @@ static int lp5521_update_program_memory(struct lp55xx_chip *chip,
> goto err;
>
> program_size = i;
> - for (i = 0; i < program_size; i++)
> - lp55xx_write(chip, addr[idx] + i, pattern[i]);
>
> - return 0;
> + mutex_lock(&chip->lock);
> +
> + for (i = 0; i < program_size; i++) {
> + ret = lp55xx_write(chip, addr[idx] + i, pattern[i]);
> + if (ret) {
> + mutex_unlock(&chip->lock);
> + return -EINVAL;
> + }
> + }
> +
> + mutex_unlock(&chip->lock);
> +
> + return size;
>
> err:
> dev_err(&chip->cl->dev, "wrong pattern format\n");
> @@ -365,6 +375,80 @@ static void lp5521_led_brightness_work(struct work_struct *work)
> mutex_unlock(&chip->lock);
> }
>
> +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)) {
> + lp5521_stop_engine(chip);
> + lp5521_load_engine(chip);
> + engine->mode = LP55XX_ENGINE_LOAD;
> + } else if (!strncmp(buf, "disabled", 8)) {
> + lp5521_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;
> +
> + mutex_lock(&chip->lock);
> +
> + chip->engine_idx = nr;
> + lp5521_load_engine(chip);
> +
> + mutex_unlock(&chip->lock);
> +
> + return lp5521_update_program_memory(chip, buf, len);
> +}
> +store_load(1)
> +store_load(2)
> +store_load(3)
> +
> static ssize_t lp5521_selftest(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -381,9 +465,21 @@ static ssize_t lp5521_selftest(struct device *dev,
> }
>
> /* device attributes */
> -static DEVICE_ATTR(selftest, S_IRUGO, lp5521_selftest, NULL);
> +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);
> +static LP55XX_DEV_ATTR_RO(selftest, lp5521_selftest);
>
> static struct attribute *lp5521_attributes[] = {
> + &dev_attr_engine1_mode.attr,
> + &dev_attr_engine2_mode.attr,
> + &dev_attr_engine3_mode.attr,
> + &dev_attr_engine1_load.attr,
> + &dev_attr_engine2_load.attr,
> + &dev_attr_engine3_load.attr,
> &dev_attr_selftest.attr,
> NULL
> };
> --
> 1.7.9.5
>

2013-08-13 20:43:03

by Bryan Wu

Subject: Re: [PATCH 04/10] leds: lp5521: remove unnecessary writing commands

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <[email protected]> wrote:
> This patch reduces the number of programming commands.
>
> (Count of sending commands)
> Old code: 32 + program size (32 counts for clearing program memory)
> New code: 32
>
> Pattern buffer is initialized to 0 in this function.
> Just update new program data and remaining buffers are filled with 0.
> So it's needless to clear whole area.
>

Good, I will merge this.
-Bryan

> Signed-off-by: Milo Kim <[email protected]>
> ---
> drivers/leds/leds-lp5521.c | 14 +++-----------
> 1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
> index c00f922..0518835 100644
> --- a/drivers/leds/leds-lp5521.c
> +++ b/drivers/leds/leds-lp5521.c
> @@ -220,17 +220,11 @@ static int lp5521_update_program_memory(struct lp55xx_chip *chip,
> };
> unsigned cmd;
> char c[3];
> - int program_size;
> int nrchars;
> - int offset = 0;
> int ret;
> - int i;
> -
> - /* clear program memory before updating */
> - for (i = 0; i < LP5521_PROGRAM_LENGTH; i++)
> - lp55xx_write(chip, addr[idx] + i, 0);
> + int offset = 0;
> + int i = 0;
>
> - 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);
> @@ -250,11 +244,9 @@ static int lp5521_update_program_memory(struct lp55xx_chip *chip,
> if (i % 2)
> goto err;
>
> - program_size = i;
> -
> mutex_lock(&chip->lock);
>
> - for (i = 0; i < program_size; i++) {
> + for (i = 0; i < LP5521_PROGRAM_LENGTH; i++) {
> ret = lp55xx_write(chip, addr[idx] + i, pattern[i]);
> if (ret) {
> mutex_unlock(&chip->lock);
> --
> 1.7.9.5
>

2013-08-13 20:54:32

by Bryan Wu

Subject: Re: [PATCH 05/10] leds: lp5523: make separate API for loading engine

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <[email protected]> wrote:
> lp5523_load_engine()
> It is called whenever the operation mode is changed to 'load'.
> It is used for simple operation mode change.
> It will be used when engine mode and LED selection is updated in later patch.
>
> lp5523_load_engine_and_select_page()
> Change the operation mode to 'load' and select program page number.
> This is used for programming a LED pattern at a time.
> So load_engine() is replaced with new API, load_engine_and_select_page()
> in lp5523_firmware_loaded().
>

Looks good, please make sure these functions are called with mutex lock hold.
Thanks,
-Bryan

> Signed-off-by: Milo Kim <[email protected]>
> ---
> drivers/leds/leds-lp5523.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index 72c10e2..b509480 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -152,15 +152,21 @@ static void lp5523_load_engine(struct lp55xx_chip *chip)
> [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;
> u8 page_sel[] = {
> [LP55XX_ENGINE_1] = LP5523_PAGE_ENG1,
> [LP55XX_ENGINE_2] = LP5523_PAGE_ENG2,
> [LP55XX_ENGINE_3] = LP5523_PAGE_ENG3,
> };
>
> - lp55xx_update_bits(chip, LP5523_REG_OP_MODE, mask[idx], val[idx]);
> -
> - lp5523_wait_opmode_done();
> + lp5523_load_engine(chip);
>
> lp55xx_write(chip, LP5523_REG_PROG_PAGE_SEL, page_sel[idx]);
> }
> @@ -290,7 +296,7 @@ static void lp5523_firmware_loaded(struct lp55xx_chip *chip)
> * 2) write firmware data into program memory
> */
>
> - lp5523_load_engine(chip);
> + lp5523_load_engine_and_select_page(chip);
> lp5523_update_program_memory(chip, fw->data, fw->size);
> }
>
> --
> 1.7.9.5
>

2013-08-13 20:57:14

by Bryan Wu

Subject: Re: [PATCH 06/10] leds: lp5523: LED MUX configuration on initializing

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <[email protected]> wrote:
> LED MUX start and stop address should be updated in the program memory
> on LP5523 initialization.
> LED pattern doesn't work without additional MUX address configuration.
> This handling is done by new function, lp5523_init_program_engine().
> Eventually, it's called during device initialization, lp5523_post_init_device().
>
> This is a conflict after git commit 632418bf65503405df3f9a6a1616f5a95f91db85
> (leds-lp5523: clean up lp5523_configure()).
> So it should be fixed.
>

Good, I will merge this.

-Bryan

> Cc: Pali Roh?r <[email protected]>
> Signed-off-by: Milo Kim <[email protected]>
> ---
> drivers/leds/leds-lp5523.c | 70 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 69 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index b509480..29c8b19 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -49,6 +49,9 @@
> #define LP5523_REG_RESET 0x3D
> #define LP5523_REG_LED_TEST_CTRL 0x41
> #define LP5523_REG_LED_TEST_ADC 0x42
> +#define LP5523_REG_CH1_PROG_START 0x4C
> +#define LP5523_REG_CH2_PROG_START 0x4D
> +#define LP5523_REG_CH3_PROG_START 0x4E
> #define LP5523_REG_PROG_PAGE_SEL 0x4F
> #define LP5523_REG_PROG_MEM 0x50
>
> @@ -65,6 +68,7 @@
> #define LP5523_RESET 0xFF
> #define LP5523_ADC_SHORTCIRC_LIM 80
> #define LP5523_EXT_CLK_USED 0x08
> +#define LP5523_ENG_STATUS_MASK 0x07
>
> /* Memory Page Selection */
> #define LP5523_PAGE_ENG1 0
> @@ -99,6 +103,8 @@ enum lp5523_chip_id {
> LP55231,
> };
>
> +static int lp5523_init_program_engine(struct lp55xx_chip *chip);
> +
> static inline void lp5523_wait_opmode_done(void)
> {
> usleep_range(1000, 2000);
> @@ -134,7 +140,11 @@ static int lp5523_post_init_device(struct lp55xx_chip *chip)
> if (ret)
> return ret;
>
> - return lp55xx_write(chip, LP5523_REG_ENABLE_LEDS_LSB, 0xff);
> + ret = lp55xx_write(chip, LP5523_REG_ENABLE_LEDS_LSB, 0xff);
> + if (ret)
> + return ret;
> +
> + return lp5523_init_program_engine(chip);
> }
>
> static void lp5523_load_engine(struct lp55xx_chip *chip)
> @@ -233,6 +243,64 @@ static void lp5523_run_engine(struct lp55xx_chip *chip, bool start)
> lp55xx_update_bits(chip, LP5523_REG_ENABLE, LP5523_EXEC_M, exec);
> }
>
> +static int lp5523_init_program_engine(struct lp55xx_chip *chip)
> +{
> + int i;
> + int j;
> + int ret;
> + u8 status;
> + /* one pattern per engine setting LED MUX start and stop addresses */
> + static const u8 pattern[][LP5523_PROGRAM_LENGTH] = {
> + { 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},
> + };
> +
> + /* hardcode 32 bytes of memory for each engine from program memory */
> + ret = lp55xx_write(chip, LP5523_REG_CH1_PROG_START, 0x00);
> + if (ret)
> + return ret;
> +
> + ret = lp55xx_write(chip, LP5523_REG_CH2_PROG_START, 0x10);
> + if (ret)
> + return ret;
> +
> + ret = lp55xx_write(chip, LP5523_REG_CH3_PROG_START, 0x20);
> + if (ret)
> + return ret;
> +
> + /* 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);
> +
> + for (j = 0; j < LP5523_PROGRAM_LENGTH; j++) {
> + ret = lp55xx_write(chip, LP5523_REG_PROG_MEM + j,
> + pattern[i - 1][j]);
> + if (ret)
> + goto out;
> + }
> + }
> +
> + lp5523_run_engine(chip, true);
> +
> + /* Let the programs run for couple of ms and check the engine status */
> + usleep_range(3000, 6000);
> + lp55xx_read(chip, LP5523_REG_STATUS, &status);
> + status &= LP5523_ENG_STATUS_MASK;
> +
> + if (status != LP5523_ENG_STATUS_MASK) {
> + dev_err(&chip->cl->dev,
> + "cound not configure LED engine, status = 0x%.2x\n",
> + status);
> + ret = -1;
> + }
> +
> +out:
> + lp5523_stop_engine(chip);
> + return ret;
> +}
> +
> static int lp5523_update_program_memory(struct lp55xx_chip *chip,
> const u8 *data, size_t size)
> {
> --
> 1.7.9.5
>

2013-08-13 20:58:31

by Bryan Wu

Subject: Re: [PATCH 07/10] leds: lp5523: restore legacy device attributes

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <[email protected]> wrote:
> git commit db6eaf8388a413a5ee1b4547ce78506b9c6456b0
> (leds-lp5523: use generic firmware interface) causes an application conflict.
> This interface should be maintained for compatibility.
>
> Restored device attributes are 'engineN_mode', 'engineN_load' and
> 'engineN_leds'. (N = 1, 2 or 3)
> A 'selftest' attribute macro is replaced with LP55xx common macro.
> Those are accessed when a LED pattern is run by an application.
>
> Use a mutex in lp5523_update_program_memory()
> : This function is called when an user-application writes a 'engineN_load' file
> or pattern data is loaded from generic firmware interface.
> So, writing program memory should be protected.
> If an error occurs on accessing this area, just it returns as -EINVAL quickly.
> This error code is exact same as old driver function, lp5523_do_store_load()
> because it should be kept for an user-application compatibility.
> Even the driver is changed, we can use the application without re-compiling
> sources.
>

Good, I will merge this.

-Bryan

> Reported-by: Pali Roh?r <[email protected]>
> Signed-off-by: Milo Kim <[email protected]>
> ---
> drivers/leds/leds-lp5523.c | 227 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 223 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index 29c8b19..9b8be6f6 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -74,6 +74,9 @@
> #define LP5523_PAGE_ENG1 0
> #define LP5523_PAGE_ENG2 1
> #define LP5523_PAGE_ENG3 2
> +#define LP5523_PAGE_MUX1 3
> +#define LP5523_PAGE_MUX2 4
> +#define LP5523_PAGE_MUX3 5
>
> /* Program Memory Operations */
> #define LP5523_MODE_ENG1_M 0x30 /* Operation Mode Register */
> @@ -98,6 +101,8 @@
> #define LP5523_RUN_ENG2 0x08
> #define LP5523_RUN_ENG3 0x02
>
> +#define LED_ACTIVE(mux, led) (!!(mux & (0x0001 << led)))
> +
> enum lp5523_chip_id {
> LP5523,
> LP55231,
> @@ -338,10 +343,20 @@ static int lp5523_update_program_memory(struct lp55xx_chip *chip,
> goto err;
>
> update_size = i;
> - for (i = 0; i < update_size; i++)
> - lp55xx_write(chip, LP5523_REG_PROG_MEM + i, pattern[i]);
>
> - return 0;
> + mutex_lock(&chip->lock);
> +
> + for (i = 0; i < update_size; i++) {
> + ret = lp55xx_write(chip, LP5523_REG_PROG_MEM + i, pattern[i]);
> + if (ret) {
> + mutex_unlock(&chip->lock);
> + return -EINVAL;
> + }
> + }
> +
> + mutex_unlock(&chip->lock);
> +
> + return size;
>
> err:
> dev_err(&chip->cl->dev, "wrong pattern format\n");
> @@ -368,6 +383,192 @@ static void lp5523_firmware_loaded(struct lp55xx_chip *chip)
> lp5523_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)
> +{
> + 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)) {
> + lp5523_run_engine(chip, true);
> + engine->mode = LP55XX_ENGINE_RUN;
> + } else if (!strncmp(buf, "load", 4)) {
> + lp5523_stop_engine(chip);
> + lp5523_load_engine(chip);
> + engine->mode = LP55XX_ENGINE_LOAD;
> + } else if (!strncmp(buf, "disabled", 8)) {
> + lp5523_stop_engine(chip);
> + engine->mode = LP55XX_ENGINE_DISABLED;
> + }
> +
> + mutex_unlock(&chip->lock);
> +
> + return len;
> +}
> +store_mode(1)
> +store_mode(2)
> +store_mode(3)
> +
> +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;
> + u8 mux_page[] = {
> + [LP55XX_ENGINE_1] = LP5523_PAGE_MUX1,
> + [LP55XX_ENGINE_2] = LP5523_PAGE_MUX2,
> + [LP55XX_ENGINE_3] = LP5523_PAGE_MUX3,
> + };
> +
> + lp5523_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 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;
> +
> + mutex_lock(&chip->lock);
> +
> + chip->engine_idx = nr;
> + lp5523_load_engine_and_select_page(chip);
> +
> + mutex_unlock(&chip->lock);
> +
> + return lp5523_update_program_memory(chip, buf, len);
> +}
> +store_load(1)
> +store_load(2)
> +store_load(3)
> +
> static ssize_t lp5523_selftest(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -467,9 +668,27 @@ static void lp5523_led_brightness_work(struct work_struct *work)
> mutex_unlock(&chip->lock);
> }
>
> -static DEVICE_ATTR(selftest, S_IRUGO, lp5523_selftest, NULL);
> +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_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);
> +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);
> +static LP55XX_DEV_ATTR_RO(selftest, lp5523_selftest);
>
> static struct attribute *lp5523_attributes[] = {
> + &dev_attr_engine1_mode.attr,
> + &dev_attr_engine2_mode.attr,
> + &dev_attr_engine3_mode.attr,
> + &dev_attr_engine1_load.attr,
> + &dev_attr_engine2_load.attr,
> + &dev_attr_engine3_load.attr,
> + &dev_attr_engine1_leds.attr,
> + &dev_attr_engine2_leds.attr,
> + &dev_attr_engine3_leds.attr,
> &dev_attr_selftest.attr,
> NULL,
> };
> --
> 1.7.9.5
>

2013-08-13 20:59:52

by Bryan Wu

Subject: Re: [PATCH 08/10] leds: lp5523: remove unnecessary writing commands

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <[email protected]> wrote:
> This patch reduces the number of programming commands.
>
> (Count of sending commands)
> Old code: 32 + program size (32 counts for clearing program memory)
> New code: 32
>
> Pattern buffer is initialized to 0 in this function.
> Just update new program data and remaining buffers are filled with 0.
> So it's needless to clear whole area.
>

Good, but I guess we can share this code with lp5521.c, right?
Thanks,
-Bryan

> Signed-off-by: Milo Kim <[email protected]>
> ---
> drivers/leds/leds-lp5523.c | 14 +++-----------
> 1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index 9b8be6f6..fe3bcbb 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -312,17 +312,11 @@ static int lp5523_update_program_memory(struct lp55xx_chip *chip,
> u8 pattern[LP5523_PROGRAM_LENGTH] = {0};
> unsigned cmd;
> char c[3];
> - int update_size;
> int nrchars;
> - int offset = 0;
> int ret;
> - int i;
> -
> - /* clear program memory before updating */
> - for (i = 0; i < LP5523_PROGRAM_LENGTH; i++)
> - lp55xx_write(chip, LP5523_REG_PROG_MEM + i, 0);
> + int offset = 0;
> + int i = 0;
>
> - i = 0;
> while ((offset < size - 1) && (i < LP5523_PROGRAM_LENGTH)) {
> /* separate sscanfs because length is working only for %s */
> ret = sscanf(data + offset, "%2s%n ", c, &nrchars);
> @@ -342,11 +336,9 @@ static int lp5523_update_program_memory(struct lp55xx_chip *chip,
> if (i % 2)
> goto err;
>
> - update_size = i;
> -
> mutex_lock(&chip->lock);
>
> - for (i = 0; i < update_size; i++) {
> + for (i = 0; i < LP5523_PROGRAM_LENGTH; i++) {
> ret = lp55xx_write(chip, LP5523_REG_PROG_MEM + i, pattern[i]);
> if (ret) {
> mutex_unlock(&chip->lock);
> --
> 1.7.9.5
>

2013-08-13 21:00:28

by Bryan Wu

Subject: Re: [PATCH 09/10] Documentation: leds-lp5521,lp5523: update device attribute information

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <[email protected]> wrote:
> Now, all legacy application interfaces are restored.
> Each driver documentation is updated.
>

Good to merge, thanks,
-Bryan

> Cc: Pali Roh?r <[email protected]>
> Signed-off-by: Milo Kim <[email protected]>
> ---
> Documentation/leds/leds-lp5521.txt | 20 +++++++++++++++++++-
> Documentation/leds/leds-lp5523.txt | 21 ++++++++++++++++++++-
> 2 files changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/leds/leds-lp5521.txt b/Documentation/leds/leds-lp5521.txt
> index 79e4c2e..d08d8c1 100644
> --- a/Documentation/leds/leds-lp5521.txt
> +++ b/Documentation/leds/leds-lp5521.txt
> @@ -18,7 +18,25 @@ All three channels can be also controlled using the engine micro programs.
> More details of the instructions can be found from the public data sheet.
>
> LP5521 has the internal program memory for running various LED patterns.
> -For the details, please refer to 'firmware' section in leds-lp55xx.txt
> +There are two ways to run LED patterns.
> +
> +1) Legacy interface - enginex_mode and enginex_load
> + Control interface for the engines:
> + x is 1 .. 3
> + enginex_mode : disabled, load, run
> + enginex_load : store program (visible only in engine load mode)
> +
> + Example (start to blink the channel 2 led):
> + cd /sys/class/leds/lp5521:channel2/device
> + echo "load" > engine3_mode
> + echo "037f4d0003ff6000" > engine3_load
> + echo "run" > engine3_mode
> +
> + To stop the engine:
> + echo "disabled" > engine3_mode
> +
> +2) Firmware interface - LP55xx common interface
> + For the details, please refer to 'firmware' section in leds-lp55xx.txt
>
> sysfs contains a selftest entry.
> The test communicates with the chip and checks that
> diff --git a/Documentation/leds/leds-lp5523.txt b/Documentation/leds/leds-lp5523.txt
> index 899fdad..5b3e91d 100644
> --- a/Documentation/leds/leds-lp5523.txt
> +++ b/Documentation/leds/leds-lp5523.txt
> @@ -28,7 +28,26 @@ If both fields are NULL, 'lp5523' is used by default.
> /sys/class/leds/lp5523:channelN (N: 0 ~ 8)
>
> LP5523 has the internal program memory for running various LED patterns.
> -For the details, please refer to 'firmware' section in leds-lp55xx.txt
> +There are two ways to run LED patterns.
> +
> +1) Legacy interface - enginex_mode, enginex_load and enginex_leds
> + Control interface for the engines:
> + x is 1 .. 3
> + enginex_mode : disabled, load, run
> + enginex_load : microcode load (visible only in load mode)
> + enginex_leds : led mux control (visible only in load mode)
> +
> + cd /sys/class/leds/lp5523:channel2/device
> + echo "load" > engine3_mode
> + echo "9d80400004ff05ff437f0000" > engine3_load
> + echo "111111111" > engine3_leds
> + echo "run" > engine3_mode
> +
> + To stop the engine:
> + echo "disabled" > engine3_mode
> +
> +2) Firmware interface - LP55xx common interface
> + For the details, please refer to 'firmware' section in leds-lp55xx.txt
>
> Selftest uses always the current from the platform data.
>
> --
> 1.7.9.5
>

2013-08-13 21:01:14

by Bryan Wu

Subject: Re: [PATCH 10/10] leds: lp5562: use LP55xx common macros for device attributes

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <[email protected]> wrote:
>

Good for merge.

-Bryan

> Signed-off-by: Milo Kim <[email protected]>
> ---
> drivers/leds/leds-lp5562.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/leds/leds-lp5562.c b/drivers/leds/leds-lp5562.c
> index a2c7398..2585cfd 100644
> --- a/drivers/leds/leds-lp5562.c
> +++ b/drivers/leds/leds-lp5562.c
> @@ -477,8 +477,8 @@ static ssize_t lp5562_store_engine_mux(struct device *dev,
> return len;
> }
>
> -static DEVICE_ATTR(led_pattern, S_IWUSR, NULL, lp5562_store_pattern);
> -static DEVICE_ATTR(engine_mux, S_IWUSR, NULL, lp5562_store_engine_mux);
> +static LP55XX_DEV_ATTR_WO(led_pattern, lp5562_store_pattern);
> +static LP55XX_DEV_ATTR_WO(engine_mux, lp5562_store_engine_mux);
>
> static struct attribute *lp5562_attributes[] = {
> &dev_attr_led_pattern.attr,
> --
> 1.7.9.5
>

2013-08-13 21:04:38

by Bryan Wu

Subject: Re: [PATCH 00/10] leds: lp5521,5523: restore device attributes for running LED patterns

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <[email protected]> wrote:
> This patch-set resolves the application conflict by restoring sysfs files.
>
> For LP5521
> engine1/2/3_mode
> engine1/2/3_load
>
> For LP5523
> engine1/2/3_mode
> engine1/2/3_load
> engine1/2/3_leds
>
> Those attributes are accessed when LED pattern is run by custom application.
> Those were removed when LED pattern interface was changed to generic firmware
> interface. Please refer to commits below.
>
> git commit 9ce7cb170f97f83a78dc948bf7d25690f15e1328
> (leds-lp5521: use generic firmware interface)
>
> git commit db6eaf8388a413a5ee1b4547ce78506b9c6456b0
> (leds-lp5523: use generic firmware interface)
>
> Necessary attributes are restored in this patch-set.
>
> (Other changes)
> New data structure is added for handling values from/to an application.
> Few code fixes for reducing writing I2C commands.
> Add LP55xx common macros for code refactoring.
> Documentation updates.
>
> You can also pull from the location below
> This branch is based on 'for-next' of linux-leds.
>
> https://github.com/milokim/lp55xx.git resolve-missing-sysfs
>

Thanks, I've already merged the whole patchset in my -devel branch [1].

Pali, could you please help to test it on your hardware? Just grab my
-devel branch and build then run.

Thanks,
-Bryan

[1]: http://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-leds.git/log/?h=devel

> Milo Kim (10):
> leds: lp55xx: add common data structure for program
> leds: lp55xx: add common macros for device attributes
> leds: lp5521: restore legacy device attributes
> leds: lp5521: remove unnecessary writing commands
> leds: lp5523: make separate API for loading engine
> leds: lp5523: LED MUX configuration on initializing
> leds: lp5523: restore legacy device attributes
> leds: lp5523: remove unnecessary writing commands
> Documentation: leds-lp5521,lp5523: update device attribute
> information
> leds: lp5562: use LP55xx common macros for device attributes
>
> Documentation/leds/leds-lp5521.txt | 20 ++-
> Documentation/leds/leds-lp5523.txt | 21 ++-
> drivers/leds/leds-lp5521.c | 114 +++++++++++--
> drivers/leds/leds-lp5523.c | 321 ++++++++++++++++++++++++++++++++++--
> drivers/leds/leds-lp5562.c | 4 +-
> drivers/leds/leds-lp55xx-common.h | 66 ++++++++
> 6 files changed, 511 insertions(+), 35 deletions(-)
>
> --
> 1.7.9.5
>

2013-08-18 22:57:17

by Kim, Milo

Subject: RE: [PATCH 01/10] leds: lp55xx: add common data structure for program

Hi Bryan,

> -----Original Message-----
> From: Bryan Wu [mailto:[email protected]]
> Sent: Wednesday, August 14, 2013 3:56 AM
> To: Milo Kim
> Cc: Pali Roh?r; Linux LED Subsystem; lkml; Kim, Milo
> Subject: Re: [PATCH 01/10] leds: lp55xx: add common data structure for
> program
>
> On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <[email protected]> wrote:
> > LP55xx family devices have internal three program engines which are
> > used for loading LED patterns.
> > To maintain legacy device attributes, specific data structure is used,
> 'mode'
> > and 'led_mux'.
> > The mode is used for showing/storing current engine mode such like
> > disabled, load and run.
> > Then led_mux is used for showing/storing current output LED selection.
> > This is only for LP5523/55231.
> >
>
> This patch looks good to me, but the commit message format is little bit odd
> to me. I will fix that and merge into my tree.

Thanks for your help.
Can I get more detailed information about this format problem?
I need to check my configurations.

Thanks,
Milo-

2013-10-25 16:38:37

by Pali Rohár

Subject: Re: [PATCH 00/10] leds: lp5521,5523: restore device attributes for running LED patterns

On Tuesday 13 August 2013 23:04:14 Bryan Wu wrote:
> On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <[email protected]> wrote:
> > This patch-set resolves the application conflict by
> > restoring sysfs files.
> >
> > For LP5521
> >
> > engine1/2/3_mode
> > engine1/2/3_load
> >
> > For LP5523
> >
> > engine1/2/3_mode
> > engine1/2/3_load
> > engine1/2/3_leds
> >
> > Those attributes are accessed when LED pattern is run by
> > custom application. Those were removed when LED pattern
> > interface was changed to generic firmware interface. Please
> > refer to commits below.
> >
> > git commit 9ce7cb170f97f83a78dc948bf7d25690f15e1328
> > (leds-lp5521: use generic firmware interface)
> >
> > git commit db6eaf8388a413a5ee1b4547ce78506b9c6456b0
> > (leds-lp5523: use generic firmware interface)
> >
> > Necessary attributes are restored in this patch-set.
> >
> > (Other changes)
> > New data structure is added for handling values from/to an
> > application. Few code fixes for reducing writing I2C
> > commands.
> > Add LP55xx common macros for code refactoring.
> > Documentation updates.
> >
> > You can also pull from the location below
> > This branch is based on 'for-next' of linux-leds.
> >
> > https://github.com/milokim/lp55xx.git
> > resolve-missing-sysfs
>
> Thanks, I've already merged the whole patchset in my -devel
> branch [1].
>
> Pali, could you please help to test it on your hardware? Just
> grab my -devel branch and build then run.
>
> Thanks,
> -Bryan
>

Hi, I see that all your patches are part of 3.12-rc5 kernel.

Now I tested this example led program:

# Clearing LED-state to be sure
echo "disabled" > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_mode
echo "disabled" > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_mode
echo 0 > /sys/class/leds/lp5523:r/brightness
echo 0 > /sys/class/leds/lp5523:g/brightness
echo 0 > /sys/class/leds/lp5523:b/brightness

# Setting yellow light pattern and running it
echo "load" > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_mode
echo "000001100" > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_leds
echo "9d804000427f0d7f7f007f0042000000" > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_load
echo "load" > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_mode
echo "000000000" > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_leds
echo "9d800000" > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_load
echo "run" > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_mode
echo "run" > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_mode
echo 20 > /sys/class/leds/lp5523:r/led_current
echo 2 > /sys/class/leds/lp5523:g/led_current
echo 0 > /sys/class/leds/lp5523:b/led_current

All sysfs entries exists and every echo returned 0.

But led does not start blinking that yellow ligh pattern.
So it not working on 3.12-rc5 kernel :-(

--
Pali Rohár
[email protected]

Attachments:

2013-10-25 17:10:32

by Bryan Wu

Subject: Re: [PATCH 00/10] leds: lp5521,5523: restore device attributes for running LED patterns

On Fri, Oct 25, 2013 at 9:38 AM, Pali Roh?r <[email protected]> wrote:
> On Tuesday 13 August 2013 23:04:14 Bryan Wu wrote:
>> On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <[email protected]> wrote:
>> > This patch-set resolves the application conflict by
>> > restoring sysfs files.
>> >
>> > For LP5521
>> >
>> > engine1/2/3_mode
>> > engine1/2/3_load
>> >
>> > For LP5523
>> >
>> > engine1/2/3_mode
>> > engine1/2/3_load
>> > engine1/2/3_leds
>> >
>> > Those attributes are accessed when LED pattern is run by
>> > custom application. Those were removed when LED pattern
>> > interface was changed to generic firmware interface. Please
>> > refer to commits below.
>> >
>> > git commit 9ce7cb170f97f83a78dc948bf7d25690f15e1328
>> > (leds-lp5521: use generic firmware interface)
>> >
>> > git commit db6eaf8388a413a5ee1b4547ce78506b9c6456b0
>> > (leds-lp5523: use generic firmware interface)
>> >
>> > Necessary attributes are restored in this patch-set.
>> >
>> > (Other changes)
>> > New data structure is added for handling values from/to an
>> > application. Few code fixes for reducing writing I2C
>> > commands.
>> > Add LP55xx common macros for code refactoring.
>> > Documentation updates.
>> >
>> > You can also pull from the location below
>> > This branch is based on 'for-next' of linux-leds.
>> >
>> > https://github.com/milokim/lp55xx.git
>> > resolve-missing-sysfs
>>
>> Thanks, I've already merged the whole patchset in my -devel
>> branch [1].
>>
>> Pali, could you please help to test it on your hardware? Just
>> grab my -devel branch and build then run.
>>
>> Thanks,
>> -Bryan
>>
>
> Hi, I see that all your patches are part of 3.12-rc5 kernel.
>
> Now I tested this example led program:
>
> # Clearing LED-state to be sure
> echo "disabled" > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_mode
> echo "disabled" > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_mode
> echo 0 > /sys/class/leds/lp5523:r/brightness
> echo 0 > /sys/class/leds/lp5523:g/brightness
> echo 0 > /sys/class/leds/lp5523:b/brightness
>
> # Setting yellow light pattern and running it
> echo "load" > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_mode
> echo "000001100" > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_leds
> echo "9d804000427f0d7f7f007f0042000000" > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_load
> echo "load" > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_mode
> echo "000000000" > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_leds
> echo "9d800000" > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_load
> echo "run" > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_mode
> echo "run" > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_mode
> echo 20 > /sys/class/leds/lp5523:r/led_current
> echo 2 > /sys/class/leds/lp5523:g/led_current
> echo 0 > /sys/class/leds/lp5523:b/led_current
>
> All sysfs entries exists and every echo returned 0.
>
> But led does not start blinking that yellow ligh pattern.
> So it not working on 3.12-rc5 kernel :-(
>


OK, great! Do you still remember which kernel version works on you system?
Milo, do you have time to take a look? I bet it's a regression somewhere.

Thanks,
-Bryan

2013-10-25 18:21:57

by Pali Rohár

Subject: Re: [PATCH 00/10] leds: lp5521,5523: restore device attributes for running LED patterns

On Friday 25 October 2013 19:10:07 Bryan Wu wrote:
> On Fri, Oct 25, 2013 at 9:38 AM, Pali Rohár
<[email protected]> wrote:
> > On Tuesday 13 August 2013 23:04:14 Bryan Wu wrote:
> >> On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim
<[email protected]> wrote:
> >> > This patch-set resolves the application conflict by
> >> > restoring sysfs files.
> >> >
> >> > For LP5521
> >> >
> >> > engine1/2/3_mode
> >> > engine1/2/3_load
> >> >
> >> > For LP5523
> >> >
> >> > engine1/2/3_mode
> >> > engine1/2/3_load
> >> > engine1/2/3_leds
> >> >
> >> > Those attributes are accessed when LED pattern is run by
> >> > custom application. Those were removed when LED pattern
> >> > interface was changed to generic firmware interface.
> >> > Please refer to commits below.
> >> >
> >> > git commit 9ce7cb170f97f83a78dc948bf7d25690f15e1328
> >> > (leds-lp5521: use generic firmware interface)
> >> >
> >> > git commit db6eaf8388a413a5ee1b4547ce78506b9c6456b0
> >> > (leds-lp5523: use generic firmware interface)
> >> >
> >> > Necessary attributes are restored in this patch-set.
> >> >
> >> > (Other changes)
> >> > New data structure is added for handling values from/to
> >> > an application. Few code fixes for reducing writing I2C
> >> > commands.
> >> > Add LP55xx common macros for code refactoring.
> >> > Documentation updates.
> >> >
> >> > You can also pull from the location below
> >> > This branch is based on 'for-next' of linux-leds.
> >> >
> >> > https://github.com/milokim/lp55xx.git
> >> > resolve-missing-sysfs
> >>
> >> Thanks, I've already merged the whole patchset in my -devel
> >> branch [1].
> >>
> >> Pali, could you please help to test it on your hardware?
> >> Just grab my -devel branch and build then run.
> >>
> >> Thanks,
> >> -Bryan
> >
> > Hi, I see that all your patches are part of 3.12-rc5 kernel.
> >
> > Now I tested this example led program:
> > # Clearing LED-state to be sure
> > echo "disabled" >
> > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_mode echo
> > "disabled" >
> > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_mode echo 0
> > > /sys/class/leds/lp5523:r/brightness
> > echo 0 > /sys/class/leds/lp5523:g/brightness
> > echo 0 > /sys/class/leds/lp5523:b/brightness
> >
> > # Setting yellow light pattern and running it
> > echo "load" >
> > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_mode echo
> > "000001100" >
> > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_leds echo
> > "9d804000427f0d7f7f007f0042000000" >
> > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_load echo
> > "load" >
> > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_mode echo
> > "000000000" >
> > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_leds echo
> > "9d800000" >
> > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_load echo
> > "run" >
> > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_mode echo
> > "run" >
> > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_mode echo
> > 20 > /sys/class/leds/lp5523:r/led_current
> > echo 2 > /sys/class/leds/lp5523:g/led_current
> > echo 0 > /sys/class/leds/lp5523:b/led_current
> >
> > All sysfs entries exists and every echo returned 0.
> >
> > But led does not start blinking that yellow ligh pattern.
> > So it not working on 3.12-rc5 kernel :-(
>
> OK, great! Do you still remember which kernel version works on
> you system? Milo, do you have time to take a look? I bet it's
> a regression somewhere.
>
> Thanks,
> -Bryan

I do not know which version. Now I tried pattern example from
Documentation/leds/leds-lp55xx.txt which using new API.

echo 2 > /sys/class/i2c-adapter/i2c-2/2-0032/select_engine
echo 1 > /sys/class/firmware/lp5523/loading
echo "9d80400004ff05ff437f0000" > /sys/class/firmware/lp5523/data
echo 0 > /sys/class/firmware/lp5523/loading
echo 1 > /sys/class/i2c-adapter/i2c-2/2-0032/run_engine

But it failed at second command. In directory /sys/class/firmware/
I have only one file with name timeout. Nothing more, no lp5523
folder.

Any idea who and when creating that lp5523 folder?

--
Pali Rohár
[email protected]

Attachments:

2013-10-29 23:17:35

by Bryan Wu

Subject: Re: [PATCH 00/10] leds: lp5521,5523: restore device attributes for running LED patterns

On Fri, Oct 25, 2013 at 11:21 AM, Pali Roh?r <[email protected]> wrote:
> On Friday 25 October 2013 19:10:07 Bryan Wu wrote:
>> On Fri, Oct 25, 2013 at 9:38 AM, Pali Roh?r
> <[email protected]> wrote:
>> > On Tuesday 13 August 2013 23:04:14 Bryan Wu wrote:
>> >> On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim
> <[email protected]> wrote:
>> >> > This patch-set resolves the application conflict by
>> >> > restoring sysfs files.
>> >> >
>> >> > For LP5521
>> >> >
>> >> > engine1/2/3_mode
>> >> > engine1/2/3_load
>> >> >
>> >> > For LP5523
>> >> >
>> >> > engine1/2/3_mode
>> >> > engine1/2/3_load
>> >> > engine1/2/3_leds
>> >> >
>> >> > Those attributes are accessed when LED pattern is run by
>> >> > custom application. Those were removed when LED pattern
>> >> > interface was changed to generic firmware interface.
>> >> > Please refer to commits below.
>> >> >
>> >> > git commit 9ce7cb170f97f83a78dc948bf7d25690f15e1328
>> >> > (leds-lp5521: use generic firmware interface)
>> >> >
>> >> > git commit db6eaf8388a413a5ee1b4547ce78506b9c6456b0
>> >> > (leds-lp5523: use generic firmware interface)
>> >> >
>> >> > Necessary attributes are restored in this patch-set.
>> >> >
>> >> > (Other changes)
>> >> > New data structure is added for handling values from/to
>> >> > an application. Few code fixes for reducing writing I2C
>> >> > commands.
>> >> > Add LP55xx common macros for code refactoring.
>> >> > Documentation updates.
>> >> >
>> >> > You can also pull from the location below
>> >> > This branch is based on 'for-next' of linux-leds.
>> >> >
>> >> > https://github.com/milokim/lp55xx.git
>> >> > resolve-missing-sysfs
>> >>
>> >> Thanks, I've already merged the whole patchset in my -devel
>> >> branch [1].
>> >>
>> >> Pali, could you please help to test it on your hardware?
>> >> Just grab my -devel branch and build then run.
>> >>
>> >> Thanks,
>> >> -Bryan
>> >
>> > Hi, I see that all your patches are part of 3.12-rc5 kernel.
>> >
>> > Now I tested this example led program:
>> > # Clearing LED-state to be sure
>> > echo "disabled" >
>> > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_mode echo
>> > "disabled" >
>> > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_mode echo 0
>> > > /sys/class/leds/lp5523:r/brightness
>> > echo 0 > /sys/class/leds/lp5523:g/brightness
>> > echo 0 > /sys/class/leds/lp5523:b/brightness
>> >
>> > # Setting yellow light pattern and running it
>> > echo "load" >
>> > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_mode echo
>> > "000001100" >
>> > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_leds echo
>> > "9d804000427f0d7f7f007f0042000000" >
>> > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_load echo
>> > "load" >
>> > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_mode echo
>> > "000000000" >
>> > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_leds echo
>> > "9d800000" >
>> > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_load echo
>> > "run" >
>> > /sys/class/i2c-adapter/i2c-2/2-0032/engine2_mode echo
>> > "run" >
>> > /sys/class/i2c-adapter/i2c-2/2-0032/engine1_mode echo
>> > 20 > /sys/class/leds/lp5523:r/led_current
>> > echo 2 > /sys/class/leds/lp5523:g/led_current
>> > echo 0 > /sys/class/leds/lp5523:b/led_current
>> >
>> > All sysfs entries exists and every echo returned 0.
>> >
>> > But led does not start blinking that yellow ligh pattern.
>> > So it not working on 3.12-rc5 kernel :-(
>>
>> OK, great! Do you still remember which kernel version works on
>> you system? Milo, do you have time to take a look? I bet it's
>> a regression somewhere.
>>
>> Thanks,
>> -Bryan
>
> I do not know which version. Now I tried pattern example from
> Documentation/leds/leds-lp55xx.txt which using new API.
>
> echo 2 > /sys/class/i2c-adapter/i2c-2/2-0032/select_engine
> echo 1 > /sys/class/firmware/lp5523/loading
> echo "9d80400004ff05ff437f0000" > /sys/class/firmware/lp5523/data
> echo 0 > /sys/class/firmware/lp5523/loading
> echo 1 > /sys/class/i2c-adapter/i2c-2/2-0032/run_engine
>
> But it failed at second command. In directory /sys/class/firmware/
> I have only one file with name timeout. Nothing more, no lp5523
> folder.
>
> Any idea who and when creating that lp5523 folder?

Milo,

Do you have time to take a look at this?

Thanks,
-Bryan