* [PATCH v3 0/2] leds: Add driver for NCP5623 3-channel I2C LED driver @ 2016-09-16 11:34 ` Florian Vaussard 0 siblings, 0 replies; 24+ messages in thread From: Florian Vaussard @ 2016-09-16 11:34 UTC (permalink / raw) To: devicetree-u79uwXL29TY76Z2rM5mHXA, Richard Purdie, Jacek Anaszewski Cc: Rob Herring, Mark Rutland, Pavel Machek, linux-leds-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Florian Vaussard Hello, This series add a new driver for On Semiconductor NCP5623, a 3-channel I2C LED driver. It is used in our design to drive a RGB LED. The first patch introduces the device tree binding, while the second patch adds the driver itself. Best regards, Florian --- v2 -> v3: - Rebased on latest leds/for-next - Minor fixes to the binding documentation - Removed ncp5623_set_pwm() by inlining it directly inside the caller - Removed ncp5623_destroy_devices() as we are already using devm_ flavours - Got rid of the 'active' state variable by using 'led_no' instead - Some other cosmetic fixes v1 -> v2: - Adapted the DT binding (led-max-microamp for each LED node) - Removed underscores from node names in the example - Use brightness_set_blocking to avoid workqueue - Introduced LED_to_CMD macro to avoid switch statement - Various other fixes Florian Vaussard (2): leds: ncp5623: Add device tree binding documentation leds: Add driver for NCP5623 3-channel I2C LED driver .../devicetree/bindings/leds/leds-ncp5623.txt | 60 ++++++ drivers/leds/Kconfig | 11 + drivers/leds/Makefile | 1 + drivers/leds/leds-ncp5623.c | 234 +++++++++++++++++++++ 4 files changed, 306 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-ncp5623.txt create mode 100644 drivers/leds/leds-ncp5623.c -- 2.5.5 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 0/2] leds: Add driver for NCP5623 3-channel I2C LED driver @ 2016-09-16 11:34 ` Florian Vaussard 0 siblings, 0 replies; 24+ messages in thread From: Florian Vaussard @ 2016-09-16 11:34 UTC (permalink / raw) To: devicetree, Richard Purdie, Jacek Anaszewski Cc: Rob Herring, Mark Rutland, Pavel Machek, linux-leds, linux-kernel, Florian Vaussard Hello, This series add a new driver for On Semiconductor NCP5623, a 3-channel I2C LED driver. It is used in our design to drive a RGB LED. The first patch introduces the device tree binding, while the second patch adds the driver itself. Best regards, Florian --- v2 -> v3: - Rebased on latest leds/for-next - Minor fixes to the binding documentation - Removed ncp5623_set_pwm() by inlining it directly inside the caller - Removed ncp5623_destroy_devices() as we are already using devm_ flavours - Got rid of the 'active' state variable by using 'led_no' instead - Some other cosmetic fixes v1 -> v2: - Adapted the DT binding (led-max-microamp for each LED node) - Removed underscores from node names in the example - Use brightness_set_blocking to avoid workqueue - Introduced LED_to_CMD macro to avoid switch statement - Various other fixes Florian Vaussard (2): leds: ncp5623: Add device tree binding documentation leds: Add driver for NCP5623 3-channel I2C LED driver .../devicetree/bindings/leds/leds-ncp5623.txt | 60 ++++++ drivers/leds/Kconfig | 11 + drivers/leds/Makefile | 1 + drivers/leds/leds-ncp5623.c | 234 +++++++++++++++++++++ 4 files changed, 306 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-ncp5623.txt create mode 100644 drivers/leds/leds-ncp5623.c -- 2.5.5 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 1/2] leds: ncp5623: Add device tree binding documentation 2016-09-16 11:34 ` Florian Vaussard (?) @ 2016-09-16 11:34 ` Florian Vaussard [not found] ` <1474025672-5040-2-git-send-email-florian.vaussard-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org> -1 siblings, 1 reply; 24+ messages in thread From: Florian Vaussard @ 2016-09-16 11:34 UTC (permalink / raw) To: devicetree, Richard Purdie, Jacek Anaszewski Cc: Rob Herring, Mark Rutland, Pavel Machek, linux-leds, linux-kernel, Florian Vaussard Add device tree binding documentation for On Semiconductor NCP5623 I2C LED driver. The driver can independently control the PWM of the 3 channels with 32 levels of intensity. The current delivered by the current source can also be controlled. To do so, the led-max-microamp property is used by each LED sub-node. The maximum value is then found and used as a limit to compute the final intensity of the current source. If a LED happens to have a lower limit, the PWM is then used to limit the current to the requested value. In order to control the current source, it is also necessary to know the current on the Iref pin, hence the onnn,led-iref-microamp property. It is usually set using an external bias resistor, following Iref = Vref/Rbias with Vref=0.6V. Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch> --- .../devicetree/bindings/leds/leds-ncp5623.txt | 60 ++++++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-ncp5623.txt diff --git a/Documentation/devicetree/bindings/leds/leds-ncp5623.txt b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt new file mode 100644 index 0000000..d83e509 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt @@ -0,0 +1,60 @@ +* ON Semiconductor - NCP5623 3-Channel LED Driver + +The NCP5623 is a 3-channel I2C LED driver. The brightness of each +channel can be independently set using 32 levels. Each LED is represented +as a sub-node of the device. + +Required properties: + - compatible: Should be "onnn,ncp5623" + - reg: I2C slave address (fixed to 0x38) + - #address-cells: must be 1 + - #size-cells: must be 0 + - onnn,led-iref-microamp: Current on the Iref pin in microampere. It depends + on the value of the external bias resistor Rbias, following + Iref = Vref / Rbias with Vref = 0.6V. This is used to set the intensity of + the current that can be provided by the internal current source, based on + the maximum current permitted by LED sub-nodes (see below), but no more than + Imax = 2400 * Iref. + +LED sub-nodes +============= + +Required properties: + - reg : LED channel number (0..2) + - led-max-microamp: Maximum allowable current inside the LED in microampere. + This property is used to limit the PWM ratio, based on the intensity of the + internal current source (see above). + +Optional properties: + - label: see Documentation/devicetree/bindings/leds/common.txt + - linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt + +Example +======= + +led1: ncp5623@38 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "onnn,ncp5623"; + reg = <0x38>; + onnn,led-iref-microamp = <10>; + + led1r@0 { + label = "ncp:power:red"; + linux,default-trigger = "default-on"; + reg = <0>; + led-max-microamp = <20000>; + }; + + led1b@1 { + label = "ncp:power:blue"; + reg = <1>; + led-max-microamp = <20000>; + }; + + led1g@2 { + label = "ncp:power:green"; + reg = <2>; + led-max-microamp = <20000>; + }; +}; -- 2.5.5 ^ permalink raw reply related [flat|nested] 24+ messages in thread
[parent not found: <1474025672-5040-2-git-send-email-florian.vaussard-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org>]
* Re: [PATCH v3 1/2] leds: ncp5623: Add device tree binding documentation 2016-09-16 11:34 ` [PATCH v3 1/2] leds: ncp5623: Add device tree binding documentation Florian Vaussard @ 2016-09-23 17:29 ` Rob Herring 0 siblings, 0 replies; 24+ messages in thread From: Rob Herring @ 2016-09-23 17:29 UTC (permalink / raw) To: Florian Vaussard Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Richard Purdie, Jacek Anaszewski, Mark Rutland, Pavel Machek, linux-leds-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Florian Vaussard On Fri, Sep 16, 2016 at 01:34:31PM +0200, Florian Vaussard wrote: > Add device tree binding documentation for On Semiconductor NCP5623 I2C > LED driver. The driver can independently control the PWM of the 3 > channels with 32 levels of intensity. > > The current delivered by the current source can also be controlled. To > do so, the led-max-microamp property is used by each LED sub-node. The > maximum value is then found and used as a limit to compute the final > intensity of the current source. If a LED happens to have a lower limit, > the PWM is then used to limit the current to the requested value. > > In order to control the current source, it is also necessary to know > the current on the Iref pin, hence the onnn,led-iref-microamp property. > It is usually set using an external bias resistor, following > Iref = Vref/Rbias with Vref=0.6V. > > Signed-off-by: Florian Vaussard <florian.vaussard-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org> > --- > .../devicetree/bindings/leds/leds-ncp5623.txt | 60 ++++++++++++++++++++++ > 1 file changed, 60 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/leds-ncp5623.txt Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] leds: ncp5623: Add device tree binding documentation @ 2016-09-23 17:29 ` Rob Herring 0 siblings, 0 replies; 24+ messages in thread From: Rob Herring @ 2016-09-23 17:29 UTC (permalink / raw) To: Florian Vaussard Cc: devicetree, Richard Purdie, Jacek Anaszewski, Mark Rutland, Pavel Machek, linux-leds, linux-kernel, Florian Vaussard On Fri, Sep 16, 2016 at 01:34:31PM +0200, Florian Vaussard wrote: > Add device tree binding documentation for On Semiconductor NCP5623 I2C > LED driver. The driver can independently control the PWM of the 3 > channels with 32 levels of intensity. > > The current delivered by the current source can also be controlled. To > do so, the led-max-microamp property is used by each LED sub-node. The > maximum value is then found and used as a limit to compute the final > intensity of the current source. If a LED happens to have a lower limit, > the PWM is then used to limit the current to the requested value. > > In order to control the current source, it is also necessary to know > the current on the Iref pin, hence the onnn,led-iref-microamp property. > It is usually set using an external bias resistor, following > Iref = Vref/Rbias with Vref=0.6V. > > Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch> > --- > .../devicetree/bindings/leds/leds-ncp5623.txt | 60 ++++++++++++++++++++++ > 1 file changed, 60 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/leds-ncp5623.txt Acked-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] leds: ncp5623: Add device tree binding documentation 2016-09-16 11:34 ` [PATCH v3 1/2] leds: ncp5623: Add device tree binding documentation Florian Vaussard @ 2016-09-24 11:58 ` Pavel Machek 0 siblings, 0 replies; 24+ messages in thread From: Pavel Machek @ 2016-09-24 11:58 UTC (permalink / raw) To: Florian Vaussard Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Richard Purdie, Jacek Anaszewski, Rob Herring, Mark Rutland, linux-leds-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Florian Vaussard [-- Attachment #1: Type: text/plain, Size: 801 bytes --] Hi! > +Example > +======= > + > +led1: ncp5623@38 { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "onnn,ncp5623"; > + reg = <0x38>; > + onnn,led-iref-microamp = <10>; > + > + led1r@0 { > + label = "ncp:power:red"; > + linux,default-trigger = "default-on"; ... > + led1b@1 { > + label = "ncp:power:blue"; > + reg = <1>; Actually... the three LEDs are packaged such as this is one colorful light to the user, right? Some day we'll need to group them, so that kernel can automatically tell this is one led, and probably add extra attributes, such as values that produce white light. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] leds: ncp5623: Add device tree binding documentation @ 2016-09-24 11:58 ` Pavel Machek 0 siblings, 0 replies; 24+ messages in thread From: Pavel Machek @ 2016-09-24 11:58 UTC (permalink / raw) To: Florian Vaussard Cc: devicetree, Richard Purdie, Jacek Anaszewski, Rob Herring, Mark Rutland, linux-leds, linux-kernel, Florian Vaussard [-- Attachment #1: Type: text/plain, Size: 801 bytes --] Hi! > +Example > +======= > + > +led1: ncp5623@38 { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "onnn,ncp5623"; > + reg = <0x38>; > + onnn,led-iref-microamp = <10>; > + > + led1r@0 { > + label = "ncp:power:red"; > + linux,default-trigger = "default-on"; ... > + led1b@1 { > + label = "ncp:power:blue"; > + reg = <1>; Actually... the three LEDs are packaged such as this is one colorful light to the user, right? Some day we'll need to group them, so that kernel can automatically tell this is one led, and probably add extra attributes, such as values that produce white light. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] leds: ncp5623: Add device tree binding documentation 2016-09-24 11:58 ` Pavel Machek (?) @ 2016-09-24 19:06 ` Jacek Anaszewski 2016-09-28 10:04 ` Florian Vaussard -1 siblings, 1 reply; 24+ messages in thread From: Jacek Anaszewski @ 2016-09-24 19:06 UTC (permalink / raw) To: Pavel Machek, Florian Vaussard Cc: devicetree, Richard Purdie, Jacek Anaszewski, Rob Herring, Mark Rutland, linux-leds, linux-kernel, Florian Vaussard On 09/24/2016 01:58 PM, Pavel Machek wrote: > Hi! > >> +Example >> +======= >> + >> +led1: ncp5623@38 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + compatible = "onnn,ncp5623"; >> + reg = <0x38>; >> + onnn,led-iref-microamp = <10>; >> + >> + led1r@0 { >> + label = "ncp:power:red"; >> + linux,default-trigger = "default-on"; > ... >> + led1b@1 { >> + label = "ncp:power:blue"; >> + reg = <1>; > > Actually... the three LEDs are packaged such as this is one colorful > light to the user, right? Some day we'll need to group them, so that > kernel can automatically tell this is one led, and probably add extra > attributes, such as values that produce white light. We could try out the trigger approach we discussed few months ago. Unfortunately I currently don't have enough time to propose the implementation. Probably this work could be done on the occasion of addition of RGB LED class driver like this, if the author had free bandwidth for that. -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] leds: ncp5623: Add device tree binding documentation 2016-09-24 19:06 ` Jacek Anaszewski @ 2016-09-28 10:04 ` Florian Vaussard 0 siblings, 0 replies; 24+ messages in thread From: Florian Vaussard @ 2016-09-28 10:04 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek Cc: devicetree, Richard Purdie, Jacek Anaszewski, Rob Herring, Mark Rutland, linux-leds, linux-kernel, Florian Vaussard Hello Jacek, Le 24. 09. 16 à 21:06, Jacek Anaszewski a écrit : > On 09/24/2016 01:58 PM, Pavel Machek wrote: >> Hi! >> >>> +Example >>> +======= >>> + >>> +led1: ncp5623@38 { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + compatible = "onnn,ncp5623"; >>> + reg = <0x38>; >>> + onnn,led-iref-microamp = <10>; >>> + >>> + led1r@0 { >>> + label = "ncp:power:red"; >>> + linux,default-trigger = "default-on"; >> ... >>> + led1b@1 { >>> + label = "ncp:power:blue"; >>> + reg = <1>; >> >> Actually... the three LEDs are packaged such as this is one colorful >> light to the user, right? Some day we'll need to group them, so that >> kernel can automatically tell this is one led, and probably add extra >> attributes, such as values that produce white light. > > We could try out the trigger approach we discussed few months ago. > Unfortunately I currently don't have enough time to propose the > implementation. Probably this work could be done on the occasion of > addition of RGB LED class driver like this, if the author had free > bandwidth for that. > Unfortunately my bandwidth is pretty well used at the moment :) Best regards, Florian ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] leds: ncp5623: Add device tree binding documentation 2016-09-24 11:58 ` Pavel Machek @ 2016-09-28 10:02 ` Florian Vaussard -1 siblings, 0 replies; 24+ messages in thread From: Florian Vaussard @ 2016-09-28 10:02 UTC (permalink / raw) To: Pavel Machek Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Richard Purdie, Jacek Anaszewski, Rob Herring, Mark Rutland, linux-leds-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Florian Vaussard Hi Pavel, Le 24. 09. 16 à 13:58, Pavel Machek a écrit : > Hi! > >> +Example >> +======= >> + >> +led1: ncp5623@38 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + compatible = "onnn,ncp5623"; >> + reg = <0x38>; >> + onnn,led-iref-microamp = <10>; >> + >> + led1r@0 { >> + label = "ncp:power:red"; >> + linux,default-trigger = "default-on"; > ... >> + led1b@1 { >> + label = "ncp:power:blue"; >> + reg = <1>; > > Actually... the three LEDs are packaged such as this is one colorful > light to the user, right? Some day we'll need to group them, so that > kernel can automatically tell this is one led, and probably add extra > attributes, such as values that produce white light. > Actually, it's up to the hardware designer to choose. On my board for instance, this chip is driving an RGB LED, but it can really drive three independent LEDs if you want. I agree that the RGB case is quite common nowadays and currently not very well managed by the LED subsystem. But I do not think that this is specific to this driver. Best regards, Florian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] leds: ncp5623: Add device tree binding documentation @ 2016-09-28 10:02 ` Florian Vaussard 0 siblings, 0 replies; 24+ messages in thread From: Florian Vaussard @ 2016-09-28 10:02 UTC (permalink / raw) To: Pavel Machek Cc: devicetree, Richard Purdie, Jacek Anaszewski, Rob Herring, Mark Rutland, linux-leds, linux-kernel, Florian Vaussard Hi Pavel, Le 24. 09. 16 à 13:58, Pavel Machek a écrit : > Hi! > >> +Example >> +======= >> + >> +led1: ncp5623@38 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + compatible = "onnn,ncp5623"; >> + reg = <0x38>; >> + onnn,led-iref-microamp = <10>; >> + >> + led1r@0 { >> + label = "ncp:power:red"; >> + linux,default-trigger = "default-on"; > ... >> + led1b@1 { >> + label = "ncp:power:blue"; >> + reg = <1>; > > Actually... the three LEDs are packaged such as this is one colorful > light to the user, right? Some day we'll need to group them, so that > kernel can automatically tell this is one led, and probably add extra > attributes, such as values that produce white light. > Actually, it's up to the hardware designer to choose. On my board for instance, this chip is driving an RGB LED, but it can really drive three independent LEDs if you want. I agree that the RGB case is quite common nowadays and currently not very well managed by the LED subsystem. But I do not think that this is specific to this driver. Best regards, Florian ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] leds: ncp5623: Add device tree binding documentation 2016-09-28 10:02 ` Florian Vaussard (?) @ 2016-09-28 10:58 ` Pavel Machek -1 siblings, 0 replies; 24+ messages in thread From: Pavel Machek @ 2016-09-28 10:58 UTC (permalink / raw) To: Florian Vaussard Cc: devicetree, Richard Purdie, Jacek Anaszewski, Rob Herring, Mark Rutland, linux-leds, linux-kernel, Florian Vaussard [-- Attachment #1: Type: text/plain, Size: 1776 bytes --] On Wed 2016-09-28 12:02:41, Florian Vaussard wrote: > Hi Pavel, > > Le 24. 09. 16 à 13:58, Pavel Machek a écrit : > > Hi! > > > >> +Example > >> +======= > >> + > >> +led1: ncp5623@38 { > >> + #address-cells = <1>; > >> + #size-cells = <0>; > >> + compatible = "onnn,ncp5623"; > >> + reg = <0x38>; > >> + onnn,led-iref-microamp = <10>; > >> + > >> + led1r@0 { > >> + label = "ncp:power:red"; > >> + linux,default-trigger = "default-on"; > > ... > >> + led1b@1 { > >> + label = "ncp:power:blue"; > >> + reg = <1>; > > > > Actually... the three LEDs are packaged such as this is one colorful > > light to the user, right? Some day we'll need to group them, so that > > kernel can automatically tell this is one led, and probably add extra > > attributes, such as values that produce white light. > > > > Actually, it's up to the hardware designer to choose. On my board for instance, > this chip is driving an RGB LED, but it can really drive three independent LEDs > if you want. Yup. And driving RGB LED is really a bit different from driving three independent LEDs: you'd for example like to be able to set the RGB LED to white, and you need to know relative intensities for that. So it would be good to have hardware description that captures difference between RGB LED and three LEDs. (And then, we'll want pattern engine to drive that. One day :-) ). > I agree that the RGB case is quite common nowadays and currently not very well > managed by the LED subsystem. But I do not think that this is specific to this > driver. No, it is not. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <1474025672-5040-1-git-send-email-florian.vaussard-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org>]
* [PATCH v3 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver 2016-09-16 11:34 ` Florian Vaussard @ 2016-09-16 11:34 ` Florian Vaussard -1 siblings, 0 replies; 24+ messages in thread From: Florian Vaussard @ 2016-09-16 11:34 UTC (permalink / raw) To: devicetree-u79uwXL29TY76Z2rM5mHXA, Richard Purdie, Jacek Anaszewski Cc: Rob Herring, Mark Rutland, Pavel Machek, linux-leds-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Florian Vaussard The NCP5623 is a 3-channel LED driver from On Semiconductor controlled through I2C. The PWM of each channel can be independently set with 32 distinct levels. In addition, the intensity of the current source can be globally set using an external bias resistor fixing the reference current (Iref) and a dedicated register (ILED), following the relationship: I = 2400*Iref/(31-ILED) with Iref = Vref/Rbias, and Vref = 0.6V. Signed-off-by: Florian Vaussard <florian.vaussard-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org> --- drivers/leds/Kconfig | 11 +++ drivers/leds/Makefile | 1 + drivers/leds/leds-ncp5623.c | 234 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 246 insertions(+) create mode 100644 drivers/leds/leds-ncp5623.c diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 7a628c6..1eda9bd 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -659,6 +659,17 @@ config LEDS_MLXCPLD This option enabled support for the LEDs on the Mellanox boards. Say Y to enabled these. +config LEDS_NCP5623 + tristate "LED Support for NCP5623 I2C chip" + depends on LEDS_CLASS + depends on I2C + help + This option enables support for LEDs connected to NCP5623 + LED driver chips accessed via the I2C bus. + Driver supports brightness control. + + Say Y to enable this driver. + comment "LED Triggers" source "drivers/leds/trigger/Kconfig" diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index 3965070..dc53dd8 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -71,6 +71,7 @@ obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o obj-$(CONFIG_LEDS_PM8058) += leds-pm8058.o obj-$(CONFIG_LEDS_MLXCPLD) += leds-mlxcpld.o +obj-$(CONFIG_LEDS_NCP5623) += leds-ncp5623.o # LED SPI Drivers obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o diff --git a/drivers/leds/leds-ncp5623.c b/drivers/leds/leds-ncp5623.c new file mode 100644 index 0000000..875785a --- /dev/null +++ b/drivers/leds/leds-ncp5623.c @@ -0,0 +1,234 @@ +/* + * Copyright 2016 Florian Vaussard <florian.vaussard-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org> + * + * Based on leds-tlc591xx.c + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + */ + +#include <linux/bitops.h> +#include <linux/i2c.h> +#include <linux/kernel.h> +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/slab.h> + +#define NCP5623_MAX_LEDS 3 +#define NCP5623_MAX_STEPS 31 +#define NCP5623_MAX_CURRENT 31 +#define NCP5623_MAX_CURRENT_UA 30000 + +#define NCP5623_CMD_SHIFT 5 +#define CMD_SHUTDOWN (0x00 << NCP5623_CMD_SHIFT) +#define CMD_ILED (0x01 << NCP5623_CMD_SHIFT) +#define CMD_PWM1 (0x02 << NCP5623_CMD_SHIFT) +#define CMD_PWM2 (0x03 << NCP5623_CMD_SHIFT) +#define CMD_PWM3 (0x04 << NCP5623_CMD_SHIFT) +#define CMD_UPWARD_DIM (0x05 << NCP5623_CMD_SHIFT) +#define CMD_DOWNWARD_DIM (0x06 << NCP5623_CMD_SHIFT) +#define CMD_DIM_STEP (0x07 << NCP5623_CMD_SHIFT) + +#define LED_TO_PWM_CMD(led) ((0x02 + led) << NCP5623_CMD_SHIFT) + +#define NCP5623_DATA_MASK GENMASK(NCP5623_CMD_SHIFT - 1, 0) +#define NCP5623_CMD(cmd, data) (cmd | (data & NCP5623_DATA_MASK)) + +struct ncp5623_led { + int led_no; + u32 led_max_current; + struct led_classdev ldev; + struct ncp5623_priv *priv; +}; + +struct ncp5623_priv { + struct ncp5623_led leds[NCP5623_MAX_LEDS]; + u32 led_iref; + u32 leds_max_current; + struct i2c_client *client; +}; + +static struct ncp5623_led *ldev_to_led(struct led_classdev *ldev) +{ + return container_of(ldev, struct ncp5623_led, ldev); +} + +static int ncp5623_send_cmd(struct ncp5623_priv *priv, u8 cmd, u8 data) +{ + char cmd_data[1] = { NCP5623_CMD(cmd, data) }; + int err; + + err = i2c_master_send(priv->client, cmd_data, ARRAY_SIZE(cmd_data)); + + return (err < 0 ? err : 0); +} + +static int ncp5623_brightness_set(struct led_classdev *led_cdev, + enum led_brightness brightness) +{ + struct ncp5623_led *led = ldev_to_led(led_cdev); + + return ncp5623_send_cmd(led->priv, LED_TO_PWM_CMD(led->led_no), + brightness); +} + +static int ncp5623_configure(struct device *dev, + struct ncp5623_priv *priv) +{ + unsigned int i; + unsigned int n; + struct ncp5623_led *led; + int effective_current; + int err; + + /* Setup the internal current source, round down */ + n = 2400 * priv->led_iref / priv->leds_max_current + 1; + if (n > NCP5623_MAX_CURRENT) + n = NCP5623_MAX_CURRENT; + + effective_current = 2400 * priv->led_iref / n; + dev_dbg(dev, "setting maximum current to %u uA\n", effective_current); + + err = ncp5623_send_cmd(priv, CMD_ILED, NCP5623_MAX_CURRENT - n); + if (err < 0) { + dev_err(dev, "cannot set the current\n"); + return err; + } + + /* Setup each individual LED */ + for (i = 0; i < NCP5623_MAX_LEDS; i++) { + led = &priv->leds[i]; + + if (led->led_no < 0) + continue; + + led->priv = priv; + led->ldev.brightness_set_blocking = ncp5623_brightness_set; + + led->ldev.max_brightness = led->led_max_current * + NCP5623_MAX_STEPS / effective_current; + if (led->ldev.max_brightness > NCP5623_MAX_STEPS) + led->ldev.max_brightness = NCP5623_MAX_STEPS; + + err = devm_led_classdev_register(dev, &led->ldev); + if (err < 0) { + dev_err(dev, "couldn't register LED %s\n", + led->ldev.name); + return err; + } + } + + return 0; +} + +static int ncp5623_parse_dt(struct ncp5623_priv *priv, struct device_node *np) +{ + struct device_node *child; + struct ncp5623_led *led; + u32 reg; + int count; + int err; + + err = of_property_read_u32(np, "onnn,led-iref-microamp", + &priv->led_iref); + if (err) + return -EINVAL; + + count = of_get_child_count(np); + if (!count || count > NCP5623_MAX_LEDS) + return -EINVAL; + + for_each_child_of_node(np, child) { + err = of_property_read_u32(child, "reg", ®); + if (err) + return err; + + if (reg >= NCP5623_MAX_LEDS) { + err = -EINVAL; + goto dt_child_parse_error; + } + + led = &priv->leds[reg]; + if (led->led_no >= 0) { + /* Already registered */ + err = -EINVAL; + goto dt_child_parse_error; + } + led->led_no = reg; + + err = of_property_read_u32(child, "led-max-microamp", + &led->led_max_current); + if (err || led->led_max_current > NCP5623_MAX_CURRENT_UA) + return -EINVAL; + if (led->led_max_current > priv->leds_max_current) + priv->leds_max_current = led->led_max_current; + + led->ldev.name = + of_get_property(child, "label", NULL) ? : child->name; + led->ldev.default_trigger = + of_get_property(child, "linux,default-trigger", NULL); + } + + return 0; + +dt_child_parse_error: + of_node_put(child); + + return err; +} + +static const struct of_device_id ncp5623_of_match[] = { + { .compatible = "onnn,ncp5623" }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, ncp5623_of_match); + +static int ncp5623_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct device *dev = &client->dev; + struct device_node *np = dev->of_node; + struct ncp5623_priv *priv; + int i, err; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + /* Mark all LEDs as inactive by default */ + for (i = 0; i < NCP5623_MAX_LEDS; i++) + priv->leds[i].led_no = -ENODEV; + + priv->client = client; + i2c_set_clientdata(client, priv); + + err = ncp5623_parse_dt(priv, np); + if (err) + return err; + + return ncp5623_configure(dev, priv); +} + +static const struct i2c_device_id ncp5623_id[] = { + { "ncp5623" }, + {}, +}; +MODULE_DEVICE_TABLE(i2c, ncp5623_id); + +static struct i2c_driver ncp5623_driver = { + .driver = { + .name = "ncp5623", + .of_match_table = of_match_ptr(ncp5623_of_match), + }, + .probe = ncp5623_probe, + .id_table = ncp5623_id, +}; + +module_i2c_driver(ncp5623_driver); + +MODULE_AUTHOR("Florian Vaussard <florian.vaussard-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org>"); +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("NCP5623 LED driver"); -- 2.5.5 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver @ 2016-09-16 11:34 ` Florian Vaussard 0 siblings, 0 replies; 24+ messages in thread From: Florian Vaussard @ 2016-09-16 11:34 UTC (permalink / raw) To: devicetree, Richard Purdie, Jacek Anaszewski Cc: Rob Herring, Mark Rutland, Pavel Machek, linux-leds, linux-kernel, Florian Vaussard The NCP5623 is a 3-channel LED driver from On Semiconductor controlled through I2C. The PWM of each channel can be independently set with 32 distinct levels. In addition, the intensity of the current source can be globally set using an external bias resistor fixing the reference current (Iref) and a dedicated register (ILED), following the relationship: I = 2400*Iref/(31-ILED) with Iref = Vref/Rbias, and Vref = 0.6V. Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch> --- drivers/leds/Kconfig | 11 +++ drivers/leds/Makefile | 1 + drivers/leds/leds-ncp5623.c | 234 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 246 insertions(+) create mode 100644 drivers/leds/leds-ncp5623.c diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 7a628c6..1eda9bd 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -659,6 +659,17 @@ config LEDS_MLXCPLD This option enabled support for the LEDs on the Mellanox boards. Say Y to enabled these. +config LEDS_NCP5623 + tristate "LED Support for NCP5623 I2C chip" + depends on LEDS_CLASS + depends on I2C + help + This option enables support for LEDs connected to NCP5623 + LED driver chips accessed via the I2C bus. + Driver supports brightness control. + + Say Y to enable this driver. + comment "LED Triggers" source "drivers/leds/trigger/Kconfig" diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index 3965070..dc53dd8 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -71,6 +71,7 @@ obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o obj-$(CONFIG_LEDS_PM8058) += leds-pm8058.o obj-$(CONFIG_LEDS_MLXCPLD) += leds-mlxcpld.o +obj-$(CONFIG_LEDS_NCP5623) += leds-ncp5623.o # LED SPI Drivers obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o diff --git a/drivers/leds/leds-ncp5623.c b/drivers/leds/leds-ncp5623.c new file mode 100644 index 0000000..875785a --- /dev/null +++ b/drivers/leds/leds-ncp5623.c @@ -0,0 +1,234 @@ +/* + * Copyright 2016 Florian Vaussard <florian.vaussard@heig-vd.ch> + * + * Based on leds-tlc591xx.c + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + */ + +#include <linux/bitops.h> +#include <linux/i2c.h> +#include <linux/kernel.h> +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/slab.h> + +#define NCP5623_MAX_LEDS 3 +#define NCP5623_MAX_STEPS 31 +#define NCP5623_MAX_CURRENT 31 +#define NCP5623_MAX_CURRENT_UA 30000 + +#define NCP5623_CMD_SHIFT 5 +#define CMD_SHUTDOWN (0x00 << NCP5623_CMD_SHIFT) +#define CMD_ILED (0x01 << NCP5623_CMD_SHIFT) +#define CMD_PWM1 (0x02 << NCP5623_CMD_SHIFT) +#define CMD_PWM2 (0x03 << NCP5623_CMD_SHIFT) +#define CMD_PWM3 (0x04 << NCP5623_CMD_SHIFT) +#define CMD_UPWARD_DIM (0x05 << NCP5623_CMD_SHIFT) +#define CMD_DOWNWARD_DIM (0x06 << NCP5623_CMD_SHIFT) +#define CMD_DIM_STEP (0x07 << NCP5623_CMD_SHIFT) + +#define LED_TO_PWM_CMD(led) ((0x02 + led) << NCP5623_CMD_SHIFT) + +#define NCP5623_DATA_MASK GENMASK(NCP5623_CMD_SHIFT - 1, 0) +#define NCP5623_CMD(cmd, data) (cmd | (data & NCP5623_DATA_MASK)) + +struct ncp5623_led { + int led_no; + u32 led_max_current; + struct led_classdev ldev; + struct ncp5623_priv *priv; +}; + +struct ncp5623_priv { + struct ncp5623_led leds[NCP5623_MAX_LEDS]; + u32 led_iref; + u32 leds_max_current; + struct i2c_client *client; +}; + +static struct ncp5623_led *ldev_to_led(struct led_classdev *ldev) +{ + return container_of(ldev, struct ncp5623_led, ldev); +} + +static int ncp5623_send_cmd(struct ncp5623_priv *priv, u8 cmd, u8 data) +{ + char cmd_data[1] = { NCP5623_CMD(cmd, data) }; + int err; + + err = i2c_master_send(priv->client, cmd_data, ARRAY_SIZE(cmd_data)); + + return (err < 0 ? err : 0); +} + +static int ncp5623_brightness_set(struct led_classdev *led_cdev, + enum led_brightness brightness) +{ + struct ncp5623_led *led = ldev_to_led(led_cdev); + + return ncp5623_send_cmd(led->priv, LED_TO_PWM_CMD(led->led_no), + brightness); +} + +static int ncp5623_configure(struct device *dev, + struct ncp5623_priv *priv) +{ + unsigned int i; + unsigned int n; + struct ncp5623_led *led; + int effective_current; + int err; + + /* Setup the internal current source, round down */ + n = 2400 * priv->led_iref / priv->leds_max_current + 1; + if (n > NCP5623_MAX_CURRENT) + n = NCP5623_MAX_CURRENT; + + effective_current = 2400 * priv->led_iref / n; + dev_dbg(dev, "setting maximum current to %u uA\n", effective_current); + + err = ncp5623_send_cmd(priv, CMD_ILED, NCP5623_MAX_CURRENT - n); + if (err < 0) { + dev_err(dev, "cannot set the current\n"); + return err; + } + + /* Setup each individual LED */ + for (i = 0; i < NCP5623_MAX_LEDS; i++) { + led = &priv->leds[i]; + + if (led->led_no < 0) + continue; + + led->priv = priv; + led->ldev.brightness_set_blocking = ncp5623_brightness_set; + + led->ldev.max_brightness = led->led_max_current * + NCP5623_MAX_STEPS / effective_current; + if (led->ldev.max_brightness > NCP5623_MAX_STEPS) + led->ldev.max_brightness = NCP5623_MAX_STEPS; + + err = devm_led_classdev_register(dev, &led->ldev); + if (err < 0) { + dev_err(dev, "couldn't register LED %s\n", + led->ldev.name); + return err; + } + } + + return 0; +} + +static int ncp5623_parse_dt(struct ncp5623_priv *priv, struct device_node *np) +{ + struct device_node *child; + struct ncp5623_led *led; + u32 reg; + int count; + int err; + + err = of_property_read_u32(np, "onnn,led-iref-microamp", + &priv->led_iref); + if (err) + return -EINVAL; + + count = of_get_child_count(np); + if (!count || count > NCP5623_MAX_LEDS) + return -EINVAL; + + for_each_child_of_node(np, child) { + err = of_property_read_u32(child, "reg", ®); + if (err) + return err; + + if (reg >= NCP5623_MAX_LEDS) { + err = -EINVAL; + goto dt_child_parse_error; + } + + led = &priv->leds[reg]; + if (led->led_no >= 0) { + /* Already registered */ + err = -EINVAL; + goto dt_child_parse_error; + } + led->led_no = reg; + + err = of_property_read_u32(child, "led-max-microamp", + &led->led_max_current); + if (err || led->led_max_current > NCP5623_MAX_CURRENT_UA) + return -EINVAL; + if (led->led_max_current > priv->leds_max_current) + priv->leds_max_current = led->led_max_current; + + led->ldev.name = + of_get_property(child, "label", NULL) ? : child->name; + led->ldev.default_trigger = + of_get_property(child, "linux,default-trigger", NULL); + } + + return 0; + +dt_child_parse_error: + of_node_put(child); + + return err; +} + +static const struct of_device_id ncp5623_of_match[] = { + { .compatible = "onnn,ncp5623" }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, ncp5623_of_match); + +static int ncp5623_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct device *dev = &client->dev; + struct device_node *np = dev->of_node; + struct ncp5623_priv *priv; + int i, err; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + /* Mark all LEDs as inactive by default */ + for (i = 0; i < NCP5623_MAX_LEDS; i++) + priv->leds[i].led_no = -ENODEV; + + priv->client = client; + i2c_set_clientdata(client, priv); + + err = ncp5623_parse_dt(priv, np); + if (err) + return err; + + return ncp5623_configure(dev, priv); +} + +static const struct i2c_device_id ncp5623_id[] = { + { "ncp5623" }, + {}, +}; +MODULE_DEVICE_TABLE(i2c, ncp5623_id); + +static struct i2c_driver ncp5623_driver = { + .driver = { + .name = "ncp5623", + .of_match_table = of_match_ptr(ncp5623_of_match), + }, + .probe = ncp5623_probe, + .id_table = ncp5623_id, +}; + +module_i2c_driver(ncp5623_driver); + +MODULE_AUTHOR("Florian Vaussard <florian.vaussard@heig-vd.ch>"); +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("NCP5623 LED driver"); -- 2.5.5 ^ permalink raw reply related [flat|nested] 24+ messages in thread
[parent not found: <1474025672-5040-3-git-send-email-florian.vaussard-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org>]
* Re: [PATCH v3 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver 2016-09-16 11:34 ` Florian Vaussard @ 2016-09-18 18:20 ` Jacek Anaszewski -1 siblings, 0 replies; 24+ messages in thread From: Jacek Anaszewski @ 2016-09-18 18:20 UTC (permalink / raw) To: Florian Vaussard, devicetree-u79uwXL29TY76Z2rM5mHXA, Richard Purdie, Jacek Anaszewski Cc: Rob Herring, Mark Rutland, Pavel Machek, linux-leds-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Florian Vaussard Hi Florian, Thanks for the updated patch set. I have few comments below. On 09/16/2016 01:34 PM, Florian Vaussard wrote: > The NCP5623 is a 3-channel LED driver from On Semiconductor controlled > through I2C. The PWM of each channel can be independently set with 32 > distinct levels. In addition, the intensity of the current source can be > globally set using an external bias resistor fixing the reference > current (Iref) and a dedicated register (ILED), following the > relationship: > > I = 2400*Iref/(31-ILED) > > with Iref = Vref/Rbias, and Vref = 0.6V. > > Signed-off-by: Florian Vaussard <florian.vaussard-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org> > --- > drivers/leds/Kconfig | 11 +++ > drivers/leds/Makefile | 1 + > drivers/leds/leds-ncp5623.c | 234 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 246 insertions(+) > create mode 100644 drivers/leds/leds-ncp5623.c > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 7a628c6..1eda9bd 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -659,6 +659,17 @@ config LEDS_MLXCPLD > This option enabled support for the LEDs on the Mellanox > boards. Say Y to enabled these. > > +config LEDS_NCP5623 > + tristate "LED Support for NCP5623 I2C chip" > + depends on LEDS_CLASS > + depends on I2C > + help > + This option enables support for LEDs connected to NCP5623 > + LED driver chips accessed via the I2C bus. > + Driver supports brightness control. > + > + Say Y to enable this driver. > + > comment "LED Triggers" > source "drivers/leds/trigger/Kconfig" > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index 3965070..dc53dd8 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -71,6 +71,7 @@ obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o > obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o > obj-$(CONFIG_LEDS_PM8058) += leds-pm8058.o > obj-$(CONFIG_LEDS_MLXCPLD) += leds-mlxcpld.o > +obj-$(CONFIG_LEDS_NCP5623) += leds-ncp5623.o > > # LED SPI Drivers > obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o > diff --git a/drivers/leds/leds-ncp5623.c b/drivers/leds/leds-ncp5623.c > new file mode 100644 > index 0000000..875785a > --- /dev/null > +++ b/drivers/leds/leds-ncp5623.c > @@ -0,0 +1,234 @@ > +/* > + * Copyright 2016 Florian Vaussard <florian.vaussard-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org> > + * > + * Based on leds-tlc591xx.c I think that besides LED class facilities the only common thing is led_no. I'd skip this statement. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; version 2 of the License. > + */ > + > +#include <linux/bitops.h> > +#include <linux/i2c.h> > +#include <linux/kernel.h> > +#include <linux/leds.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/slab.h> > + > +#define NCP5623_MAX_LEDS 3 > +#define NCP5623_MAX_STEPS 31 > +#define NCP5623_MAX_CURRENT 31 Both values refer in fact to the same thing. And actually the name is not accurate since max current level is 30. > +#define NCP5623_MAX_CURRENT_UA 30000 > + > +#define NCP5623_CMD_SHIFT 5 > +#define CMD_SHUTDOWN (0x00 << NCP5623_CMD_SHIFT) > +#define CMD_ILED (0x01 << NCP5623_CMD_SHIFT) > +#define CMD_PWM1 (0x02 << NCP5623_CMD_SHIFT) > +#define CMD_PWM2 (0x03 << NCP5623_CMD_SHIFT) > +#define CMD_PWM3 (0x04 << NCP5623_CMD_SHIFT) > +#define CMD_UPWARD_DIM (0x05 << NCP5623_CMD_SHIFT) > +#define CMD_DOWNWARD_DIM (0x06 << NCP5623_CMD_SHIFT) > +#define CMD_DIM_STEP (0x07 << NCP5623_CMD_SHIFT) > + > +#define LED_TO_PWM_CMD(led) ((0x02 + led) << NCP5623_CMD_SHIFT) > + > +#define NCP5623_DATA_MASK GENMASK(NCP5623_CMD_SHIFT - 1, 0) > +#define NCP5623_CMD(cmd, data) (cmd | (data & NCP5623_DATA_MASK)) > + > +struct ncp5623_led { > + int led_no; > + u32 led_max_current; > + struct led_classdev ldev; > + struct ncp5623_priv *priv; > +}; > + > +struct ncp5623_priv { > + struct ncp5623_led leds[NCP5623_MAX_LEDS]; > + u32 led_iref; > + u32 leds_max_current; > + struct i2c_client *client; > +}; > + > +static struct ncp5623_led *ldev_to_led(struct led_classdev *ldev) > +{ > + return container_of(ldev, struct ncp5623_led, ldev); > +} > + > +static int ncp5623_send_cmd(struct ncp5623_priv *priv, u8 cmd, u8 data) > +{ > + char cmd_data[1] = { NCP5623_CMD(cmd, data) }; > + int err; > + > + err = i2c_master_send(priv->client, cmd_data, ARRAY_SIZE(cmd_data)); > + > + return (err < 0 ? err : 0); > +} > + > +static int ncp5623_brightness_set(struct led_classdev *led_cdev, > + enum led_brightness brightness) > +{ > + struct ncp5623_led *led = ldev_to_led(led_cdev); > + > + return ncp5623_send_cmd(led->priv, LED_TO_PWM_CMD(led->led_no), > + brightness); > +} > + > +static int ncp5623_configure(struct device *dev, > + struct ncp5623_priv *priv) > +{ > + unsigned int i; > + unsigned int n; > + struct ncp5623_led *led; > + int effective_current; > + int err; Below way of calculating max_brightness is not clear to me. Let's analyze it below, using values from your DT example. > + > + /* Setup the internal current source, round down */ > + n = 2400 * priv->led_iref / priv->leds_max_current + 1; n = 2400 * 10 / 20000 + 1 = 2 > + if (n > NCP5623_MAX_CURRENT) > + n = NCP5623_MAX_CURRENT; > + > + effective_current = 2400 * priv->led_iref / n; effective_current = 2400 * 10 / 2 = 12000 > + dev_dbg(dev, "setting maximum current to %u uA\n", effective_current); > + > + err = ncp5623_send_cmd(priv, CMD_ILED, NCP5623_MAX_CURRENT - n); > + if (err < 0) { > + dev_err(dev, "cannot set the current\n"); > + return err; > + } > + > + /* Setup each individual LED */ > + for (i = 0; i < NCP5623_MAX_LEDS; i++) { > + led = &priv->leds[i]; > + > + if (led->led_no < 0) > + continue; > + > + led->priv = priv; > + led->ldev.brightness_set_blocking = ncp5623_brightness_set; > + > + led->ldev.max_brightness = led->led_max_current * > + NCP5623_MAX_STEPS / effective_current; led->ldev.max_brightness = 20000 * 31 / 12000 = 51 This is not intuitive, and I'm not even sure if the result is in line with what you intended. Instead I propose the following: n_iled_max = 31 - (priv->led_iref * 2400 / priv->leds_max_current + !!(priv->led_iref * 2400 % priv->leds_max_current)) (n_iled_max = 31 - (24000 / 20000 + !!(24000 % 20000)) = 31 - (1 + 1) = 29) ncp5623_send_cmd(priv, CMD_ILED, n_iled_max) and then below for each LED: led->ldev.max_brightness = 31 - (priv->led_iref * 2400 / led->led_max_current + !!(priv->led_iref * 2400 % led->led_max_current)) (for led-max-microamp = 5000 led->ldev.max_brightness = 31 - (24000 / 5000 + !!(24000 % 5000)) = 31 - (4 + 1) = 26, which reflects quite well the logarithmic relation shown on Figure 5 in the documentation) Of course 31 should be replaced with NCP5623_MAX_CURRENT_LEVEL + 1, or another macro with some smart name, but I am currently unable to come up with satisfactory name. > + if (led->ldev.max_brightness > NCP5623_MAX_STEPS) > + led->ldev.max_brightness = NCP5623_MAX_STEPS; > + > + err = devm_led_classdev_register(dev, &led->ldev); > + if (err < 0) { > + dev_err(dev, "couldn't register LED %s\n", > + led->ldev.name); > + return err; > + } > + } > + > + return 0; > +} > + > +static int ncp5623_parse_dt(struct ncp5623_priv *priv, struct device_node *np) > +{ > + struct device_node *child; > + struct ncp5623_led *led; > + u32 reg; > + int count; > + int err; > + > + err = of_property_read_u32(np, "onnn,led-iref-microamp", > + &priv->led_iref); > + if (err) > + return -EINVAL; > + > + count = of_get_child_count(np); > + if (!count || count > NCP5623_MAX_LEDS) > + return -EINVAL; > + > + for_each_child_of_node(np, child) { > + err = of_property_read_u32(child, "reg", ®); > + if (err) > + return err; > + > + if (reg >= NCP5623_MAX_LEDS) { > + err = -EINVAL; > + goto dt_child_parse_error; > + } > + > + led = &priv->leds[reg]; > + if (led->led_no >= 0) { > + /* Already registered */ > + err = -EINVAL; > + goto dt_child_parse_error; > + } > + led->led_no = reg; > + > + err = of_property_read_u32(child, "led-max-microamp", > + &led->led_max_current); > + if (err || led->led_max_current > NCP5623_MAX_CURRENT_UA) > + return -EINVAL; > + if (led->led_max_current > priv->leds_max_current) > + priv->leds_max_current = led->led_max_current; > + > + led->ldev.name = > + of_get_property(child, "label", NULL) ? : child->name; > + led->ldev.default_trigger = > + of_get_property(child, "linux,default-trigger", NULL); > + } > + > + return 0; > + > +dt_child_parse_error: > + of_node_put(child); > + > + return err; > +} > + > +static const struct of_device_id ncp5623_of_match[] = { > + { .compatible = "onnn,ncp5623" }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, ncp5623_of_match); Please move it below ncp5623_probe(). > + > +static int ncp5623_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct device *dev = &client->dev; > + struct device_node *np = dev->of_node; > + struct ncp5623_priv *priv; > + int i, err; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + /* Mark all LEDs as inactive by default */ > + for (i = 0; i < NCP5623_MAX_LEDS; i++) > + priv->leds[i].led_no = -ENODEV; If you assumed that valid led_no start from 1 (in the documentation they LEDs are also called LED1, LED2, LED3), and modified your macros slightly, then you could get rid of this loop as led_no is zeroed by kzalloc(). > + > + priv->client = client; > + i2c_set_clientdata(client, priv); > + > + err = ncp5623_parse_dt(priv, np); > + if (err) > + return err; > + > + return ncp5623_configure(dev, priv); > +} > + > +static const struct i2c_device_id ncp5623_id[] = { > + { "ncp5623" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(i2c, ncp5623_id); > + > +static struct i2c_driver ncp5623_driver = { > + .driver = { > + .name = "ncp5623", > + .of_match_table = of_match_ptr(ncp5623_of_match), > + }, > + .probe = ncp5623_probe, > + .id_table = ncp5623_id, > +}; > + > +module_i2c_driver(ncp5623_driver); > + > +MODULE_AUTHOR("Florian Vaussard <florian.vaussard-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org>"); > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("NCP5623 LED driver"); > -- Best regards, Jacek Anaszewski -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver @ 2016-09-18 18:20 ` Jacek Anaszewski 0 siblings, 0 replies; 24+ messages in thread From: Jacek Anaszewski @ 2016-09-18 18:20 UTC (permalink / raw) To: Florian Vaussard, devicetree, Richard Purdie, Jacek Anaszewski Cc: Rob Herring, Mark Rutland, Pavel Machek, linux-leds, linux-kernel, Florian Vaussard Hi Florian, Thanks for the updated patch set. I have few comments below. On 09/16/2016 01:34 PM, Florian Vaussard wrote: > The NCP5623 is a 3-channel LED driver from On Semiconductor controlled > through I2C. The PWM of each channel can be independently set with 32 > distinct levels. In addition, the intensity of the current source can be > globally set using an external bias resistor fixing the reference > current (Iref) and a dedicated register (ILED), following the > relationship: > > I = 2400*Iref/(31-ILED) > > with Iref = Vref/Rbias, and Vref = 0.6V. > > Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch> > --- > drivers/leds/Kconfig | 11 +++ > drivers/leds/Makefile | 1 + > drivers/leds/leds-ncp5623.c | 234 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 246 insertions(+) > create mode 100644 drivers/leds/leds-ncp5623.c > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 7a628c6..1eda9bd 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -659,6 +659,17 @@ config LEDS_MLXCPLD > This option enabled support for the LEDs on the Mellanox > boards. Say Y to enabled these. > > +config LEDS_NCP5623 > + tristate "LED Support for NCP5623 I2C chip" > + depends on LEDS_CLASS > + depends on I2C > + help > + This option enables support for LEDs connected to NCP5623 > + LED driver chips accessed via the I2C bus. > + Driver supports brightness control. > + > + Say Y to enable this driver. > + > comment "LED Triggers" > source "drivers/leds/trigger/Kconfig" > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index 3965070..dc53dd8 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -71,6 +71,7 @@ obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o > obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o > obj-$(CONFIG_LEDS_PM8058) += leds-pm8058.o > obj-$(CONFIG_LEDS_MLXCPLD) += leds-mlxcpld.o > +obj-$(CONFIG_LEDS_NCP5623) += leds-ncp5623.o > > # LED SPI Drivers > obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o > diff --git a/drivers/leds/leds-ncp5623.c b/drivers/leds/leds-ncp5623.c > new file mode 100644 > index 0000000..875785a > --- /dev/null > +++ b/drivers/leds/leds-ncp5623.c > @@ -0,0 +1,234 @@ > +/* > + * Copyright 2016 Florian Vaussard <florian.vaussard@heig-vd.ch> > + * > + * Based on leds-tlc591xx.c I think that besides LED class facilities the only common thing is led_no. I'd skip this statement. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; version 2 of the License. > + */ > + > +#include <linux/bitops.h> > +#include <linux/i2c.h> > +#include <linux/kernel.h> > +#include <linux/leds.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/slab.h> > + > +#define NCP5623_MAX_LEDS 3 > +#define NCP5623_MAX_STEPS 31 > +#define NCP5623_MAX_CURRENT 31 Both values refer in fact to the same thing. And actually the name is not accurate since max current level is 30. > +#define NCP5623_MAX_CURRENT_UA 30000 > + > +#define NCP5623_CMD_SHIFT 5 > +#define CMD_SHUTDOWN (0x00 << NCP5623_CMD_SHIFT) > +#define CMD_ILED (0x01 << NCP5623_CMD_SHIFT) > +#define CMD_PWM1 (0x02 << NCP5623_CMD_SHIFT) > +#define CMD_PWM2 (0x03 << NCP5623_CMD_SHIFT) > +#define CMD_PWM3 (0x04 << NCP5623_CMD_SHIFT) > +#define CMD_UPWARD_DIM (0x05 << NCP5623_CMD_SHIFT) > +#define CMD_DOWNWARD_DIM (0x06 << NCP5623_CMD_SHIFT) > +#define CMD_DIM_STEP (0x07 << NCP5623_CMD_SHIFT) > + > +#define LED_TO_PWM_CMD(led) ((0x02 + led) << NCP5623_CMD_SHIFT) > + > +#define NCP5623_DATA_MASK GENMASK(NCP5623_CMD_SHIFT - 1, 0) > +#define NCP5623_CMD(cmd, data) (cmd | (data & NCP5623_DATA_MASK)) > + > +struct ncp5623_led { > + int led_no; > + u32 led_max_current; > + struct led_classdev ldev; > + struct ncp5623_priv *priv; > +}; > + > +struct ncp5623_priv { > + struct ncp5623_led leds[NCP5623_MAX_LEDS]; > + u32 led_iref; > + u32 leds_max_current; > + struct i2c_client *client; > +}; > + > +static struct ncp5623_led *ldev_to_led(struct led_classdev *ldev) > +{ > + return container_of(ldev, struct ncp5623_led, ldev); > +} > + > +static int ncp5623_send_cmd(struct ncp5623_priv *priv, u8 cmd, u8 data) > +{ > + char cmd_data[1] = { NCP5623_CMD(cmd, data) }; > + int err; > + > + err = i2c_master_send(priv->client, cmd_data, ARRAY_SIZE(cmd_data)); > + > + return (err < 0 ? err : 0); > +} > + > +static int ncp5623_brightness_set(struct led_classdev *led_cdev, > + enum led_brightness brightness) > +{ > + struct ncp5623_led *led = ldev_to_led(led_cdev); > + > + return ncp5623_send_cmd(led->priv, LED_TO_PWM_CMD(led->led_no), > + brightness); > +} > + > +static int ncp5623_configure(struct device *dev, > + struct ncp5623_priv *priv) > +{ > + unsigned int i; > + unsigned int n; > + struct ncp5623_led *led; > + int effective_current; > + int err; Below way of calculating max_brightness is not clear to me. Let's analyze it below, using values from your DT example. > + > + /* Setup the internal current source, round down */ > + n = 2400 * priv->led_iref / priv->leds_max_current + 1; n = 2400 * 10 / 20000 + 1 = 2 > + if (n > NCP5623_MAX_CURRENT) > + n = NCP5623_MAX_CURRENT; > + > + effective_current = 2400 * priv->led_iref / n; effective_current = 2400 * 10 / 2 = 12000 > + dev_dbg(dev, "setting maximum current to %u uA\n", effective_current); > + > + err = ncp5623_send_cmd(priv, CMD_ILED, NCP5623_MAX_CURRENT - n); > + if (err < 0) { > + dev_err(dev, "cannot set the current\n"); > + return err; > + } > + > + /* Setup each individual LED */ > + for (i = 0; i < NCP5623_MAX_LEDS; i++) { > + led = &priv->leds[i]; > + > + if (led->led_no < 0) > + continue; > + > + led->priv = priv; > + led->ldev.brightness_set_blocking = ncp5623_brightness_set; > + > + led->ldev.max_brightness = led->led_max_current * > + NCP5623_MAX_STEPS / effective_current; led->ldev.max_brightness = 20000 * 31 / 12000 = 51 This is not intuitive, and I'm not even sure if the result is in line with what you intended. Instead I propose the following: n_iled_max = 31 - (priv->led_iref * 2400 / priv->leds_max_current + !!(priv->led_iref * 2400 % priv->leds_max_current)) (n_iled_max = 31 - (24000 / 20000 + !!(24000 % 20000)) = 31 - (1 + 1) = 29) ncp5623_send_cmd(priv, CMD_ILED, n_iled_max) and then below for each LED: led->ldev.max_brightness = 31 - (priv->led_iref * 2400 / led->led_max_current + !!(priv->led_iref * 2400 % led->led_max_current)) (for led-max-microamp = 5000 led->ldev.max_brightness = 31 - (24000 / 5000 + !!(24000 % 5000)) = 31 - (4 + 1) = 26, which reflects quite well the logarithmic relation shown on Figure 5 in the documentation) Of course 31 should be replaced with NCP5623_MAX_CURRENT_LEVEL + 1, or another macro with some smart name, but I am currently unable to come up with satisfactory name. > + if (led->ldev.max_brightness > NCP5623_MAX_STEPS) > + led->ldev.max_brightness = NCP5623_MAX_STEPS; > + > + err = devm_led_classdev_register(dev, &led->ldev); > + if (err < 0) { > + dev_err(dev, "couldn't register LED %s\n", > + led->ldev.name); > + return err; > + } > + } > + > + return 0; > +} > + > +static int ncp5623_parse_dt(struct ncp5623_priv *priv, struct device_node *np) > +{ > + struct device_node *child; > + struct ncp5623_led *led; > + u32 reg; > + int count; > + int err; > + > + err = of_property_read_u32(np, "onnn,led-iref-microamp", > + &priv->led_iref); > + if (err) > + return -EINVAL; > + > + count = of_get_child_count(np); > + if (!count || count > NCP5623_MAX_LEDS) > + return -EINVAL; > + > + for_each_child_of_node(np, child) { > + err = of_property_read_u32(child, "reg", ®); > + if (err) > + return err; > + > + if (reg >= NCP5623_MAX_LEDS) { > + err = -EINVAL; > + goto dt_child_parse_error; > + } > + > + led = &priv->leds[reg]; > + if (led->led_no >= 0) { > + /* Already registered */ > + err = -EINVAL; > + goto dt_child_parse_error; > + } > + led->led_no = reg; > + > + err = of_property_read_u32(child, "led-max-microamp", > + &led->led_max_current); > + if (err || led->led_max_current > NCP5623_MAX_CURRENT_UA) > + return -EINVAL; > + if (led->led_max_current > priv->leds_max_current) > + priv->leds_max_current = led->led_max_current; > + > + led->ldev.name = > + of_get_property(child, "label", NULL) ? : child->name; > + led->ldev.default_trigger = > + of_get_property(child, "linux,default-trigger", NULL); > + } > + > + return 0; > + > +dt_child_parse_error: > + of_node_put(child); > + > + return err; > +} > + > +static const struct of_device_id ncp5623_of_match[] = { > + { .compatible = "onnn,ncp5623" }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, ncp5623_of_match); Please move it below ncp5623_probe(). > + > +static int ncp5623_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct device *dev = &client->dev; > + struct device_node *np = dev->of_node; > + struct ncp5623_priv *priv; > + int i, err; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + /* Mark all LEDs as inactive by default */ > + for (i = 0; i < NCP5623_MAX_LEDS; i++) > + priv->leds[i].led_no = -ENODEV; If you assumed that valid led_no start from 1 (in the documentation they LEDs are also called LED1, LED2, LED3), and modified your macros slightly, then you could get rid of this loop as led_no is zeroed by kzalloc(). > + > + priv->client = client; > + i2c_set_clientdata(client, priv); > + > + err = ncp5623_parse_dt(priv, np); > + if (err) > + return err; > + > + return ncp5623_configure(dev, priv); > +} > + > +static const struct i2c_device_id ncp5623_id[] = { > + { "ncp5623" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(i2c, ncp5623_id); > + > +static struct i2c_driver ncp5623_driver = { > + .driver = { > + .name = "ncp5623", > + .of_match_table = of_match_ptr(ncp5623_of_match), > + }, > + .probe = ncp5623_probe, > + .id_table = ncp5623_id, > +}; > + > +module_i2c_driver(ncp5623_driver); > + > +MODULE_AUTHOR("Florian Vaussard <florian.vaussard@heig-vd.ch>"); > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("NCP5623 LED driver"); > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <d6dfc1aa-941d-fdbb-5fda-74c85b571bc5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH v3 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver 2016-09-18 18:20 ` Jacek Anaszewski @ 2016-09-24 12:00 ` Pavel Machek -1 siblings, 0 replies; 24+ messages in thread From: Pavel Machek @ 2016-09-24 12:00 UTC (permalink / raw) To: Jacek Anaszewski Cc: Florian Vaussard, devicetree-u79uwXL29TY76Z2rM5mHXA, Richard Purdie, Jacek Anaszewski, Rob Herring, Mark Rutland, linux-leds-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Florian Vaussard [-- Attachment #1: Type: text/plain, Size: 384 bytes --] Hi! > Instead I propose the following: > > n_iled_max = > 31 - (priv->led_iref * 2400 / priv->leds_max_current + > !!(priv->led_iref * 2400 % priv->leds_max_current)) > div_round_up? Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver @ 2016-09-24 12:00 ` Pavel Machek 0 siblings, 0 replies; 24+ messages in thread From: Pavel Machek @ 2016-09-24 12:00 UTC (permalink / raw) To: Jacek Anaszewski Cc: Florian Vaussard, devicetree, Richard Purdie, Jacek Anaszewski, Rob Herring, Mark Rutland, linux-leds, linux-kernel, Florian Vaussard [-- Attachment #1: Type: text/plain, Size: 384 bytes --] Hi! > Instead I propose the following: > > n_iled_max = > 31 - (priv->led_iref * 2400 / priv->leds_max_current + > !!(priv->led_iref * 2400 % priv->leds_max_current)) > div_round_up? Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver 2016-09-24 12:00 ` Pavel Machek (?) @ 2016-09-24 18:45 ` Jacek Anaszewski -1 siblings, 0 replies; 24+ messages in thread From: Jacek Anaszewski @ 2016-09-24 18:45 UTC (permalink / raw) To: Pavel Machek Cc: Florian Vaussard, devicetree, Richard Purdie, Jacek Anaszewski, Rob Herring, Mark Rutland, linux-leds, linux-kernel, Florian Vaussard On 09/24/2016 02:00 PM, Pavel Machek wrote: > Hi! > >> Instead I propose the following: >> >> n_iled_max = >> 31 - (priv->led_iref * 2400 / priv->leds_max_current + >> !!(priv->led_iref * 2400 % priv->leds_max_current)) >> > > div_round_up? Exactly. Thanks. -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver 2016-09-18 18:20 ` Jacek Anaszewski @ 2016-09-29 16:18 ` Florian Vaussard -1 siblings, 0 replies; 24+ messages in thread From: Florian Vaussard @ 2016-09-29 16:18 UTC (permalink / raw) To: Jacek Anaszewski, Jacek Anaszewski Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Richard Purdie, Rob Herring, Mark Rutland, Pavel Machek, linux-leds-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Florian Vaussard [-- Attachment #1: Type: text/plain, Size: 7628 bytes --] Hi Jacek, Thank you for your comments! Le 18. 09. 16 à 20:20, Jacek Anaszewski a écrit : > Hi Florian, > > Thanks for the updated patch set. I have few comments below. > > On 09/16/2016 01:34 PM, Florian Vaussard wrote: >> The NCP5623 is a 3-channel LED driver from On Semiconductor controlled >> through I2C. The PWM of each channel can be independently set with 32 >> distinct levels. In addition, the intensity of the current source can be >> globally set using an external bias resistor fixing the reference >> current (Iref) and a dedicated register (ILED), following the >> relationship: >> >> I = 2400*Iref/(31-ILED) >> >> with Iref = Vref/Rbias, and Vref = 0.6V. >> >> Signed-off-by: Florian Vaussard <florian.vaussard-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org> >> --- >> drivers/leds/Kconfig | 11 +++ >> drivers/leds/Makefile | 1 + >> drivers/leds/leds-ncp5623.c | 234 ++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 246 insertions(+) >> create mode 100644 drivers/leds/leds-ncp5623.c >> [...] >> diff --git a/drivers/leds/leds-ncp5623.c b/drivers/leds/leds-ncp5623.c >> new file mode 100644 >> index 0000000..875785a >> --- /dev/null >> +++ b/drivers/leds/leds-ncp5623.c >> @@ -0,0 +1,234 @@ >> +/* >> + * Copyright 2016 Florian Vaussard <florian.vaussard-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org> >> + * >> + * Based on leds-tlc591xx.c > > I think that besides LED class facilities the only > common thing is led_no. I'd skip this statement. > >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; version 2 of the License. >> + */ >> + >> +#include <linux/bitops.h> >> +#include <linux/i2c.h> >> +#include <linux/kernel.h> >> +#include <linux/leds.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> >> +#include <linux/slab.h> >> + >> +#define NCP5623_MAX_LEDS 3 >> +#define NCP5623_MAX_STEPS 31 >> +#define NCP5623_MAX_CURRENT 31 > > Both values refer in fact to the same thing. > And actually the name is not accurate since max current level > is 30. > They do not refer to the same thing: * NCP5623_MAX_STEPS is the maximum number of PWM steps (0x1F -> 100% duty cycle) * NCP5623_MAX_CURRENT is related to the current delivered by the current source (0x1F -> ILED = ILEDmax = 2400*Iref). In this case, 30 and 31 give the same ILED according to the remark in Eq. 2 and according to Fig. 5 in the datasheet, but 31 is still a valid value. >> +#define NCP5623_MAX_CURRENT_UA 30000 >> + >> +#define NCP5623_CMD_SHIFT 5 >> +#define CMD_SHUTDOWN (0x00 << NCP5623_CMD_SHIFT) >> +#define CMD_ILED (0x01 << NCP5623_CMD_SHIFT) >> +#define CMD_PWM1 (0x02 << NCP5623_CMD_SHIFT) >> +#define CMD_PWM2 (0x03 << NCP5623_CMD_SHIFT) >> +#define CMD_PWM3 (0x04 << NCP5623_CMD_SHIFT) >> +#define CMD_UPWARD_DIM (0x05 << NCP5623_CMD_SHIFT) >> +#define CMD_DOWNWARD_DIM (0x06 << NCP5623_CMD_SHIFT) >> +#define CMD_DIM_STEP (0x07 << NCP5623_CMD_SHIFT) >> + >> +#define LED_TO_PWM_CMD(led) ((0x02 + led) << NCP5623_CMD_SHIFT) >> + >> +#define NCP5623_DATA_MASK GENMASK(NCP5623_CMD_SHIFT - 1, 0) >> +#define NCP5623_CMD(cmd, data) (cmd | (data & NCP5623_DATA_MASK)) >> + >> +struct ncp5623_led { >> + int led_no; >> + u32 led_max_current; >> + struct led_classdev ldev; >> + struct ncp5623_priv *priv; >> +}; >> + >> +struct ncp5623_priv { >> + struct ncp5623_led leds[NCP5623_MAX_LEDS]; >> + u32 led_iref; >> + u32 leds_max_current; >> + struct i2c_client *client; >> +}; >> + >> +static struct ncp5623_led *ldev_to_led(struct led_classdev *ldev) >> +{ >> + return container_of(ldev, struct ncp5623_led, ldev); >> +} >> + >> +static int ncp5623_send_cmd(struct ncp5623_priv *priv, u8 cmd, u8 data) >> +{ >> + char cmd_data[1] = { NCP5623_CMD(cmd, data) }; >> + int err; >> + >> + err = i2c_master_send(priv->client, cmd_data, ARRAY_SIZE(cmd_data)); >> + >> + return (err < 0 ? err : 0); >> +} >> + >> +static int ncp5623_brightness_set(struct led_classdev *led_cdev, >> + enum led_brightness brightness) >> +{ >> + struct ncp5623_led *led = ldev_to_led(led_cdev); >> + >> + return ncp5623_send_cmd(led->priv, LED_TO_PWM_CMD(led->led_no), >> + brightness); >> +} >> + >> +static int ncp5623_configure(struct device *dev, >> + struct ncp5623_priv *priv) >> +{ >> + unsigned int i; >> + unsigned int n; >> + struct ncp5623_led *led; >> + int effective_current; >> + int err; > > Below way of calculating max_brightness is not clear to me. > Let's analyze it below, using values from your DT example. > >> + >> + /* Setup the internal current source, round down */ >> + n = 2400 * priv->led_iref / priv->leds_max_current + 1; > > n = 2400 * 10 / 20000 + 1 = 2 > >> + if (n > NCP5623_MAX_CURRENT) >> + n = NCP5623_MAX_CURRENT; >> + >> + effective_current = 2400 * priv->led_iref / n; > > effective_current = 2400 * 10 / 2 = 12000 > >> + dev_dbg(dev, "setting maximum current to %u uA\n", effective_current); >> + >> + err = ncp5623_send_cmd(priv, CMD_ILED, NCP5623_MAX_CURRENT - n); >> + if (err < 0) { >> + dev_err(dev, "cannot set the current\n"); >> + return err; >> + } >> + >> + /* Setup each individual LED */ >> + for (i = 0; i < NCP5623_MAX_LEDS; i++) { >> + led = &priv->leds[i]; >> + >> + if (led->led_no < 0) >> + continue; >> + >> + led->priv = priv; >> + led->ldev.brightness_set_blocking = ncp5623_brightness_set; >> + >> + led->ldev.max_brightness = led->led_max_current * >> + NCP5623_MAX_STEPS / effective_current; > > led->ldev.max_brightness = 20000 * 31 / 12000 = 51 > > This is not intuitive, and I'm not even sure if the result is in line > with what you intended. > There is indeed a problem in the case the allowed current on the LED is greater than the effective current provided by the current source, as in your example. Here I should put something like: led->ldev.max_brightness = min(NCP5623_MAX_STEPS, x * NCP5623_MAX_STEPS / y); > Instead I propose the following: > > n_iled_max = > 31 - (priv->led_iref * 2400 / priv->leds_max_current + > !!(priv->led_iref * 2400 % priv->leds_max_current)) > > (n_iled_max = > 31 - (24000 / 20000 + !!(24000 % 20000)) = 31 - (1 + 1) = 29) > > ncp5623_send_cmd(priv, CMD_ILED, n_iled_max) > This is a good proposition, especially with the DIV_ROUND_UP proposed by Pavel. I simulated both and I noticed a problem in both cases for very low currents, as we would have negative values for the register setting (see attached figure). I will fix this in the next version. > and then below for each LED: > > led->ldev.max_brightness = > 31 - (priv->led_iref * 2400 / led->led_max_current + > !!(priv->led_iref * 2400 % led->led_max_current)) > > (for led-max-microamp = 5000 > led->ldev.max_brightness = > 31 - (24000 / 5000 + !!(24000 % 5000)) = 31 - (4 + 1) = 26, > which reflects quite well the logarithmic relation shown > on Figure 5 in the documentation) > Here you are mixing the current source and the PWM settings together. For example, if my current source was set to 20mA at the previous stage, but my LED can only sustain 10mA, I must limit the PWM duty cycle to 50%. Thus: max_brightness = 10mA * 31 / 20mA = 15 (0 => 0% duty cycle, 31 => 100% duty cycle) Best regards, Florian [-- Attachment #2: current-comparison.pdf --] [-- Type: application/pdf, Size: 4329 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver @ 2016-09-29 16:18 ` Florian Vaussard 0 siblings, 0 replies; 24+ messages in thread From: Florian Vaussard @ 2016-09-29 16:18 UTC (permalink / raw) To: Jacek Anaszewski, Jacek Anaszewski Cc: devicetree, Richard Purdie, Rob Herring, Mark Rutland, Pavel Machek, linux-leds, linux-kernel, Florian Vaussard [-- Attachment #1: Type: text/plain, Size: 7570 bytes --] Hi Jacek, Thank you for your comments! Le 18. 09. 16 à 20:20, Jacek Anaszewski a écrit : > Hi Florian, > > Thanks for the updated patch set. I have few comments below. > > On 09/16/2016 01:34 PM, Florian Vaussard wrote: >> The NCP5623 is a 3-channel LED driver from On Semiconductor controlled >> through I2C. The PWM of each channel can be independently set with 32 >> distinct levels. In addition, the intensity of the current source can be >> globally set using an external bias resistor fixing the reference >> current (Iref) and a dedicated register (ILED), following the >> relationship: >> >> I = 2400*Iref/(31-ILED) >> >> with Iref = Vref/Rbias, and Vref = 0.6V. >> >> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch> >> --- >> drivers/leds/Kconfig | 11 +++ >> drivers/leds/Makefile | 1 + >> drivers/leds/leds-ncp5623.c | 234 ++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 246 insertions(+) >> create mode 100644 drivers/leds/leds-ncp5623.c >> [...] >> diff --git a/drivers/leds/leds-ncp5623.c b/drivers/leds/leds-ncp5623.c >> new file mode 100644 >> index 0000000..875785a >> --- /dev/null >> +++ b/drivers/leds/leds-ncp5623.c >> @@ -0,0 +1,234 @@ >> +/* >> + * Copyright 2016 Florian Vaussard <florian.vaussard@heig-vd.ch> >> + * >> + * Based on leds-tlc591xx.c > > I think that besides LED class facilities the only > common thing is led_no. I'd skip this statement. > >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; version 2 of the License. >> + */ >> + >> +#include <linux/bitops.h> >> +#include <linux/i2c.h> >> +#include <linux/kernel.h> >> +#include <linux/leds.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> >> +#include <linux/slab.h> >> + >> +#define NCP5623_MAX_LEDS 3 >> +#define NCP5623_MAX_STEPS 31 >> +#define NCP5623_MAX_CURRENT 31 > > Both values refer in fact to the same thing. > And actually the name is not accurate since max current level > is 30. > They do not refer to the same thing: * NCP5623_MAX_STEPS is the maximum number of PWM steps (0x1F -> 100% duty cycle) * NCP5623_MAX_CURRENT is related to the current delivered by the current source (0x1F -> ILED = ILEDmax = 2400*Iref). In this case, 30 and 31 give the same ILED according to the remark in Eq. 2 and according to Fig. 5 in the datasheet, but 31 is still a valid value. >> +#define NCP5623_MAX_CURRENT_UA 30000 >> + >> +#define NCP5623_CMD_SHIFT 5 >> +#define CMD_SHUTDOWN (0x00 << NCP5623_CMD_SHIFT) >> +#define CMD_ILED (0x01 << NCP5623_CMD_SHIFT) >> +#define CMD_PWM1 (0x02 << NCP5623_CMD_SHIFT) >> +#define CMD_PWM2 (0x03 << NCP5623_CMD_SHIFT) >> +#define CMD_PWM3 (0x04 << NCP5623_CMD_SHIFT) >> +#define CMD_UPWARD_DIM (0x05 << NCP5623_CMD_SHIFT) >> +#define CMD_DOWNWARD_DIM (0x06 << NCP5623_CMD_SHIFT) >> +#define CMD_DIM_STEP (0x07 << NCP5623_CMD_SHIFT) >> + >> +#define LED_TO_PWM_CMD(led) ((0x02 + led) << NCP5623_CMD_SHIFT) >> + >> +#define NCP5623_DATA_MASK GENMASK(NCP5623_CMD_SHIFT - 1, 0) >> +#define NCP5623_CMD(cmd, data) (cmd | (data & NCP5623_DATA_MASK)) >> + >> +struct ncp5623_led { >> + int led_no; >> + u32 led_max_current; >> + struct led_classdev ldev; >> + struct ncp5623_priv *priv; >> +}; >> + >> +struct ncp5623_priv { >> + struct ncp5623_led leds[NCP5623_MAX_LEDS]; >> + u32 led_iref; >> + u32 leds_max_current; >> + struct i2c_client *client; >> +}; >> + >> +static struct ncp5623_led *ldev_to_led(struct led_classdev *ldev) >> +{ >> + return container_of(ldev, struct ncp5623_led, ldev); >> +} >> + >> +static int ncp5623_send_cmd(struct ncp5623_priv *priv, u8 cmd, u8 data) >> +{ >> + char cmd_data[1] = { NCP5623_CMD(cmd, data) }; >> + int err; >> + >> + err = i2c_master_send(priv->client, cmd_data, ARRAY_SIZE(cmd_data)); >> + >> + return (err < 0 ? err : 0); >> +} >> + >> +static int ncp5623_brightness_set(struct led_classdev *led_cdev, >> + enum led_brightness brightness) >> +{ >> + struct ncp5623_led *led = ldev_to_led(led_cdev); >> + >> + return ncp5623_send_cmd(led->priv, LED_TO_PWM_CMD(led->led_no), >> + brightness); >> +} >> + >> +static int ncp5623_configure(struct device *dev, >> + struct ncp5623_priv *priv) >> +{ >> + unsigned int i; >> + unsigned int n; >> + struct ncp5623_led *led; >> + int effective_current; >> + int err; > > Below way of calculating max_brightness is not clear to me. > Let's analyze it below, using values from your DT example. > >> + >> + /* Setup the internal current source, round down */ >> + n = 2400 * priv->led_iref / priv->leds_max_current + 1; > > n = 2400 * 10 / 20000 + 1 = 2 > >> + if (n > NCP5623_MAX_CURRENT) >> + n = NCP5623_MAX_CURRENT; >> + >> + effective_current = 2400 * priv->led_iref / n; > > effective_current = 2400 * 10 / 2 = 12000 > >> + dev_dbg(dev, "setting maximum current to %u uA\n", effective_current); >> + >> + err = ncp5623_send_cmd(priv, CMD_ILED, NCP5623_MAX_CURRENT - n); >> + if (err < 0) { >> + dev_err(dev, "cannot set the current\n"); >> + return err; >> + } >> + >> + /* Setup each individual LED */ >> + for (i = 0; i < NCP5623_MAX_LEDS; i++) { >> + led = &priv->leds[i]; >> + >> + if (led->led_no < 0) >> + continue; >> + >> + led->priv = priv; >> + led->ldev.brightness_set_blocking = ncp5623_brightness_set; >> + >> + led->ldev.max_brightness = led->led_max_current * >> + NCP5623_MAX_STEPS / effective_current; > > led->ldev.max_brightness = 20000 * 31 / 12000 = 51 > > This is not intuitive, and I'm not even sure if the result is in line > with what you intended. > There is indeed a problem in the case the allowed current on the LED is greater than the effective current provided by the current source, as in your example. Here I should put something like: led->ldev.max_brightness = min(NCP5623_MAX_STEPS, x * NCP5623_MAX_STEPS / y); > Instead I propose the following: > > n_iled_max = > 31 - (priv->led_iref * 2400 / priv->leds_max_current + > !!(priv->led_iref * 2400 % priv->leds_max_current)) > > (n_iled_max = > 31 - (24000 / 20000 + !!(24000 % 20000)) = 31 - (1 + 1) = 29) > > ncp5623_send_cmd(priv, CMD_ILED, n_iled_max) > This is a good proposition, especially with the DIV_ROUND_UP proposed by Pavel. I simulated both and I noticed a problem in both cases for very low currents, as we would have negative values for the register setting (see attached figure). I will fix this in the next version. > and then below for each LED: > > led->ldev.max_brightness = > 31 - (priv->led_iref * 2400 / led->led_max_current + > !!(priv->led_iref * 2400 % led->led_max_current)) > > (for led-max-microamp = 5000 > led->ldev.max_brightness = > 31 - (24000 / 5000 + !!(24000 % 5000)) = 31 - (4 + 1) = 26, > which reflects quite well the logarithmic relation shown > on Figure 5 in the documentation) > Here you are mixing the current source and the PWM settings together. For example, if my current source was set to 20mA at the previous stage, but my LED can only sustain 10mA, I must limit the PWM duty cycle to 50%. Thus: max_brightness = 10mA * 31 / 20mA = 15 (0 => 0% duty cycle, 31 => 100% duty cycle) Best regards, Florian [-- Attachment #2: current-comparison.pdf --] [-- Type: application/pdf, Size: 4329 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <4990cd83-ae67-aec4-ae8b-4d0db3cbc9fe-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH v3 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver 2016-09-29 16:18 ` Florian Vaussard @ 2016-09-29 17:13 ` Florian Vaussard -1 siblings, 0 replies; 24+ messages in thread From: Florian Vaussard @ 2016-09-29 17:13 UTC (permalink / raw) To: Jacek Anaszewski, Jacek Anaszewski Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Richard Purdie, Rob Herring, Mark Rutland, Pavel Machek, linux-leds-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Florian Vaussard [-- Attachment #1: Type: text/plain, Size: 3990 bytes --] Replying to my self after thinking twice... Le 29. 09. 16 à 18:18, Florian Vaussard a écrit : > Hi Jacek, > > Thank you for your comments! > > Le 18. 09. 16 à 20:20, Jacek Anaszewski a écrit : >> Hi Florian, >> >> Thanks for the updated patch set. I have few comments below. >> >> On 09/16/2016 01:34 PM, Florian Vaussard wrote: >>> The NCP5623 is a 3-channel LED driver from On Semiconductor controlled >>> through I2C. The PWM of each channel can be independently set with 32 >>> distinct levels. In addition, the intensity of the current source can be >>> globally set using an external bias resistor fixing the reference >>> current (Iref) and a dedicated register (ILED), following the >>> relationship: >>> >>> I = 2400*Iref/(31-ILED) >>> >>> with Iref = Vref/Rbias, and Vref = 0.6V. >>> >>> Signed-off-by: Florian Vaussard <florian.vaussard-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org> >>> --- >>> drivers/leds/Kconfig | 11 +++ >>> drivers/leds/Makefile | 1 + >>> drivers/leds/leds-ncp5623.c | 234 ++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 246 insertions(+) >>> create mode 100644 drivers/leds/leds-ncp5623.c >>> [...] >>> +static int ncp5623_configure(struct device *dev, >>> + struct ncp5623_priv *priv) >>> +{ >>> + unsigned int i; >>> + unsigned int n; >>> + struct ncp5623_led *led; >>> + int effective_current; >>> + int err; >> >> Below way of calculating max_brightness is not clear to me. >> Let's analyze it below, using values from your DT example. >> >>> + >>> + /* Setup the internal current source, round down */ >>> + n = 2400 * priv->led_iref / priv->leds_max_current + 1; >> >> n = 2400 * 10 / 20000 + 1 = 2 >> >>> + if (n > NCP5623_MAX_CURRENT) >>> + n = NCP5623_MAX_CURRENT; >>> + >>> + effective_current = 2400 * priv->led_iref / n; >> >> effective_current = 2400 * 10 / 2 = 12000 >> >>> + dev_dbg(dev, "setting maximum current to %u uA\n", effective_current); >>> + >>> + err = ncp5623_send_cmd(priv, CMD_ILED, NCP5623_MAX_CURRENT - n); >>> + if (err < 0) { >>> + dev_err(dev, "cannot set the current\n"); >>> + return err; >>> + } >>> + >>> + /* Setup each individual LED */ >>> + for (i = 0; i < NCP5623_MAX_LEDS; i++) { >>> + led = &priv->leds[i]; >>> + >>> + if (led->led_no < 0) >>> + continue; >>> + >>> + led->priv = priv; >>> + led->ldev.brightness_set_blocking = ncp5623_brightness_set; >>> + >>> + led->ldev.max_brightness = led->led_max_current * >>> + NCP5623_MAX_STEPS / effective_current; >> >> led->ldev.max_brightness = 20000 * 31 / 12000 = 51 >> >> This is not intuitive, and I'm not even sure if the result is in line >> with what you intended. >> > > There is indeed a problem in the case the allowed current on the LED is greater > than the effective current provided by the current source, as in your example. > Here I should put something like: > > led->ldev.max_brightness = > min(NCP5623_MAX_STEPS, x * NCP5623_MAX_STEPS / y); > >> Instead I propose the following: >> >> n_iled_max = >> 31 - (priv->led_iref * 2400 / priv->leds_max_current + >> !!(priv->led_iref * 2400 % priv->leds_max_current)) >> >> (n_iled_max = >> 31 - (24000 / 20000 + !!(24000 % 20000)) = 31 - (1 + 1) = 29) >> >> ncp5623_send_cmd(priv, CMD_ILED, n_iled_max) >> > > This is a good proposition, especially with the DIV_ROUND_UP proposed by Pavel. > I simulated both and I noticed a problem in both cases for very low currents, as > we would have negative values for the register setting (see attached figure). I > will fix this in the next version. > In fact my original solution does not have this problem because of the (n > NCP5623_MAX_CURRENT) check and clipping before computing the effective current. This was not included in my simulation, here is the updated graph. So I will enhance your solution to avoid this exact problem. Best, Florian [-- Attachment #2: current-comparison.pdf --] [-- Type: application/pdf, Size: 4320 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver @ 2016-09-29 17:13 ` Florian Vaussard 0 siblings, 0 replies; 24+ messages in thread From: Florian Vaussard @ 2016-09-29 17:13 UTC (permalink / raw) To: Jacek Anaszewski, Jacek Anaszewski Cc: devicetree, Richard Purdie, Rob Herring, Mark Rutland, Pavel Machek, linux-leds, linux-kernel, Florian Vaussard [-- Attachment #1: Type: text/plain, Size: 3961 bytes --] Replying to my self after thinking twice... Le 29. 09. 16 à 18:18, Florian Vaussard a écrit : > Hi Jacek, > > Thank you for your comments! > > Le 18. 09. 16 à 20:20, Jacek Anaszewski a écrit : >> Hi Florian, >> >> Thanks for the updated patch set. I have few comments below. >> >> On 09/16/2016 01:34 PM, Florian Vaussard wrote: >>> The NCP5623 is a 3-channel LED driver from On Semiconductor controlled >>> through I2C. The PWM of each channel can be independently set with 32 >>> distinct levels. In addition, the intensity of the current source can be >>> globally set using an external bias resistor fixing the reference >>> current (Iref) and a dedicated register (ILED), following the >>> relationship: >>> >>> I = 2400*Iref/(31-ILED) >>> >>> with Iref = Vref/Rbias, and Vref = 0.6V. >>> >>> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch> >>> --- >>> drivers/leds/Kconfig | 11 +++ >>> drivers/leds/Makefile | 1 + >>> drivers/leds/leds-ncp5623.c | 234 ++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 246 insertions(+) >>> create mode 100644 drivers/leds/leds-ncp5623.c >>> [...] >>> +static int ncp5623_configure(struct device *dev, >>> + struct ncp5623_priv *priv) >>> +{ >>> + unsigned int i; >>> + unsigned int n; >>> + struct ncp5623_led *led; >>> + int effective_current; >>> + int err; >> >> Below way of calculating max_brightness is not clear to me. >> Let's analyze it below, using values from your DT example. >> >>> + >>> + /* Setup the internal current source, round down */ >>> + n = 2400 * priv->led_iref / priv->leds_max_current + 1; >> >> n = 2400 * 10 / 20000 + 1 = 2 >> >>> + if (n > NCP5623_MAX_CURRENT) >>> + n = NCP5623_MAX_CURRENT; >>> + >>> + effective_current = 2400 * priv->led_iref / n; >> >> effective_current = 2400 * 10 / 2 = 12000 >> >>> + dev_dbg(dev, "setting maximum current to %u uA\n", effective_current); >>> + >>> + err = ncp5623_send_cmd(priv, CMD_ILED, NCP5623_MAX_CURRENT - n); >>> + if (err < 0) { >>> + dev_err(dev, "cannot set the current\n"); >>> + return err; >>> + } >>> + >>> + /* Setup each individual LED */ >>> + for (i = 0; i < NCP5623_MAX_LEDS; i++) { >>> + led = &priv->leds[i]; >>> + >>> + if (led->led_no < 0) >>> + continue; >>> + >>> + led->priv = priv; >>> + led->ldev.brightness_set_blocking = ncp5623_brightness_set; >>> + >>> + led->ldev.max_brightness = led->led_max_current * >>> + NCP5623_MAX_STEPS / effective_current; >> >> led->ldev.max_brightness = 20000 * 31 / 12000 = 51 >> >> This is not intuitive, and I'm not even sure if the result is in line >> with what you intended. >> > > There is indeed a problem in the case the allowed current on the LED is greater > than the effective current provided by the current source, as in your example. > Here I should put something like: > > led->ldev.max_brightness = > min(NCP5623_MAX_STEPS, x * NCP5623_MAX_STEPS / y); > >> Instead I propose the following: >> >> n_iled_max = >> 31 - (priv->led_iref * 2400 / priv->leds_max_current + >> !!(priv->led_iref * 2400 % priv->leds_max_current)) >> >> (n_iled_max = >> 31 - (24000 / 20000 + !!(24000 % 20000)) = 31 - (1 + 1) = 29) >> >> ncp5623_send_cmd(priv, CMD_ILED, n_iled_max) >> > > This is a good proposition, especially with the DIV_ROUND_UP proposed by Pavel. > I simulated both and I noticed a problem in both cases for very low currents, as > we would have negative values for the register setting (see attached figure). I > will fix this in the next version. > In fact my original solution does not have this problem because of the (n > NCP5623_MAX_CURRENT) check and clipping before computing the effective current. This was not included in my simulation, here is the updated graph. So I will enhance your solution to avoid this exact problem. Best, Florian [-- Attachment #2: current-comparison.pdf --] [-- Type: application/pdf, Size: 4320 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver 2016-09-29 16:18 ` Florian Vaussard (?) (?) @ 2016-09-30 21:00 ` Jacek Anaszewski -1 siblings, 0 replies; 24+ messages in thread From: Jacek Anaszewski @ 2016-09-30 21:00 UTC (permalink / raw) To: florian.vaussard, Jacek Anaszewski Cc: devicetree, Richard Purdie, Rob Herring, Mark Rutland, Pavel Machek, linux-leds, linux-kernel, Florian Vaussard Hi Florian, On 09/29/2016 06:18 PM, Florian Vaussard wrote: > Hi Jacek, > > Thank you for your comments! > > Le 18. 09. 16 à 20:20, Jacek Anaszewski a écrit : >> Hi Florian, >> >> Thanks for the updated patch set. I have few comments below. >> >> On 09/16/2016 01:34 PM, Florian Vaussard wrote: >>> The NCP5623 is a 3-channel LED driver from On Semiconductor controlled >>> through I2C. The PWM of each channel can be independently set with 32 >>> distinct levels. In addition, the intensity of the current source can be >>> globally set using an external bias resistor fixing the reference >>> current (Iref) and a dedicated register (ILED), following the >>> relationship: >>> >>> I = 2400*Iref/(31-ILED) >>> >>> with Iref = Vref/Rbias, and Vref = 0.6V. >>> >>> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch> >>> --- >>> drivers/leds/Kconfig | 11 +++ >>> drivers/leds/Makefile | 1 + >>> drivers/leds/leds-ncp5623.c | 234 ++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 246 insertions(+) >>> create mode 100644 drivers/leds/leds-ncp5623.c >>> > > [...] > >>> diff --git a/drivers/leds/leds-ncp5623.c b/drivers/leds/leds-ncp5623.c >>> new file mode 100644 >>> index 0000000..875785a >>> --- /dev/null >>> +++ b/drivers/leds/leds-ncp5623.c >>> @@ -0,0 +1,234 @@ >>> +/* >>> + * Copyright 2016 Florian Vaussard <florian.vaussard@heig-vd.ch> >>> + * >>> + * Based on leds-tlc591xx.c >> >> I think that besides LED class facilities the only >> common thing is led_no. I'd skip this statement. >> >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License as published by >>> + * the Free Software Foundation; version 2 of the License. >>> + */ >>> + >>> +#include <linux/bitops.h> >>> +#include <linux/i2c.h> >>> +#include <linux/kernel.h> >>> +#include <linux/leds.h> >>> +#include <linux/module.h> >>> +#include <linux/of.h> >>> +#include <linux/of_device.h> >>> +#include <linux/slab.h> >>> + >>> +#define NCP5623_MAX_LEDS 3 >>> +#define NCP5623_MAX_STEPS 31 >>> +#define NCP5623_MAX_CURRENT 31 >> >> Both values refer in fact to the same thing. >> And actually the name is not accurate since max current level >> is 30. >> > > They do not refer to the same thing: > > * NCP5623_MAX_STEPS is the maximum number of PWM steps (0x1F -> 100% duty cycle) > * NCP5623_MAX_CURRENT is related to the current delivered by the current source > (0x1F -> ILED = ILEDmax = 2400*Iref). In this case, 30 and 31 give the same ILED > according to the remark in Eq. 2 and according to Fig. 5 in the datasheet, but > 31 is still a valid value. > >>> +#define NCP5623_MAX_CURRENT_UA 30000 >>> + >>> +#define NCP5623_CMD_SHIFT 5 >>> +#define CMD_SHUTDOWN (0x00 << NCP5623_CMD_SHIFT) >>> +#define CMD_ILED (0x01 << NCP5623_CMD_SHIFT) >>> +#define CMD_PWM1 (0x02 << NCP5623_CMD_SHIFT) >>> +#define CMD_PWM2 (0x03 << NCP5623_CMD_SHIFT) >>> +#define CMD_PWM3 (0x04 << NCP5623_CMD_SHIFT) >>> +#define CMD_UPWARD_DIM (0x05 << NCP5623_CMD_SHIFT) >>> +#define CMD_DOWNWARD_DIM (0x06 << NCP5623_CMD_SHIFT) >>> +#define CMD_DIM_STEP (0x07 << NCP5623_CMD_SHIFT) >>> + >>> +#define LED_TO_PWM_CMD(led) ((0x02 + led) << NCP5623_CMD_SHIFT) >>> + >>> +#define NCP5623_DATA_MASK GENMASK(NCP5623_CMD_SHIFT - 1, 0) >>> +#define NCP5623_CMD(cmd, data) (cmd | (data & NCP5623_DATA_MASK)) >>> + >>> +struct ncp5623_led { >>> + int led_no; >>> + u32 led_max_current; >>> + struct led_classdev ldev; >>> + struct ncp5623_priv *priv; >>> +}; >>> + >>> +struct ncp5623_priv { >>> + struct ncp5623_led leds[NCP5623_MAX_LEDS]; >>> + u32 led_iref; >>> + u32 leds_max_current; >>> + struct i2c_client *client; >>> +}; >>> + >>> +static struct ncp5623_led *ldev_to_led(struct led_classdev *ldev) >>> +{ >>> + return container_of(ldev, struct ncp5623_led, ldev); >>> +} >>> + >>> +static int ncp5623_send_cmd(struct ncp5623_priv *priv, u8 cmd, u8 data) >>> +{ >>> + char cmd_data[1] = { NCP5623_CMD(cmd, data) }; >>> + int err; >>> + >>> + err = i2c_master_send(priv->client, cmd_data, ARRAY_SIZE(cmd_data)); >>> + >>> + return (err < 0 ? err : 0); >>> +} >>> + >>> +static int ncp5623_brightness_set(struct led_classdev *led_cdev, >>> + enum led_brightness brightness) >>> +{ >>> + struct ncp5623_led *led = ldev_to_led(led_cdev); >>> + >>> + return ncp5623_send_cmd(led->priv, LED_TO_PWM_CMD(led->led_no), >>> + brightness); >>> +} >>> + >>> +static int ncp5623_configure(struct device *dev, >>> + struct ncp5623_priv *priv) >>> +{ >>> + unsigned int i; >>> + unsigned int n; >>> + struct ncp5623_led *led; >>> + int effective_current; >>> + int err; >> >> Below way of calculating max_brightness is not clear to me. >> Let's analyze it below, using values from your DT example. >> >>> + >>> + /* Setup the internal current source, round down */ >>> + n = 2400 * priv->led_iref / priv->leds_max_current + 1; >> >> n = 2400 * 10 / 20000 + 1 = 2 >> >>> + if (n > NCP5623_MAX_CURRENT) >>> + n = NCP5623_MAX_CURRENT; >>> + >>> + effective_current = 2400 * priv->led_iref / n; >> >> effective_current = 2400 * 10 / 2 = 12000 >> >>> + dev_dbg(dev, "setting maximum current to %u uA\n", effective_current); >>> + >>> + err = ncp5623_send_cmd(priv, CMD_ILED, NCP5623_MAX_CURRENT - n); >>> + if (err < 0) { >>> + dev_err(dev, "cannot set the current\n"); >>> + return err; >>> + } >>> + >>> + /* Setup each individual LED */ >>> + for (i = 0; i < NCP5623_MAX_LEDS; i++) { >>> + led = &priv->leds[i]; >>> + >>> + if (led->led_no < 0) >>> + continue; >>> + >>> + led->priv = priv; >>> + led->ldev.brightness_set_blocking = ncp5623_brightness_set; >>> + >>> + led->ldev.max_brightness = led->led_max_current * >>> + NCP5623_MAX_STEPS / effective_current; >> >> led->ldev.max_brightness = 20000 * 31 / 12000 = 51 >> >> This is not intuitive, and I'm not even sure if the result is in line >> with what you intended. >> > > There is indeed a problem in the case the allowed current on the LED is greater > than the effective current provided by the current source, as in your example. > Here I should put something like: > > led->ldev.max_brightness = > min(NCP5623_MAX_STEPS, x * NCP5623_MAX_STEPS / y); > >> Instead I propose the following: >> >> n_iled_max = >> 31 - (priv->led_iref * 2400 / priv->leds_max_current + >> !!(priv->led_iref * 2400 % priv->leds_max_current)) >> >> (n_iled_max = >> 31 - (24000 / 20000 + !!(24000 % 20000)) = 31 - (1 + 1) = 29) >> >> ncp5623_send_cmd(priv, CMD_ILED, n_iled_max) >> > > This is a good proposition, especially with the DIV_ROUND_UP proposed by Pavel. > I simulated both and I noticed a problem in both cases for very low currents, as > we would have negative values for the register setting (see attached figure). I > will fix this in the next version. > >> and then below for each LED: >> >> led->ldev.max_brightness = >> 31 - (priv->led_iref * 2400 / led->led_max_current + >> !!(priv->led_iref * 2400 % led->led_max_current)) >> >> (for led-max-microamp = 5000 >> led->ldev.max_brightness = >> 31 - (24000 / 5000 + !!(24000 % 5000)) = 31 - (4 + 1) = 26, >> which reflects quite well the logarithmic relation shown >> on Figure 5 in the documentation) >> > > Here you are mixing the current source and the PWM settings together. For Indeed, you're right. So, I'll wait for the next version of the patch to have a well established ground for max_brightness calculation analysis. > example, if my current source was set to 20mA at the previous stage, but my LED > can only sustain 10mA, I must limit the PWM duty cycle to 50%. Thus: > > max_brightness = 10mA * 31 / 20mA = 15 > > (0 => 0% duty cycle, 31 => 100% duty cycle) -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2016-09-30 21:00 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-16 11:34 [PATCH v3 0/2] leds: Add driver for NCP5623 3-channel I2C LED driver Florian Vaussard 2016-09-16 11:34 ` Florian Vaussard 2016-09-16 11:34 ` [PATCH v3 1/2] leds: ncp5623: Add device tree binding documentation Florian Vaussard [not found] ` <1474025672-5040-2-git-send-email-florian.vaussard-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org> 2016-09-23 17:29 ` Rob Herring 2016-09-23 17:29 ` Rob Herring 2016-09-24 11:58 ` Pavel Machek 2016-09-24 11:58 ` Pavel Machek 2016-09-24 19:06 ` Jacek Anaszewski 2016-09-28 10:04 ` Florian Vaussard 2016-09-28 10:02 ` Florian Vaussard 2016-09-28 10:02 ` Florian Vaussard 2016-09-28 10:58 ` Pavel Machek [not found] ` <1474025672-5040-1-git-send-email-florian.vaussard-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org> 2016-09-16 11:34 ` [PATCH v3 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver Florian Vaussard 2016-09-16 11:34 ` Florian Vaussard [not found] ` <1474025672-5040-3-git-send-email-florian.vaussard-EWQkb/GNqlFyDzI6CaY1VQ@public.gmane.org> 2016-09-18 18:20 ` Jacek Anaszewski 2016-09-18 18:20 ` Jacek Anaszewski [not found] ` <d6dfc1aa-941d-fdbb-5fda-74c85b571bc5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-09-24 12:00 ` Pavel Machek 2016-09-24 12:00 ` Pavel Machek 2016-09-24 18:45 ` Jacek Anaszewski 2016-09-29 16:18 ` Florian Vaussard 2016-09-29 16:18 ` Florian Vaussard [not found] ` <4990cd83-ae67-aec4-ae8b-4d0db3cbc9fe-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-09-29 17:13 ` Florian Vaussard 2016-09-29 17:13 ` Florian Vaussard 2016-09-30 21:00 ` Jacek Anaszewski
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.