Add multicolor framework support for the lp55xx family.
Signed-off-by: Dan Murphy <[email protected]>
---
drivers/leds/Kconfig | 1 +
drivers/leds/leds-lp55xx-common.c | 185 +++++++++++++++++++---
drivers/leds/leds-lp55xx-common.h | 9 ++
include/linux/platform_data/leds-lp55xx.h | 7 +
4 files changed, 179 insertions(+), 23 deletions(-)
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index fb614a6b9afa..5706bf8d8bd1 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -377,6 +377,7 @@ config LEDS_LP50XX
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
+ depends on OF
select FW_LOADER
select FW_LOADER_USER_HELPER
help
diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
index 882ef39e4965..197b87ca5ca2 100644
--- a/drivers/leds/leds-lp55xx-common.c
+++ b/drivers/leds/leds-lp55xx-common.c
@@ -131,14 +131,62 @@ static struct attribute *lp55xx_led_attrs[] = {
};
ATTRIBUTE_GROUPS(lp55xx_led);
+#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
+static int lp55xx_map_channel(struct lp55xx_led *led, int color_id,
+ enum led_brightness brightness)
+{
+ int i;
+
+ for (i = 0; i < led->mc_cdev.num_leds; i++) {
+ if (led->color_components[i].color_id == color_id) {
+ led->color_components[i].brightness = brightness;
+ return 0;
+ }
+ }
+
+ return -EINVAL;
+}
+#endif
+
+static int lp55xx_set_mc_brightness(struct lp55xx_led *led,
+ struct lp55xx_device_config *cfg,
+ enum led_brightness brightness)
+{
+ int ret = -EINVAL;
+#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
+ struct led_mc_color_conversion color_components[LP55XX_MAX_GROUPED_CHAN];
+ int i;
+
+ if (!cfg->multicolor_brightness_fn)
+ return -EINVAL;
+
+ led_mc_calc_color_components(&led->mc_cdev, brightness,
+ color_components);
+
+ for (i = 0; i < led->mc_cdev.num_leds; i++) {
+ ret = lp55xx_map_channel(led, color_components[i].color_id,
+ color_components[i].brightness);
+ if (ret)
+ return ret;
+ }
+
+ ret = cfg->multicolor_brightness_fn(led);
+#endif
+ return ret;
+}
+
static int lp55xx_set_brightness(struct led_classdev *cdev,
enum led_brightness brightness)
{
struct lp55xx_led *led = cdev_to_lp55xx_led(cdev);
struct lp55xx_device_config *cfg = led->chip->cfg;
- led->brightness = (u8)brightness;
- return cfg->brightness_fn(led);
+ if (led->mc_cdev.num_leds > 1) {
+ return lp55xx_set_mc_brightness(led, cfg, brightness);
+ } else {
+ led->brightness = (u8)brightness;
+ return cfg->brightness_fn(led);
+ }
}
static int lp55xx_init_led(struct lp55xx_led *led,
@@ -147,9 +195,9 @@ static int lp55xx_init_led(struct lp55xx_led *led,
struct lp55xx_platform_data *pdata = chip->pdata;
struct lp55xx_device_config *cfg = chip->cfg;
struct device *dev = &chip->cl->dev;
+ int max_channel = cfg->max_channel;
char name[32];
int ret;
- int max_channel = cfg->max_channel;
if (chan >= max_channel) {
dev_err(dev, "invalid channel: %d / %d\n", chan, max_channel);
@@ -159,10 +207,34 @@ static int lp55xx_init_led(struct lp55xx_led *led,
if (pdata->led_config[chan].led_current == 0)
return 0;
+ if (pdata->led_config[chan].name) {
+ led->cdev.name = pdata->led_config[chan].name;
+ } else {
+ snprintf(name, sizeof(name), "%s:channel%d",
+ pdata->label ? : chip->cl->name, chan);
+ led->cdev.name = name;
+ }
+
+ if (pdata->led_config[chan].num_colors > 1) {
+ led->mc_cdev.led_cdev = &led->cdev;
+ led->cdev.brightness_set_blocking = lp55xx_set_brightness;
+ led->cdev.groups = lp55xx_led_groups;
+ led->mc_cdev.num_leds = pdata->led_config[chan].num_colors;
+ led->mc_cdev.available_colors =
+ pdata->led_config[chan].available_colors;
+ memcpy(led->color_components,
+ pdata->led_config[chan].color_components,
+ sizeof(led->color_components));
+ } else {
+
+ led->cdev.default_trigger =
+ pdata->led_config[chan].default_trigger;
+ led->cdev.brightness_set_blocking = lp55xx_set_brightness;
+ } led->cdev.groups = lp55xx_led_groups;
+
led->led_current = pdata->led_config[chan].led_current;
led->max_current = pdata->led_config[chan].max_current;
led->chan_nr = pdata->led_config[chan].chan_nr;
- led->cdev.default_trigger = pdata->led_config[chan].default_trigger;
if (led->chan_nr >= max_channel) {
dev_err(dev, "Use channel numbers between 0 and %d\n",
@@ -170,18 +242,13 @@ static int lp55xx_init_led(struct lp55xx_led *led,
return -EINVAL;
}
- led->cdev.brightness_set_blocking = lp55xx_set_brightness;
- led->cdev.groups = lp55xx_led_groups;
-
- if (pdata->led_config[chan].name) {
- led->cdev.name = pdata->led_config[chan].name;
- } else {
- snprintf(name, sizeof(name), "%s:channel%d",
- pdata->label ? : chip->cl->name, chan);
- led->cdev.name = name;
- }
+#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
+ if (pdata->led_config[chan].num_colors > 1)
+ ret = devm_led_classdev_multicolor_register(dev, &led->mc_cdev);
+ else
+#endif
+ ret = devm_led_classdev_register(dev, &led->cdev);
- ret = devm_led_classdev_register(dev, &led->cdev);
if (ret) {
dev_err(dev, "led register err: %d\n", ret);
return ret;
@@ -525,6 +592,82 @@ void lp55xx_unregister_sysfs(struct lp55xx_chip *chip)
}
EXPORT_SYMBOL_GPL(lp55xx_unregister_sysfs);
+static int lp5xx_parse_common_child(struct device_node *np,
+ struct lp55xx_led_config *cfg,
+ int chan_num, bool is_multicolor,
+ int color_num)
+{
+ u32 led_number;
+ int ret;
+
+ of_property_read_string(np, "chan-name",
+ &cfg[chan_num].name);
+ of_property_read_u8(np, "led-cur",
+ &cfg[chan_num].led_current);
+ of_property_read_u8(np, "max-cur",
+ &cfg[chan_num].max_current);
+
+ ret = of_property_read_u32(np, "reg", &led_number);
+ if (ret)
+ return ret;
+
+ if (led_number < 0 || led_number > 6)
+ return -EINVAL;
+
+ if (is_multicolor)
+ cfg[chan_num].color_components[color_num].output_num =
+ led_number;
+ else
+ cfg[chan_num].chan_nr = led_number;
+
+ return 0;
+}
+
+static int lp5xx_parse_channel_child(struct device_node *np,
+ struct lp55xx_led_config *cfg,
+ int child_number)
+{
+ struct device_node *child;
+ int channel_color;
+ int num_colors = 0;
+ u32 color_id;
+ int ret;
+
+ cfg[child_number].default_trigger =
+ of_get_property(np, "linux,default-trigger", NULL);
+
+ ret = of_property_read_u32(np, "color", &channel_color);
+ if (ret)
+ channel_color = ret;
+
+
+ if (channel_color == LED_COLOR_ID_MULTI) {
+ for_each_child_of_node(np, child) {
+ ret = lp5xx_parse_common_child(child, cfg,
+ child_number, true,
+ num_colors);
+ if (ret)
+ return ret;
+
+ ret = of_property_read_u32(child, "color", &color_id);
+ if (ret)
+ return ret;
+
+ cfg[child_number].color_components[num_colors].color_id =
+ color_id;
+ set_bit(color_id, &cfg[child_number].available_colors);
+ num_colors++;
+ }
+
+ cfg[child_number].num_colors = num_colors;
+ } else {
+ return lp5xx_parse_common_child(np, cfg, child_number, false,
+ num_colors);
+ }
+
+ return 0;
+}
+
struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
struct device_node *np)
{
@@ -533,6 +676,7 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
struct lp55xx_led_config *cfg;
int num_channels;
int i = 0;
+ int ret;
pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata)
@@ -552,14 +696,9 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
pdata->num_channels = num_channels;
for_each_child_of_node(np, child) {
- cfg[i].chan_nr = i;
-
- of_property_read_string(child, "chan-name", &cfg[i].name);
- of_property_read_u8(child, "led-cur", &cfg[i].led_current);
- of_property_read_u8(child, "max-cur", &cfg[i].max_current);
- cfg[i].default_trigger =
- of_get_property(child, "linux,default-trigger", NULL);
-
+ ret = lp5xx_parse_channel_child(child, cfg, i);
+ if (ret)
+ return ERR_PTR(-EINVAL);
i++;
}
diff --git a/drivers/leds/leds-lp55xx-common.h b/drivers/leds/leds-lp55xx-common.h
index b9b1041e8143..4a0cdbfe54a6 100644
--- a/drivers/leds/leds-lp55xx-common.h
+++ b/drivers/leds/leds-lp55xx-common.h
@@ -12,6 +12,8 @@
#ifndef _LEDS_LP55XX_COMMON_H
#define _LEDS_LP55XX_COMMON_H
+#include <linux/led-class-multicolor.h>
+
enum lp55xx_engine_index {
LP55XX_ENGINE_INVALID,
LP55XX_ENGINE_1,
@@ -109,6 +111,9 @@ struct lp55xx_device_config {
/* access brightness register */
int (*brightness_fn)(struct lp55xx_led *led);
+ /* perform brightness value to multiple LEDs */
+ int (*multicolor_brightness_fn)(struct lp55xx_led *led);
+
/* current setting function */
void (*set_led_current) (struct lp55xx_led *led, u8 led_current);
@@ -159,6 +164,8 @@ struct lp55xx_chip {
* struct lp55xx_led
* @chan_nr : Channel number
* @cdev : LED class device
+ * @mc_cdev : Multi color class device
+ * @color_components: Multi color LED map information
* @led_current : Current setting at each led channel
* @max_current : Maximun current at each led channel
* @brightness : Brightness value
@@ -167,6 +174,8 @@ struct lp55xx_chip {
struct lp55xx_led {
int chan_nr;
struct led_classdev cdev;
+ struct led_classdev_mc mc_cdev;
+ struct led_mc_color_conversion color_components[LP55XX_MAX_GROUPED_CHAN];
u8 led_current;
u8 max_current;
u8 brightness;
diff --git a/include/linux/platform_data/leds-lp55xx.h b/include/linux/platform_data/leds-lp55xx.h
index 96a787100fda..ead9d145de0d 100644
--- a/include/linux/platform_data/leds-lp55xx.h
+++ b/include/linux/platform_data/leds-lp55xx.h
@@ -12,17 +12,24 @@
#ifndef _LEDS_LP55XX_H
#define _LEDS_LP55XX_H
+#include <linux/led-class-multicolor.h>
+
/* Clock configuration */
#define LP55XX_CLOCK_AUTO 0
#define LP55XX_CLOCK_INT 1
#define LP55XX_CLOCK_EXT 2
+#define LP55XX_MAX_GROUPED_CHAN 4
+
struct lp55xx_led_config {
const char *name;
const char *default_trigger;
u8 chan_nr;
u8 led_current; /* mA x10, 0 if led is not connected */
u8 max_current;
+ int num_colors;
+ unsigned long available_colors;
+ struct led_mc_color_conversion color_components[LP55XX_MAX_GROUPED_CHAN];
};
struct lp55xx_predef_pattern {
--
2.22.0.214.g8dca754b1e
Dan,
On 10/18/19 2:25 PM, Dan Murphy wrote:
> Add multicolor framework support for the lp55xx family.
>
> Signed-off-by: Dan Murphy <[email protected]>
> ---
> drivers/leds/Kconfig | 1 +
> drivers/leds/leds-lp55xx-common.c | 185 +++++++++++++++++++---
> drivers/leds/leds-lp55xx-common.h | 9 ++
> include/linux/platform_data/leds-lp55xx.h | 7 +
> 4 files changed, 179 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index fb614a6b9afa..5706bf8d8bd1 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -377,6 +377,7 @@ config LEDS_LP50XX
> 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
> + depends on OF
> select FW_LOADER
> select FW_LOADER_USER_HELPER
> help
> diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> index 882ef39e4965..197b87ca5ca2 100644
> --- a/drivers/leds/leds-lp55xx-common.c
> +++ b/drivers/leds/leds-lp55xx-common.c
> @@ -131,14 +131,62 @@ static struct attribute *lp55xx_led_attrs[] = {
> };
> ATTRIBUTE_GROUPS(lp55xx_led);
>
> +#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
> +static int lp55xx_map_channel(struct lp55xx_led *led, int color_id,
> + enum led_brightness brightness)
If you changed the type of the first parameter to
struct led_mc_color_conversion* then you could make this function local
in LED mc class and call it in led_mc_calc_color_components() after
calculating brightness components.
> +{
> + int i;
> +
> + for (i = 0; i < led->mc_cdev.num_leds; i++) {
> + if (led->color_components[i].color_id == color_id) {
> + led->color_components[i].brightness = brightness;
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +#endif
> +
> +static int lp55xx_set_mc_brightness(struct lp55xx_led *led,
> + struct lp55xx_device_config *cfg,
> + enum led_brightness brightness)
> +{
> + int ret = -EINVAL;
> +#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
> + struct led_mc_color_conversion color_components[LP55XX_MAX_GROUPED_CHAN];
You wouldn't need this local variable then.
> + int i;
> +
> + if (!cfg->multicolor_brightness_fn)
> + return -EINVAL;
> +
> + led_mc_calc_color_components(&led->mc_cdev, brightness,
> + color_components);
Because you could pass what you already have in the struct lp55xx_led:
led_mc_calc_color_components(&led->mc_cdev, brightness,
led->color_components);
> +
> + for (i = 0; i < led->mc_cdev.num_leds; i++) {
> + ret = lp55xx_map_channel(led, color_components[i].color_id,
> + color_components[i].brightness);
> + if (ret)
> + return ret;
> + }
And this loop would execute inside the previous call, thus it is to be
optimized out from here.
> +
> + ret = cfg->multicolor_brightness_fn(led);
> +#endif
> + return ret;
> +}
> +
> static int lp55xx_set_brightness(struct led_classdev *cdev,
> enum led_brightness brightness)
> {
> struct lp55xx_led *led = cdev_to_lp55xx_led(cdev);
> struct lp55xx_device_config *cfg = led->chip->cfg;
>
> - led->brightness = (u8)brightness;
> - return cfg->brightness_fn(led);
> + if (led->mc_cdev.num_leds > 1) {
> + return lp55xx_set_mc_brightness(led, cfg, brightness);
> + } else {
> + led->brightness = (u8)brightness;
> + return cfg->brightness_fn(led);
> + }
> }
>
> static int lp55xx_init_led(struct lp55xx_led *led,
> @@ -147,9 +195,9 @@ static int lp55xx_init_led(struct lp55xx_led *led,
> struct lp55xx_platform_data *pdata = chip->pdata;
> struct lp55xx_device_config *cfg = chip->cfg;
> struct device *dev = &chip->cl->dev;
> + int max_channel = cfg->max_channel;
> char name[32];
> int ret;
> - int max_channel = cfg->max_channel;
>
> if (chan >= max_channel) {
> dev_err(dev, "invalid channel: %d / %d\n", chan, max_channel);
> @@ -159,10 +207,34 @@ static int lp55xx_init_led(struct lp55xx_led *led,
> if (pdata->led_config[chan].led_current == 0)
> return 0;
>
> + if (pdata->led_config[chan].name) {
> + led->cdev.name = pdata->led_config[chan].name;
> + } else {
> + snprintf(name, sizeof(name), "%s:channel%d",
> + pdata->label ? : chip->cl->name, chan);
> + led->cdev.name = name;
> + }
> +
> + if (pdata->led_config[chan].num_colors > 1) {
> + led->mc_cdev.led_cdev = &led->cdev;
> + led->cdev.brightness_set_blocking = lp55xx_set_brightness;
> + led->cdev.groups = lp55xx_led_groups;
> + led->mc_cdev.num_leds = pdata->led_config[chan].num_colors;
> + led->mc_cdev.available_colors =
> + pdata->led_config[chan].available_colors;
> + memcpy(led->color_components,
> + pdata->led_config[chan].color_components,
> + sizeof(led->color_components));
> + } else {
> +
> + led->cdev.default_trigger =
> + pdata->led_config[chan].default_trigger;
> + led->cdev.brightness_set_blocking = lp55xx_set_brightness;
> + } led->cdev.groups = lp55xx_led_groups;
> +
> led->led_current = pdata->led_config[chan].led_current;
> led->max_current = pdata->led_config[chan].max_current;
> led->chan_nr = pdata->led_config[chan].chan_nr;
> - led->cdev.default_trigger = pdata->led_config[chan].default_trigger;
>
> if (led->chan_nr >= max_channel) {
> dev_err(dev, "Use channel numbers between 0 and %d\n",
> @@ -170,18 +242,13 @@ static int lp55xx_init_led(struct lp55xx_led *led,
> return -EINVAL;
> }
>
> - led->cdev.brightness_set_blocking = lp55xx_set_brightness;
> - led->cdev.groups = lp55xx_led_groups;
> -
> - if (pdata->led_config[chan].name) {
> - led->cdev.name = pdata->led_config[chan].name;
> - } else {
> - snprintf(name, sizeof(name), "%s:channel%d",
> - pdata->label ? : chip->cl->name, chan);
> - led->cdev.name = name;
> - }
> +#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
> + if (pdata->led_config[chan].num_colors > 1)
> + ret = devm_led_classdev_multicolor_register(dev, &led->mc_cdev);
> + else
> +#endif
> + ret = devm_led_classdev_register(dev, &led->cdev);
>
> - ret = devm_led_classdev_register(dev, &led->cdev);
> if (ret) {
> dev_err(dev, "led register err: %d\n", ret);
> return ret;
> @@ -525,6 +592,82 @@ void lp55xx_unregister_sysfs(struct lp55xx_chip *chip)
> }
> EXPORT_SYMBOL_GPL(lp55xx_unregister_sysfs);
>
> +static int lp5xx_parse_common_child(struct device_node *np,
> + struct lp55xx_led_config *cfg,
> + int chan_num, bool is_multicolor,
> + int color_num)
> +{
> + u32 led_number;
> + int ret;
> +
> + of_property_read_string(np, "chan-name",
> + &cfg[chan_num].name);
> + of_property_read_u8(np, "led-cur",
> + &cfg[chan_num].led_current);
> + of_property_read_u8(np, "max-cur",
> + &cfg[chan_num].max_current);
> +
> + ret = of_property_read_u32(np, "reg", &led_number);
> + if (ret)
> + return ret;
> +
> + if (led_number < 0 || led_number > 6)
> + return -EINVAL;
> +
> + if (is_multicolor)
> + cfg[chan_num].color_components[color_num].output_num =
> + led_number;
> + else
> + cfg[chan_num].chan_nr = led_number;
> +
> + return 0;
> +}
> +
> +static int lp5xx_parse_channel_child(struct device_node *np,
> + struct lp55xx_led_config *cfg,
> + int child_number)
> +{
> + struct device_node *child;
> + int channel_color;
> + int num_colors = 0;
> + u32 color_id;
> + int ret;
> +
> + cfg[child_number].default_trigger =
> + of_get_property(np, "linux,default-trigger", NULL);
> +
> + ret = of_property_read_u32(np, "color", &channel_color);
> + if (ret)
> + channel_color = ret;
> +
> +
> + if (channel_color == LED_COLOR_ID_MULTI) {
> + for_each_child_of_node(np, child) {
> + ret = lp5xx_parse_common_child(child, cfg,
> + child_number, true,
> + num_colors);
> + if (ret)
> + return ret;
> +
> + ret = of_property_read_u32(child, "color", &color_id);
> + if (ret)
> + return ret;
> +
> + cfg[child_number].color_components[num_colors].color_id =
> + color_id;
> + set_bit(color_id, &cfg[child_number].available_colors);
> + num_colors++;
> + }
> +
> + cfg[child_number].num_colors = num_colors;
> + } else {
> + return lp5xx_parse_common_child(np, cfg, child_number, false,
> + num_colors);
> + }
> +
> + return 0;
> +}
> +
> struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
> struct device_node *np)
> {
> @@ -533,6 +676,7 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
> struct lp55xx_led_config *cfg;
> int num_channels;
> int i = 0;
> + int ret;
>
> pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> if (!pdata)
> @@ -552,14 +696,9 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
> pdata->num_channels = num_channels;
>
> for_each_child_of_node(np, child) {
> - cfg[i].chan_nr = i;
> -
> - of_property_read_string(child, "chan-name", &cfg[i].name);
> - of_property_read_u8(child, "led-cur", &cfg[i].led_current);
> - of_property_read_u8(child, "max-cur", &cfg[i].max_current);
> - cfg[i].default_trigger =
> - of_get_property(child, "linux,default-trigger", NULL);
> -
> + ret = lp5xx_parse_channel_child(child, cfg, i);
I went into details of this parsing and finally came up with
the code which is a bit greater in size, but IMHO cleaner.
Note changes in variable naming. It is not even compile-tested.
static int lp55xx_parse_common_child(struct device_node *np,
struct lp55xx_led_config *cfg,
int led_number, int *chan_nr)
{
int ret;
of_property_read_string(np, "chan-name",
&cfg[led_number].name);
of_property_read_u8(np, "led-cur",
&cfg[led_number].led_current);
of_property_read_u8(np, "max-cur",
&cfg[led_number].max_current);
ret = of_property_read_u32(np, "reg", chan_nr);
if (ret)
return ret;
if (chan_nr < 0 || chan_nr > cfg->max_chan_nr) /* side note: new
max_chan_nr property needed in cfg */
return -EINVAL;
return 0;
}
static int lp55xx_parse_mutli_led_child(struct device_node *np,
struct lp55xx_led_config *cfg,
int child_number,
int color_number)
{
int chan_nr, color_id;
ret = lp55xx_parse_common_child(child, cfg, child_number,
color_number,
&chan_nr);
if (ret)
return ret;
ret = of_property_read_u32(child, "color", &color_id);
if (ret)
return ret;
cfg[child_number].color_components[color_number].color_id =
color_id;
cfg[child_number].color_components[color_number].output_num =
chan_nr;
set_bit(color_id, &cfg[child_number].available_colors);
return 0;
}
staitc int lp55xx_parse_mutli_led(struct device_node *np,
struct lp55xx_led_config *cfg,
int child_number)
{
struct device_node *child;
int num_colors = 0, i = 0;
for_each_child_of_node(np, child) {
ret = lp55xx_parse_mutli_led_child(child, cfg, num_colors,
child_number, i))
if (ret)
return ret;
num_colors++;
}
}
static int lp5xx_parse_logical_led(struct device_node *np,
struct lp55xx_led_config *cfg,
int child_number)
{
int led_color, ret;
cfg[child_number].default_trigger =
of_get_property(np, "linux,default-trigger", NULL);
ret = of_property_read_u32(np, "color", &led_color);
if (ret) {
int chan_nr;
ret = lp55xx_parse_common_child(np, cfg, child_number,
&chan_nr);
if (ret < 0)
return ret;
cfg[child_number].chan_nr = chan_nr;
} else if (led_color == LED_COLOR_ID_MULTI) {
return lp55xx_parse_mutli_led(np, cfg, child_number);
} else
return ret;
return 0;
}
for_each_child_of_node(np, child) {
ret = lp55xx_parse_logical_led(child, cfg, i);
if (ret)
return ERR_PTR(-EINVAL);
i++;
}
It maybe worth also to check if channel has not been already taken.
> + if (ret)
> + return ERR_PTR(-EINVAL);
> i++;
> }
>
> diff --git a/drivers/leds/leds-lp55xx-common.h b/drivers/leds/leds-lp55xx-common.h
> index b9b1041e8143..4a0cdbfe54a6 100644
> --- a/drivers/leds/leds-lp55xx-common.h
> +++ b/drivers/leds/leds-lp55xx-common.h
> @@ -12,6 +12,8 @@
> #ifndef _LEDS_LP55XX_COMMON_H
> #define _LEDS_LP55XX_COMMON_H
>
> +#include <linux/led-class-multicolor.h>
> +
> enum lp55xx_engine_index {
> LP55XX_ENGINE_INVALID,
> LP55XX_ENGINE_1,
> @@ -109,6 +111,9 @@ struct lp55xx_device_config {
> /* access brightness register */
> int (*brightness_fn)(struct lp55xx_led *led);
>
> + /* perform brightness value to multiple LEDs */
> + int (*multicolor_brightness_fn)(struct lp55xx_led *led);
> +
> /* current setting function */
> void (*set_led_current) (struct lp55xx_led *led, u8 led_current);
>
> @@ -159,6 +164,8 @@ struct lp55xx_chip {
> * struct lp55xx_led
> * @chan_nr : Channel number
> * @cdev : LED class device
> + * @mc_cdev : Multi color class device
> + * @color_components: Multi color LED map information
> * @led_current : Current setting at each led channel
> * @max_current : Maximun current at each led channel
> * @brightness : Brightness value
> @@ -167,6 +174,8 @@ struct lp55xx_chip {
> struct lp55xx_led {
> int chan_nr;
> struct led_classdev cdev;
> + struct led_classdev_mc mc_cdev;
> + struct led_mc_color_conversion color_components[LP55XX_MAX_GROUPED_CHAN];
> u8 led_current;
> u8 max_current;
> u8 brightness;
> diff --git a/include/linux/platform_data/leds-lp55xx.h b/include/linux/platform_data/leds-lp55xx.h
> index 96a787100fda..ead9d145de0d 100644
> --- a/include/linux/platform_data/leds-lp55xx.h
> +++ b/include/linux/platform_data/leds-lp55xx.h
> @@ -12,17 +12,24 @@
> #ifndef _LEDS_LP55XX_H
> #define _LEDS_LP55XX_H
>
> +#include <linux/led-class-multicolor.h>
> +
> /* Clock configuration */
> #define LP55XX_CLOCK_AUTO 0
> #define LP55XX_CLOCK_INT 1
> #define LP55XX_CLOCK_EXT 2
>
> +#define LP55XX_MAX_GROUPED_CHAN 4
> +
> struct lp55xx_led_config {
> const char *name;
> const char *default_trigger;
> u8 chan_nr;
> u8 led_current; /* mA x10, 0 if led is not connected */
> u8 max_current;
> + int num_colors;
> + unsigned long available_colors;
> + struct led_mc_color_conversion color_components[LP55XX_MAX_GROUPED_CHAN];
> };
>
> struct lp55xx_predef_pattern {
>
--
Best regards,
Jacek Anaszewski
On 10/18/19 11:48 PM, Jacek Anaszewski wrote:
> Dan,
+ ret = lp5xx_parse_channel_child(child, cfg, i);
>
> I went into details of this parsing and finally came up with
> the code which is a bit greater in size, but IMHO cleaner.
> Note changes in variable naming. It is not even compile-tested.
>
> static int lp55xx_parse_common_child(struct device_node *np,
> struct lp55xx_led_config *cfg,
> int led_number, int *chan_nr)
> {
> int ret;
>
> of_property_read_string(np, "chan-name",
> &cfg[led_number].name);
> of_property_read_u8(np, "led-cur",
> &cfg[led_number].led_current);
> of_property_read_u8(np, "max-cur",
> &cfg[led_number].max_current);
>
> ret = of_property_read_u32(np, "reg", chan_nr);
> if (ret)
> return ret;
>
> if (chan_nr < 0 || chan_nr > cfg->max_chan_nr) /* side note: new
> max_chan_nr property needed in cfg */
> return -EINVAL;
>
> return 0;
> }
>
> static int lp55xx_parse_mutli_led_child(struct device_node *np,
> struct lp55xx_led_config *cfg,
> int child_number,
> int color_number)
> {
> int chan_nr, color_id;
>
> ret = lp55xx_parse_common_child(child, cfg, child_number,
> color_number,
> &chan_nr);
> if (ret)
> return ret;
>
> ret = of_property_read_u32(child, "color", &color_id);
> if (ret)
> return ret;
>
> cfg[child_number].color_components[color_number].color_id =
> color_id;
> cfg[child_number].color_components[color_number].output_num =
> chan_nr;
> set_bit(color_id, &cfg[child_number].available_colors);
>
> return 0;
> }
>
> staitc int lp55xx_parse_mutli_led(struct device_node *np,
> struct lp55xx_led_config *cfg,
> int child_number)
> {
> struct device_node *child;
> int num_colors = 0, i = 0;
s/, i = 0//
>
> for_each_child_of_node(np, child) {
> ret = lp55xx_parse_mutli_led_child(child, cfg, num_colors,
> child_number, i))
Replace above call with below:
ret = lp55xx_parse_mutli_led_child(child, cfg, child_number, num_colors);
> if (ret)
> return ret;
> num_colors++;
> }
> }
>
> static int lp5xx_parse_logical_led(struct device_node *np,
> struct lp55xx_led_config *cfg,
> int child_number)
> {
> int led_color, ret;
>
> cfg[child_number].default_trigger =
> of_get_property(np, "linux,default-trigger", NULL);
>
> ret = of_property_read_u32(np, "color", &led_color);
>
> if (ret) {
> int chan_nr;
> ret = lp55xx_parse_common_child(np, cfg, child_number,
> &chan_nr);
> if (ret < 0)
> return ret;
> cfg[child_number].chan_nr = chan_nr;
> } else if (led_color == LED_COLOR_ID_MULTI) {
> return lp55xx_parse_mutli_led(np, cfg, child_number);
> } else
> return ret;
>
> return 0;
> }
>
>
> for_each_child_of_node(np, child) {
> ret = lp55xx_parse_logical_led(child, cfg, i);
> if (ret)
> return ERR_PTR(-EINVAL);
> i++;
> }
>
>
--
Best regards,
Jacek Anaszewski
Dan,
I forgot to mention one thing below.
On 10/18/19 2:25 PM, Dan Murphy wrote:
> Add multicolor framework support for the lp55xx family.
>
> Signed-off-by: Dan Murphy <[email protected]>
> ---
[...]
> - led->cdev.default_trigger = pdata->led_config[chan].default_trigger;
>
> if (led->chan_nr >= max_channel) {
> dev_err(dev, "Use channel numbers between 0 and %d\n",
> @@ -170,18 +242,13 @@ static int lp55xx_init_led(struct lp55xx_led *led,
> return -EINVAL;
> }
>
> - led->cdev.brightness_set_blocking = lp55xx_set_brightness;
> - led->cdev.groups = lp55xx_led_groups;
> -
> - if (pdata->led_config[chan].name) {
> - led->cdev.name = pdata->led_config[chan].name;
> - } else {
> - snprintf(name, sizeof(name), "%s:channel%d",
> - pdata->label ? : chip->cl->name, chan);
> - led->cdev.name = name;
> - }
> +#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
> + if (pdata->led_config[chan].num_colors > 1)
> + ret = devm_led_classdev_multicolor_register(dev, &led->mc_cdev);
> + else
> +#endif
This looks odd. I think that no-ops from previous version were OK.
> + ret = devm_led_classdev_register(dev, &led->cdev);
>
> - ret = devm_led_classdev_register(dev, &led->cdev);
> if (ret) {
> dev_err(dev, "led register err: %d\n", ret);
> return ret;
> @@ -525,6 +592,82 @@ void lp55xx_unregister_sysfs(struct lp55xx_chip *chip)
> }
> EXPORT_SYMBOL_GPL(lp55xx_unregister_sysfs);
>
--
Best regards,
Jacek Anaszewski
Jacek
On 10/18/19 4:56 PM, Jacek Anaszewski wrote:
> On 10/18/19 11:48 PM, Jacek Anaszewski wrote:
>> Dan,
> + ret = lp5xx_parse_channel_child(child, cfg, i);
>> I went into details of this parsing and finally came up with
>> the code which is a bit greater in size, but IMHO cleaner.
>> Note changes in variable naming. It is not even compile-tested.
>>
>> static int lp55xx_parse_common_child(struct device_node *np,
>> struct lp55xx_led_config *cfg,
>> int led_number, int *chan_nr)
>> {
>> int ret;
>>
>> of_property_read_string(np, "chan-name",
>> &cfg[led_number].name);
>> of_property_read_u8(np, "led-cur",
>> &cfg[led_number].led_current);
>> of_property_read_u8(np, "max-cur",
>> &cfg[led_number].max_current);
>>
>> ret = of_property_read_u32(np, "reg", chan_nr);
>> if (ret)
>> return ret;
>>
>> if (chan_nr < 0 || chan_nr > cfg->max_chan_nr) /* side note: new
>> max_chan_nr property needed in cfg */
>> return -EINVAL;
>>
>> return 0;
>> }
>>
>> static int lp55xx_parse_mutli_led_child(struct device_node *np,
>> struct lp55xx_led_config *cfg,
>> int child_number,
>> int color_number)
>> {
>> int chan_nr, color_id;
>>
>> ret = lp55xx_parse_common_child(child, cfg, child_number,
>> color_number,
>> &chan_nr);
>> if (ret)
>> return ret;
>>
>> ret = of_property_read_u32(child, "color", &color_id);
>> if (ret)
>> return ret;
>>
>> cfg[child_number].color_components[color_number].color_id =
>> color_id;
>> cfg[child_number].color_components[color_number].output_num =
>> chan_nr;
>> set_bit(color_id, &cfg[child_number].available_colors);
>>
>> return 0;
>> }
>>
>> staitc int lp55xx_parse_mutli_led(struct device_node *np,
>> struct lp55xx_led_config *cfg,
>> int child_number)
>> {
>> struct device_node *child;
>> int num_colors = 0, i = 0;
> s/, i = 0//
>
>> for_each_child_of_node(np, child) {
>> ret = lp55xx_parse_mutli_led_child(child, cfg, num_colors,
>> child_number, i))
> Replace above call with below:
>
> ret = lp55xx_parse_mutli_led_child(child, cfg, child_number, num_colors);
>
I applied your DT parser patch from the v13 series. Which eliminates
this comment correct?
Dan
Dan,
On 10/22/19 6:37 PM, Dan Murphy wrote:
> Jacek
>
> On 10/18/19 4:56 PM, Jacek Anaszewski wrote:
>> On 10/18/19 11:48 PM, Jacek Anaszewski wrote:
>>> Dan,
>> + ret = lp5xx_parse_channel_child(child, cfg, i);
>>> I went into details of this parsing and finally came up with
>>> the code which is a bit greater in size, but IMHO cleaner.
>>> Note changes in variable naming. It is not even compile-tested.
>>>
>>> static int lp55xx_parse_common_child(struct device_node *np,
>>> struct lp55xx_led_config *cfg,
>>> int led_number, int *chan_nr)
>>> {
>>> int ret;
>>>
>>> of_property_read_string(np, "chan-name",
>>> &cfg[led_number].name);
>>> of_property_read_u8(np, "led-cur",
>>> &cfg[led_number].led_current);
>>> of_property_read_u8(np, "max-cur",
>>> &cfg[led_number].max_current);
>>>
>>> ret = of_property_read_u32(np, "reg", chan_nr);
>>> if (ret)
>>> return ret;
>>>
>>> if (chan_nr < 0 || chan_nr > cfg->max_chan_nr) /* side note:
>>> new
>>> max_chan_nr property needed in cfg */
>>> return -EINVAL;
>>>
>>> return 0;
>>> }
>>>
>>> static int lp55xx_parse_mutli_led_child(struct device_node *np,
>>> struct lp55xx_led_config *cfg,
>>> int child_number,
>>> int color_number)
>>> {
>>> int chan_nr, color_id;
>>>
>>> ret = lp55xx_parse_common_child(child, cfg, child_number,
>>> color_number,
>>> &chan_nr);
>>> if (ret)
>>> return ret;
>>>
>>> ret = of_property_read_u32(child, "color", &color_id);
>>> if (ret)
>>> return ret;
>>>
>>> cfg[child_number].color_components[color_number].color_id =
>>> color_id;
>>> cfg[child_number].color_components[color_number].output_num =
>>> chan_nr;
>>> set_bit(color_id, &cfg[child_number].available_colors);
>>>
>>> return 0;
>>> }
>>>
>>> staitc int lp55xx_parse_mutli_led(struct device_node *np,
>>> struct lp55xx_led_config *cfg,
>>> int child_number)
>>> {
>>> struct device_node *child;
>>> int num_colors = 0, i = 0;
>> s/, i = 0//
>>
>>> for_each_child_of_node(np, child) {
>>> ret = lp55xx_parse_mutli_led_child(child, cfg,
>>> num_colors,
>>> child_number, i))
>> Replace above call with below:
>>
>> ret = lp55xx_parse_mutli_led_child(child, cfg, child_number, num_colors);
>>
> I applied your DT parser patch from the v13 series. Which eliminates
> this comment correct?
Yes, it contains this fix.
--
Best regards,
Jacek Anaszewski
Jacek
On 10/18/19 4:48 PM, Jacek Anaszewski wrote:
> Dan,
>
> On 10/18/19 2:25 PM, Dan Murphy wrote:
>> Add multicolor framework support for the lp55xx family.
>>
>> Signed-off-by: Dan Murphy <[email protected]>
>> ---
>> drivers/leds/Kconfig | 1 +
>> drivers/leds/leds-lp55xx-common.c | 185 +++++++++++++++++++---
>> drivers/leds/leds-lp55xx-common.h | 9 ++
>> include/linux/platform_data/leds-lp55xx.h | 7 +
>> 4 files changed, 179 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index fb614a6b9afa..5706bf8d8bd1 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -377,6 +377,7 @@ config LEDS_LP50XX
>> 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
>> + depends on OF
>> select FW_LOADER
>> select FW_LOADER_USER_HELPER
>> help
>> diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
>> index 882ef39e4965..197b87ca5ca2 100644
>> --- a/drivers/leds/leds-lp55xx-common.c
>> +++ b/drivers/leds/leds-lp55xx-common.c
>> @@ -131,14 +131,62 @@ static struct attribute *lp55xx_led_attrs[] = {
>> };
>> ATTRIBUTE_GROUPS(lp55xx_led);
>>
>> +#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
>> +static int lp55xx_map_channel(struct lp55xx_led *led, int color_id,
>> + enum led_brightness brightness)
> If you changed the type of the first parameter to
> struct led_mc_color_conversion* then you could make this function local
> in LED mc class and call it in led_mc_calc_color_components() after
> calculating brightness components.
I prefer to leave this here and if this code is ever integrated we can
see if there is a common need for the MC class to expose a mapping API.
>
>> +{
>> + int i;
>> +
>> + for (i = 0; i < led->mc_cdev.num_leds; i++) {
>> + if (led->color_components[i].color_id == color_id) {
>> + led->color_components[i].brightness = brightness;
>> + return 0;
>> + }
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +#endif
>> +
>> +static int lp55xx_set_mc_brightness(struct lp55xx_led *led,
>> + struct lp55xx_device_config *cfg,
>> + enum led_brightness brightness)
>> +{
>> + int ret = -EINVAL;
>> +#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
>> + struct led_mc_color_conversion color_components[LP55XX_MAX_GROUPED_CHAN];
> You wouldn't need this local variable then.
>> + int i;
>> +
>> + if (!cfg->multicolor_brightness_fn)
>> + return -EINVAL;
>> +
>> + led_mc_calc_color_components(&led->mc_cdev, brightness,
>> + color_components);
> Because you could pass what you already have in the struct lp55xx_led:
>
> led_mc_calc_color_components(&led->mc_cdev, brightness,
> led->color_components);
Well that is not entirely accurate the led->color_components is the data
that we have from the DT that should not be changed. Passing this into
the MC calc function would mean that the framework would have to map the
output to the color_id. As I indicated above for now I don't think the
MC class should do any mapping of color_id to the output.
>
>> +
>> + for (i = 0; i < led->mc_cdev.num_leds; i++) {
>> + ret = lp55xx_map_channel(led, color_components[i].color_id,
>> + color_components[i].brightness);
>> + if (ret)
>> + return ret;
>> + }
> And this loop would execute inside the previous call, thus it is to be
> optimized out from here.
>
>> +
>> + ret = cfg->multicolor_brightness_fn(led);
>> +#endif
>> + return ret;
>> +}
>> +
>> static int lp55xx_set_brightness(struct led_classdev *cdev,
>> enum led_brightness brightness)
>> {
>> struct lp55xx_led *led = cdev_to_lp55xx_led(cdev);
>> struct lp55xx_device_config *cfg = led->chip->cfg;
>>
>> - led->brightness = (u8)brightness;
>> - return cfg->brightness_fn(led);
>> + if (led->mc_cdev.num_leds > 1) {
>> + return lp55xx_set_mc_brightness(led, cfg, brightness);
>> + } else {
>> + led->brightness = (u8)brightness;
>> + return cfg->brightness_fn(led);
>> + }
>> }
>>
>> static int lp55xx_init_led(struct lp55xx_led *led,
>> @@ -147,9 +195,9 @@ static int lp55xx_init_led(struct lp55xx_led *led,
>> struct lp55xx_platform_data *pdata = chip->pdata;
>> struct lp55xx_device_config *cfg = chip->cfg;
>> struct device *dev = &chip->cl->dev;
>> + int max_channel = cfg->max_channel;
>> char name[32];
>> int ret;
>> - int max_channel = cfg->max_channel;
>>
>> if (chan >= max_channel) {
>> dev_err(dev, "invalid channel: %d / %d\n", chan, max_channel);
>> @@ -159,10 +207,34 @@ static int lp55xx_init_led(struct lp55xx_led *led,
>> if (pdata->led_config[chan].led_current == 0)
>> return 0;
>>
>> + if (pdata->led_config[chan].name) {
>> + led->cdev.name = pdata->led_config[chan].name;
>> + } else {
>> + snprintf(name, sizeof(name), "%s:channel%d",
>> + pdata->label ? : chip->cl->name, chan);
>> + led->cdev.name = name;
>> + }
>> +
>> + if (pdata->led_config[chan].num_colors > 1) {
>> + led->mc_cdev.led_cdev = &led->cdev;
>> + led->cdev.brightness_set_blocking = lp55xx_set_brightness;
>> + led->cdev.groups = lp55xx_led_groups;
>> + led->mc_cdev.num_leds = pdata->led_config[chan].num_colors;
>> + led->mc_cdev.available_colors =
>> + pdata->led_config[chan].available_colors;
>> + memcpy(led->color_components,
>> + pdata->led_config[chan].color_components,
>> + sizeof(led->color_components));
>> + } else {
>> +
>> + led->cdev.default_trigger =
>> + pdata->led_config[chan].default_trigger;
>> + led->cdev.brightness_set_blocking = lp55xx_set_brightness;
>> + } led->cdev.groups = lp55xx_led_groups;
>> +
>> led->led_current = pdata->led_config[chan].led_current;
>> led->max_current = pdata->led_config[chan].max_current;
>> led->chan_nr = pdata->led_config[chan].chan_nr;
>> - led->cdev.default_trigger = pdata->led_config[chan].default_trigger;
>>
>> if (led->chan_nr >= max_channel) {
>> dev_err(dev, "Use channel numbers between 0 and %d\n",
>> @@ -170,18 +242,13 @@ static int lp55xx_init_led(struct lp55xx_led *led,
>> return -EINVAL;
>> }
>>
>> - led->cdev.brightness_set_blocking = lp55xx_set_brightness;
>> - led->cdev.groups = lp55xx_led_groups;
>> -
>> - if (pdata->led_config[chan].name) {
>> - led->cdev.name = pdata->led_config[chan].name;
>> - } else {
>> - snprintf(name, sizeof(name), "%s:channel%d",
>> - pdata->label ? : chip->cl->name, chan);
>> - led->cdev.name = name;
>> - }
>> +#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
>> + if (pdata->led_config[chan].num_colors > 1)
>> + ret = devm_led_classdev_multicolor_register(dev, &led->mc_cdev);
>> + else
>> +#endif
>> + ret = devm_led_classdev_register(dev, &led->cdev);
>>
>> - ret = devm_led_classdev_register(dev, &led->cdev);
>> if (ret) {
>> dev_err(dev, "led register err: %d\n", ret);
>> return ret;
>> @@ -525,6 +592,82 @@ void lp55xx_unregister_sysfs(struct lp55xx_chip *chip)
>> }
>> EXPORT_SYMBOL_GPL(lp55xx_unregister_sysfs);
>>
>> +static int lp5xx_parse_common_child(struct device_node *np,
>> + struct lp55xx_led_config *cfg,
>> + int chan_num, bool is_multicolor,
>> + int color_num)
>> +{
>> + u32 led_number;
>> + int ret;
>> +
>> + of_property_read_string(np, "chan-name",
>> + &cfg[chan_num].name);
>> + of_property_read_u8(np, "led-cur",
>> + &cfg[chan_num].led_current);
>> + of_property_read_u8(np, "max-cur",
>> + &cfg[chan_num].max_current);
>> +
>> + ret = of_property_read_u32(np, "reg", &led_number);
>> + if (ret)
>> + return ret;
>> +
>> + if (led_number < 0 || led_number > 6)
>> + return -EINVAL;
>> +
>> + if (is_multicolor)
>> + cfg[chan_num].color_components[color_num].output_num =
>> + led_number;
>> + else
>> + cfg[chan_num].chan_nr = led_number;
>> +
>> + return 0;
>> +}
>> +
>> +static int lp5xx_parse_channel_child(struct device_node *np,
>> + struct lp55xx_led_config *cfg,
>> + int child_number)
>> +{
>> + struct device_node *child;
>> + int channel_color;
>> + int num_colors = 0;
>> + u32 color_id;
>> + int ret;
>> +
>> + cfg[child_number].default_trigger =
>> + of_get_property(np, "linux,default-trigger", NULL);
>> +
>> + ret = of_property_read_u32(np, "color", &channel_color);
>> + if (ret)
>> + channel_color = ret;
>> +
>> +
>> + if (channel_color == LED_COLOR_ID_MULTI) {
>> + for_each_child_of_node(np, child) {
>> + ret = lp5xx_parse_common_child(child, cfg,
>> + child_number, true,
>> + num_colors);
>> + if (ret)
>> + return ret;
>> +
>> + ret = of_property_read_u32(child, "color", &color_id);
>> + if (ret)
>> + return ret;
>> +
>> + cfg[child_number].color_components[num_colors].color_id =
>> + color_id;
>> + set_bit(color_id, &cfg[child_number].available_colors);
>> + num_colors++;
>> + }
>> +
>> + cfg[child_number].num_colors = num_colors;
>> + } else {
>> + return lp5xx_parse_common_child(np, cfg, child_number, false,
>> + num_colors);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
>> struct device_node *np)
>> {
>> @@ -533,6 +676,7 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
>> struct lp55xx_led_config *cfg;
>> int num_channels;
>> int i = 0;
>> + int ret;
>>
>> pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> if (!pdata)
>> @@ -552,14 +696,9 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
>> pdata->num_channels = num_channels;
>>
>> for_each_child_of_node(np, child) {
>> - cfg[i].chan_nr = i;
>> -
>> - of_property_read_string(child, "chan-name", &cfg[i].name);
>> - of_property_read_u8(child, "led-cur", &cfg[i].led_current);
>> - of_property_read_u8(child, "max-cur", &cfg[i].max_current);
>> - cfg[i].default_trigger =
>> - of_get_property(child, "linux,default-trigger", NULL);
>> -
>> + ret = lp5xx_parse_channel_child(child, cfg, i);
> I went into details of this parsing and finally came up with
> the code which is a bit greater in size, but IMHO cleaner.
> Note changes in variable naming. It is not even compile-tested.
>
> static int lp55xx_parse_common_child(struct device_node *np,
> struct lp55xx_led_config *cfg,
> int led_number, int *chan_nr)
> {
> int ret;
>
> of_property_read_string(np, "chan-name",
> &cfg[led_number].name);
> of_property_read_u8(np, "led-cur",
> &cfg[led_number].led_current);
> of_property_read_u8(np, "max-cur",
> &cfg[led_number].max_current);
>
> ret = of_property_read_u32(np, "reg", chan_nr);
> if (ret)
> return ret;
>
> if (chan_nr < 0 || chan_nr > cfg->max_chan_nr) /* side note: new
> max_chan_nr property needed in cfg */
> return -EINVAL;
>
> return 0;
> }
>
> static int lp55xx_parse_mutli_led_child(struct device_node *np,
> struct lp55xx_led_config *cfg,
> int child_number,
> int color_number)
> {
> int chan_nr, color_id;
>
> ret = lp55xx_parse_common_child(child, cfg, child_number,
> color_number,
> &chan_nr);
> if (ret)
> return ret;
>
> ret = of_property_read_u32(child, "color", &color_id);
> if (ret)
> return ret;
>
> cfg[child_number].color_components[color_number].color_id =
> color_id;
> cfg[child_number].color_components[color_number].output_num =
> chan_nr;
> set_bit(color_id, &cfg[child_number].available_colors);
>
> return 0;
> }
>
> staitc int lp55xx_parse_mutli_led(struct device_node *np,
> struct lp55xx_led_config *cfg,
> int child_number)
> {
> struct device_node *child;
> int num_colors = 0, i = 0;
>
> for_each_child_of_node(np, child) {
> ret = lp55xx_parse_mutli_led_child(child, cfg, num_colors,
> child_number, i))
> if (ret)
> return ret;
> num_colors++;
> }
> }
>
> static int lp5xx_parse_logical_led(struct device_node *np,
> struct lp55xx_led_config *cfg,
> int child_number)
> {
> int led_color, ret;
>
> cfg[child_number].default_trigger =
> of_get_property(np, "linux,default-trigger", NULL);
>
> ret = of_property_read_u32(np, "color", &led_color);
>
> if (ret) {
> int chan_nr;
> ret = lp55xx_parse_common_child(np, cfg, child_number,
> &chan_nr);
> if (ret < 0)
> return ret;
> cfg[child_number].chan_nr = chan_nr;
> } else if (led_color == LED_COLOR_ID_MULTI) {
> return lp55xx_parse_mutli_led(np, cfg, child_number);
> } else
> return ret;
>
> return 0;
> }
>
>
> for_each_child_of_node(np, child) {
> ret = lp55xx_parse_logical_led(child, cfg, i);
> if (ret)
> return ERR_PTR(-EINVAL);
> i++;
> }
>
>
> It maybe worth also to check if channel has not been already taken.
>
I will board test this solution.
Dan
Jacek
On 10/18/19 5:02 PM, Jacek Anaszewski wrote:
> Dan,
>
> I forgot to mention one thing below.
>
> On 10/18/19 2:25 PM, Dan Murphy wrote:
>> Add multicolor framework support for the lp55xx family.
>>
>> Signed-off-by: Dan Murphy <[email protected]>
>> ---
> [...]
>
>> - led->cdev.default_trigger = pdata->led_config[chan].default_trigger;
>>
>> if (led->chan_nr >= max_channel) {
>> dev_err(dev, "Use channel numbers between 0 and %d\n",
>> @@ -170,18 +242,13 @@ static int lp55xx_init_led(struct lp55xx_led *led,
>> return -EINVAL;
>> }
>>
>> - led->cdev.brightness_set_blocking = lp55xx_set_brightness;
>> - led->cdev.groups = lp55xx_led_groups;
>> -
>> - if (pdata->led_config[chan].name) {
>> - led->cdev.name = pdata->led_config[chan].name;
>> - } else {
>> - snprintf(name, sizeof(name), "%s:channel%d",
>> - pdata->label ? : chip->cl->name, chan);
>> - led->cdev.name = name;
>> - }
>> +#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
>> + if (pdata->led_config[chan].num_colors > 1)
>> + ret = devm_led_classdev_multicolor_register(dev, &led->mc_cdev);
>> + else
>> +#endif
> This looks odd. I think that no-ops from previous version were OK.
Hmm. I will add it back but there were issues compiling with modules.
But maybe if I use IS_ENABLED in the header that may solve the issue
Dan
Dan,
On 10/23/19 2:22 PM, Dan Murphy wrote:
> Jacek
>
> On 10/18/19 4:48 PM, Jacek Anaszewski wrote:
>> Dan,
>>
>> On 10/18/19 2:25 PM, Dan Murphy wrote:
>>> Add multicolor framework support for the lp55xx family.
>>>
>>> Signed-off-by: Dan Murphy <[email protected]>
>>> ---
>>> drivers/leds/Kconfig | 1 +
>>> drivers/leds/leds-lp55xx-common.c | 185 +++++++++++++++++++---
>>> drivers/leds/leds-lp55xx-common.h | 9 ++
>>> include/linux/platform_data/leds-lp55xx.h | 7 +
>>> 4 files changed, 179 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index fb614a6b9afa..5706bf8d8bd1 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -377,6 +377,7 @@ config LEDS_LP50XX
>>> 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
>>> + depends on OF
>>> select FW_LOADER
>>> select FW_LOADER_USER_HELPER
>>> help
>>> diff --git a/drivers/leds/leds-lp55xx-common.c
>>> b/drivers/leds/leds-lp55xx-common.c
>>> index 882ef39e4965..197b87ca5ca2 100644
>>> --- a/drivers/leds/leds-lp55xx-common.c
>>> +++ b/drivers/leds/leds-lp55xx-common.c
>>> @@ -131,14 +131,62 @@ static struct attribute *lp55xx_led_attrs[] = {
>>> };
>>> ATTRIBUTE_GROUPS(lp55xx_led);
>>> +#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
>>> +static int lp55xx_map_channel(struct lp55xx_led *led, int color_id,
>>> + enum led_brightness brightness)
>> If you changed the type of the first parameter to
>> struct led_mc_color_conversion* then you could make this function local
>> in LED mc class and call it in led_mc_calc_color_components() after
>> calculating brightness components.
>
> I prefer to leave this here and if this code is ever integrated we can
> see if there is a common need for the MC class to expose a mapping API.
>
>>
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i < led->mc_cdev.num_leds; i++) {
>>> + if (led->color_components[i].color_id == color_id) {
>>> + led->color_components[i].brightness = brightness;
>>> + return 0;
>>> + }
>>> + }
>>> +
>>> + return -EINVAL;
>>> +}
>>> +#endif
>>> +
>>> +static int lp55xx_set_mc_brightness(struct lp55xx_led *led,
>>> + struct lp55xx_device_config *cfg,
>>> + enum led_brightness brightness)
>>> +{
>>> + int ret = -EINVAL;
>>> +#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
>>> + struct led_mc_color_conversion
>>> color_components[LP55XX_MAX_GROUPED_CHAN];
>> You wouldn't need this local variable then.
>
>>> + int i;
>>> +
>>> + if (!cfg->multicolor_brightness_fn)
>>> + return -EINVAL;
>>> +
>>> + led_mc_calc_color_components(&led->mc_cdev, brightness,
>>> + color_components);
>> Because you could pass what you already have in the struct lp55xx_led:
>>
>> led_mc_calc_color_components(&led->mc_cdev, brightness,
>> led->color_components);
>
> Well that is not entirely accurate the led->color_components is the data
> that we have from the DT that should not be changed. Passing this into
> the MC calc function would mean that the framework would have to map the
> output to the color_id. As I indicated above for now I don't think the
> MC class should do any mapping of color_id to the output.
I proposed to use color_components because it is already there
and has required data in place. You could always copy
that data to some other local structure but it would be unnecessary
overhead.
Anyway, all what I proposed here are just nice-to-have details,
that can be covered in the future.
--
Best regards,
Jacek Anaszewski
Jacek
On 10/22/19 12:41 PM, Jacek Anaszewski wrote:
> Dan,
>
> On 10/22/19 6:37 PM, Dan Murphy wrote:
>> Jacek
>>
>> On 10/18/19 4:56 PM, Jacek Anaszewski wrote:
>>> On 10/18/19 11:48 PM, Jacek Anaszewski wrote:
>>>> Dan,
>>> + ret = lp5xx_parse_channel_child(child, cfg, i);
>>>> I went into details of this parsing and finally came up with
>>>> the code which is a bit greater in size, but IMHO cleaner.
>>>> Note changes in variable naming. It is not even compile-tested.
>>>>
>>>> static int lp55xx_parse_common_child(struct device_node *np,
>>>> struct lp55xx_led_config *cfg,
>>>> int led_number, int *chan_nr)
>>>> {
>>>> int ret;
>>>>
>>>> of_property_read_string(np, "chan-name",
>>>> &cfg[led_number].name);
>>>> of_property_read_u8(np, "led-cur",
>>>> &cfg[led_number].led_current);
>>>> of_property_read_u8(np, "max-cur",
>>>> &cfg[led_number].max_current);
>>>>
>>>> ret = of_property_read_u32(np, "reg", chan_nr);
>>>> if (ret)
>>>> return ret;
>>>>
>>>> if (chan_nr < 0 || chan_nr > cfg->max_chan_nr) /* side note:
>>>> new
>>>> max_chan_nr property needed in cfg */
>>>> return -EINVAL;
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> static int lp55xx_parse_mutli_led_child(struct device_node *np,
>>>> struct lp55xx_led_config *cfg,
>>>> int child_number,
>>>> int color_number)
>>>> {
>>>> int chan_nr, color_id;
>>>>
>>>> ret = lp55xx_parse_common_child(child, cfg, child_number,
>>>> color_number,
>>>> &chan_nr);
>>>> if (ret)
>>>> return ret;
>>>>
>>>> ret = of_property_read_u32(child, "color", &color_id);
>>>> if (ret)
>>>> return ret;
>>>>
>>>> cfg[child_number].color_components[color_number].color_id =
>>>> color_id;
>>>> cfg[child_number].color_components[color_number].output_num =
>>>> chan_nr;
>>>> set_bit(color_id, &cfg[child_number].available_colors);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> staitc int lp55xx_parse_mutli_led(struct device_node *np,
>>>> struct lp55xx_led_config *cfg,
>>>> int child_number)
>>>> {
>>>> struct device_node *child;
>>>> int num_colors = 0, i = 0;
>>> s/, i = 0//
>>>
>>>> for_each_child_of_node(np, child) {
>>>> ret = lp55xx_parse_mutli_led_child(child, cfg,
>>>> num_colors,
>>>> child_number, i))
>>> Replace above call with below:
>>>
>>> ret = lp55xx_parse_mutli_led_child(child, cfg, child_number, num_colors);
>>>
>> I applied your DT parser patch from the v13 series. Which eliminates
>> this comment correct?
> Yes, it contains this fix.
>
OK I added your patch and it broke a lot of the DT parsing for the LP55xx.
I would prefer to stick with the original code without having to
re-write this again.
Dan
Jacek
On 10/25/19 1:13 PM, Jacek Anaszewski wrote:
> Dan,
>
> On 10/25/19 7:57 PM, Dan Murphy wrote:
>> Jacek
>>
>> On 10/22/19 12:41 PM, Jacek Anaszewski wrote:
>>> Dan,
>>>
>>> On 10/22/19 6:37 PM, Dan Murphy wrote:
>>>> Jacek
>>>>
>>>> On 10/18/19 4:56 PM, Jacek Anaszewski wrote:
>>>>> On 10/18/19 11:48 PM, Jacek Anaszewski wrote:
>>>>>> Dan,
>>>>> + ret = lp5xx_parse_channel_child(child, cfg, i);
>>>>>> I went into details of this parsing and finally came up with
>>>>>> the code which is a bit greater in size, but IMHO cleaner.
>>>>>> Note changes in variable naming. It is not even compile-tested.
>>>>>>
>>>>>> static int lp55xx_parse_common_child(struct device_node *np,
>>>>>> struct lp55xx_led_config *cfg,
>>>>>> int led_number, int *chan_nr)
>>>>>> {
>>>>>> int ret;
>>>>>>
>>>>>> of_property_read_string(np, "chan-name",
>>>>>> &cfg[led_number].name);
>>>>>> of_property_read_u8(np, "led-cur",
>>>>>> &cfg[led_number].led_current);
>>>>>> of_property_read_u8(np, "max-cur",
>>>>>> &cfg[led_number].max_current);
>>>>>>
>>>>>> ret = of_property_read_u32(np, "reg", chan_nr);
>>>>>> if (ret)
>>>>>> return ret;
>>>>>>
>>>>>> if (chan_nr < 0 || chan_nr > cfg->max_chan_nr) /* side note:
>>>>>> new
>>>>>> max_chan_nr property needed in cfg */
>>>>>> return -EINVAL;
>>>>>>
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> static int lp55xx_parse_mutli_led_child(struct device_node *np,
>>>>>> struct lp55xx_led_config
>>>>>> *cfg,
>>>>>> int child_number,
>>>>>> int color_number)
>>>>>> {
>>>>>> int chan_nr, color_id;
>>>>>>
>>>>>> ret = lp55xx_parse_common_child(child, cfg, child_number,
>>>>>> color_number,
>>>>>> &chan_nr);
>>>>>> if (ret)
>>>>>> return ret;
>>>>>>
>>>>>> ret = of_property_read_u32(child, "color", &color_id);
>>>>>> if (ret)
>>>>>> return ret;
>>>>>>
>>>>>> cfg[child_number].color_components[color_number].color_id =
>>>>>> color_id;
>>>>>>
>>>>>> cfg[child_number].color_components[color_number].output_num =
>>>>>> chan_nr;
>>>>>> set_bit(color_id, &cfg[child_number].available_colors);
>>>>>>
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> staitc int lp55xx_parse_mutli_led(struct device_node *np,
>>>>>> struct lp55xx_led_config *cfg,
>>>>>> int child_number)
>>>>>> {
>>>>>> struct device_node *child;
>>>>>> int num_colors = 0, i = 0;
>>>>> s/, i = 0//
>>>>>
>>>>>> for_each_child_of_node(np, child) {
>>>>>> ret = lp55xx_parse_mutli_led_child(child, cfg,
>>>>>> num_colors,
>>>>>> child_number, i))
>>>>> Replace above call with below:
>>>>>
>>>>> ret = lp55xx_parse_mutli_led_child(child, cfg, child_number,
>>>>> num_colors);
>>>>>
>>>> I applied your DT parser patch from the v13 series. Which eliminates
>>>> this comment correct?
>>> Yes, it contains this fix.
>>>
>> OK I added your patch and it broke a lot of the DT parsing for the LP55xx.
>>
>> I would prefer to stick with the original code without having to
>> re-write this again.
> Let me test that with Qemu first.
>
max_channel is never set so not sure where that is supposed to come from
since each child device has different number of channels. And if the
user has to populate that information from the DT then it does not make
sense as the user would already be aware of the number of channels.
This information would have to come from the child device some how and
the children do not have access to the structure to set it.
In checking the chan_nr to the max_channels you are comparing a pointer
to an integer. Easy fix but did not solve the registration issues.
cfg->num_colors is not set anywhere so the registration always goes to
base led_registration. Unless we key off a different value to determine
to register to multicolor class or not. Or we default this to the
multi_class registration to figure out how to register based on
available_colors.
That is what I am seeing so far in my debugging and I still don't have
it working.
Dan
Dan,
On 10/25/19 7:57 PM, Dan Murphy wrote:
> Jacek
>
> On 10/22/19 12:41 PM, Jacek Anaszewski wrote:
>> Dan,
>>
>> On 10/22/19 6:37 PM, Dan Murphy wrote:
>>> Jacek
>>>
>>> On 10/18/19 4:56 PM, Jacek Anaszewski wrote:
>>>> On 10/18/19 11:48 PM, Jacek Anaszewski wrote:
>>>>> Dan,
>>>> + ret = lp5xx_parse_channel_child(child, cfg, i);
>>>>> I went into details of this parsing and finally came up with
>>>>> the code which is a bit greater in size, but IMHO cleaner.
>>>>> Note changes in variable naming. It is not even compile-tested.
>>>>>
>>>>> static int lp55xx_parse_common_child(struct device_node *np,
>>>>> struct lp55xx_led_config *cfg,
>>>>> int led_number, int *chan_nr)
>>>>> {
>>>>> int ret;
>>>>>
>>>>> of_property_read_string(np, "chan-name",
>>>>> &cfg[led_number].name);
>>>>> of_property_read_u8(np, "led-cur",
>>>>> &cfg[led_number].led_current);
>>>>> of_property_read_u8(np, "max-cur",
>>>>> &cfg[led_number].max_current);
>>>>>
>>>>> ret = of_property_read_u32(np, "reg", chan_nr);
>>>>> if (ret)
>>>>> return ret;
>>>>>
>>>>> if (chan_nr < 0 || chan_nr > cfg->max_chan_nr) /* side note:
>>>>> new
>>>>> max_chan_nr property needed in cfg */
>>>>> return -EINVAL;
>>>>>
>>>>> return 0;
>>>>> }
>>>>>
>>>>> static int lp55xx_parse_mutli_led_child(struct device_node *np,
>>>>> struct lp55xx_led_config
>>>>> *cfg,
>>>>> int child_number,
>>>>> int color_number)
>>>>> {
>>>>> int chan_nr, color_id;
>>>>>
>>>>> ret = lp55xx_parse_common_child(child, cfg, child_number,
>>>>> color_number,
>>>>> &chan_nr);
>>>>> if (ret)
>>>>> return ret;
>>>>>
>>>>> ret = of_property_read_u32(child, "color", &color_id);
>>>>> if (ret)
>>>>> return ret;
>>>>>
>>>>> cfg[child_number].color_components[color_number].color_id =
>>>>> color_id;
>>>>>
>>>>> cfg[child_number].color_components[color_number].output_num =
>>>>> chan_nr;
>>>>> set_bit(color_id, &cfg[child_number].available_colors);
>>>>>
>>>>> return 0;
>>>>> }
>>>>>
>>>>> staitc int lp55xx_parse_mutli_led(struct device_node *np,
>>>>> struct lp55xx_led_config *cfg,
>>>>> int child_number)
>>>>> {
>>>>> struct device_node *child;
>>>>> int num_colors = 0, i = 0;
>>>> s/, i = 0//
>>>>
>>>>> for_each_child_of_node(np, child) {
>>>>> ret = lp55xx_parse_mutli_led_child(child, cfg,
>>>>> num_colors,
>>>>> child_number, i))
>>>> Replace above call with below:
>>>>
>>>> ret = lp55xx_parse_mutli_led_child(child, cfg, child_number,
>>>> num_colors);
>>>>
>>> I applied your DT parser patch from the v13 series. Which eliminates
>>> this comment correct?
>> Yes, it contains this fix.
>>
> OK I added your patch and it broke a lot of the DT parsing for the LP55xx.
>
> I would prefer to stick with the original code without having to
> re-write this again.
Let me test that with Qemu first.
--
Best regards,
Jacek Anaszewski
Dan,
On 10/25/19 8:18 PM, Dan Murphy wrote:
> Jacek
>
> On 10/25/19 1:13 PM, Jacek Anaszewski wrote:
>> Dan,
>>
>> On 10/25/19 7:57 PM, Dan Murphy wrote:
>>> Jacek
>>>
>>> On 10/22/19 12:41 PM, Jacek Anaszewski wrote:
>>>> Dan,
>>>>
>>>> On 10/22/19 6:37 PM, Dan Murphy wrote:
>>>>> Jacek
>>>>>
>>>>> On 10/18/19 4:56 PM, Jacek Anaszewski wrote:
>>>>>> On 10/18/19 11:48 PM, Jacek Anaszewski wrote:
>>>>>>> Dan,
>>>>>> + ret = lp5xx_parse_channel_child(child, cfg, i);
>>>>>>> I went into details of this parsing and finally came up with
>>>>>>> the code which is a bit greater in size, but IMHO cleaner.
>>>>>>> Note changes in variable naming. It is not even compile-tested.
>>>>>>>
>>>>>>> static int lp55xx_parse_common_child(struct device_node *np,
>>>>>>> struct lp55xx_led_config
>>>>>>> *cfg,
>>>>>>> int led_number, int *chan_nr)
>>>>>>> {
>>>>>>> int ret;
>>>>>>>
>>>>>>> of_property_read_string(np, "chan-name",
>>>>>>> &cfg[led_number].name);
>>>>>>> of_property_read_u8(np, "led-cur",
>>>>>>> &cfg[led_number].led_current);
>>>>>>> of_property_read_u8(np, "max-cur",
>>>>>>> &cfg[led_number].max_current);
>>>>>>>
>>>>>>> ret = of_property_read_u32(np, "reg", chan_nr);
>>>>>>> if (ret)
>>>>>>> return ret;
>>>>>>>
>>>>>>> if (chan_nr < 0 || chan_nr > cfg->max_chan_nr) /* side
>>>>>>> note:
>>>>>>> new
>>>>>>> max_chan_nr property needed in cfg */
>>>>>>> return -EINVAL;
>>>>>>>
>>>>>>> return 0;
>>>>>>> }
>>>>>>>
>>>>>>> static int lp55xx_parse_mutli_led_child(struct device_node *np,
>>>>>>> struct lp55xx_led_config
>>>>>>> *cfg,
>>>>>>> int child_number,
>>>>>>> int color_number)
>>>>>>> {
>>>>>>> int chan_nr, color_id;
>>>>>>>
>>>>>>> ret = lp55xx_parse_common_child(child, cfg, child_number,
>>>>>>> color_number,
>>>>>>> &chan_nr);
>>>>>>> if (ret)
>>>>>>> return ret;
>>>>>>>
>>>>>>> ret = of_property_read_u32(child, "color", &color_id);
>>>>>>> if (ret)
>>>>>>> return ret;
>>>>>>>
>>>>>>>
>>>>>>> cfg[child_number].color_components[color_number].color_id =
>>>>>>> color_id;
>>>>>>>
>>>>>>> cfg[child_number].color_components[color_number].output_num =
>>>>>>> chan_nr;
>>>>>>> set_bit(color_id, &cfg[child_number].available_colors);
>>>>>>>
>>>>>>> return 0;
>>>>>>> }
>>>>>>>
>>>>>>> staitc int lp55xx_parse_mutli_led(struct device_node *np,
>>>>>>> struct lp55xx_led_config *cfg,
>>>>>>> int child_number)
>>>>>>> {
>>>>>>> struct device_node *child;
>>>>>>> int num_colors = 0, i = 0;
>>>>>> s/, i = 0//
>>>>>>
>>>>>>> for_each_child_of_node(np, child) {
>>>>>>> ret = lp55xx_parse_mutli_led_child(child, cfg,
>>>>>>> num_colors,
>>>>>>>
>>>>>>> child_number, i))
>>>>>> Replace above call with below:
>>>>>>
>>>>>> ret = lp55xx_parse_mutli_led_child(child, cfg, child_number,
>>>>>> num_colors);
>>>>>>
>>>>> I applied your DT parser patch from the v13 series. Which eliminates
>>>>> this comment correct?
>>>> Yes, it contains this fix.
>>>>
>>> OK I added your patch and it broke a lot of the DT parsing for the
>>> LP55xx.
>>>
>>> I would prefer to stick with the original code without having to
>>> re-write this again.
>> Let me test that with Qemu first.
>>
> max_channel is never set so not sure where that is supposed to come from
> since each child device has different number of channels. And if the
> user has to populate that information from the DT then it does not make
> sense as the user would already be aware of the number of channels.
> This information would have to come from the child device some how and
> the children do not have access to the structure to set it.
This was my silent assumption that the child will initialize that.
And I was not thoroughly seeking the most proper place for this
property, just chose first I could think . You are free to
change its location so that it was accessible for the child.
>
> In checking the chan_nr to the max_channels you are comparing a pointer
> to an integer. Easy fix but did not solve the registration issues.
>
> cfg->num_colors is not set anywhere so the registration always goes to
> base led_registration. Unless we key off a different value to determine
> to register to multicolor class or not. Or we default this to the
> multi_class registration to figure out how to register based on
> available_colors.
You need to add below at the end of lp55xx_parse_mutli_led():
cfg[child_number].num_colors = num_colors;
and below in led_parse_logical_led() at the end of the
"if (ret)" branch.
cfg[child_number].num_colors = 1;
> That is what I am seeing so far in my debugging and I still don't have
> it working.
I didn't pretend it was flawless, just wanted to show general idea
how I would see that.
--
Best regards,
Jacek Anaszewski
Jacek
On 10/25/19 1:56 PM, Jacek Anaszewski wrote:
> Dan,
>
> On 10/25/19 8:18 PM, Dan Murphy wrote:
>> Jacek
>>
>> On 10/25/19 1:13 PM, Jacek Anaszewski wrote:
>>> Dan,
>>>
>>> On 10/25/19 7:57 PM, Dan Murphy wrote:
>>>> Jacek
>>>>
>>>> On 10/22/19 12:41 PM, Jacek Anaszewski wrote:
>>>>> Dan,
>>>>>
>>>>> On 10/22/19 6:37 PM, Dan Murphy wrote:
>>>>>> Jacek
>>>>>>
>>>>>> On 10/18/19 4:56 PM, Jacek Anaszewski wrote:
>>>>>>> On 10/18/19 11:48 PM, Jacek Anaszewski wrote:
>>>>>>>> Dan,
>>>>>>> + ret = lp5xx_parse_channel_child(child, cfg, i);
>>>>>>>> I went into details of this parsing and finally came up with
>>>>>>>> the code which is a bit greater in size, but IMHO cleaner.
>>>>>>>> Note changes in variable naming. It is not even compile-tested.
>>>>>>>>
>>>>>>>> static int lp55xx_parse_common_child(struct device_node *np,
>>>>>>>> struct lp55xx_led_config
>>>>>>>> *cfg,
>>>>>>>> int led_number, int *chan_nr)
>>>>>>>> {
>>>>>>>> int ret;
>>>>>>>>
>>>>>>>> of_property_read_string(np, "chan-name",
>>>>>>>> &cfg[led_number].name);
>>>>>>>> of_property_read_u8(np, "led-cur",
>>>>>>>> &cfg[led_number].led_current);
>>>>>>>> of_property_read_u8(np, "max-cur",
>>>>>>>> &cfg[led_number].max_current);
>>>>>>>>
>>>>>>>> ret = of_property_read_u32(np, "reg", chan_nr);
>>>>>>>> if (ret)
>>>>>>>> return ret;
>>>>>>>>
>>>>>>>> if (chan_nr < 0 || chan_nr > cfg->max_chan_nr) /* side
>>>>>>>> note:
>>>>>>>> new
>>>>>>>> max_chan_nr property needed in cfg */
>>>>>>>> return -EINVAL;
>>>>>>>>
>>>>>>>> return 0;
>>>>>>>> }
>>>>>>>>
>>>>>>>> static int lp55xx_parse_mutli_led_child(struct device_node *np,
>>>>>>>> struct lp55xx_led_config
>>>>>>>> *cfg,
>>>>>>>> int child_number,
>>>>>>>> int color_number)
>>>>>>>> {
>>>>>>>> int chan_nr, color_id;
>>>>>>>>
>>>>>>>> ret = lp55xx_parse_common_child(child, cfg, child_number,
>>>>>>>> color_number,
>>>>>>>> &chan_nr);
>>>>>>>> if (ret)
>>>>>>>> return ret;
>>>>>>>>
>>>>>>>> ret = of_property_read_u32(child, "color", &color_id);
>>>>>>>> if (ret)
>>>>>>>> return ret;
>>>>>>>>
>>>>>>>>
>>>>>>>> cfg[child_number].color_components[color_number].color_id =
>>>>>>>> color_id;
>>>>>>>>
>>>>>>>> cfg[child_number].color_components[color_number].output_num =
>>>>>>>> chan_nr;
>>>>>>>> set_bit(color_id, &cfg[child_number].available_colors);
>>>>>>>>
>>>>>>>> return 0;
>>>>>>>> }
>>>>>>>>
>>>>>>>> staitc int lp55xx_parse_mutli_led(struct device_node *np,
>>>>>>>> struct lp55xx_led_config *cfg,
>>>>>>>> int child_number)
>>>>>>>> {
>>>>>>>> struct device_node *child;
>>>>>>>> int num_colors = 0, i = 0;
>>>>>>> s/, i = 0//
>>>>>>>
>>>>>>>> for_each_child_of_node(np, child) {
>>>>>>>> ret = lp55xx_parse_mutli_led_child(child, cfg,
>>>>>>>> num_colors,
>>>>>>>>
>>>>>>>> child_number, i))
>>>>>>> Replace above call with below:
>>>>>>>
>>>>>>> ret = lp55xx_parse_mutli_led_child(child, cfg, child_number,
>>>>>>> num_colors);
>>>>>>>
>>>>>> I applied your DT parser patch from the v13 series. Which eliminates
>>>>>> this comment correct?
>>>>> Yes, it contains this fix.
>>>>>
>>>> OK I added your patch and it broke a lot of the DT parsing for the
>>>> LP55xx.
>>>>
>>>> I would prefer to stick with the original code without having to
>>>> re-write this again.
>>> Let me test that with Qemu first.
>>>
>> max_channel is never set so not sure where that is supposed to come from
>> since each child device has different number of channels. And if the
>> user has to populate that information from the DT then it does not make
>> sense as the user would already be aware of the number of channels.
>> This information would have to come from the child device some how and
>> the children do not have access to the structure to set it.
> This was my silent assumption that the child will initialize that.
> And I was not thoroughly seeking the most proper place for this
> property, just chose first I could think . You are free to
> change its location so that it was accessible for the child.
>
OK I got it limping along now by passing in the chip->cfg to the
populate data along with rearranging the child probe.
I think it is a lot of change to check 1 value but I can include it
>> In checking the chan_nr to the max_channels you are comparing a pointer
>> to an integer. Easy fix but did not solve the registration issues.
>>
>> cfg->num_colors is not set anywhere so the registration always goes to
>> base led_registration. Unless we key off a different value to determine
>> to register to multicolor class or not. Or we default this to the
>> multi_class registration to figure out how to register based on
>> available_colors.
> You need to add below at the end of lp55xx_parse_mutli_led():
>
> cfg[child_number].num_colors = num_colors;
>
> and below in led_parse_logical_led() at the end of the
> "if (ret)" branch.
>
> cfg[child_number].num_colors = 1;
This was added
>> That is what I am seeing so far in my debugging and I still don't have
>> it working.
> I didn't pretend it was flawless, just wanted to show general idea
> how I would see that.
>
Yes I know you only compile tested this.
Dan