Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[H. Nikolaus Schaller]

Hi,

> Am 07.07.2016 um 10:46 schrieb Jacek Anaszewski :
> 
> Hi Nikolaus,
> 
> On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:
>> Hi,
>> finally, I found the time to update the driver according to the many comments
>> received a while ago.
>> 
>> Most of them have been worked in, including the regmap idea and
>> brightness_set_blocking().
>> 
>> The driver works on our system, so that I will mail [PATCH v2] as a followup.
>> 
>> There is only one aspect of the new solution I am not sure if it is
>> really better than our old proposal (see below).
>> 
>> 
>>> Am 20.04.2016 um 23:04 schrieb Jacek Anaszewski 
>>> :
>>> 
>>> On 04/19/2016 07:21 PM, H. Nikolaus Schaller wrote:
> I believe the LEDS core now handles the workqueues generically for
> blocking operations, so it's no longer needed in the individual drivers.
 
 We had a lot of trouble with locking and blocking especially if we
 want to indicate CPU or (root) disk activity.
>>> 
>>> What kind of troubles you had? Could you share more details?
>>> Does it mean that current LED core design doesn't fit for your
>>> use cases?
>> 
>> The system started to flicker the LEDs irregularily and sometimes
>> the whole kernel stalled.
>> 
>>> 
 So it is implemented in a way that changes can be requested faster
 than the I2C bus can write new values to the chip.
 
 Only after one sequence of I2C writes is done, another work function
 can be scheduled. And each group of writes updates as many LEDs
 in parallel if necessary.
>>> 
>>> You can serialize the operations in brightness_set_blocking with
>>> a mutex.
>> 
>> Yes, that works fine in our (incomplete) test setup.
>> 
>> But I think it assumes that the i2c bus is never congested by other i2c 
>> traffic.
>> 
>> I have not found code that obviously takes care of the situation if led
>> trigger events (e.g. mmc or cpu triggers) are coming in faster than the
>> i2c (even using regmap) can write out over i2c.
>> 
>> If I understand the led core code correctly, it will just do another 
>> schedule_work
>> for every single change of led brightness.
> 
> Please look at schedule_work documentation in include/linux/workqueue.h:
> 
> /**
> * schedule_work - put work task in global workqueue
> * @work: job to be done
> *
> * Returns %false if @work was already on the kernel-global workqueue and
> * %true otherwise.
> *
> * This puts a job in the kernel-global workqueue if it was not already

Ah, ok. I missed that. Thanks!

> * queued and leaves it in the same position on the kernel-global
> * workqueue otherwise.
> */
> static inline bool schedule_work(struct work_struct *work)
> {
>return queue_work(system_wq, work);
> }

> 
>> 
>> So I wonder if Is there some guarantee that this work queue will not fill
>> up memory and is really processed faster than being filled? I.e. can the
>> queue overflow?
>> 
>> To reduce this risk, my original implementation strategy was different. The
>> update speed was limited by i2c writing. A new register update batch job
>> was only scheduled if the previous one is finished. If i2c was 
>> blocked/congested,
>> the writing worker thread would come to a halt.
>> 
>> All incoming led brightness changes were simply accumulated until a new
>> batch job is started, because LEDs would anyways flicker invisibly fast.
>> 
>> Tests with the new driver have shown that it seems not to run into this 
>> situation
>> on our system but it might depend on factors we have not yet tested (slow 
>> i2c,
>> other i2c traffic on the same bus, CPU speed, event types choosen).
>> 
>> So I am a little in doubt about this risk. But I may have simply missed
>> the reason why the standard approach works and can never overflow.
>> 
>> BR,
>> Nikolaus
>> 
>> 
>> 
> 
> 
> -- 
> Best regards,
> Jacek Anaszewski

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[H. Nikolaus Schaller]

Hi,

> Am 07.07.2016 um 10:46 schrieb Jacek Anaszewski :
> 
> Hi Nikolaus,
> 
> On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:
>> Hi,
>> finally, I found the time to update the driver according to the many comments
>> received a while ago.
>> 
>> Most of them have been worked in, including the regmap idea and
>> brightness_set_blocking().
>> 
>> The driver works on our system, so that I will mail [PATCH v2] as a followup.
>> 
>> There is only one aspect of the new solution I am not sure if it is
>> really better than our old proposal (see below).
>> 
>> 
>>> Am 20.04.2016 um 23:04 schrieb Jacek Anaszewski 
>>> :
>>> 
>>> On 04/19/2016 07:21 PM, H. Nikolaus Schaller wrote:
> I believe the LEDS core now handles the workqueues generically for
> blocking operations, so it's no longer needed in the individual drivers.
 
 We had a lot of trouble with locking and blocking especially if we
 want to indicate CPU or (root) disk activity.
>>> 
>>> What kind of troubles you had? Could you share more details?
>>> Does it mean that current LED core design doesn't fit for your
>>> use cases?
>> 
>> The system started to flicker the LEDs irregularily and sometimes
>> the whole kernel stalled.
>> 
>>> 
 So it is implemented in a way that changes can be requested faster
 than the I2C bus can write new values to the chip.
 
 Only after one sequence of I2C writes is done, another work function
 can be scheduled. And each group of writes updates as many LEDs
 in parallel if necessary.
>>> 
>>> You can serialize the operations in brightness_set_blocking with
>>> a mutex.
>> 
>> Yes, that works fine in our (incomplete) test setup.
>> 
>> But I think it assumes that the i2c bus is never congested by other i2c 
>> traffic.
>> 
>> I have not found code that obviously takes care of the situation if led
>> trigger events (e.g. mmc or cpu triggers) are coming in faster than the
>> i2c (even using regmap) can write out over i2c.
>> 
>> If I understand the led core code correctly, it will just do another 
>> schedule_work
>> for every single change of led brightness.
> 
> Please look at schedule_work documentation in include/linux/workqueue.h:
> 
> /**
> * schedule_work - put work task in global workqueue
> * @work: job to be done
> *
> * Returns %false if @work was already on the kernel-global workqueue and
> * %true otherwise.
> *
> * This puts a job in the kernel-global workqueue if it was not already

Ah, ok. I missed that. Thanks!

> * queued and leaves it in the same position on the kernel-global
> * workqueue otherwise.
> */
> static inline bool schedule_work(struct work_struct *work)
> {
>return queue_work(system_wq, work);
> }

> 
>> 
>> So I wonder if Is there some guarantee that this work queue will not fill
>> up memory and is really processed faster than being filled? I.e. can the
>> queue overflow?
>> 
>> To reduce this risk, my original implementation strategy was different. The
>> update speed was limited by i2c writing. A new register update batch job
>> was only scheduled if the previous one is finished. If i2c was 
>> blocked/congested,
>> the writing worker thread would come to a halt.
>> 
>> All incoming led brightness changes were simply accumulated until a new
>> batch job is started, because LEDs would anyways flicker invisibly fast.
>> 
>> Tests with the new driver have shown that it seems not to run into this 
>> situation
>> on our system but it might depend on factors we have not yet tested (slow 
>> i2c,
>> other i2c traffic on the same bus, CPU speed, event types choosen).
>> 
>> So I am a little in doubt about this risk. But I may have simply missed
>> the reason why the standard approach works and can never overflow.
>> 
>> BR,
>> Nikolaus
>> 
>> 
>> 
> 
> 
> -- 
> Best regards,
> Jacek Anaszewski

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[Jacek Anaszewski]

Hi Nikolaus,

On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:

Hi,
finally, I found the time to update the driver according to the many comments
received a while ago.

Most of them have been worked in, including the regmap idea and
brightness_set_blocking().

The driver works on our system, so that I will mail [PATCH v2] as a followup.

There is only one aspect of the new solution I am not sure if it is
really better than our old proposal (see below).



Am 20.04.2016 um 23:04 schrieb Jacek Anaszewski :

On 04/19/2016 07:21 PM, H. Nikolaus Schaller wrote:

I believe the LEDS core now handles the workqueues generically for
blocking operations, so it's no longer needed in the individual drivers.


We had a lot of trouble with locking and blocking especially if we
want to indicate CPU or (root) disk activity.


What kind of troubles you had? Could you share more details?
Does it mean that current LED core design doesn't fit for your
use cases?


The system started to flicker the LEDs irregularily and sometimes
the whole kernel stalled.




So it is implemented in a way that changes can be requested faster
than the I2C bus can write new values to the chip.

Only after one sequence of I2C writes is done, another work function
can be scheduled. And each group of writes updates as many LEDs
in parallel if necessary.


You can serialize the operations in brightness_set_blocking with
a mutex.


Yes, that works fine in our (incomplete) test setup.

But I think it assumes that the i2c bus is never congested by other i2c traffic.

I have not found code that obviously takes care of the situation if led
trigger events (e.g. mmc or cpu triggers) are coming in faster than the
i2c (even using regmap) can write out over i2c.

If I understand the led core code correctly, it will just do another 
schedule_work
for every single change of led brightness.


Please look at schedule_work documentation in include/linux/workqueue.h:

/**
 * schedule_work - put work task in global workqueue
 * @work: job to be done
 *
 * Returns %false if @work was already on the kernel-global workqueue and
 * %true otherwise.
 *
 * This puts a job in the kernel-global workqueue if it was not already
 * queued and leaves it in the same position on the kernel-global
 * workqueue otherwise.
 */
static inline bool schedule_work(struct work_struct *work)
{
return queue_work(system_wq, work);
}



So I wonder if Is there some guarantee that this work queue will not fill
up memory and is really processed faster than being filled? I.e. can the
queue overflow?

To reduce this risk, my original implementation strategy was different. The
update speed was limited by i2c writing. A new register update batch job
was only scheduled if the previous one is finished. If i2c was 
blocked/congested,
the writing worker thread would come to a halt.

All incoming led brightness changes were simply accumulated until a new
batch job is started, because LEDs would anyways flicker invisibly fast.

Tests with the new driver have shown that it seems not to run into this 
situation
on our system but it might depend on factors we have not yet tested (slow i2c,
other i2c traffic on the same bus, CPU speed, event types choosen).

So I am a little in doubt about this risk. But I may have simply missed
the reason why the standard approach works and can never overflow.

BR,
Nikolaus






--
Best regards,
Jacek Anaszewski

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[Jacek Anaszewski]

Hi Nikolaus,

On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:

Hi,
finally, I found the time to update the driver according to the many comments
received a while ago.

Most of them have been worked in, including the regmap idea and
brightness_set_blocking().

The driver works on our system, so that I will mail [PATCH v2] as a followup.

There is only one aspect of the new solution I am not sure if it is
really better than our old proposal (see below).



Am 20.04.2016 um 23:04 schrieb Jacek Anaszewski :

On 04/19/2016 07:21 PM, H. Nikolaus Schaller wrote:

I believe the LEDS core now handles the workqueues generically for
blocking operations, so it's no longer needed in the individual drivers.


We had a lot of trouble with locking and blocking especially if we
want to indicate CPU or (root) disk activity.


What kind of troubles you had? Could you share more details?
Does it mean that current LED core design doesn't fit for your
use cases?


The system started to flicker the LEDs irregularily and sometimes
the whole kernel stalled.




So it is implemented in a way that changes can be requested faster
than the I2C bus can write new values to the chip.

Only after one sequence of I2C writes is done, another work function
can be scheduled. And each group of writes updates as many LEDs
in parallel if necessary.


You can serialize the operations in brightness_set_blocking with
a mutex.


Yes, that works fine in our (incomplete) test setup.

But I think it assumes that the i2c bus is never congested by other i2c traffic.

I have not found code that obviously takes care of the situation if led
trigger events (e.g. mmc or cpu triggers) are coming in faster than the
i2c (even using regmap) can write out over i2c.

If I understand the led core code correctly, it will just do another 
schedule_work
for every single change of led brightness.


