2015-05-11 19:11:01

by Toshi Kikuchi

[permalink] [raw]
Subject: [PATCH] leds: lp5523: add master_fader support

This patch introduces 4 new attributes:
master_fader_leds
master_fader1
master_fader2
master_fader3

Fo example, to map channel 0,6 to master_fader1,
map channel 1,7 to master_fader2,
map channel 2,8 to master_fader3, and
map channel 3,4,5 to none

echo "123000123" > master_fader_leds

A different factor can be set to each master_fader:

echo 255 > master_fader1
echo 100 > master_fader2
echo 0 > master_fader3

Signed-off-by: Toshi Kikuchi <[email protected]>
Acked-by: Milo Kim <[email protected]>
---
drivers/leds/leds-lp5523.c | 148 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 148 insertions(+)

diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index 4ade66a..a266f90 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -51,6 +51,7 @@
#define LP5523_REG_OP_MODE 0x01
#define LP5523_REG_ENABLE_LEDS_MSB 0x04
#define LP5523_REG_ENABLE_LEDS_LSB 0x05
+#define LP5523_REG_LED_CTRL_BASE 0x06
#define LP5523_REG_LED_PWM_BASE 0x16
#define LP5523_REG_LED_CURRENT_BASE 0x26
#define LP5523_REG_CONFIG 0x36
@@ -58,6 +59,7 @@
#define LP5523_REG_RESET 0x3D
#define LP5523_REG_LED_TEST_CTRL 0x41
#define LP5523_REG_LED_TEST_ADC 0x42
+#define LP5523_REG_MASTER_FADER_BASE 0x48
#define LP5523_REG_CH1_PROG_START 0x4C
#define LP5523_REG_CH2_PROG_START 0x4D
#define LP5523_REG_CH3_PROG_START 0x4E
@@ -79,6 +81,9 @@
#define LP5523_EXT_CLK_USED 0x08
#define LP5523_ENG_STATUS_MASK 0x07

+#define LP5523_FADER_MAPPING_MASK 0xC0
+#define LP5523_FADER_MAPPING_SHIFT 6
+
/* Memory Page Selection */
#define LP5523_PAGE_ENG1 0
#define LP5523_PAGE_ENG2 1
@@ -667,6 +672,137 @@ release_lock:
return pos;
}

