dt: bindings: leds: Add NCP5623 multi-LED Controller
Subsystems (3)
PATCH 1/2 dt: bindings: leds: Add NCP5623 multi-LED Controller
NCP5623 is DC-DC multi-LED controller which can be used for RGB illumination or backlight LCD display. NCP5623 provides 94% peak efficiency.
--- .../bindings/leds/onnn,ncp5623.yaml | 98 +++++++++++++++++++ 1 file changed, 98 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml
Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml new file +98 -0
@@ -0,0 +1,98 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/leds/onnn,ncp5623.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ON Semiconductor NCP5623 multi-LED Driver + +maintainers: + - Abdel Alkuor <[email protected]> + +description: | + NCP5623 Triple Output I2C Controlled LED Driver. + https://www.onsemi.com/pdf/datasheet/ncp5623-d.pdf + +properties: + compatible: + enum: + - onnn,ncp5623 + + reg: + enum: + - 0x38 + + multi-led: + type: object + $ref: leds-class-multicolor.yaml# + unevaluatedProperties: false + + properties: + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + + patternProperties: + "^led@[0-2]$": + type: object + $ref: common.yaml# + unevaluatedProperties: false + + properties: + reg: + description: Index of the LED. + minimum: 0 + maximum: 2 + + required: + - reg + - color + + required: + - "#address-cells" + - "#size-cells" + +required: + - compatible + - reg + - multi-led + +additionalProperties: false + +examples: + - | + #include <dt-bindings/leds/common.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + led-controller@38 { + compatible = "onnn,ncp5623"; + reg = <0x38>; + + multi-led { + color = <LED_COLOR_ID_RGB>; + + #address-cells = <1>; + #size-cells = <0>; + + led@0 { + reg = <0>; + color = <LED_COLOR_ID_RED>; + }; + + led@1 { + reg = <1>; + color = <LED_COLOR_ID_GREEN>; + }; + + led@2 { + reg = <2>; + color = <LED_COLOR_ID_BLUE>; + }; + }; + }; + };
Signature · 1 line
2.34.1
PATCH 2/2 leds: Add NCP5623 multi-led driver
NCP5623 is DC-DC multi-LEDs driver with 94% peak efficiency. NCP5623 has three PWMs which can be programmed up to 32 steps giving 32768 colors hue.
NCP5623 driver supports gradual dimming upward/downward with programmable delay steps. Also, the driver supports driving a single LED or multi-LED like RGB.
--- .../sysfs-class-led-multicolor-driver-ncp5623 | 46 +++ drivers/leds/rgb/Kconfig | 11 + drivers/leds/rgb/Makefile | 1 + drivers/leds/rgb/leds-ncp5623.c | 320 ++++++++++++++++++ 4 files changed, 378 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 create mode 100644 drivers/leds/rgb/leds-ncp5623.c
Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 new file +46 -0
@@ -0,0 +1,46 @@ +What: /sys/class/leds/<led>/direction +Date: Feb 2024 +KernelVersion: 6.8 +Contact: Abdel Alkuor <[email protected]> +Description: + Set gradual dimming direction which + can be either none, up, or down. + After setting the direction, brightness + can be set using one of 31 steps. + + ==== ======== ==== ======== ==== ======== + Step ILED(mA) Step ILED(mA) Step ILED(mA) + 0 0 11 1.38 22 3.06 + 1 0.92 12 1.45 23 3.45 + 2 0.95 13 1.53 24 3.94 + 3 0.98 14 1.62 25 4.60 + 4 1.02 15 1.72 26 5.52 + 5 1.06 16 1.84 27 6.90 + 6 1.10 17 1.97 28 9.20 + 7 1.15 18 2.12 29 13.80 + 8 1.20 19 2.30 30 27.60 + 9 1.25 20 2.50 31 27.60 + 10 1.31 21 2.76 + ==== ======== ==== ======== ==== ======== + +What: /sys/class/leds/<led>/dim_step +Date: Feb 2024 +KernelVersion: 6.8 +Contact: Abdel Alkuor <[email protected]> +Description: + Set gradual dimming time. + + ==== ======== ==== ======== ==== ======== + Step Time(ms) Step Time(ms) Step Time(ms) + 0 0 11 88 22 176 + 1 8 12 96 23 184 + 2 16 13 104 24 192 + 3 24 14 112 25 200 + 4 32 15 120 26 208 + 5 40 16 128 27 216 + 6 48 17 136 28 224 + 7 56 18 144 29 232 + 8 64 19 152 30 240 + 9 72 20 160 31 248 + 10 80 21 168 + ==== ======== ==== ======== ==== ========
drivers/leds/rgb/Kconfig +11 -0
@@ -27,6 +27,17 @@ config LEDS_KTD202X To compile this driver as a module, choose M here: the module will be called leds-ktd202x. +config LEDS_NCP5623 + tristate "LED support for NCP5623" + depends on I2C + depends on OF + help + This option enables support for ON semiconductor NCP5623 + Triple Output I2C Controlled RGB LED Driver. + + To compile this driver as a module, choose M here: the module + will be called leds-ncp5623. + config LEDS_PWM_MULTICOLOR tristate "PWM driven multi-color LED Support" depends on PWM
drivers/leds/rgb/Makefile +1 -0
@@ -2,6 +2,7 @@ obj-$(CONFIG_LEDS_GROUP_MULTICOLOR) += leds-group-multicolor.o obj-$(CONFIG_LEDS_KTD202X) += leds-ktd202x.o +obj-$(CONFIG_LEDS_NCP5623) += leds-ncp5623.o obj-$(CONFIG_LEDS_PWM_MULTICOLOR) += leds-pwm-multicolor.o obj-$(CONFIG_LEDS_QCOM_LPG) += leds-qcom-lpg.o obj-$(CONFIG_LEDS_MT6370_RGB) += leds-mt6370-rgb.o
drivers/leds/rgb/leds-ncp5623.c new file +320 -0
320 lines not on this page · raw message
Signature · 1 line
2.34.1
Lee Jones
maintainerRe: [PATCH 2/2] leds: Add NCP5623 multi-led driver
Quoted 1 line · Abdel Alkuor
NCP5623 is DC-DC multi-LEDs driver with 94% peak
50 char lines look odd. Please expand.
Quoted 23 lines
efficiency. NCP5623 has three PWMs which can be programmed up to 32 steps giving 32768 colors hue.
NCP5623 driver supports gradual dimming upward/downward with programmable delay steps. Also, the driver supports driving a single LED or multi-LED like RGB.
Signed-off-by Abdel Alkuor <[email protected]>--- .../sysfs-class-led-multicolor-driver-ncp5623 | 46 +++ drivers/leds/rgb/Kconfig | 11 + drivers/leds/rgb/Makefile | 1 + drivers/leds/rgb/leds-ncp5623.c | 320 ++++++++++++++++++ 4 files changed, 378 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 create mode 100644 drivers/leds/rgb/leds-ncp5623.cDocumentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 new file +0
-0
Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 line 1
Quoted 1 line
Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 new file +1
-0@@ -0,0 +1,46 @@ +What: /sys/class/leds/<led>/direction
Not sure I understand this interface.
Can't we take the direction by comparing the old and new brightness?
Quoted 15 lines
Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 new file +15
-0+Date: Feb 2024 +KernelVersion: 6.8 +Contact: Abdel Alkuor <[email protected]> +Description: + Set gradual dimming direction which + can be either none, up, or down. + After setting the direction, brightness + can be set using one of 31 steps. + + ==== ======== ==== ======== ==== ======== + Step ILED(mA) Step ILED(mA) Step ILED(mA) + 0 0 11 1.38 22 3.06 + 1 0.92 12 1.45 23 3.45 + 2 0.95 13 1.53 24 3.94 + 3 0.98 14 1.62 25 4.60
Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 line 26
Quoted 10 lines
Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 new file +10
-0+ 4 1.02 15 1.72 26 5.52 + 5 1.06 16 1.84 27 6.90 + 6 1.10 17 1.97 28 9.20 + 7 1.15 18 2.12 29 13.80 + 8 1.20 19 2.30 30 27.60 + 9 1.25 20 2.50 31 27.60 + 10 1.31 21 2.76 + ==== ======== ==== ======== ==== ======== + +What: /sys/class/leds/<led>/dim_step
The step principle seems a bit arbitrary.
Why not provide the time directly?
dim_step_delay?
I already see documentation for risetime and falltime.
Perhaps that will omit the need for both direction and step?
Quoted 60 lines
Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 new file +20
-0+Date: Feb 2024 +KernelVersion: 6.8 +Contact: Abdel Alkuor <[email protected]> +Description: + Set gradual dimming time. + + ==== ======== ==== ======== ==== ======== + Step Time(ms) Step Time(ms) Step Time(ms) + 0 0 11 88 22 176 + 1 8 12 96 23 184 + 2 16 13 104 24 192 + 3 24 14 112 25 200 + 4 32 15 120 26 208 + 5 40 16 128 27 216 + 6 48 17 136 28 224 + 7 56 18 144 29 232 + 8 64 19 152 30 240 + 9 72 20 160 31 248 + 10 80 21 168 + ==== ======== ==== ======== ==== ========drivers/leds/rgb/Kconfig +11
-0@@ -27,6 +27,17 @@ config LEDS_KTD202X To compile this driver as a module, choose M here: the module will be called leds-ktd202x. +config LEDS_NCP5623 + tristate "LED support for NCP5623" + depends on I2C + depends on OF + help + This option enables support for ON semiconductor NCP5623 + Triple Output I2C Controlled RGB LED Driver. + + To compile this driver as a module, choose M here: the module + will be called leds-ncp5623. + config LEDS_PWM_MULTICOLOR tristate "PWM driven multi-color LED Support" depends on PWMdrivers/leds/rgb/Makefile +1
-0@@ -2,6 +2,7 @@ obj-$(CONFIG_LEDS_GROUP_MULTICOLOR) += leds-group-multicolor.o obj-$(CONFIG_LEDS_KTD202X) += leds-ktd202x.o +obj-$(CONFIG_LEDS_NCP5623) += leds-ncp5623.o obj-$(CONFIG_LEDS_PWM_MULTICOLOR) += leds-pwm-multicolor.o obj-$(CONFIG_LEDS_QCOM_LPG) += leds-qcom-lpg.o obj-$(CONFIG_LEDS_MT6370_RGB) += leds-mt6370-rgb.odrivers/leds/rgb/leds-ncp5623.c new file +0
-0
drivers/leds/rgb/leds-ncp5623.c line 3
Quoted 3 lines
drivers/leds/rgb/leds-ncp5623.c new file +3
-0@@ -0,0 +1,320 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* +* leds-ncp5623.c - Multi-LED Driver
Remove filenames they have a habit of becoming out of sync.
Quoted 20 lines
drivers/leds/rgb/leds-ncp5623.c new file +20
-0+* Author: Abdel Alkuor <[email protected]> +* Datasheet: https://www.onsemi.com/pdf/datasheet/ncp5623-d.pdf +*/ + +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/sysfs.h> +#include <linux/workqueue.h> + +#include <linux/led-class-multicolor.h> + +#define NCP5623_REG (x) ((x) << 0x5) + +#define NCP5623_SHUTDOWN_REG NCP5623_REG(0x0) +#define NCP5623_ILED_REG NCP5623_REG(0x1) +#define NCP5623_PWM_REG (index) NCP5623_REG(0x2 + (index)) +#define NCP5623_UPWARD_STEP_REG NCP5623_REG(0x5) +#define NCP5623_DOWNWARD_STEP_REG NCP5623_REG(0x6) +#define NCP5623_DIMMING_TIME_REG NCP5623_REG(0x7) +
drivers/leds/rgb/leds-ncp5623.c line 34
Quoted 10 lines
drivers/leds/rgb/leds-ncp5623.c new file +10
-0+#define NCP5623_MAX_BRIGHTNESS 0x1f + +enum { +NCP5623_DIR_NONE , +NCP5623_DIR_UPWARD , +NCP5623_DIR_DOWNWARD , +}; + +static const char *const directions[] = { + [NCP5623_DIR_NONE ] ="none" ,
What does "none" do?
Quoted 13 lines
drivers/leds/rgb/leds-ncp5623.c new file +13
-0+ [NCP5623_DIR_UPWARD] = "up", + [NCP5623_DIR_DOWNWARD] = "down", +}; + +struct ncp5623 { +struct i2c_client *client; +struct led_classdev_mc mc_dev; +struct mutex lock; + +u8 direction; +u8 dim_step; +u8 old_brightness; +unsigned long delay;
drivers/leds/rgb/leds-ncp5623.c line 57
Quoted 10 lines
drivers/leds/rgb/leds-ncp5623.c new file +10
-0+}; + +static int ncp5623_write (struct i2c_client *client,u8 reg,u8 data) +{ +return i2c_smbus_write_byte_data (client, reg | data,0 ); +} + +static inline unsigned long +ncp5623_get_completion_steps (u8 dir,int brightness,int old_brightness) +{
Why not pass in ncp and omit dir and old_brightness?
Quoted 7 lines
drivers/leds/rgb/leds-ncp5623.c new file +7
-0+int diff = brightness - old_brightness; + +if (dir ==NCP5623_DIR_UPWARD ) +return diff <=1 ?NCP5623_MAX_BRIGHTNESS + diff : diff; + +return diff >=-1 ?NCP5623_MAX_BRIGHTNESS - diff : -diff; +
drivers/leds/rgb/leds-ncp5623.c line 74
Quoted 10 lines
drivers/leds/rgb/leds-ncp5623.c new file +10
-0+} + +static int ncp5623_brightness_set (struct led_classdev *cdev, +enum led_brightness brightness) +{ +struct led_classdev_mc *mc_cdev =lcdev_to_mccdev (cdev); + struct ncp5623 *ncp = container_of(mc_cdev, struct ncp5623, mc_dev); +int ret; +u8 reg; +int i;
+
drivers/leds/rgb/leds-ncp5623.c line 83
Quoted 8 lines
drivers/leds/rgb/leds-ncp5623.c new file +8
-0+guard (mutex)(&ncp->lock); + +if (ncp->delay &&time_is_after_jiffies (ncp->delay)) +return -EBUSY ; + + ncp->delay =0 ; + +for (i =0 ; i < mc_cdev->num_colors; i++) {
Looks like C++ style "for (int i" just became popular.
Quoted 26 lines
drivers/leds/rgb/leds-ncp5623.c new file +26
-0+ ret =ncp5623_write (ncp->client, +NCP5623_PWM_REG (mc_cdev->subled_info[i].channel), +min (mc_cdev->subled_info[i].intensity, +NCP5623_MAX_BRIGHTNESS )); +if (ret) +return ret; + } + +switch (ncp->direction) { +case NCP5623_DIR_NONE : + reg =NCP5623_ILED_REG ; +break ; +case NCP5623_DIR_UPWARD : + reg =NCP5623_UPWARD_STEP_REG ; +break ; +case NCP5623_DIR_DOWNWARD : + reg =NCP5623_DOWNWARD_STEP_REG ; +break ; +default : +return -EINVAL ; + } + + ret =ncp5623_write (ncp->client, reg, brightness); +if (ret) +return ret; +
drivers/leds/rgb/leds-ncp5623.c line 119
Quoted 10 lines
drivers/leds/rgb/leds-ncp5623.c new file +10
-0+if ((ncp->direction !=NCP5623_DIR_NONE ) && ncp->dim_step) { + ret =ncp5623_write (ncp->client, +NCP5623_DIMMING_TIME_REG , ncp->dim_step); +if (ret) +return ret; + + ncp->delay =ncp5623_get_completion_steps (ncp->direction, + brightness, + ncp->old_brightness); +/* dim step resolution is 8ms */
Start comments with a capital letter.
Quoted 48 lines
drivers/leds/rgb/leds-ncp5623.c new file +48
-0+ ncp->delay *= ncp->dim_step *8 ; + ncp->delay =msecs_to_jiffies (ncp->delay) + jiffies; + } + + ncp->old_brightness = brightness; + +return 0 ; +} + +static ssize_t dim_step_store (struct device *dev, +struct device_attribute *attr, +const char *buf,size_t count) +{ +struct led_classdev_mc *mc_cdev =lcdev_to_mccdev (dev_get_drvdata (dev)); + struct ncp5623 *ncp = container_of(mc_cdev, struct ncp5623, mc_dev); +u8 value; +int ret; + + ret =kstrtou8 (buf,0 , &value); +if (ret) +return ret; + +if (value >0x1f ) +return -EINVAL ; + +guard (mutex)(&ncp->lock); + ncp->dim_step = value; + +return count; +} + +static ssize_t dim_step_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +struct led_classdev_mc *mc_cdev =lcdev_to_mccdev (dev_get_drvdata (dev)); + struct ncp5623 *ncp = container_of(mc_cdev, struct ncp5623, mc_dev); + +guard (mutex)(&ncp->lock); + +return sysfs_emit (buf,"%u\n" , ncp->dim_step); +} + +static ssize_t direction_store (struct device *dev, +struct device_attribute *attr, +const char *buf,size_t count) +{ +struct led_classdev_mc *mc_cdev =lcdev_to_mccdev (dev_get_drvdata (dev)); + struct ncp5623 *ncp = container_of(mc_cdev, struct ncp5623, mc_dev);
drivers/leds/rgb/leds-ncp5623.c line 177
Quoted 10 lines
drivers/leds/rgb/leds-ncp5623.c new file +10
-0+int ret; + + ret =__sysfs_match_string (directions,ARRAY_SIZE (directions), buf); + +switch (ret) { +case NCP5623_DIR_NONE : +case NCP5623_DIR_UPWARD : +case NCP5623_DIR_DOWNWARD : +guard (mutex)(&ncp->lock); + ncp->direction = ret;
What's stopping you setting reg here?
Quoted 8 lines
drivers/leds/rgb/leds-ncp5623.c new file +8
-0+return count; +default : +return -EINVAL ; + } +} + +static ssize_t direction_show (struct device *dev, +struct device_attribute *attr,char *buf)
drivers/leds/rgb/leds-ncp5623.c line 195
Quoted 10 lines
drivers/leds/rgb/leds-ncp5623.c new file +10
-0+{ +struct led_classdev_mc *mc_cdev =lcdev_to_mccdev (dev_get_drvdata (dev)); + struct ncp5623 *ncp = container_of(mc_cdev, struct ncp5623, mc_dev); + +guard (mutex)(&ncp->lock); + +return sysfs_emit (buf,"%s\n" , directions[ncp->direction]); +} + +
Too many '\n's
Quoted 13 lines
drivers/leds/rgb/leds-ncp5623.c new file +13
-0+static DEVICE_ATTR_RW (direction); +static DEVICE_ATTR_RW (dim_step); + +static struct attribute *ncp5623_led_attrs[] = { + &dev_attr_direction.attr, + &dev_attr_dim_step.attr, +NULL +}; + +static const struct attribute_group ncp5623_led_group = { + .attrs = ncp5623_led_attrs, +}; +
drivers/leds/rgb/leds-ncp5623.c line 218
Quoted 10 lines
drivers/leds/rgb/leds-ncp5623.c new file +10
-0+static int ncp5623_probe (struct i2c_client *client) +{ +struct device *dev = &client->dev; +struct fwnode_handle *mc_node, *led_node; +struct led_init_data init_data = { }; +struct ncp5623 *ncp; +struct mc_subled *subled_info; +u32 color_index; +u32 reg; +int count =0 ;
num_subleds?
Quoted 69 lines
drivers/leds/rgb/leds-ncp5623.c new file +69
-0+int ret; + + ncp =devm_kzalloc (dev,sizeof (*ncp),GFP_KERNEL ); +if (!ncp) +return -ENOMEM ; + + ncp->client = client; + + mc_node =device_get_named_child_node (dev,"multi-led" ); +if (!mc_node) +return -EINVAL ; + +fwnode_for_each_child_node (mc_node, led_node) + count++; + + subled_info =devm_kcalloc (dev, count,sizeof (*subled_info),GFP_KERNEL ); +if (!subled_info) { + ret = -ENOMEM ; + goto release_mc_node; + } + +fwnode_for_each_available_child_node (mc_node, led_node) { + ret =fwnode_property_read_u32 (led_node,"color" , &color_index); +if (ret) { +fwnode_handle_put (led_node); + goto release_mc_node; + } + + ret =fwnode_property_read_u32 (led_node,"reg" , ®); +if (ret) { +fwnode_handle_put (led_node); + goto release_mc_node; + } + + subled_info[ncp->mc_dev.num_colors].channel = reg; + subled_info[ncp->mc_dev.num_colors++].color_index = color_index; + } + + init_data.fwnode = mc_node; + + ncp->mc_dev.led_cdev.max_brightness =NCP5623_MAX_BRIGHTNESS ; + ncp->mc_dev.subled_info = subled_info; + ncp->mc_dev.led_cdev.brightness_set_blocking = ncp5623_brightness_set; + +mutex_init (&ncp->lock); +i2c_set_clientdata (client, ncp); + + ret =devm_led_classdev_multicolor_register_ext (dev, &ncp->mc_dev, &init_data); +if (ret) + goto destroy_lock; + + ret =sysfs_update_group (&ncp->mc_dev.led_cdev.dev->kobj, &ncp5623_led_group); +if (ret) + goto destroy_lock; + +fwnode_handle_put (mc_node); + +return 0 ; + +destroy_lock: +mutex_destroy (&ncp->lock); + +release_mc_node: +fwnode_handle_put (mc_node); + +return ret; +} + +static void ncp5623_remove (struct i2c_client *client)
drivers/leds/rgb/leds-ncp5623.c line 297
Quoted 10 lines
drivers/leds/rgb/leds-ncp5623.c new file +10
-0+{ +struct ncp5623 *ncp =i2c_get_clientdata (client); + +ncp5623_write (client,NCP5623_SHUTDOWN_REG ,0 ); +mutex_destroy (&ncp->lock); +} + +static void ncp5623_shutdown (struct i2c_client *client) +{ +ncp5623_write (client,NCP5623_SHUTDOWN_REG ,0 );
No need to destroy the mutex?
Quoted 27 lines
drivers/leds/rgb/leds-ncp5623.c new file +23
-0+} + +static const struct of_device_id ncp5623_id[] = { + { .compatible ="onnn,ncp5623" }, + { } +}; +MODULE_DEVICE_TABLE (of, ncp5623_id); + +static struct i2c_driver ncp5623_i2c_driver = { + .driver = { + .name ="ncp5623" , + .of_match_table = ncp5623_id, + }, + .probe = ncp5623_probe, + .remove = ncp5623_remove, + .shutdown = ncp5623_shutdown, +}; + +module_i2c_driver (ncp5623_i2c_driver); + +MODULE_AUTHOR ("Abdel Alkuor <[email protected]>" ); +MODULE_DESCRIPTION ("NCP5623 Multi-LED driver" ); +MODULE_LICENSE ("GPL" );Signature · 1 line
2.34.1
Signature · 1 line
Lee Jones [李琼斯]
Re: [PATCH 2/2] leds: Add NCP5623 multi-led driver
Quoted 2 lines · Lee Jones
On Sat, 03 Feb 2024, Abdel Alkuor wrote:
Hi Lee,
Please check the inline comment. All other comments will be addressed in v2.
Quoted 4 lines
Quoted 1 line
+What: /sys/class/leds/<led>/dim_step
The step principle seems a bit arbitrary.
Quoted 8 lines
Why not provide the time directly?
dim_step_delay?I already see documentation for risetime and falltime.
Perhaps that will omit the need for both direction and step?
I'm going to drop off both and use risetime and falltime. That being said, the documented risetime and falltime for lm3533 use steps instead of entering the time directly. This is my first time doing this, should I document risetime/falltime in sysfs-class-led-multicolor-driver-ncp5623? or should I update risetime/falltime in sysfs-class-led-driver-lm3533 to reflect risetime/falltime for ncp5623?
Quoted 12 lines
Quoted 12 lines
Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 new file +12
-0+Date: Feb 2024 +KernelVersion: 6.8 +Contact: Abdel Alkuor <[email protected]> +Description: + Set gradual dimming time. + + ==== ======== ==== ======== ==== ======== + Step Time(ms) Step Time(ms) Step Time(ms) + 0 0 11 88 22 176 + 1 8 12 96 23 184 + 2 16 13 104 24 192 + 3 24 14 112 25 200
Quoted 8 lines
Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 line 46
Quoted 8 lines
Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 new file +8
-0+ 4 32 15 120 26 208 + 5 40 16 128 27 216 + 6 48 17 136 28 224 + 7 56 18 144 29 232 + 8 64 19 152 30 240 + 9 72 20 160 31 248 + 10 80 21 168 + ==== ======== ==== ======== ==== ========
Thanks,
Abdel
Lee Jones
maintainerRe: [PATCH 2/2] leds: Add NCP5623 multi-led driver
Quoted 18 lines · Abdel Alkuor
Quoted 2 lines · Lee Jones
On Sat, 03 Feb 2024, Abdel Alkuor wrote:
Hi Lee,
Please check the inline comment. All other comments will be addressed in v2.
Quoted 10 lines
Quoted 1 line
+What: /sys/class/leds/<led>/dim_step
The step principle seems a bit arbitrary.
Why not provide the time directly?
dim_step_delay?I already see documentation for risetime and falltime.
Quoted 8 lines
Quoted 2 lines
Perhaps that will omit the need for both direction and step?
I'm going to drop off both and use risetime and falltime. That being said, the documented risetime and falltime for lm3533 use steps instead of entering the time directly. This is my first time doing this, should I document risetime/falltime in sysfs-class-led-multicolor-driver-ncp5623? or should I update risetime/falltime in sysfs-class-led-driver-lm3533 to reflect risetime/falltime for ncp5623?
Keep them separate please.
Quoted 23 lines
Quoted 12 lines
Quoted 12 lines
Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 new file +12
-0+Date: Feb 2024 +KernelVersion: 6.8 +Contact: Abdel Alkuor <[email protected]> +Description: + Set gradual dimming time. + + ==== ======== ==== ======== ==== ======== + Step Time(ms) Step Time(ms) Step Time(ms) + 0 0 11 88 22 176 + 1 8 12 96 23 184 + 2 16 13 104 24 192 + 3 24 14 112 25 200Quoted 8 lines
Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 line 46
Quoted 8 lines
Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 new file +8
-0+ 4 32 15 120 26 208 + 5 40 16 128 27 216 + 6 48 17 136 28 224 + 7 56 18 144 29 232 + 8 64 19 152 30 240 + 9 72 20 160 31 248 + 10 80 21 168 + ==== ======== ==== ======== ==== ========Thanks,
Abdel
Signature · 1 line
Lee Jones [李琼斯]
Re: [PATCH 2/2] leds: Add NCP5623 multi-led driver
In reply to Abdel AlkuorHi!
Quoted 14 lines
.../sysfs-class-led-multicolor-driver-ncp5623 | 46 +++ drivers/leds/rgb/Kconfig | 11 + drivers/leds/rgb/Makefile | 1 + drivers/leds/rgb/leds-ncp5623.c | 320 ++++++++++++++++++ 4 files changed, 378 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 create mode 100644 drivers/leds/rgb/leds-ncp5623.cDocumentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 new file +0
-0
Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 line 3
Quoted 3 lines
Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 new file +3
-0@@ -0,0 +1,46 @@ +What: /sys/class/leds/<led>/direction +Date: Feb 2024 +What: /sys/class/leds/<led>/dim_step
You are reinventing hardware_pattern trigger. NAK.
I suggest you add basic support first, then look at hardware pattern trigger and add that support in separate patch.
Thanks,
Pavel
Signature · 1 line
People of Russia, stop Putin before his war on Ukraine escalates.
Re: [PATCH 2/2] leds: Add NCP5623 multi-led driver
On Tue, Feb 13, 2024 at 10:18:39AM +0100, Pavel Machek wrote:
Hi Pavel,
Quoted 7 lines
Quoted 1 line
+What: /sys/class/leds/<led>/dim_step
You are reinventing
hardware_patterntrigger. NAK.I suggest you add basic support first, then look at hardware pattern trigger and add that support in separate patch.
This makes a lot of sense and simplifies the driver.
I'd like to add the hardware pattern as part of this patch as it is already implemented previously but I needed to do some clean up :)
Thanks,
Abdel
Quoted 3 lines · Abdel Alkuor
NCP5623 is DC-DC multi-LED controller which can be used for RGB illumination or backlight LCD display. NCP5623 provides 94% peak efficiency.
Drop marketing.
Please use subject prefixes matching the subsystem. You can get them for example with git log --oneline -- DIRECTORY_OR_FILE on the directory your patch is touching.
It's dt-bindings.
Quoted 15 lines
Signed-off-by Abdel Alkuor <[email protected]>--- .../bindings/leds/onnn,ncp5623.yaml | 98 +++++++++++++++++++ 1 file changed, 98 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/onnn,ncp5623.yamlDocumentation/devicetree/bindings/leds/onnn,ncp5623.yaml new file +2
-0@@ -0,0 +1,98 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2
Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml line 12
Quoted 10 lines
Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml new file +10
-0+--- +$id: http://devicetree.org/schemas/leds/onnn,ncp5623.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ON Semiconductor NCP5623 multi-LED Driver + +maintainers: + - Abdel Alkuor <[email protected]> + +description: |
Do not need '|' unless you need to preserve formatting.
Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml line 22
Quoted 10 lines
Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml new file +10
-0+ NCP5623 Triple Output I2C Controlled LED Driver. + https://www.onsemi.com/pdf/datasheet/ncp5623-d.pdf + +properties: + compatible: + enum: + - onnn,ncp5623 + + reg: + enum:
Instead "const", or just maxItems: 1
Quoted 13 lines
Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml new file +13
-0+ - 0x38 + + multi-led: + type: object + $ref: leds-class-multicolor.yaml# + unevaluatedProperties: false + + properties: + "#address-cells": + const: 1 + + "#size-cells": + const: 0
Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml line 45
Quoted 10 lines
Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml new file +10
-0+ + patternProperties: + "^led@[0-2]$": + type: object + $ref: common.yaml# + unevaluatedProperties: false + + properties: + reg: + description: Index of the LED.
Drop description, it is obvious.
Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml line 55
Quoted 10 lines
Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml new file +10
-0+ minimum: 0 + maximum: 2 + + required: + - reg + - color + + required: + - "#address-cells" + - "#size-cells"
Best regards,
Krzysztof