Please look at schedule_work documentation in include/linux/workqueue.h:

/**
 * schedule_work - put work task in global workqueue
 * @work: job to be done
 *
 * Returns %false if @work was already on the kernel-global workqueue and
 * %true otherwise.
 *
 * This puts a job in the kernel-global workqueue if it was not already
 * queued and leaves it in the same position on the kernel-global
 * workqueue otherwise.
 */
static inline bool schedule_work(struct work_struct *work)
{
return queue_work(system_wq, work);
}



So I wonder if Is there some guarantee that this work queue will not fill
up memory and is really processed faster than being filled? I.e. can the
queue overflow?

To reduce this risk, my original implementation strategy was different. The
update speed was limited by i2c writing. A new register update batch job
was only scheduled if the previous one is finished. If i2c was 
blocked/congested,
the writing worker thread would come to a halt.

All incoming led brightness changes were simply accumulated until a new
batch job is started, because LEDs would anyways flicker invisibly fast.

Tests with the new driver have shown that it seems not to run into this 
situation
on our system but it might depend on factors we have not yet tested (slow i2c,
other i2c traffic on the same bus, CPU speed, event types choosen).

So I am a little in doubt about this risk. But I may have simply missed
the reason why the standard approach works and can never overflow.

BR,
Nikolaus






--
Best regards,
Jacek Anaszewski

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[H. Nikolaus Schaller]

Hi,

> Am 21.04.2016 um 17:01 schrieb Rob Herring :
> 
> On Mon, Apr 18, 2016 at 08:43:16PM +0200, H. Nikolaus Schaller wrote:
>> This is a driver for the Integrated Silicon Solution Inc. LED driver
>> chips IS31FL3196 and IS31FL3199. They can drive up to 6 or 9
>> LEDs.
>> 
>> Each LED is individually controllable in brightness (through pwm)
>> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
>> 
>> The maximum current of the LEDs can be programmed and limited to
>> 5 .. 40mA through a device tree property.
>> 
>> The chip is connected through I2C and can have one of 4 addresses (0x64 .. 
>> 0x67)
>> depending on how the AD pin is connected. The address is defined by the
>> reg property as usual.
>> 
>> The chip also has a shutdown input which could be connected to a GPIO,
>> but this driver uses software shutdown if all LEDs are inactivated.
>> 
>> The chip also has breathing and audio features which are not supported
>> by this driver.
>> 
>> This driver was developed and tested on OMAP5 based Pyra handheld prototype.
>> 
>> Signed-off-by: H. Nikolaus Schaller 
>> ---
>> .../devicetree/bindings/leds/is31fl319x.txt|  41 +++
>> drivers/leds/Kconfig   |   8 +
>> drivers/leds/Makefile  |   1 +
>> drivers/leds/leds-is31fl319x.c | 406 
>> +
>> include/linux/leds-is31fl319x.h|  24 ++
>> 5 files changed, 480 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
>> create mode 100644 drivers/leds/leds-is31fl319x.c
>> create mode 100644 include/linux/leds-is31fl319x.h
>> 
>> diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt 
>> b/Documentation/devicetree/bindings/leds/is31fl319x.txt
>> new file mode 100644
>> index 000..d13c7ca
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
>> @@ -0,0 +1,41 @@
>> +LEDs connected to is31fl319x RGB LED controller chip
>> +
>> +Required properties:
>> +- compatible : should be : "issi,is31fl3196" or "issi,is31fl3199".
> 
> One per line please.
> 
>> +- #address-cells: must be 1
>> +- #size-cells: must be 0
>> +- reg: 0x64, 0x65, 0x66, 0x67.
>> +
>> +Optional properties:
>> +- max-current-ma:   maximum led current (5..40 mA, default 20 mA)
> 
> Use standard property led-max-microamp.
> 
>> +- audio-gain-db:audio gain selection (0..21 dB. default 0 dB)
>> +- breathing & audio:not implemented
>> +
>> +Each led is represented as a sub-node of the issi,is31fl319x device.
>> +
>> +LED sub-node properties:
>> +- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
>> +- reg : number of LED line (0 to 5 or 0 to 8 - not all need to be present)
>> +- linux,default-trigger : (optional)
>> +   see Documentation/devicetree/bindings/leds/common.txt
>> +
>> +Examples:
>> +
>> +fancy_leds: is31fl3196@65 {
>> +compatible = "issi,is31fl319x";
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +reg = <0x65>;
>> +
>> +led0: red-aux@0 {
> 
> Use generic unit names:
> 
> red-aux: led@0
> 
>> +label = "red:aux";
>> +reg = <0>;
>> +};
>> +
>> +led1: green-power@4 {
> 
> ditto
> 
>> +label = "green:power";
>> +reg = <4>;
>> +linux,default-trigger = "default-on";
>> +};
>> +};
>> +

thanks! Will be included in upcoming V2.

BR,
Nikolaus

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[H. Nikolaus Schaller]

Hi,

> Am 21.04.2016 um 17:01 schrieb Rob Herring :
> 
> On Mon, Apr 18, 2016 at 08:43:16PM +0200, H. Nikolaus Schaller wrote:
>> This is a driver for the Integrated Silicon Solution Inc. LED driver
>> chips IS31FL3196 and IS31FL3199. They can drive up to 6 or 9
>> LEDs.
>> 
>> Each LED is individually controllable in brightness (through pwm)
>> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
>> 
>> The maximum current of the LEDs can be programmed and limited to
>> 5 .. 40mA through a device tree property.
>> 
>> The chip is connected through I2C and can have one of 4 addresses (0x64 .. 
>> 0x67)
>> depending on how the AD pin is connected. The address is defined by the
>> reg property as usual.
>> 
>> The chip also has a shutdown input which could be connected to a GPIO,
>> but this driver uses software shutdown if all LEDs are inactivated.
>> 
>> The chip also has breathing and audio features which are not supported
>> by this driver.
>> 
>> This driver was developed and tested on OMAP5 based Pyra handheld prototype.
>> 
>> Signed-off-by: H. Nikolaus Schaller 
>> ---
>> .../devicetree/bindings/leds/is31fl319x.txt|  41 +++
>> drivers/leds/Kconfig   |   8 +
>> drivers/leds/Makefile  |   1 +
>> drivers/leds/leds-is31fl319x.c | 406 
>> +
>> include/linux/leds-is31fl319x.h|  24 ++
>> 5 files changed, 480 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
>> create mode 100644 drivers/leds/leds-is31fl319x.c
>> create mode 100644 include/linux/leds-is31fl319x.h
>> 
>> diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt 
>> b/Documentation/devicetree/bindings/leds/is31fl319x.txt
>> new file mode 100644
>> index 000..d13c7ca
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
>> @@ -0,0 +1,41 @@
>> +LEDs connected to is31fl319x RGB LED controller chip
>> +
>> +Required properties:
>> +- compatible : should be : "issi,is31fl3196" or "issi,is31fl3199".
> 
> One per line please.
> 
>> +- #address-cells: must be 1
>> +- #size-cells: must be 0
>> +- reg: 0x64, 0x65, 0x66, 0x67.
>> +
>> +Optional properties:
>> +- max-current-ma:   maximum led current (5..40 mA, default 20 mA)
> 
> Use standard property led-max-microamp.
> 
>> +- audio-gain-db:audio gain selection (0..21 dB. default 0 dB)
>> +- breathing & audio:not implemented
>> +
>> +Each led is represented as a sub-node of the issi,is31fl319x device.
>> +
>> +LED sub-node properties:
>> +- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
>> +- reg : number of LED line (0 to 5 or 0 to 8 - not all need to be present)
>> +- linux,default-trigger : (optional)
>> +   see Documentation/devicetree/bindings/leds/common.txt
>> +
>> +Examples:
>> +
>> +fancy_leds: is31fl3196@65 {
>> +compatible = "issi,is31fl319x";
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +reg = <0x65>;
>> +
>> +led0: red-aux@0 {
> 
> Use generic unit names:
> 
> red-aux: led@0
> 
>> +label = "red:aux";
>> +reg = <0>;
>> +};
>> +
>> +led1: green-power@4 {
> 
> ditto
> 
>> +label = "green:power";
>> +reg = <4>;
>> +linux,default-trigger = "default-on";
>> +};
>> +};
>> +

thanks! Will be included in upcoming V2.

BR,
Nikolaus

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[Rob Herring]

On Mon, Apr 18, 2016 at 08:43:16PM +0200, H. Nikolaus Schaller wrote:
> This is a driver for the Integrated Silicon Solution Inc. LED driver
> chips IS31FL3196 and IS31FL3199. They can drive up to 6 or 9
> LEDs.
> 
> Each LED is individually controllable in brightness (through pwm)
> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
> 
> The maximum current of the LEDs can be programmed and limited to
> 5 .. 40mA through a device tree property.
> 
> The chip is connected through I2C and can have one of 4 addresses (0x64 .. 
> 0x67)
> depending on how the AD pin is connected. The address is defined by the
> reg property as usual.
> 
> The chip also has a shutdown input which could be connected to a GPIO,
> but this driver uses software shutdown if all LEDs are inactivated.
> 
> The chip also has breathing and audio features which are not supported
> by this driver.
> 
> This driver was developed and tested on OMAP5 based Pyra handheld prototype.
> 
> Signed-off-by: H. Nikolaus Schaller 
> ---
>  .../devicetree/bindings/leds/is31fl319x.txt|  41 +++
>  drivers/leds/Kconfig   |   8 +
>  drivers/leds/Makefile  |   1 +
>  drivers/leds/leds-is31fl319x.c | 406 
> +
>  include/linux/leds-is31fl319x.h|  24 ++
>  5 files changed, 480 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
>  create mode 100644 drivers/leds/leds-is31fl319x.c
>  create mode 100644 include/linux/leds-is31fl319x.h
> 
> diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt 
> b/Documentation/devicetree/bindings/leds/is31fl319x.txt
> new file mode 100644
> index 000..d13c7ca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
> @@ -0,0 +1,41 @@
> +LEDs connected to is31fl319x RGB LED controller chip
> +
> +Required properties:
> +- compatible : should be : "issi,is31fl3196" or "issi,is31fl3199".

One per line please.

> +- #address-cells: must be 1
> +- #size-cells: must be 0
> +- reg: 0x64, 0x65, 0x66, 0x67.
> +
> +Optional properties:
> +- max-current-ma:maximum led current (5..40 mA, default 20 mA)

Use standard property led-max-microamp.

> +- audio-gain-db: audio gain selection (0..21 dB. default 0 dB)
> +- breathing & audio: not implemented
> +
> +Each led is represented as a sub-node of the issi,is31fl319x device.
> +
> +LED sub-node properties:
> +- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
> +- reg : number of LED line (0 to 5 or 0 to 8 - not all need to be present)
> +- linux,default-trigger : (optional)
> +   see Documentation/devicetree/bindings/leds/common.txt
> +
> +Examples:
> +
> +fancy_leds: is31fl3196@65 {
> + compatible = "issi,is31fl319x";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x65>;
> +
> + led0: red-aux@0 {

Use generic unit names:

red-aux: led@0

> + label = "red:aux";
> + reg = <0>;
> + };
> +
> + led1: green-power@4 {

ditto

> + label = "green:power";
> + reg = <4>;
> + linux,default-trigger = "default-on";
> + };
> +};
> +

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[Rob Herring]

