| Message ID | 20200102141246.370-4-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Add support for Okaya RH128128T | expand |
Hi Geert. Good to see we add more functionality to the smallest driver in DRM. The patch triggered a few comments - see below. Some comments relates to the original driver - and not your changes. Sam On Thu, Jan 02, 2020 at 03:12:46PM +0100, Geert Uytterhoeven wrote: > Add support for the Okaya RH128128T display to the st7735r driver. > > The RH128128T is a 128x128 1.44" TFT display driven by a Sitronix > ST7715R TFT Controller/Driver. The latter is very similar to the > ST7735R, and can be handled by the existing st7735r driver. As a general comment - it would have eased review if this was split in two patches. One patch to introduce the infrastructure to deal with another set of controller/display and one patch introducing the new combination. > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/gpu/drm/tiny/st7735r.c | 65 ++++++++++++++++++++++++++++------ > 1 file changed, 55 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c > index 3f4487c716848cf8..05d162e76d8481e5 100644 > --- a/drivers/gpu/drm/tiny/st7735r.c > +++ b/drivers/gpu/drm/tiny/st7735r.c > @@ -1,8 +1,9 @@ > // SPDX-License-Identifier: GPL-2.0+ > /* > - * DRM driver for Sitronix ST7735R panels > + * DRM driver for Sitronix ST7715R/ST7735R panels This comment could describe the situation a little better. This is a sitronix st7735r controller with a jianda jd-t18003-t01 display. Or a sitronix st7715r controller with a okaya rh128128t display. > * > * Copyright 2017 David Lechner <david@lechnology.com> > + * Copyright (C) 2019 Glider bvba > */ > > #include <linux/backlight.h> > @@ -10,6 +11,7 @@ > #include <linux/dma-buf.h> > #include <linux/gpio/consumer.h> > #include <linux/module.h> > +#include <linux/of_device.h> > #include <linux/property.h> > #include <linux/spi/spi.h> > #include <video/mipi_display.h> > @@ -37,12 +39,28 @@ > #define ST7735R_MY BIT(7) > #define ST7735R_MX BIT(6) > #define ST7735R_MV BIT(5) > +#define ST7735R_RGB BIT(3) > + > +struct st7735r_cfg { > + const struct drm_display_mode mode; > + unsigned int left_offset; > + unsigned int top_offset; > + unsigned int write_only:1; > + unsigned int rgb:1; /* RGB (vs. BGR) */ > +}; > + > +struct st7735r_priv { > + struct mipi_dbi_dev dbidev; /* Must be first for .release() */ > + unsigned int rgb:1; > +}; The structs here uses "st7735r" as the generic prefix. But the rest of this file uses "jd_t18003_t01" as the generic prefix. It would help readability if the same prefix is used for the common stuff everywhere. struct st7735r_priv includes "rgb" which is copied from struct st7735r_cfg. Maybe just add a const pointer to struct st7735r_cfg, so when we later add more configuration items we do not need to have two copies. And then ofc drop st7735r_priv.rgb. > > static void jd_t18003_t01_pipe_enable(struct drm_simple_display_pipe *pipe, > struct drm_crtc_state *crtc_state, > struct drm_plane_state *plane_state) > { > struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev); > + struct st7735r_priv *priv = container_of(dbidev, struct st7735r_priv, > + dbidev); > struct mipi_dbi *dbi = &dbidev->dbi; > int ret, idx; > u8 addr_mode; > @@ -87,6 +105,10 @@ static void jd_t18003_t01_pipe_enable(struct drm_simple_display_pipe *pipe, > addr_mode = ST7735R_MY | ST7735R_MV; > break; > } > + > + if (priv->rgb) > + addr_mode |= ST7735R_RGB; > + > mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode); > mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT, > MIPI_DCS_PIXEL_FMT_16BIT); > @@ -116,8 +138,17 @@ static const struct drm_simple_display_pipe_funcs jd_t18003_t01_pipe_funcs = { > .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb, > }; > > -static const struct drm_display_mode jd_t18003_t01_mode = { > - DRM_SIMPLE_MODE(128, 160, 28, 35), > +static const struct st7735r_cfg jd_t18003_t01_cfg = { > + .mode = { DRM_SIMPLE_MODE(128, 160, 28, 35) }, > + /* Cannot read from Adafruit 1.8" display via SPI */ > + .write_only = true, > +}; > + > +static const struct st7735r_cfg rh128128t_cfg = { > + .mode = { DRM_SIMPLE_MODE(128, 128, 25, 26) }, > + .left_offset = 2, > + .top_offset = 3, > + .rgb = true, > }; > > DEFINE_DRM_GEM_CMA_FOPS(st7735r_fops); > @@ -136,13 +167,14 @@ static struct drm_driver st7735r_driver = { > }; > > static const struct of_device_id st7735r_of_match[] = { > - { .compatible = "jianda,jd-t18003-t01" }, > + { .compatible = "jianda,jd-t18003-t01", .data = &jd_t18003_t01_cfg }, > + { .compatible = "okaya,rh128128t", .data = &rh128128t_cfg }, > { }, { /* sentinel }, Also - which is not a new thing - this fails to check that we have the correct combination of two compatibles. From the binding: Must be one of the following combinations: - "jianda,jd-t18003-t01", "sitronix,st7735r" - "okaya,rh128128t", "sitronix,st7715r" > }; > MODULE_DEVICE_TABLE(of, st7735r_of_match); > > static const struct spi_device_id st7735r_id[] = { > - { "jd-t18003-t01", 0 }, > + { "jd-t18003-t01", (uintptr_t)&jd_t18003_t01_cfg }, > { }, { /* sentinel */ }, Do we need an entry for "okaya,rh128128t" here? Note: I have not fully understood how MODULE_DEVICE_TABLE() works - so forgive me my ignorance. > }; > MODULE_DEVICE_TABLE(spi, st7735r_id); > @@ -150,17 +182,26 @@ MODULE_DEVICE_TABLE(spi, st7735r_id); > static int st7735r_probe(struct spi_device *spi) > { > struct device *dev = &spi->dev; > + const struct st7735r_cfg *cfg; > struct mipi_dbi_dev *dbidev; > + struct st7735r_priv *priv; > struct drm_device *drm; > struct mipi_dbi *dbi; > struct gpio_desc *dc; > u32 rotation = 0; > int ret; > > - dbidev = kzalloc(sizeof(*dbidev), GFP_KERNEL); > - if (!dbidev) > + cfg = of_device_get_match_data(&spi->dev); > + if (!cfg) > + cfg = (void *)spi_get_device_id(spi)->driver_data; > + > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > + if (!priv) > return -ENOMEM; > > + dbidev = &priv->dbidev; > + priv->rgb = cfg->rgb; > + > dbi = &dbidev->dbi; > drm = &dbidev->drm; > ret = devm_drm_dev_init(dev, drm, &st7735r_driver); > @@ -193,10 +234,14 @@ static int st7735r_probe(struct spi_device *spi) > if (ret) > return ret; > > - /* Cannot read from Adafruit 1.8" display via SPI */ > - dbi->read_commands = NULL; > + if (cfg->write_only) > + dbi->read_commands = NULL; > + > + dbidev->left_offset = cfg->left_offset; > + dbidev->top_offset = cfg->top_offset; > > - ret = mipi_dbi_dev_init(dbidev, &jd_t18003_t01_pipe_funcs, &jd_t18003_t01_mode, rotation); > + ret = mipi_dbi_dev_init(dbidev, &jd_t18003_t01_pipe_funcs, &cfg->mode, > + rotation); > if (ret) > return ret; > > -- > 2.17.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Sam, On Sun, Jan 5, 2020 at 10:13 AM Sam Ravnborg <sam@ravnborg.org> wrote: > Good to see we add more functionality to the smallest driver in DRM. > The patch triggered a few comments - see below. > Some comments relates to the original driver - and not your changes. Thanks for your comments! > On Thu, Jan 02, 2020 at 03:12:46PM +0100, Geert Uytterhoeven wrote: > > Add support for the Okaya RH128128T display to the st7735r driver. > > > > The RH128128T is a 128x128 1.44" TFT display driven by a Sitronix > > ST7715R TFT Controller/Driver. The latter is very similar to the > > ST7735R, and can be handled by the existing st7735r driver. > > As a general comment - it would have eased review if this was split > in two patches. > One patch to introduce the infrastructure to deal with another set of > controller/display and one patch introducing the new combination. I had thought about that, but didn't pursue as the new combination is just 7 added lines. If you prefer a split, I can do that. > > --- a/drivers/gpu/drm/tiny/st7735r.c > > +++ b/drivers/gpu/drm/tiny/st7735r.c > > @@ -1,8 +1,9 @@ > > // SPDX-License-Identifier: GPL-2.0+ > > /* > > - * DRM driver for Sitronix ST7735R panels > > + * DRM driver for Sitronix ST7715R/ST7735R panels > > This comment could describe the situation a little better. > This is a sitronix st7735r controller with a jianda jd-t18003-t01 > display. > Or a sitronix st7715r controller with a okaya rh128128t display. Indeed. It is currently limited to two controller/display combos. But I expect more combos to be added over time. Hence does it make sense to describe all of that in the top comments? > > @@ -37,12 +39,28 @@ > > #define ST7735R_MY BIT(7) > > #define ST7735R_MX BIT(6) > > #define ST7735R_MV BIT(5) > > +#define ST7735R_RGB BIT(3) > > + > > +struct st7735r_cfg { > > + const struct drm_display_mode mode; > > + unsigned int left_offset; > > + unsigned int top_offset; > > + unsigned int write_only:1; > > + unsigned int rgb:1; /* RGB (vs. BGR) */ > > +}; > > + > > +struct st7735r_priv { > > + struct mipi_dbi_dev dbidev; /* Must be first for .release() */ > > + unsigned int rgb:1; > > +}; > > The structs here uses "st7735r" as the generic prefix. > But the rest of this file uses "jd_t18003_t01" as the generic prefix. > > It would help readability if the same prefix is used for the common > stuff everywhere. Agreed. So I think it makes most sense to rename jd_t18003_t01_pipe_{enable,funcs} to sh7735r_pipe_{enable,funcs}? If needed, the display-specific parts (e.g. gamma parameters) could be factored out in st7735r_cfg later, if neeeded. > struct st7735r_priv includes "rgb" which is copied from struct > st7735r_cfg. > Maybe just add a const pointer to struct st7735r_cfg, > so when we later add more configuration items we do not need to have two > copies. And then ofc drop st7735r_priv.rgb. I thought about that, but didn't do so, as the rgb field is the only parameter used outside the probe function. Can be changed, of course. > > @@ -136,13 +167,14 @@ static struct drm_driver st7735r_driver = { > > }; > > > > static const struct of_device_id st7735r_of_match[] = { > > - { .compatible = "jianda,jd-t18003-t01" }, > > + { .compatible = "jianda,jd-t18003-t01", .data = &jd_t18003_t01_cfg }, > > + { .compatible = "okaya,rh128128t", .data = &rh128128t_cfg }, > > { }, > { /* sentinel }, > > Also - which is not a new thing - this fails to check that we have the > correct combination of two compatibles. > From the binding: > > Must be one of the following combinations: > - "jianda,jd-t18003-t01", "sitronix,st7735r" > - "okaya,rh128128t", "sitronix,st7715r" That will be checked by "make dtbs_check", once I have converted the DT bindings to yaml ;-) In reality, there are lots of different ways to communicate with an ST77[13]5R display controller (3/4-wire SPI, or 6800/8080 bus, ...), and lots of different ways to wire a display to the controller. Ideally, this should be described in DT. As I don't have schematics for the display board, doing so is difficult, and will miss important details, which may lead to DTB ABI compatibility issues later. I understand using these compatible combinations was a pragmatic solution to this problem. > > }; > > MODULE_DEVICE_TABLE(of, st7735r_of_match); > > > > static const struct spi_device_id st7735r_id[] = { > > - { "jd-t18003-t01", 0 }, > > + { "jd-t18003-t01", (uintptr_t)&jd_t18003_t01_cfg }, > > { }, > { /* sentinel */ }, > > Do we need an entry for "okaya,rh128128t" here? > > Note: I have not fully understood how MODULE_DEVICE_TABLE() > works - so forgive me my ignorance. st7735r_id[] is used for matching based on platform device name. Hence an entry is needed only to use the display on non-DT systems. Can be added later, if ever needed. Note that there is a separate MODULE_DEVICE_TABLE() for DT-based matching, so the driver module will be auto-loaded on DT-based systems. Gr{oetje,eeting}s, Geert
Hi Geert. > Thanks for your comments! > > > On Thu, Jan 02, 2020 at 03:12:46PM +0100, Geert Uytterhoeven wrote: > > > Add support for the Okaya RH128128T display to the st7735r driver. > > > > > > The RH128128T is a 128x128 1.44" TFT display driven by a Sitronix > > > ST7715R TFT Controller/Driver. The latter is very similar to the > > > ST7735R, and can be handled by the existing st7735r driver. > > > > As a general comment - it would have eased review if this was split > > in two patches. > > One patch to introduce the infrastructure to deal with another set of > > controller/display and one patch introducing the new combination. > > I had thought about that, but didn't pursue as the new combination is > just 7 added lines. If you prefer a split, I can do that. The good thing with a split is that is shows how little is really required to support a new controller/panel pair. So it would be good if you did so. > > > > --- a/drivers/gpu/drm/tiny/st7735r.c > > > +++ b/drivers/gpu/drm/tiny/st7735r.c > > > @@ -1,8 +1,9 @@ > > > // SPDX-License-Identifier: GPL-2.0+ > > > /* > > > - * DRM driver for Sitronix ST7735R panels > > > + * DRM driver for Sitronix ST7715R/ST7735R panels > > > > This comment could describe the situation a little better. > > This is a sitronix st7735r controller with a jianda jd-t18003-t01 > > display. > > Or a sitronix st7715r controller with a okaya rh128128t display. > > Indeed. It is currently limited to two controller/display combos. > But I expect more combos to be added over time. > Hence does it make sense to describe all of that in the top comments? If we do describe it we should do so as kernel-doc and then wire up the driver somwhere under Documentation/gpu/ But there is no good place to wire it up yet. > > > @@ -37,12 +39,28 @@ > > > #define ST7735R_MY BIT(7) > > > #define ST7735R_MX BIT(6) > > > #define ST7735R_MV BIT(5) > > > +#define ST7735R_RGB BIT(3) > > > + > > > +struct st7735r_cfg { > > > + const struct drm_display_mode mode; > > > + unsigned int left_offset; > > > + unsigned int top_offset; > > > + unsigned int write_only:1; > > > + unsigned int rgb:1; /* RGB (vs. BGR) */ > > > +}; > > > + > > > +struct st7735r_priv { > > > + struct mipi_dbi_dev dbidev; /* Must be first for .release() */ > > > + unsigned int rgb:1; > > > +}; > > > > The structs here uses "st7735r" as the generic prefix. > > But the rest of this file uses "jd_t18003_t01" as the generic prefix. > > > > It would help readability if the same prefix is used for the common > > stuff everywhere. > > Agreed. > So I think it makes most sense to rename jd_t18003_t01_pipe_{enable,funcs} > to sh7735r_pipe_{enable,funcs}? s/sh/st/ Yeah, or maybe just sitronix_pipe_{enable,funcs} as we have support for more than one Sitronix controller anyway. > If needed, the display-specific parts (e.g. gamma parameters) could be > factored out in st7735r_cfg later, if neeeded. > > In reality, there are lots of different ways to communicate with an > ST77[13]5R display controller (3/4-wire SPI, or 6800/8080 bus, ...), and > lots of different ways to wire a display to the controller. Ideally, > this should be described in DT. As I don't have schematics for the > display board, doing so is difficult, and will miss important details, > which may lead to DTB ABI compatibility issues later. > I understand using these compatible combinations was a pragmatic solution > to this problem. Agreed, let's bite that when we have a display we care enough about and maybe then we also can write a proper driver for it. I have a few displays here using 8080 and I hope someone steps up to do proper support for such displays. > > > > > > static const struct spi_device_id st7735r_id[] = { > > > - { "jd-t18003-t01", 0 }, > > > + { "jd-t18003-t01", (uintptr_t)&jd_t18003_t01_cfg }, > > > { }, > > { /* sentinel */ }, > > > > Do we need an entry for "okaya,rh128128t" here? > > > > Note: I have not fully understood how MODULE_DEVICE_TABLE() > > works - so forgive me my ignorance. > > st7735r_id[] is used for matching based on platform device name. > Hence an entry is needed only to use the display on non-DT systems. > Can be added later, if ever needed. > > Note that there is a separate MODULE_DEVICE_TABLE() for DT-based matching, > so the driver module will be auto-loaded on DT-based systems. Thanks for the explanation. Sam
On 1/6/20 3:28 AM, Geert Uytterhoeven wrote: > Hi Sam, > > On Sun, Jan 5, 2020 at 10:13 AM Sam Ravnborg <sam@ravnborg.org> wrote: >> Good to see we add more functionality to the smallest driver in DRM. >> The patch triggered a few comments - see below. >> Some comments relates to the original driver - and not your changes. > > Thanks for your comments! > >> On Thu, Jan 02, 2020 at 03:12:46PM +0100, Geert Uytterhoeven wrote: >>> Add support for the Okaya RH128128T display to the st7735r driver. >>> >>> The RH128128T is a 128x128 1.44" TFT display driven by a Sitronix >>> ST7715R TFT Controller/Driver. The latter is very similar to the >>> ST7735R, and can be handled by the existing st7735r driver. >> >> As a general comment - it would have eased review if this was split >> in two patches. >> One patch to introduce the infrastructure to deal with another set of >> controller/display and one patch introducing the new combination. > > I had thought about that, but didn't pursue as the new combination is > just 7 added lines. If you prefer a split, I can do that. > >>> --- a/drivers/gpu/drm/tiny/st7735r.c >>> +++ b/drivers/gpu/drm/tiny/st7735r.c >>> @@ -1,8 +1,9 @@ >>> // SPDX-License-Identifier: GPL-2.0+ >>> /* >>> - * DRM driver for Sitronix ST7735R panels >>> + * DRM driver for Sitronix ST7715R/ST7735R panels >> >> This comment could describe the situation a little better. >> This is a sitronix st7735r controller with a jianda jd-t18003-t01 >> display. >> Or a sitronix st7715r controller with a okaya rh128128t display. > > Indeed. It is currently limited to two controller/display combos. > But I expect more combos to be added over time. > Hence does it make sense to describe all of that in the top comments? > >>> @@ -37,12 +39,28 @@ >>> #define ST7735R_MY BIT(7) >>> #define ST7735R_MX BIT(6) >>> #define ST7735R_MV BIT(5) >>> +#define ST7735R_RGB BIT(3) >>> + >>> +struct st7735r_cfg { >>> + const struct drm_display_mode mode; >>> + unsigned int left_offset; >>> + unsigned int top_offset; >>> + unsigned int write_only:1; >>> + unsigned int rgb:1; /* RGB (vs. BGR) */ >>> +}; >>> + >>> +struct st7735r_priv { >>> + struct mipi_dbi_dev dbidev; /* Must be first for .release() */ >>> + unsigned int rgb:1; >>> +}; >> >> The structs here uses "st7735r" as the generic prefix. >> But the rest of this file uses "jd_t18003_t01" as the generic prefix. >> >> It would help readability if the same prefix is used for the common >> stuff everywhere. > > Agreed. > So I think it makes most sense to rename jd_t18003_t01_pipe_{enable,funcs} > to sh7735r_pipe_{enable,funcs}? > If needed, the display-specific parts (e.g. gamma parameters) could be > factored out in st7735r_cfg later, if neeeded. IIRC, the original intention here is that functions/structs with the jd_t18003_t01_ prefix are specific to the panel, not the controller. E.g. things like power settings and gamma curves. The idea is that it is much easier to write and understand the init sequence as a function rather than trying to make a generic function that can parse a any possible init sequence from a data structure. This new panel really has all of the same settings as the existing one? Having a separate pipe enable function for the new panel would also eliminate the need for the extra private rgb data.
Hi Sam, On Mon, Jan 6, 2020 at 6:08 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > > On Thu, Jan 02, 2020 at 03:12:46PM +0100, Geert Uytterhoeven wrote: > > > > Add support for the Okaya RH128128T display to the st7735r driver. > > > > > > > > The RH128128T is a 128x128 1.44" TFT display driven by a Sitronix > > > > ST7715R TFT Controller/Driver. The latter is very similar to the > > > > ST7735R, and can be handled by the existing st7735r driver. > > > > > > As a general comment - it would have eased review if this was split > > > in two patches. > > > One patch to introduce the infrastructure to deal with another set of > > > controller/display and one patch introducing the new combination. > > > > I had thought about that, but didn't pursue as the new combination is > > just 7 added lines. If you prefer a split, I can do that. > > The good thing with a split is that is shows how little is really > required to support a new controller/panel pair. > So it would be good if you did so. OK. > > > > --- a/drivers/gpu/drm/tiny/st7735r.c > > > > +++ b/drivers/gpu/drm/tiny/st7735r.c > > > > @@ -1,8 +1,9 @@ > > > > // SPDX-License-Identifier: GPL-2.0+ > > > > /* > > > > - * DRM driver for Sitronix ST7735R panels > > > > + * DRM driver for Sitronix ST7715R/ST7735R panels > > > > > > This comment could describe the situation a little better. > > > This is a sitronix st7735r controller with a jianda jd-t18003-t01 > > > display. > > > Or a sitronix st7715r controller with a okaya rh128128t display. > > > > Indeed. It is currently limited to two controller/display combos. > > But I expect more combos to be added over time. > > Hence does it make sense to describe all of that in the top comments? > > If we do describe it we should do so as kernel-doc and then wire up the > driver somwhere under Documentation/gpu/ > But there is no good place to wire it up yet. Documentation/devicetree/bindings/display/sitronix,st7735r.yaml? ;-) > > > > @@ -37,12 +39,28 @@ > > > > #define ST7735R_MY BIT(7) > > > > #define ST7735R_MX BIT(6) > > > > #define ST7735R_MV BIT(5) > > > > +#define ST7735R_RGB BIT(3) > > > > + > > > > +struct st7735r_cfg { > > > > + const struct drm_display_mode mode; > > > > + unsigned int left_offset; > > > > + unsigned int top_offset; > > > > + unsigned int write_only:1; > > > > + unsigned int rgb:1; /* RGB (vs. BGR) */ > > > > +}; > > > > + > > > > +struct st7735r_priv { > > > > + struct mipi_dbi_dev dbidev; /* Must be first for .release() */ > > > > + unsigned int rgb:1; > > > > +}; > > > > > > The structs here uses "st7735r" as the generic prefix. > > > But the rest of this file uses "jd_t18003_t01" as the generic prefix. > > > > > > It would help readability if the same prefix is used for the common > > > stuff everywhere. > > > > Agreed. > > So I think it makes most sense to rename jd_t18003_t01_pipe_{enable,funcs} > > to sh7735r_pipe_{enable,funcs}? > s/sh/st/ Oops, seen to much SuperH-derived hardware ;-) > Yeah, or maybe just sitronix_pipe_{enable,funcs} as we have support > for more than one Sitronix controller anyway. Note that there are other drivers for Sitronix controllers, e.g. drivers/gpu/drm/tiny/st7586.c, which is a different beast. So st7735r_* sounds better to me ('15 and '35 are very similar). 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
Hi David, On Mon, Jan 6, 2020 at 6:12 PM David Lechner <david@lechnology.com> wrote: > On 1/6/20 3:28 AM, Geert Uytterhoeven wrote: > > On Sun, Jan 5, 2020 at 10:13 AM Sam Ravnborg <sam@ravnborg.org> wrote: > >> On Thu, Jan 02, 2020 at 03:12:46PM +0100, Geert Uytterhoeven wrote: > >>> Add support for the Okaya RH128128T display to the st7735r driver. > >>> > >>> The RH128128T is a 128x128 1.44" TFT display driven by a Sitronix > >>> ST7715R TFT Controller/Driver. The latter is very similar to the > >>> ST7735R, and can be handled by the existing st7735r driver. > >>> --- a/drivers/gpu/drm/tiny/st7735r.c > >>> +++ b/drivers/gpu/drm/tiny/st7735r.c > >>> @@ -37,12 +39,28 @@ > >>> #define ST7735R_MY BIT(7) > >>> #define ST7735R_MX BIT(6) > >>> #define ST7735R_MV BIT(5) > >>> +#define ST7735R_RGB BIT(3) > >>> + > >>> +struct st7735r_cfg { > >>> + const struct drm_display_mode mode; > >>> + unsigned int left_offset; > >>> + unsigned int top_offset; > >>> + unsigned int write_only:1; > >>> + unsigned int rgb:1; /* RGB (vs. BGR) */ > >>> +}; > >>> + > >>> +struct st7735r_priv { > >>> + struct mipi_dbi_dev dbidev; /* Must be first for .release() */ > >>> + unsigned int rgb:1; > >>> +}; > >> > >> The structs here uses "st7735r" as the generic prefix. > >> But the rest of this file uses "jd_t18003_t01" as the generic prefix. > >> > >> It would help readability if the same prefix is used for the common > >> stuff everywhere. > > > > Agreed. > > So I think it makes most sense to rename jd_t18003_t01_pipe_{enable,funcs} > > to sh7735r_pipe_{enable,funcs}? > > If needed, the display-specific parts (e.g. gamma parameters) could be > > factored out in st7735r_cfg later, if neeeded. > > IIRC, the original intention here is that functions/structs with the > jd_t18003_t01_ prefix are specific to the panel, not the controller. Makes sense. > E.g. things like power settings and gamma curves. > > The idea is that it is much easier to write and understand the init sequence > as a function rather than trying to make a generic function that can parse > a any possible init sequence from a data structure. I believe the init sequence is the same. The init parameters may not be. What happened to the separation of code and data? ;-) > This new panel really has all of the same settings as the existing one? I went through all the ST77[13]5R-specific register settings in the pipe enable function. All these registers exist on both ST7715R and ST7735R. Unfortunately the Okaya display documentation doesn't give any clues w.r.t. the expected values to program. However, my display seems to work fine. Even the grayscale bands from fbtest test006 look good, so the gamma parameters must be correct ;-) > Having a separate pipe enable function for the new panel would also eliminate > the need for the extra private rgb data. At the expense of duplicating the whole function, for one single bit of difference... P.S. Note that I'm using this on an RSK+RZA1 board with 32 MiB of SDRAM. Enabling support for this display increases kernel size by 316 KiB. And apparently most real world users of the RZ/A1 SoC are not even using SDRAM, but doing XIP with the builtin 10 MiB of SRAM... Thanks! Gr{oetje,eeting}s, Geert
diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c index 3f4487c716848cf8..05d162e76d8481e5 100644 --- a/drivers/gpu/drm/tiny/st7735r.c +++ b/drivers/gpu/drm/tiny/st7735r.c @@ -1,8 +1,9 @@ // SPDX-License-Identifier: GPL-2.0+ /* - * DRM driver for Sitronix ST7735R panels + * DRM driver for Sitronix ST7715R/ST7735R panels * * Copyright 2017 David Lechner <david@lechnology.com> + * Copyright (C) 2019 Glider bvba */ #include <linux/backlight.h> @@ -10,6 +11,7 @@ #include <linux/dma-buf.h> #include <linux/gpio/consumer.h> #include <linux/module.h> +#include <linux/of_device.h> #include <linux/property.h> #include <linux/spi/spi.h> #include <video/mipi_display.h> @@ -37,12 +39,28 @@ #define ST7735R_MY BIT(7) #define ST7735R_MX BIT(6) #define ST7735R_MV BIT(5) +#define ST7735R_RGB BIT(3) + +struct st7735r_cfg { + const struct drm_display_mode mode; + unsigned int left_offset; + unsigned int top_offset; + unsigned int write_only:1; + unsigned int rgb:1; /* RGB (vs. BGR) */ +}; + +struct st7735r_priv { + struct mipi_dbi_dev dbidev; /* Must be first for .release() */ + unsigned int rgb:1; +}; static void jd_t18003_t01_pipe_enable(struct drm_simple_display_pipe *pipe, struct drm_crtc_state *crtc_state, struct drm_plane_state *plane_state) { struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev); + struct st7735r_priv *priv = container_of(dbidev, struct st7735r_priv, + dbidev); struct mipi_dbi *dbi = &dbidev->dbi; int ret, idx; u8 addr_mode; @@ -87,6 +105,10 @@ static void jd_t18003_t01_pipe_enable(struct drm_simple_display_pipe *pipe, addr_mode = ST7735R_MY | ST7735R_MV; break; } + + if (priv->rgb) + addr_mode |= ST7735R_RGB; + mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode); mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT, MIPI_DCS_PIXEL_FMT_16BIT); @@ -116,8 +138,17 @@ static const struct drm_simple_display_pipe_funcs jd_t18003_t01_pipe_funcs = { .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb, }; -static const struct drm_display_mode jd_t18003_t01_mode = { - DRM_SIMPLE_MODE(128, 160, 28, 35), +static const struct st7735r_cfg jd_t18003_t01_cfg = { + .mode = { DRM_SIMPLE_MODE(128, 160, 28, 35) }, + /* Cannot read from Adafruit 1.8" display via SPI */ + .write_only = true, +}; + +static const struct st7735r_cfg rh128128t_cfg = { + .mode = { DRM_SIMPLE_MODE(128, 128, 25, 26) }, + .left_offset = 2, + .top_offset = 3, + .rgb = true, }; DEFINE_DRM_GEM_CMA_FOPS(st7735r_fops); @@ -136,13 +167,14 @@ static struct drm_driver st7735r_driver = { }; static const struct of_device_id st7735r_of_match[] = { - { .compatible = "jianda,jd-t18003-t01" }, + { .compatible = "jianda,jd-t18003-t01", .data = &jd_t18003_t01_cfg }, + { .compatible = "okaya,rh128128t", .data = &rh128128t_cfg }, { }, }; MODULE_DEVICE_TABLE(of, st7735r_of_match); static const struct spi_device_id st7735r_id[] = { - { "jd-t18003-t01", 0 }, + { "jd-t18003-t01", (uintptr_t)&jd_t18003_t01_cfg }, { }, }; MODULE_DEVICE_TABLE(spi, st7735r_id); @@ -150,17 +182,26 @@ MODULE_DEVICE_TABLE(spi, st7735r_id); static int st7735r_probe(struct spi_device *spi) { struct device *dev = &spi->dev; + const struct st7735r_cfg *cfg; struct mipi_dbi_dev *dbidev; + struct st7735r_priv *priv; struct drm_device *drm; struct mipi_dbi *dbi; struct gpio_desc *dc; u32 rotation = 0; int ret; - dbidev = kzalloc(sizeof(*dbidev), GFP_KERNEL); - if (!dbidev) + cfg = of_device_get_match_data(&spi->dev); + if (!cfg) + cfg = (void *)spi_get_device_id(spi)->driver_data; + + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (!priv) return -ENOMEM; + dbidev = &priv->dbidev; + priv->rgb = cfg->rgb; + dbi = &dbidev->dbi; drm = &dbidev->drm; ret = devm_drm_dev_init(dev, drm, &st7735r_driver); @@ -193,10 +234,14 @@ static int st7735r_probe(struct spi_device *spi) if (ret) return ret; - /* Cannot read from Adafruit 1.8" display via SPI */ - dbi->read_commands = NULL; + if (cfg->write_only) + dbi->read_commands = NULL; + + dbidev->left_offset = cfg->left_offset; + dbidev->top_offset = cfg->top_offset; - ret = mipi_dbi_dev_init(dbidev, &jd_t18003_t01_pipe_funcs, &jd_t18003_t01_mode, rotation); + ret = mipi_dbi_dev_init(dbidev, &jd_t18003_t01_pipe_funcs, &cfg->mode, + rotation); if (ret) return ret;
Add support for the Okaya RH128128T display to the st7735r driver. The RH128128T is a 128x128 1.44" TFT display driven by a Sitronix ST7715R TFT Controller/Driver. The latter is very similar to the ST7735R, and can be handled by the existing st7735r driver. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/gpu/drm/tiny/st7735r.c | 65 ++++++++++++++++++++++++++++------ 1 file changed, 55 insertions(+), 10 deletions(-)