| Message ID | 20181219162626.12297-1-dmurphy@ti.com |
|---|---|
| Headers | show |
| Series | LP5024/18 LED introduction | expand |
On 12/19/2018 10:26 AM, Dan Murphy wrote: > Introduce the LP5024 and LP5018 RGB LED driver. > The difference in these 2 parts are only in the number of > LED outputs where the LP5024 can control 24 LEDs the LP5018 > can only control 18. > > The device has the ability to group LED output into control banks > so that multiple LED banks can be controlled with the same mixing and > brightness. Inversely the LEDs can also be controlled independently. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > drivers/leds/Kconfig | 7 + > drivers/leds/Makefile | 1 + > drivers/leds/leds-lp5024.c | 610 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 618 insertions(+) > create mode 100644 drivers/leds/leds-lp5024.c > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index a72f97fca57b..d306bedb00b7 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -326,6 +326,13 @@ config LEDS_LP3952 > To compile this driver as a module, choose M here: the > module will be called leds-lp3952. > > +config LEDS_LP5024 > + tristate "LED Support for TI LP5024/18 LED driver chip" > + depends on LEDS_CLASS && REGMAP_I2C > + help > + If you say yes here you get support for the Texas Instruments > + LP5024 and LP5018 LED driver. > + > config LEDS_LP55XX_COMMON > tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501" > depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501 > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index 4c1b0054f379..60b4e4ddd3ee 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -32,6 +32,7 @@ obj-$(CONFIG_LEDS_GPIO_REGISTER) += leds-gpio-register.o > obj-$(CONFIG_LEDS_GPIO) += leds-gpio.o > obj-$(CONFIG_LEDS_LP3944) += leds-lp3944.o > obj-$(CONFIG_LEDS_LP3952) += leds-lp3952.o > +obj-$(CONFIG_LEDS_LP5024) += leds-lp5024.o > obj-$(CONFIG_LEDS_LP55XX_COMMON) += leds-lp55xx-common.o > obj-$(CONFIG_LEDS_LP5521) += leds-lp5521.o > obj-$(CONFIG_LEDS_LP5523) += leds-lp5523.o > diff --git a/drivers/leds/leds-lp5024.c b/drivers/leds/leds-lp5024.c > new file mode 100644 > index 000000000000..90e8dca15609 > --- /dev/null > +++ b/drivers/leds/leds-lp5024.c > @@ -0,0 +1,610 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* TI LP50XX LED chip family driver > + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ > + */ > + > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/leds.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of.h> > +#include <linux/of_gpio.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <linux/slab.h> > +#include <uapi/linux/uleds.h> > + > +#define LP5024_DEV_CFG0 0x00 > +#define LP5024_DEV_CFG1 0x01 > +#define LP5024_LED_CFG0 0x02 I have 2 more devices that are the same as this one the LP5036/5030. The major difference is the register map and some of the register offsets. As well as enabling the control bank for the extra LEDs. The delta starts here as the new devices add LED_CFG1 at 0x03 and then at LED7_BRT there are another 4 registers and then for the OUTX_CLR there are 12 more registers. So this pushes the RESET register way out there. So how should I handle this? I kicked around creating a new driver with the register and offset differences called the LP5036. Or is there some example on how to plug those devices in as well with a different reg map? I think a framework is over kill. Dan > +#define LP5024_BNK_BRT 0x03 > +#define LP5024_BNKA_CLR 0x04 > +#define LP5024_BNKB_CLR 0x05 > +#define LP5024_BNKC_CLR 0x06 > +#define LP5024_LED0_BRT 0x07 > +#define LP5024_LED1_BRT 0x08 > +#define LP5024_LED2_BRT 0x09 > +#define LP5024_LED3_BRT 0x0a > +#define LP5024_LED4_BRT 0x0b > +#define LP5024_LED5_BRT 0x0c > +#define LP5024_LED6_BRT 0x0d > +#define LP5024_LED7_BRT 0x0e > + > +#define LP5024_OUT0_CLR 0x0f > +#define LP5024_OUT1_CLR 0x10 > +#define LP5024_OUT2_CLR 0x11 > +#define LP5024_OUT3_CLR 0x12 > +#define LP5024_OUT4_CLR 0x13 > +#define LP5024_OUT5_CLR 0x14 > +#define LP5024_OUT6_CLR 0x15 > +#define LP5024_OUT7_CLR 0x16 > +#define LP5024_OUT8_CLR 0x17 > +#define LP5024_OUT9_CLR 0x18 > +#define LP5024_OUT10_CLR 0x19 > +#define LP5024_OUT11_CLR 0x1a > +#define LP5024_OUT12_CLR 0x1b > +#define LP5024_OUT13_CLR 0x1c > +#define LP5024_OUT14_CLR 0x1d > +#define LP5024_OUT15_CLR 0x1e > +#define LP5024_OUT16_CLR 0x1f > +#define LP5024_OUT17_CLR 0x20 > +#define LP5024_OUT18_CLR 0x21 > +#define LP5024_OUT19_CLR 0x22 > +#define LP5024_OUT20_CLR 0x23 > +#define LP5024_OUT21_CLR 0x24 > +#define LP5024_OUT22_CLR 0x25 > +#define LP5024_OUT23_CLR 0x26 > + > +#define LP5024_RESET 0x27 > +#define LP5024_SW_RESET 0xff > + > +#define LP5024_CHIP_EN BIT(6) > + > +#define LP5024_CONTROL_A 0 > +#define LP5024_CONTROL_B 1 > +#define LP5024_CONTROL_C 2 > +#define LP5024_MAX_CONTROL_BANKS 3 > + > +#define LP5018_MAX_LED_STRINGS 6 > +#define LP5024_MAX_LED_STRINGS 8 > + > +enum lp5024_model { > + LP5018, > + LP5024, > +}; > + > +struct lp5024_led { > + u32 led_strings[LP5024_MAX_LED_STRINGS]; > + char label[LED_MAX_NAME_SIZE]; > + struct led_classdev led_dev; > + struct lp5024 *priv; > + int led_number; > + u8 ctrl_bank_enabled; > +}; > + > +/** > + * struct lp5024 - > + * @enable_gpio: Hardware enable gpio > + * @regulator: LED supply regulator pointer > + * @client: Pointer to the I2C client > + * @regmap: Devices register map > + * @dev: Pointer to the devices device struct > + * @lock: Lock for reading/writing the device > + * @model_id: ID of the device > + * @leds: Array of LED strings > + */ > +struct lp5024 { > + struct gpio_desc *enable_gpio; > + struct regulator *regulator; > + struct i2c_client *client; > + struct regmap *regmap; > + struct device *dev; > + struct mutex lock; > + int model_id; > + int max_leds; > + int num_of_leds; > + > + /* This needs to be at the end of the struct */ > + struct lp5024_led leds[]; > +}; > + > +static const struct reg_default lp5024_reg_defs[] = { > + {LP5024_DEV_CFG0, 0x0}, > + {LP5024_DEV_CFG1, 0x3c}, > + {LP5024_BNK_BRT, 0xff}, > + {LP5024_BNKA_CLR, 0x0f}, > + {LP5024_BNKB_CLR, 0x0f}, > + {LP5024_BNKC_CLR, 0x0f}, > + {LP5024_LED0_BRT, 0x0f}, > + {LP5024_LED1_BRT, 0xff}, > + {LP5024_LED2_BRT, 0xff}, > + {LP5024_LED3_BRT, 0xff}, > + {LP5024_LED4_BRT, 0xff}, > + {LP5024_LED5_BRT, 0xff}, > + {LP5024_LED6_BRT, 0xff}, > + {LP5024_LED7_BRT, 0xff}, > + {LP5024_OUT0_CLR, 0x0f}, > + {LP5024_OUT1_CLR, 0x00}, > + {LP5024_OUT2_CLR, 0x00}, > + {LP5024_OUT3_CLR, 0x00}, > + {LP5024_OUT4_CLR, 0x00}, > + {LP5024_OUT5_CLR, 0x00}, > + {LP5024_OUT6_CLR, 0x00}, > + {LP5024_OUT7_CLR, 0x00}, > + {LP5024_OUT8_CLR, 0x00}, > + {LP5024_OUT9_CLR, 0x00}, > + {LP5024_OUT10_CLR, 0x00}, > + {LP5024_OUT11_CLR, 0x00}, > + {LP5024_OUT12_CLR, 0x00}, > + {LP5024_OUT13_CLR, 0x00}, > + {LP5024_OUT14_CLR, 0x00}, > + {LP5024_OUT15_CLR, 0x00}, > + {LP5024_OUT16_CLR, 0x00}, > + {LP5024_OUT17_CLR, 0x00}, > + {LP5024_OUT18_CLR, 0x00}, > + {LP5024_OUT19_CLR, 0x00}, > + {LP5024_OUT20_CLR, 0x00}, > + {LP5024_OUT21_CLR, 0x00}, > + {LP5024_OUT22_CLR, 0x00}, > + {LP5024_OUT23_CLR, 0x00}, > + {LP5024_RESET, 0x00} > +}; > + > +static const struct regmap_config lp5024_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + > + .max_register = LP5024_RESET, > + .reg_defaults = lp5024_reg_defs, > + .num_reg_defaults = ARRAY_SIZE(lp5024_reg_defs), > + .cache_type = REGCACHE_RBTREE, > +}; > + > +static int lp5024_set_color_mix(struct lp5024_led *led, u8 color_reg, > + u8 color_val) > +{ > + return regmap_write(led->priv->regmap, color_reg, color_val); > +} > + > + > +static ssize_t ctrl_bank_a_mix_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct lp5024_led *led = container_of(led_cdev, struct lp5024_led, > + led_dev); > + u8 mix_value; > + int ret; > + > + ret = kstrtou8(buf, 0, &mix_value); > + if (ret) > + return ret; > + > + lp5024_set_color_mix(led, LP5024_BNKA_CLR, mix_value); > + > + return size; > +} > +static ssize_t ctrl_bank_b_mix_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct lp5024_led *led = container_of(led_cdev, struct lp5024_led, > + led_dev); > + u8 mix_value; > + int ret; > + > + ret = kstrtou8(buf, 0, &mix_value); > + if (ret) > + return ret; > + > + lp5024_set_color_mix(led, LP5024_BNKB_CLR, mix_value); > + > + return size; > +} > +static ssize_t ctrl_bank_c_mix_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct lp5024_led *led = container_of(led_cdev, struct lp5024_led, > + led_dev); > + u8 mix_value; > + int ret; > + > + ret = kstrtou8(buf, 0, &mix_value); > + if (ret) > + return ret; > + > + lp5024_set_color_mix(led, LP5024_BNKC_CLR, mix_value); > + > + return size; > +} > + > +static DEVICE_ATTR_WO(ctrl_bank_a_mix); > +static DEVICE_ATTR_WO(ctrl_bank_b_mix); > +static DEVICE_ATTR_WO(ctrl_bank_c_mix); > + > +static struct attribute *lp5024_ctrl_bank_attrs[] = { > + &dev_attr_ctrl_bank_a_mix.attr, > + &dev_attr_ctrl_bank_b_mix.attr, > + &dev_attr_ctrl_bank_c_mix.attr, > + NULL > +}; > +ATTRIBUTE_GROUPS(lp5024_ctrl_bank); > + > +static ssize_t led3_mix_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct lp5024_led *led = container_of(led_cdev, struct lp5024_led, > + led_dev); > + u8 mix_value; > + u8 reg_value; > + int ret; > + > + ret = kstrtou8(buf, 0, &mix_value); > + if (ret) > + return ret; > + > + reg_value = (led->led_number * 3) + LP5024_OUT2_CLR; > + > + lp5024_set_color_mix(led, reg_value, mix_value); > + > + return size; > +} > + > +static ssize_t led2_mix_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct lp5024_led *led = container_of(led_cdev, struct lp5024_led, > + led_dev); > + u8 mix_value; > + u8 reg_value; > + int ret; > + > + ret = kstrtou8(buf, 0, &mix_value); > + if (ret) > + return ret; > + > + reg_value = (led->led_number * 3) + LP5024_OUT1_CLR; > + > + lp5024_set_color_mix(led, reg_value, mix_value); > + > + return size; > +} > + > +static ssize_t led1_mix_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct lp5024_led *led = container_of(led_cdev, struct lp5024_led, > + led_dev); > + u8 mix_value; > + u8 reg_value; > + int ret; > + > + ret = kstrtou8(buf, 0, &mix_value); > + if (ret) > + return ret; > + > + reg_value = (led->led_number * 3) + LP5024_OUT0_CLR; > + > + lp5024_set_color_mix(led, reg_value, mix_value); > + > + return size; > +} > + > +static DEVICE_ATTR_WO(led1_mix); > +static DEVICE_ATTR_WO(led2_mix); > +static DEVICE_ATTR_WO(led3_mix); > + > +static struct attribute *lp5024_led_independent_attrs[] = { > + &dev_attr_led1_mix.attr, > + &dev_attr_led2_mix.attr, > + &dev_attr_led3_mix.attr, > + NULL > +}; > +ATTRIBUTE_GROUPS(lp5024_led_independent); > + > +static int lp5024_brightness_set(struct led_classdev *led_cdev, > + enum led_brightness brt_val) > +{ > + struct lp5024_led *led = container_of(led_cdev, struct lp5024_led, > + led_dev); > + int ret = 0; > + u8 reg_val; > + > + mutex_lock(&led->priv->lock); > + > + if (led->ctrl_bank_enabled) > + reg_val = LP5024_BNK_BRT; > + else > + reg_val = led->led_number + LP5024_LED0_BRT; > + > + ret = regmap_write(led->priv->regmap, reg_val, brt_val); > + > + mutex_unlock(&led->priv->lock); > + > + return ret; > +} > + > +static enum led_brightness lp5024_brightness_get(struct led_classdev *led_cdev) > +{ > + struct lp5024_led *led = container_of(led_cdev, struct lp5024_led, > + led_dev); > + unsigned int brt_val; > + u8 reg_val; > + int ret; > + > + mutex_lock(&led->priv->lock); > + > + if (led->ctrl_bank_enabled) > + reg_val = LP5024_BNK_BRT; > + else > + reg_val = led->led_number + LP5024_LED0_BRT; > + > + ret = regmap_read(led->priv->regmap, reg_val, &brt_val); > + > + mutex_unlock(&led->priv->lock); > + > + return brt_val; > +} > + > +static int lp5024_set_led_values(struct lp5024 *priv) > +{ > + struct lp5024_led *led; > + int i, j; > + u8 led_ctrl_enable = 0; > + > + for (i = 0; i <= priv->num_of_leds; i++) { > + led = &priv->leds[i]; > + if (led->ctrl_bank_enabled) { > + for (j = 0; j <= LP5024_MAX_LED_STRINGS - 1; j++) > + led_ctrl_enable |= (1 << led->led_strings[j]); > + } > + } > + > + regmap_write(priv->regmap, LP5024_LED_CFG0, led_ctrl_enable); > + > + return 0; > +} > + > +static int lp5024_init(struct lp5024 *priv) > +{ > + int ret; > + > + if (priv->enable_gpio) { > + gpiod_direction_output(priv->enable_gpio, 1); > + } else { > + ret = regmap_write(priv->regmap, LP5024_RESET, LP5024_SW_RESET); > + if (ret) { > + dev_err(&priv->client->dev, > + "Cannot reset the device\n"); > + goto out; > + } > + } > + > + ret = lp5024_set_led_values(priv); > + if (ret) > + dev_err(&priv->client->dev, "Setting the CRTL bank failed\n"); > + > + ret = regmap_write(priv->regmap, LP5024_DEV_CFG0, LP5024_CHIP_EN); > + if (ret) { > + dev_err(&priv->client->dev, "Cannot write ctrl enable\n"); > + goto out; > + } > +out: > + return ret; > +} > + > +static int lp5024_probe_dt(struct lp5024 *priv) > +{ > + struct fwnode_handle *child = NULL; > + struct lp5024_led *led; > + const char *name; > + int led_number; > + size_t i = 0; > + int ret; > + > + priv->enable_gpio = devm_gpiod_get_optional(&priv->client->dev, > + "enable", GPIOD_OUT_LOW); > + if (IS_ERR(priv->enable_gpio)) { > + ret = PTR_ERR(priv->enable_gpio); > + dev_err(&priv->client->dev, "Failed to get enable gpio: %d\n", > + ret); > + return ret; > + } > + > + priv->regulator = devm_regulator_get(&priv->client->dev, "vled"); > + if (IS_ERR(priv->regulator)) > + priv->regulator = NULL; > + > + if (priv->model_id == LP5018) > + priv->max_leds = LP5018_MAX_LED_STRINGS; > + else > + priv->max_leds = LP5024_MAX_LED_STRINGS; > + > + device_for_each_child_node(&priv->client->dev, child) { > + led = &priv->leds[i]; > + > + if (fwnode_property_present(child, "ti,control-bank")) > + led->ctrl_bank_enabled = 1; > + else > + led->ctrl_bank_enabled = 0; > + > + if (led->ctrl_bank_enabled) { > + ret = fwnode_property_read_u32_array(child, > + "led-sources", > + NULL, 0); > + ret = fwnode_property_read_u32_array(child, > + "led-sources", > + led->led_strings, > + ret); > + > + led->led_number = led->led_strings[0]; > + > + } else { > + ret = fwnode_property_read_u32(child, "led-sources", > + &led_number); > + > + led->led_number = led_number; > + } > + if (ret) { > + dev_err(&priv->client->dev, > + "led-sources property missing\n"); > + fwnode_handle_put(child); > + goto child_out; > + } > + > + if (led_number > priv->max_leds) { > + dev_err(&priv->client->dev, > + "led-sources property is invalid\n"); > + ret = -EINVAL; > + fwnode_handle_put(child); > + goto child_out; > + } > + > + ret = fwnode_property_read_string(child, "label", &name); > + if (ret) > + snprintf(led->label, sizeof(led->label), > + "%s::", priv->client->name); > + else > + snprintf(led->label, sizeof(led->label), > + "%s:%s", priv->client->name, name); > + > + fwnode_property_read_string(child, "linux,default-trigger", > + &led->led_dev.default_trigger); > + > + led->priv = priv; > + led->led_dev.name = led->label; > + led->led_dev.max_brightness = 255; > + led->led_dev.brightness_set_blocking = lp5024_brightness_set; > + led->led_dev.brightness_get = lp5024_brightness_get; > + > + if (led->ctrl_bank_enabled) > + led->led_dev.groups = lp5024_ctrl_bank_groups; > + else > + led->led_dev.groups = lp5024_led_independent_groups; > + > + ret = devm_led_classdev_register(&priv->client->dev, > + &led->led_dev); > + if (ret) { > + dev_err(&priv->client->dev, "led register err: %d\n", > + ret); > + fwnode_handle_put(child); > + goto child_out; > + } > + i++; > + } > + priv->num_of_leds = i; > + > +child_out: > + return ret; > +} > + > +static int lp5024_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct lp5024 *led; > + int count; > + int ret; > + > + count = device_get_child_node_count(&client->dev); > + if (!count) { > + dev_err(&client->dev, "LEDs are not defined in device tree!"); > + return -ENODEV; > + } > + > + led = devm_kzalloc(&client->dev, struct_size(led, leds, count), > + GFP_KERNEL); > + if (!led) > + return -ENOMEM; > + > + mutex_init(&led->lock); > + led->client = client; > + led->dev = &client->dev; > + led->model_id = id->driver_data; > + i2c_set_clientdata(client, led); > + > + led->regmap = devm_regmap_init_i2c(client, &lp5024_regmap_config); > + if (IS_ERR(led->regmap)) { > + ret = PTR_ERR(led->regmap); > + dev_err(&client->dev, "Failed to allocate register map: %d\n", > + ret); > + return ret; > + } > + > + ret = lp5024_probe_dt(led); > + if (ret) > + return ret; > + > + ret = lp5024_init(led); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int lp5024_remove(struct i2c_client *client) > +{ > + struct lp5024 *led = i2c_get_clientdata(client); > + int ret; > + > + ret = regmap_update_bits(led->regmap, LP5024_DEV_CFG0, > + LP5024_CHIP_EN, 0); > + if (ret) { > + dev_err(&led->client->dev, "Failed to disable regulator\n"); > + return ret; > + } > + > + if (led->enable_gpio) > + gpiod_direction_output(led->enable_gpio, 0); > + > + if (led->regulator) { > + ret = regulator_disable(led->regulator); > + if (ret) > + dev_err(&led->client->dev, > + "Failed to disable regulator\n"); > + } > + > + mutex_destroy(&led->lock); > + > + return 0; > +} > + > +static const struct i2c_device_id lp5024_id[] = { > + { "lp5018", LP5018 }, > + { "lp5024", LP5024 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, lp5024_id); > + > +static const struct of_device_id of_lp5024_leds_match[] = { > + { .compatible = "ti,lp5018", }, > + { .compatible = "ti,lp5024", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, of_lp5024_leds_match); > + > +static struct i2c_driver lp5024_driver = { > + .driver = { > + .name = "lp5024", > + .of_match_table = of_lp5024_leds_match, > + }, > + .probe = lp5024_probe, > + .remove = lp5024_remove, > + .id_table = lp5024_id, > +}; > +module_i2c_driver(lp5024_driver); > + > +MODULE_DESCRIPTION("Texas Instruments LP5024 LED driver"); > +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>"); > +MODULE_LICENSE("GPL v2"); > -- ------------------ Dan Murphy
Hi! > +static DEVICE_ATTR_WO(ctrl_bank_a_mix); > +static DEVICE_ATTR_WO(ctrl_bank_b_mix); > +static DEVICE_ATTR_WO(ctrl_bank_c_mix); > + > +static struct attribute *lp5024_ctrl_bank_attrs[] = { > + &dev_attr_ctrl_bank_a_mix.attr, > + &dev_attr_ctrl_bank_b_mix.attr, > + &dev_attr_ctrl_bank_c_mix.attr, > + NULL > +}; > +ATTRIBUTE_GROUPS(lp5024_ctrl_bank); ... > + > +static DEVICE_ATTR_WO(led1_mix); > +static DEVICE_ATTR_WO(led2_mix); > +static DEVICE_ATTR_WO(led3_mix); > + > +static struct attribute *lp5024_led_independent_attrs[] = { > + &dev_attr_led1_mix.attr, > + &dev_attr_led2_mix.attr, > + &dev_attr_led3_mix.attr, > + NULL > +}; > +ATTRIBUTE_GROUPS(lp5024_led_independent); Ok, so you are adding new sysfs files. Are they _really_ neccessary? We'd like to have uniform interfaces for the leds. If they are neccessary, we need documentation for them. Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Pavel Thanks for the review. On 12/19/2018 01:34 PM, Pavel Machek wrote: > Hi! > >> +static DEVICE_ATTR_WO(ctrl_bank_a_mix); >> +static DEVICE_ATTR_WO(ctrl_bank_b_mix); >> +static DEVICE_ATTR_WO(ctrl_bank_c_mix); >> + >> +static struct attribute *lp5024_ctrl_bank_attrs[] = { >> + &dev_attr_ctrl_bank_a_mix.attr, >> + &dev_attr_ctrl_bank_b_mix.attr, >> + &dev_attr_ctrl_bank_c_mix.attr, >> + NULL >> +}; >> +ATTRIBUTE_GROUPS(lp5024_ctrl_bank); > > ... >> + >> +static DEVICE_ATTR_WO(led1_mix); >> +static DEVICE_ATTR_WO(led2_mix); >> +static DEVICE_ATTR_WO(led3_mix); >> + >> +static struct attribute *lp5024_led_independent_attrs[] = { >> + &dev_attr_led1_mix.attr, >> + &dev_attr_led2_mix.attr, >> + &dev_attr_led3_mix.attr, >> + NULL >> +}; >> +ATTRIBUTE_GROUPS(lp5024_led_independent); > > Ok, so you are adding new sysfs files. Are they _really_ neccessary? > We'd like to have uniform interfaces for the leds. Yes I am adding these for this driver. They adjust the individual brightness of each LED connected (what the HW guys called mixing). The standard brightness sysfs adjusts all 3 LEDs simultaneously so that all 3 LEDs brightness are uniform. I did not think these belonged in the dt as they should be user space configurable > > If they are neccessary, we need documentation for them. Sure I have no problem documenting them but where do I do that? I am assuming Documentation/leds/leds-lp5024.txt This looks to be where Milo did this before. Dan > > Thanks, > Pavel > -- ------------------ Dan Murphy
Vesa On 12/20/2018 06:40 AM, Vesa Jääskeläinen wrote: > Hi All, > > On 19/12/2018 23.50, Dan Murphy wrote: >> On 12/19/2018 03:36 PM, Jacek Anaszewski wrote: >>> Hi Dan and Pavel, >>> Some time ago we had discussion with Vesa Jääskeläinen about possible >>> approaches to RGB LEDs [0]. What seemed to be the most suitable >>> variation of the discussed out-of-tree approach was the "color" property >>> and array of color triplets defined in Device Tree per each color. >>> >> >> Why does Device tree define the color? >> >> Rob indicated that Device tree is supposed to define the hardware. >> This thread seems to be defining the operation. >> >> Shouldn't the color be done via user space and not dt? >> >> Especially if they want to change the color real time? >> >> Dan >> >>> Please refer to [0] for the details. >>> >>> [0] https://lkml.org/lkml/2018/11/9/938 >>> > > Idea was to define preset colors in device tree as an example when you are dealing with multi-color LEDs without PWM. In that case you only have GPIOs to control and then have a problem what does those GPIO's mean. > > With preset definitions one can use color names to act as a shortcut to configure GPIO's to proper state for that particular color. > > For more flexible setups where you have PWM or such control you have larger space of available colors. In this case you need to somehow define also meaning of those controls. > > Also we may not have LED with only red, green and blue elements. There might in example be amber, ultraviolet, white elements. > > This is where device tree is concerned. It helps us craft the logical definition for LED so that we can control it from user space in common way. > > Now the next problem then is how does user space work then. > > For multi-color LEDs it it important to change the color atomically so that no wrong colors are being shown as user space got interrupted when controlling it. > > Also we have brightness setting that would be useful for PWM controlled LEDs. > > Setting color is easy when you use preset names then you only need to deal with brightness value (eg. RGB -> HSV * brightness -> RGB). Of course here additional problem is other color elements are they then scaled according to brightness value?. > > Setting color as "raw" values is then next problem. In order to do it atomically it needs to be one atomic activation and could be eg. one write to "color" sysfs entry with combination of all color elements and perhaps additionally also brightness value. Next question is then what is the format for such entry then? What are the value ranges? In here we can utilize device tree definition to help define what kind of LED we do have and what kind of capabilities it does have. > > Additional problem risen also in discussion was non-linearity of some control mechanisms vs. perceived color. So there might be a need for curve mapping similarly what is with backlight control and that would be defined either in device tree and possibly in user space if there is a need for that. I suppose golden curve definition in device tree should be good enough. > > Then there was additional discussion about possible animation support but I would leave that for future design as that would then be utilizing the same framework. > > I suppose color space handling and that kind of stuff should be in some led core functionality and then raw control should be part of physical led driver. > > I was planning to play with it during holiday season but lets see how it goes. Feel free to also experiment with the idea. > Again I don't think device tree should be used to set color policy. This is to restricting it may be good for GPIO fixed current RGB LEDs but for variable RGB LEDs it would be very restricting. I believe this needs to be part of the LED framework and leave the device tree to define the HW and not define the product. Maybe a new devm_rgb_register call that exposes the color palette and can consolidate the 3 LEDs into a single sysfs node. Dan > Thanks, > Vesa Jääskeläinen -- ------------------ Dan Murphy
On 12/21/2018 01:32 AM, Jacek Anaszewski wrote: > On 12/20/18 9:31 PM, Jacek Anaszewski wrote: >> On 12/19/18 10:50 PM, Dan Murphy wrote: >>> On 12/19/2018 03:36 PM, Jacek Anaszewski wrote: >>>> Hi Dan and Pavel, >>>> >>>> On 12/19/18 9:41 PM, Dan Murphy wrote: >>>>> Pavel >>>>> >>>>> On 12/19/2018 02:10 PM, Pavel Machek wrote: >>>>>> On Wed 2018-12-19 13:41:18, Dan Murphy wrote: >>>>>>> Pavel >>>>>>> >>>>>>> Thanks for the review. >>>>>>> >>>>>>> On 12/19/2018 01:34 PM, Pavel Machek wrote: >>>>>>>> Hi! >>>>>>>> >>>>>>>>> +static DEVICE_ATTR_WO(ctrl_bank_a_mix); >>>>>>>>> +static DEVICE_ATTR_WO(ctrl_bank_b_mix); >>>>>>>>> +static DEVICE_ATTR_WO(ctrl_bank_c_mix); >>>>>>>>> + >>>>>>>>> +static struct attribute *lp5024_ctrl_bank_attrs[] = { >>>>>>>>> + &dev_attr_ctrl_bank_a_mix.attr, >>>>>>>>> + &dev_attr_ctrl_bank_b_mix.attr, >>>>>>>>> + &dev_attr_ctrl_bank_c_mix.attr, >>>>>>>>> + NULL >>>>>>>>> +}; >>>>>>>>> +ATTRIBUTE_GROUPS(lp5024_ctrl_bank); >>>>>>>> >>>>>>>> ... >>>>>>>>> + >>>>>>>>> +static DEVICE_ATTR_WO(led1_mix); >>>>>>>>> +static DEVICE_ATTR_WO(led2_mix); >>>>>>>>> +static DEVICE_ATTR_WO(led3_mix); >>>>>>>>> + >>>>>>>>> +static struct attribute *lp5024_led_independent_attrs[] = { >>>>>>>>> + &dev_attr_led1_mix.attr, >>>>>>>>> + &dev_attr_led2_mix.attr, >>>>>>>>> + &dev_attr_led3_mix.attr, >>>>>>>>> + NULL >>>>>>>>> +}; >>>>>>>>> +ATTRIBUTE_GROUPS(lp5024_led_independent); >>>>>>>> >>>>>>>> Ok, so you are adding new sysfs files. Are they _really_ neccessary? >>>>>>>> We'd like to have uniform interfaces for the leds. >>>>>>> >>>>>>> Yes I am adding these for this driver. >>>>>>> They adjust the individual brightness of each LED connected (what the HW guys called mixing). >>>>>>> >>>>>>> The standard brightness sysfs adjusts all 3 LEDs simultaneously so that all 3 LEDs brightness are >>>>>>> uniform. >>>>>> >>>>>> 1 LED, 1 brightness file... that's what we do. Why should this one be different? >>>>>> >>>>> >>>>> Yes I understand this and 1 DT child node per LED out but... >>>>> >>>>> This device has a single register to control 3 LEDs brightness as a group and individual brightness >>>>> registers to control the LEDs independently to mix the LEDs to a specific color. >>>>> >>>>> There needs to be a way to control both so that developers can mix and adjust the individual RGB and >>>>> then control the brightness of the group during run time without touching the "mixing" value. >>>>> >>>>> I don't think a user needs nor would want to have 24 different LED nodes with 24 different brightness files. >>>>> Or with the LP5036 that would have 36 LED nodes. >>>>> >>>>> Table 1 in the data sheet shows how the outputs map to the control banks to the LED registers. >>>> >>>> Some time ago we had discussion with Vesa Jääskeläinen about possible >>>> approaches to RGB LEDs [0]. What seemed to be the most suitable >>>> variation of the discussed out-of-tree approach was the "color" property >>>> and array of color triplets defined in Device Tree per each color. >>>> >>> >>> Why does Device tree define the color? >>> >>> Rob indicated that Device tree is supposed to define the hardware. >>> This thread seems to be defining the operation. >> >> Perceived colors produced by LEDs from different manufacturers may >> differ and this alone should be deemed a sufficient argument for having >> board specific color definitions. >> >>> Shouldn't the color be done via user space and not dt? >> >> I think that we should keep the userspace interface as simple >> as possible and backwards compatible with monochrome LEDs. >> >> I also propose to avoid the introduction of a color sysfs >> property in favor of creating separate LED class devices >> for different "color ranges". The devices would drive the same >> LED but using different preset color levels. > > On the other hand, scattering the control over the hardware > among multiple LED class devices would complicate extension > of pattern trigger with the support for RGB LEDs. > > I looks like we will need the "color" sysfs file anyway. > >> We don't have to expose all device knobs to the userspace, >> but instead provide some predefined configurations. It would >> improve user experience by keeping LED class devices simple >> in use. It would be Device Tree designer's responsibility to >> provide color definitions that make sense for given RGB LED >> controller and RGB LED element configuration. >> >> Registering color palette with devm_rgb_register() you proposed >> is also an option, but with one LED class device per color palette >> it would mean allowing for creation/destruction of LED class >> devices by any user having access to given LED's sysfs interface, >> which is really bad solution. > > With the "color" sysfs file it will make more sense to allow for user > defined color palettes. > I think defining these values in the device tree or acpi severely limits the devices capabilities. Especially in development phases. If the knobs were exposed then the user space can create new experiences. The color definition should be an absolute color defined in the dt and either the framework or user space needs to mix these appropriately. IMO user space should set the policy of the user experience and the dt/acpi needs to set the capabilities. I do like Pavels idea on defining the more standard binding pattern to "group" leds into a single group. Maybe the framework could take these groups and combine/group them into a single node with the groups colors. Dan -- ------------------ Dan Murphy
Hi! > >>With the "color" sysfs file it will make more sense to allow for user > >>defined color palettes. > >> > > > >I think defining these values in the device tree or acpi severely limits the devices > >capabilities. Especially in development phases. If the knobs were exposed then the user space > >can create new experiences. The color definition should be an absolute color defined in the dt and > >either the framework or user space needs to mix these appropriately. IMO user space should set the policy > >of the user experience and the dt/acpi needs to set the capabilities. > > > >I do like Pavels idea on defining the more standard binding pattern to "group" leds into a single group. > > > >Maybe the framework could take these groups and combine/group them into a single node with the groups colors. > > There is still HSV approach [0] in store. One problem with proposed > implementation is fixed algorithm of RGB <-> HSV color space conversion. > Maybe allowing for some board specific adjustments in DT would add > more flexibility. > > [0] https://lkml.org/lkml/2017/8/31/255 Yes we could do HSV. Problem is that that we do not really have RGB available. We do have integers for red, green and blue, but they do not correspond to RGB colorspace. Anyway, this should not be driver specific; all drivers should use one interface. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sun 2018-12-30 18:09:35, Jacek Anaszewski wrote: > On 12/29/18 8:07 PM, Pavel Machek wrote: > >Hi! > > > >>>>With the "color" sysfs file it will make more sense to allow for user > >>>>defined color palettes. > >>>> > >>> > >>>I think defining these values in the device tree or acpi severely limits the devices > >>>capabilities. Especially in development phases. If the knobs were exposed then the user space > >>>can create new experiences. The color definition should be an absolute color defined in the dt and > >>>either the framework or user space needs to mix these appropriately. IMO user space should set the policy > >>>of the user experience and the dt/acpi needs to set the capabilities. > >>> > >>>I do like Pavels idea on defining the more standard binding pattern to "group" leds into a single group. > >>> > >>>Maybe the framework could take these groups and combine/group them into a single node with the groups colors. > >> > >>There is still HSV approach [0] in store. One problem with proposed > >>implementation is fixed algorithm of RGB <-> HSV color space conversion. > >>Maybe allowing for some board specific adjustments in DT would add > >>more flexibility. > >> > >>[0] https://lkml.org/lkml/2017/8/31/255 > > > >Yes we could do HSV. Problem is that that we do not really have RGB > >available. We do have integers for red, green and blue, but they do > >not correspond to RGB colorspace. > > OK, so conversion from HSV to RGB would only increase the aberration. > So, let's stick to RGB - we've got to have some stable ground and this > is something that the hardware at least pretends to be compliant >with. I'm not saying that we should stick to RGB. I'm just saying that problem is complex. And no, hardware does not even pretend to be compliant with RGB color model ( https://en.wikipedia.org/wiki/RGB_color_model ). In particular, in RGB there is non-linear brightness curve. > Our problem is how to set the color atomically. With HSV approach we > were to obviate the problem by mapping brightness file to the "V" > component of that color space, and write all H,S and V values to the > hardware only on write to brightness file. I'm not sure how realistic the "atomic color" problem is. Computers are way faster than human vision. I believe problem to start with is the "white" problem. Setting R=G=B=255 will _not_ result in anything close to white light on hardware I have. Anyway, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 12/30/18 6:35 PM, Pavel Machek wrote: > On Sun 2018-12-30 18:09:35, Jacek Anaszewski wrote: >> On 12/29/18 8:07 PM, Pavel Machek wrote: >>> Hi! >>> >>>>>> With the "color" sysfs file it will make more sense to allow for user >>>>>> defined color palettes. >>>>>> >>>>> >>>>> I think defining these values in the device tree or acpi severely limits the devices >>>>> capabilities. Especially in development phases. If the knobs were exposed then the user space >>>>> can create new experiences. The color definition should be an absolute color defined in the dt and >>>>> either the framework or user space needs to mix these appropriately. IMO user space should set the policy >>>>> of the user experience and the dt/acpi needs to set the capabilities. >>>>> >>>>> I do like Pavels idea on defining the more standard binding pattern to "group" leds into a single group. >>>>> >>>>> Maybe the framework could take these groups and combine/group them into a single node with the groups colors. >>>> >>>> There is still HSV approach [0] in store. One problem with proposed >>>> implementation is fixed algorithm of RGB <-> HSV color space conversion. >>>> Maybe allowing for some board specific adjustments in DT would add >>>> more flexibility. >>>> >>>> [0] https://lkml.org/lkml/2017/8/31/255 >>> >>> Yes we could do HSV. Problem is that that we do not really have RGB >>> available. We do have integers for red, green and blue, but they do >>> not correspond to RGB colorspace. >> >> OK, so conversion from HSV to RGB would only increase the aberration. >> So, let's stick to RGB - we've got to have some stable ground and this >> is something that the hardware at least pretends to be compliant >> with. > > I'm not saying that we should stick to RGB. I'm just saying that > problem is complex. > > And no, hardware does not even pretend to be compliant with RGB color > model ( https://en.wikipedia.org/wiki/RGB_color_model ). In > particular, in RGB there is non-linear brightness curve. Quotation from the wiki page you referred to: "RGB is a device-dependent color model: different devices detect or reproduce a given RGB value differently, since the color elements (such as phosphors or dyes) and their response to the individual R, G, and B levels vary from manufacturer to manufacturer, or even in the same device over time. Thus an RGB value does not define the same color across devices without some kind of color management" This claim alone leaves much room for the manufacturers to pretend that their devices are compliant with RGB model. And the documentation of the hardware the discussed driver is for also refers to RGB model in many places - e.g. see Table 1, page 15 in the document [0], where mapping of output triplets to an RGB module is shown. One thing that I missed is that the discussed hardware provides LEDn_BRIGHTNESS registers for each RGB LED module, that can be configured to set color intensity in linear or logarithmic fashion. Actually this stands in contradiction with RGB model, since change of "color intensity" means change of all RGB components. We could use brightness file as for monochrome LEDs for that, but we'd need to come up with consistent interface semantics for all devices, also those which don't have corresponding functionality. Probably this is the place where we could apply some RGB<->HSV conversion, as color intensity feels something more of HSV's saturation and value. It would be good to hear from Dan how that looks in reality in case of lp5024 device. >> Our problem is how to set the color atomically. With HSV approach we >> were to obviate the problem by mapping brightness file to the "V" >> component of that color space, and write all H,S and V values to the >> hardware only on write to brightness file. > > I'm not sure how realistic the "atomic color" problem is. Computers > are way faster than human vision. With LEDn_BRIGHTNESS registers of lp5024 it seems that we need the ability for grouping LEDs in triplets and be able to set their intensity with a single write operation. > I believe problem to start with is the "white" problem. Setting > R=G=B=255 will _not_ result in anything close to white light on > hardware I have. RGBW LED controllers solve this problem. For the devices without white/amber we cannot do more than the hardware allows for. [0] http://www.ti.com/lit/ds/symlink/lp5024.pdf -- Best regards, Jacek Anaszewski
On 12/31/18 4:43 PM, Jacek Anaszewski wrote: > On 12/30/18 6:35 PM, Pavel Machek wrote: >> On Sun 2018-12-30 18:09:35, Jacek Anaszewski wrote: >>> On 12/29/18 8:07 PM, Pavel Machek wrote: >>>> Hi! >>>> >>>>>>> With the "color" sysfs file it will make more sense to allow for >>>>>>> user >>>>>>> defined color palettes. >>>>>>> >>>>>> >>>>>> I think defining these values in the device tree or acpi severely >>>>>> limits the devices >>>>>> capabilities. Especially in development phases. If the knobs >>>>>> were exposed then the user space >>>>>> can create new experiences. The color definition should be an >>>>>> absolute color defined in the dt and >>>>>> either the framework or user space needs to mix these >>>>>> appropriately. IMO user space should set the policy >>>>>> of the user experience and the dt/acpi needs to set the capabilities. >>>>>> >>>>>> I do like Pavels idea on defining the more standard binding >>>>>> pattern to "group" leds into a single group. >>>>>> >>>>>> Maybe the framework could take these groups and combine/group them >>>>>> into a single node with the groups colors. >>>>> >>>>> There is still HSV approach [0] in store. One problem with proposed >>>>> implementation is fixed algorithm of RGB <-> HSV color space >>>>> conversion. >>>>> Maybe allowing for some board specific adjustments in DT would add >>>>> more flexibility. >>>>> >>>>> [0] https://lkml.org/lkml/2017/8/31/255 >>>> >>>> Yes we could do HSV. Problem is that that we do not really have RGB >>>> available. We do have integers for red, green and blue, but they do >>>> not correspond to RGB colorspace. >>> >>> OK, so conversion from HSV to RGB would only increase the aberration. >>> So, let's stick to RGB - we've got to have some stable ground and this >>> is something that the hardware at least pretends to be compliant >>> with. >> >> I'm not saying that we should stick to RGB. I'm just saying that >> problem is complex. >> >> And no, hardware does not even pretend to be compliant with RGB color >> model ( https://en.wikipedia.org/wiki/RGB_color_model ). In >> particular, in RGB there is non-linear brightness curve. > > Quotation from the wiki page you referred to: > > "RGB is a device-dependent color model: different devices detect or > reproduce a given RGB value differently, since the color elements (such > as phosphors or dyes) and their response to the individual R, G, and B > levels vary from manufacturer to manufacturer, or even in the same > device over time. Thus an RGB value does not define the same color > across devices without some kind of color management" > > This claim alone leaves much room for the manufacturers to pretend that > their devices are compliant with RGB model. > > And the documentation of the hardware the discussed driver is for > also refers to RGB model in many places - e.g. see Table 1, page 15 > in the document [0], where mapping of output triplets to an RGB module > is shown. > > One thing that I missed is that the discussed hardware provides > LEDn_BRIGHTNESS registers for each RGB LED module, that can be > configured to set color intensity in linear or logarithmic fashion. > > Actually this stands in contradiction with RGB model, since > change of "color intensity" means change of all RGB components. > > We could use brightness file as for monochrome LEDs for that, Here I mean brightness file in addition to the previously proposed red, green and blue files. > but we'd need to come up with consistent interface semantics > for all devices, also those which don't have corresponding > functionality. Probably this is the place where we could apply > some RGB<->HSV conversion, as color intensity feels something > more of HSV's saturation and value. > > It would be good to hear from Dan how that looks in reality > in case of lp5024 device. > >>> Our problem is how to set the color atomically. With HSV approach we >>> were to obviate the problem by mapping brightness file to the "V" >>> component of that color space, and write all H,S and V values to the >>> hardware only on write to brightness file. >> >> I'm not sure how realistic the "atomic color" problem is. Computers >> are way faster than human vision. > > With LEDn_BRIGHTNESS registers of lp5024 it seems that we need the > ability for grouping LEDs in triplets and be able to set their intensity > with a single write operation. > >> I believe problem to start with is the "white" problem. Setting >> R=G=B=255 will _not_ result in anything close to white light on >> hardware I have. > > RGBW LED controllers solve this problem. For the devices without > white/amber we cannot do more than the hardware allows for. > > [0] http://www.ti.com/lit/ds/symlink/lp5024.pdf > -- Best regards, Jacek Anaszewski
On 12/31/18 9:47 AM, Jacek Anaszewski wrote: > On 12/31/18 4:43 PM, Jacek Anaszewski wrote: >> On 12/30/18 6:35 PM, Pavel Machek wrote: >>> On Sun 2018-12-30 18:09:35, Jacek Anaszewski wrote: >>>> On 12/29/18 8:07 PM, Pavel Machek wrote: >>>>> Hi! >>>>> >>>>>>>> With the "color" sysfs file it will make more sense to allow for user >>>>>>>> defined color palettes. >>>>>>>> >>>>>>> >>>>>>> I think defining these values in the device tree or acpi severely limits the devices >>>>>>> capabilities. Especially in development phases. If the knobs were exposed then the user space >>>>>>> can create new experiences. The color definition should be an absolute color defined in the dt and >>>>>>> either the framework or user space needs to mix these appropriately. IMO user space should set the policy >>>>>>> of the user experience and the dt/acpi needs to set the capabilities. >>>>>>> >>>>>>> I do like Pavels idea on defining the more standard binding pattern to "group" leds into a single group. >>>>>>> >>>>>>> Maybe the framework could take these groups and combine/group them into a single node with the groups colors. >>>>>> >>>>>> There is still HSV approach [0] in store. One problem with proposed >>>>>> implementation is fixed algorithm of RGB <-> HSV color space conversion. >>>>>> Maybe allowing for some board specific adjustments in DT would add >>>>>> more flexibility. >>>>>> >>>>>> [0] https://lkml.org/lkml/2017/8/31/255 >>>>> >>>>> Yes we could do HSV. Problem is that that we do not really have RGB >>>>> available. We do have integers for red, green and blue, but they do >>>>> not correspond to RGB colorspace. >>>> >>>> OK, so conversion from HSV to RGB would only increase the aberration. >>>> So, let's stick to RGB - we've got to have some stable ground and this >>>> is something that the hardware at least pretends to be compliant >>>> with. >>> >>> I'm not saying that we should stick to RGB. I'm just saying that >>> problem is complex. >>> >>> And no, hardware does not even pretend to be compliant with RGB color >>> model ( https://en.wikipedia.org/wiki/RGB_color_model ). In >>> particular, in RGB there is non-linear brightness curve. >> >> Quotation from the wiki page you referred to: >> >> "RGB is a device-dependent color model: different devices detect or >> reproduce a given RGB value differently, since the color elements (such >> as phosphors or dyes) and their response to the individual R, G, and B >> levels vary from manufacturer to manufacturer, or even in the same >> device over time. Thus an RGB value does not define the same color >> across devices without some kind of color management" >> >> This claim alone leaves much room for the manufacturers to pretend that >> their devices are compliant with RGB model. >> >> And the documentation of the hardware the discussed driver is for >> also refers to RGB model in many places - e.g. see Table 1, page 15 >> in the document [0], where mapping of output triplets to an RGB module >> is shown. >> >> One thing that I missed is that the discussed hardware provides >> LEDn_BRIGHTNESS registers for each RGB LED module, that can be >> configured to set color intensity in linear or logarithmic fashion. >> >> Actually this stands in contradiction with RGB model, since >> change of "color intensity" means change of all RGB components. >> >> We could use brightness file as for monochrome LEDs for that, > > Here I mean brightness file in addition to the previously proposed > red, green and blue files. > >> but we'd need to come up with consistent interface semantics >> for all devices, also those which don't have corresponding >> functionality. Probably this is the place where we could apply >> some RGB<->HSV conversion, as color intensity feels something >> more of HSV's saturation and value. >> >> It would be good to hear from Dan how that looks in reality >> in case of lp5024 device. Sorry for the non-response I have had a passing in my family and have not been at my computer for some time. I am not seeing how HSV will fit into this device. Not sure what the V is in HSV. I am still not a fan of defining colors in the kernel. I think the user space needs to do this based on information it is given. When I look at Android the user space sets all the policies of the hardware the kernel just provides the vehicle to hardware. I think defining any set colors in the kernel for devices that have a full color spectrum palette is very restricting. The kernel should indicate the absolute colors available and not the colors that are allowed. So in this case we indicate that a Red, green and blue LED are available or that the palette is variable. Or in the case of a white LED driver we just say white. In the case of this device there are RGB outputs that are grouped in clusters and controlled by the LEDn_BRIGHTNESS register. This is what the brightness file is mapped to. Within that cluster the individual intensity of the RGB can be modified via the OUTn_color register. Not knowing what color LED is on what output means the sysfs node has to be left generic. So as Pavel pointed out white would need to be achieved through the RGB individual LEDs being set to certain values and the difuser disfusing the light to achieve the color. This was done on the original DROID device with a RGB and we were able to get a "white" color but had to set the RGB LEDs to different values. For this device, once the color is achieved there may be no reason to adjust the color so adjusting the overall brightness of the LEDs without adjusting the individual color can be done with a single write and look seamless to the user. Or other colors can be tuned by setting the unneeded colors in the OUTn_color to 0. These RGB LED clusters can also be grouped into LED banks as well so that all LEDs of the same color within the group will have the same color gradient and brightness. This is achieved with the BANK_X_COLOR and BRIGHTNESS registers. Again I am not sure how the HSV would work for this device since there is no reason to create a node for each LED output. As the overall brightness of the cluster or bank is controlled by a single brightness file. Dan >> >>>> Our problem is how to set the color atomically. With HSV approach we >>>> were to obviate the problem by mapping brightness file to the "V" >>>> component of that color space, and write all H,S and V values to the >>>> hardware only on write to brightness file. >>> >>> I'm not sure how realistic the "atomic color" problem is. Computers >>> are way faster than human vision. >> >> With LEDn_BRIGHTNESS registers of lp5024 it seems that we need the >> ability for grouping LEDs in triplets and be able to set their intensity >> with a single write operation. >> >>> I believe problem to start with is the "white" problem. Setting >>> R=G=B=255 will _not_ result in anything close to white light on >>> hardware I have. >> >> RGBW LED controllers solve this problem. For the devices without >> white/amber we cannot do more than the hardware allows for. >> >> [0] http://www.ti.com/lit/ds/symlink/lp5024.pdf >> > -- ------------------ Dan Murphy
Hi Pavel, On 12/31/18 5:28 PM, Pavel Machek wrote: > Hi! > >>>>>> There is still HSV approach [0] in store. One problem with proposed >>>>>> implementation is fixed algorithm of RGB <-> HSV color space conversion. >>>>>> Maybe allowing for some board specific adjustments in DT would add >>>>>> more flexibility. >>>>>> >>>>>> [0] https://lkml.org/lkml/2017/8/31/255 >>>>> >>>>> Yes we could do HSV. Problem is that that we do not really have RGB >>>>> available. We do have integers for red, green and blue, but they do >>>>> not correspond to RGB colorspace. >>>> >>>> OK, so conversion from HSV to RGB would only increase the aberration. >>>> So, let's stick to RGB - we've got to have some stable ground and this >>>> is something that the hardware at least pretends to be compliant >>>> with. >>> >>> I'm not saying that we should stick to RGB. I'm just saying that >>> problem is complex. >>> >>> And no, hardware does not even pretend to be compliant with RGB color >>> model ( https://en.wikipedia.org/wiki/RGB_color_model ). In >>> particular, in RGB there is non-linear brightness curve. >> >> Quotation from the wiki page you referred to: >> >> "RGB is a device-dependent color model: different devices detect or >> reproduce a given RGB value differently, since the color elements (such >> as phosphors or dyes) and their response to the individual R, G, and B >> levels vary from manufacturer to manufacturer, or even in the same >> device over time. Thus an RGB value does not define the same color >> across devices without some kind of color management" >> >> This claim alone leaves much room for the manufacturers to pretend that >> their devices are compliant with RGB model. > > Much room, but everyone agrees that R=G=B=255 should be some kind of > white, and what other colors "approximately" mean. > > I have two monitors, and no, colors don't match. > > Do you have access to RGB led? Try to compare two monitors, and then > RGB led with monitor. RGB led will be _way_ off. > > This chart makes sense: > > https://www.rapidtables.com/web/color/RGB_Color.html > > Try it on your LED device... > >>> I believe problem to start with is the "white" problem. Setting >>> R=G=B=255 will _not_ result in anything close to white light on >>> hardware I have. >> >> RGBW LED controllers solve this problem. For the devices without >> white/amber we cannot do more than the hardware allows for. > > But the hardware can do some kind of white. It is just that R=G=B=255 > will result in green-ish-blue and something like R=255, G=50, B=10 is > neccessary to get approximation of white. > > IMO good start would be to specify what kind of intensities are > neccessary to approximate white. And then try converting from RGB to > led intensities, and see if it somehow works. I don't have access to RGB LED, and unfortunately have lack of time currently to play with it even if I had one. In order to gain a full understanding of your idea of RGB to LED intensity conversion, we'd need to see the full algorithm. It feels like imposing some restrictions on user regarding the available scope of colors to set. -- Best regards, Jacek Anaszewski
On 12/31/18 8:15 PM, Dan Murphy wrote: > On 12/31/18 9:47 AM, Jacek Anaszewski wrote: >> On 12/31/18 4:43 PM, Jacek Anaszewski wrote: >>> On 12/30/18 6:35 PM, Pavel Machek wrote: >>>> On Sun 2018-12-30 18:09:35, Jacek Anaszewski wrote: >>>>> On 12/29/18 8:07 PM, Pavel Machek wrote: >>>>>> Hi! >>>>>> >>>>>>>>> With the "color" sysfs file it will make more sense to allow for user >>>>>>>>> defined color palettes. >>>>>>>>> >>>>>>>> >>>>>>>> I think defining these values in the device tree or acpi severely limits the devices >>>>>>>> capabilities. Especially in development phases. If the knobs were exposed then the user space >>>>>>>> can create new experiences. The color definition should be an absolute color defined in the dt and >>>>>>>> either the framework or user space needs to mix these appropriately. IMO user space should set the policy >>>>>>>> of the user experience and the dt/acpi needs to set the capabilities. >>>>>>>> >>>>>>>> I do like Pavels idea on defining the more standard binding pattern to "group" leds into a single group. >>>>>>>> >>>>>>>> Maybe the framework could take these groups and combine/group them into a single node with the groups colors. >>>>>>> >>>>>>> There is still HSV approach [0] in store. One problem with proposed >>>>>>> implementation is fixed algorithm of RGB <-> HSV color space conversion. >>>>>>> Maybe allowing for some board specific adjustments in DT would add >>>>>>> more flexibility. >>>>>>> >>>>>>> [0] https://lkml.org/lkml/2017/8/31/255 >>>>>> >>>>>> Yes we could do HSV. Problem is that that we do not really have RGB >>>>>> available. We do have integers for red, green and blue, but they do >>>>>> not correspond to RGB colorspace. >>>>> >>>>> OK, so conversion from HSV to RGB would only increase the aberration. >>>>> So, let's stick to RGB - we've got to have some stable ground and this >>>>> is something that the hardware at least pretends to be compliant >>>>> with. >>>> >>>> I'm not saying that we should stick to RGB. I'm just saying that >>>> problem is complex. >>>> >>>> And no, hardware does not even pretend to be compliant with RGB color >>>> model ( https://en.wikipedia.org/wiki/RGB_color_model ). In >>>> particular, in RGB there is non-linear brightness curve. >>> >>> Quotation from the wiki page you referred to: >>> >>> "RGB is a device-dependent color model: different devices detect or >>> reproduce a given RGB value differently, since the color elements (such >>> as phosphors or dyes) and their response to the individual R, G, and B >>> levels vary from manufacturer to manufacturer, or even in the same >>> device over time. Thus an RGB value does not define the same color >>> across devices without some kind of color management" >>> >>> This claim alone leaves much room for the manufacturers to pretend that >>> their devices are compliant with RGB model. >>> >>> And the documentation of the hardware the discussed driver is for >>> also refers to RGB model in many places - e.g. see Table 1, page 15 >>> in the document [0], where mapping of output triplets to an RGB module >>> is shown. >>> >>> One thing that I missed is that the discussed hardware provides >>> LEDn_BRIGHTNESS registers for each RGB LED module, that can be >>> configured to set color intensity in linear or logarithmic fashion. >>> >>> Actually this stands in contradiction with RGB model, since >>> change of "color intensity" means change of all RGB components. >>> >>> We could use brightness file as for monochrome LEDs for that, >> >> Here I mean brightness file in addition to the previously proposed >> red, green and blue files. >> >>> but we'd need to come up with consistent interface semantics >>> for all devices, also those which don't have corresponding >>> functionality. Probably this is the place where we could apply >>> some RGB<->HSV conversion, as color intensity feels something >>> more of HSV's saturation and value. >>> >>> It would be good to hear from Dan how that looks in reality >>> in case of lp5024 device. > > Sorry for the non-response I have had a passing in my family and have not been at my > computer for some time. Sorry to hear that. In fact there was no delay, since I wrote that yesterday. > I am not seeing how HSV will fit into this device. Not sure what the V is in HSV. I meant RGB<->HSV conversion as a fallback for RGB LED controllers that don't have the functionality similar to LEDn_BRIGHTNESS. For lp5024 we'd use just what hardware offers. You can get the flavor of the relationship between RGB and HSV using e.g. GIMP color editor. After that you could share a feedback if changing LEDn_BRIGHTNESS feels like changing saturation and value of HSV. Remember that we're still talking about generic approach to the problem. > I am still not a fan of defining colors in the kernel. I think the user space needs to do this based > on information it is given. When I look at Android the user space sets all the policies of the hardware > the kernel just provides the vehicle to hardware. I think defining any set colors in the kernel for devices > that have a full color spectrum palette is very restricting. The kernel should indicate the absolute colors > available and not the colors that are allowed. So in this case we indicate that a Red, green and blue LED are > available or that the palette is variable. Or in the case of a white LED driver we just say white. HSV is in no way restrictive. But I'm not pushing for that. Vesa in his recent message mentions some difficulties in mapping all RGB combinations to HSV. We just need something generic and don't want ledn_mix and ctrl_bank*mix sysfs files, which are device specific. > In the case of this device there are RGB outputs that are grouped in clusters and controlled by the LEDn_BRIGHTNESS > register. This is what the brightness file is mapped to. Within that cluster the individual intensity of the RGB can > be modified via the OUTn_color register. Not knowing what color LED is on what output means the sysfs node has to be left generic. > > So as Pavel pointed out white would need to be achieved through the RGB individual LEDs being set to certain values and > the difuser disfusing the light to achieve the color. This was done on the original DROID device with a RGB and we > were able to get a "white" color but had to set the RGB LEDs to different values. For this device, once the color is > achieved there may be no reason to adjust the color so adjusting the overall brightness of the LEDs without adjusting the > individual color can be done with a single write and look seamless to the user. > Or other colors can be tuned by setting the unneeded colors in the OUTn_color to 0. > > These RGB LED clusters can also be grouped into LED banks as well so that all LEDs of the same color within the group will have > the same color gradient and brightness. This is achieved with the BANK_X_COLOR and BRIGHTNESS registers. > > Again I am not sure how the HSV would work for this device since there is no reason to create a node for each LED output. > As the overall brightness of the cluster or bank is controlled by a single brightness file. I think that you misunderstood me. I didn't mention creating separate nodes for each LED output anywhere. -- Best regards, Jacek Anaszewski
On 1/1/19 7:11 PM, Dan Murphy wrote: > Jacek > > Thanks for the reply! > > All > > Happy New Year! Happy New Year! > On 1/1/19 8:42 AM, Jacek Anaszewski wrote: >> On 12/31/18 8:15 PM, Dan Murphy wrote: >>> On 12/31/18 9:47 AM, Jacek Anaszewski wrote: >>>> On 12/31/18 4:43 PM, Jacek Anaszewski wrote: >>>>> On 12/30/18 6:35 PM, Pavel Machek wrote: >>>>>> On Sun 2018-12-30 18:09:35, Jacek Anaszewski wrote: >>>>>>> On 12/29/18 8:07 PM, Pavel Machek wrote: >>>>>>>> Hi! >>>>>>>> >>>>>>>>>>> With the "color" sysfs file it will make more sense to allow for user >>>>>>>>>>> defined color palettes. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> I think defining these values in the device tree or acpi severely limits the devices >>>>>>>>>> capabilities. Especially in development phases. If the knobs were exposed then the user space >>>>>>>>>> can create new experiences. The color definition should be an absolute color defined in the dt and >>>>>>>>>> either the framework or user space needs to mix these appropriately. IMO user space should set the policy >>>>>>>>>> of the user experience and the dt/acpi needs to set the capabilities. >>>>>>>>>> >>>>>>>>>> I do like Pavels idea on defining the more standard binding pattern to "group" leds into a single group. >>>>>>>>>> >>>>>>>>>> Maybe the framework could take these groups and combine/group them into a single node with the groups colors. >>>>>>>>> >>>>>>>>> There is still HSV approach [0] in store. One problem with proposed >>>>>>>>> implementation is fixed algorithm of RGB <-> HSV color space conversion. >>>>>>>>> Maybe allowing for some board specific adjustments in DT would add >>>>>>>>> more flexibility. >>>>>>>>> >>>>>>>>> [0] https://lkml.org/lkml/2017/8/31/255 >>>>>>>> >>>>>>>> Yes we could do HSV. Problem is that that we do not really have RGB >>>>>>>> available. We do have integers for red, green and blue, but they do >>>>>>>> not correspond to RGB colorspace. >>>>>>> >>>>>>> OK, so conversion from HSV to RGB would only increase the aberration. >>>>>>> So, let's stick to RGB - we've got to have some stable ground and this >>>>>>> is something that the hardware at least pretends to be compliant >>>>>>> with. >>>>>> >>>>>> I'm not saying that we should stick to RGB. I'm just saying that >>>>>> problem is complex. >>>>>> >>>>>> And no, hardware does not even pretend to be compliant with RGB color >>>>>> model ( https://en.wikipedia.org/wiki/RGB_color_model ). In >>>>>> particular, in RGB there is non-linear brightness curve. >>>>> >>>>> Quotation from the wiki page you referred to: >>>>> >>>>> "RGB is a device-dependent color model: different devices detect or >>>>> reproduce a given RGB value differently, since the color elements (such >>>>> as phosphors or dyes) and their response to the individual R, G, and B >>>>> levels vary from manufacturer to manufacturer, or even in the same >>>>> device over time. Thus an RGB value does not define the same color >>>>> across devices without some kind of color management" >>>>> >>>>> This claim alone leaves much room for the manufacturers to pretend that >>>>> their devices are compliant with RGB model. >>>>> >>>>> And the documentation of the hardware the discussed driver is for >>>>> also refers to RGB model in many places - e.g. see Table 1, page 15 >>>>> in the document [0], where mapping of output triplets to an RGB module >>>>> is shown. >>>>> >>>>> One thing that I missed is that the discussed hardware provides >>>>> LEDn_BRIGHTNESS registers for each RGB LED module, that can be >>>>> configured to set color intensity in linear or logarithmic fashion. >>>>> >>>>> Actually this stands in contradiction with RGB model, since >>>>> change of "color intensity" means change of all RGB components. >>>>> >>>>> We could use brightness file as for monochrome LEDs for that, >>>> >>>> Here I mean brightness file in addition to the previously proposed >>>> red, green and blue files. >>>> >>>>> but we'd need to come up with consistent interface semantics >>>>> for all devices, also those which don't have corresponding >>>>> functionality. Probably this is the place where we could apply >>>>> some RGB<->HSV conversion, as color intensity feels something >>>>> more of HSV's saturation and value. >>>>> >>>>> It would be good to hear from Dan how that looks in reality >>>>> in case of lp5024 device. >>> >>> Sorry for the non-response I have had a passing in my family and have not been at my >>> computer for some time. >> >> Sorry to hear that. In fact there was no delay, since I wrote that >> yesterday. >> >>> I am not seeing how HSV will fit into this device. Not sure what the V is in HSV. >> I meant RGB<->HSV conversion as a fallback for RGB LED >> controllers that don't have the functionality similar to >> LEDn_BRIGHTNESS. For lp5024 we'd use just what hardware offers. >> >> You can get the flavor of the relationship between RGB and HSV using >> e.g. GIMP color editor. After that you could share a feedback if >> changing LEDn_BRIGHTNESS feels like changing saturation and value of >> HSV. >> >> Remember that we're still talking about generic approach to the problem. >> >>> I am still not a fan of defining colors in the kernel. I think the user space needs to do this based >>> on information it is given. When I look at Android the user space sets all the policies of the hardware >>> the kernel just provides the vehicle to hardware. I think defining any set colors in the kernel for devices >>> that have a full color spectrum palette is very restricting. The kernel should indicate the absolute colors >>> available and not the colors that are allowed. So in this case we indicate that a Red, green and blue LED are >>> available or that the palette is variable. Or in the case of a white LED driver we just say white. >> >> HSV is in no way restrictive. But I'm not pushing for that. >> Vesa in his recent message mentions some difficulties in mapping all RGB >> combinations to HSV. >> >> We just need something generic and don't want ledn_mix and >> ctrl_bank*mix sysfs files, which are device specific. > > OK that makes sense. I will have to think about how to map HSV to this device. It becomes complex because > of the way this device groups the LEDs and has a single brightness control. I got back to the HSV idea only because of this LEDn_BRIGHTNESS feature of lp5024. If it maps easily to S and V components of HSV then we would have this conversion for free for this device. Generally increasing S and V for given hue (H) results in going from grayish to more saturated version of the same hue, where the greater V, the greater "lightness". Of course we would have to make some research to compare if other RGB LED controllers available on the market approach this similarly. > > If we had a version of the HSV->RGB framework that looks to be set for application then I can present a new version with that FW being used > on top of that patch? > > Is that on linux-leds-next? We have older attempt on devel branch of linux-leds.git and a newer one by Pavel I gave a pointer to in one of previous messages in this thread. But please hold on for a while until we agree on that. There is also recent proposal from Vesa Jääskeläinen, but I haven't analyzed it in detail yet. >>> In the case of this device there are RGB outputs that are grouped in clusters and controlled by the LEDn_BRIGHTNESS >>> register. This is what the brightness file is mapped to. Within that cluster the individual intensity of the RGB can >>> be modified via the OUTn_color register. Not knowing what color LED is on what output means the sysfs node has to be left generic. >>> >>> So as Pavel pointed out white would need to be achieved through the RGB individual LEDs being set to certain values and >>> the difuser disfusing the light to achieve the color. This was done on the original DROID device with a RGB and we >>> were able to get a "white" color but had to set the RGB LEDs to different values. For this device, once the color is >>> achieved there may be no reason to adjust the color so adjusting the overall brightness of the LEDs without adjusting the >>> individual color can be done with a single write and look seamless to the user. >>> Or other colors can be tuned by setting the unneeded colors in the OUTn_color to 0. >>> >>> These RGB LED clusters can also be grouped into LED banks as well so that all LEDs of the same color within the group will have >>> the same color gradient and brightness. This is achieved with the BANK_X_COLOR and BRIGHTNESS registers. >>> >>> Again I am not sure how the HSV would work for this device since there is no reason to create a node for each LED output. >>> As the overall brightness of the cluster or bank is controlled by a single brightness file. >> >> I think that you misunderstood me. I didn't mention creating >> separate nodes for each LED output anywhere. >> > > No this was not something you indicated it was an earlier comment by Pavel, and it is which the LED file nodes are expected > as we have discussed in the past. > > Dan > -- Best regards, Jacek Anaszewski
Hi! > >Regarding led_scale_color_elements() - I checked it in GIMP and > >the results are not satisfactory when increasing brightness. > >Even if we managed to fix it, the result would not be guaranteed > >to be the same across all devices. > > No and they will never be the same. I was told by our hardware expert that > it is rather impossible to get linearly behaving LED control without special > curve fitting trimmed for particular hardware and LED component in use. And > if you go and change LED component/vendor it would need to be "calibrated" > again if such accuracy would be required. Also LEDs age and that has also > effect on this. Well, it is not possible to "perfectly" calibrate LCD monitors, either. Yet, color tables make sense for them. And we should aim for the same thing. And yes, it may mean re-doing calibration when vendor changes. And it will mean some math and some understanding of colors. And... LEDs are linear-enough as it is. That is not a problem. But RGB does _not_ expect linear response. That's why colors are _way_ off currently. > >I have another proposal, being a mix of what has been discussed so far: > > > > RGB LED class will expose following files: > > a) available by default: > > - red, green, blue > > Writing any of these file will result in writing corresponding > > device register. > > Problem with this is that we are basically back at square one and one cannot > do "atomic" color change with this. > > In order to set or activate new values one would need "load values" file or > such that when writing to it would activate new values. However it becomes > quite clumsy interface at that point as you need to handle multiple writes > to multiple files and makes those operations rather slow. If you don't like the interface, create an shared library. It may be neccessary, anyway, for the color operations. You say it is "rather slow" to change all 3 colors. How long does it take, and how long do you need it to take? > Then we have color presets left that could kinda solve the issue on setting > the color to fixed values atomically. Lets not design crazy interface "because sysfs writing is too slow". Hint: it is not. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Vesa, On 1/4/19 12:19 AM, Vesa Jääskeläinen wrote: > Hi Jacek, > > Comments below. > > On 04/01/2019 0.05, Jacek Anaszewski wrote: >> Hi Vesa, >> >> Thank you for sharing your ideas. >> >> Please find my comment below. >> >> On 1/1/19 2:45 PM, Vesa Jääskeläinen wrote: >>> Hi All, >>> >>> On 20/12/2018 14.40, Vesa Jääskeläinen wrote: >>>> Idea was to define preset colors in device tree as an example when >>>> you are dealing with multi-color LEDs without PWM. In that case you >>>> only have GPIOs to control and then have a problem what does those >>>> GPIO's mean. >>>> >>>> With preset definitions one can use color names to act as a shortcut >>>> to configure GPIO's to proper state for that particular color. >>>> >>>> For more flexible setups where you have PWM or such control you have >>>> larger space of available colors. In this case you need to somehow >>>> define also meaning of those controls. >>>> >>>> Also we may not have LED with only red, green and blue elements. >>>> There might in example be amber, ultraviolet, white elements. >>>> >>>> This is where device tree is concerned. It helps us craft the >>>> logical definition for LED so that we can control it from user space >>>> in common way. >>>> >>>> Now the next problem then is how does user space work then. >>>> >>>> For multi-color LEDs it it important to change the color atomically >>>> so that no wrong colors are being shown as user space got >>>> interrupted when controlling it. >>>> >>>> Also we have brightness setting that would be useful for PWM >>>> controlled LEDs. >>>> >>>> Setting color is easy when you use preset names then you only need >>>> to deal with brightness value (eg. RGB -> HSV * brightness -> RGB). >>>> Of course here additional problem is other color elements are they >>>> then scaled according to brightness value?. >>>> >>>> Setting color as "raw" values is then next problem. In order to do >>>> it atomically it needs to be one atomic activation and could be eg. >>>> one write to "color" sysfs entry with combination of all color >>>> elements and perhaps additionally also brightness value. Next >>>> question is then what is the format for such entry then? What are >>>> the value ranges? In here we can utilize device tree definition to >>>> help define what kind of LED we do have and what kind of >>>> capabilities it does have. >>>> >>>> Additional problem risen also in discussion was non-linearity of >>>> some control mechanisms vs. perceived color. So there might be a >>>> need for curve mapping similarly what is with backlight control and >>>> that would be defined either in device tree and possibly in user >>>> space if there is a need for that. I suppose golden curve definition >>>> in device tree should be good enough. >>>> >>>> Then there was additional discussion about possible animation >>>> support but I would leave that for future design as that would then >>>> be utilizing the same framework. >>>> >>>> I suppose color space handling and that kind of stuff should be in >>>> some led core functionality and then raw control should be part of >>>> physical led driver. >>>> >>>> I was planning to play with it during holiday season but lets see >>>> how it goes. Feel free to also experiment with the idea. >>> >>> I was playing with this and got some results with PWM LED driver. I >>> would like to get feedback now even thou it is not yet ready for >>> patch sending. >>> >>> They still need more work but the idea can be seen here: >>> https://github.com/vesajaaskelainen/linux/tree/wip-multi-color-led >>> >>> This branch is now based on Linux kernel 4.20 release. >>> >>> Consider that branch as volatile as I will forcibly update it when >>> there are updates. >>> >>> From there specifically in commits (while they last): >>> >>> drivers: leds: Add core support for multi color element LEDs >>> https://github.com/vesajaaskelainen/linux/commit/55d553906d0a158591435bb6323a318462079d59 >>> >>> >>> WIP: drivers: leds: leds-pwm: Add multi color element LED support. >>> https://github.com/vesajaaskelainen/linux/commit/efccef08cbf3b2e1e49b95b69ff81cd380519fe3 >>> >>> >>> What is there now: >>> >>> - led-core supports color elements >>> - led-class supports users space configuration >>> - both led-core and led-class are driver agnostic so they should be >>> treated as generic code. >>> - leds-pwm: my testing code with PWM led. >>> - no HSV support for brightness as there could be multiple color >>> elements out from traditional red-green-blue space or odd >>> combinations of colors and they are a bit hard to map to HSV formula >>> (and it needs fixed point math). >>> - no color presets that could be optionally be selected >>> - when I configure led trigger to heartbeat it actually blinks with >>> color specified -- thou trigger gets zeroed out with one sets new >>> color or brightness as that was previous functionality with brightness. >>> - some documentation added >>> - code should pass checkpatch >>> >>> What I was planning to do next: >>> >>> - cleanup PWM LED driver so that it works with and without >>> LED_MULTI_COLOR_LED being defined. >>> - improve documentation >>> - try out how my other device behaves which have dual color element >>> LED controlled with GPIO's and see how it would integrate to gpio-led >>> driver. >>> >>> I would like to get feedback on: >>> - Device tree idea >>> - Internal logic >>> - Should the trigger be really reseted when one changes value of >>> brightness? I would think it should function like setting brightness >>> entry from sysfs would set current brightness for trigger when it is >>> lit. Setting color should change color and brightness and it should >>> be active from there one until trigger is disabled from trigger sysfs >>> node. >>> >>> My testing device has RGB LED with all color elements controlled with >>> individual PWM channels from TI's AM335x's integrated PWM controller. >>> >>> In device tree I have following: >>> >>> multi-color-leds { >>> compatible = "pwm-leds"; >>> >>> status-led { >>> label = "status"; >>> >>> element-red { >>> pwms = <&ehrpwm0 0 100000 0>; >>> }; >>> element-green { >>> pwms = <&ehrpwm1 0 100000 0>; >>> }; >>> element-blue { >>> pwms = <&ehrpwm1 1 100000 0>; >>> }; >>> }; >>> }; >>> >>> For my second test device I was planning to replace "pwms" with >>> "gpios" or such entries. >>> >>> In user space one can use it like: >>> >>> # --- start of snippet --- >>> >>> hostname ~ # cd /sys/class/leds/ >>> hostname leds # ls >>> status >>> hostname leds # cd status >>> hostname status # ls >>> brightness color device max_brightness >>> max_color power subsystem trigger uevent >>> hostname status # cat color >>> brightness=0 red=0 green=0 blue=0 >> >> This breaks one-value-per-file sysfs rule. > > I believe you are referring to this text in: > https://www.kernel.org/doc/Documentation/filesystems/sysfs.txt > > "Attributes should be ASCII text files, preferably with only one value > per file. It is noted that it may not be efficient to contain only one > value per file, so it is socially acceptable to express an array of > values of the same type." > > I suppose if one would just make it an array of values (separated by > space) and then one file with string array of color element names and on > file with maximum value array it could be within those words. > > The it would be something like: > > $ echo "23 54 32" > color Go ahead, but first convince Pavel, and then Greg :-) I'd personally would not have much against, especially that the list of values will not grow for that one like in case of old patch set [0] where Pavel and Greg thwarted my similar attempt. > $ cat max_color > 255 255 255 > > $ cat color_names > red green blue > > In addition to this -- one could also export individual color element > files. > >> Regarding led_scale_color_elements() - I checked it in GIMP and >> the results are not satisfactory when increasing brightness. >> Even if we managed to fix it, the result would not be guaranteed >> to be the same across all devices. > > No and they will never be the same. I was told by our hardware expert > that it is rather impossible to get linearly behaving LED control > without special curve fitting trimmed for particular hardware and LED > component in use. And if you go and change LED component/vendor it would > need to be "calibrated" again if such accuracy would be required. Also > LEDs age and that has also effect on this. >> This is still the same problem. >> >> I have another proposal, being a mix of what has been discussed so far: >> >> RGB LED class will expose following files: >> a) available by default: >> - red, green, blue >> Writing any of these file will result in writing corresponding >> device register. > > Problem with this is that we are basically back at square one and one > cannot do "atomic" color change with this. > > In order to set or activate new values one would need "load values" file > or such that when writing to it would activate new values. However it > becomes quite clumsy interface at that point as you need to handle > multiple writes to multiple files and makes those operations rather slow. > > Then we have color presets left that could kinda solve the issue on > setting the color to fixed values atomically. That's why I proposed "color_space" file that is also a part of my proposed design below. > Of course one direction is what happened with gpio driver was own device > node with ioctl's allowing more faster and more fancier control. > >> - color_space: it would accept color space, e,g. "hsv", that would >> have to be supported by LED RGB core; setting color >> space would create relevant files, e.g. for hsv >> hue. saturation, brightness, and remove default ones >> other "color spaces" could be defined in DT as >> proposed by Vesa; reading this file would print >> available color spaces >> >> b) available conditionally: >> - brightness >> It will be exposed by devices that have hardware support for >> changing color brightness, like lp5024, or it will be made >> available after setting relevant color space, like "hsv", or >> other color presets defined in DT >> >> I think it will be flexible enough to meet everyone's needs. >> >> Current triggers would work only when brightness file is available. > > Or we could transition it in that case to simulated "on/off" type of > thing. As that is what triggers more or less use. > > When "on" LED would have its configured color and when "off" LED would > be turned off (eg. values of zero). Yeah, it would be even better solution. [0] https://www.spinics.net/lists/devicetree/msg69730.html -- Best regards, Jacek Anaszewski
Hi! > >I suppose if one would just make it an array of values (separated by > >space) and then one file with string array of color element names and on > >file with maximum value array it could be within those words. > > > >The it would be something like: > > > >$ echo "23 54 32" > color > > Go ahead, but first convince Pavel, and then Greg :-) > > I'd personally would not have much against, especially that the > list of values will not grow for that one like in case of old patch set > [0] where Pavel and Greg thwarted my similar attempt. Oh, you can get this past Pavel (and probably Greg) -- if you have a good reason. Performance is _not_ a good reason. You don't need to change LED color 50000 times a second. If you can demonstrate a reasonable design, where user selects color from well-known RGB pallete and either kernel or library+kernel acts to set that color on the LED, we can talk. When user asks for white, it has to be white. Exact color temperature does not matter. When user asks for pink, it has to be pink. Again, exact color does not matter; different monitors display slightly different colors, too. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 1/4/19 9:12 PM, Pavel Machek wrote: > Hi! > >>> I suppose if one would just make it an array of values (separated by >>> space) and then one file with string array of color element names and on >>> file with maximum value array it could be within those words. >>> >>> The it would be something like: >>> >>> $ echo "23 54 32" > color >> >> Go ahead, but first convince Pavel, and then Greg :-) >> >> I'd personally would not have much against, especially that the >> list of values will not grow for that one like in case of old patch set >> [0] where Pavel and Greg thwarted my similar attempt. > > Oh, you can get this past Pavel (and probably Greg) -- if you have a > good reason. Performance is _not_ a good reason. You don't need to > change LED color 50000 times a second. We would need to do some profiling here to check if the problem exists. But, aside from that hypothetic issue, we need a solution for LEDn_BRIGHTNESS feature of lp5024, i.e. setting color intensity via a single register write. How would you propose to address that? > If you can demonstrate a reasonable design, where user selects color > from well-known RGB pallete and either kernel or library+kernel acts > to set that color on the LED, we can talk. > > When user asks for white, it has to be white. Exact color temperature > does not matter. When user asks for pink, it has to be pink. Again, > exact color does not matter; different monitors display slightly > different colors, too -- Best regards, Jacek Anaszewski
Hi! > But, aside from that hypothetic issue, we need a solution for > LEDn_BRIGHTNESS feature of lp5024, i.e. setting color intensity > via a single register write. How would you propose to address that? So they have hardware feature that allows control of 3 LEDs at the same time, right? Actually turris routers (IIRC) have similar feature, but shared for all their LEDs. I'd suggest simply ignoring that feature for now :-). We will need to solve RGB leds somehow, hopefully this is solved with it. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Pavel, On 1/5/19 11:12 PM, Pavel Machek wrote: > Hi! > >>> Grab yourself an RGB LED and play with it; you'll see what the >>> problems are. It is hard to explain colors over email... >> >> Video [0] gives some overview of lp5024 capabilities. >> >> I don't see any problems in exposing separate red,green,blue >> files and brightness for the devices with hardware support for >> that. > > Well, that's what we do today, as three separate LEDs, right? No. It doesn't allow for setting color intensity by having the color fixed beforehand. Below is relevant excerpt from the lp5024 documentation. This is not something that can be mapped to RGB color space, but rather to HSV/HSL, with the reservation that the hardware implementation uses PWM for setting color intensity. <quote> 8.3.1.2 Independent Intensity Control Per RGB LED Module When color is fixed, the independent intensity-control is used to achieve accurate and flexible dimming control for every RGB LED module. 8.3.1.2.1 Intensity-Control Register Configuration Every three consecutive output channels are assigned to their respective intensity-control register (LEDx_BRIGHTNESS). For example, OUT0, OUT1, and OUT2 are assigned to LED0_BRIGHTNESS, so it is recommended to connect the RGB LEDs in the sequence as shown in Table 1. The LP50xx device allows 256-step intensity control for each RGB LED module, which helps achieve a smooth dimming effect. </quote> > I don't have problem with that, either; other drivers already do > that. He's free to use existing same interface. > > But that is insufficient, as it does not allow simple stuff, such as > turning led "white". > > So... perhaps we should agree on requirements, first, and then we can > discuss solutions? > > Requirements for RGB LED interface: > > 1) Userspace should be able to set the white color > > 2) Userspace should be able to arbitrary color from well known list > and it should approximately match what would CRT, LCD or OLED monitor display The difference is that monitor display driver is pre-calibrated for given display by the manufacturer. With the LED controllers the manufacturer has no control over what LEDs will be connected to the iouts. Therefore it should be not surprising that colors produced by custom LEDs are not as user would expect when comparing to the RGB color displayed on the monitor display. TI provides "Various LED Ring Lighting Patterns Reference Design" [0] that show how to produce various patterns with use of the reference board with LED ring built using sixteen 19-337/R6GHBHC-A01/2T LEDs [1]. Document [0] mentions also specific "Design considerations" in the chapter 2.2: <quote> Several considerations are taken into account for this particular design: • LED map (ring) for meeting the requirement of popular human-machine interaction style. • LED size, numbers and the diffuse design for meeting lighting pattern uniformity. • Analog dimming in the difference ambient light lux without losing dimming resolution in lighting pattern. These considerations apply to most human-machine interaction end equipment with day and night vision designs in some way, but the designer must decide the particular considerations to take into account for a specific design. </quote> This renders your requirement 2) infeasible with use of custom LEDs any fixed algorithm, since the final effect will always heavily depend on the LED circuit design. > > 2a) LEDs probably slightly change color as they age. That's out of > scope, unless the variation is much greater than on monitors. > > 2b) Manufacturing differences cause small color variation. Again, > that's out of scope, unless the variation is much greater than on > monitors. > > Nice to have features: > > 3) Full range of available colors/intensities should be available to > userspace > > 4) Interface should work well with existing triggers > > 5) It would be nice if userland knew how many lumens are produced at > each wavelength for each setting (again, minus aging and manufacturing > variations). > > 6) Complexity of math in kernel should be low, and preferably it > should be integer or fixed point > > Problems: > > a) RGB LEDs are usually not balanced. Setting 100% PWM on > red/green/blue channels will result in nothing close to white > light. In fact, to get white light on N900, blue and green channel's > PWM needs to be set pretty low, as in 5%. > > b) LED class does not define any relation between "brightness" in > sysfs and ammount of light in lumens. Some drivers use close to linear > relation, some use exponential relation. Human eyes percieve logarithm > of lumens. RGB color model uses even more complex function. > > c) Except for white LEDs, LEDs are basically sources of single > wavelength of light, while CRTs and LCDs produce broader > spectrums. > > d) RG, RGBW and probably other LED combinations exist. > > e) Not all "red" LEDs will produce same wavelength. Similar > differences will exist for other colors. > > f) We have existing RGB LEDs represented as three separate > monochromatic LEDs in sysfs. One general question: do you have any solutions in store? [0] http://www.ti.com/lit/ug/tiduee6/tiduee6.pdf [1] http://www.everlight.com/file/ProductFile/19-337-R6GHBHC-A01-2T.pdf -- Best regards, Jacek Anaszewski
Vesa On 1/4/19 6:39 PM, Vesa Jääskeläinen wrote: > Hi Jacek, > > On 04/01/2019 23.37, Jacek Anaszewski wrote: >> But, aside from that hypothetic issue, we need a solution for >> LEDn_BRIGHTNESS feature of lp5024, i.e. setting color intensity >> via a single register write. How would you propose to address that? > > You could model it to something like this in device tree: > > led-module @ <i2c-address> { > compatible = "lp5024"; > > // There is in hardware setup to use either linear or > // logarithmic scaling: > //enable-logarithmic-brightness; > > led0 { > // this will create led instance for LED0 in lp5024 > label = "lp-led0"; > > // This specifies LED number within lp5024 > led-index = <0>; // set output-base as 0*3 == 0 > > element-red { > // refers to OUT0 > output-offset = <0>; > }; > > element-green { > // refers to OUT1 > output-offset = <1>; > }; > > element-blue { > // refers to OUT2 > output-offset = <2>; > }; > > }; > > led1 { > // this will create led instance for LED1 in lp5024 > label = "lp-led1"; > > // This specifies LED number within lp5024 > led-index = <1>; // set output-base as 1*3 == 3 > Can we not use led-sources like I have done already? I really like to keep the DT nodes simple and re-use nodes that exist if possible. My code already maps and groups the outputs into the associated banks Dan <snip> -- ------------------ Dan Murphy
Jacek On 1/7/19 3:13 PM, Jacek Anaszewski wrote: > Hi Vesa, > > On 1/5/19 1:39 AM, Vesa Jääskeläinen wrote: >> Hi Jacek, >> >> On 04/01/2019 23.37, Jacek Anaszewski wrote: >>> But, aside from that hypothetic issue, we need a solution for >>> LEDn_BRIGHTNESS feature of lp5024, i.e. setting color intensity >>> via a single register write. How would you propose to address that? >> >> You could model it to something like this in device tree: >> >> led-module @ <i2c-address> { >> compatible = "lp5024"; >> >> // There is in hardware setup to use either linear or >> // logarithmic scaling: >> //enable-logarithmic-brightness; >> >> led0 { >> // this will create led instance for LED0 in lp5024 >> label = "lp-led0"; >> >> // This specifies LED number within lp5024 >> led-index = <0>; // set output-base as 0*3 == 0 >> >> element-red { >> // refers to OUT0 >> output-offset = <0>; >> }; >> >> element-green { >> // refers to OUT1 >> output-offset = <1>; >> }; >> >> element-blue { >> // refers to OUT2 >> output-offset = <2>; >> }; >> >> }; >> >> led1 { >> // this will create led instance for LED1 in lp5024 >> label = "lp-led1"; >> >> // This specifies LED number within lp5024 >> led-index = <1>; // set output-base as 1*3 == 3 >> >> element-red { >> // refers to OUT3 >> output-offset = <0>; >> }; >> >> element-green { >> // refers to OUT4 >> output-offset = <1>; >> }; >> >> element-blue { >> // refers to OUT5 >> output-offset = <2>; >> }; >> >> }; >> >> bank-led { >> // this will create led instance for bank leds in lp5024 >> label = "lp-bank-led"; >> >> // configured bank led configuration >> led-index = <2 3 4 5 6 7>; >> // As here is list of led-indices this entry is >> // assumed to be bank configuration. Bank mode is enable >> // for the indices. >> >> // set output-base as BANK A >> >> element-red { >> // refers to BANK A >> output-offset = <0>; >> }; >> >> element-green { >> // refers to BANK B >> output-offset = <1>; >> }; >> >> element-blue { >> // refers to BANK C >> output-offset = <2>; >> }; >> }; >> }; >> >> This would then create three led instances and each led instance has brightness setting and that goes straight to hardware. >> >> If one would want to override hardware control for brightness then I suppose you would define in led node something like: >> >> brightness-model = "hsl" >> >> This would then pick red, green and blue elements for hsl calculations and others color elements for linear. LED specific hardware brightness would then be either 0 or 0xFF depending if all of LED color elements are zero or not. >> >> Would that kind of model work? > > I'd prefer to have single RGB LED device. And your DT design > is unnecessarily complex and a bit confusing. +1 to that comment > > Also, you provided scarce information about sysfs interface. > It would be nice to see the sequence of commands. > -- ------------------ Dan Murphy
Dan, On 12/19/18 5:26 PM, Dan Murphy wrote: > Introduce the LP5024 and LP5018 RGB LED driver. > The difference in these 2 parts are only in the number of > LED outputs where the LP5024 can control 24 LEDs the LP5018 > can only control 18. > > The device has the ability to group LED output into control banks > so that multiple LED banks can be controlled with the same mixing and > brightness. Inversely the LEDs can also be controlled independently. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > drivers/leds/Kconfig | 7 + > drivers/leds/Makefile | 1 + > drivers/leds/leds-lp5024.c | 610 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 618 insertions(+) > create mode 100644 drivers/leds/leds-lp5024.c > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index a72f97fca57b..d306bedb00b7 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -326,6 +326,13 @@ config LEDS_LP3952 > To compile this driver as a module, choose M here: the > module will be called leds-lp3952. > > +config LEDS_LP5024 > + tristate "LED Support for TI LP5024/18 LED driver chip" > + depends on LEDS_CLASS && REGMAP_I2C > + help > + If you say yes here you get support for the Texas Instruments > + LP5024 and LP5018 LED driver. > + > config LEDS_LP55XX_COMMON > tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501" > depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501 > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index 4c1b0054f379..60b4e4ddd3ee 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -32,6 +32,7 @@ obj-$(CONFIG_LEDS_GPIO_REGISTER) += leds-gpio-register.o > obj-$(CONFIG_LEDS_GPIO) += leds-gpio.o > obj-$(CONFIG_LEDS_LP3944) += leds-lp3944.o > obj-$(CONFIG_LEDS_LP3952) += leds-lp3952.o > +obj-$(CONFIG_LEDS_LP5024) += leds-lp5024.o > obj-$(CONFIG_LEDS_LP55XX_COMMON) += leds-lp55xx-common.o > obj-$(CONFIG_LEDS_LP5521) += leds-lp5521.o > obj-$(CONFIG_LEDS_LP5523) += leds-lp5523.o > diff --git a/drivers/leds/leds-lp5024.c b/drivers/leds/leds-lp5024.c > new file mode 100644 > index 000000000000..90e8dca15609 > --- /dev/null > +++ b/drivers/leds/leds-lp5024.c > @@ -0,0 +1,610 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* TI LP50XX LED chip family driver > + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ > + */ > + > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/leds.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of.h> > +#include <linux/of_gpio.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <linux/slab.h> > +#include <uapi/linux/uleds.h> > + > +#define LP5024_DEV_CFG0 0x00 > +#define LP5024_DEV_CFG1 0x01 > +#define LP5024_LED_CFG0 0x02 > +#define LP5024_BNK_BRT 0x03 > +#define LP5024_BNKA_CLR 0x04 > +#define LP5024_BNKB_CLR 0x05 > +#define LP5024_BNKC_CLR 0x06 > +#define LP5024_LED0_BRT 0x07 > +#define LP5024_LED1_BRT 0x08 > +#define LP5024_LED2_BRT 0x09 > +#define LP5024_LED3_BRT 0x0a > +#define LP5024_LED4_BRT 0x0b > +#define LP5024_LED5_BRT 0x0c > +#define LP5024_LED6_BRT 0x0d > +#define LP5024_LED7_BRT 0x0e > + > +#define LP5024_OUT0_CLR 0x0f > +#define LP5024_OUT1_CLR 0x10 > +#define LP5024_OUT2_CLR 0x11 > +#define LP5024_OUT3_CLR 0x12 > +#define LP5024_OUT4_CLR 0x13 > +#define LP5024_OUT5_CLR 0x14 > +#define LP5024_OUT6_CLR 0x15 > +#define LP5024_OUT7_CLR 0x16 > +#define LP5024_OUT8_CLR 0x17 > +#define LP5024_OUT9_CLR 0x18 > +#define LP5024_OUT10_CLR 0x19 > +#define LP5024_OUT11_CLR 0x1a > +#define LP5024_OUT12_CLR 0x1b > +#define LP5024_OUT13_CLR 0x1c > +#define LP5024_OUT14_CLR 0x1d > +#define LP5024_OUT15_CLR 0x1e > +#define LP5024_OUT16_CLR 0x1f > +#define LP5024_OUT17_CLR 0x20 > +#define LP5024_OUT18_CLR 0x21 > +#define LP5024_OUT19_CLR 0x22 > +#define LP5024_OUT20_CLR 0x23 > +#define LP5024_OUT21_CLR 0x24 > +#define LP5024_OUT22_CLR 0x25 > +#define LP5024_OUT23_CLR 0x26 > + > +#define LP5024_RESET 0x27 > +#define LP5024_SW_RESET 0xff > + > +#define LP5024_CHIP_EN BIT(6) > + > +#define LP5024_CONTROL_A 0 > +#define LP5024_CONTROL_B 1 > +#define LP5024_CONTROL_C 2 > +#define LP5024_MAX_CONTROL_BANKS 3 > + > +#define LP5018_MAX_LED_STRINGS 6 > +#define LP5024_MAX_LED_STRINGS 8 > + > +enum lp5024_model { > + LP5018, > + LP5024, > +}; > + > +struct lp5024_led { > + u32 led_strings[LP5024_MAX_LED_STRINGS]; > + char label[LED_MAX_NAME_SIZE]; > + struct led_classdev led_dev; > + struct lp5024 *priv; > + int led_number; > + u8 ctrl_bank_enabled; > +}; > + > +/** > + * struct lp5024 - > + * @enable_gpio: Hardware enable gpio > + * @regulator: LED supply regulator pointer > + * @client: Pointer to the I2C client > + * @regmap: Devices register map > + * @dev: Pointer to the devices device struct > + * @lock: Lock for reading/writing the device > + * @model_id: ID of the device > + * @leds: Array of LED strings > + */ > +struct lp5024 { > + struct gpio_desc *enable_gpio; > + struct regulator *regulator; > + struct i2c_client *client; > + struct regmap *regmap; > + struct device *dev; > + struct mutex lock; > + int model_id; > + int max_leds; > + int num_of_leds; > + > + /* This needs to be at the end of the struct */ > + struct lp5024_led leds[]; > +}; > + > +static const struct reg_default lp5024_reg_defs[] = { > + {LP5024_DEV_CFG0, 0x0}, > + {LP5024_DEV_CFG1, 0x3c}, > + {LP5024_BNK_BRT, 0xff}, > + {LP5024_BNKA_CLR, 0x0f}, > + {LP5024_BNKB_CLR, 0x0f}, > + {LP5024_BNKC_CLR, 0x0f}, > + {LP5024_LED0_BRT, 0x0f}, > + {LP5024_LED1_BRT, 0xff}, > + {LP5024_LED2_BRT, 0xff}, > + {LP5024_LED3_BRT, 0xff}, > + {LP5024_LED4_BRT, 0xff}, > + {LP5024_LED5_BRT, 0xff}, > + {LP5024_LED6_BRT, 0xff}, > + {LP5024_LED7_BRT, 0xff}, > + {LP5024_OUT0_CLR, 0x0f}, > + {LP5024_OUT1_CLR, 0x00}, > + {LP5024_OUT2_CLR, 0x00}, > + {LP5024_OUT3_CLR, 0x00}, > + {LP5024_OUT4_CLR, 0x00}, > + {LP5024_OUT5_CLR, 0x00}, > + {LP5024_OUT6_CLR, 0x00}, > + {LP5024_OUT7_CLR, 0x00}, > + {LP5024_OUT8_CLR, 0x00}, > + {LP5024_OUT9_CLR, 0x00}, > + {LP5024_OUT10_CLR, 0x00}, > + {LP5024_OUT11_CLR, 0x00}, > + {LP5024_OUT12_CLR, 0x00}, > + {LP5024_OUT13_CLR, 0x00}, > + {LP5024_OUT14_CLR, 0x00}, > + {LP5024_OUT15_CLR, 0x00}, > + {LP5024_OUT16_CLR, 0x00}, > + {LP5024_OUT17_CLR, 0x00}, > + {LP5024_OUT18_CLR, 0x00}, > + {LP5024_OUT19_CLR, 0x00}, > + {LP5024_OUT20_CLR, 0x00}, > + {LP5024_OUT21_CLR, 0x00}, > + {LP5024_OUT22_CLR, 0x00}, > + {LP5024_OUT23_CLR, 0x00}, > + {LP5024_RESET, 0x00} > +}; > + > +static const struct regmap_config lp5024_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + > + .max_register = LP5024_RESET, > + .reg_defaults = lp5024_reg_defs, > + .num_reg_defaults = ARRAY_SIZE(lp5024_reg_defs), > + .cache_type = REGCACHE_RBTREE, > +}; > + > +static int lp5024_set_color_mix(struct lp5024_led *led, u8 color_reg, > + u8 color_val) > +{ > + return regmap_write(led->priv->regmap, color_reg, color_val); > +} > + > + > +static ssize_t ctrl_bank_a_mix_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct lp5024_led *led = container_of(led_cdev, struct lp5024_led, > + led_dev); > + u8 mix_value; > + int ret; > + > + ret = kstrtou8(buf, 0, &mix_value); > + if (ret) > + return ret; > + > + lp5024_set_color_mix(led, LP5024_BNKA_CLR, mix_value); > + > + return size; > +} > +static ssize_t ctrl_bank_b_mix_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct lp5024_led *led = container_of(led_cdev, struct lp5024_led, > + led_dev); > + u8 mix_value; > + int ret; > + > + ret = kstrtou8(buf, 0, &mix_value); > + if (ret) > + return ret; > + > + lp5024_set_color_mix(led, LP5024_BNKB_CLR, mix_value); > + > + return size; > +} > +static ssize_t ctrl_bank_c_mix_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct lp5024_led *led = container_of(led_cdev, struct lp5024_led, > + led_dev); > + u8 mix_value; > + int ret; > + > + ret = kstrtou8(buf, 0, &mix_value); > + if (ret) > + return ret; > + > + lp5024_set_color_mix(led, LP5024_BNKC_CLR, mix_value); > + > + return size; > +} > + > +static DEVICE_ATTR_WO(ctrl_bank_a_mix); > +static DEVICE_ATTR_WO(ctrl_bank_b_mix); > +static DEVICE_ATTR_WO(ctrl_bank_c_mix); Why WO? > + > +static struct attribute *lp5024_ctrl_bank_attrs[] = { > + &dev_attr_ctrl_bank_a_mix.attr, > + &dev_attr_ctrl_bank_b_mix.attr, > + &dev_attr_ctrl_bank_c_mix.attr, > + NULL > +}; > +ATTRIBUTE_GROUPS(lp5024_ctrl_bank); > + > +static ssize_t led3_mix_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct lp5024_led *led = container_of(led_cdev, struct lp5024_led, > + led_dev); > + u8 mix_value; > + u8 reg_value; > + int ret; > + > + ret = kstrtou8(buf, 0, &mix_value); > + if (ret) > + return ret; > + > + reg_value = (led->led_number * 3) + LP5024_OUT2_CLR; It is more natural if constant goes first. Like BASE_ADDR + offset. > + > + lp5024_set_color_mix(led, reg_value, mix_value); > + > + return size; > +} > + > +static ssize_t led2_mix_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct lp5024_led *led = container_of(led_cdev, struct lp5024_led, > + led_dev); > + u8 mix_value; > + u8 reg_value; > + int ret; > + > + ret = kstrtou8(buf, 0, &mix_value); > + if (ret) > + return ret; > + > + reg_value = (led->led_number * 3) + LP5024_OUT1_CLR; > + > + lp5024_set_color_mix(led, reg_value, mix_value); > + > + return size; > +} > + > +static ssize_t led1_mix_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct lp5024_led *led = container_of(led_cdev, struct lp5024_led, > + led_dev); > + u8 mix_value; > + u8 reg_value; > + int ret; > + > + ret = kstrtou8(buf, 0, &mix_value); > + if (ret) > + return ret; > + > + reg_value = (led->led_number * 3) + LP5024_OUT0_CLR; > + > + lp5024_set_color_mix(led, reg_value, mix_value); > + > + return size; > +} > + > +static DEVICE_ATTR_WO(led1_mix); > +static DEVICE_ATTR_WO(led2_mix); > +static DEVICE_ATTR_WO(led3_mix); Why WO? > +static struct attribute *lp5024_led_independent_attrs[] = { > + &dev_attr_led1_mix.attr, > + &dev_attr_led2_mix.attr, > + &dev_attr_led3_mix.attr, > + NULL > +}; > +ATTRIBUTE_GROUPS(lp5024_led_independent); > + > +static int lp5024_brightness_set(struct led_classdev *led_cdev, > + enum led_brightness brt_val) > +{ > + struct lp5024_led *led = container_of(led_cdev, struct lp5024_led, > + led_dev); > + int ret = 0; > + u8 reg_val; > + > + mutex_lock(&led->priv->lock); > + > + if (led->ctrl_bank_enabled) > + reg_val = LP5024_BNK_BRT; > + else > + reg_val = led->led_number + LP5024_LED0_BRT; > + > + ret = regmap_write(led->priv->regmap, reg_val, brt_val); > + > + mutex_unlock(&led->priv->lock); > + > + return ret; > +} > + > +static enum led_brightness lp5024_brightness_get(struct led_classdev *led_cdev) > +{ > + struct lp5024_led *led = container_of(led_cdev, struct lp5024_led, > + led_dev); > + unsigned int brt_val; > + u8 reg_val; > + int ret; > + > + mutex_lock(&led->priv->lock); > + > + if (led->ctrl_bank_enabled) > + reg_val = LP5024_BNK_BRT; > + else > + reg_val = led->led_number + LP5024_LED0_BRT; > + > + ret = regmap_read(led->priv->regmap, reg_val, &brt_val); > + > + mutex_unlock(&led->priv->lock); > + > + return brt_val; > +} > + > +static int lp5024_set_led_values(struct lp5024 *priv) > +{ > + struct lp5024_led *led; > + int i, j; > + u8 led_ctrl_enable = 0; > + > + for (i = 0; i <= priv->num_of_leds; i++) { > + led = &priv->leds[i]; > + if (led->ctrl_bank_enabled) { > + for (j = 0; j <= LP5024_MAX_LED_STRINGS - 1; j++) > + led_ctrl_enable |= (1 << led->led_strings[j]); > + } > + } > + > + regmap_write(priv->regmap, LP5024_LED_CFG0, led_ctrl_enable); > + > + return 0; > +} > + > +static int lp5024_init(struct lp5024 *priv) > +{ > + int ret; > + > + if (priv->enable_gpio) { > + gpiod_direction_output(priv->enable_gpio, 1); > + } else { > + ret = regmap_write(priv->regmap, LP5024_RESET, LP5024_SW_RESET); > + if (ret) { > + dev_err(&priv->client->dev, > + "Cannot reset the device\n"); > + goto out; > + } > + } > + > + ret = lp5024_set_led_values(priv); > + if (ret) > + dev_err(&priv->client->dev, "Setting the CRTL bank failed\n"); > + > + ret = regmap_write(priv->regmap, LP5024_DEV_CFG0, LP5024_CHIP_EN); > + if (ret) { > + dev_err(&priv->client->dev, "Cannot write ctrl enable\n"); > + goto out; > + } > +out: > + return ret; > +} > + > +static int lp5024_probe_dt(struct lp5024 *priv) > +{ > + struct fwnode_handle *child = NULL; > + struct lp5024_led *led; > + const char *name; > + int led_number; > + size_t i = 0; > + int ret; > + > + priv->enable_gpio = devm_gpiod_get_optional(&priv->client->dev, > + "enable", GPIOD_OUT_LOW); > + if (IS_ERR(priv->enable_gpio)) { > + ret = PTR_ERR(priv->enable_gpio); > + dev_err(&priv->client->dev, "Failed to get enable gpio: %d\n", > + ret); > + return ret; > + } > + > + priv->regulator = devm_regulator_get(&priv->client->dev, "vled"); > + if (IS_ERR(priv->regulator)) > + priv->regulator = NULL; > + > + if (priv->model_id == LP5018) > + priv->max_leds = LP5018_MAX_LED_STRINGS; > + else > + priv->max_leds = LP5024_MAX_LED_STRINGS; > + > + device_for_each_child_node(&priv->client->dev, child) { > + led = &priv->leds[i]; > + > + if (fwnode_property_present(child, "ti,control-bank")) > + led->ctrl_bank_enabled = 1; > + else > + led->ctrl_bank_enabled = 0; > + > + if (led->ctrl_bank_enabled) { > + ret = fwnode_property_read_u32_array(child, > + "led-sources", > + NULL, 0); > + ret = fwnode_property_read_u32_array(child, > + "led-sources", > + led->led_strings, > + ret); > + > + led->led_number = led->led_strings[0]; > + > + } else { > + ret = fwnode_property_read_u32(child, "led-sources", > + &led_number); > + > + led->led_number = led_number; > + } > + if (ret) { > + dev_err(&priv->client->dev, > + "led-sources property missing\n"); > + fwnode_handle_put(child); > + goto child_out; > + } > + > + if (led_number > priv->max_leds) { > + dev_err(&priv->client->dev, > + "led-sources property is invalid\n"); > + ret = -EINVAL; > + fwnode_handle_put(child); > + goto child_out; > + } > + > + ret = fwnode_property_read_string(child, "label", &name); > + if (ret) > + snprintf(led->label, sizeof(led->label), > + "%s::", priv->client->name); > + else > + snprintf(led->label, sizeof(led->label), > + "%s:%s", priv->client->name, name); > + > + fwnode_property_read_string(child, "linux,default-trigger", > + &led->led_dev.default_trigger); > + > + led->priv = priv; > + led->led_dev.name = led->label; > + led->led_dev.max_brightness = 255; > + led->led_dev.brightness_set_blocking = lp5024_brightness_set; > + led->led_dev.brightness_get = lp5024_brightness_get; > + > + if (led->ctrl_bank_enabled) > + led->led_dev.groups = lp5024_ctrl_bank_groups; > + else > + led->led_dev.groups = lp5024_led_independent_groups; > + > + ret = devm_led_classdev_register(&priv->client->dev, > + &led->led_dev); > + if (ret) { > + dev_err(&priv->client->dev, "led register err: %d\n", > + ret); > + fwnode_handle_put(child); > + goto child_out; > + } > + i++; > + } > + priv->num_of_leds = i; > + > +child_out: > + return ret; > +} > + > +static int lp5024_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct lp5024 *led; > + int count; > + int ret; > + > + count = device_get_child_node_count(&client->dev); > + if (!count) { > + dev_err(&client->dev, "LEDs are not defined in device tree!"); > + return -ENODEV; > + } > + > + led = devm_kzalloc(&client->dev, struct_size(led, leds, count), > + GFP_KERNEL); > + if (!led) > + return -ENOMEM; > + > + mutex_init(&led->lock); > + led->client = client; > + led->dev = &client->dev; > + led->model_id = id->driver_data; > + i2c_set_clientdata(client, led); > + > + led->regmap = devm_regmap_init_i2c(client, &lp5024_regmap_config); > + if (IS_ERR(led->regmap)) { > + ret = PTR_ERR(led->regmap); > + dev_err(&client->dev, "Failed to allocate register map: %d\n", > + ret); > + return ret; > + } > + > + ret = lp5024_probe_dt(led); > + if (ret) > + return ret; > + > + ret = lp5024_init(led); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int lp5024_remove(struct i2c_client *client) > +{ > + struct lp5024 *led = i2c_get_clientdata(client); > + int ret; > + > + ret = regmap_update_bits(led->regmap, LP5024_DEV_CFG0, > + LP5024_CHIP_EN, 0); > + if (ret) { > + dev_err(&led->client->dev, "Failed to disable regulator\n"); > + return ret; > + } > + > + if (led->enable_gpio) > + gpiod_direction_output(led->enable_gpio, 0); > + > + if (led->regulator) { > + ret = regulator_disable(led->regulator); > + if (ret) > + dev_err(&led->client->dev, > + "Failed to disable regulator\n"); > + } > + > + mutex_destroy(&led->lock); > + > + return 0; > +} > + > +static const struct i2c_device_id lp5024_id[] = { > + { "lp5018", LP5018 }, > + { "lp5024", LP5024 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, lp5024_id); > + > +static const struct of_device_id of_lp5024_leds_match[] = { > + { .compatible = "ti,lp5018", }, > + { .compatible = "ti,lp5024", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, of_lp5024_leds_match); > + > +static struct i2c_driver lp5024_driver = { > + .driver = { > + .name = "lp5024", > + .of_match_table = of_lp5024_leds_match, > + }, > + .probe = lp5024_probe, > + .remove = lp5024_remove, > + .id_table = lp5024_id, > +}; > +module_i2c_driver(lp5024_driver); > + > +MODULE_DESCRIPTION("Texas Instruments LP5024 LED driver"); > +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>"); > +MODULE_LICENSE("GPL v2"); > -- Best regards, Jacek Anaszewski