2010-12-17 10:20:29

by Samu Onkalo

[permalink] [raw]
Subject: [PATCH 0/3] leds: lp5521 and lp523 corrections

Both drivers created and removed sysfs entries based on the
programmable engine state. Engine and mux control sysfs entries
were visible only when the engine was set to load state.

However, this caused circular locking error between sysfs-core
and the driver internal mutex. Now all the sysfs entries are
visible all the time but the access to engine program load
entry causes error if the engine mode is not correct.

lp5523 led channel naming changed to support name from the
platform data to allow use of several chips without overlapping
sysfs names.

Samu Onkalo (3):
leds: leds-lp5523: modify the way of setting led device name
leds: lp5523: Fix circular locking
leds: lp5521: Fix circular locking

drivers/leds/leds-lp5521.c | 52 ++++++--------------------------------
drivers/leds/leds-lp5523.c | 59 +++++++++++++-----------------------------
include/linux/leds-lp5523.h | 1 +
3 files changed, 27 insertions(+), 85 deletions(-)


2010-12-17 10:20:30

by Samu Onkalo

[permalink] [raw]
Subject: [PATCH 2/3] leds: lp5523: Fix circular locking

Driver contained possibility for circular locking.
One lock is held by sysfs-core and another one by the driver itself.
This happened when the driver created or removed sysfs entries dynamically.
There is no real need to do those operations. Now all the sysfs entries
are created at probe and removed at removal. Engine load and mux configuration
sysfs entries are now visible all the time. However, access to the entries
fails if the engine is disabled or running.

Signed-off-by: Samu Onkalo <[email protected]>
---
drivers/leds/leds-lp5523.c | 56 ++++++++++++-------------------------------
1 files changed, 16 insertions(+), 40 deletions(-)

diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index 6cb4160..d0c4068 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -105,7 +105,6 @@
#define SHIFT_MASK(id) (((id) - 1) * 2)