On Mon, Apr 18, 2016 at 08:43:16PM +0200, H. Nikolaus Schaller wrote:
> This is a driver for the Integrated Silicon Solution Inc. LED driver
> chips IS31FL3196 and IS31FL3199. They can drive up to 6 or 9
> LEDs.
> 
> Each LED is individually controllable in brightness (through pwm)
> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
> 
> The maximum current of the LEDs can be programmed and limited to
> 5 .. 40mA through a device tree property.
> 
> The chip is connected through I2C and can have one of 4 addresses (0x64 .. 
> 0x67)
> depending on how the AD pin is connected. The address is defined by the
> reg property as usual.
> 
> The chip also has a shutdown input which could be connected to a GPIO,
> but this driver uses software shutdown if all LEDs are inactivated.
> 
> The chip also has breathing and audio features which are not supported
> by this driver.
> 
> This driver was developed and tested on OMAP5 based Pyra handheld prototype.
> 
> Signed-off-by: H. Nikolaus Schaller 
> ---
>  .../devicetree/bindings/leds/is31fl319x.txt|  41 +++
>  drivers/leds/Kconfig   |   8 +
>  drivers/leds/Makefile  |   1 +
>  drivers/leds/leds-is31fl319x.c | 406 
> +
>  include/linux/leds-is31fl319x.h|  24 ++
>  5 files changed, 480 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
>  create mode 100644 drivers/leds/leds-is31fl319x.c
>  create mode 100644 include/linux/leds-is31fl319x.h
> 
> diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt 
> b/Documentation/devicetree/bindings/leds/is31fl319x.txt
> new file mode 100644
> index 000..d13c7ca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
> @@ -0,0 +1,41 @@
> +LEDs connected to is31fl319x RGB LED controller chip
> +
> +Required properties:
> +- compatible : should be : "issi,is31fl3196" or "issi,is31fl3199".

One per line please.

> +- #address-cells: must be 1
> +- #size-cells: must be 0
> +- reg: 0x64, 0x65, 0x66, 0x67.
> +
> +Optional properties:
> +- max-current-ma:maximum led current (5..40 mA, default 20 mA)

Use standard property led-max-microamp.

> +- audio-gain-db: audio gain selection (0..21 dB. default 0 dB)
> +- breathing & audio: not implemented
> +
> +Each led is represented as a sub-node of the issi,is31fl319x device.
> +
> +LED sub-node properties:
> +- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
> +- reg : number of LED line (0 to 5 or 0 to 8 - not all need to be present)
> +- linux,default-trigger : (optional)
> +   see Documentation/devicetree/bindings/leds/common.txt
> +
> +Examples:
> +
> +fancy_leds: is31fl3196@65 {
> + compatible = "issi,is31fl319x";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x65>;
> +
> + led0: red-aux@0 {

Use generic unit names:

red-aux: led@0

> + label = "red:aux";
> + reg = <0>;
> + };
> +
> + led1: green-power@4 {

ditto

> + label = "green:power";
> + reg = <4>;
> + linux,default-trigger = "default-on";
> + };
> +};
> +

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[Jacek Anaszewski]

Hi Nikolaus,

On 04/19/2016 07:21 PM, H. Nikolaus Schaller wrote:

Hi David,



Am 19.04.2016 um 03:25 schrieb David Rivshin (Allworx) 
:

[...]

+struct is31fl319x_chip {
+   struct i2c_client   *client;
+   struct work_struct  work;
+   boolwork_scheduled;
+   spinlock_t  lock;
+   u8  reg_file[IS31FL319X_REG_CNT];


I would suggest using the regmap infrastructure if you need
to cache the register values. It also helpfully exports a
debugfs interface.


There was an issue with regmap I don't exactly remember.

Maybe it was because it is impossible to read (initial) values from the
chip.

>

Only write new ones. And check for chip presence can also only be
done by checking if it responds or fails to a write command. So we would
have to use tricks to make regmap work as intended.


Wouldn't it be enough to write Reset Register, to check
the device presence? You would hit two birds with one stone,
by having registers reset to a known default state, according
to the documentation.


And it may introduce overhead in initialization and does
not save memory.




+
+   struct is31fl319x_led {
+   struct is31fl319x_chip  *chip;
+   struct led_classdev led_cdev;
+   } leds[NUM_LEDS];
+};
+
+static const struct i2c_device_id is31fl319x_id[] = {
+   { "is31fl3196", 6 },
+   { "is31fl3196", 9 },


^^^there is also a bug using is31fl3196 twice


+   { }
+};
+MODULE_DEVICE_TABLE(i2c, is31fl319x_id);
+
+
+static int is31fl319x_write(struct is31fl319x_chip *is31, u8 reg, u8 byte)
+{
+   struct i2c_client *cl = is31->client;
+
+   if (reg >= IS31FL319X_REG_CNT)
+   return -EIO;
+   is31->reg_file[reg] = byte;  /* save in cache */
+   dev_dbg(>client->dev, "write %02x to reg %02x\n", byte, reg);
+   return i2c_smbus_write_byte_data(cl, reg, byte);
+}
+
+static int is31fl319x_read(struct is31fl319x_chip *is31, u8 reg)
+{
+   if (reg >= IS31FL319X_REG_CNT)
+   return -EIO;
+   return is31->reg_file[reg];  /* read crom cache (can't read chip) */
+}
+
+static void is31fl319x_work(struct work_struct *work)
+{
+   struct is31fl319x_chip *is31 = container_of(work,
+   struct is31fl319x_chip,
+   work);
+   unsigned long flags;
+   int i;
+   u8 ctrl1, ctrl2;
+
+   dev_dbg(>client->dev, "work called\n");
+
+   spin_lock_irqsave(>lock, flags);
+   /* make subsequent changes run another schedule_work */
+   is31->work_scheduled = false;
+   spin_unlock_irqrestore(>lock, flags);


I believe the LEDS core now handles the workqueues generically for
blocking operations, so it's no longer needed in the individual drivers.


We had a lot of trouble with locking and blocking especially if we
want to indicate CPU or (root) disk activity.


What kind of troubles you had? Could you share more details?
Does it mean that current LED core design doesn't fit for your
use cases?


So it is implemented in a way that changes can be requested faster
than the I2C bus can write new values to the chip.

Only after one sequence of I2C writes is done, another work function
can be scheduled. And each group of writes updates as many LEDs
in parallel if necessary.


You can serialize the operations in brightness_set_blocking with
a mutex.




+
+   dev_dbg(>client->dev, "write to chip\n");
+
+   ctrl1 = 0;
+   ctrl2 = 0;
+
+   for (i = 0; i < NUM_LEDS; i++) {
+   struct led_classdev *led = >leds[i].led_cdev;
+   bool on;
+
+   if (!is31->leds[i].led_cdev.name)
+   continue;
+
+   dev_dbg(>client->dev, "set brightness %u for led %u\n",
+   led->brightness, i);
+
+   /* update brightness register */
+   is31fl319x_write(is31, IS31FL319X_PWM1 + i, led->brightness);
+
+   /* update output enable bits */
+   on = led->brightness > LED_OFF;
+   if (i < 3)
+   ctrl1 |= on << i; /* 0..2 => bit 0..2 */
+   else if (i < 6)
+   ctrl1 |= on << (i+1); /* 3..5 => bit 4..6 */
+   else
+   ctrl2 |= on << (i-6); /* 6..8 => bit 0..2 */
+   }


Is any locking needed while iterating over is31->leds[]? Is there any
opportunity for a race?


No, because our locking mechanism ensures that only one work function
is running at a time.

Yes there is a small race if a brightness value is updated by an interrupt.

But since the worker is already running, another one will be scheduled that
will fix the value. So for an invisibly short moment there might be the wrong 
value.




+
+   /* check if any PWM is enabled or all outputs are now off */
+   if (ctrl1 > 0 || 

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[Jacek Anaszewski]

Hi Nikolaus,

On 04/19/2016 07:21 PM, H. Nikolaus Schaller wrote:

Hi David,



Am 19.04.2016 um 03:25 schrieb David Rivshin (Allworx) 
:

[...]

+struct is31fl319x_chip {
+   struct i2c_client   *client;
+   struct work_struct  work;
+   boolwork_scheduled;
+   spinlock_t  lock;
+   u8  reg_file[IS31FL319X_REG_CNT];


I would suggest using the regmap infrastructure if you need
to cache the register values. It also helpfully exports a
debugfs interface.


There was an issue with regmap I don't exactly remember.

Maybe it was because it is impossible to read (initial) values from the
chip.

>

Only write new ones. And check for chip presence can also only be
done by checking if it responds or fails to a write command. So we would
have to use tricks to make regmap work as intended.


Wouldn't it be enough to write Reset Register, to check
the device presence? You would hit two birds with one stone,
by having registers reset to a known default state, according
to the documentation.


And it may introduce overhead in initialization and does
not save memory.




+
+   struct is31fl319x_led {
+   struct is31fl319x_chip  *chip;
+   struct led_classdev led_cdev;
+   } leds[NUM_LEDS];
+};
+
+static const struct i2c_device_id is31fl319x_id[] = {
+   { "is31fl3196", 6 },
+   { "is31fl3196", 9 },


^^^there is also a bug using is31fl3196 twice


+   { }
+};
+MODULE_DEVICE_TABLE(i2c, is31fl319x_id);
+
+
+static int is31fl319x_write(struct is31fl319x_chip *is31, u8 reg, u8 byte)
+{
+   struct i2c_client *cl = is31->client;
+
+   if (reg >= IS31FL319X_REG_CNT)
+   return -EIO;
+   is31->reg_file[reg] = byte;  /* save in cache */
+   dev_dbg(>client->dev, "write %02x to reg %02x\n", byte, reg);
+   return i2c_smbus_write_byte_data(cl, reg, byte);
+}
+
+static int is31fl319x_read(struct is31fl319x_chip *is31, u8 reg)
+{
+   if (reg >= IS31FL319X_REG_CNT)
+   return -EIO;
+   return is31->reg_file[reg];  /* read crom cache (can't read chip) */
+}
+
+static void is31fl319x_work(struct work_struct *work)
+{
+   struct is31fl319x_chip *is31 = container_of(work,
+   struct is31fl319x_chip,
+   work);
+   unsigned long flags;
+   int i;
+   u8 ctrl1, ctrl2;
+
+   dev_dbg(>client->dev, "work called\n");
+
+   spin_lock_irqsave(>lock, flags);
+   /* make subsequent changes run another schedule_work */
+   is31->work_scheduled = false;
+   spin_unlock_irqrestore(>lock, flags);


I believe the LEDS core now handles the workqueues generically for
blocking operations, so it's no longer needed in the individual drivers.


We had a lot of trouble with locking and blocking especially if we
want to indicate CPU or (root) disk activity.


What kind of troubles you had? Could you share more details?
Does it mean that current LED core design doesn't fit for your
use cases?


So it is implemented in a way that changes can be requested faster
than the I2C bus can write new values to the chip.

Only after one sequence of I2C writes is done, another work function
can be scheduled. And each group of writes updates as many LEDs
in parallel if necessary.


You can serialize the operations in brightness_set_blocking with
a mutex.




+
+   dev_dbg(>client->dev, "write to chip\n");
+
+   ctrl1 = 0;
+   ctrl2 = 0;
+
+   for (i = 0; i < NUM_LEDS; i++) {
+   struct led_classdev *led = >leds[i].led_cdev;
+   bool on;
+
+   if (!is31->leds[i].led_cdev.name)
+   continue;
+
+   dev_dbg(>client->dev, "set brightness %u for led %u\n",
+   led->brightness, i);
+
+   /* update brightness register */
+   is31fl319x_write(is31, IS31FL319X_PWM1 + i, led->brightness);
+
+   /* update output enable bits */
+   on = led->brightness > LED_OFF;
+   if (i < 3)
+   ctrl1 |= on << i; /* 0..2 => bit 0..2 */
+   else if (i < 6)
+   ctrl1 |= on << (i+1); /* 3..5 => bit 4..6 */
+   else
+   ctrl2 |= on << (i-6); /* 6..8 => bit 0..2 */
+   }


Is any locking needed while iterating over is31->leds[]? Is there any
opportunity for a race?


No, because our locking mechanism ensures that only one work function
is running at a time.

Yes there is a small race if a brightness value is updated by an interrupt.

But since the worker is already running, another one will be scheduled that
will fix the value. So for an invisibly short moment there might be the wrong 
value.




+
+   /* check if any PWM is enabled or all outputs are now off */
+   if (ctrl1 > 0 || ctrl2 > 0) {
+   

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[Jacek Anaszewski]

Hi Nikolaus,

On 04/19/2016 07:21 PM, H. Nikolaus Schaller wrote:

Hi Jacek,


Am 19.04.2016 um 11:14 schrieb Jacek Anaszewski :

Hi Nikolaus,

Thanks for the patch. Please refer to my remarks in the code.


Thanks for the remarks!


You're welcome.



On 04/18/2016 08:43 PM, H. Nikolaus Schaller wrote:

This is a driver for the Integrated Silicon Solution Inc. LED driver
chips IS31FL3196 and IS31FL3199. They can drive up to 6 or 9
LEDs.

Each LED is individually controllable in brightness (through pwm)
in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.

The maximum current of the LEDs can be programmed and limited to
5 .. 40mA through a device tree property.

The chip is connected through I2C and can have one of 4 addresses (0x64 .. 0x67)
depending on how the AD pin is connected. The address is defined by the
reg property as usual.

The chip also has a shutdown input which could be connected to a GPIO,
but this driver uses software shutdown if all LEDs are inactivated.

The chip also has breathing and audio features which are not supported
by this driver.

This driver was developed and tested on OMAP5 based Pyra handheld prototype.

Signed-off-by: H. Nikolaus Schaller 
---
  .../devicetree/bindings/leds/is31fl319x.txt|  41 +++
  drivers/leds/Kconfig   |   8 +
  drivers/leds/Makefile  |   1 +
  drivers/leds/leds-is31fl319x.c | 406 +
  include/linux/leds-is31fl319x.h|  24 ++
  5 files changed, 480 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
  create mode 100644 drivers/leds/leds-is31fl319x.c
  create mode 100644 include/linux/leds-is31fl319x.h

diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt 
b/Documentation/devicetree/bindings/leds/is31fl319x.txt
new file mode 100644
index 000..d13c7ca
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
@@ -0,0 +1,41 @@
+LEDs connected to is31fl319x RGB LED controller chip
+
+Required properties:
+- compatible : should be : "issi,is31fl3196" or "issi,is31fl3199".


s/should be :/Should be/


done.




+- #address-cells: must be 1
+- #size-cells: must be 0


s/must/Must/

Please begin the property description with a capital letter,
and also end it with a dot.
This remark applies also to the other properties.


Done.




+- reg: 0x64, 0x65, 0x66, 0x67.
+
+Optional properties:
+- max-current-ma:  maximum led current (5..40 mA, default 20 mA)


There is leds-max-microamp property for this -
see Documentation/devicetree/bindings/leds/common.txt.


Ah, I wasn't aware that this property should also apply to the whole chip (in 
that
document it is described as a property for each individual LED).


It is indented for individual LEDs. I missed that you defined it for
the whole chip. The property, when present, should have an impact on the
max_brightness property of the struct led_classdev.


Renamed.


I'd rephrase this as follows:

- led-max-microamp: See Documentation/devicetree/bindings/leds/common.txt.
Valid values: 5 - 40, step by (?) (rounded {up|down})
Default: 2


+- audio-gain-db:   audio gain selection (0..21 dB. default 0 dB)


Please follow the aforementioned "Valid values" pattern.
It would also be nice to have more informative description on the
purpose of this property.


Done.




+- breathing & audio:   not implemented


There is no point in documenting unused properties.


Done.




+
+Each led is represented as a sub-node of the issi,is31fl319x device.
+
+LED sub-node properties:
+- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
+- reg : number of LED line (0 to 5 or 0 to 8 - not all need to be present)
+- linux,default-trigger : (optional)
+   see Documentation/devicetree/bindings/leds/common.txt
+
+Examples:
+
+fancy_leds: is31fl3196@65 {
+   compatible = "issi,is31fl319x";
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <0x65>;
+
+   led0: red-aux@0 {
+   label = "red:aux";
+   reg = <0>;
+   };
+
+   led1: green-power@4 {
+   label = "green:power";
+   reg = <4>;
+   linux,default-trigger = "default-on";
+   };
+};


Generally I prefer to have DT bindings documentation in a separate
patch.


Yes, you are right and not only you prefer it :)
It just gets forgotten for such simple drivers...




diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 2251478..f099bcb 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -491,6 +491,14 @@ config LEDS_ASIC3
  cannot be used. This driver supports hardware blinking with an on+off
  period from 62ms to 125s. Say Y to enable LEDs on the HP iPAQ hx4700.

+config LEDS_IS31FL319X
+   tristate "LED Support for IS31FL319x I2C chip"
+   depends on 

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[Jacek Anaszewski]

Hi Nikolaus,

On 04/19/2016 07:21 PM, H. Nikolaus Schaller wrote:

Hi Jacek,


Am 19.04.2016 um 11:14 schrieb Jacek Anaszewski :

Hi Nikolaus,

Thanks for the patch. Please refer to my remarks in the code.


Thanks for the remarks!


You're welcome.



On 04/18/2016 08:43 PM, H. Nikolaus Schaller wrote:

This is a driver for the Integrated Silicon Solution Inc. LED driver
chips IS31FL3196 and IS31FL3199. They can drive up to 6 or 9
LEDs.

Each LED is individually controllable in brightness (through pwm)
in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.

The maximum current of the LEDs can be programmed and limited to
5 .. 40mA through a device tree property.

The chip is connected through I2C and can have one of 4 addresses (0x64 .. 0x67)
depending on how the AD pin is connected. The address is defined by the
reg property as usual.

The chip also has a shutdown input which could be connected to a GPIO,
but this driver uses software shutdown if all LEDs are inactivated.

The chip also has breathing and audio features which are not supported
by this driver.

This driver was developed and tested on OMAP5 based Pyra handheld prototype.

Signed-off-by: H. Nikolaus Schaller 
---
  .../devicetree/bindings/leds/is31fl319x.txt|  41 +++
  drivers/leds/Kconfig   |   8 +
  drivers/leds/Makefile  |   1 +
  drivers/leds/leds-is31fl319x.c | 406 +
  include/linux/leds-is31fl319x.h|  24 ++
  5 files changed, 480 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
  create mode 100644 drivers/leds/leds-is31fl319x.c
  create mode 100644 include/linux/leds-is31fl319x.h

diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt 
b/Documentation/devicetree/bindings/leds/is31fl319x.txt
new file mode 100644
index 000..d13c7ca
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
@@ -0,0 +1,41 @@
+LEDs connected to is31fl319x RGB LED controller chip
+
+Required properties:
+- compatible : should be : "issi,is31fl3196" or "issi,is31fl3199".


s/should be :/Should be/


done.




+- #address-cells: must be 1
+- #size-cells: must be 0


s/must/Must/

Please begin the property description with a capital letter,
and also end it with a dot.
This remark applies also to the other properties.


Done.




+- reg: 0x64, 0x65, 0x66, 0x67.
+
+Optional properties:
+- max-current-ma:  maximum led current (5..40 mA, default 20 mA)


There is leds-max-microamp property for this -
see Documentation/devicetree/bindings/leds/common.txt.


Ah, I wasn't aware that this property should also apply to the whole chip (in 
that
document it is described as a property for each individual LED).


It is indented for individual LEDs. I missed that you defined it for
the whole chip. The property, when present, should have an impact on the
max_brightness property of the struct led_classdev.


Renamed.


I'd rephrase this as follows:

- led-max-microamp: See Documentation/devicetree/bindings/leds/common.txt.
Valid values: 5 - 40, step by (?) (rounded {up|down})
Default: 2


+- audio-gain-db:   audio gain selection (0..21 dB. default 0 dB)


Please follow the aforementioned "Valid values" pattern.
It would also be nice to have more informative description on the
purpose of this property.


Done.




+- breathing & audio:   not implemented


There is no point in documenting unused properties.


Done.




+
+Each led is represented as a sub-node of the issi,is31fl319x device.
+
+LED sub-node properties:
+- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
+- reg : number of LED line (0 to 5 or 0 to 8 - not all need to be present)
+- linux,default-trigger : (optional)
+   see Documentation/devicetree/bindings/leds/common.txt
+
+Examples:
+
+fancy_leds: is31fl3196@65 {
+   compatible = "issi,is31fl319x";
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <0x65>;
+
+   led0: red-aux@0 {
+   label = "red:aux";
+   reg = <0>;
+   };
+
+   led1: green-power@4 {
+   label = "green:power";
+   reg = <4>;
+   linux,default-trigger = "default-on";
+   };
+};


Generally I prefer to have DT bindings documentation in a separate
patch.


Yes, you are right and not only you prefer it :)
It just gets forgotten for such simple drivers...




diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 2251478..f099bcb 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -491,6 +491,14 @@ config LEDS_ASIC3
  cannot be used. This driver supports hardware blinking with an on+off
  period from 62ms to 125s. Say Y to enable LEDs on the HP iPAQ hx4700.

+config LEDS_IS31FL319X
+   tristate "LED Support for IS31FL319x I2C chip"
+   depends on LEDS_CLASS && I2C
+   help
+ This 

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[H. Nikolaus Schaller]

Hi David,


> Am 19.04.2016 um 03:25 schrieb David Rivshin (Allworx) 
> :
> 
> Hi Nikolaus,
> 
> I recently submitted a driver for the IS31FL32xx family of devices, so
> this driver caught my eye. I have a few comments below.
> 
> On Mon, 18 Apr 2016 20:43:16 +0200
> "H. Nikolaus Schaller"  wrote:
> 
>> This is a driver for the Integrated Silicon Solution Inc. LED driver
>> chips IS31FL3196 and IS31FL3199. They can drive up to 6 or 9
>> LEDs.
>> 
>> Each LED is individually controllable in brightness (through pwm)
>> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
>> 
>> The maximum current of the LEDs can be programmed and limited to
>> 5 .. 40mA through a device tree property.
>> 
>> The chip is connected through I2C and can have one of 4 addresses (0x64 .. 
>> 0x67)
>> depending on how the AD pin is connected. The address is defined by the
>> reg property as usual.
>> 
>> The chip also has a shutdown input which could be connected to a GPIO,
>> but this driver uses software shutdown if all LEDs are inactivated.
>> 
>> The chip also has breathing and audio features which are not supported
>> by this driver.
>> 
>> This driver was developed and tested on OMAP5 based Pyra handheld prototype.
>> 
>> Signed-off-by: H. Nikolaus Schaller 
>> ---
>> .../devicetree/bindings/leds/is31fl319x.txt|  41 +++
>> drivers/leds/Kconfig   |   8 +
>> drivers/leds/Makefile  |   1 +
>> drivers/leds/leds-is31fl319x.c | 406 
>> +
>> include/linux/leds-is31fl319x.h|  24 ++
>> 5 files changed, 480 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
>> create mode 100644 drivers/leds/leds-is31fl319x.c
>> create mode 100644 include/linux/leds-is31fl319x.h
>> 
>> diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt 
>> b/Documentation/devicetree/bindings/leds/is31fl319x.txt
>> new file mode 100644
>> index 000..d13c7ca
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
>> @@ -0,0 +1,41 @@
>> +LEDs connected to is31fl319x RGB LED controller chip
>> +
>> +Required properties:
>> +- compatible : should be : "issi,is31fl3196" or "issi,is31fl3199".
> 
> From a quick look at the datasheets, I believe the Si-En SN3199 [1] 
> is the same as the ISSI IS31FL3199. ISSI purchased Si-En in 2011,
> and looks to have rebranded Si-En's existing LED controllers. 
> Assuming the SN3199 is indeed the same as the IS31FL3199, I would
> suggest adding a compatible string of "si-en,sn3199" both here
> and in the driver itself (and update the Kconfig text as well). 
> 
> [1] http://www.si-en.com/product.asp?parentid=890

