Hi!
There's some locking weirdness, and few missing comments in lp5523
driver.
Now, this is untested patch from my reverse-engineering. I hope I
understood things right...
In particular, there's unbalanced unlock in
lp5523_update_program_memory, and lp5523_update_program_memory needs
to be protected by the lock.
Comments? Does someone maintain this?
Thanks,
Pavel
Signed-off-by: Pavel Machek <[email protected]>
diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index c00f55e4..e8b83e9 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -34,7 +34,15 @@
#include "leds-lp55xx-common.h"
-#define LP5523_PROGRAM_LENGTH 32
+#define LP5523_PROGRAM_LENGTH 32 /* bytes */
+/* Memory is used like this:
+ 0x00 engine 1 program
+ 0x10 engine 2 program
+ 0x20 engine 3 program
+ 0x30 engine 1 muxing info
+ 0x40 engine 2 muxing info
+ 0x50 engine 3 muxing info
+ ...and offsets are hard-coded all around :-( */
#define LP5523_MAX_LEDS 9
/* Registers */
@@ -265,20 +273,25 @@ static int lp5523_init_program_engine(struct lp55xx_chip *chip)
/* 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},
+ /* 9c30 -- mux_map_start(0x30)
+ 9cb0 -- mux_ld_end(0x50)
+ 9d80 -- mux_sel???(0x00)
+ d800 -- invalid?? */
{ 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 */
+ /* hardcode LP5523_PROGRAM_LENGTH 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);
+ ret = lp55xx_write(chip, LP5523_REG_CH2_PROG_START, LP5523_PROGRAM_LENGTH / 2);
if (ret)
return ret;
- ret = lp55xx_write(chip, LP5523_REG_CH3_PROG_START, 0x20);
+ ret = lp55xx_write(chip, LP5523_REG_CH3_PROG_START, LP5523_PROGRAM_LENGTH);
if (ret)
return ret;
@@ -346,10 +359,8 @@ static int lp5523_update_program_memory(struct lp55xx_chip *chip,
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);
+ if (ret)
return -EINVAL;
- }
}
return size;
@@ -551,15 +562,17 @@ static ssize_t store_engine_load(struct device *dev,
{
struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
struct lp55xx_chip *chip = led->chip;
+ ssize_t res;
mutex_lock(&chip->lock);
chip->engine_idx = nr;
lp5523_load_engine_and_select_page(chip);
+ res = lp5523_update_program_memory(chip, buf, len);
mutex_unlock(&chip->lock);
- return lp5523_update_program_memory(chip, buf, len);
+ return res;
}
store_load(1)
store_load(2)
diff --git a/drivers/leds/leds-lp8501.c b/drivers/leds/leds-lp8501.c
index 8d55a780..ae5c6fe 100644
--- a/drivers/leds/leds-lp8501.c
+++ b/drivers/leds/leds-lp8501.c
@@ -262,7 +262,7 @@ static void lp8501_firmware_loaded(struct lp55xx_chip *chip)
}
/*
- * Program momery sequence
+ * Program memory sequence
* 1) set engine mode to "LOAD"
* 2) write firmware data into program memory
*/
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tue, Jan 7, 2014 at 12:42 PM, Pavel Machek <[email protected]> wrote:
> Hi!
>
> There's some locking weirdness, and few missing comments in lp5523
> driver.
>
> Now, this is untested patch from my reverse-engineering. I hope I
> understood things right...
>
> In particular, there's unbalanced unlock in
> lp5523_update_program_memory, and lp5523_update_program_memory needs
> to be protected by the lock.
>
> Comments? Does someone maintain this?
>
I think Milo Kim is maintaining this driver for the LP55xx chip family.
Thanks,
-Bryan
> Thanks,
> Pavel
> Signed-off-by: Pavel Machek <[email protected]>
>
>
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index c00f55e4..e8b83e9 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -34,7 +34,15 @@
>
> #include "leds-lp55xx-common.h"
>
> -#define LP5523_PROGRAM_LENGTH 32
> +#define LP5523_PROGRAM_LENGTH 32 /* bytes */
> +/* Memory is used like this:
> + 0x00 engine 1 program
> + 0x10 engine 2 program
> + 0x20 engine 3 program
> + 0x30 engine 1 muxing info
> + 0x40 engine 2 muxing info
> + 0x50 engine 3 muxing info
> + ...and offsets are hard-coded all around :-( */
> #define LP5523_MAX_LEDS 9
>
> /* Registers */
> @@ -265,20 +273,25 @@ static int lp5523_init_program_engine(struct lp55xx_chip *chip)
> /* 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},
> + /* 9c30 -- mux_map_start(0x30)
> + 9cb0 -- mux_ld_end(0x50)
> + 9d80 -- mux_sel???(0x00)
> + d800 -- invalid?? */
> { 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 */
> + /* hardcode LP5523_PROGRAM_LENGTH 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);
> + ret = lp55xx_write(chip, LP5523_REG_CH2_PROG_START, LP5523_PROGRAM_LENGTH / 2);
> if (ret)
> return ret;
>
> - ret = lp55xx_write(chip, LP5523_REG_CH3_PROG_START, 0x20);
> + ret = lp55xx_write(chip, LP5523_REG_CH3_PROG_START, LP5523_PROGRAM_LENGTH);
> if (ret)
> return ret;
>
> @@ -346,10 +359,8 @@ static int lp5523_update_program_memory(struct lp55xx_chip *chip,
>
> 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);
> + if (ret)
> return -EINVAL;
> - }
> }
>
> return size;
> @@ -551,15 +562,17 @@ static ssize_t store_engine_load(struct device *dev,
> {
> struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
> struct lp55xx_chip *chip = led->chip;
> + ssize_t res;
>
> mutex_lock(&chip->lock);
>
> chip->engine_idx = nr;
> lp5523_load_engine_and_select_page(chip);
>
> + res = lp5523_update_program_memory(chip, buf, len);
> mutex_unlock(&chip->lock);
>
> - return lp5523_update_program_memory(chip, buf, len);
> + return res;
> }
> store_load(1)
> store_load(2)
> diff --git a/drivers/leds/leds-lp8501.c b/drivers/leds/leds-lp8501.c
> index 8d55a780..ae5c6fe 100644
> --- a/drivers/leds/leds-lp8501.c
> +++ b/drivers/leds/leds-lp8501.c
> @@ -262,7 +262,7 @@ static void lp8501_firmware_loaded(struct lp55xx_chip *chip)
> }
>
> /*
> - * Program momery sequence
> + * Program memory sequence
> * 1) set engine mode to "LOAD"
> * 2) write firmware data into program memory
> */
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tue, Jan 7, 2014 at 12:42 PM, Pavel Machek <[email protected]> wrote:
> Hi!
>
> There's some locking weirdness, and few missing comments in lp5523
> driver.
>
> Now, this is untested patch from my reverse-engineering. I hope I
> understood things right...
>
> In particular, there's unbalanced unlock in
> lp5523_update_program_memory, and lp5523_update_program_memory needs
> to be protected by the lock.
>
> Comments? Does someone maintain this?
>
Actually the locking issue was fixed in my for-next branch,
http://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-leds.git/commit/?h=for-next&id=0d70bdb957cde2c25a4b3e4c93d0d33403795be9
Thanks,
-Bryan
> Thanks,
> Pavel
> Signed-off-by: Pavel Machek <[email protected]>
>
>
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index c00f55e4..e8b83e9 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -34,7 +34,15 @@
>
> #include "leds-lp55xx-common.h"
>
> -#define LP5523_PROGRAM_LENGTH 32
> +#define LP5523_PROGRAM_LENGTH 32 /* bytes */
> +/* Memory is used like this:
> + 0x00 engine 1 program
> + 0x10 engine 2 program
> + 0x20 engine 3 program
> + 0x30 engine 1 muxing info
> + 0x40 engine 2 muxing info
> + 0x50 engine 3 muxing info
> + ...and offsets are hard-coded all around :-( */
> #define LP5523_MAX_LEDS 9
>
> /* Registers */
> @@ -265,20 +273,25 @@ static int lp5523_init_program_engine(struct lp55xx_chip *chip)
> /* 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},
> + /* 9c30 -- mux_map_start(0x30)
> + 9cb0 -- mux_ld_end(0x50)
> + 9d80 -- mux_sel???(0x00)
> + d800 -- invalid?? */
> { 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 */
> + /* hardcode LP5523_PROGRAM_LENGTH 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);
> + ret = lp55xx_write(chip, LP5523_REG_CH2_PROG_START, LP5523_PROGRAM_LENGTH / 2);
> if (ret)
> return ret;
>
> - ret = lp55xx_write(chip, LP5523_REG_CH3_PROG_START, 0x20);
> + ret = lp55xx_write(chip, LP5523_REG_CH3_PROG_START, LP5523_PROGRAM_LENGTH);
> if (ret)
> return ret;
>
> @@ -346,10 +359,8 @@ static int lp5523_update_program_memory(struct lp55xx_chip *chip,
>
> 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);
> + if (ret)
> return -EINVAL;
> - }
> }
>
> return size;
> @@ -551,15 +562,17 @@ static ssize_t store_engine_load(struct device *dev,
> {
> struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
> struct lp55xx_chip *chip = led->chip;
> + ssize_t res;
>
> mutex_lock(&chip->lock);
>
> chip->engine_idx = nr;
> lp5523_load_engine_and_select_page(chip);
>
> + res = lp5523_update_program_memory(chip, buf, len);
> mutex_unlock(&chip->lock);
>
> - return lp5523_update_program_memory(chip, buf, len);
> + return res;
> }
> store_load(1)
> store_load(2)
> diff --git a/drivers/leds/leds-lp8501.c b/drivers/leds/leds-lp8501.c
> index 8d55a780..ae5c6fe 100644
> --- a/drivers/leds/leds-lp8501.c
> +++ b/drivers/leds/leds-lp8501.c
> @@ -262,7 +262,7 @@ static void lp8501_firmware_loaded(struct lp55xx_chip *chip)
> }
>
> /*
> - * Program momery sequence
> + * Program memory sequence
> * 1) set engine mode to "LOAD"
> * 2) write firmware data into program memory
> */
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wednesday 08 January 2014 01:32:15 Bryan Wu wrote:
> On Tue, Jan 7, 2014 at 12:42 PM, Pavel Machek <[email protected]>
wrote:
> > Hi!
> >
> > There's some locking weirdness, and few missing comments in
> > lp5523 driver.
> >
> > Now, this is untested patch from my reverse-engineering. I
> > hope I understood things right...
> >
> > In particular, there's unbalanced unlock in
> > lp5523_update_program_memory, and
> > lp5523_update_program_memory needs to be protected by the
> > lock.
> >
> > Comments? Does someone maintain this?
>
> Actually the locking issue was fixed in my for-next branch,
> http://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-led
> s.git/commit/?h=for-next&id=0d70bdb957cde2c25a4b3e4c93d0d33403
> 795be9
>
> Thanks,
> -Bryan
>
Now, with linus tree lp5523 driver does not work without above
commit on Nokia N900 device. I think that commit should go to
3.13 and stable... What do you think?
--
Pali Rohár
[email protected]
On Thu, Jan 9, 2014 at 6:24 AM, Pali Roh?r <[email protected]> wrote:
> On Wednesday 08 January 2014 01:32:15 Bryan Wu wrote:
>> On Tue, Jan 7, 2014 at 12:42 PM, Pavel Machek <[email protected]>
> wrote:
>> > Hi!
>> >
>> > There's some locking weirdness, and few missing comments in
>> > lp5523 driver.
>> >
>> > Now, this is untested patch from my reverse-engineering. I
>> > hope I understood things right...
>> >
>> > In particular, there's unbalanced unlock in
>> > lp5523_update_program_memory, and
>> > lp5523_update_program_memory needs to be protected by the
>> > lock.
>> >
>> > Comments? Does someone maintain this?
>>
>> Actually the locking issue was fixed in my for-next branch,
>> http://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-led
>> s.git/commit/?h=for-next&id=0d70bdb957cde2c25a4b3e4c93d0d33403
>> 795be9
>>
>> Thanks,
>> -Bryan
>>
>
> Now, with linus tree lp5523 driver does not work without above
> commit on Nokia N900 device. I think that commit should go to
> 3.13 and stable... What do you think?
>
OK, can you just apply this fix on Linus tree and verify it on N900?
If only this fix will make N900 LED work, I will send out to Linus as
a critical fix for 3.13 as well as stable tree. Because there 2 more
patches for LED comes from Milo also fixed something and this patch
probably depends on them.
Please help to verify.
Thanks,
-Bryan
Hi!
> > Now, this is untested patch from my reverse-engineering. I hope I
> > understood things right...
> >
> > In particular, there's unbalanced unlock in
> > lp5523_update_program_memory, and lp5523_update_program_memory needs
> > to be protected by the lock.
> >
> > Comments? Does someone maintain this?
> >
>
> Actually the locking issue was fixed in my for-next branch,
> http://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-leds.git/commit/?h=for-next&id=0d70bdb957cde2c25a4b3e4c93d0d33403795be9
>
Yes, that should solve it. Thanks!
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> > There's some locking weirdness, and few missing comments in lp5523
> > driver.
> >
> > Now, this is untested patch from my reverse-engineering. I hope I
> > understood things right...
> >
> > In particular, there's unbalanced unlock in
> > lp5523_update_program_memory, and lp5523_update_program_memory needs
> > to be protected by the lock.
> >
> > Comments? Does someone maintain this?
>
> I think Milo Kim is maintaining this driver for the LP55xx chip
> family.
Yes, he's on the Cc list. I was hoping he would comment.
> > -#define LP5523_PROGRAM_LENGTH 32
> > +#define LP5523_PROGRAM_LENGTH 32 /* bytes */
> > +/* Memory is used like this:
> > + 0x00 engine 1 program
> > + 0x10 engine 2 program
> > + 0x20 engine 3 program
> > + 0x30 engine 1 muxing info
> > + 0x40 engine 2 muxing info
> > + 0x50 engine 3 muxing info
> > + ...and offsets are hard-coded all around :-( */
For example, I'd like to increase limit here -- 62 byte programs
should be feasible AFAICT.
> > #define LP5523_MAX_LEDS 9
> >
> > /* Registers */
> > @@ -265,20 +273,25 @@ static int lp5523_init_program_engine(struct lp55xx_chip *chip)
> > /* 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},
> > + /* 9c30 -- mux_map_start(0x30)
> > + 9cb0 -- mux_ld_end(0x50)
> > + 9d80 -- mux_sel???(0x00)
> > + d800 -- invalid?? */
And it would be good to get comment on what this does. I could not
disassemble/decipher it.
Regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Add some comments that are not obvious from first look at the driver
to lp5523, fix typo in lp8501.
Signed-off-by: Pavel Machek <[email protected]>
diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index c00f55e4..ac64ebc 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -34,7 +34,15 @@
#include "leds-lp55xx-common.h"
-#define LP5523_PROGRAM_LENGTH 32
+#define LP5523_PROGRAM_LENGTH 32 /* bytes */
+/* Memory is used like this:
+ 0x00 engine 1 program
+ 0x10 engine 2 program
+ 0x20 engine 3 program
+ 0x30 engine 1 muxing info
+ 0x40 engine 2 muxing info
+ 0x50 engine 3 muxing info
+*/
#define LP5523_MAX_LEDS 9
/* Registers */
index 8d55a780..ae5c6fe 100644
--- a/drivers/leds/leds-lp8501.c
+++ b/drivers/leds/leds-lp8501.c
@@ -262,7 +262,7 @@ static void lp8501_firmware_loaded(struct lp55xx_chip *chip)
}
/*
- * Program momery sequence
+ * Program memory sequence
* 1) set engine mode to "LOAD"
* 2) write firmware data into program memory
*/
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Thu, Jan 9, 2014 at 3:13 PM, Pavel Machek <[email protected]> wrote:
> Add some comments that are not obvious from first look at the driver
> to lp5523, fix typo in lp8501.
>
Milo, can you take a look at this. If it's OK, I will take it.
-Bryan
> Signed-off-by: Pavel Machek <[email protected]>
>
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index c00f55e4..ac64ebc 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -34,7 +34,15 @@
>
> #include "leds-lp55xx-common.h"
>
> -#define LP5523_PROGRAM_LENGTH 32
> +#define LP5523_PROGRAM_LENGTH 32 /* bytes */
> +/* Memory is used like this:
> + 0x00 engine 1 program
> + 0x10 engine 2 program
> + 0x20 engine 3 program
> + 0x30 engine 1 muxing info
> + 0x40 engine 2 muxing info
> + 0x50 engine 3 muxing info
> +*/
> #define LP5523_MAX_LEDS 9
>
> /* Registers */
> index 8d55a780..ae5c6fe 100644
> --- a/drivers/leds/leds-lp8501.c
> +++ b/drivers/leds/leds-lp8501.c
> @@ -262,7 +262,7 @@ static void lp8501_firmware_loaded(struct lp55xx_chip *chip)
> }
>
> /*
> - * Program momery sequence
> + * Program memory sequence
> * 1) set engine mode to "LOAD"
> * 2) write firmware data into program memory
> */
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html