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

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

[Milo Kim]

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

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

[Milo Kim]

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 <milo.kim@ti.com>
---
 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

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

[Bryan Wu]

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <woogyom.kim@gmail.com> 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 <milo.kim@ti.com>
> ---
>  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
>

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

[Kim, Milo]

Hi Bryan,

> -----Original Message-----
> From: Bryan Wu [mailto:cooloney@gmail.com]
> 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 <woogyom.kim@gmail.com> 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-
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Milo Kim]

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 <milo.kim@ti.com>
---
 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

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

[Bryan Wu]

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <woogyom.kim@gmail.com> 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 <milo.kim@ti.com>
> ---
>  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
>

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

[Milo Kim]

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 <milo.kim@ti.com>
---
 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

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

[Bryan Wu]

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <woogyom.kim@gmail.com> 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 <milo.kim@ti.com>
> ---
>  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
>

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

[Milo Kim]

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 <milo.kim@ti.com>
---
 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

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

[Bryan Wu]

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <woogyom.kim@gmail.com> 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 <milo.kim@ti.com>
> ---
>  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
>

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

[Milo Kim]

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 <milo.kim@ti.com>
---
 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

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

[Bryan Wu]

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <woogyom.kim@gmail.com> 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 <milo.kim@ti.com>
> ---
>  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
>

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

[Milo Kim]

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 <pali.rohar@gmail.com>
Signed-off-by: Milo Kim <milo.kim@ti.com>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bryan Wu]

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <woogyom.kim@gmail.com> 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 <pali.rohar@gmail.com>
> Signed-off-by: Milo Kim <milo.kim@ti.com>
> ---
>  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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Milo Kim]

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 <pali.rohar@gmail.com>
Signed-off-by: Milo Kim <milo.kim@ti.com>
---
 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

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

[Bryan Wu]

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <woogyom.kim@gmail.com> 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 <pali.rohar@gmail.com>
> Signed-off-by: Milo Kim <milo.kim@ti.com>
> ---
>  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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Milo Kim]

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 <milo.kim@ti.com>
---
 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

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

[Bryan Wu]

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <woogyom.kim@gmail.com> 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 <milo.kim@ti.com>
> ---
>  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
>

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

[Milo Kim]

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

Cc: Pali Rohár <pali.rohar@gmail.com>
Signed-off-by: Milo Kim <milo.kim@ti.com>
---
 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

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

[Bryan Wu]

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <woogyom.kim@gmail.com> wrote:
> Now, all legacy application interfaces are restored.
> Each driver documentation is updated.
>

Good to merge, thanks,
-Bryan

> Cc: Pali Rohár <pali.rohar@gmail.com>
> Signed-off-by: Milo Kim <milo.kim@ti.com>
> ---
>  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
>

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

[Milo Kim]

Signed-off-by: Milo Kim <milo.kim@ti.com>
---
 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

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

[Bryan Wu]

On Thu, Aug 8, 2013 at 12:59 AM, Milo Kim <woogyom.kim@gmail.com> wrote:
>

Good for merge.

-Bryan

> Signed-off-by: Milo Kim <milo.kim@ti.com>
> ---
>  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
>

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

[Bryan Wu]

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

[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
>

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3001 bytes --]

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

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
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Bryan Wu]

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
>
> 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

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 4272 bytes --]

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
> > 
> > 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
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Bryan Wu]

On Fri, Oct 25, 2013 at 11:21 AM, Pali Rohár <pali.rohar@gmail.com> 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
>> >
>> > 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

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

[Milo Kim]

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

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

[Pali Rohár]

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

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

[Bryan Wu]

On Tue, Nov 19, 2013 at 2:35 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> 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.
>

That's good. We can post this fixing patch to stable tree as well as
latest upstream kernel.

Milo, do you need to update your patch, if not I'm going to take it
into my tree and for stable tree.

Thanks,
-Bryan

> 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

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

[Milo Kim]

Hi Bryan,

On 11/20/2013 04:20 AM, Bryan Wu wrote:
> On Tue, Nov 19, 2013 at 2:35 AM, Pali Rohár<pali.rohar@gmail.com>  wrote:
>> >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.
>> >
> That's good. We can post this fixing patch to stable tree as well as
> latest upstream kernel.
>
> Milo, do you need to update your patch, if not I'm going to take it
> into my tree and for stable tree.

I'll send the patch-set again because LP5521 is also required to be fixed.
And _remove() also needs to be updated.
I can send it by tomorrow.

Thanks,
- Milo