Message-ID: <Y5IACDDFH/yBoxPn@google.com>
Date:   Thu, 8 Dec 2022 15:17:28 +0000
From:   Lee Jones <lee@...nel.org>
To:     Chuanhong Guo <gch981213@...il.com>
Cc:     linux-leds@...r.kernel.org, Pavel Machek <pavel@....cz>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Stanislav Jakubek <stano.jakubek@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Johan Hovold <johan+linaro@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        Sven Schwermer <sven.schwermer@...ruptive-technologies.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 3/3] leds: add driver for SPI driven WorldSemi WS2812B
 RGB LEDs

On Thu, 08 Dec 2022, Chuanhong Guo wrote:

> Hi!
> 
> On Wed, Dec 7, 2022 at 7:02 PM Lee Jones <lee@...nel.org> wrote:
> >
> > On Wed, 07 Dec 2022, Chuanhong Guo wrote:
> >
> > > This patch adds support for driving a chain of WS2812B LED chips
> > > using SPI bus.
> > >
> > > WorldSemi WS2812B is a individually addressable LED chip that
> > > can be chained together and controlled individually using a
> > > single wire. The chip recognize a long pulse as a bit of 1 and
> > > a short pulse as a bit of 0. Host sends a continuous stream
> > > of 24-bits color values, each LED chip takes the first 3 byte
> > > it receives as its color value and passes the leftover bytes to
> > > the next LED on the chain.
> > >
> > > This driver simulates this protocol using SPI bus by sending
> > > a long pulse as 3'b110 and a short pulse as 3'b100. The SPI
> > > frequency needs to be 2.105MHz~2.85MHz for the timing to be
> > > correct and the controller needs to transfer all the bytes
> > > continuously.
> > >
> > > Signed-off-by: Chuanhong Guo <gch981213@...il.com>
> > > ---
> > > Changes since v1:
> > > rename the driver to drop -spi suffix
> > > add support for default-brightness
> > > use fwnode apis for properties
> > >
> > > Changes since v2:
> > > drop default-brightness and default-intensity
> > >
> > >  drivers/leds/rgb/Kconfig        |  11 ++
> > >  drivers/leds/rgb/Makefile       |   1 +
> > >  drivers/leds/rgb/leds-ws2812b.c | 219 ++++++++++++++++++++++++++++++++
> > >  3 files changed, 231 insertions(+)
> > >  create mode 100644 drivers/leds/rgb/leds-ws2812b.c
> > >
> > > diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
> > > index 204cf470beae..5c2081852f01 100644
> > > --- a/drivers/leds/rgb/Kconfig
> > > +++ b/drivers/leds/rgb/Kconfig
> > > @@ -26,4 +26,15 @@ config LEDS_QCOM_LPG
> > >
> > >         If compiled as a module, the module will be named leds-qcom-lpg.
> > >
> > > +config LEDS_WS2812B
> > > +     tristate "SPI driven WS2812B RGB LED support"
> > > +     depends on OF
> > > +     depends on SPI
> > > +     help
> > > +       This option enables support for driving daisy-chained WS2812B RGB
> > > +       LED chips using SPI bus. This driver simulates the single-wire
> > > +       protocol by sending bits over the SPI MOSI pin. For this to work,
> > > +       the SPI frequency should be 2.105MHz~2.85MHz and the controller
> > > +       needs to transfer all the bytes continuously.
> > > +
> > >  endif # LEDS_CLASS_MULTICOLOR
> > > diff --git a/drivers/leds/rgb/Makefile b/drivers/leds/rgb/Makefile
> > > index 0675bc0f6e18..a6f855eaeb14 100644
> > > --- a/drivers/leds/rgb/Makefile
> > > +++ b/drivers/leds/rgb/Makefile
> > > @@ -2,3 +2,4 @@
> > >
> > >  obj-$(CONFIG_LEDS_PWM_MULTICOLOR)    += leds-pwm-multicolor.o
> > >  obj-$(CONFIG_LEDS_QCOM_LPG)          += leds-qcom-lpg.o
> > > +obj-$(CONFIG_LEDS_WS2812B)           += leds-ws2812b.o
> > > diff --git a/drivers/leds/rgb/leds-ws2812b.c b/drivers/leds/rgb/leds-ws2812b.c
> > > new file mode 100644
> > > index 000000000000..68c80beb304c
> > > --- /dev/null
> > > +++ b/drivers/leds/rgb/leds-ws2812b.c
> > > @@ -0,0 +1,219 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * WorldSemi WS2812B individually-addressable LED driver using SPI
> > > + *
> > > + * Copyright 2022 Chuanhong Guo <gch981213@...il.com>
> > > + *
> > > + * This driver simulates WS2812B protocol using SPI MOSI pin. A one pulse
> > > + * is transferred as 3'b110 and a zero pulse is 3'b100. For this driver to
> > > + * work properly, the SPI frequency should be 2.105MHz~2.85MHz and it needs
> > > + * to transfer all the bytes continuously.
> > > + */
> > > +
> > > +#include <linux/led-class-multicolor.h>
> > > +#include <linux/leds.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/property.h>
> > > +#include <linux/spi/spi.h>
> > > +#include <linux/mutex.h>
> > > +
> > > +#define WS2812B_BYTES_PER_COLOR 3
> > > +#define WS2812B_NUM_COLORS 3
> > > +#define WS2812B_RESET_LEN 18
> > > +
> > > +struct ws2812b_led {
> > > +     struct led_classdev_mc mc_cdev;
> > > +     struct mc_subled subled[WS2812B_NUM_COLORS];
> > > +     struct ws2812b_priv *priv;
> > > +     int reg;
> >
> > Looks like you're leaking the Device Tree nomenclature into the
> > driver.  IIUC, this is not a reg(ister) value at all, but the LED
> > indices.
> 
> You are right. reg is a bit weird here. I'll go with idx instead.

idx is a terrible variable name (like 'data' or 'value').

Please use something better, like cascade (as per the datasheet).

> > How does the datasheet describe / differentiate them?
> 
> The datasheet only describes a single chip instead of
> a chain of them, so there's no specific word for this,
> it says:
> 
> After the pixel power-on reset, the DIN port receive
> data from controller, the first pixel collect initial 24bit
> data then sent to the internal data latch, the other
> data which reshaping by the internal signal reshaping
> amplification circuit sent to the next cascade pixel
> through the DO port.
> 
> Here's the datasheet:
> https://cdn-shop.adafruit.com/datasheets/WS2812B.pdf
> 
> >
> > > +};
> > > +
> > > +struct ws2812b_priv {
> > > +     struct led_classdev ldev;
> > > +     struct spi_device *spi;
> > > +     struct mutex mutex;
> > > +     int num_leds;
> > > +     size_t data_len;
> > > +     u8 *data_buf;
> > > +     struct ws2812b_led leds[];
> > > +};
> > > +
> > > +static void ws2812b_set_byte(u8 *p, u8 val)
> > > +{
> > > +     /*
> > > +      * Every bit of data is represented using 3 bits: 3'b100 for
> > > +      * 0 and 3'b110 for 1.
> > > +      * 1 byte of data takes up 3 bytes in a SPI transfer. The higher
> > > +      * 3 bits, middle 2 bits and lower 3 bits are represented
> > > +      * with the 1st, 2nd and 3rd byte in the SPI transfer.
> > > +      * Here's the lookup table for them.
> >
> > Sometimes a little ASCII representation can help people visualise the
> > data stream / layout.
> >
> > > +      */
> > > +     const u8 h3b[] = { 0x92, 0x93, 0x9a, 0x9b, 0xd2, 0xd3, 0xda, 0xdb };
> > > +     const u8 m2b[] = { 0x49, 0x4d, 0x69, 0x6d };
> > > +     const u8 l3b[] = { 0x24, 0x26, 0x34, 0x36, 0xa4, 0xa6, 0xb4, 0xb6 };
> >
> > It's taken me a couple of minutes to parse this, which leads me to
> > believe it requires more explanation.  The blurb you've written so
> > far is good, please keep going.  What do the values in the lookup
> > table represent?  I see that brightness is passed in (should val be
> > called brightness too?).  Is the returned data the register values to
> > set that brightness, or something else?
> 
> It is used to set brightness, but it's not register values. I'm abusing
> SPI MOSI to send pulses of specific length. See the comments
> below.
> 
> >
> > Please also consider adding these comments to further the clarity:
> >
> > > +     p[0] = h3b[val >> 5];            /* 0-7 */
> > > +     p[1] = m2b[(val >> 3) & 0x3];    /* 0-3 */
> > > +     p[2] = l3b[val & 0x7];           /* 0-7 */
> > > +}
> 
> /**
> * ws2812b_set_byte - convert a byte of data to 3-byte SPI data for pulses
> * @p: pointer to the 3-byte SPI data
> * @val: 1-byte input data to be converted
> *
> * WS2812B receives a stream of bytes from DI, takes the first 3 byte as LED
> * brightness and pases the rest to the next LED through the DO pin.
> * This function assembles a single byte of data to the LED:
> * A bit is represented with a pulse of specific length. A long pulse is a 1
> * and a short pulse is a 0.
> * SPI transfers data continuously, MSB first. We can send 3'b100 to create a
> * 0 pulse and 3'b110 for a 1 pulse. In this way, a byte of data takes up 3
> * bytes in a SPI transfer:
> * 1x0 1x0 1x0 1x0 1x0 1x0 1x0 1x0
> * Let's rearrange it in 8 bits:
> * 1x01x01x 01x01x01 x01x01x0
> * The higher 3 bits, middle 2 bits and lower 3 bits are represented with the
> * 1st, 2nd and 3rd byte in the SPI transfer respectively.
> * There are only 8 combinations for 3 bits and 4 for 2 bits, so we can create
> * a lookup table for the 3 bytes. e.g. For 0x6b -> 2'b01101011:
> * Bit 7-5: 3'b011 -> 10011011 -> 0x9b
> * Bit 4-3: 2'b01 -> 01001101 -> 0x4d
> * Bit 2-0: 3'b011 -> 00110110 -> 0x36
> */