Sounds reasonable. Added.

> 
>> +- #address-cells: must be 1
>> +- #size-cells: must be 0
>> +- reg: 0x64, 0x65, 0x66, 0x67.
>> +
>> +Optional properties:
>> +- max-current-ma:   maximum led current (5..40 mA, default 20 mA)
>> +- audio-gain-db:audio gain selection (0..21 dB. default 0 dB)
>> +- breathing & audio:not implemented
> 
> I'm not certain, but that last line feels wrong to me. I would expect
> to see just defined properties in this section, while that is more
> commentary. I would just leave it out, myself.

Yes, you are right. We had planned to add something and written the bindings
document first but never implemented it (because we don't need it any more).

> 
>> +
>> +Each led is represented as a sub-node of the issi,is31fl319x device.
>> +
>> +LED sub-node properties:
>> +- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
>> +- reg : number of LED line (0 to 5 or 0 to 8 - not all need to be present)
> 
> Since the datasheets for these devices name the outputs "OUT1" through 
> "OUT6"/"OUT9", I believe the correct range for the 'reg' property should 
> be 1-6 and 1-9. At least that was the result of a discussion with Rob
> Herring [2], and therefore how the IS31FL32xx binding/driver was done.
> It also makes devicetrees easier to write relative to schematics which 
> likely use the OUT1-N naming.
> 
> [2] http://www.spinics.net/lists/devicetree/msg116019.html

Ok, done.

> 
> 
>> +- linux,default-trigger : (optional)
>> +   see Documentation/devicetree/bindings/leds/common.txt
>> +
>> +Examples:
>> +
>> +fancy_leds: is31fl3196@65 {
> 
> I believe the current preference would be to name the node by
> its function, rather than the device. For example:
>   fancy_leds: leds@65

Updated.

> 
>> +compatible = "issi,is31fl319x";
> 
> I believe this should be "issi,is31fl3196" here.

Indeed.

> 
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +reg = <0x65>;
>> +
>> +led0: red-aux@0 {
>> +label = "red:aux";
>> +reg = <0>;
>> +};
>> +
>> +led1: green-power@4 {
>> +label = "green:power";
>> +reg = <4>;
>> +

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[H. Nikolaus Schaller]

Hi Jacek,

> Am 19.04.2016 um 11:14 schrieb Jacek Anaszewski :
> 
> Hi Nikolaus,
> 
> Thanks for the patch. Please refer to my remarks in the code.

Thanks for the remarks!

> 
> On 04/18/2016 08:43 PM, H. Nikolaus Schaller wrote:
>> This is a driver for the Integrated Silicon Solution Inc. LED driver
>> chips IS31FL3196 and IS31FL3199. They can drive up to 6 or 9
>> LEDs.
>> 
>> Each LED is individually controllable in brightness (through pwm)
>> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
>> 
>> The maximum current of the LEDs can be programmed and limited to
>> 5 .. 40mA through a device tree property.
>> 
>> The chip is connected through I2C and can have one of 4 addresses (0x64 .. 
>> 0x67)
>> depending on how the AD pin is connected. The address is defined by the
>> reg property as usual.
>> 
>> The chip also has a shutdown input which could be connected to a GPIO,
>> but this driver uses software shutdown if all LEDs are inactivated.
>> 
>> The chip also has breathing and audio features which are not supported
>> by this driver.
>> 
>> This driver was developed and tested on OMAP5 based Pyra handheld prototype.
>> 
>> Signed-off-by: H. Nikolaus Schaller 
>> ---
>>  .../devicetree/bindings/leds/is31fl319x.txt|  41 +++
>>  drivers/leds/Kconfig   |   8 +
>>  drivers/leds/Makefile  |   1 +
>>  drivers/leds/leds-is31fl319x.c | 406 
>> +
>>  include/linux/leds-is31fl319x.h|  24 ++
>>  5 files changed, 480 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
>>  create mode 100644 drivers/leds/leds-is31fl319x.c
>>  create mode 100644 include/linux/leds-is31fl319x.h
>> 
>> diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt 
>> b/Documentation/devicetree/bindings/leds/is31fl319x.txt
>> new file mode 100644
>> index 000..d13c7ca
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
>> @@ -0,0 +1,41 @@
>> +LEDs connected to is31fl319x RGB LED controller chip
>> +
>> +Required properties:
>> +- compatible : should be : "issi,is31fl3196" or "issi,is31fl3199".
> 
> s/should be :/Should be/

done.

> 
>> +- #address-cells: must be 1
>> +- #size-cells: must be 0
> 
> s/must/Must/
> 
> Please begin the property description with a capital letter,
> and also end it with a dot.
> This remark applies also to the other properties.

Done.

> 
>> +- reg: 0x64, 0x65, 0x66, 0x67.
>> +
>> +Optional properties:
>> +- max-current-ma:   maximum led current (5..40 mA, default 20 mA)
> 
> There is leds-max-microamp property for this -
> see Documentation/devicetree/bindings/leds/common.txt.

Ah, I wasn't aware that this property should also apply to the whole chip (in 
that
document it is described as a property for each individual LED).

Renamed.

> I'd rephrase this as follows:
> 
> - led-max-microamp: See Documentation/devicetree/bindings/leds/common.txt.
>Valid values: 5 - 40, step by (?) (rounded {up|down})
>Default: 2
> 
>> +- audio-gain-db:audio gain selection (0..21 dB. default 0 dB)
> 
> Please follow the aforementioned "Valid values" pattern.
> It would also be nice to have more informative description on the
> purpose of this property.

Done.

> 
>> +- breathing & audio:not implemented
> 
> There is no point in documenting unused properties.

Done.

> 
>> +
>> +Each led is represented as a sub-node of the issi,is31fl319x device.
>> +
>> +LED sub-node properties:
>> +- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
>> +- reg : number of LED line (0 to 5 or 0 to 8 - not all need to be present)
>> +- linux,default-trigger : (optional)
>> +   see Documentation/devicetree/bindings/leds/common.txt
>> +
>> +Examples:
>> +
>> +fancy_leds: is31fl3196@65 {
>> +compatible = "issi,is31fl319x";
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +reg = <0x65>;
>> +
>> +led0: red-aux@0 {
>> +label = "red:aux";
>> +reg = <0>;
>> +};
>> +
>> +led1: green-power@4 {
>> +label = "green:power";
>> +reg = <4>;
>> +linux,default-trigger = "default-on";
>> +};
>> +};
> 
> Generally I prefer to have DT bindings documentation in a separate
> patch.

Yes, you are right and not only you prefer it :)
It just gets forgotten for such simple drivers...

> 
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 2251478..f099bcb 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -491,6 +491,14 @@ config LEDS_ASIC3
>>cannot be used. This driver supports hardware blinking with an on+off
>>period from 62ms to 125s. Say Y to enable LEDs on the HP iPAQ hx4700.
>> 
>> +config LEDS_IS31FL319X
>> +tristate "LED Support for IS31FL319x I2C chip"
>> +depends on LEDS_CLASS && I2C

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[H. Nikolaus Schaller]

Hi David,


> Am 19.04.2016 um 03:25 schrieb David Rivshin (Allworx) 
> :
> 
> Hi Nikolaus,
> 
> I recently submitted a driver for the IS31FL32xx family of devices, so
> this driver caught my eye. I have a few comments below.
> 
> On Mon, 18 Apr 2016 20:43:16 +0200
> "H. Nikolaus Schaller"  wrote:
> 
>> This is a driver for the Integrated Silicon Solution Inc. LED driver
>> chips IS31FL3196 and IS31FL3199. They can drive up to 6 or 9
>> LEDs.
>> 
>> Each LED is individually controllable in brightness (through pwm)
>> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
>> 
>> The maximum current of the LEDs can be programmed and limited to
>> 5 .. 40mA through a device tree property.
>> 
>> The chip is connected through I2C and can have one of 4 addresses (0x64 .. 
>> 0x67)
>> depending on how the AD pin is connected. The address is defined by the
>> reg property as usual.
>> 
>> The chip also has a shutdown input which could be connected to a GPIO,
>> but this driver uses software shutdown if all LEDs are inactivated.
>> 
>> The chip also has breathing and audio features which are not supported
>> by this driver.
>> 
>> This driver was developed and tested on OMAP5 based Pyra handheld prototype.
>> 
>> Signed-off-by: H. Nikolaus Schaller 
>> ---
>> .../devicetree/bindings/leds/is31fl319x.txt|  41 +++
>> drivers/leds/Kconfig   |   8 +
>> drivers/leds/Makefile  |   1 +
>> drivers/leds/leds-is31fl319x.c | 406 
>> +
>> include/linux/leds-is31fl319x.h|  24 ++
>> 5 files changed, 480 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
>> create mode 100644 drivers/leds/leds-is31fl319x.c
>> create mode 100644 include/linux/leds-is31fl319x.h
>> 
>> diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt 
>> b/Documentation/devicetree/bindings/leds/is31fl319x.txt
>> new file mode 100644
>> index 000..d13c7ca
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
>> @@ -0,0 +1,41 @@
>> +LEDs connected to is31fl319x RGB LED controller chip
>> +
>> +Required properties:
>> +- compatible : should be : "issi,is31fl3196" or "issi,is31fl3199".
> 
> From a quick look at the datasheets, I believe the Si-En SN3199 [1] 
> is the same as the ISSI IS31FL3199. ISSI purchased Si-En in 2011,
> and looks to have rebranded Si-En's existing LED controllers. 
> Assuming the SN3199 is indeed the same as the IS31FL3199, I would
> suggest adding a compatible string of "si-en,sn3199" both here
> and in the driver itself (and update the Kconfig text as well). 
> 
> [1] http://www.si-en.com/product.asp?parentid=890

Sounds reasonable. Added.

> 
>> +- #address-cells: must be 1
>> +- #size-cells: must be 0
>> +- reg: 0x64, 0x65, 0x66, 0x67.
>> +
>> +Optional properties:
>> +- max-current-ma:   maximum led current (5..40 mA, default 20 mA)
>> +- audio-gain-db:audio gain selection (0..21 dB. default 0 dB)
>> +- breathing & audio:not implemented
> 
> I'm not certain, but that last line feels wrong to me. I would expect
> to see just defined properties in this section, while that is more
> commentary. I would just leave it out, myself.

Yes, you are right. We had planned to add something and written the bindings
document first but never implemented it (because we don't need it any more).

> 
>> +
>> +Each led is represented as a sub-node of the issi,is31fl319x device.
>> +
>> +LED sub-node properties:
>> +- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
>> +- reg : number of LED line (0 to 5 or 0 to 8 - not all need to be present)
> 
> Since the datasheets for these devices name the outputs "OUT1" through 
> "OUT6"/"OUT9", I believe the correct range for the 'reg' property should 
> be 1-6 and 1-9. At least that was the result of a discussion with Rob
> Herring [2], and therefore how the IS31FL32xx binding/driver was done.
> It also makes devicetrees easier to write relative to schematics which 
> likely use the OUT1-N naming.
> 
> [2] http://www.spinics.net/lists/devicetree/msg116019.html

Ok, done.

> 
> 
>> +- linux,default-trigger : (optional)
>> +   see Documentation/devicetree/bindings/leds/common.txt
>> +
>> +Examples:
>> +
>> +fancy_leds: is31fl3196@65 {
> 
> I believe the current preference would be to name the node by
> its function, rather than the device. For example:
>   fancy_leds: leds@65

Updated.

> 
>> +compatible = "issi,is31fl319x";
> 
> I believe this should be "issi,is31fl3196" here.

Indeed.

> 
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +reg = <0x65>;
>> +
>> +led0: red-aux@0 {
>> +label = "red:aux";
>> +reg = <0>;
>> +};
>> +
>> +led1: green-power@4 {
>> +label = "green:power";
>> +reg = <4>;
>> +linux,default-trigger = "default-on";
>> +};
>> +};
>> +
>> diff --git 

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[H. Nikolaus Schaller]

Hi Jacek,

> Am 19.04.2016 um 11:14 schrieb Jacek Anaszewski :
> 
> Hi Nikolaus,
> 
> Thanks for the patch. Please refer to my remarks in the code.

Thanks for the remarks!

> 
> On 04/18/2016 08:43 PM, H. Nikolaus Schaller wrote:
>> This is a driver for the Integrated Silicon Solution Inc. LED driver
>> chips IS31FL3196 and IS31FL3199. They can drive up to 6 or 9
>> LEDs.
>> 
>> Each LED is individually controllable in brightness (through pwm)
>> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
>> 
>> The maximum current of the LEDs can be programmed and limited to
>> 5 .. 40mA through a device tree property.
>> 
>> The chip is connected through I2C and can have one of 4 addresses (0x64 .. 
>> 0x67)
>> depending on how the AD pin is connected. The address is defined by the
>> reg property as usual.
>> 
>> The chip also has a shutdown input which could be connected to a GPIO,
>> but this driver uses software shutdown if all LEDs are inactivated.
>> 
>> The chip also has breathing and audio features which are not supported
>> by this driver.
>> 
>> This driver was developed and tested on OMAP5 based Pyra handheld prototype.
>> 
>> Signed-off-by: H. Nikolaus Schaller 
>> ---
>>  .../devicetree/bindings/leds/is31fl319x.txt|  41 +++
>>  drivers/leds/Kconfig   |   8 +
>>  drivers/leds/Makefile  |   1 +
>>  drivers/leds/leds-is31fl319x.c | 406 
>> +
>>  include/linux/leds-is31fl319x.h|  24 ++
>>  5 files changed, 480 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
>>  create mode 100644 drivers/leds/leds-is31fl319x.c
>>  create mode 100644 include/linux/leds-is31fl319x.h
>> 
>> diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt 
>> b/Documentation/devicetree/bindings/leds/is31fl319x.txt
>> new file mode 100644
>> index 000..d13c7ca
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
>> @@ -0,0 +1,41 @@
>> +LEDs connected to is31fl319x RGB LED controller chip
>> +
>> +Required properties:
>> +- compatible : should be : "issi,is31fl3196" or "issi,is31fl3199".
> 
> s/should be :/Should be/

done.

> 
>> +- #address-cells: must be 1
>> +- #size-cells: must be 0
> 
> s/must/Must/
> 
> Please begin the property description with a capital letter,
> and also end it with a dot.
> This remark applies also to the other properties.

Done.

> 
>> +- reg: 0x64, 0x65, 0x66, 0x67.
>> +
>> +Optional properties:
>> +- max-current-ma:   maximum led current (5..40 mA, default 20 mA)
> 
> There is leds-max-microamp property for this -
> see Documentation/devicetree/bindings/leds/common.txt.

Ah, I wasn't aware that this property should also apply to the whole chip (in 
that
document it is described as a property for each individual LED).

Renamed.

> I'd rephrase this as follows:
> 
> - led-max-microamp: See Documentation/devicetree/bindings/leds/common.txt.
>Valid values: 5 - 40, step by (?) (rounded {up|down})
>Default: 2
> 
>> +- audio-gain-db:audio gain selection (0..21 dB. default 0 dB)
> 
> Please follow the aforementioned "Valid values" pattern.
> It would also be nice to have more informative description on the
> purpose of this property.

Done.

> 
>> +- breathing & audio:not implemented
> 
> There is no point in documenting unused properties.

Done.

> 
>> +
>> +Each led is represented as a sub-node of the issi,is31fl319x device.
>> +
>> +LED sub-node properties:
>> +- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
>> +- reg : number of LED line (0 to 5 or 0 to 8 - not all need to be present)
>> +- linux,default-trigger : (optional)
>> +   see Documentation/devicetree/bindings/leds/common.txt
>> +
>> +Examples:
>> +
>> +fancy_leds: is31fl3196@65 {
>> +compatible = "issi,is31fl319x";
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +reg = <0x65>;
>> +
>> +led0: red-aux@0 {
>> +label = "red:aux";
>> +reg = <0>;
>> +};
>> +
>> +led1: green-power@4 {
>> +label = "green:power";
>> +reg = <4>;
>> +linux,default-trigger = "default-on";
>> +};
>> +};
> 
> Generally I prefer to have DT bindings documentation in a separate
> patch.

Yes, you are right and not only you prefer it :)
It just gets forgotten for such simple drivers...

