Re: [PATCH 21/60] drm/panel: Add driver for the Sharp LS037V7DW01 panel
Hi Sam,
On Mon, Jul 08, 2019 at 09:44:52PM +0200, Sam Ravnborg wrote:
> Hi Laurent.
>
> Third panel driver in line for review.
> Review comments that are duplicates from the first two will have only a
> brief remark - if any.
I'll address them the same way. Please consider all unanswered comments
below as addressed.
> On Sun, Jul 07, 2019 at 09:18:58PM +0300, Laurent Pinchart wrote:
> > This panel is used on the SDP3430.
>
> Add a little more context and put it in Kconfig help.
> Maybe this is the TI board, and maybe it is something else.
>
> > Signed-off-by: Laurent Pinchart
> > ---
> > drivers/gpu/drm/panel/Kconfig | 7 +
> > drivers/gpu/drm/panel/Makefile| 1 +
> > .../gpu/drm/panel/panel-sharp-ls037v7dw01.c | 231 ++
> > 3 files changed, 239 insertions(+)
> > create mode 100644 drivers/gpu/drm/panel/panel-sharp-ls037v7dw01.c
> >
> > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> > index da613c04b835..04fd152efe4c 100644
> > --- a/drivers/gpu/drm/panel/Kconfig
> > +++ b/drivers/gpu/drm/panel/Kconfig
> > @@ -271,6 +271,13 @@ config DRM_PANEL_SHARP_LS043T1LE01
> > Say Y here if you want to enable support for Sharp LS043T1LE01 qHD
> > (540x960) DSI panel as found on the Qualcomm APQ8074 Dragonboard
> >
> > +config DRM_PANEL_SHARP_LS037V7DW01
> > + tristate "Sharp LS037V7DW01 VGA LCD panel"
> > + depends on GPIOLIB && OF && REGULATOR
> > + help
> > + Say Y here if you want to enable support for Sharp LS037V7DW01 VGA
> > + (480x640) LCD panel.
> > +
>
> Alphabetical order, so it comes before DRM_PANEL_SHARP_LS043T1LE01
>
> > config DRM_PANEL_SITRONIX_ST7701
> > tristate "Sitronix ST7701 panel driver"
> > depends on OF
> > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> > index e81ed1535024..12dcd76eb87c 100644
> > --- a/drivers/gpu/drm/panel/Makefile
> > +++ b/drivers/gpu/drm/panel/Makefile
> > @@ -27,6 +27,7 @@ obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63M0) +=
> > panel-samsung-s6e63m0.o
> > obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += panel-samsung-s6e8aa0.o
> > obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) += panel-seiko-43wvf1g.o
> > obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
> > +obj-$(CONFIG_DRM_PANEL_SHARP_LS037V7DW01) += panel-sharp-ls037v7dw01.o
> > obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += panel-sharp-ls043t1le01.o
>
> And here it is right.
>
> > --- /dev/null
> > +++ b/drivers/gpu/drm/panel/panel-sharp-ls037v7dw01.c
> > @@ -0,0 +1,231 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Sharp LS037V7DW01 LCD Panel Driver
> > + *
> > + * Copyright (C) 2019 Texas Instruments Incorporated
> > + *
> > + * Based on the omapdrm-specific panel-sharp-ls037v7dw01 driver
> > + *
> > + * Copyright (C) 2013 Texas Instruments Incorporated
> > + * Author: Tomi Valkeinen
>
> Add your copyright?
As with the previous patches, the copyright goes to TI.
> > +struct ls037v7dw01_device {
> > + struct drm_panel panel;
> > + struct platform_device *pdev;
> > +
> > + struct regulator *vcc;
>
> The property is named envdd - should they use the same name?
I'll rename that to vdd.
> > + struct gpio_desc *resb_gpio;/* low = reset active min 20 us */
> > + struct gpio_desc *ini_gpio; /* high = power on */
> > + struct gpio_desc *mo_gpio; /* low = 480x640, high = 240x320 */
> > + struct gpio_desc *lr_gpio; /* high = conventional horizontal
> > scanning */
> > + struct gpio_desc *ud_gpio; /* high = conventional vertical
> > scanning */
> > +};
>
> device versus panel, but bikeshedding, so feel free to ignore.
I'll rename it.
> > +
> > +static int ls037v7dw01_disable(struct drm_panel *panel)
> > +{
> > + struct ls037v7dw01_device *lcd = to_ls037v7dw01_device(panel);
> > +
> > + gpiod_set_value_cansleep(lcd->ini_gpio, 0);
> > + gpiod_set_value_cansleep(lcd->resb_gpio, 0);
> > +
> > + /* Wait at least 5 vsyncs after disabling the LCD. */
> > + msleep(100);
> > +
> > + return 0;
> > +}
> > +
> > +static int ls037v7dw01_unprepare(struct drm_panel *panel)
> > +{
> > + struct ls037v7dw01_device *lcd = to_ls037v7dw01_device(panel);
> > +
> > + if (lcd->vcc)
> > + regulator_disable(lcd->vcc);
>
> Why is the if (lcd-vcc) needed?
> If I read the probe code correct then we either get a regulator or we
> error out.
>
> Same goes for all other checks of lcd->vcc
>
> > +static const struct drm_display_mode ls037v7dw01_mode = {
> > + .clock = 19200,
> > + .hdisplay = 480,
> > + .hsync_start = 480 + 1,
> > + .hsync_end = 480 + 1 + 2,
> > + .htotal = 480 + 1 + 2 + 28,
> > + .vdisplay = 640,
> > + .vsync_start = 640 + 1,
> > + .vsync_end = 640 + 1 + 1,
> > + .vtotal = 640 + 1 + 1 + 1,
> > + .vrefresh = 58,
> > + .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
> > + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> > +};
> > +
> > +static int
Re: [PATCH 21/60] drm/panel: Add driver for the Sharp LS037V7DW01 panel
Hi Laurent.
> > +
> > +MODULE_DEVICE_TABLE(of, ls037v7dw01_of_match);
> > +
> > +static struct platform_driver ls037v7dw01_driver = {
> > + .probe = ls037v7dw01_probe,
> > + .remove = __exit_p(ls037v7dw01_remove),
I hope _exit_p() is not needed.
No other panel drivers use it as far as I could see.
Sam
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 21/60] drm/panel: Add driver for the Sharp LS037V7DW01 panel
Hi Laurent.
Third panel driver in line for review.
Review comments that are duplicates from the first two will have only a
brief remark - if any.
On Sun, Jul 07, 2019 at 09:18:58PM +0300, Laurent Pinchart wrote:
> This panel is used on the SDP3430.
Add a little more context and put it in Kconfig help.
Maybe this is the TI board, and maybe it is something else.
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/gpu/drm/panel/Kconfig | 7 +
> drivers/gpu/drm/panel/Makefile| 1 +
> .../gpu/drm/panel/panel-sharp-ls037v7dw01.c | 231 ++
> 3 files changed, 239 insertions(+)
> create mode 100644 drivers/gpu/drm/panel/panel-sharp-ls037v7dw01.c
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index da613c04b835..04fd152efe4c 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -271,6 +271,13 @@ config DRM_PANEL_SHARP_LS043T1LE01
> Say Y here if you want to enable support for Sharp LS043T1LE01 qHD
> (540x960) DSI panel as found on the Qualcomm APQ8074 Dragonboard
>
> +config DRM_PANEL_SHARP_LS037V7DW01
> + tristate "Sharp LS037V7DW01 VGA LCD panel"
> + depends on GPIOLIB && OF && REGULATOR
> + help
> + Say Y here if you want to enable support for Sharp LS037V7DW01 VGA
> + (480x640) LCD panel.
> +
Alphabetical order, so it comes before DRM_PANEL_SHARP_LS043T1LE01
> config DRM_PANEL_SITRONIX_ST7701
> tristate "Sitronix ST7701 panel driver"
> depends on OF
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index e81ed1535024..12dcd76eb87c 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63M0) +=
> panel-samsung-s6e63m0.o
> obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += panel-samsung-s6e8aa0.o
> obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) += panel-seiko-43wvf1g.o
> obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
> +obj-$(CONFIG_DRM_PANEL_SHARP_LS037V7DW01) += panel-sharp-ls037v7dw01.o
> obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += panel-sharp-ls043t1le01.o
And here it is right.
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-sharp-ls037v7dw01.c
> @@ -0,0 +1,231 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Sharp LS037V7DW01 LCD Panel Driver
> + *
> + * Copyright (C) 2019 Texas Instruments Incorporated
> + *
> + * Based on the omapdrm-specific panel-sharp-ls037v7dw01 driver
> + *
> + * Copyright (C) 2013 Texas Instruments Incorporated
> + * Author: Tomi Valkeinen
Add your copyright?
> +struct ls037v7dw01_device {
> + struct drm_panel panel;
> + struct platform_device *pdev;
> +
> + struct regulator *vcc;
The property is named envdd - should they use the same name?
> + struct gpio_desc *resb_gpio;/* low = reset active min 20 us */
> + struct gpio_desc *ini_gpio; /* high = power on */
> + struct gpio_desc *mo_gpio; /* low = 480x640, high = 240x320 */
> + struct gpio_desc *lr_gpio; /* high = conventional horizontal
> scanning */
> + struct gpio_desc *ud_gpio; /* high = conventional vertical
> scanning */
> +};
device versus panel, but bikeshedding, so feel free to ignore.
> +
> +static int ls037v7dw01_disable(struct drm_panel *panel)
> +{
> + struct ls037v7dw01_device *lcd = to_ls037v7dw01_device(panel);
> +
> + gpiod_set_value_cansleep(lcd->ini_gpio, 0);
> + gpiod_set_value_cansleep(lcd->resb_gpio, 0);
> +
> + /* Wait at least 5 vsyncs after disabling the LCD. */
> + msleep(100);
> +
> + return 0;
> +}
> +
> +static int ls037v7dw01_unprepare(struct drm_panel *panel)
> +{
> + struct ls037v7dw01_device *lcd = to_ls037v7dw01_device(panel);
> +
> + if (lcd->vcc)
> + regulator_disable(lcd->vcc);
Why is the if (lcd-vcc) needed?
If I read the probe code correct then we either get a regulator or we
error out.
Same goes for all other checks of lcd->vcc
> +static const struct drm_display_mode ls037v7dw01_mode = {
> + .clock = 19200,
> + .hdisplay = 480,
> + .hsync_start = 480 + 1,
> + .hsync_end = 480 + 1 + 2,
> + .htotal = 480 + 1 + 2 + 28,
> + .vdisplay = 640,
> + .vsync_start = 640 + 1,
> + .vsync_end = 640 + 1 + 1,
> + .vtotal = 640 + 1 + 1 + 1,
> + .vrefresh = 58,
> + .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
> + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> +};
> +
> +static int ls037v7dw01_get_modes(struct drm_panel *panel)
> +{
> + struct drm_connector *connector = panel->connector;
> + struct drm_display_mode *mode;
> +
> + mode = drm_mode_duplicate(panel->drm, _mode);
> + if (!mode)
> + return -ENOMEM;
> +
> + drm_mode_set_name(mode);
> + drm_mode_probed_add(connector, mode);
> +
> + connector->display_info.width_mm = 56;
> + connector->display_info.height_mm = 75;
> +
