| Message ID | 20190707181937.6250-16-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/omap: Replace custom display drivers with drm_bridge and drm_panel | expand |
Hi Laurent. Good to move omapdrm to a more standard way to do things. > new file mode 100644 > index 000000000000..d8a8c3a3a8c5 > --- /dev/null > +++ b/drivers/gpu/drm/panel/panel-lg-lb035q02.c > @@ -0,0 +1,235 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * LG.Philips LB035Q02 LCD Panel Driver Looks like a typo. As far as I know LG and Philips are not the same. But I can see this is used in several places, so I need to check up on actual status here and driver is likely OK. Google... this is fine. Some joint venture in 2001. > + * Based on the omapdrm-specific panel-lg-lb035q02 driver Will we have two drivers with the same name, or are this above already disabled from the build? > + unsigned int i; index to arrays are default "int" IIRC. Not that it matters but noticed it. > + int ret; > + > + for (i = 0; i < ARRAY_SIZE(init_data); ++i) { > + ret = lb035q02_write(lcd, init_data[i].index, > + init_data[i].value); > + if (ret < 0) > + return ret; > + } > + > + return 0; > +} > + > +static const struct drm_display_mode lb035q02_mode = { > + .clock = 6500, > + .hdisplay = 320, > + .hsync_start = 320 + 20, > + .hsync_end = 320 + 20 + 2, > + .htotal = 320 + 20 + 2 + 68, > + .vdisplay = 240, > + .vsync_start = 240 + 4, > + .vsync_end = 240 + 4 + 2, > + .vtotal = 240 + 4 + 2 + 18, > + .vrefresh = 60, > + .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED, > + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC, > +}; We already specify all the timing details. Consider to use display_mode to specify the width/height too. So the panel specificatiosn are hardcoded only in one place. > + > +static int lb035q02_get_modes(struct drm_panel *panel) > +{ > + struct drm_connector *connector = panel->connector; > + struct drm_display_mode *mode; > + > + mode = drm_mode_duplicate(panel->drm, &lb035q02_mode); > + if (!mode) > + return -ENOMEM; > + > + drm_mode_set_name(mode); > + drm_mode_probed_add(connector, mode); > + > + connector->display_info.width_mm = 70; > + connector->display_info.height_mm = 53; So we avoid hardcoding height/width here, but do it with the timing above. > + /* > + * FIXME: According to the datasheet pixel data is sampled on the > + * rising edge of the clock, but the code running on the Gumstix Overo > + * Palo35 indicates sampling on the negative edge. This should be > + * tested on a real device. > + */ > + connector->display_info.bus_flags = DRM_BUS_FLAG_DE_HIGH > + | DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE > + | DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE; > + > + return 1; > +} > + > +static const struct drm_panel_funcs lb035q02_funcs = { > + .disable = lb035q02_disable, > + .enable = lb035q02_enable, > + .get_modes = lb035q02_get_modes, > +}; > + > +static int lb035q02_probe(struct spi_device *spi) > +{ > + struct lb035q02_device *lcd; > + int ret; > + > + lcd = devm_kzalloc(&spi->dev, sizeof(*lcd), GFP_KERNEL); > + if (lcd == NULL) > + return -ENOMEM; > + > + spi_set_drvdata(spi, lcd); > + lcd->spi = spi; > + > + lcd->enable_gpio = devm_gpiod_get(&spi->dev, "enable", GPIOD_OUT_LOW); > + if (IS_ERR(lcd->enable_gpio)) { > + dev_err(&spi->dev, "failed to parse enable gpio\n"); > + return PTR_ERR(lcd->enable_gpio); > + } > + > + ret = lb035q02_init(lcd); > + if (ret < 0) > + return ret; > + > + drm_panel_init(&lcd->panel); > + lcd->panel.dev = &lcd->spi->dev; > + lcd->panel.funcs = &lb035q02_funcs; > + > + return drm_panel_add(&lcd->panel); > +} > + > +static int lb035q02_remove(struct spi_device *spi) > +{ > + struct lb035q02_device *lcd = spi_get_drvdata(spi); > + > + drm_panel_remove(&lcd->panel); > + lb035q02_disable(&lcd->panel); Use drm_panel_disable() so the driver will benefit when we move more functionality to the drm_panel_disable() function. > + > + return 0; > +} > + > +static const struct of_device_id lb035q02_of_match[] = { > + { .compatible = "lgphilips,lb035q02", }, > + {}, Some drivers use { /* sentinel */ }, to document this is on purpose the last entry. > +}; > + > +MODULE_DEVICE_TABLE(of, lb035q02_of_match); > + > +static struct spi_driver lb035q02_driver = { > + .probe = lb035q02_probe, > + .remove = lb035q02_remove, > + .driver = { > + .name = "panel-lg-lb035q02", > + .of_match_table = lb035q02_of_match, > + }, > +}; > + > +module_spi_driver(lb035q02_driver); > + > +MODULE_ALIAS("spi:lgphilips,lb035q02"); > +MODULE_AUTHOR("Tomi Valkeinen <tomi.valkeinen@ti.com>"); > +MODULE_DESCRIPTION("LG.Philips LB035Q02 LCD Panel driver"); > +MODULE_LICENSE("GPL"); This should be "GPL v2" if I read https://www.kernel.org/doc/html/latest/process/license-rules.html correct. See "MODULE_LICENSE" table. With the above comments addressed/considered: Reviewed-by: Sam Ravnborg <sam@ravnborg.org> Sam
Hi Sam, On Mon, Jul 08, 2019 at 08:51:29PM +0200, Sam Ravnborg wrote: > Hi Laurent. > > Good to move omapdrm to a more standard way to do things. I hope it will help defining the next step for the standard ;-) > > new file mode 100644 > > index 000000000000..d8a8c3a3a8c5 > > --- /dev/null > > +++ b/drivers/gpu/drm/panel/panel-lg-lb035q02.c > > @@ -0,0 +1,235 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * LG.Philips LB035Q02 LCD Panel Driver > > Looks like a typo. As far as I know LG and Philips are not the same. > But I can see this is used in several places, so I need to check up on > actual status here and driver is likely OK. > Google... this is fine. Some joint venture in 2001. > > > + * Based on the omapdrm-specific panel-lg-lb035q02 driver > > Will we have two drivers with the same name, or are this above already > disabled from the build? The omapdrm-specific driver is called panel-lgphilips-lb035q02.c. I'll update the comment. > > + unsigned int i; > > index to arrays are default "int" IIRC. > Not that it matters but noticed it. Are they ? I've always advocated for unsigned indexes to use unsigned int. > > + int ret; > > + > > + for (i = 0; i < ARRAY_SIZE(init_data); ++i) { > > + ret = lb035q02_write(lcd, init_data[i].index, > > + init_data[i].value); > > + if (ret < 0) > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static const struct drm_display_mode lb035q02_mode = { > > + .clock = 6500, > > + .hdisplay = 320, > > + .hsync_start = 320 + 20, > > + .hsync_end = 320 + 20 + 2, > > + .htotal = 320 + 20 + 2 + 68, > > + .vdisplay = 240, > > + .vsync_start = 240 + 4, > > + .vsync_end = 240 + 4 + 2, > > + .vtotal = 240 + 4 + 2 + 18, > > + .vrefresh = 60, > > + .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED, > > + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC, > > +}; > > We already specify all the timing details. > Consider to use display_mode to specify the width/height too. > So the panel specificatiosn are hardcoded only in one place. I didn't know drm_display_mode had width_mm and height_mm fields. I'll fix this. > > + > > +static int lb035q02_get_modes(struct drm_panel *panel) > > +{ > > + struct drm_connector *connector = panel->connector; > > + struct drm_display_mode *mode; > > + > > + mode = drm_mode_duplicate(panel->drm, &lb035q02_mode); > > + if (!mode) > > + return -ENOMEM; > > + > > + drm_mode_set_name(mode); > > + drm_mode_probed_add(connector, mode); > > + > > + connector->display_info.width_mm = 70; > > + connector->display_info.height_mm = 53; > > So we avoid hardcoding height/width here, but do it with the timing > above. > > > + /* > > + * FIXME: According to the datasheet pixel data is sampled on the > > + * rising edge of the clock, but the code running on the Gumstix Overo > > + * Palo35 indicates sampling on the negative edge. This should be > > + * tested on a real device. > > + */ > > + connector->display_info.bus_flags = DRM_BUS_FLAG_DE_HIGH > > + | DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE > > + | DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE; > > + > > + return 1; > > +} > > + > > +static const struct drm_panel_funcs lb035q02_funcs = { > > + .disable = lb035q02_disable, > > + .enable = lb035q02_enable, > > + .get_modes = lb035q02_get_modes, > > +}; > > + > > +static int lb035q02_probe(struct spi_device *spi) > > +{ > > + struct lb035q02_device *lcd; > > + int ret; > > + > > + lcd = devm_kzalloc(&spi->dev, sizeof(*lcd), GFP_KERNEL); > > + if (lcd == NULL) > > + return -ENOMEM; > > + > > + spi_set_drvdata(spi, lcd); > > + lcd->spi = spi; > > + > > + lcd->enable_gpio = devm_gpiod_get(&spi->dev, "enable", GPIOD_OUT_LOW); > > + if (IS_ERR(lcd->enable_gpio)) { > > + dev_err(&spi->dev, "failed to parse enable gpio\n"); > > + return PTR_ERR(lcd->enable_gpio); > > + } > > + > > + ret = lb035q02_init(lcd); > > + if (ret < 0) > > + return ret; > > + > > + drm_panel_init(&lcd->panel); > > + lcd->panel.dev = &lcd->spi->dev; > > + lcd->panel.funcs = &lb035q02_funcs; > > + > > + return drm_panel_add(&lcd->panel); > > +} > > + > > +static int lb035q02_remove(struct spi_device *spi) > > +{ > > + struct lb035q02_device *lcd = spi_get_drvdata(spi); > > + > > + drm_panel_remove(&lcd->panel); > > + lb035q02_disable(&lcd->panel); > > Use drm_panel_disable() so the driver will benefit when we move more > functionality to the drm_panel_disable() function. Will do. > > + > > + return 0; > > +} > > + > > +static const struct of_device_id lb035q02_of_match[] = { > > + { .compatible = "lgphilips,lb035q02", }, > > + {}, > > Some drivers use { /* sentinel */ }, to document this is on purpose the > last entry. I don't mind either way so I'll change it. > > +}; > > + > > +MODULE_DEVICE_TABLE(of, lb035q02_of_match); > > + > > +static struct spi_driver lb035q02_driver = { > > + .probe = lb035q02_probe, > > + .remove = lb035q02_remove, > > + .driver = { > > + .name = "panel-lg-lb035q02", > > + .of_match_table = lb035q02_of_match, > > + }, > > +}; > > + > > +module_spi_driver(lb035q02_driver); > > + > > +MODULE_ALIAS("spi:lgphilips,lb035q02"); > > +MODULE_AUTHOR("Tomi Valkeinen <tomi.valkeinen@ti.com>"); > > +MODULE_DESCRIPTION("LG.Philips LB035Q02 LCD Panel driver"); > > +MODULE_LICENSE("GPL"); > > This should be "GPL v2" if I read https://www.kernel.org/doc/html/latest/process/license-rules.html > correct. See "MODULE_LICENSE" table. According to that table, "GPL v2" is defined as "Same as “GPL”. It exists for historic reasons.". My understanding is that "GPL v2" exists for historical reasons and should not be used in new code. > With the above comments addressed/considered: > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> Thank you.
Hi Laurent. > > > > + unsigned int i; > > > > index to arrays are default "int" IIRC. > > Not that it matters but noticed it. > > Are they ? I've always advocated for unsigned indexes to use unsigned > int. I did not dig up anything authorative - but found this: https://stackoverflow.com/questions/8111357/type-of-array-index-in-c There is some confusion betwwen the type of array and the type of the index. But the part that looks to answer the questions say that index can be negative, so the integral type is default int. Again, nothing to worry about, as code wokrs and unsigen int is used for index in many places. > > > +MODULE_LICENSE("GPL"); > > > > This should be "GPL v2" if I read https://www.kernel.org/doc/html/latest/process/license-rules.html > > correct. See "MODULE_LICENSE" table. > > According to that table, "GPL v2" is defined as "Same as “GPL”. It > exists for historic reasons.". My understanding is that "GPL v2" exists > for historical reasons and should not be used in new code. Re-reading the link you are right. module license is to be specified as "GPL" and then one has to visit the file. So ignore that comment in following reviews. Seems simple to remember, will keep in mind for future reviews. Sam
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index d9d931aa6e26..1843135cbeb1 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -103,6 +103,13 @@ config DRM_PANEL_SAMSUNG_LD9040 depends on OF && SPI select VIDEOMODE_HELPERS +config DRM_PANEL_LG_LB035Q02 + tristate "LG LB035Q024573 RGB panel" + depends on GPIOLIB && OF && SPI + help + Say Y here if you want to enable support for the LB035Q02 RGB panel. + To compile this driver as a module, choose M here. + config DRM_PANEL_LG_LG4573 tristate "LG4573 RGB/SPI panel" depends on OF && SPI diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index fb0cb3aaa9e6..675b5696c685 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PANEL_ILITEK_ILI9881C) += panel-ilitek-ili9881c.o obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) += panel-innolux-p079zca.o obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD097D04) += panel-kingdisplay-kd097d04.o +obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o obj-$(CONFIG_DRM_PANEL_OLIMEX_LCD_OLINUXINO) += panel-olimex-lcd-olinuxino.o obj-$(CONFIG_DRM_PANEL_ORISETECH_OTM8009A) += panel-orisetech-otm8009a.o diff --git a/drivers/gpu/drm/panel/panel-lg-lb035q02.c b/drivers/gpu/drm/panel/panel-lg-lb035q02.c new file mode 100644 index 000000000000..d8a8c3a3a8c5 --- /dev/null +++ b/drivers/gpu/drm/panel/panel-lg-lb035q02.c @@ -0,0 +1,235 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * LG.Philips LB035Q02 LCD Panel Driver + * + * Copyright (C) 2019 Texas Instruments Incorporated + * + * Based on the omapdrm-specific panel-lg-lb035q02 driver + * + * Copyright (C) 2013 Texas Instruments Incorporated + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com> + * + * Based on a driver by: Steve Sakoman <steve@sakoman.com> + */ + +#include <linux/gpio/consumer.h> +#include <linux/module.h> +#include <linux/spi/spi.h> + +#include <drm/drm_connector.h> +#include <drm/drm_modes.h> +#include <drm/drm_panel.h> + +struct lb035q02_device { + struct drm_panel panel; + + struct spi_device *spi; + struct gpio_desc *enable_gpio; +}; + +#define to_lb035q02_device(p) container_of(p, struct lb035q02_device, panel) + +static int lb035q02_write(struct lb035q02_device *lcd, u16 reg, u16 val) +{ + struct spi_message msg; + struct spi_transfer index_xfer = { + .len = 3, + .cs_change = 1, + }; + struct spi_transfer value_xfer = { + .len = 3, + }; + u8 buffer[16]; + + spi_message_init(&msg); + + /* register index */ + buffer[0] = 0x70; + buffer[1] = 0x00; + buffer[2] = reg & 0x7f; + index_xfer.tx_buf = buffer; + spi_message_add_tail(&index_xfer, &msg); + + /* register value */ + buffer[4] = 0x72; + buffer[5] = val >> 8; + buffer[6] = val; + value_xfer.tx_buf = buffer + 4; + spi_message_add_tail(&value_xfer, &msg); + + return spi_sync(lcd->spi, &msg); +} + +static int lb035q02_init(struct lb035q02_device *lcd) +{ + /* Init sequence from page 28 of the lb035q02 spec. */ + static const struct { + u16 index; + u16 value; + } init_data[] = { + { 0x01, 0x6300 }, + { 0x02, 0x0200 }, + { 0x03, 0x0177 }, + { 0x04, 0x04c7 }, + { 0x05, 0xffc0 }, + { 0x06, 0xe806 }, + { 0x0a, 0x4008 }, + { 0x0b, 0x0000 }, + { 0x0d, 0x0030 }, + { 0x0e, 0x2800 }, + { 0x0f, 0x0000 }, + { 0x16, 0x9f80 }, + { 0x17, 0x0a0f }, + { 0x1e, 0x00c1 }, + { 0x30, 0x0300 }, + { 0x31, 0x0007 }, + { 0x32, 0x0000 }, + { 0x33, 0x0000 }, + { 0x34, 0x0707 }, + { 0x35, 0x0004 }, + { 0x36, 0x0302 }, + { 0x37, 0x0202 }, + { 0x3a, 0x0a0d }, + { 0x3b, 0x0806 }, + }; + + unsigned int i; + int ret; + + for (i = 0; i < ARRAY_SIZE(init_data); ++i) { + ret = lb035q02_write(lcd, init_data[i].index, + init_data[i].value); + if (ret < 0) + return ret; + } + + return 0; +} + +static int lb035q02_disable(struct drm_panel *panel) +{ + struct lb035q02_device *lcd = to_lb035q02_device(panel); + + gpiod_set_value_cansleep(lcd->enable_gpio, 0); + + return 0; +} + +static int lb035q02_enable(struct drm_panel *panel) +{ + struct lb035q02_device *lcd = to_lb035q02_device(panel); + + gpiod_set_value_cansleep(lcd->enable_gpio, 1); + + return 0; +} + +static const struct drm_display_mode lb035q02_mode = { + .clock = 6500, + .hdisplay = 320, + .hsync_start = 320 + 20, + .hsync_end = 320 + 20 + 2, + .htotal = 320 + 20 + 2 + 68, + .vdisplay = 240, + .vsync_start = 240 + 4, + .vsync_end = 240 + 4 + 2, + .vtotal = 240 + 4 + 2 + 18, + .vrefresh = 60, + .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED, + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC, +}; + +static int lb035q02_get_modes(struct drm_panel *panel) +{ + struct drm_connector *connector = panel->connector; + struct drm_display_mode *mode; + + mode = drm_mode_duplicate(panel->drm, &lb035q02_mode); + if (!mode) + return -ENOMEM; + + drm_mode_set_name(mode); + drm_mode_probed_add(connector, mode); + + connector->display_info.width_mm = 70; + connector->display_info.height_mm = 53; + /* + * FIXME: According to the datasheet pixel data is sampled on the + * rising edge of the clock, but the code running on the Gumstix Overo + * Palo35 indicates sampling on the negative edge. This should be + * tested on a real device. + */ + connector->display_info.bus_flags = DRM_BUS_FLAG_DE_HIGH + | DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE + | DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE; + + return 1; +} + +static const struct drm_panel_funcs lb035q02_funcs = { + .disable = lb035q02_disable, + .enable = lb035q02_enable, + .get_modes = lb035q02_get_modes, +}; + +static int lb035q02_probe(struct spi_device *spi) +{ + struct lb035q02_device *lcd; + int ret; + + lcd = devm_kzalloc(&spi->dev, sizeof(*lcd), GFP_KERNEL); + if (lcd == NULL) + return -ENOMEM; + + spi_set_drvdata(spi, lcd); + lcd->spi = spi; + + lcd->enable_gpio = devm_gpiod_get(&spi->dev, "enable", GPIOD_OUT_LOW); + if (IS_ERR(lcd->enable_gpio)) { + dev_err(&spi->dev, "failed to parse enable gpio\n"); + return PTR_ERR(lcd->enable_gpio); + } + + ret = lb035q02_init(lcd); + if (ret < 0) + return ret; + + drm_panel_init(&lcd->panel); + lcd->panel.dev = &lcd->spi->dev; + lcd->panel.funcs = &lb035q02_funcs; + + return drm_panel_add(&lcd->panel); +} + +static int lb035q02_remove(struct spi_device *spi) +{ + struct lb035q02_device *lcd = spi_get_drvdata(spi); + + drm_panel_remove(&lcd->panel); + lb035q02_disable(&lcd->panel); + + return 0; +} + +static const struct of_device_id lb035q02_of_match[] = { + { .compatible = "lgphilips,lb035q02", }, + {}, +}; + +MODULE_DEVICE_TABLE(of, lb035q02_of_match); + +static struct spi_driver lb035q02_driver = { + .probe = lb035q02_probe, + .remove = lb035q02_remove, + .driver = { + .name = "panel-lg-lb035q02", + .of_match_table = lb035q02_of_match, + }, +}; + +module_spi_driver(lb035q02_driver); + +MODULE_ALIAS("spi:lgphilips,lb035q02"); +MODULE_AUTHOR("Tomi Valkeinen <tomi.valkeinen@ti.com>"); +MODULE_DESCRIPTION("LG.Philips LB035Q02 LCD Panel driver"); +MODULE_LICENSE("GPL");
This panel is used on the Gumstix Overo Palo35. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/gpu/drm/panel/Kconfig | 7 + drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-lg-lb035q02.c | 235 ++++++++++++++++++++++ 3 files changed, 243 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-lg-lb035q02.c