| Message ID | 20220917081339.3354075-3-jjhiblot@traphandler.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add a multicolor LED driver for groups of monochromatic LEDs | expand |
On Sat, Sep 17, 2022 at 11:14 AM Jean-Jacques Hiblot <jjhiblot@traphandler.com> wrote: > > This information might be useful for more than only deriving the led's ... > + if (fwnode_property_present(init_data->fwnode, "color")) > + fwnode_property_read_u32(init_data->fwnode, "color", > + &led_cdev->color); Is it already described in the schema? ... > unsigned int brightness; > unsigned int max_brightness; > + unsigned int color; The above two are exposed via sysfs, do you need to expose a new one as well? (Just a question, I am not taking any side here, want to hear explanation of the either choice)
Hi Jean, On 9/17/22 10:13, Jean-Jacques Hiblot wrote: > This information might be useful for more than only deriving the led's > name. > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> > --- > drivers/leds/led-class.c | 7 +++++++ > include/linux/leds.h | 1 + > 2 files changed, 8 insertions(+) > > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c > index 2c0d979d0c8a..537379f09801 100644 > --- a/drivers/leds/led-class.c > +++ b/drivers/leds/led-class.c > @@ -350,6 +350,10 @@ int led_classdev_register_ext(struct device *parent, > if (fwnode_property_present(init_data->fwnode, > "retain-state-shutdown")) > led_cdev->flags |= LED_RETAIN_AT_SHUTDOWN; > + > + if (fwnode_property_present(init_data->fwnode, "color")) > + fwnode_property_read_u32(init_data->fwnode, "color", > + &led_cdev->color); > } > } else { > proposed_name = led_cdev->name; > @@ -359,6 +363,9 @@ int led_classdev_register_ext(struct device *parent, > if (ret < 0) > return ret; > > + if (led_cdev->color >= LED_COLOR_ID_MAX) > + dev_warn(parent, "LED %s color identifier out of range\n", final_name); > + > mutex_init(&led_cdev->led_access); > mutex_lock(&led_cdev->led_access); > led_cdev->dev = device_create_with_groups(leds_class, parent, 0, > diff --git a/include/linux/leds.h b/include/linux/leds.h > index ba4861ec73d3..fe6346604e36 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -71,6 +71,7 @@ struct led_classdev { > const char *name; > unsigned int brightness; > unsigned int max_brightness; > + unsigned int color; We have color_index in struct mc_subled. What would this serve for? > int flags; > > /* Lower 16 bits reflect status */
On 9/18/22 13:25, Jacek Anaszewski wrote: > Hi Jean, > > On 9/17/22 10:13, Jean-Jacques Hiblot wrote: >> This information might be useful for more than only deriving the led's >> name. >> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> >> --- >> drivers/leds/led-class.c | 7 +++++++ >> include/linux/leds.h | 1 + >> 2 files changed, 8 insertions(+) >> >> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c >> index 2c0d979d0c8a..537379f09801 100644 >> --- a/drivers/leds/led-class.c >> +++ b/drivers/leds/led-class.c >> @@ -350,6 +350,10 @@ int led_classdev_register_ext(struct device *parent, >> if (fwnode_property_present(init_data->fwnode, >> "retain-state-shutdown")) >> led_cdev->flags |= LED_RETAIN_AT_SHUTDOWN; >> + >> + if (fwnode_property_present(init_data->fwnode, "color")) >> + fwnode_property_read_u32(init_data->fwnode, "color", >> + &led_cdev->color); >> } >> } else { >> proposed_name = led_cdev->name; >> @@ -359,6 +363,9 @@ int led_classdev_register_ext(struct device *parent, >> if (ret < 0) >> return ret; >> + if (led_cdev->color >= LED_COLOR_ID_MAX) >> + dev_warn(parent, "LED %s color identifier out of range\n", >> final_name); >> + >> mutex_init(&led_cdev->led_access); >> mutex_lock(&led_cdev->led_access); >> led_cdev->dev = device_create_with_groups(leds_class, parent, 0, >> diff --git a/include/linux/leds.h b/include/linux/leds.h >> index ba4861ec73d3..fe6346604e36 100644 >> --- a/include/linux/leds.h >> +++ b/include/linux/leds.h >> @@ -71,6 +71,7 @@ struct led_classdev { >> const char *name; >> unsigned int brightness; >> unsigned int max_brightness; >> + unsigned int color; > > We have color_index in struct mc_subled. What would this serve for? OK, I missed that you're using it leds-group-multicolor.c.
On 17/09/2022 10:40, Andy Shevchenko wrote: > On Sat, Sep 17, 2022 at 11:14 AM Jean-Jacques Hiblot > <jjhiblot@traphandler.com> wrote: >> This information might be useful for more than only deriving the led's > ... > >> + if (fwnode_property_present(init_data->fwnode, "color")) >> + fwnode_property_read_u32(init_data->fwnode, "color", >> + &led_cdev->color); > Is it already described in the schema? Hello Andy, thanks for the reviews on the patch, and sorry for the delay in my responses. Yes. This is already part of the schema. > ... > >> unsigned int brightness; >> unsigned int max_brightness; >> + unsigned int color; > The above two are exposed via sysfs, do you need to expose a new one > as well? (Just a question, I am not taking any side here, want to hear > explanation of the either choice) I didn't really think about it because I didn't need it. It probably doesn't hurt to expose t in the sysfs. I'll add this in the next round. >
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 2c0d979d0c8a..537379f09801 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -350,6 +350,10 @@ int led_classdev_register_ext(struct device *parent, if (fwnode_property_present(init_data->fwnode, "retain-state-shutdown")) led_cdev->flags |= LED_RETAIN_AT_SHUTDOWN; + + if (fwnode_property_present(init_data->fwnode, "color")) + fwnode_property_read_u32(init_data->fwnode, "color", + &led_cdev->color); } } else { proposed_name = led_cdev->name; @@ -359,6 +363,9 @@ int led_classdev_register_ext(struct device *parent, if (ret < 0) return ret; + if (led_cdev->color >= LED_COLOR_ID_MAX) + dev_warn(parent, "LED %s color identifier out of range\n", final_name); + mutex_init(&led_cdev->led_access); mutex_lock(&led_cdev->led_access); led_cdev->dev = device_create_with_groups(leds_class, parent, 0, diff --git a/include/linux/leds.h b/include/linux/leds.h index ba4861ec73d3..fe6346604e36 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -71,6 +71,7 @@ struct led_classdev { const char *name; unsigned int brightness; unsigned int max_brightness; + unsigned int color; int flags; /* Lower 16 bits reflect status */
This information might be useful for more than only deriving the led's name. Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> --- drivers/leds/led-class.c | 7 +++++++ include/linux/leds.h | 1 + 2 files changed, 8 insertions(+)