| Message ID | 20220413162359.325021-6-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | drm/solomon: Add SSD130x OLED displays SPI support | expand |
Hi Javier, On Wed, Apr 13, 2022 at 6:24 PM Javier Martinez Canillas <javierm@redhat.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@redhat.com> > Acked-by: Mark Brown <broonie@kernel.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@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-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
Hello Geert, Thanks a lot for your feedback. On 4/19/22 09:52, Geert Uytterhoeven wrote: [snip] >> +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. > It's true that is easier to read. I just wanted to make it clear that we were removing one byte from the data but I believe the comment is enough. Andy also pointed out an unnecessary blank line in patch 4/5, so I think these two changes + your R-b warrants a v5. I will post one later today. [snip] >> +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. > Right, I'm quite sure about this. See for example [0] vs [1]. The latter does of_device_uevent_modalias(dev, env) while the former always does add_uevent_var(env, "MODALIAS=%s%s", SPI_MODULE_PREFIX, spi->modalias) even for devices registered through OF. Also, commits 3ce6c9e2617e ("spi: add of_device_uevent_modalias support") and 96c8395e2166 ("spi: Revert modalias changes") have some more context. [0]: https://elixir.bootlin.com/linux/latest/source/drivers/spi/spi.c#L360 [1]: https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L139 > The rest LGTM, so > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > Thanks!
diff --git a/drivers/gpu/drm/solomon/Kconfig b/drivers/gpu/drm/solomon/Kconfig index 8c0a0c788385..e170716d976b 100644 --- a/drivers/gpu/drm/solomon/Kconfig +++ b/drivers/gpu/drm/solomon/Kconfig @@ -20,3 +20,12 @@ config DRM_SSD130X_I2C I2C bus. If M is selected the module will be called ssd130x-i2c. + +config DRM_SSD130X_SPI + tristate "DRM support for Solomon SSD130X OLED displays (SPI bus)" + depends on DRM_SSD130X && SPI + select REGMAP + help + Say Y here if the SSD130x OLED display is connected via SPI bus. + + If M is selected the module will be called ssd130x-spi. diff --git a/drivers/gpu/drm/solomon/Makefile b/drivers/gpu/drm/solomon/Makefile index 4bfc5acb0447..b5fc792257d7 100644 --- a/drivers/gpu/drm/solomon/Makefile +++ b/drivers/gpu/drm/solomon/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_DRM_SSD130X) += ssd130x.o obj-$(CONFIG_DRM_SSD130X_I2C) += ssd130x-i2c.o +obj-$(CONFIG_DRM_SSD130X_SPI) += ssd130x-spi.o diff --git a/drivers/gpu/drm/solomon/ssd130x-spi.c b/drivers/gpu/drm/solomon/ssd130x-spi.c new file mode 100644 index 000000000000..c94bbaa731da --- /dev/null +++ b/drivers/gpu/drm/solomon/ssd130x-spi.c @@ -0,0 +1,178 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * DRM driver for Solomon SSD130X OLED displays (SPI bus) + * + * Copyright 2022 Red Hat Inc. + * Authors: Javier Martinez Canillas <javierm@redhat.com> + */ +#include <linux/spi/spi.h> +#include <linux/module.h> + +#include "ssd130x.h" + +#define DRIVER_NAME "ssd130x-spi" +#define DRIVER_DESC "DRM driver for Solomon SSD130X OLED displays (SPI)" + +struct ssd130x_spi_transport { + struct spi_device *spi; + struct gpio_desc *dc; +}; + +static const struct regmap_config ssd130x_spi_regmap_config = { + .reg_bits = 8, + .val_bits = 8, +}; + +/* + * 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); +} + +/* The ssd130x driver does not read registers but regmap expects a .read */ +static int ssd130x_spi_read(void *context, const void *reg, size_t reg_size, + void *val, size_t val_size) +{ + return -EOPNOTSUPP; +} + +/* + * A custom bus is needed due the special write that toggles a D/C# pin, + * another option could be to just have a .reg_write() callback but that + * will prevent to do data writes in bulk. + * + * Once the regmap API is extended to support defining a bulk write handler + * in the struct regmap_config, this can be simplified and the bus dropped. + */ +static struct regmap_bus regmap_ssd130x_spi_bus = { + .write = ssd130x_spi_write, + .read = ssd130x_spi_read, +}; + +static int ssd130x_spi_probe(struct spi_device *spi) +{ + struct ssd130x_spi_transport *t; + struct ssd130x_device *ssd130x; + struct regmap *regmap; + struct gpio_desc *dc; + struct device *dev = &spi->dev; + + dc = devm_gpiod_get(dev, "dc", GPIOD_OUT_LOW); + if (IS_ERR(dc)) + return dev_err_probe(dev, PTR_ERR(dc), + "Failed to get dc gpio\n"); + + t = devm_kzalloc(dev, sizeof(*t), GFP_KERNEL); + if (!t) + return dev_err_probe(dev, -ENOMEM, + "Failed to allocate SPI transport data\n"); + + t->spi = spi; + t->dc = dc; + + regmap = devm_regmap_init(dev, ®map_ssd130x_spi_bus, t, + &ssd130x_spi_regmap_config); + if (IS_ERR(regmap)) + return PTR_ERR(regmap); + + ssd130x = ssd130x_probe(dev, regmap); + if (IS_ERR(ssd130x)) + return PTR_ERR(ssd130x); + + spi_set_drvdata(spi, ssd130x); + + return 0; +} + +static void ssd130x_spi_remove(struct spi_device *spi) +{ + struct ssd130x_device *ssd130x = spi_get_drvdata(spi); + + ssd130x_remove(ssd130x); +} + +static void ssd130x_spi_shutdown(struct spi_device *spi) +{ + struct ssd130x_device *ssd130x = spi_get_drvdata(spi); + + ssd130x_shutdown(ssd130x); +} + +static const struct of_device_id ssd130x_of_match[] = { + { + .compatible = "sinowealth,sh1106", + .data = &ssd130x_variants[SH1106_ID], + }, + { + .compatible = "solomon,ssd1305", + .data = &ssd130x_variants[SSD1305_ID], + }, + { + .compatible = "solomon,ssd1306", + .data = &ssd130x_variants[SSD1306_ID], + }, + { + .compatible = "solomon,ssd1307", + .data = &ssd130x_variants[SSD1307_ID], + }, + { + .compatible = "solomon,ssd1309", + .data = &ssd130x_variants[SSD1309_ID], + }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, ssd130x_of_match); + +/* + * 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); + +static struct spi_driver ssd130x_spi_driver = { + .driver = { + .name = DRIVER_NAME, + .of_match_table = ssd130x_of_match, + }, + .probe = ssd130x_spi_probe, + .remove = ssd130x_spi_remove, + .shutdown = ssd130x_spi_shutdown, +}; +module_spi_driver(ssd130x_spi_driver); + +MODULE_DESCRIPTION(DRIVER_DESC); +MODULE_AUTHOR("Javier Martinez Canillas <javierm@redhat.com>"); +MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS(DRM_SSD130X);