This is good, thank you.

Please change the formatting so it's a little nicer to read.

E.g. tab out the examples.

> > > +
> > > +static int ws2812b_set(struct led_classdev *cdev,
> > > +                    enum led_brightness brightness)
> > > +{
> > > +     struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> > > +     struct ws2812b_led *led =
> > > +             container_of(mc_cdev, struct ws2812b_led, mc_cdev);
> > > +     struct ws2812b_priv *priv = led->priv;
> > > +     u8 *buf = priv->data_buf + WS2812B_RESET_LEN +
> > > +               led->reg * WS2812B_NUM_COLORS * WS2812B_BYTES_PER_COLOR;
> >
> > Please add some bracketing.  This also goes for the other places you
> > have complex BODMAS type arithmetic where ordering may cause issues.
> 
> OK.
> 
> > Actually, I'm very comfortable with all of this, mostly unparsable (at
> > least quickly) pointer arithmetic happening in this driver.  We have
> > some very readable / maintainable ways of referencing registers /
> > offsets that does not involve register address hopping.  Would you
> > mind revisiting this please?  Have you considered Regmap for instance?
> 
> I couldn't figure out how regmap could make this simpler. If I create a
> regmap for the entire buffer, it just moves this whole calculation into
> reg_read callback isn't it?

Have a go at putting the logic into a MACRO.

Then you can swap out all of the repeated pointer arithmetic.

> BTW the WS2812B_RESET_LEN is for a continuous 0 of more
> than 50us. This indicates the start of a byte stream.

[...]

> > > +
> > > +     for (i = 0; i < num_leds * WS2812B_NUM_COLORS; i++)
> > > +             ws2812b_set_byte(priv->data_buf + WS2812B_RESET_LEN +
> > > +                                      i * WS2812B_BYTES_PER_COLOR,
> > > +                              0);
> >
> > At which point do you usually line-wrap?
> 
> I just write everything in one line and clang-format it :P

Probably best not to do that.  100-chars is good.

-- 
Lee Jones [李琼斯]

Powered by blists - more mailing lists