* [PATCH] leds: add LED driver for Worldsemi WS2812B
@ 2022-03-14 12:12 Ivan Vozvakhov
2022-03-16 15:28 ` Andy Shevchenko
0 siblings, 1 reply; 2+ messages in thread
From: Ivan Vozvakhov @ 2022-03-14 12:12 UTC (permalink / raw)
To: pavel, linux-leds; +Cc: Ivan Vozvakhov
This patch adds a LED class driver (powered by SPI)
for the WS2812B LEDs that's is widely used in
consumer electronic devices and DIY.
Signed-off-by: Ivan Vozvakhov <i.vozvakhov@corp.mail.ru>
---
.../bindings/leds/leds-ws2812b.yaml | 76 ++++
drivers/leds/Kconfig | 12 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-ws2812b.c | 420 ++++++++++++++++++
4 files changed, 509 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/leds-ws2812b.yaml
create mode 100644 drivers/leds/leds-ws2812b.c
diff --git a/Documentation/devicetree/bindings/leds/leds-ws2812b.yaml b/Documentation/devicetree/bindings/leds/leds-ws2812b.yaml
new file mode 100644
index 000000000000..a71f37f51e2a
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-ws2812b.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-ws2812b.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Worldsemi WS2812B LED's driver powered by SPI
+
+maintainers:
+ - Ivan Vozvakhov <i.vozvakhov@vk.team>
+
+description: |
+ Bindings for the Worldsemi WS2812B LED's powered by SPI.
+ Used SPI-MOSI only.
+
+ For more product information please see the link below:
+ http://www.world-semi.com/Certifications/WS2812B.html
+
+properties:
+ compatible:
+ const: worldsemi,ws2812b
+
+ reg:
+ maxItems: 1
+
+ spi-max-frequency:
+ const: 2500000
+
+ device-name:
+ type: string
+
+patternProperties:
+ "(^led[0-9a-f]$|led)":
+ type: object
+ $ref: common.yaml#
+
+required:
+ - compatible
+ - reg
+ - spi-max-frequency
+
+additionalProperties: false
+
+examples:
+ - |
+ &spi0 {
+ status = "okay";
+ pinctrl-0 = <&spi0_mosi>;
+
+ ws2812b@00 {
+ compatible = "worldsemi,ws2812b";
+ reg = <0x00>;
+ spi-max-frequency = <2500000>;
+
+ led1 {
+ label = "top-led1";
+ color = <LED_COLOR_ID_GREEN>;
+ };
+
+ led2 {
+ label = "top-led2";
+ color = <LED_COLOR_ID_RED>;
+ };
+
+ led3 {
+ label = "top-led3";
+ color = <LED_COLOR_ID_BLUE>;
+ };
+ };
+ };
+
+ &spi0_mosi_hs {
+ rockchip,pins = <2 RK_PA1 2 &pcfg_pull_down>;
+ };
+
+...
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 6090e647daee..4eda92a2c0b2 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -157,6 +157,18 @@ config LEDS_EL15203000
To compile this driver as a module, choose M here: the module
will be called leds-el15203000.
+config LEDS_WS2812B
+ tristate "LED Support for Worldsemi WS2812B"
+ depends on LEDS_CLASS
+ depends on SPI
+ depends on OF
+ help
+ This option enables support for WS2812B LED's
+ through SPI.
+
+ To compile this driver as a module, choose M here: the module
+ will be called leds-ws2812b.
+
config LEDS_TURRIS_OMNIA
tristate "LED support for CZ.NIC's Turris Omnia"
depends on LEDS_CLASS_MULTICOLOR
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index e58ecb36360f..6eef9b731884 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -92,6 +92,7 @@ obj-$(CONFIG_LEDS_CR0014114) += leds-cr0014114.o
obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
obj-$(CONFIG_LEDS_EL15203000) += leds-el15203000.o
obj-$(CONFIG_LEDS_SPI_BYTE) += leds-spi-byte.o
+obj-$(CONFIG_LEDS_WS2812B) += leds-ws2812b.o
# LED Userspace Drivers
obj-$(CONFIG_LEDS_USER) += uleds.o
diff --git a/drivers/leds/leds-ws2812b.c b/drivers/leds/leds-ws2812b.c
new file mode 100644
index 000000000000..daef470e073e
--- /dev/null
+++ b/drivers/leds/leds-ws2812b.c
@@ -0,0 +1,420 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * LEDs driver for Worldsemi WS2812B through SPI
+ * SPI-MOSI for data transfer
+ * Required DMA transfers
+ *
+ * Copyright (C) 2022 Ivan Vozvakhov <i.vozvakhov@vk.team>
+ *
+ * Inspired by (C) Martin Sperl <kernel@martin.sperl.org>
+ *
+ */
+#include <linux/leds.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/workqueue.h>
+#include <linux/spi/spi.h>
+#include <linux/uaccess.h>
+#include <linux/miscdevice.h>
+
+/*
+ * WS2812B timings:
+ * TH + TL = 1.25us +-600us
+ * T0H: 0.4us +-150ns
+ * T1H: 0.8us +-150ns
+ * T0L: 0.85us +-150ns
+ * T1L: 0.45us +-150ns
+ * RESL: >50us
+ *
+ * Each bit led's state coding by 3 real bits (see tables above):
+ * T0H and T0L as 1 bit, T1H and T1L as 2 bits.
+ *
+ * And let's assume SPI bus freq. to 2.5MHz.
+ * By that:
+ * T0H: 0.4us
+ * T1H: 0.8us
+ * T0L: 0.8us
+ * T1L: 0.4us
+ * RESL: > (50 / 0.4 = 125) bit (16 bytes)
+ */
+#define SPI_BUS_SPEED_HZ 2500000
+#define RESET_BYTES 16
+/*
+ * Basically, SPI pull-up MOSI line, but for correct state it should be pull-down
+ * (RES is detected by low signal).
+ * SPI-MOSI for some controllers could have z-state with pull-down for MOSI
+ * before first SPI-CLK edges.
+ * To eliminate it, send RES sequence before first bit's.
+ */
+#define DELAY_BEFORE_FIRST_DATA RESET_BYTES
+#define DEFAULT_DEVICE_NAME "ws2812b"
+
+/*
+ * Ioctl interface for set's several led's at one time.
+ *
+ * [start_led, stop_led)
+ */
+struct ws2812b_multi_set {
+ int start_led;
+ int stop_led;
+ uint8_t *brightnesses;
+};
+
+#define LEDS_WS2812B_IOCTL_MAGIC 'z'
+#define LEDS_WS2812B_IOCTL_MULTI_SET \
+ _IOW(LEDS_WS2812B_IOCTL_MAGIC, 0x01, struct ws2812b_multi_set)
+#define LEDS_WS2812B_IOCTL_GET_LEDS_NUMBER \
+ _IOR(LEDS_WS2812B_IOCTL_MAGIC, 0x02, int)
+
+/*
+ * Each led's state bits coded by 3 bits,
+ * 8 led's one-color state (actual LED) would take 24 real-bits.
+ * That 24 bits divided into high, medium, low groups.
+ * All possible states defined there (see brightess_encode func. for masks).
+ */
+const char byte2encoding_h[] = {
+ 0x92, 0x93, 0x9a, 0x9b,
+ 0xd2, 0xd3, 0xda, 0xdb
+};
+
+const char byte2encoding_m[] = {
+ 0x49, 0x4d, 0x69, 0x6d
+};
+
+const char byte2encoding_l[] = {
+ 0x24, 0x26, 0x34, 0x36,
+ 0xa4, 0xa6, 0xb4, 0xb6
+};
+
+struct ws2812b_encoding {
+ uint8_t h, m, l;
+};
+
+static void brightess_encode(
+ struct ws2812b_encoding *enc,
+ const uint8_t val)
+{
+ enc->h = byte2encoding_h[(val >> 5) & 0x07];
+ enc->m = byte2encoding_m[(val >> 3) & 0x03];
+ enc->l = byte2encoding_l[(val >> 0) & 0x07];
+}
+
+struct ws2812b_led {
+ struct led_classdev ldev;
+ spinlock_t led_data_lock;
+
+ uint8_t brightness;
+ int num;
+
+ struct device *dev;
+ struct device_node *child;
+
+ struct work_struct work;
+ struct ws2812b_priv *priv;
+};
+
+struct ws2812b_priv {
+ struct mutex ws2812b_mutex;
+
+ struct spi_device *spi;
+ struct spi_message spi_msg;
+ struct spi_transfer spi_xfer;
+ struct ws2812b_encoding *spi_data;
+
+ struct miscdevice mdev;
+ struct work_struct work_update_all;
+ int num_leds;
+
+ struct ws2812b_led *leds;
+};
+
+static void ws2812b_all_leds_update_work(struct work_struct *work)
+{
+ struct ws2812b_priv *priv = container_of(work, struct ws2812b_priv, work_update_all);
+ struct ws2812b_encoding *led_enc = priv->spi_data;
+ struct ws2812b_led *led = priv->leds;
+ int i;
+
+ led_enc = (struct ws2812b_encoding *)((uint8_t *)led_enc + DELAY_BEFORE_FIRST_DATA);
+
+ mutex_lock(&priv->ws2812b_mutex);
+ for (i = 0; i < priv->num_leds; i++, led_enc++, led++)
+ brightess_encode(led_enc, led->brightness);
+ spi_sync(priv->spi, &priv->spi_msg);
+ mutex_unlock(&priv->ws2812b_mutex);
+}
+
+static void ws2812b_led_work(struct work_struct *work)
+{
+ struct ws2812b_led *led = container_of(work, struct ws2812b_led, work);
+ struct ws2812b_priv *priv = led->priv;
+ struct ws2812b_encoding *led_enc = &priv->spi_data[led->num];
+
+ led_enc = (struct ws2812b_encoding *)((uint8_t *)led_enc + DELAY_BEFORE_FIRST_DATA);
+
+ mutex_lock(&priv->ws2812b_mutex);
+ brightess_encode(led_enc, led->brightness);
+ spi_sync(priv->spi, &priv->spi_msg);
+ mutex_unlock(&priv->ws2812b_mutex);
+}
+
+static void ws2812b_led_set_brightness(struct led_classdev *ldev,
+ enum led_brightness brightness)
+{
+ struct ws2812b_led *led = container_of(ldev, struct ws2812b_led, ldev);
+
+ spin_lock(&led->led_data_lock);
+ led->brightness = (uint8_t) brightness;
+ schedule_work(&led->work);
+ spin_unlock(&led->led_data_lock);
+}
+
+static int ws2812b_open(struct inode *inode, struct file *file)
+{
+ return 0;
+}
+
+static int ws2812b_release(struct inode *inode, struct file *file)
+{
+ return 0;
+}
+
+static long ws2812b_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ struct miscdevice *mdev = file->private_data;
+ struct ws2812b_priv *priv = container_of(mdev, struct ws2812b_priv, mdev);
+ struct ws2812b_led *led;
+ struct ws2812b_multi_set ms;
+ uint8_t *brightness;
+ int i = 0, ret = 0, leds_to_change;
+
+ switch (cmd) {
+ case LEDS_WS2812B_IOCTL_MULTI_SET:
+ {
+ if (copy_from_user(&ms, (void __user *)arg,
+ sizeof(struct ws2812b_multi_set))) {
+ ret = -EFAULT;
+ break;
+ }
+
+ leds_to_change = ms.stop_led - ms.start_led;
+ if (ms.start_led < 0
+ || leds_to_change > priv->num_leds
+ || leds_to_change < 1) {
+ ret = -EINVAL;
+ break;
+ }
+
+ brightness = kmalloc(sizeof(uint8_t) * leds_to_change, GFP_KERNEL);
+ if (!brightness)
+ return -ENOMEM;
+
+ if (copy_from_user(brightness, ms.brightnesses,
+ sizeof(uint8_t) * leds_to_change)) {
+ ret = -EFAULT;
+ break;
+ }
+
+ for (i = ms.start_led, led = priv->leds+ms.start_led;
+ i < ms.stop_led;
+ i++, led++, brightness++) {
+ spin_lock(&led->led_data_lock);
+ led->brightness = *brightness;
+ }
+ schedule_work(&priv->work_update_all);
+
+ for (i = ms.start_led, led = priv->leds+ms.start_led;
+ i < ms.stop_led;
+ i++, led++) {
+ spin_unlock(&led->led_data_lock);
+ }
+ kfree(brightness-leds_to_change);
+ break;
+ }
+ case LEDS_WS2812B_IOCTL_GET_LEDS_NUMBER:
+ {
+ int __user *p = (int __user *)arg;
+
+ ret = put_user(priv->num_leds, p);
+ break;
+ }
+ default:
+ break;
+ }
+
+ return ret;
+}
+
+static const struct file_operations ws2812b_ops = {
+ .owner = THIS_MODULE,
+ .open = ws2812b_open,
+ .release = ws2812b_release,
+ .unlocked_ioctl = ws2812b_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = ws2812b_ioctl,
+#endif
+};
+
+static int ws2812b_parse_child_dt(const struct device *dev,
+ struct device_node *child,
+ struct ws2812b_led *led)
+{
+ struct led_classdev *ldev = &led->ldev;
+ const char *state;
+
+ if (of_property_read_string(child, "label", &ldev->name))
+ ldev->name = child->name;
+
+ state = of_get_property(child, "default-state", NULL);
+ if (state) {
+ if (!strcmp(state, "on")) {
+ ldev->brightness = LED_FULL;
+ } else if (strcmp(state, "off")) {
+ dev_err(dev, "default-state can only be 'on' or 'off'");
+ return -EINVAL;
+ }
+ ldev->brightness = LED_OFF;
+ }
+
+ ldev->brightness_set = ws2812b_led_set_brightness;
+
+ INIT_WORK(&led->work, ws2812b_led_work);
+
+ return 0;
+}
+
+static int ws2812b_parse_dt(struct device *dev,
+ struct ws2812b_priv *priv)
+{
+ struct device_node *child;
+ int ret = 0, i = 0;
+
+ for_each_child_of_node(dev->of_node, child) {
+ struct ws2812b_led *led = &priv->leds[i];
+
+ led->priv = priv;
+ led->dev = dev;
+ led->child = child;
+ led->num = i;
+
+ spin_lock_init(&led->led_data_lock);
+
+ ret = ws2812b_parse_child_dt(dev, child, led);
+
+ if (ret)
+ goto err;
+
+ ret = devm_led_classdev_register(dev, &led->ldev);
+ if (ret) {
+ dev_err(dev, "failed to register led for %s: %d\n", led->ldev.name, ret);
+ goto err;
+ }
+
+ led->ldev.dev->of_node = child;
+ i++;
+ }
+
+ return 0;
+err:
+ of_node_put(child);
+ return ret;
+}
+
+static const struct of_device_id ws2812b_driver_ids[] = {
+ { .compatible = "worldsemi,ws2812b" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, ws2812b_driver_ids);
+
+static int ws2812b_probe(struct spi_device *spi)
+{
+ struct device *dev = &spi->dev;
+ struct ws2812b_priv *priv;
+ struct ws2812b_encoding *spi_data;
+ int ret, len, count_leds;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ count_leds = of_get_child_count(dev->of_node);
+ if (!count_leds) {
+ dev_err(dev, "should define at least one led\n");
+ return -EINVAL;
+ }
+
+ priv->num_leds = count_leds;
+ priv->leds = devm_kzalloc(dev, sizeof(struct ws2812b_led) * count_leds, GFP_KERNEL);
+
+ mutex_init(&priv->ws2812b_mutex);
+
+ len = DELAY_BEFORE_FIRST_DATA + count_leds * sizeof(struct ws2812b_encoding) + RESET_BYTES;
+ spi_data = devm_kzalloc(dev, len, GFP_KERNEL);
+ if (!spi_data)
+ return -ENOMEM;
+ priv->spi_data = spi_data;
+
+ priv->spi = spi;
+ spi_message_init(&priv->spi_msg);
+ priv->spi_xfer.len = len;
+ priv->spi_xfer.tx_buf = spi_data;
+ priv->spi_xfer.speed_hz = SPI_BUS_SPEED_HZ;
+ spi_message_add_tail(&priv->spi_xfer, &priv->spi_msg);
+
+ priv->mdev.minor = MISC_DYNAMIC_MINOR;
+ priv->mdev.fops = &ws2812b_ops;
+ priv->mdev.parent = NULL;
+
+ if (of_property_read_string(dev->of_node, "device-name", &priv->mdev.name))
+ priv->mdev.name = DEFAULT_DEVICE_NAME;
+
+ ret = misc_register(&priv->mdev);
+ if (ret) {
+ dev_err(dev, "can't register %s device\n", priv->mdev.name);
+ return ret;
+ }
+
+ INIT_WORK(&priv->work_update_all, ws2812b_all_leds_update_work);
+
+ spi_set_drvdata(spi, priv);
+
+ ret = ws2812b_parse_dt(dev, priv);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int ws2812b_remove(struct spi_device *spi)
+{
+ struct ws2812b_priv *priv = spi_get_drvdata(spi);
+ int i;
+
+ for (i = 0; i < priv->num_leds; i++) {
+ led_classdev_unregister(&priv->leds[i].ldev);
+ cancel_work_sync(&priv->leds[i].work);
+ }
+ cancel_work_sync(&priv->work_update_all);
+
+ return 0;
+}
+
+static struct spi_driver ws2812b_driver = {
+ .probe = ws2812b_probe,
+ .remove = ws2812b_remove,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .owner = THIS_MODULE,
+ .of_match_table = ws2812b_driver_ids,
+ },
+};
+
+module_spi_driver(ws2812b_driver);
+
+MODULE_AUTHOR("Ivan Vozvakhov <i.vozvakhov@vk.team>");
+MODULE_DESCRIPTION("WS2812B LED driver powered by SPI");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("spi:ws2812b");
--
2.25.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] leds: add LED driver for Worldsemi WS2812B
2022-03-14 12:12 [PATCH] leds: add LED driver for Worldsemi WS2812B Ivan Vozvakhov
@ 2022-03-16 15:28 ` Andy Shevchenko
0 siblings, 0 replies; 2+ messages in thread
From: Andy Shevchenko @ 2022-03-16 15:28 UTC (permalink / raw)
To: Ivan Vozvakhov; +Cc: Pavel Machek, Linux LED Subsystem
On Mon, Mar 14, 2022 at 7:23 PM Ivan Vozvakhov <i.vozvakhov@corp.mail.ru> wrote:
>
> This patch adds a LED class driver (powered by SPI)
> for the WS2812B LEDs that's is widely used in
that's --> that
> consumer electronic devices and DIY.
Any links to the datasheet?
You may add it as a Datasheet: tag.
...
> +maintainers:
> + - Ivan Vozvakhov <i.vozvakhov@vk.team>
By email I can't think of this being the same person as the author /
submitter of the patch. Can you elaborate why the addresses are
different?
...
> +description: |
> + Bindings for the Worldsemi WS2812B LED's powered by SPI.
LEDs
> + Used SPI-MOSI only.
I believe you meant slightly different, i.e. "The only SPI MOSI wire
is in use." But then the question here is, what about CS?
...
> +config LEDS_WS2812B
> + tristate "LED Support for Worldsemi WS2812B"
> + depends on LEDS_CLASS
> + depends on SPI
> + depends on OF
It should be quite good justification why this (OF) depency is here
> + help
> + This option enables support for WS2812B LED's
LEDs
> + through SPI.
...
> +/*
> + * LEDs driver for Worldsemi WS2812B through SPI
> + * SPI-MOSI for data transfer
Isn't it obvious?
> + * Required DMA transfers
Why?
> + * Copyright (C) 2022 Ivan Vozvakhov <i.vozvakhov@vk.team>
> + *
> + * Inspired by (C) Martin Sperl <kernel@martin.sperl.org>
Inspired by a person? Or by some work done by this person? I would
recommend finding somebody for a proof-reading the English text in the
comments and similar pieces of this contribution.
> + *
Redundant blank line.
> + */
...
> +#include <linux/of.h>
I believe this can be replaced by mod_devicetable.h and property.h,
and the latter one if it's really needed. Let's see below...
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/workqueue.h>
> +#include <linux/spi/spi.h>
> +#include <linux/uaccess.h>
> +#include <linux/miscdevice.h>
Perhaps ordered?
...
> +#define SPI_BUS_SPEED_HZ 2500000
Why? It should come at least from DT.
> +/*
> + * Basically, SPI pull-up MOSI line, but for correct state it should be pull-down
the correct
pulled-down
> + * (RES is detected by low signal).
> + * SPI-MOSI for some controllers could have z-state with pull-down for MOSI
a Z-state
> + * before first SPI-CLK edges.
the first
> + * To eliminate it, send RES sequence before first bit's.
> + */
...
> +/*
> + * Ioctl interface for set's several led's at one time.
IOCTL
LEDs
> + *
> + * [start_led, stop_led)
> + */
> +struct ws2812b_multi_set {
> + int start_led;
> + int stop_led;
> + uint8_t *brightnesses;
AFAIUC this is not gonna work in IOCTLs.
> +};
> +
> +#define LEDS_WS2812B_IOCTL_MAGIC 'z'
> +#define LEDS_WS2812B_IOCTL_MULTI_SET \
> + _IOW(LEDS_WS2812B_IOCTL_MAGIC, 0x01, struct ws2812b_multi_set)
> +#define LEDS_WS2812B_IOCTL_GET_LEDS_NUMBER \
> + _IOR(LEDS_WS2812B_IOCTL_MAGIC, 0x02, int)
Where is the documentation part?
And why do you need a non-standard IOCTL?
> +/*
> + * Each led's state bits coded by 3 bits,
If I got right the meaning it should be something like:
"Each of the LED state is coded by 3 bits,"
> + * 8 led's one-color state (actual LED) would take 24 real-bits.
LEDs
> + * That 24 bits divided into high, medium, low groups.
> + * All possible states defined there (see brightess_encode func. for masks).
> + */
> +const char byte2encoding_h[] = {
> + 0x92, 0x93, 0x9a, 0x9b,
> + 0xd2, 0xd3, 0xda, 0xdb
Here and elsewhere in similar cases leave the comma at the end.
> +};
...
> +static void brightess_encode(
> + struct ws2812b_encoding *enc,
> + const uint8_t val)
You have issues with indentation. Same applies to many other places in
this patch.
> +{
> + enc->h = byte2encoding_h[(val >> 5) & 0x07];
> + enc->m = byte2encoding_m[(val >> 3) & 0x03];
> + enc->l = byte2encoding_l[(val >> 0) & 0x07];
Use GENMASK().
> +}
...
> + led_enc = (struct ws2812b_encoding *)((uint8_t *)led_enc + DELAY_BEFORE_FIRST_DATA);
Urgh! Why so many interesting castings? Can you avoid this?
...
> + for (i = 0; i < priv->num_leds; i++, led_enc++, led++)
Why do you need the last two increments? Wouldn't array indices work?
...
> + brightness = kmalloc(sizeof(uint8_t) * leds_to_change, GFP_KERNEL);
> + if (!brightness)
> + return -ENOMEM;
> +
> + if (copy_from_user(brightness, ms.brightnesses,
> + sizeof(uint8_t) * leds_to_change)) {
> + ret = -EFAULT;
> + break;
> + }
Seems like NIH memdup_user()
...
> +static int ws2812b_parse_child_dt(const struct device *dev,
> + struct device_node *child,
> + struct ws2812b_led *led)
> +{
> + struct led_classdev *ldev = &led->ldev;
> + const char *state;
> +
> + if (of_property_read_string(child, "label", &ldev->name))
> + ldev->name = child->name;
> +
> + state = of_get_property(child, "default-state", NULL);
> + if (state) {
> + if (!strcmp(state, "on")) {
> + ldev->brightness = LED_FULL;
> + } else if (strcmp(state, "off")) {
> + dev_err(dev, "default-state can only be 'on' or 'off'");
> + return -EINVAL;
> + }
> + ldev->brightness = LED_OFF;
> + }
Isn't it done in the LED core?
> + ldev->brightness_set = ws2812b_led_set_brightness;
> +
> + INIT_WORK(&led->work, ws2812b_led_work);
> +
> + return 0;
> +}
> +
> +static int ws2812b_parse_dt(struct device *dev,
> + struct ws2812b_priv *priv)
> +{
> + struct device_node *child;
> + int ret = 0, i = 0;
'ret = 0' is a redundant assignment.
> + for_each_child_of_node(dev->of_node, child) {
device_for_each_child_node() ?
> + struct ws2812b_led *led = &priv->leds[i];
> +
> + led->priv = priv;
> + led->dev = dev;
> + led->child = child;
> + led->num = i;
> +
> + spin_lock_init(&led->led_data_lock);
> +
> + ret = ws2812b_parse_child_dt(dev, child, led);
> +
> + if (ret)
> + goto err;
> +
> + ret = devm_led_classdev_register(dev, &led->ldev);
> + if (ret) {
> + dev_err(dev, "failed to register led for %s: %d\n", led->ldev.name, ret);
> + goto err;
> + }
> +
> + led->ldev.dev->of_node = child;
> + i++;
> + }
> +
> + return 0;
> +err:
> + of_node_put(child);
> + return ret;
> +}
...
> +static const struct of_device_id ws2812b_driver_ids[] = {
> + { .compatible = "worldsemi,ws2812b" },
> + {},
No comma for terminator entry.
> +};
...
> + count_leds = of_get_child_count(dev->of_node);
> + if (!count_leds) {
> + dev_err(dev, "should define at least one led\n");
> + return -EINVAL;
return dev_err_probe(...);
> + }
...
> + priv->leds = devm_kzalloc(dev, sizeof(struct ws2812b_led) * count_leds, GFP_KERNEL);
devm_kcalloc()
...
> + len = DELAY_BEFORE_FIRST_DATA + count_leds * sizeof(struct ws2812b_encoding) + RESET_BYTES;
Needs a use of a macro from overflow.h. I believe it's struct_size() or so.
...
> + if (of_property_read_string(dev->of_node, "device-name", &priv->mdev.name))
Is it standard binding?
> + priv->mdev.name = DEFAULT_DEVICE_NAME;
...
> + ret = misc_register(&priv->mdev);
> + if (ret) {
> + dev_err(dev, "can't register %s device\n", priv->mdev.name);
> + return ret;
return dev_err_probe(...);
> + }
...
> + ret = ws2812b_parse_dt(dev, priv);
> + if (ret)
Resource leak, no?
> + return ret;
In general mixing devm with non-devm is a bad idea.
...
> +static int ws2812b_remove(struct spi_device *spi)
> +{
> + struct ws2812b_priv *priv = spi_get_drvdata(spi);
> + int i;
> +
> + for (i = 0; i < priv->num_leds; i++) {
> + led_classdev_unregister(&priv->leds[i].ldev);
> + cancel_work_sync(&priv->leds[i].work);
> + }
> + cancel_work_sync(&priv->work_update_all);
Ditto.
> + return 0;
> +}
...
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .owner = THIS_MODULE,
This is done by a macro. Read its implementation.
> + .of_match_table = ws2812b_driver_ids,
> + },
> +};
> +
Redundant blank line.
> +module_spi_driver(ws2812b_driver);
...
> +MODULE_ALIAS("spi:ws2812b");
Why?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-03-16 15:29 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-14 12:12 [PATCH] leds: add LED driver for Worldsemi WS2812B Ivan Vozvakhov
2022-03-16 15:28 ` Andy Shevchenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.