> 
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 2251478..f099bcb 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -491,6 +491,14 @@ config LEDS_ASIC3
>>cannot be used. This driver supports hardware blinking with an on+off
>>period from 62ms to 125s. Say Y to enable LEDs on the HP iPAQ hx4700.
>> 
>> +config LEDS_IS31FL319X
>> +tristate "LED Support for IS31FL319x I2C chip"
>> +depends on LEDS_CLASS && I2C
>> +help
>> +  This option enables 

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[Jacek Anaszewski]

Hi David,

Thanks for the review.
Nikolaus - please address David's remarks.

Thanks,
Jacek Anaszewski

On 04/19/2016 03:25 AM, David Rivshin (Allworx) wrote:

Hi Nikolaus,

I recently submitted a driver for the IS31FL32xx family of devices, so
this driver caught my eye. I have a few comments below.

On Mon, 18 Apr 2016 20:43:16 +0200
"H. Nikolaus Schaller"  wrote:


This is a driver for the Integrated Silicon Solution Inc. LED driver
chips IS31FL3196 and IS31FL3199. They can drive up to 6 or 9
LEDs.

Each LED is individually controllable in brightness (through pwm)
in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.

The maximum current of the LEDs can be programmed and limited to
5 .. 40mA through a device tree property.

The chip is connected through I2C and can have one of 4 addresses (0x64 .. 0x67)
depending on how the AD pin is connected. The address is defined by the
reg property as usual.

The chip also has a shutdown input which could be connected to a GPIO,
but this driver uses software shutdown if all LEDs are inactivated.

The chip also has breathing and audio features which are not supported
by this driver.

This driver was developed and tested on OMAP5 based Pyra handheld prototype.

Signed-off-by: H. Nikolaus Schaller 
---
  .../devicetree/bindings/leds/is31fl319x.txt|  41 +++
  drivers/leds/Kconfig   |   8 +
  drivers/leds/Makefile  |   1 +
  drivers/leds/leds-is31fl319x.c | 406 +
  include/linux/leds-is31fl319x.h|  24 ++
  5 files changed, 480 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
  create mode 100644 drivers/leds/leds-is31fl319x.c
  create mode 100644 include/linux/leds-is31fl319x.h

diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt 
b/Documentation/devicetree/bindings/leds/is31fl319x.txt
new file mode 100644
index 000..d13c7ca
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
@@ -0,0 +1,41 @@
+LEDs connected to is31fl319x RGB LED controller chip
+
+Required properties:
+- compatible : should be : "issi,is31fl3196" or "issi,is31fl3199".



From a quick look at the datasheets, I believe the Si-En SN3199 [1]

is the same as the ISSI IS31FL3199. ISSI purchased Si-En in 2011,
and looks to have rebranded Si-En's existing LED controllers.
Assuming the SN3199 is indeed the same as the IS31FL3199, I would
suggest adding a compatible string of "si-en,sn3199" both here
and in the driver itself (and update the Kconfig text as well).

[1] http://www.si-en.com/product.asp?parentid=890


+- #address-cells: must be 1
+- #size-cells: must be 0
+- reg: 0x64, 0x65, 0x66, 0x67.
+
+Optional properties:
+- max-current-ma:  maximum led current (5..40 mA, default 20 mA)
+- audio-gain-db:   audio gain selection (0..21 dB. default 0 dB)
+- breathing & audio:   not implemented


I'm not certain, but that last line feels wrong to me. I would expect
to see just defined properties in this section, while that is more
commentary. I would just leave it out, myself.


+
+Each led is represented as a sub-node of the issi,is31fl319x device.
+
+LED sub-node properties:
+- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
+- reg : number of LED line (0 to 5 or 0 to 8 - not all need to be present)


Since the datasheets for these devices name the outputs "OUT1" through
"OUT6"/"OUT9", I believe the correct range for the 'reg' property should
be 1-6 and 1-9. At least that was the result of a discussion with Rob
Herring [2], and therefore how the IS31FL32xx binding/driver was done.
It also makes devicetrees easier to write relative to schematics which
likely use the OUT1-N naming.

[2] http://www.spinics.net/lists/devicetree/msg116019.html



+- linux,default-trigger : (optional)
+   see Documentation/devicetree/bindings/leds/common.txt
+
+Examples:
+
+fancy_leds: is31fl3196@65 {


I believe the current preference would be to name the node by
its function, rather than the device. For example:
fancy_leds: leds@65


+   compatible = "issi,is31fl319x";


I believe this should be "issi,is31fl3196" here.


+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <0x65>;
+
+   led0: red-aux@0 {
+   label = "red:aux";
+   reg = <0>;
+   };
+
+   led1: green-power@4 {
+   label = "green:power";
+   reg = <4>;
+   linux,default-trigger = "default-on";
+   };
+};
+
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 2251478..f099bcb 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -491,6 +491,14 @@ config LEDS_ASIC3
  cannot be used. This driver supports hardware blinking with an on+off
  period from 62ms to 125s. Say Y to enable LEDs on the HP iPAQ hx4700.

+config LEDS_IS31FL319X
+   

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[Jacek Anaszewski]

Hi David,

Thanks for the review.
Nikolaus - please address David's remarks.

Thanks,
Jacek Anaszewski

On 04/19/2016 03:25 AM, David Rivshin (Allworx) wrote:

Hi Nikolaus,

I recently submitted a driver for the IS31FL32xx family of devices, so
this driver caught my eye. I have a few comments below.

On Mon, 18 Apr 2016 20:43:16 +0200
"H. Nikolaus Schaller"  wrote:


This is a driver for the Integrated Silicon Solution Inc. LED driver
chips IS31FL3196 and IS31FL3199. They can drive up to 6 or 9
LEDs.

Each LED is individually controllable in brightness (through pwm)
in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.

The maximum current of the LEDs can be programmed and limited to
5 .. 40mA through a device tree property.

The chip is connected through I2C and can have one of 4 addresses (0x64 .. 0x67)
depending on how the AD pin is connected. The address is defined by the
reg property as usual.

The chip also has a shutdown input which could be connected to a GPIO,
but this driver uses software shutdown if all LEDs are inactivated.

The chip also has breathing and audio features which are not supported
by this driver.

This driver was developed and tested on OMAP5 based Pyra handheld prototype.

Signed-off-by: H. Nikolaus Schaller 
---
  .../devicetree/bindings/leds/is31fl319x.txt|  41 +++
  drivers/leds/Kconfig   |   8 +
  drivers/leds/Makefile  |   1 +
  drivers/leds/leds-is31fl319x.c | 406 +
  include/linux/leds-is31fl319x.h|  24 ++
  5 files changed, 480 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
  create mode 100644 drivers/leds/leds-is31fl319x.c
  create mode 100644 include/linux/leds-is31fl319x.h

diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt 
b/Documentation/devicetree/bindings/leds/is31fl319x.txt
new file mode 100644
index 000..d13c7ca
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
@@ -0,0 +1,41 @@
+LEDs connected to is31fl319x RGB LED controller chip
+
+Required properties:
+- compatible : should be : "issi,is31fl3196" or "issi,is31fl3199".