+#define show_fader(nr) \
+static ssize_t show_master_fader##nr(struct device *dev, \
+ struct device_attribute *attr, \
+ char *buf) \
+{ \
+ return show_master_fader(dev, attr, buf, nr); \
+}
+
+#define store_fader(nr) \
+static ssize_t store_master_fader##nr(struct device *dev, \
+ struct device_attribute *attr, \
+ const char *buf, size_t len) \
+{ \
+ return store_master_fader(dev, attr, buf, len, nr); \
+}
+
+static ssize_t show_master_fader(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;
+ int ret;
+ u8 val;
+
+ mutex_lock(&chip->lock);
+ ret = lp55xx_read(chip, LP5523_REG_MASTER_FADER_BASE + nr - 1, &val);
+ mutex_unlock(&chip->lock);
+
+ if (ret == 0)
+ ret = sprintf(buf, "%u\n", val);
+
+ return ret;
+}
+show_fader(1)
+show_fader(2)
+show_fader(3)
+
+static ssize_t store_master_fader(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;
+ int ret;
+ unsigned long val;
+
+ if (kstrtoul(buf, 0, &val))
+ return -EINVAL;
+
+ if (val > 0xff)
+ return -EINVAL;
+
+ mutex_lock(&chip->lock);
+ ret = lp55xx_write(chip, LP5523_REG_MASTER_FADER_BASE + nr - 1,
+ (u8)val);
+ mutex_unlock(&chip->lock);
+
+ if (ret == 0)
+ ret = len;
+
+ return ret;
+}
+store_fader(1)
+store_fader(2)
+store_fader(3)
+
+static ssize_t show_master_fader_leds(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
+ struct lp55xx_chip *chip = led->chip;
+ int i, ret, pos = 0;
+ u8 val;
+
+ mutex_lock(&chip->lock);
+
+ for (i = 0; i < LP5523_MAX_LEDS; i++) {
+ ret = lp55xx_read(chip, LP5523_REG_LED_CTRL_BASE + i, &val);
+ if (ret)
+ goto leave;
+
+ val = (val & LP5523_FADER_MAPPING_MASK)
+ >> LP5523_FADER_MAPPING_SHIFT;
+ if (val > 3) {
+ ret = -EINVAL;
+ goto leave;
+ }
+ buf[pos++] = val + '0';
+ }
+ buf[pos++] = '\n';
+ ret = pos;
+leave:
+ mutex_unlock(&chip->lock);
+ return ret;
+}
+
+static ssize_t store_master_fader_leds(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
+ struct lp55xx_chip *chip = led->chip;
+ int i, n, ret;
+ u8 val;
+
+ n = min_t(int, len, LP5523_MAX_LEDS);
+
+ mutex_lock(&chip->lock);
+
+ for (i = 0; i < n; i++) {
+ if (buf[i] >= '0' && buf[i] <= '3') {
+ val = (buf[i] - '0') << LP5523_FADER_MAPPING_SHIFT;
+ ret = lp55xx_update_bits(chip,
+ LP5523_REG_LED_CTRL_BASE + i,
+ LP5523_FADER_MAPPING_MASK,
+ val);
+ if (ret)
+ goto leave;
+ } else {
+ ret = -EINVAL;
+ goto leave;
+ }
+ }
+ ret = len;
+leave:
+ mutex_unlock(&chip->lock);
+ return ret;
+}
+
static void lp5523_led_brightness_work(struct work_struct *work)
{
struct lp55xx_led *led = container_of(work, struct lp55xx_led,
@@ -689,6 +825,14 @@ 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 LP55XX_DEV_ATTR_RW(master_fader1, show_master_fader1,
+ store_master_fader1);
+static LP55XX_DEV_ATTR_RW(master_fader2, show_master_fader2,
+ store_master_fader2);
+static LP55XX_DEV_ATTR_RW(master_fader3, show_master_fader3,
+ store_master_fader3);
+static LP55XX_DEV_ATTR_RW(master_fader_leds, show_master_fader_leds,
+ store_master_fader_leds);

static struct attribute *lp5523_attributes[] = {
&dev_attr_engine1_mode.attr,
@@ -701,6 +845,10 @@ static struct attribute *lp5523_attributes[] = {
&dev_attr_engine2_leds.attr,
&dev_attr_engine3_leds.attr,
&dev_attr_selftest.attr,
+ &dev_attr_master_fader1.attr,
+ &dev_attr_master_fader2.attr,
+ &dev_attr_master_fader3.attr,
+ &dev_attr_master_fader_leds.attr,
NULL,
};

--
2.1.0


2015-05-11 22:53:11

by Kim, Milo

[permalink] [raw]
Subject: Re: [PATCH] leds: lp5523: add master_fader support

Hi Bryan,

On 5/12/2015 4:10 AM, Toshi Kikuchi wrote:
> This patch introduces 4 new attributes:
> master_fader_leds
> master_fader1
> master_fader2
> master_fader3
>
> Fo example, to map channel 0,6 to master_fader1,
> map channel 1,7 to master_fader2,
> map channel 2,8 to master_fader3, and
> map channel 3,4,5 to none
>
> echo "123000123" > master_fader_leds
>
> A different factor can be set to each master_fader:
>
> echo 255 > master_fader1
> echo 100 > master_fader2
> echo 0 > master_fader3
>
> Signed-off-by: Toshi Kikuchi <[email protected]>
> Acked-by: Milo Kim <[email protected]>

Tested-by: Milo Kim <[email protected]>

Best regards,
Milo

2015-05-12 07:25:52

by Jacek Anaszewski

[permalink] [raw]
Subject: Re: [PATCH] leds: lp5523: add master_fader support

Hi Toshi,

On 05/11/2015 09:10 PM, Toshi Kikuchi wrote:
> This patch introduces 4 new attributes:
> master_fader_leds
> master_fader1
> master_fader2
> master_fader3
>
> Fo example, to map channel 0,6 to master_fader1,
> map channel 1,7 to master_fader2,
> map channel 2,8 to master_fader3, and
> map channel 3,4,5 to none
>
> echo "123000123" > master_fader_leds

I propose to add ABI documentation for this driver. It already exposes
many custom attributes but I can't find documentation for them.

For this patch:

Acked-by: Jacek Anaszewski <[email protected]>

> A different factor can be set to each master_fader:
>
> echo 255 > master_fader1
> echo 100 > master_fader2
> echo 0 > master_fader3
>
> Signed-off-by: Toshi Kikuchi <[email protected]>
> Acked-by: Milo Kim <[email protected]>
> ---
> drivers/leds/leds-lp5523.c | 148 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 148 insertions(+)
>
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index 4ade66a..a266f90 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -51,6 +51,7 @@
> #define LP5523_REG_OP_MODE 0x01
> #define LP5523_REG_ENABLE_LEDS_MSB 0x04
> #define LP5523_REG_ENABLE_LEDS_LSB 0x05
> +#define LP5523_REG_LED_CTRL_BASE 0x06
> #define LP5523_REG_LED_PWM_BASE 0x16
> #define LP5523_REG_LED_CURRENT_BASE 0x26
> #define LP5523_REG_CONFIG 0x36
> @@ -58,6 +59,7 @@
> #define LP5523_REG_RESET 0x3D
> #define LP5523_REG_LED_TEST_CTRL 0x41
> #define LP5523_REG_LED_TEST_ADC 0x42
> +#define LP5523_REG_MASTER_FADER_BASE 0x48
> #define LP5523_REG_CH1_PROG_START 0x4C
> #define LP5523_REG_CH2_PROG_START 0x4D
> #define LP5523_REG_CH3_PROG_START 0x4E
> @@ -79,6 +81,9 @@
> #define LP5523_EXT_CLK_USED 0x08
> #define LP5523_ENG_STATUS_MASK 0x07
>
> +#define LP5523_FADER_MAPPING_MASK 0xC0
> +#define LP5523_FADER_MAPPING_SHIFT 6
> +
> /* Memory Page Selection */
> #define LP5523_PAGE_ENG1 0
> #define LP5523_PAGE_ENG2 1
> @@ -667,6 +672,137 @@ release_lock:
> return pos;
> }
>
> +#define show_fader(nr) \
> +static ssize_t show_master_fader##nr(struct device *dev, \
> + struct device_attribute *attr, \
> + char *buf) \
> +{ \
> + return show_master_fader(dev, attr, buf, nr); \
> +}
> +
> +#define store_fader(nr) \
> +static ssize_t store_master_fader##nr(struct device *dev, \
> + struct device_attribute *attr, \
> + const char *buf, size_t len) \
> +{ \
> + return store_master_fader(dev, attr, buf, len, nr); \
> +}
> +
> +static ssize_t show_master_fader(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;
> + int ret;
> + u8 val;
> +
> + mutex_lock(&chip->lock);
> + ret = lp55xx_read(chip, LP5523_REG_MASTER_FADER_BASE + nr - 1, &val);
> + mutex_unlock(&chip->lock);
> +
> + if (ret == 0)
> + ret = sprintf(buf, "%u\n", val);
> +
> + return ret;
> +}
> +show_fader(1)
> +show_fader(2)
> +show_fader(3)
> +
> +static ssize_t store_master_fader(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;
> + int ret;
> + unsigned long val;
> +
> + if (kstrtoul(buf, 0, &val))
> + return -EINVAL;
> +
> + if (val > 0xff)
> + return -EINVAL;
> +
> + mutex_lock(&chip->lock);
> + ret = lp55xx_write(chip, LP5523_REG_MASTER_FADER_BASE + nr - 1,
> + (u8)val);
> + mutex_unlock(&chip->lock);
> +
> + if (ret == 0)
> + ret = len;
> +
> + return ret;
> +}
> +store_fader(1)
> +store_fader(2)
> +store_fader(3)
> +
> +static ssize_t show_master_fader_leds(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
> + struct lp55xx_chip *chip = led->chip;
> + int i, ret, pos = 0;
> + u8 val;
> +
> + mutex_lock(&chip->lock);
> +
> + for (i = 0; i < LP5523_MAX_LEDS; i++) {
> + ret = lp55xx_read(chip, LP5523_REG_LED_CTRL_BASE + i, &val);
> + if (ret)
> + goto leave;
> +
> + val = (val & LP5523_FADER_MAPPING_MASK)
> + >> LP5523_FADER_MAPPING_SHIFT;
> + if (val > 3) {
> + ret = -EINVAL;
> + goto leave;
> + }
> + buf[pos++] = val + '0';
> + }
> + buf[pos++] = '\n';
> + ret = pos;
> +leave:
> + mutex_unlock(&chip->lock);
> + return ret;
> +}
> +
> +static ssize_t store_master_fader_leds(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
> + struct lp55xx_chip *chip = led->chip;
> + int i, n, ret;
> + u8 val;
> +
> + n = min_t(int, len, LP5523_MAX_LEDS);
> +
> + mutex_lock(&chip->lock);
> +
> + for (i = 0; i < n; i++) {
> + if (buf[i] >= '0' && buf[i] <= '3') {
> + val = (buf[i] - '0') << LP5523_FADER_MAPPING_SHIFT;
> + ret = lp55xx_update_bits(chip,
> + LP5523_REG_LED_CTRL_BASE + i,
> + LP5523_FADER_MAPPING_MASK,
> + val);
> + if (ret)
> + goto leave;
> + } else {
> + ret = -EINVAL;
> + goto leave;
> + }
> + }
> + ret = len;
> +leave:
> + mutex_unlock(&chip->lock);
> + return ret;
> +}
> +
> static void lp5523_led_brightness_work(struct work_struct *work)
> {
> struct lp55xx_led *led = container_of(work, struct lp55xx_led,
> @@ -689,6 +825,14 @@ 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 LP55XX_DEV_ATTR_RW(master_fader1, show_master_fader1,
> + store_master_fader1);
> +static LP55XX_DEV_ATTR_RW(master_fader2, show_master_fader2,
> + store_master_fader2);
> +static LP55XX_DEV_ATTR_RW(master_fader3, show_master_fader3,
> + store_master_fader3);
> +static LP55XX_DEV_ATTR_RW(master_fader_leds, show_master_fader_leds,
> + store_master_fader_leds);
>
> static struct attribute *lp5523_attributes[] = {
> &dev_attr_engine1_mode.attr,
> @@ -701,6 +845,10 @@ static struct attribute *lp5523_attributes[] = {
> &dev_attr_engine2_leds.attr,
> &dev_attr_engine3_leds.attr,
> &dev_attr_selftest.attr,
> + &dev_attr_master_fader1.attr,
> + &dev_attr_master_fader2.attr,
> + &dev_attr_master_fader3.attr,
> + &dev_attr_master_fader_leds.attr,
> NULL,
> };
>
>


--
Best Regards,
Jacek Anaszewski

2015-05-12 08:25:43

by Kim, Milo

[permalink] [raw]
Subject: Re: [PATCH] leds: lp5523: add master_fader support

Hi Jacek and Toshi,

On 5/12/2015 4:25 PM, Jacek Anaszewski wrote:
> Hi Toshi,
>
> On 05/11/2015 09:10 PM, Toshi Kikuchi wrote:
>> This patch introduces 4 new attributes:
>> master_fader_leds
>> master_fader1
>> master_fader2
>> master_fader3
>>
>> Fo example, to map channel 0,6 to master_fader1,
>> map channel 1,7 to master_fader2,
>> map channel 2,8 to master_fader3, and
>> map channel 3,4,5 to none
> >
>> echo "123000123" > master_fader_leds
>
> I propose to add ABI documentation for this driver. It already exposes
> many custom attributes but I can't find documentation for them.

I agree with Jacek's comment but it would be better if we could see the
description in lp5523 driver
documentation(Documentation/leds/leds-lp5523.txt).

Best regards,
Milo

2015-05-12 08:51:21

by Jacek Anaszewski

[permalink] [raw]
Subject: Re: [PATCH] leds: lp5523: add master_fader support

On 05/12/2015 10:25 AM, Kim, Milo wrote:
> Hi Jacek and Toshi,
>
> On 5/12/2015 4:25 PM, Jacek Anaszewski wrote:
>> Hi Toshi,
>>
>> On 05/11/2015 09:10 PM, Toshi Kikuchi wrote:
>>> This patch introduces 4 new attributes:
>>> master_fader_leds
>>> master_fader1
>>> master_fader2
>>> master_fader3
>>>
>>> Fo example, to map channel 0,6 to master_fader1,
>>> map channel 1,7 to master_fader2,
>>> map channel 2,8 to master_fader3, and
>>> map channel 3,4,5 to none
>> >
>>> echo "123000123" > master_fader_leds
>>
>> I propose to add ABI documentation for this driver. It already exposes
>> many custom attributes but I can't find documentation for them.
>
> I agree with Jacek's comment but it would be better if we could see the
> description in lp5523 driver
> documentation(Documentation/leds/leds-lp5523.txt).

Right, I missed that file.

--
Best Regards,
Jacek Anaszewski

2015-05-12 17:59:52

by Bryan Wu

[permalink] [raw]
Subject: Re: [PATCH] leds: lp5523: add master_fader support

On Tue, May 12, 2015 at 1:51 AM, Jacek Anaszewski
<[email protected]> wrote:
> On 05/12/2015 10:25 AM, Kim, Milo wrote:
>>
>> Hi Jacek and Toshi,
>>
>> On 5/12/2015 4:25 PM, Jacek Anaszewski wrote:
>>>
>>> Hi Toshi,
>>>
>>> On 05/11/2015 09:10 PM, Toshi Kikuchi wrote:
>>>>
>>>> This patch introduces 4 new attributes:
>>>> master_fader_leds
>>>> master_fader1
>>>> master_fader2
>>>> master_fader3
>>>>
>>>> Fo example, to map channel 0,6 to master_fader1,
>>>> map channel 1,7 to master_fader2,
>>>> map channel 2,8 to master_fader3, and
>>>> map channel 3,4,5 to none
>>>
>>> >
>>>>
>>>> echo "123000123" > master_fader_leds
>>>
>>>
>>> I propose to add ABI documentation for this driver. It already exposes
>>> many custom attributes but I can't find documentation for them.
>>
>>
>> I agree with Jacek's comment but it would be better if we could see the
>> description in lp5523 driver
>> documentation(Documentation/leds/leds-lp5523.txt).
>
>
> Right, I missed that file.
>
>

Toshi,

I merged this patch into my tree with Milo and Jacek Acks. But please
follow their advice and provide a patch to update those document
files.

Thanks,
-Bryan

2015-06-22 11:59:03

by Pavel Machek

[permalink] [raw]
Subject: Re: [PATCH] leds: lp5523: add master_fader support

Hi!

> >>>> This patch introduces 4 new attributes:
> >>>> master_fader_leds
> >>>> master_fader1
> >>>> master_fader2
> >>>> master_fader3
> >>>>
> >>>> Fo example, to map channel 0,6 to master_fader1,
> >>>> map channel 1,7 to master_fader2,
> >>>> map channel 2,8 to master_fader3, and
> >>>> map channel 3,4,5 to none
> >>>
> >>> >
> >>>>
> >>>> echo "123000123" > master_fader_leds
> >>>
> >>>
> >>> I propose to add ABI documentation for this driver. It already exposes
> >>> many custom attributes but I can't find documentation for them.
> >>
> >>
> >> I agree with Jacek's comment but it would be better if we could see the
> >> description in lp5523 driver
> >> documentation(Documentation/leds/leds-lp5523.txt).
> >
> >
> > Right, I missed that file.
> >
> >
>
> Toshi,
>
> I merged this patch into my tree with Milo and Jacek Acks. But please
> follow their advice and provide a patch to update those document
> files.

Are you sure this is the right interface? lp5523 is turing-complete. Yes, it
can multiply brightness values by a constant. It can also do morse code on its
own, and compute prime numbers.

Why is this useful addition to kernel<->user interface?

Can we do better interface that works on more than one chip?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html