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

["Pali Rohár" <pali.rohar@gmail.com>]

Hello,

now I tested that patch on top of 3.12-rc5 with this led example:

    # 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

And now it working, led blinks :-) I also tested Maemo
application for managing lp5523 led and now it working too.

But new interface via request_firmware still not working.
Directory /sys/class/firmware/lp5523/data not exists.

I think that there can be race condition with udev which is
responsible for firmware loading... Anyway main problem solved,
old interface is working again and old applications too.

On Tuesday 19 November 2013 02:13:49 Bryan Wu wrote:
> Pali,
> 
> Could you please test Milo's patch?
> 
> Thanks,
> -Bryan
> 
> On Thu, Nov 7, 2013 at 9:15 PM, Milo Kim <milo.kim@ti.com> wrote:
> > Hi Pali,
> > 
> > Sorry for the late reply.
> > 
> > On 10/26/2013 03:21 AM, Pali Rohár wrote:
> >> On Friday 25 October 2013 19:10:07 Bryan Wu wrote:
> >>> On Fri, Oct 25, 2013 at 9:38 AM, Pali Rohár
> >> 
> >> <pali.rohar@gmail.com> wrote:
> >>> On Tuesday 13 August 2013 23:04:14 Bryan Wu wrote:
> >>>>> On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim
> >>>> 
> >>>> <woogyom.kim@gmail.com> 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
> > 
> > In the latest LP5523 driver, I just found wrong operation
> > mode setting in case of multiple engine used. (please find
> > a patch below)
> > 
> >>>> 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.
> > 
> > I can't reproduce this issue but I found some duplicated
> > mutex problem. But in my case, I could found
> > /sys/class/firmware/lp5523 directory. Is any permission
> > issue? Anyway, I've added fixed mutex code below.
> > 
> > Could you check this patch below?
> > If it's resolved, then I'll send official patch-set.
> > 
> > -------------------- 8< ---------------- 8<
> > ---------------------------
> > 
> > From dad27e0d834a5635232116569aaa1fa471ac46bf Mon Sep 17
> > 00:00:00 2001 From: Milo Kim <milo.kim@ti.com>
> > Date: Fri, 8 Nov 2013 13:43:33 +0900
> > Subject: [PATCH] leds: lp5523: Fix multiple engine usage bug
> > and remove
> > 
> >  duplicated mutex
> > 
> > Fix multiple engine usage bug:
> >   Whenever the engine is loaded by the user-application, the
> >   operation
> > 
> > mode is
> > 
> >   reset first. But it has a problem in case of multiple
> >   engine used because previous engine settings are cleared.
> >   The driver should update not whole 8bits but each engine
> >   bits by masking.
> > 
> > Remove duplicate mutex:
> >   It can be a problem when a pattern is loaded via the
> >   firmware interface. LP55xx common driver has already
> >   locked the mutex in 'lp55xx_firmware_loaded()'.
> >   So it should be deleted.
> > 
> > Signed-off-by: Milo Kim <milo.kim@ti.com>
> > ---
> > 
> >  drivers/leds/leds-lp5523.c |   14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/leds/leds-lp5523.c
> > b/drivers/leds/leds-lp5523.c index fe3bcbb..c00f55e 100644
> > --- a/drivers/leds/leds-lp5523.c
> > +++ b/drivers/leds/leds-lp5523.c
> > @@ -188,7 +188,15 @@ static void
> > lp5523_load_engine_and_select_page(struct lp55xx_chip
> > *chip)
> > 
> >  static void lp5523_stop_engine(struct lp55xx_chip *chip)
> >  {
> > 
> > -       lp55xx_write(chip, LP5523_REG_OP_MODE, 0);
> > +       enum lp55xx_engine_index idx = chip->engine_idx;
> > +       u8 mask[] = {
> > +               [LP55XX_ENGINE_1] = LP5523_MODE_ENG1_M,
> > +               [LP55XX_ENGINE_2] = LP5523_MODE_ENG2_M,
> > +               [LP55XX_ENGINE_3] = LP5523_MODE_ENG3_M,
> > +       };
> > +
> > +       lp55xx_update_bits(chip, LP5523_REG_OP_MODE,
> > mask[idx], 0); +
> > 
> >         lp5523_wait_opmode_done();
> >  
> >  }
> > 
> > @@ -336,8 +344,6 @@ static int
> > lp5523_update_program_memory(struct lp55xx_chip *chip,
> > 
> >         if (i % 2)
> >         
> >                 goto err;
> > 
> > -       mutex_lock(&chip->lock);
> > -
> > 
> >         for (i = 0; i < LP5523_PROGRAM_LENGTH; i++) {
> >         
> >                 ret = lp55xx_write(chip, LP5523_REG_PROG_MEM
> >                 + i,
> > 
> > pattern[i]);
> > 
> >                 if (ret) {
> > 
> > @@ -346,8 +352,6 @@ static int
> > lp5523_update_program_memory(struct lp55xx_chip *chip,
> > 
> >                 }
> >         
> >         }
> > 
> > -       mutex_unlock(&chip->lock);
> > -
> > 
> >         return size;
> >  
> >  err:
> > --
> > 1.7.9.5
> > -------------------- 8< ---------------- 8<
> > ---------------------------
> > 
> > Best regards,
> > Milo

-- 
Pali Rohár
pali.rohar@gmail.com