RFC series improving common LED binding parsing support
Qucik grep for 'for_each' or 'linux,default-trigger' says it all.
Multiple LED controller drivers implement the very similar looping
through the child DT nodes in order to locate the LED nodes and read
and support the common LED dt bindings. Implementing this same
stuff for all LED controllers gets old pretty fast.
This RFC contains 3 suggestions:
Simplest is adding support for parsing the linux,default-trigger,
and default-state DT properties in led-core.
More interesting part is adding correct LED DT node lookup in
LED core. This RFC uses LED DT node names as a 'key' in a same
way regulator framework does for regulators. The thing is that
this approach requires the LED controller binding to dictate allowed
LED node names - which may or may not be doable. I need your help to
evaluate this and suggest better options :) If we still look at the
regulators, the regulator core did originally use "regulator-core"
property to do driver data/DT node pairing - but has since then
changed the approach to using the DT node names.
Last and least clear point is isolating the led_classdev to be owned
by the LED core. Controller drivers should pretty much never touch
it after the initialization. So one approach would be that drivers
only provided initialization data and operations to the core.
The patch series contains the led-core and led-class changes which
introduce (yet another) APIs for registering led class device to
core. Adding new interface is probably not the best option - one
might consider changing the (devm_)led_classdev_register_ext to do
what this new RFC API is doing.
In addition to core changes this series converted two (randomly
selected) existing drivers to use the new API. This can give an
overview how offloading the DT parsing to core could simplify many
of the LED controlled drivers.
Patches HAVE NOT BEEN TESTED other than for compiling. They are
only intended to be a starting point for discussion - and if the
ideas are seen worthy - then the patches should be further worked
and properly tested before being applied.
Matti Vaittinen (5):
leds: Add common LED binding parsing support to LED class/core
dt-bindings: an30259a: example for using fixed LED node names.
leds: an30259a: Offload DT node locating and parsing to core
dt-bindings: lm3692x: example for using fixed LED node names.
leds: lm3692x: Offload DT node locating and parsing to core
.../bindings/leds/leds-an30259a.txt | 9 +-
.../devicetree/bindings/leds/leds-lm3692x.txt | 4 +-
drivers/leds/led-class.c | 247 +++++++++++++++++-
drivers/leds/led-core.c | 111 +++++---
drivers/leds/leds-an30259a.c | 181 ++++++-------
drivers/leds/leds-lm3692x.c | 75 +++---
include/linux/leds.h | 144 +++++++++-
7 files changed, 586 insertions(+), 185 deletions(-)
--
2.21.0
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
Use predefined LED node name to match the LED node in driver.
It would be nice to offload common LED property parsing to
LED core driver. One of the key things to allow this is somehow
'pair' the LED DT node with LED driver initialization data.
This patch uses LED node name as a 'key' in a same fashion
as regulators do. The an30259a was selected as demonstration
example and this change may not be really feasible for an30259a
as I have no idea whether the existing DTs for devices out there
have specific node names (or can be changed). This servers just
as an example to initiate discussion as to how we could pair the
driver data and DT node.
Signed-off-by: Matti Vaittinen <[email protected]>
---
Documentation/devicetree/bindings/leds/leds-an30259a.txt | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/leds/leds-an30259a.txt b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
index cbd833906b2b..bd1a2d11a0ad 100644
--- a/Documentation/devicetree/bindings/leds/leds-an30259a.txt
+++ b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
@@ -9,7 +9,8 @@ Required properties:
- #address-cells: Must be 1.
- #size-cells: Must be 0.
-Each LED is represented as a sub-node of the panasonic,an30259a node.
+Each LED is represented as a sub-node of the panasonic,an30259a node. LED nodes
+must be named as led1 led2 and led3.
Required sub-node properties:
- reg: Pin that the LED is connected to. Must be 1, 2, or 3.
@@ -34,20 +35,20 @@ led-controller@30 {
#address-cells = <1>;
#size-cells = <0>;
- led@1 {
+ led1 {
reg = <1>;
linux,default-trigger = "heartbeat";
function = LED_FUNCTION_INDICATOR;
color = <LED_COLOR_ID_RED>;
};
- led@2 {
+ led2 {
reg = <2>;
function = LED_FUNCTION_INDICATOR;
color = <LED_COLOR_ID_GREEN>;
};
- led@3 {
+ led3 {
reg = <3>;
function = LED_FUNCTION_INDICATOR;
color = <LED_COLOR_ID_BLUE>;
--
2.21.0
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
This comment serves as an example how led controller drivers
could be simplified if led-class/led-core would handle DT node
locating and parsing. leds-an30259a was randomly selected from
drivers using 'devm_led_classdev_register_ext' API. No further
study was done :)
This commit HAS NOT BEEN TESTED at all. Only compile tested.
This is only RFC - Eg, request for comments. If people see some
of the ideas as useful then properly tested patch should be
provided.
Signed-off-by: Matti Vaittinen <[email protected]>
---
drivers/leds/leds-an30259a.c | 181 +++++++++++++++++------------------
1 file changed, 87 insertions(+), 94 deletions(-)
diff --git a/drivers/leds/leds-an30259a.c b/drivers/leds/leds-an30259a.c
index 250dc9d6f635..3df91866d6a2 100644
--- a/drivers/leds/leds-an30259a.c
+++ b/drivers/leds/leds-an30259a.c
@@ -61,10 +61,33 @@
struct an30259a;
+/*
+ * RFC_NOTE: What would be the correct way to match the DT node with driver?
+ * This may not work for leds-an30259a.c as the DT entries may not be exactly
+ * what is used in dt-binding example. For _new_ drivers we could either fix the
+ * node names (?) can we or introduce some "led-compatible" property (like
+ * regulators had regulator-compatible prior switching to node-names). But if we
+ * wish to convert old drivers to use this new API (I suggested merging the
+ * led_classdev_register_ext and led_classdev_register_dt into one in order to
+ * not create yet another LED class registration API) we may need to add some
+ * of_match_cb call-back in led_init_data so that old drivers can implement own
+ * DT matching mechanism and LED core just calls that for each child node. I'd
+ * prefer not having such as ideally drivers should not need to care about DT
+ * nodes unless they have driver specific properties. Core should handle generic
+ * LED properties.
+ */
+static const char *of_led_names[AN30259A_MAX_LEDS] = {
+ "led1", "led2", "led3",
+};
+
struct an30259a_led {
struct an30259a *chip;
struct fwnode_handle *fwnode;
- struct led_classdev cdev;
+/*
+ * RFC_NOTE: We give ownership of led_classdev to LED core. LED driver should
+ * not really need it for anything?
+ */
+ struct led_init_data init_data;
u32 num;
u32 default_state;
bool sloping;
@@ -85,7 +108,7 @@ static int an30259a_brightness_set(struct led_classdev *cdev,
int ret;
unsigned int led_on;
- led = container_of(cdev, struct an30259a_led, cdev);
+ led = container_of(cdev->init_data, struct an30259a_led, init_data);
mutex_lock(&led->chip->mutex);
ret = regmap_read(led->chip->regmap, AN30259A_REG_LED_ON, &led_on);
@@ -132,7 +155,7 @@ static int an30259a_blink_set(struct led_classdev *cdev,
unsigned int led_on;
unsigned long off = *delay_off, on = *delay_on;
- led = container_of(cdev, struct an30259a_led, cdev);
+ led = container_of(cdev->init_data, struct an30259a_led, init_data);
mutex_lock(&led->chip->mutex);
num = led->num;
@@ -199,56 +222,76 @@ static int an30259a_blink_set(struct led_classdev *cdev,
return ret;
}
-static int an30259a_dt_init(struct i2c_client *client,
- struct an30259a *chip)
+static int of_check_reg(struct led_classdev *ld, struct fwnode_handle *fw,
+ struct led_properties *props)
{
- struct device_node *np = client->dev.of_node, *child;
- int count, ret;
- int i = 0;
+ u32 source;
+ int ret;
+ struct an30259a *chip;
const char *str;
- struct an30259a_led *led;
+ unsigned int led_on;
+ struct an30259a_led *led = container_of(ld->init_data,
+ struct an30259a_led, init_data);
- count = of_get_child_count(np);
- if (!count || count > AN30259A_MAX_LEDS)
+ chip = led->chip;
+
+ ret = fwnode_property_read_u32(fw, "reg", &source);
+ if (ret != 0 || !source || source > AN30259A_MAX_LEDS) {
+ dev_err(&chip->client->dev, "Couldn't read LED address: %d\n",
+ ret);
return -EINVAL;
+ }
+ led->num = source;
+ chip->num_leds++;
+
+ /*
+ * RFC_NOTE: We need to add default-state = "keep" handling here
+ * if we don't implement get_brightness and keep support in core
+ */
+ if (!fwnode_property_read_string(fw, "default-state", &str)) {
+ if (!strcmp(str, "keep"))
+ ret = regmap_read(chip->regmap, AN30259A_REG_LED_ON,
+ &led_on);
+ if (ret)
+ return ret;
- for_each_available_child_of_node(np, child) {
- u32 source;
+ if (!(led_on & AN30259A_LED_EN(led->num)))
+ ld->brightness = LED_OFF;
+ else
+ regmap_read(chip->regmap, AN30259A_REG_LEDCC(led->num),
+ &ld->brightness);
+ }
- ret = of_property_read_u32(child, "reg", &source);
- if (ret != 0 || !source || source > AN30259A_MAX_LEDS) {
- dev_err(&client->dev, "Couldn't read LED address: %d\n",
- ret);
- count--;
- continue;
- }
+ return 0;
+}
- led = &chip->leds[i];
+const static struct led_ops an30259a_ops = {
+ .brightness_set_blocking = an30259a_brightness_set,
+ .blink_set = an30259a_blink_set,
+};
- led->num = source;
- led->chip = chip;
- led->fwnode = of_fwnode_handle(child);
+static int an30259a_dt_init(struct i2c_client *client,
+ struct an30259a *chip)
+{
+ void *ret;
+ int i = 0;
+ struct an30259a_led *led;
- if (!of_property_read_string(child, "default-state", &str)) {
- if (!strcmp(str, "on"))
- led->default_state = STATE_ON;
- else if (!strcmp(str, "keep"))
- led->default_state = STATE_KEEP;
- else
- led->default_state = STATE_OFF;
- }
+ for (i = 0; i < AN30259A_MAX_LEDS; i++) {
+ led = &chip->leds[i];
- of_property_read_string(child, "linux,default-trigger",
- &led->cdev.default_trigger);
+ led->init_data.of_match = of_match_ptr(of_led_names[i]);
+ led->init_data.devicename = AN30259A_NAME;
+ led->init_data.default_label = ":";
+ led->init_data.of_parse_cb = of_check_reg;
+ led->chip = chip;
- i++;
+ ret = devm_led_classdev_register_dt(&client->dev, &an30259a_ops,
+ &led->init_data);
+ if (IS_ERR(ret))
+ return PTR_ERR(ret);
}
- if (!count)
- return -EINVAL;
-
- chip->num_leds = i;
-
return 0;
}
@@ -258,75 +301,25 @@ static const struct regmap_config an30259a_regmap_config = {
.max_register = AN30259A_REG_MAX,
};
-static void an30259a_init_default_state(struct an30259a_led *led)
-{
- struct an30259a *chip = led->chip;
- int led_on, err;
-
- switch (led->default_state) {
- case STATE_ON:
- led->cdev.brightness = LED_FULL;
- break;
- case STATE_KEEP:
- err = regmap_read(chip->regmap, AN30259A_REG_LED_ON, &led_on);
- if (err)
- break;
-
- if (!(led_on & AN30259A_LED_EN(led->num))) {
- led->cdev.brightness = LED_OFF;
- break;
- }
- regmap_read(chip->regmap, AN30259A_REG_LEDCC(led->num),
- &led->cdev.brightness);
- break;
- default:
- led->cdev.brightness = LED_OFF;
- }
-
- an30259a_brightness_set(&led->cdev, led->cdev.brightness);
-}
-
static int an30259a_probe(struct i2c_client *client)
{
struct an30259a *chip;
- int i, err;
+ int err;
chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
if (!chip)
return -ENOMEM;
- err = an30259a_dt_init(client, chip);
- if (err < 0)
- return err;
-
mutex_init(&chip->mutex);
chip->client = client;
i2c_set_clientdata(client, chip);
chip->regmap = devm_regmap_init_i2c(client, &an30259a_regmap_config);
- for (i = 0; i < chip->num_leds; i++) {
- struct led_init_data init_data = {};
-
- an30259a_init_default_state(&chip->leds[i]);
- chip->leds[i].cdev.brightness_set_blocking =
- an30259a_brightness_set;
- chip->leds[i].cdev.blink_set = an30259a_blink_set;
-
- init_data.fwnode = chip->leds[i].fwnode;
- init_data.devicename = AN30259A_NAME;
- init_data.default_label = ":";
-
- err = devm_led_classdev_register_ext(&client->dev,
- &chip->leds[i].cdev,
- &init_data);
- if (err < 0)
- goto exit;
- }
- return 0;
+ err = an30259a_dt_init(client, chip);
+ if (err)
+ mutex_destroy(&chip->mutex);
-exit:
- mutex_destroy(&chip->mutex);
return err;
}
--
2.21.0
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
Use predefined LED node name to match the LED node in driver.
It would be nice to offload common LED property parsing to
LED core driver. One of the key things to allow this is somehow
'pair' the LED DT node with LED driver initialization data.
This patch uses LED node name as a 'key' in a same fashion
as regulators do. The lm3692x was selected as demonstration
example and this change is not intended to be feasible as such
(surprize =]) This servers just as an example to initiate
discussion as to how (if) we could pair the driver data and DT
node.
Signed-off-by: Matti Vaittinen <[email protected]>
---
Documentation/devicetree/bindings/leds/leds-lm3692x.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
index 4c2d923f8758..03866d491d01 100644
--- a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
+++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
@@ -5,7 +5,7 @@ white-LED driver designed for LCD display backlighting.
The main difference between the LM36922 and LM36923 is the number of
LED strings it supports. The LM36922 supports two strings while the LM36923
-supports three strings.
+supports three strings. LED sub-node must be named as "led_node_name_here".
Required properties:
- compatible:
@@ -45,7 +45,7 @@ led-controller@36 {
enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
vled-supply = <&vbatt>;
- led@0 {
+ led_node_name_here {
reg = <0>;
function = LED_FUNCTION_BACKLIGHT;
color = <LED_COLOR_ID_WHITE>;
--
2.21.0
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
On Tue, Oct 29, 2019 at 02:47:26PM +0200, Matti Vaittinen wrote:
> Use predefined LED node name to match the LED node in driver.
>
> It would be nice to offload common LED property parsing to
> LED core driver. One of the key things to allow this is somehow
> 'pair' the LED DT node with LED driver initialization data.
>
> This patch uses LED node name as a 'key' in a same fashion
> as regulators do. The an30259a was selected as demonstration
> example and this change may not be really feasible for an30259a
> as I have no idea whether the existing DTs for devices out there
> have specific node names (or can be changed). This servers just
> as an example to initiate discussion as to how we could pair the
> driver data and DT node.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
> ---
> Documentation/devicetree/bindings/leds/leds-an30259a.txt | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-an30259a.txt b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
> index cbd833906b2b..bd1a2d11a0ad 100644
> --- a/Documentation/devicetree/bindings/leds/leds-an30259a.txt
> +++ b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
> @@ -9,7 +9,8 @@ Required properties:
> - #address-cells: Must be 1.
> - #size-cells: Must be 0.
>
> -Each LED is represented as a sub-node of the panasonic,an30259a node.
> +Each LED is represented as a sub-node of the panasonic,an30259a node. LED nodes
> +must be named as led1 led2 and led3.
>
> Required sub-node properties:
> - reg: Pin that the LED is connected to. Must be 1, 2, or 3.
> @@ -34,20 +35,20 @@ led-controller@30 {
> #address-cells = <1>;
> #size-cells = <0>;
>
> - led@1 {
> + led1 {
> reg = <1>;
This is wrong. reg requires a unit-address and vice-versa.
> linux,default-trigger = "heartbeat";
> function = LED_FUNCTION_INDICATOR;
> color = <LED_COLOR_ID_RED>;
> };
>
> - led@2 {
> + led2 {
> reg = <2>;
> function = LED_FUNCTION_INDICATOR;
> color = <LED_COLOR_ID_GREEN>;
> };
>
> - led@3 {
> + led3 {
> reg = <3>;
> function = LED_FUNCTION_INDICATOR;
> color = <LED_COLOR_ID_BLUE>;
> --
> 2.21.0
>
>
> --
> Matti Vaittinen, Linux device drivers
> ROHM Semiconductors, Finland SWDC
> Kiviharjunlenkki 1E
> 90220 OULU
> FINLAND
>
> ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
> Simon says - in Latin please.
> ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
> Thanks to Simon Glass for the translation =]
Hello Rob & All,
On Tue, 2019-11-05 at 21:42 -0600, Rob Herring wrote:
> On Tue, Oct 29, 2019 at 02:47:26PM +0200, Matti Vaittinen wrote:
> > Use predefined LED node name to match the LED node in driver.
> >
> > It would be nice to offload common LED property parsing to
> > LED core driver. One of the key things to allow this is somehow
> > 'pair' the LED DT node with LED driver initialization data.
> >
> > This patch uses LED node name as a 'key' in a same fashion
> > as regulators do. The an30259a was selected as demonstration
> > example and this change may not be really feasible for an30259a
> > as I have no idea whether the existing DTs for devices out there
> > have specific node names (or can be changed). This servers just
> > as an example to initiate discussion as to how we could pair the
> > driver data and DT node.
> >
> > Signed-off-by: Matti Vaittinen <[email protected]>
> > ---
> > Documentation/devicetree/bindings/leds/leds-an30259a.txt | 9
> > +++++----
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/leds/leds-
> > an30259a.txt b/Documentation/devicetree/bindings/leds/leds-
> > an30259a.txt
> > index cbd833906b2b..bd1a2d11a0ad 100644
> > --- a/Documentation/devicetree/bindings/leds/leds-an30259a.txt
> > +++ b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
> > @@ -9,7 +9,8 @@ Required properties:
> > - #address-cells: Must be 1.
> > - #size-cells: Must be 0.
> >
> > -Each LED is represented as a sub-node of the panasonic,an30259a
> > node.
> > +Each LED is represented as a sub-node of the panasonic,an30259a
> > node. LED nodes
> > +must be named as led1 led2 and led3.
> >
> > Required sub-node properties:
> > - reg: Pin that the LED is connected to. Must be 1, 2, or 3.
> > @@ -34,20 +35,20 @@ led-controller@30 {
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > - led@1 {
> > + led1 {
> > reg = <1>;
>
> This is wrong. reg requires a unit-address and vice-versa.
Right.
I'd be interested to know how using node name(s) to match the driver
data (like regulators do) is seen in general? Is this a bad idea for
LEDs? Would it be better to have a leds-compatible (like we had
regulator-compatible before)?
If node names can be used - would
led1@1 {
};
led2@2 {
};
... do?
Using node names as key requires them to be unique. Other option could
be doing the search based on node name and unit-address combination -
but if unit-address depends on board the led is placed - then driver
might not know it. Should I convert the RFC to introduce and use led-
compatible?
> > linux,default-trigger = "heartbeat";
> > function = LED_FUNCTION_INDICATOR;
> > color = <LED_COLOR_ID_RED>;
> > };
> >
> > - led@2 {
> > + led2 {
> > reg = <2>;
> > function = LED_FUNCTION_INDICATOR;
> > color = <LED_COLOR_ID_GREEN>;
> > };
> >
> > - led@3 {
> > + led3 {
> > reg = <3>;
> > function = LED_FUNCTION_INDICATOR;
> > color = <LED_COLOR_ID_BLUE>;
> > --
> > 2.21.0
> >
> >
> > --
> > Matti Vaittinen, Linux device drivers
> > ROHM Semiconductors, Finland SWDC
> > Kiviharjunlenkki 1E
> > 90220 OULU
> > FINLAND
> >
> > ~~~ "I don't think so," said Rene Descartes. Just then he vanished
> > ~~~
> > Simon says - in Latin please.
> > ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
> > Thanks to Simon Glass for the translation =]
Hi,
> The thing is that
> this approach requires the LED controller binding to dictate allowed
> LED node names - which may or may not be doable. I need your help to
> evaluate this and suggest better options :)
even though I like the idea of convention-over-code, but if that's
changing allowed LED names that would risk breaking things, eg:
a) existing DT's (in the field) become incompatible with newer
kernel versions
b) existing userlands that rely on speicific LED names become
incomatible with newer kernel versions.
--mtx
--
Dringender Hinweis: aufgrund existenzieller Bedrohung durch "Emotet"
sollten Sie *niemals* MS-Office-Dokumente via E-Mail annehmen/öffenen,
selbst wenn diese von vermeintlich vertrauenswürdigen Absendern zu
stammen scheinen. Andernfalls droht Totalschaden.
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
[email protected] -- +49-151-27565287
Hello Enrico & All,
First of all - thanks for showing the interest :] I am happy knowing
someone actually was interested about the RFC :)
On Mon, 2019-11-18 at 10:21 +0100, Enrico Weigelt, metux IT consult
wrote:
> Hi,
>
>
> > The thing is that
> > this approach requires the LED controller binding to dictate
> > allowed
> > LED node names - which may or may not be doable. I need your help
> > to
> > evaluate this and suggest better options :)
>
> even though I like the idea of convention-over-code, but if that's
> changing allowed LED names that would risk breaking things, eg:
>
> a) existing DT's (in the field) become incompatible with newer
> kernel versions
This was my main concern. This of course would not mean that we could
not take this approach for new LED controller drivers - but that would
(probably) lead to dual led registration interface - one for current
approach where LED drivers do all the dirty work themself - and parse
the DT - one for new drivers which could leave DT parsing to LED core.
> b) existing userlands that rely on speicific LED names become
> incomatible with newer kernel versions.
I didn't even think this far, but yes, I see... LED node name might be
reflected in user-space LED name. I won't start arguing if this is sane
or not - this is what we seem to be living with today :)
Anyways, I did send out a LED core change patch which allows one to add
either the node-name, or a property-value pair in init_data. LED core
can then use either of these to do LED node lookup. If none of these is
given, then we can fall-back to existing logic. That way we won't
change existing stuff.
Here:
https://lore.kernel.org/lkml/258b5c9934e2b31a5f433a7dbb908dfe5da3d30c.1574059625.git.matti.vaittinen@fi.rohmeurope.com/T/#u
I didn't invest too much of time on this yet - but at first glimpse it
seemed that at least some of the drivers did use reg - property with
fixed value to do the matching. Those could set the property name to
'reg' and value to 'X' and leave the DT node lookup and common property
parsing to the LED core. If my patch won't get too big objection (and
if no fatal flaws are found from the idea) - then I might try and find
the time to do some follow-up patches simplifying existing LED
drivers...
>
>
>
> --mtx
>
On 18.11.19 11:38, Vaittinen, Matti wrote:
Hi,
>> a) existing DT's (in the field) become incompatible with newer
>> kernel versions
>
> This was my main concern. This of course would not mean that we could
> not take this approach for new LED controller drivers - but that would
> (probably) lead to dual led registration interface
Maybe just a flag for that ? Perhaps the driver could also specify a
list of node names for the LEDs, so led-core can do the lookup for them.
>> b) existing userlands that rely on speicific LED names become
>> incomatible with newer kernel versions.
>
> I didn't even think this far, but yes, I see... LED node name might be
> reflected in user-space LED name. I won't start arguing if this is sane
> or not - this is what we seem to be living with today :)
Especially in embedded world, this can really make sense: applications
just use a defined LED name, no matter which board it's running on.
Convention over configuration.
Personally, I also like to use LED subsystem as frontend for things like
gpio-driven relais, etc, and assign semantically fitting names instead
of "technical" ones,
> I didn't invest too much of time on this yet - but at first glimpse it
> seemed that at least some of the drivers did use reg - property with
> fixed value to do the matching. Those could set the property name to
> 'reg' and value to 'X' and leave the DT node lookup and common property
> parsing to the LED core. If my patch won't get too big objection (and
> if no fatal flaws are found from the idea) - then I might try and find
> the time to do some follow-up patches simplifying existing LED
> drivers...
Sounds good :)
--mtx
--
Dringender Hinweis: aufgrund existenzieller Bedrohung durch "Emotet"
sollten Sie *niemals* MS-Office-Dokumente via E-Mail annehmen/öffenen,
selbst wenn diese von vermeintlich vertrauenswürdigen Absendern zu
stammen scheinen. Andernfalls droht Totalschaden.
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
[email protected] -- +49-151-27565287
On Tue, 2019-11-19 at 20:52 +0100, Enrico Weigelt, metux IT consult
wrote:
> On 18.11.19 11:38, Vaittinen, Matti wrote:
>
> Hi,
>
> > > a) existing DT's (in the field) become incompatible with newer
> > > kernel versions
> >
> > This was my main concern. This of course would not mean that we
> > could
> > not take this approach for new LED controller drivers - but that
> > would
> > (probably) lead to dual led registration interface
>
> Maybe just a flag for that ? Perhaps the driver could also specify a
> list of node names for the LEDs, so led-core can do the lookup for
> them.
This is actually close to what I suggested in my other email to Jacek.
> > > b) existing userlands that rely on speicific LED names become
> > > incomatible with newer kernel versions.
> >
> > I didn't even think this far, but yes, I see... LED node name might
> > be
> > reflected in user-space LED name. I won't start arguing if this is
> > sane
> > or not - this is what we seem to be living with today :)
>
> Especially in embedded world, this can really make sense:
> applications
> just use a defined LED name, no matter which board it's running on.
> Convention over configuration.
Definitely. I am all for generating the name based on LED _function_ -
no matter what the board is. I like the LED name generation base on
'function' DT property. But node names tend to be somewhat generic - or
board specific (to avoid collisions). So using node name directly is
not (as far as my understanding goes - which is limited on this topic)
optimal for guaranteeing coherent view (across the boards) for user-
space. Wow, what a nice sentence for non native English speaker like me
xD
> Personally, I also like to use LED subsystem as frontend for things
> like
> gpio-driven relais, etc, and assign semantically fitting names
> instead
> of "technical" ones,
This is outside of my experience so I just believe what you say :)
>
> > I didn't invest too much of time on this yet - but at first glimpse
> > it
> > seemed that at least some of the drivers did use reg - property
> > with
> > fixed value to do the matching. Those could set the property name
> > to
> > 'reg' and value to 'X' and leave the DT node lookup and common
> > property
> > parsing to the LED core. If my patch won't get too big objection
> > (and
> > if no fatal flaws are found from the idea) - then I might try and
> > find
> > the time to do some follow-up patches simplifying existing LED
> > drivers...
>
> Sounds good :)
>
>
> --mtx
>
Hi!
> Personally, I also like to use LED subsystem as frontend for things like
> gpio-driven relais, etc, and assign semantically fitting names instead
> of "technical" ones,
Don't do that. Maybe we need "named GPIOs", but lets not abuse LED
subsystem for that. (Even if I may have made that mistake before).
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html