From a quick look at the datasheets, I believe the Si-En SN3199 [1]

is the same as the ISSI IS31FL3199. ISSI purchased Si-En in 2011,
and looks to have rebranded Si-En's existing LED controllers.
Assuming the SN3199 is indeed the same as the IS31FL3199, I would
suggest adding a compatible string of "si-en,sn3199" both here
and in the driver itself (and update the Kconfig text as well).

[1] http://www.si-en.com/product.asp?parentid=890


+- #address-cells: must be 1
+- #size-cells: must be 0
+- reg: 0x64, 0x65, 0x66, 0x67.
+
+Optional properties:
+- max-current-ma:  maximum led current (5..40 mA, default 20 mA)
+- audio-gain-db:   audio gain selection (0..21 dB. default 0 dB)
+- breathing & audio:   not implemented


I'm not certain, but that last line feels wrong to me. I would expect
to see just defined properties in this section, while that is more
commentary. I would just leave it out, myself.


+
+Each led is represented as a sub-node of the issi,is31fl319x device.
+
+LED sub-node properties:
+- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
+- reg : number of LED line (0 to 5 or 0 to 8 - not all need to be present)


Since the datasheets for these devices name the outputs "OUT1" through
"OUT6"/"OUT9", I believe the correct range for the 'reg' property should
be 1-6 and 1-9. At least that was the result of a discussion with Rob
Herring [2], and therefore how the IS31FL32xx binding/driver was done.
It also makes devicetrees easier to write relative to schematics which
likely use the OUT1-N naming.

[2] http://www.spinics.net/lists/devicetree/msg116019.html



+- linux,default-trigger : (optional)
+   see Documentation/devicetree/bindings/leds/common.txt
+
+Examples:
+
+fancy_leds: is31fl3196@65 {


I believe the current preference would be to name the node by
its function, rather than the device. For example:
fancy_leds: leds@65


+   compatible = "issi,is31fl319x";


I believe this should be "issi,is31fl3196" here.


+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <0x65>;
+
+   led0: red-aux@0 {
+   label = "red:aux";
+   reg = <0>;
+   };
+
+   led1: green-power@4 {
+   label = "green:power";
+   reg = <4>;
+   linux,default-trigger = "default-on";
+   };
+};
+
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 2251478..f099bcb 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -491,6 +491,14 @@ config LEDS_ASIC3
  cannot be used. This driver supports hardware blinking with an on+off
  period from 62ms to 125s. Say Y to enable LEDs on the HP iPAQ hx4700.

+config LEDS_IS31FL319X
+   tristate "LED Support for IS31FL319x I2C 

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[Jacek Anaszewski]

Hi Nikolaus,

Thanks for the patch. Please refer to my remarks in the code.

On 04/18/2016 08:43 PM, H. Nikolaus Schaller wrote:

This is a driver for the Integrated Silicon Solution Inc. LED driver
chips IS31FL3196 and IS31FL3199. They can drive up to 6 or 9
LEDs.

Each LED is individually controllable in brightness (through pwm)
in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.

The maximum current of the LEDs can be programmed and limited to
5 .. 40mA through a device tree property.

The chip is connected through I2C and can have one of 4 addresses (0x64 .. 0x67)
depending on how the AD pin is connected. The address is defined by the
reg property as usual.

The chip also has a shutdown input which could be connected to a GPIO,
but this driver uses software shutdown if all LEDs are inactivated.

The chip also has breathing and audio features which are not supported
by this driver.

This driver was developed and tested on OMAP5 based Pyra handheld prototype.

Signed-off-by: H. Nikolaus Schaller 
---
  .../devicetree/bindings/leds/is31fl319x.txt|  41 +++
  drivers/leds/Kconfig   |   8 +
  drivers/leds/Makefile  |   1 +
  drivers/leds/leds-is31fl319x.c | 406 +
  include/linux/leds-is31fl319x.h|  24 ++
  5 files changed, 480 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
  create mode 100644 drivers/leds/leds-is31fl319x.c
  create mode 100644 include/linux/leds-is31fl319x.h

diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt 
b/Documentation/devicetree/bindings/leds/is31fl319x.txt
new file mode 100644
index 000..d13c7ca
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
@@ -0,0 +1,41 @@
+LEDs connected to is31fl319x RGB LED controller chip
+
+Required properties:
+- compatible : should be : "issi,is31fl3196" or "issi,is31fl3199".


s/should be :/Should be/


+- #address-cells: must be 1
+- #size-cells: must be 0


s/must/Must/

Please begin the property description with a capital letter,
and also end it with a dot.
This remark applies also to the other properties.


+- reg: 0x64, 0x65, 0x66, 0x67.
+
+Optional properties:
+- max-current-ma:  maximum led current (5..40 mA, default 20 mA)


There is leds-max-microamp property for this -
see Documentation/devicetree/bindings/leds/common.txt.
I'd rephrase this as follows:

- led-max-microamp: See Documentation/devicetree/bindings/leds/common.txt.
Valid values: 5 - 40, step by (?) (rounded {up|down})
Default: 2


+- audio-gain-db:   audio gain selection (0..21 dB. default 0 dB)


Please follow the aforementioned "Valid values" pattern.
It would also be nice to have more informative description on the
purpose of this property.


+- breathing & audio:   not implemented


There is no point in documenting unused properties.


+
+Each led is represented as a sub-node of the issi,is31fl319x device.
+
+LED sub-node properties:
+- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
+- reg : number of LED line (0 to 5 or 0 to 8 - not all need to be present)
+- linux,default-trigger : (optional)
+   see Documentation/devicetree/bindings/leds/common.txt
+
+Examples:
+
+fancy_leds: is31fl3196@65 {
+   compatible = "issi,is31fl319x";
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <0x65>;
+
+   led0: red-aux@0 {
+   label = "red:aux";
+   reg = <0>;
+   };
+
+   led1: green-power@4 {
+   label = "green:power";
+   reg = <4>;
+   linux,default-trigger = "default-on";
+   };
+};


Generally I prefer to have DT bindings documentation in a separate
patch.


diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 2251478..f099bcb 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -491,6 +491,14 @@ config LEDS_ASIC3
  cannot be used. This driver supports hardware blinking with an on+off
  period from 62ms to 125s. Say Y to enable LEDs on the HP iPAQ hx4700.

+config LEDS_IS31FL319X
+   tristate "LED Support for IS31FL319x I2C chip"
+   depends on LEDS_CLASS && I2C
+   help
+ This option enables support for LEDs connected to IS31FL3196
+ or IS31FL3199 LED driver chips accessed via the I2C bus.
+ Driver support brightness control and hardware-assisted blinking.
+
  config LEDS_TCA6507
tristate "LED Support for TCA6507 I2C chip"
depends on LEDS_CLASS && I2C
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index cb2013d..eee3010 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_LEDS_TCA6507)+= leds-tca6507.o
  obj-$(CONFIG_LEDS_TLC591XX)   += leds-tlc591xx.o
  obj-$(CONFIG_LEDS_CLEVO_MAIL) += leds-clevo-mail.o
  

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[Jacek Anaszewski]

Hi Nikolaus,

Thanks for the patch. Please refer to my remarks in the code.

On 04/18/2016 08:43 PM, H. Nikolaus Schaller wrote:

This is a driver for the Integrated Silicon Solution Inc. LED driver
chips IS31FL3196 and IS31FL3199. They can drive up to 6 or 9
LEDs.

Each LED is individually controllable in brightness (through pwm)
in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.

The maximum current of the LEDs can be programmed and limited to
5 .. 40mA through a device tree property.

The chip is connected through I2C and can have one of 4 addresses (0x64 .. 0x67)
depending on how the AD pin is connected. The address is defined by the
reg property as usual.

The chip also has a shutdown input which could be connected to a GPIO,
but this driver uses software shutdown if all LEDs are inactivated.

The chip also has breathing and audio features which are not supported
by this driver.

This driver was developed and tested on OMAP5 based Pyra handheld prototype.

Signed-off-by: H. Nikolaus Schaller 
---
  .../devicetree/bindings/leds/is31fl319x.txt|  41 +++
  drivers/leds/Kconfig   |   8 +
  drivers/leds/Makefile  |   1 +
  drivers/leds/leds-is31fl319x.c | 406 +
  include/linux/leds-is31fl319x.h|  24 ++
  5 files changed, 480 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
  create mode 100644 drivers/leds/leds-is31fl319x.c
  create mode 100644 include/linux/leds-is31fl319x.h

diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt 
b/Documentation/devicetree/bindings/leds/is31fl319x.txt
new file mode 100644
index 000..d13c7ca
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
@@ -0,0 +1,41 @@
+LEDs connected to is31fl319x RGB LED controller chip
+
+Required properties:
+- compatible : should be : "issi,is31fl3196" or "issi,is31fl3199".


s/should be :/Should be/


+- #address-cells: must be 1
+- #size-cells: must be 0


s/must/Must/

Please begin the property description with a capital letter,
and also end it with a dot.
This remark applies also to the other properties.


+- reg: 0x64, 0x65, 0x66, 0x67.
+
+Optional properties:
+- max-current-ma:  maximum led current (5..40 mA, default 20 mA)


There is leds-max-microamp property for this -
see Documentation/devicetree/bindings/leds/common.txt.
I'd rephrase this as follows:

- led-max-microamp: See Documentation/devicetree/bindings/leds/common.txt.
Valid values: 5 - 40, step by (?) (rounded {up|down})
Default: 2


+- audio-gain-db:   audio gain selection (0..21 dB. default 0 dB)


Please follow the aforementioned "Valid values" pattern.
It would also be nice to have more informative description on the
purpose of this property.


+- breathing & audio:   not implemented


There is no point in documenting unused properties.


+
+Each led is represented as a sub-node of the issi,is31fl319x device.
+
+LED sub-node properties:
+- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
+- reg : number of LED line (0 to 5 or 0 to 8 - not all need to be present)
+- linux,default-trigger : (optional)
+   see Documentation/devicetree/bindings/leds/common.txt
+
+Examples:
+
+fancy_leds: is31fl3196@65 {
+   compatible = "issi,is31fl319x";
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <0x65>;
+
+   led0: red-aux@0 {
+   label = "red:aux";
+   reg = <0>;
+   };
+
+   led1: green-power@4 {
+   label = "green:power";
+   reg = <4>;
+   linux,default-trigger = "default-on";
+   };
+};


Generally I prefer to have DT bindings documentation in a separate
patch.


diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 2251478..f099bcb 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -491,6 +491,14 @@ config LEDS_ASIC3
  cannot be used. This driver supports hardware blinking with an on+off
  period from 62ms to 125s. Say Y to enable LEDs on the HP iPAQ hx4700.

+config LEDS_IS31FL319X
+   tristate "LED Support for IS31FL319x I2C chip"
+   depends on LEDS_CLASS && I2C
+   help
+ This option enables support for LEDs connected to IS31FL3196
+ or IS31FL3199 LED driver chips accessed via the I2C bus.
+ Driver support brightness control and hardware-assisted blinking.
+
  config LEDS_TCA6507
tristate "LED Support for TCA6507 I2C chip"
depends on LEDS_CLASS && I2C
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index cb2013d..eee3010 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_LEDS_TCA6507)+= leds-tca6507.o
  obj-$(CONFIG_LEDS_TLC591XX)   += leds-tlc591xx.o
  obj-$(CONFIG_LEDS_CLEVO_MAIL) += leds-clevo-mail.o
  

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[David Rivshin (Allworx)]

Hi Nikolaus,

I recently submitted a driver for the IS31FL32xx family of devices, so
this driver caught my eye. I have a few comments below.

On Mon, 18 Apr 2016 20:43:16 +0200
"H. Nikolaus Schaller"  wrote:

