linux-kernel - Re: [PATCH v4 5/5] drm/solomon: Add SSD130x OLED displays SPI support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 19 Apr 2022 09:52:08 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Javier Martinez Canillas <javierm@...hat.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Mark Brown <broonie@...nel.org>,
        Neil Armstrong <narmstrong@...libre.com>,
        Chen-Yu Tsai <wens@...nel.org>, Rob Herring <robh@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        DRI Development <dri-devel@...ts.freedesktop.org>,
        Chen-Yu Tsai <wens@...e.org>, Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Maxime Ripard <maxime@...no.tech>,
        YueHaibing <yuehaibing@...wei.com>
Subject: Re: [PATCH v4 5/5] drm/solomon: Add SSD130x OLED displays SPI support

Hi Javier,

On Wed, Apr 13, 2022 at 6:24 PM Javier Martinez Canillas
<javierm@...hat.com> wrote:
> The ssd130x driver only provides the core support for these devices but it
> does not have any bus transport logic. Add a driver to interface over SPI.
>
> There is a difference in the communication protocol when using 4-wire SPI
> instead of I2C. For the latter, a control byte that contains a D/C# field
> has to be sent. This field tells the controller whether the data has to be
> written to the command register or to the graphics display data memory.
>
> But for 4-wire SPI that control byte is not used, instead a real D/C# line
> must be pulled HIGH for commands data and LOW for graphics display data.
>
> For this reason the standard SPI regmap can't be used and a custom .write
> bus handler is needed.
>
> Signed-off-by: Javier Martinez Canillas <javierm@...hat.com>
> Acked-by: Mark Brown <broonie@...nel.org>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/gpu/drm/solomon/ssd130x-spi.c

> +/*
> + * The regmap bus .write handler, it is just a wrapper around spi_write()
> + * but toggling the Data/Command control pin (D/C#). Since for 4-wire SPI
> + * a D/C# pin is used, in contrast with I2C where a control byte is sent,
> + * prior to every data byte, that contains a bit with the D/C# value.
> + *
> + * These control bytes are considered registers by the ssd130x core driver
> + * and can be used by the ssd130x SPI driver to determine if the data sent
> + * is for a command register or for the Graphic Display Data RAM (GDDRAM).
> + */
> +static int ssd130x_spi_write(void *context, const void *data, size_t count)
> +{
> +       struct ssd130x_spi_transport *t = context;
> +       struct spi_device *spi = t->spi;
> +       const u8 *reg = data;
> +
> +       if (*reg == SSD130X_COMMAND)
> +               gpiod_set_value_cansleep(t->dc, 0);
> +
> +       if (*reg == SSD130X_DATA)
> +               gpiod_set_value_cansleep(t->dc, 1);
> +
> +       /* Remove the control byte since is not used by the 4-wire SPI */
> +       return spi_write(spi, ((u8 *)data) + 1, count - 1);

As I don't like casts, perhaps

    spi_write(spi, reg + 1, count - 1);

? But this is up to you.

> +/*
> + * The SPI core always reports a MODALIAS uevent of the form "spi:<dev>", even
> + * if the device was registered via OF. This means that the module will not be
> + * auto loaded, unless it contains an alias that matches the MODALIAS reported.
> + *
> + * To workaround this issue, add a SPI device ID table. Even when this should
> + * not be needed for this driver to match the registered SPI devices.
> + */
> +static const struct spi_device_id ssd130x_spi_table[] = {
> +       { "sh1106",  SH1106_ID },
> +       { "ssd1305", SSD1305_ID },
> +       { "ssd1306", SSD1306_ID },
> +       { "ssd1307", SSD1307_ID },
> +       { "ssd1309", SSD1309_ID },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(spi, ssd130x_spi_table);

I'm not sure about the need for this part, but as Mark provided his
Ac-ed--by, I assume it's correct.

The rest LGTM, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@...der.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds