On Sat, 03 Feb 2024, Abdel Alkuor wrote: > NCP5623 is DC-DC multi-LEDs driver with 94% peak 50 char lines look odd. Please expand. > 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 <alkuor@xxxxxxxxx> > --- > .../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 > > diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 b/Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 > new file mode 100644 > index 000000000000..6b9f4849852b > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 > @@ -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? > +Date: Feb 2024 > +KernelVersion: 6.8 > +Contact: Abdel Alkuor <alkuor@xxxxxxxxx> > +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 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? > +Date: Feb 2024 > +KernelVersion: 6.8 > +Contact: Abdel Alkuor <alkuor@xxxxxxxxx> > +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 > + ==== ======== ==== ======== ==== ======== > diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig > index a6a21f564673..81ab6a526a78 100644 > --- a/drivers/leds/rgb/Kconfig > +++ b/drivers/leds/rgb/Kconfig > @@ -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 > diff --git a/drivers/leds/rgb/Makefile b/drivers/leds/rgb/Makefile > index 243f31e4d70d..a501fd27f179 100644 > --- a/drivers/leds/rgb/Makefile > +++ b/drivers/leds/rgb/Makefile > @@ -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 > diff --git a/drivers/leds/rgb/leds-ncp5623.c b/drivers/leds/rgb/leds-ncp5623.c > new file mode 100644 > index 000000000000..e77dfca69ca3 > --- /dev/null > +++ b/drivers/leds/rgb/leds-ncp5623.c > @@ -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. > + * Author: Abdel Alkuor <alkuor@xxxxxxxxx> > + * 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) > + > +#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? > + [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; > +}; > + > +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? > + 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; > + > +} > + > +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; + > + 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. > + 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; > + > + 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. > + 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); > + 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? > + return count; > + default: > + return -EINVAL; > + } > +} > + > +static ssize_t direction_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, "%s\n", directions[ncp->direction]); > +} > + > + Too many '\n's > +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, > +}; > + > +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? > + 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) > +{ > + 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? > +} > + > +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 <alkuor@xxxxxxxxx>"); > +MODULE_DESCRIPTION("NCP5623 Multi-LED driver"); > +MODULE_LICENSE("GPL"); > -- > 2.34.1 > > -- Lee Jones [李琼斯]