> This is a driver for the Integrated Silicon Solution Inc. LED driver
> chips IS31FL3196 and IS31FL3199. They can drive up to 6 or 9
> LEDs.
> 
> Each LED is individually controllable in brightness (through pwm)
> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
> 
> The maximum current of the LEDs can be programmed and limited to
> 5 .. 40mA through a device tree property.
> 
> The chip is connected through I2C and can have one of 4 addresses (0x64 .. 
> 0x67)
> depending on how the AD pin is connected. The address is defined by the
> reg property as usual.
> 
> The chip also has a shutdown input which could be connected to a GPIO,
> but this driver uses software shutdown if all LEDs are inactivated.
> 
> The chip also has breathing and audio features which are not supported
> by this driver.
> 
> This driver was developed and tested on OMAP5 based Pyra handheld prototype.
> 
> Signed-off-by: H. Nikolaus Schaller 
> ---
>  .../devicetree/bindings/leds/is31fl319x.txt|  41 +++
>  drivers/leds/Kconfig   |   8 +
>  drivers/leds/Makefile  |   1 +
>  drivers/leds/leds-is31fl319x.c | 406 
> +
>  include/linux/leds-is31fl319x.h|  24 ++
>  5 files changed, 480 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
>  create mode 100644 drivers/leds/leds-is31fl319x.c
>  create mode 100644 include/linux/leds-is31fl319x.h
> 
> diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt 
> b/Documentation/devicetree/bindings/leds/is31fl319x.txt
> new file mode 100644
> index 000..d13c7ca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
> @@ -0,0 +1,41 @@
> +LEDs connected to is31fl319x RGB LED controller chip
> +
> +Required properties:
> +- compatible : should be : "issi,is31fl3196" or "issi,is31fl3199".

>From a quick look at the datasheets, I believe the Si-En SN3199 [1] 
is the same as the ISSI IS31FL3199. ISSI purchased Si-En in 2011,
and looks to have rebranded Si-En's existing LED controllers. 
Assuming the SN3199 is indeed the same as the IS31FL3199, I would
suggest adding a compatible string of "si-en,sn3199" both here
and in the driver itself (and update the Kconfig text as well). 

[1] http://www.si-en.com/product.asp?parentid=890

> +- #address-cells: must be 1
> +- #size-cells: must be 0
> +- reg: 0x64, 0x65, 0x66, 0x67.
> +
> +Optional properties:
> +- max-current-ma:maximum led current (5..40 mA, default 20 mA)
> +- audio-gain-db: audio gain selection (0..21 dB. default 0 dB)
> +- breathing & audio: not implemented

I'm not certain, but that last line feels wrong to me. I would expect
to see just defined properties in this section, while that is more
commentary. I would just leave it out, myself.

> +
> +Each led is represented as a sub-node of the issi,is31fl319x device.
> +
> +LED sub-node properties:
> +- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
> +- reg : number of LED line (0 to 5 or 0 to 8 - not all need to be present)

Since the datasheets for these devices name the outputs "OUT1" through 
"OUT6"/"OUT9", I believe the correct range for the 'reg' property should 
be 1-6 and 1-9. At least that was the result of a discussion with Rob
Herring [2], and therefore how the IS31FL32xx binding/driver was done.
It also makes devicetrees easier to write relative to schematics which 
likely use the OUT1-N naming.

[2] http://www.spinics.net/lists/devicetree/msg116019.html


> +- linux,default-trigger : (optional)
> +   see Documentation/devicetree/bindings/leds/common.txt
> +
> +Examples:
> +
> +fancy_leds: is31fl3196@65 {

I believe the current preference would be to name the node by
its function, rather than the device. For example:
fancy_leds: leds@65

> + compatible = "issi,is31fl319x";

I believe this should be "issi,is31fl3196" here.

> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x65>;
> +
> + led0: red-aux@0 {
> + label = "red:aux";
> + reg = <0>;
> + };
> +
> + led1: green-power@4 {
> + label = "green:power";
> + reg = <4>;
> + linux,default-trigger = "default-on";
> + };
> +};
> +
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 2251478..f099bcb 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -491,6 +491,14 @@ config LEDS_ASIC3
> cannot be used. This driver supports hardware blinking with an on+off
> period from 62ms to 125s. Say Y to enable LEDs on the HP iPAQ hx4700.
>  
> +config LEDS_IS31FL319X
> + tristate "LED Support for 

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[David Rivshin (Allworx)]

Hi Nikolaus,

I recently submitted a driver for the IS31FL32xx family of devices, so
this driver caught my eye. I have a few comments below.

On Mon, 18 Apr 2016 20:43:16 +0200
"H. Nikolaus Schaller"  wrote:

> This is a driver for the Integrated Silicon Solution Inc. LED driver
> chips IS31FL3196 and IS31FL3199. They can drive up to 6 or 9
> LEDs.
> 
> Each LED is individually controllable in brightness (through pwm)
> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
> 
> The maximum current of the LEDs can be programmed and limited to
> 5 .. 40mA through a device tree property.
> 
> The chip is connected through I2C and can have one of 4 addresses (0x64 .. 
> 0x67)
> depending on how the AD pin is connected. The address is defined by the
> reg property as usual.
> 
> The chip also has a shutdown input which could be connected to a GPIO,
> but this driver uses software shutdown if all LEDs are inactivated.
> 
> The chip also has breathing and audio features which are not supported
> by this driver.
> 
> This driver was developed and tested on OMAP5 based Pyra handheld prototype.
> 
> Signed-off-by: H. Nikolaus Schaller 
> ---
>  .../devicetree/bindings/leds/is31fl319x.txt|  41 +++
>  drivers/leds/Kconfig   |   8 +
>  drivers/leds/Makefile  |   1 +
>  drivers/leds/leds-is31fl319x.c | 406 
> +
>  include/linux/leds-is31fl319x.h|  24 ++
>  5 files changed, 480 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/is31fl319x.txt
>  create mode 100644 drivers/leds/leds-is31fl319x.c
>  create mode 100644 include/linux/leds-is31fl319x.h
> 
> diff --git a/Documentation/devicetree/bindings/leds/is31fl319x.txt 
> b/Documentation/devicetree/bindings/leds/is31fl319x.txt
> new file mode 100644
> index 000..d13c7ca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/is31fl319x.txt
> @@ -0,0 +1,41 @@
> +LEDs connected to is31fl319x RGB LED controller chip
> +
> +Required properties:
> +- compatible : should be : "issi,is31fl3196" or "issi,is31fl3199".

>From a quick look at the datasheets, I believe the Si-En SN3199 [1] 
is the same as the ISSI IS31FL3199. ISSI purchased Si-En in 2011,
and looks to have rebranded Si-En's existing LED controllers. 
Assuming the SN3199 is indeed the same as the IS31FL3199, I would
suggest adding a compatible string of "si-en,sn3199" both here
and in the driver itself (and update the Kconfig text as well). 

[1] http://www.si-en.com/product.asp?parentid=890

> +- #address-cells: must be 1
> +- #size-cells: must be 0
> +- reg: 0x64, 0x65, 0x66, 0x67.
> +
> +Optional properties:
> +- max-current-ma:maximum led current (5..40 mA, default 20 mA)
> +- audio-gain-db: audio gain selection (0..21 dB. default 0 dB)
> +- breathing & audio: not implemented

I'm not certain, but that last line feels wrong to me. I would expect
to see just defined properties in this section, while that is more
commentary. I would just leave it out, myself.

> +
> +Each led is represented as a sub-node of the issi,is31fl319x device.
> +
> +LED sub-node properties:
> +- label : (optional) see Documentation/devicetree/bindings/leds/common.txt
> +- reg : number of LED line (0 to 5 or 0 to 8 - not all need to be present)

Since the datasheets for these devices name the outputs "OUT1" through 
"OUT6"/"OUT9", I believe the correct range for the 'reg' property should 
be 1-6 and 1-9. At least that was the result of a discussion with Rob
Herring [2], and therefore how the IS31FL32xx binding/driver was done.
It also makes devicetrees easier to write relative to schematics which 
likely use the OUT1-N naming.

[2] http://www.spinics.net/lists/devicetree/msg116019.html


> +- linux,default-trigger : (optional)
> +   see Documentation/devicetree/bindings/leds/common.txt
> +
> +Examples:
> +
> +fancy_leds: is31fl3196@65 {

I believe the current preference would be to name the node by
its function, rather than the device. For example:
fancy_leds: leds@65

> + compatible = "issi,is31fl319x";

I believe this should be "issi,is31fl3196" here.

> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x65>;
> +
> + led0: red-aux@0 {
> + label = "red:aux";
> + reg = <0>;
> + };
> +
> + led1: green-power@4 {
> + label = "green:power";
> + reg = <4>;
> + linux,default-trigger = "default-on";
> + };
> +};
> +
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 2251478..f099bcb 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -491,6 +491,14 @@ config LEDS_ASIC3
> cannot be used. This driver supports hardware blinking with an on+off
> period from 62ms to 125s. Say Y to enable LEDs on the HP iPAQ hx4700.
>  
> +config LEDS_IS31FL319X
> + tristate "LED Support for IS31FL319x I2C chip"

Pedantic: "chips" 

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[kbuild test robot]

Hi Nikolaus,

[auto build test ERROR on j.anaszewski-leds/for-next]
[also build test ERROR on v4.6-rc4 next-20160418]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/H-Nikolaus-Schaller/drivers-led-is31fl319x-6-9-channel-light-effect-led-driver/20160419-024650
base:   https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds 
for-next
config: i386-randconfig-b0-04190529 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/leds/leds-is31fl319x.c: In function 'is31fl319x_probe':
>> drivers/leds/leds-is31fl319x.c:299:11: error: too many arguments to function 
>> 'is31fl319x_led_dt_init'
  pdata = is31fl319x_led_dt_init(client, (int) id->driver_data);
  ^
   drivers/leds/leds-is31fl319x.c:271:1: note: declared here
is31fl319x_led_dt_init(struct i2c_client *client)
^

vim +/is31fl319x_led_dt_init +299 drivers/leds/leds-is31fl319x.c

   293  NUM_LEDS, (int) id->driver_data);
   294  
   295  if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
   296  return -EIO;
   297  
   298  if (!pdata) {
 > 299  pdata = is31fl319x_led_dt_init(client, (int) 
 > id->driver_data);
   300  if (IS_ERR(pdata)) {
   301  dev_err(>dev, "DT led error %d\n",
   302  (int) PTR_ERR(pdata));

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

[kbuild test robot]

Hi Nikolaus,

[auto build test ERROR on j.anaszewski-leds/for-next]
[also build test ERROR on v4.6-rc4 next-20160418]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/H-Nikolaus-Schaller/drivers-led-is31fl319x-6-9-channel-light-effect-led-driver/20160419-024650
base:   https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds 
for-next
config: i386-randconfig-b0-04190529 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/leds/leds-is31fl319x.c: In function 'is31fl319x_probe':
>> drivers/leds/leds-is31fl319x.c:299:11: error: too many arguments to function 
>> 'is31fl319x_led_dt_init'
  pdata = is31fl319x_led_dt_init(client, (int) id->driver_data);
  ^
   drivers/leds/leds-is31fl319x.c:271:1: note: declared here
is31fl319x_led_dt_init(struct i2c_client *client)
^

vim +/is31fl319x_led_dt_init +299 drivers/leds/leds-is31fl319x.c

   293  NUM_LEDS, (int) id->driver_data);
   294  
   295  if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
   296  return -EIO;
   297  
   298  if (!pdata) {
 > 299  pdata = is31fl319x_led_dt_init(client, (int) 
 > id->driver_data);
   300  if (IS_ERR(pdata)) {
   301  dev_err(>dev, "DT led error %d\n",
   302  (int) PTR_ERR(pdata));

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data