struct lp5523_engine {
- const struct attribute_group *attributes;
int id;
u8 mode;
u8 prog_page;
@@ -403,14 +402,23 @@ static ssize_t store_engine_leds(struct device *dev,
struct i2c_client *client = to_i2c_client(dev);
struct lp5523_chip *chip = i2c_get_clientdata(client);
u16 mux = 0;
+ ssize_t ret;

if (lp5523_mux_parse(buf, &mux, len))
return -EINVAL;

+ mutex_lock(&chip->lock);
+ ret = -EINVAL;
+ if (chip->engines[nr - 1].mode != LP5523_CMD_LOAD)
+ goto leave;
+
if (lp5523_load_mux(&chip->engines[nr - 1], mux))
- return -EINVAL;
+ goto leave;

- return len;
+ ret = len;
+leave:
+ mutex_unlock(&chip->lock);
+ return ret;
}

#define store_leds(nr) \
@@ -556,7 +564,11 @@ static int lp5523_do_store_load(struct lp5523_engine *engine,

mutex_lock(&chip->lock);

- ret = lp5523_load_program(engine, pattern);
+ if (engine->mode == LP5523_CMD_LOAD)
+ ret = lp5523_load_program(engine, pattern);
+ else
+ ret = -EINVAL;
+
mutex_unlock(&chip->lock);

if (ret) {
@@ -737,37 +749,18 @@ static struct attribute *lp5523_attributes[] = {
&dev_attr_engine2_mode.attr,
&dev_attr_engine3_mode.attr,
&dev_attr_selftest.attr,
- NULL
-};
-
-static struct attribute *lp5523_engine1_attributes[] = {
&dev_attr_engine1_load.attr,
&dev_attr_engine1_leds.attr,
- NULL
-};
-
-static struct attribute *lp5523_engine2_attributes[] = {
&dev_attr_engine2_load.attr,
&dev_attr_engine2_leds.attr,
- NULL
-};
-
-static struct attribute *lp5523_engine3_attributes[] = {
&dev_attr_engine3_load.attr,
&dev_attr_engine3_leds.attr,
- NULL
};

static const struct attribute_group lp5523_group = {
.attrs = lp5523_attributes,
};

-static const struct attribute_group lp5523_engine_group[] = {
- {.attrs = lp5523_engine1_attributes },
- {.attrs = lp5523_engine2_attributes },
- {.attrs = lp5523_engine3_attributes },
-};
-
static int lp5523_register_sysfs(struct i2c_client *client)
{
struct device *dev = &client->dev;
@@ -788,10 +781,6 @@ static void lp5523_unregister_sysfs(struct i2c_client *client)

sysfs_remove_group(&dev->kobj, &lp5523_group);

- for (i = 0; i < ARRAY_SIZE(chip->engines); i++)
- if (chip->engines[i].mode == LP5523_CMD_LOAD)
- sysfs_remove_group(&dev->kobj, &lp5523_engine_group[i]);
-
for (i = 0; i < chip->num_leds; i++)
sysfs_remove_group(&chip->leds[i].cdev.dev->kobj,
&lp5523_led_attribute_group);
@@ -802,10 +791,6 @@ static void lp5523_unregister_sysfs(struct i2c_client *client)
/*--------------------------------------------------------------*/
static int lp5523_set_mode(struct lp5523_engine *engine, u8 mode)
{
- /* engine to chip */
- struct lp5523_chip *chip = engine_to_lp5523(engine);
- struct i2c_client *client = chip->client;
- struct device *dev = &client->dev;
int ret = 0;

/* if in that mode already do nothing, except for run */
@@ -817,18 +802,10 @@ static int lp5523_set_mode(struct lp5523_engine *engine, u8 mode)
} else if (mode == LP5523_CMD_LOAD) {
lp5523_set_engine_mode(engine, LP5523_CMD_DISABLED);
lp5523_set_engine_mode(engine, LP5523_CMD_LOAD);
-
- ret = sysfs_create_group(&dev->kobj, engine->attributes);
- if (ret)
- return ret;
} else if (mode == LP5523_CMD_DISABLED) {
lp5523_set_engine_mode(engine, LP5523_CMD_DISABLED);
}

- /* remove load attribute from sysfs if not in load mode */
- if (engine->mode == LP5523_CMD_LOAD && mode != LP5523_CMD_LOAD)
- sysfs_remove_group(&dev->kobj, engine->attributes);
-
engine->mode = mode;

return ret;
@@ -845,7 +822,6 @@ static int __init lp5523_init_engine(struct lp5523_engine *engine, int id)
engine->engine_mask = LP5523_ENG_MASK_BASE >> SHIFT_MASK(id);
engine->prog_page = id - 1;
engine->mux_page = id + 2;
- engine->attributes = &lp5523_engine_group[id - 1];

return 0;
}
--
1.7.0.4

2010-12-17 10:20:48

by Samu Onkalo

[permalink] [raw]
Subject: [PATCH 1/3] leds: leds-lp5523: modify the way of setting led device name

Currently all leds channels begins with string lp5523.
Patch adds a possibility to provide name via platform
data. This makes it possible to have several
chips without overlapping sysfs names.

Signed-off-by: Samu Onkalo <[email protected]>
---
drivers/leds/leds-lp5523.c | 3 ++-
include/linux/leds-lp5523.h | 1 +
2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index 0cc4ead..6cb4160 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -870,7 +870,8 @@ static int __init lp5523_init_led(struct lp5523_led *led, struct device *dev,
return -EINVAL;
}

- snprintf(name, 32, "lp5523:channel%d", chan);
+ snprintf(name, sizeof(name), "%s:channel%d",
+ pdata->label ?: "lp5523", chan);

led->cdev.name = name;
led->cdev.brightness_set = lp5523_set_brightness;
diff --git a/include/linux/leds-lp5523.h b/include/linux/leds-lp5523.h
index 7967476..2694289 100644
--- a/include/linux/leds-lp5523.h
+++ b/include/linux/leds-lp5523.h
@@ -42,6 +42,7 @@ struct lp5523_platform_data {
int (*setup_resources)(void);
void (*release_resources)(void);
void (*enable)(bool state);
+ const char *label;
};

#endif /* __LINUX_LP5523_H */
--
1.7.0.4

2010-12-17 10:20:49

by Samu Onkalo

[permalink] [raw]
Subject: [PATCH 3/3] leds: lp5521: Fix circular locking

Driver contained possibility for circular locking.
One lock is held by sysfs-core and another one by the driver itself.
This happened when the driver created or removed sysfs entries dynamically.
There is no real need to do those operations. Now all the sysfs entries
are created at probe and removed at removal. Engine load sysfs entries are
now visible all the time. However, access to the entries fails if
the engine is disabled or running.

Signed-off-by: Samu Onkalo <[email protected]>
---
drivers/leds/leds-lp5521.c | 52 ++++++-------------------------------------
1 files changed, 8 insertions(+), 44 deletions(-)

diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
index 745ad99..eda99b9 100644
--- a/drivers/leds/leds-lp5521.c
+++ b/drivers/leds/leds-lp5521.c
@@ -98,7 +98,6 @@
#define LP5521_EXT_CLK_USED 0x08

struct lp5521_engine {
- const struct attribute_group *attributes;
int id;
u8 mode;
u8 prog_page;
@@ -225,25 +224,22 @@ static int lp5521_set_led_current(struct lp5521_chip *chip, int led, u8 curr)
curr);
}

-static void lp5521_init_engine(struct lp5521_chip *chip,
- const struct attribute_group *attr_group)
+static void lp5521_init_engine(struct lp5521_chip *chip)
{
int i;
for (i = 0; i < ARRAY_SIZE(chip->engines); i++) {
chip->engines[i].id = i + 1;
chip->engines[i].engine_mask = LP5521_ENG_MASK_BASE >> (i * 2);
chip->engines[i].prog_page = i;
- chip->engines[i].attributes = &attr_group[i];
}
}

-static int lp5521_configure(struct i2c_client *client,
- const struct attribute_group *attr_group)
+static int lp5521_configure(struct i2c_client *client)
{
struct lp5521_chip *chip = i2c_get_clientdata(client);
int ret;

- lp5521_init_engine(chip, attr_group);
+ lp5521_init_engine(chip);

/* Set all PWMs to direct control mode */
ret = lp5521_write(client, LP5521_REG_OP_MODE, 0x3F);
@@ -329,9 +325,6 @@ static int lp5521_detect(struct i2c_client *client)
/* Set engine mode and create appropriate sysfs attributes, if required. */
static int lp5521_set_mode(struct lp5521_engine *engine, u8 mode)
{
- struct lp5521_chip *chip = engine_to_lp5521(engine);
- struct i2c_client *client = chip->client;
- struct device *dev = &client->dev;
int ret = 0;

/* if in that mode already do nothing, except for run */
@@ -343,18 +336,10 @@ static int lp5521_set_mode(struct lp5521_engine *engine, u8 mode)
} else if (mode == LP5521_CMD_LOAD) {
lp5521_set_engine_mode(engine, LP5521_CMD_DISABLED);
lp5521_set_engine_mode(engine, LP5521_CMD_LOAD);
-
- ret = sysfs_create_group(&dev->kobj, engine->attributes);
- if (ret)
- return ret;
} else if (mode == LP5521_CMD_DISABLED) {
lp5521_set_engine_mode(engine, LP5521_CMD_DISABLED);
}

- /* remove load attribute from sysfs if not in load mode */
- if (engine->mode == LP5521_CMD_LOAD && mode != LP5521_CMD_LOAD)
- sysfs_remove_group(&dev->kobj, engine->attributes);
-
engine->mode = mode;

return ret;
@@ -387,7 +372,10 @@ static int lp5521_do_store_load(struct lp5521_engine *engine,
goto fail;

mutex_lock(&chip->lock);
- ret = lp5521_load_program(engine, pattern);
+ if (engine->mode == LP5521_CMD_LOAD)
+ ret = lp5521_load_program(engine, pattern);
+ else
+ ret = -EINVAL;
mutex_unlock(&chip->lock);

if (ret) {
@@ -574,20 +562,8 @@ static struct attribute *lp5521_attributes[] = {
&dev_attr_engine2_mode.attr,
&dev_attr_engine3_mode.attr,
&dev_attr_selftest.attr,
- NULL
-};
-
-static struct attribute *lp5521_engine1_attributes[] = {
&dev_attr_engine1_load.attr,
- NULL
-};
-
-static struct attribute *lp5521_engine2_attributes[] = {
&dev_attr_engine2_load.attr,
- NULL
-};
-
-static struct attribute *lp5521_engine3_attributes[] = {
&dev_attr_engine3_load.attr,
NULL
};
@@ -596,12 +572,6 @@ static const struct attribute_group lp5521_group = {
.attrs = lp5521_attributes,
};

-static const struct attribute_group lp5521_engine_group[] = {
- {.attrs = lp5521_engine1_attributes },
- {.attrs = lp5521_engine2_attributes },
- {.attrs = lp5521_engine3_attributes },
-};
-
static int lp5521_register_sysfs(struct i2c_client *client)
{
struct device *dev = &client->dev;
@@ -616,12 +586,6 @@ static void lp5521_unregister_sysfs(struct i2c_client *client)

sysfs_remove_group(&dev->kobj, &lp5521_group);

- for (i = 0; i < ARRAY_SIZE(chip->engines); i++) {
- if (chip->engines[i].mode == LP5521_CMD_LOAD)
- sysfs_remove_group(&dev->kobj,
- chip->engines[i].attributes);
- }
-
for (i = 0; i < chip->num_leds; i++)
sysfs_remove_group(&chip->leds[i].cdev.dev->kobj,
&lp5521_led_attribute_group);
@@ -724,7 +688,7 @@ static int lp5521_probe(struct i2c_client *client,

dev_info(&client->dev, "%s programmable led chip found\n", id->name);

- ret = lp5521_configure(client, lp5521_engine_group);
+ ret = lp5521_configure(client);
if (ret < 0) {
dev_err(&client->dev, "error configuring chip\n");
goto fail2;
--
1.7.0.4

2010-12-17 12:46:27

by Ilkka Koskinen

[permalink] [raw]
Subject: RE: [PATCH 2/3] leds: lp5523: Fix circular locking

Hi,

>From: Onkalo Samu.P (Nokia-MS/Tampere)
>Sent: 17 December, 2010 12:19
>Subject: [PATCH 2/3] leds: lp5523: Fix circular locking
>
>Driver contained possibility for circular locking.
>One lock is held by sysfs-core and another one by the driver itself.
>This happened when the driver created or removed sysfs entries
>dynamically.
>There is no real need to do those operations. Now all the sysfs entries
>are created at probe and removed at removal. Engine load and mux
>configuration
>sysfs entries are now visible all the time. However, access to the
>entries
>fails if the engine is disabled or running.
>
>Signed-off-by: Samu Onkalo <[email protected]>
>---
> drivers/leds/leds-lp5523.c | 56 ++++++++++++-------------------------
>------
> 1 files changed, 16 insertions(+), 40 deletions(-)
>
>diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
>index 6cb4160..d0c4068 100644
>--- a/drivers/leds/leds-lp5523.c
>+++ b/drivers/leds/leds-lp5523.c
>@@ -105,7 +105,6 @@
> #define SHIFT_MASK(id) (((id) - 1) * 2)
>
> struct lp5523_engine {
>- const struct attribute_group *attributes;
> int id;
> u8 mode;
> u8 prog_page;
>@@ -403,14 +402,23 @@ static ssize_t store_engine_leds(struct device
>*dev,
> struct i2c_client *client = to_i2c_client(dev);
> struct lp5523_chip *chip = i2c_get_clientdata(client);
> u16 mux = 0;
>+ ssize_t ret;
>
> if (lp5523_mux_parse(buf, &mux, len))
> return -EINVAL;
>
>+ mutex_lock(&chip->lock);
>+ ret = -EINVAL;

You could move this assignment to declaration. But otherwise
looks good to me. So,

Reviewed-by: Ilkka Koskinen <[email protected]>


Cheers,


>+ if (chip->engines[nr - 1].mode != LP5523_CMD_LOAD)
>+ goto leave;
>+
> if (lp5523_load_mux(&chip->engines[nr - 1], mux))
>- return -EINVAL;
>+ goto leave;
>
>- return len;
>+ ret = len;
>+leave:
>+ mutex_unlock(&chip->lock);
>+ return ret;
> }
>
> #define store_leds(nr) \
>@@ -556,7 +564,11 @@ static int lp5523_do_store_load(struct
>lp5523_engine *engine,
>
> mutex_lock(&chip->lock);
>
>- ret = lp5523_load_program(engine, pattern);
>+ if (engine->mode == LP5523_CMD_LOAD)
>+ ret = lp5523_load_program(engine, pattern);
>+ else
>+ ret = -EINVAL;
>+
> mutex_unlock(&chip->lock);
>
> if (ret) {
>@@ -737,37 +749,18 @@ static struct attribute *lp5523_attributes[] = {
> &dev_attr_engine2_mode.attr,
> &dev_attr_engine3_mode.attr,
> &dev_attr_selftest.attr,
>- NULL
>-};
>-
>-static struct attribute *lp5523_engine1_attributes[] = {
> &dev_attr_engine1_load.attr,
> &dev_attr_engine1_leds.attr,
>- NULL
>-};
>-
>-static struct attribute *lp5523_engine2_attributes[] = {
> &dev_attr_engine2_load.attr,
> &dev_attr_engine2_leds.attr,
>- NULL
>-};
>-
>-static struct attribute *lp5523_engine3_attributes[] = {
> &dev_attr_engine3_load.attr,
> &dev_attr_engine3_leds.attr,
>- NULL
> };
>
> static const struct attribute_group lp5523_group = {
> .attrs = lp5523_attributes,
> };
>
>-static const struct attribute_group lp5523_engine_group[] = {
>- {.attrs = lp5523_engine1_attributes },
>- {.attrs = lp5523_engine2_attributes },
>- {.attrs = lp5523_engine3_attributes },
>-};
>-
> static int lp5523_register_sysfs(struct i2c_client *client)
> {
> struct device *dev = &client->dev;
>@@ -788,10 +781,6 @@ static void lp5523_unregister_sysfs(struct
>i2c_client *client)
>
> sysfs_remove_group(&dev->kobj, &lp5523_group);
>
>- for (i = 0; i < ARRAY_SIZE(chip->engines); i++)
>- if (chip->engines[i].mode == LP5523_CMD_LOAD)
>- sysfs_remove_group(&dev->kobj,
>&lp5523_engine_group[i]);
>-
> for (i = 0; i < chip->num_leds; i++)
> sysfs_remove_group(&chip->leds[i].cdev.dev->kobj,
> &lp5523_led_attribute_group);
>@@ -802,10 +791,6 @@ static void lp5523_unregister_sysfs(struct
>i2c_client *client)
> /*--------------------------------------------------------------*/
> static int lp5523_set_mode(struct lp5523_engine *engine, u8 mode)
> {
>- /* engine to chip */
>- struct lp5523_chip *chip = engine_to_lp5523(engine);
>- struct i2c_client *client = chip->client;
>- struct device *dev = &client->dev;
> int ret = 0;
>
> /* if in that mode already do nothing, except for run */
>@@ -817,18 +802,10 @@ static int lp5523_set_mode(struct lp5523_engine
>*engine, u8 mode)
> } else if (mode == LP5523_CMD_LOAD) {
> lp5523_set_engine_mode(engine, LP5523_CMD_DISABLED);
> lp5523_set_engine_mode(engine, LP5523_CMD_LOAD);
>-
>- ret = sysfs_create_group(&dev->kobj, engine->attributes);
>- if (ret)
>- return ret;
> } else if (mode == LP5523_CMD_DISABLED) {
> lp5523_set_engine_mode(engine, LP5523_CMD_DISABLED);
> }
>
>- /* remove load attribute from sysfs if not in load mode */
>- if (engine->mode == LP5523_CMD_LOAD && mode != LP5523_CMD_LOAD)
>- sysfs_remove_group(&dev->kobj, engine->attributes);
>-
> engine->mode = mode;
>
> return ret;
>@@ -845,7 +822,6 @@ static int __init lp5523_init_engine(struct
>lp5523_engine *engine, int id)
> engine->engine_mask = LP5523_ENG_MASK_BASE >> SHIFT_MASK(id);
> engine->prog_page = id - 1;
> engine->mux_page = id + 2;
>- engine->attributes = &lp5523_engine_group[id - 1];
>
> return 0;
> }
>--
>1.7.0.4

2010-12-17 12:46:29

by Ilkka Koskinen

[permalink] [raw]
Subject: RE: [PATCH 3/3] leds: lp5521: Fix circular locking

Hi,

>From: Samu Onkalo [mailto:[email protected]]
>Sent: 17 December, 2010 12:19
>Subject: [PATCH 3/3] leds: lp5521: Fix circular locking
>
>Driver contained possibility for circular locking.
>One lock is held by sysfs-core and another one by the driver itself.
>This happened when the driver created or removed sysfs entries
>dynamically.
>There is no real need to do those operations. Now all the sysfs entries
>are created at probe and removed at removal. Engine load sysfs entries
>are
>now visible all the time. However, access to the entries fails if
>the engine is disabled or running.
>
>Signed-off-by: Samu Onkalo <[email protected]>

Reviewed-by: Ilkka Koskinen <[email protected]>


>---
> drivers/leds/leds-lp5521.c | 52 ++++++-------------------------------
>------
> 1 files changed, 8 insertions(+), 44 deletions(-)
>
>diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
>index 745ad99..eda99b9 100644
>--- a/drivers/leds/leds-lp5521.c
>+++ b/drivers/leds/leds-lp5521.c
>@@ -98,7 +98,6 @@
> #define LP5521_EXT_CLK_USED 0x08
>
> struct lp5521_engine {
>- const struct attribute_group *attributes;
> int id;
> u8 mode;
> u8 prog_page;
>@@ -225,25 +224,22 @@ static int lp5521_set_led_current(struct
>lp5521_chip *chip, int led, u8 curr)
> curr);
> }
>
>-static void lp5521_init_engine(struct lp5521_chip *chip,
>- const struct attribute_group *attr_group)
>+static void lp5521_init_engine(struct lp5521_chip *chip)
> {
> int i;
> for (i = 0; i < ARRAY_SIZE(chip->engines); i++) {
> chip->engines[i].id = i + 1;
> chip->engines[i].engine_mask = LP5521_ENG_MASK_BASE >> (i *
>2);
> chip->engines[i].prog_page = i;
>- chip->engines[i].attributes = &attr_group[i];
> }
> }
>
>-static int lp5521_configure(struct i2c_client *client,
>- const struct attribute_group *attr_group)
>+static int lp5521_configure(struct i2c_client *client)
> {
> struct lp5521_chip *chip = i2c_get_clientdata(client);
> int ret;
>
>- lp5521_init_engine(chip, attr_group);
>+ lp5521_init_engine(chip);
>
> /* Set all PWMs to direct control mode */
> ret = lp5521_write(client, LP5521_REG_OP_MODE, 0x3F);
>@@ -329,9 +325,6 @@ static int lp5521_detect(struct i2c_client *client)
> /* Set engine mode and create appropriate sysfs attributes, if
>required. */
> static int lp5521_set_mode(struct lp5521_engine *engine, u8 mode)
> {
>- struct lp5521_chip *chip = engine_to_lp5521(engine);
>- struct i2c_client *client = chip->client;
>- struct device *dev = &client->dev;
> int ret = 0;
>
> /* if in that mode already do nothing, except for run */
>@@ -343,18 +336,10 @@ static int lp5521_set_mode(struct lp5521_engine
>*engine, u8 mode)
> } else if (mode == LP5521_CMD_LOAD) {
> lp5521_set_engine_mode(engine, LP5521_CMD_DISABLED);
> lp5521_set_engine_mode(engine, LP5521_CMD_LOAD);
>-
>- ret = sysfs_create_group(&dev->kobj, engine->attributes);
>- if (ret)
>- return ret;
> } else if (mode == LP5521_CMD_DISABLED) {
> lp5521_set_engine_mode(engine, LP5521_CMD_DISABLED);
> }
>
>- /* remove load attribute from sysfs if not in load mode */
>- if (engine->mode == LP5521_CMD_LOAD && mode != LP5521_CMD_LOAD)
>- sysfs_remove_group(&dev->kobj, engine->attributes);
>-
> engine->mode = mode;
>
> return ret;
>@@ -387,7 +372,10 @@ static int lp5521_do_store_load(struct
>lp5521_engine *engine,
> goto fail;
>
> mutex_lock(&chip->lock);
>- ret = lp5521_load_program(engine, pattern);
>+ if (engine->mode == LP5521_CMD_LOAD)
>+ ret = lp5521_load_program(engine, pattern);
>+ else
>+ ret = -EINVAL;
> mutex_unlock(&chip->lock);
>
> if (ret) {
>@@ -574,20 +562,8 @@ static struct attribute *lp5521_attributes[] = {
> &dev_attr_engine2_mode.attr,
> &dev_attr_engine3_mode.attr,
> &dev_attr_selftest.attr,
>- NULL
>-};
>-
>-static struct attribute *lp5521_engine1_attributes[] = {
> &dev_attr_engine1_load.attr,
>- NULL
>-};
>-
>-static struct attribute *lp5521_engine2_attributes[] = {
> &dev_attr_engine2_load.attr,
>- NULL
>-};
>-
>-static struct attribute *lp5521_engine3_attributes[] = {
> &dev_attr_engine3_load.attr,
> NULL
> };
>@@ -596,12 +572,6 @@ static const struct attribute_group lp5521_group =
>{
> .attrs = lp5521_attributes,
> };
>
>-static const struct attribute_group lp5521_engine_group[] = {
>- {.attrs = lp5521_engine1_attributes },
>- {.attrs = lp5521_engine2_attributes },
>- {.attrs = lp5521_engine3_attributes },
>-};
>-
> static int lp5521_register_sysfs(struct i2c_client *client)
> {
> struct device *dev = &client->dev;
>@@ -616,12 +586,6 @@ static void lp5521_unregister_sysfs(struct
>i2c_client *client)
>
> sysfs_remove_group(&dev->kobj, &lp5521_group);
>
>- for (i = 0; i < ARRAY_SIZE(chip->engines); i++) {
>- if (chip->engines[i].mode == LP5521_CMD_LOAD)
>- sysfs_remove_group(&dev->kobj,
>- chip->engines[i].attributes);
>- }
>-
> for (i = 0; i < chip->num_leds; i++)
> sysfs_remove_group(&chip->leds[i].cdev.dev->kobj,
> &lp5521_led_attribute_group);
>@@ -724,7 +688,7 @@ static int lp5521_probe(struct i2c_client *client,
>
> dev_info(&client->dev, "%s programmable led chip found\n", id-
>>name);
>
>- ret = lp5521_configure(client, lp5521_engine_group);
>+ ret = lp5521_configure(client);
> if (ret < 0) {
> dev_err(&client->dev, "error configuring chip\n");
> goto fail2;
>--
>1.7.0.4