[PATCH 0/2] leds: add generic LED level meter driver

[PATCH 0/2] leds: add generic LED level meter driver

[Akinobu Mita]

This introduces a new LED driver that enables us to create a virtual LED
level meter device that consists of multiple LED devices by different
drivers.

Previously I developed the level meter feature for leds-gpio ("leds: gpio:
support multi-level brightness") [1].  Then I got a feedback from
Bjorn Andersson and made more generic new driver.  This driver is also
inspired by led-backlight driver patchset [2] and actually requires
devm_of_led_get() function provided by the patchset.

[1] https://lore.kernel.org/linux-leds/1570203299-4270-1-git-send-email-akinobu.mita@gmail.com/
[2] https://lore.kernel.org/linux-leds/20191009085127.22843-1-jjhiblot@ti.com/

Akinobu Mita (2):
  leds: add LED level meter driver
  leds: meter: add leds-meter binding

 .../devicetree/bindings/leds/leds-meter.yaml       |  42 +++++++
 drivers/leds/Kconfig                               |  10 ++
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds-meter.c                          | 134 +++++++++++++++++++++
 4 files changed, 187 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-meter.yaml
 create mode 100644 drivers/leds/leds-meter.c

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Bjorn Andersson <bjorn@kryo.se>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Jean-Jacques Hiblot <jjhiblot@ti.com>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Dan Murphy <dmurphy@ti.com>
-- 
2.7.4

[PATCH 1/2] leds: add LED level meter driver

[Akinobu Mita]

This introduces a new LED driver that enables us to create a virtual LED
level meter device that consists of multiple LED devices by different
drivers.

The brightness of the LED level meter is proportional to the number of
actual LEDs that are turned on.

For example, the LED level meter with four LEDs can be described by the
following DT node.

    meter-leds {
        compatible = "meter-leds";
        leds = <&led0>, <&led1>, <&led2>, <&led3>;
    };

When the brightness of meter-leds is LED_HALF, the brightness of the led0
and led1 is set to maximum brightness and the brightness of led2 and led3
is set to zero.

We can optionally control the proportionality by overvaluing some of LEDs
with the brightness-weights proporty.

For example, when we assign the weight of led0 to the total weigt of the
rest, only the led0 is turned on if the brightness of meter-leds is within
the range of [1, LED_HALD].

    meter-leds {
        compatible = "meter-leds";
        leds = <&led0>, <&led1>, <&led2>, <&led3>;
        brightness-weights = <3 1 1 1>;
    };

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Bjorn Andersson <bjorn@kryo.se>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Jean-Jacques Hiblot <jjhiblot@ti.com>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Dan Murphy <dmurphy@ti.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/leds/Kconfig      |  10 ++++
 drivers/leds/Makefile     |   1 +
 drivers/leds/leds-meter.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 145 insertions(+)
 create mode 100644 drivers/leds/leds-meter.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1988de1..feecd2e 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -823,6 +823,16 @@ config LEDS_LM36274
 	  Say Y to enable the LM36274 LED driver for TI LMU devices.
 	  This supports the LED device LM36274.
 
+config LEDS_METER
+	tristate "LED support for generic level meter driver"
+	depends on LEDS_CLASS
+	help
+	  This option enables support for generic LED level meter that
+	  consists of multiple LED devices.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called leds-meter.
+
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
 
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 41fb073..0fe2a28 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -85,6 +85,7 @@ obj-$(CONFIG_LEDS_LM3601X)		+= leds-lm3601x.o
 obj-$(CONFIG_LEDS_TI_LMU_COMMON)	+= leds-ti-lmu-common.o
 obj-$(CONFIG_LEDS_LM3697)		+= leds-lm3697.o
 obj-$(CONFIG_LEDS_LM36274)		+= leds-lm36274.o
+obj-$(CONFIG_LEDS_METER)		+= leds-meter.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_CR0014114)		+= leds-cr0014114.o
diff --git a/drivers/leds/leds-meter.c b/drivers/leds/leds-meter.c
new file mode 100644
index 0000000..a47b639
--- /dev/null
+++ b/drivers/leds/leds-meter.c
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+struct meter_led_ref {
+	struct led_classdev *cdev;
+	unsigned int brightness_weight;
+};
+
+struct meter_led_data {
+	struct led_classdev cdev;
+	int num_leds;
+	int total_weight;
+	struct meter_led_ref leds[];
+};
+
+struct leds_fract {
+	unsigned int numerator;
+	unsigned int denominator;
+};
+
+/* Compare two leds_fract structs */
+#define LEDS_FRACT_COMPARE(a, OP, b)			\
+	((u64)(a).numerator * (b).denominator OP	\
+	(u64)(b).numerator * (a).denominator)
+
+static void meter_led_set(struct led_classdev *led_cdev,
+			  enum led_brightness value)
+{
+	struct meter_led_data *meter_led =
+		container_of(led_cdev, struct meter_led_data, cdev);
+	struct leds_fract level = { 0, meter_led->total_weight };
+	struct leds_fract br = { value, LED_FULL };
+	int i;
+
+	for (i = 0; i < meter_led->num_leds; i++) {
+		struct meter_led_ref *led = &meter_led->leds[i];
+
+		if (LEDS_FRACT_COMPARE(br, <=, level))
+			break;
+
+		level.numerator += led->brightness_weight;
+
+		led_set_brightness(led->cdev, led->cdev->max_brightness);
+	}
+
+	for (; i < meter_led->num_leds; i++) {
+		struct meter_led_ref *led = &meter_led->leds[i];
+
+		led_set_brightness(led->cdev, 0);
+	}
+}
+
+static int meter_led_probe(struct platform_device *pdev)
+{
+	int i;
+	int num_leds;
+	int num_weights;
+	struct meter_led_data *meter_led;
+	struct led_init_data init_data = {};
+
+	num_leds = of_count_phandle_with_args(pdev->dev.of_node, "leds", NULL);
+	if (num_leds < 0)
+		return num_leds;
+
+	if (num_leds < 1) {
+		dev_err(&pdev->dev, "At least one LED must be specified!\n");
+		return -EINVAL;
+	}
+
+	num_weights = of_property_count_u32_elems(pdev->dev.of_node,
+						  "brightness-weights");
+	if (num_weights < 0) {
+		num_weights = 0;
+	} else if (num_weights != num_leds) {
+		dev_err(&pdev->dev,
+			"The number of brightness-weights must be equal to the number of LEDs\n");
+		return -EINVAL;
+	}
+
+	meter_led = devm_kzalloc(&pdev->dev,
+				 struct_size(meter_led, leds, num_leds),
+				 GFP_KERNEL);
+	if (!meter_led)
+		return -ENOMEM;
+
+	for (i = 0; i < num_leds; i++) {
+		meter_led->leds[i].cdev = devm_of_led_get(&pdev->dev, i);
+		if (IS_ERR(meter_led->leds[i].cdev))
+			return PTR_ERR(meter_led->leds[i].cdev);
+
+		if (num_weights) {
+			of_property_read_u32_index(pdev->dev.of_node,
+					"brightness-weights", i,
+					&meter_led->leds[i].brightness_weight);
+		} else {
+			meter_led->leds[i].brightness_weight = 1;
+		}
+
+		meter_led->total_weight += meter_led->leds[i].brightness_weight;
+	}
+
+	meter_led->num_leds = num_leds;
+	meter_led->cdev.brightness_set = meter_led_set;
+
+	platform_set_drvdata(pdev, meter_led);
+	init_data.fwnode = of_fwnode_handle(pdev->dev.of_node);
+
+	return devm_led_classdev_register_ext(&pdev->dev, &meter_led->cdev,
+					      &init_data);
+}
+
+static const struct of_device_id of_meter_leds_match[] = {
+	{ .compatible = "meter-leds", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_meter_leds_match);
+
+static struct platform_driver meter_led_driver = {
+	.driver = {
+		.name = "meter-led",
+		.of_match_table = of_meter_leds_match,
+	},
+	.probe = meter_led_probe,
+};
+module_platform_driver(meter_led_driver);
+
+MODULE_AUTHOR("Akinobu Mita");
+MODULE_DESCRIPTION("LED level meter driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:leds-meter");
-- 
2.7.4

[PATCH 2/2] leds: meter: add leds-meter binding

[Akinobu Mita]

Add DT binding for leds-meter.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Bjorn Andersson <bjorn@kryo.se>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Jean-Jacques Hiblot <jjhiblot@ti.com>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Dan Murphy <dmurphy@ti.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 .../devicetree/bindings/leds/leds-meter.yaml       | 42 ++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-meter.yaml

diff --git a/Documentation/devicetree/bindings/leds/leds-meter.yaml b/Documentation/devicetree/bindings/leds/leds-meter.yaml
new file mode 100644
index 0000000..d5dfa261
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-meter.yaml
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-meter.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic LED level meter
+
+maintainers:
+  - Akinobu Mita <akinobu.mita@gmail.com>
+
+description:
+  Generic LED level meter consists of multiple LED devices by different drivers.
+
+properties:
+  compatible:
+    const: meter-leds
+
+  leds:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    minItems: 1
+    description: List of phandles to LED node that are members of a level meter.
+
+  brightness-weights:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    description: Each integer represents a contribution ratio within a level
+      meter.
+
+required:
+ - compatible
+ - leds
+
+examples:
+  - |
+    leds {
+        compatible = "meter-leds";
+        leds = <&led0>, <&led1>, <&led2>, <&led3>;
+        brightness-weights = <3 1 1 1>;
+    };
+
+...
-- 
2.7.4

Re: [PATCH 2/2] leds: meter: add leds-meter binding

[Dan Murphy]

Akinobu

On 10/22/19 10:06 AM, Akinobu Mita wrote:
> Add DT binding for leds-meter.

$subject should be

  dt-bindings: leds: Add leds-meter binding

And this patch should be first in the series

> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Bjorn Andersson <bjorn@kryo.se>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Jean-Jacques Hiblot <jjhiblot@ti.com>
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Dan Murphy <dmurphy@ti.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>   .../devicetree/bindings/leds/leds-meter.yaml       | 42 ++++++++++++++++++++++
>   1 file changed, 42 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/leds/leds-meter.yaml
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-meter.yaml b/Documentation/devicetree/bindings/leds/leds-meter.yaml
> new file mode 100644
> index 0000000..d5dfa261
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-meter.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-meter.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic LED level meter
> +
> +maintainers:
> +  - Akinobu Mita <akinobu.mita@gmail.com>
> +
> +description:
> +  Generic LED level meter consists of multiple LED devices by different drivers.
> +
> +properties:
> +  compatible:
> +    const: meter-leds
> +
> +  leds:

This seems a bit generic for the property name

> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    minItems: 1
> +    description: List of phandles to LED node that are members of a level meter.
> +
> +  brightness-weights:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 1
> +    description: Each integer represents a contribution ratio within a level
> +      meter.

This description is a bit vague I would not be sure what I would set 
this to.

Dan

> +
> +required:
> + - compatible
> + - leds
> +
> +examples:
> +  - |
> +    leds {
> +        compatible = "meter-leds";
> +        leds = <&led0>, <&led1>, <&led2>, <&led3>;
> +        brightness-weights = <3 1 1 1>;
> +    };
> +
> +...

Re: [PATCH 2/2] leds: meter: add leds-meter binding

[Akinobu Mita]

2019年10月23日(水) 21:35 Dan Murphy <dmurphy@ti.com>:
>
> Akinobu
>
> On 10/22/19 10:06 AM, Akinobu Mita wrote:
> > Add DT binding for leds-meter.
>
> $subject should be
>
>   dt-bindings: leds: Add leds-meter binding
>
> And this patch should be first in the series

OK.

> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Bjorn Andersson <bjorn@kryo.se>
> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > Cc: Jean-Jacques Hiblot <jjhiblot@ti.com>
> > Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Cc: Dan Murphy <dmurphy@ti.com>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > ---
> >   .../devicetree/bindings/leds/leds-meter.yaml       | 42 ++++++++++++++++++++++
> >   1 file changed, 42 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/leds/leds-meter.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/leds/leds-meter.yaml b/Documentation/devicetree/bindings/leds/leds-meter.yaml
> > new file mode 100644
> > index 0000000..d5dfa261
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/leds-meter.yaml
> > @@ -0,0 +1,42 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/leds/leds-meter.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Generic LED level meter
> > +
> > +maintainers:
> > +  - Akinobu Mita <akinobu.mita@gmail.com>
> > +
> > +description:
> > +  Generic LED level meter consists of multiple LED devices by different drivers.
> > +
> > +properties:
> > +  compatible:
> > +    const: meter-leds
> > +
> > +  leds:
>
> This seems a bit generic for the property name

I have no problem with changing to more descriptive name like "meter-leds".
But this property is parsed by of_led_get(), so we need to change of_led_get()
to take additional 'prefix' parameter.  I would like to hear led-backlight
authors' opinion.

> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    minItems: 1
> > +    description: List of phandles to LED node that are members of a level meter.
> > +
> > +  brightness-weights:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    minItems: 1
> > +    description: Each integer represents a contribution ratio within a level
> > +      meter.
>
> This description is a bit vague I would not be sure what I would set
> this to.

OK.  I'll add some description and examples like I wrote in the commit
message for patch 1/2.

Re: [PATCH 2/2] leds: meter: add leds-meter binding

[Rob Herring]

On Tue, Oct 22, 2019 at 10:07 AM Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
> Add DT binding for leds-meter.

What's an leds meter? Need a better explanation to understand if this
makes sense at all, but some comments on the schema below.

> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Bjorn Andersson <bjorn@kryo.se>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Jean-Jacques Hiblot <jjhiblot@ti.com>
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Dan Murphy <dmurphy@ti.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  .../devicetree/bindings/leds/leds-meter.yaml       | 42 ++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-meter.yaml
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-meter.yaml b/Documentation/devicetree/bindings/leds/leds-meter.yaml
> new file mode 100644
> index 0000000..d5dfa261
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-meter.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: GPL-2.0

(GPL-2.0-only OR BSD-2-Clause) for new bindings.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-meter.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic LED level meter
> +
> +maintainers:
> +  - Akinobu Mita <akinobu.mita@gmail.com>
> +
> +description:
> +  Generic LED level meter consists of multiple LED devices by different drivers.
> +
> +properties:
> +  compatible:
> +    const: meter-leds
> +
> +  leds:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    minItems: 1

No need for this as the minimum for arrays is already 1. And it
doesn't work either. You'd need the $ref under an 'allOf'.

> +    description: List of phandles to LED node that are members of a level meter.
> +
> +  brightness-weights:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 1
> +    description: Each integer represents a contribution ratio within a level
> +      meter.
> +
> +required:
> + - compatible
> + - leds

Add a:

additionalProperties: false

> +
> +examples:
> +  - |
> +    leds {

Needs to be a name that's not also a property name. 'leds-meter' or
'meter-leds' perhaps. And define the name above under a $nodename
property.

> +        compatible = "meter-leds";
> +        leds = <&led0>, <&led1>, <&led2>, <&led3>;
> +        brightness-weights = <3 1 1 1>;
> +    };
> +
> +...
> --
> 2.7.4
>

Re: [PATCH 2/2] leds: meter: add leds-meter binding

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

On Wed 2019-10-23 09:56:03, Rob Herring wrote:
> On Tue, Oct 22, 2019 at 10:07 AM Akinobu Mita <akinobu.mita@gmail.com> wrote:
> >
> > Add DT binding for leds-meter.
> 
> What's an leds meter? Need a better explanation to understand if this
> makes sense at all, but some comments on the schema below.

It groups several LEDs into one "virtual" LED.

I'm not sure I like it. What is it good for?

We do not have many triggers that dim the LEDs, and if it is only used
from userspace, it can stay in userspace...?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]