| Message ID | 1415006021-29313-16-git-send-email-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, Nov 3, 2014 at 4:13 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > From: Thierry Reding <treding@nvidia.com> > > This panel requires dual-channel mode. The device accepts command-mode > data on 8 lanes and will therefore need a dual-channel DSI controller. > The two interfaces that make up this device need to be instantiated in > the controllers that gang up to provide the dual-channel DSI host. > > Signed-off-by: Thierry Reding <treding@nvidia.com> Aside from the ENODATA nit I picked in patch 13, feel free to add my R-b to all of the patches in this set. Reviewed-by: Sean Paul <seanpaul@chromium.org> > --- > Changes in v4: > - use low power mode since highspeed message transfers don't work > - clarify required and optional properties for both DSI links > - power off panel when .prepare() fails > - properly drop reference to DSI-LINK2 > - don't allocate memory for DSI-LINK2 > - propagate errors on failure > > .../bindings/panel/sharp,lq101r1sx01.txt | 49 +++ > drivers/gpu/drm/panel/Kconfig | 13 + > drivers/gpu/drm/panel/Makefile | 1 + > drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 464 +++++++++++++++++++++ > 4 files changed, 527 insertions(+) > create mode 100644 Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt > create mode 100644 drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c > > diff --git a/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt > new file mode 100644 > index 000000000000..f522bb8e47e1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt > @@ -0,0 +1,49 @@ > +Sharp Microelectronics 10.1" WQXGA TFT LCD panel > + > +This panel requires a dual-channel DSI host to operate. It supports two modes: > +- left-right: each channel drives the left or right half of the screen > +- even-odd: each channel drives the even or odd lines of the screen > + > +Each of the DSI channels controls a separate DSI peripheral. The peripheral > +driven by the first link (DSI-LINK1), left or even, is considered the primary > +peripheral and controls the device. The 'link2' property contains a phandle > +to the peripheral driven by the second link (DSI-LINK2, right or odd). > + > +Note that in video mode the DSI-LINK1 interface always provides the left/even > +pixels and DSI-LINK2 always provides the right/odd pixels. In command mode it > +is possible to program either link to drive the left/even or right/odd pixels > +but for the sake of consistency this binding assumes that the same assignment > +is chosen as for video mode. > + > +Required properties: > +- compatible: should be "sharp,lq101r1sx01" > +- reg: DSI virtual channel of the peripheral > + > +Required properties (for DSI-LINK1 only): > +- link2: phandle to the DSI peripheral on the secondary link. Note that the > + presence of this property marks the containing node as DSI-LINK1. > +- power-supply: phandle of the regulator that provides the supply voltage > + > +Optional properties (for DSI-LINK1 only): > +- backlight: phandle of the backlight device attached to the panel > + > +Example: > + > + dsi@54300000 { > + panel: panel@0 { > + compatible = "sharp,lq101r1sx01"; > + reg = <0>; > + > + link2 = <&secondary>; > + > + power-supply = <...>; > + backlight = <...>; > + }; > + }; > + > + dsi@54400000 { > + secondary: panel@0 { > + compatible = "sharp,lq101r1sx01"; > + reg = <0>; > + }; > + }; > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig > index bee9f72b3a93..024e98ef8e4d 100644 > --- a/drivers/gpu/drm/panel/Kconfig > +++ b/drivers/gpu/drm/panel/Kconfig > @@ -27,4 +27,17 @@ config DRM_PANEL_S6E8AA0 > select DRM_MIPI_DSI > select VIDEOMODE_HELPERS > > +config DRM_PANEL_SHARP_LQ101R1SX01 > + tristate "Sharp LQ101R1SX01 panel" > + depends on OF > + depends on DRM_MIPI_DSI > + help > + Say Y here if you want to enable support for Sharp LQ101R1SX01 > + TFT-LCD modules. The panel has a 2560x1600 resolution and uses > + 24 bit RGB per pixel. It provides a dual MIPI DSI interface to > + the host and has a built-in LED backlight. > + > + To compile this driver as a module, choose M here: the module > + will be called panel-sharp-lq101r1sx01. > + > endmenu > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile > index 8b929212fad7..4b2a0430804b 100644 > --- a/drivers/gpu/drm/panel/Makefile > +++ b/drivers/gpu/drm/panel/Makefile > @@ -1,3 +1,4 @@ > obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o > obj-$(CONFIG_DRM_PANEL_LD9040) += panel-ld9040.o > obj-$(CONFIG_DRM_PANEL_S6E8AA0) += panel-s6e8aa0.o > +obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o > diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c > new file mode 100644 > index 000000000000..ee0e7f57e4a1 > --- /dev/null > +++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c > @@ -0,0 +1,464 @@ > +/* > + * Copyright (C) 2014 NVIDIA Corporation > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/backlight.h> > +#include <linux/gpio/consumer.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/regulator/consumer.h> > + > +#include <drm/drmP.h> > +#include <drm/drm_crtc.h> > +#include <drm/drm_mipi_dsi.h> > +#include <drm/drm_panel.h> > + > +#include <video/mipi_display.h> > + > +#include <linux/host1x.h> > + > +struct sharp_panel { > + struct drm_panel base; > + /* the datasheet refers to them as DSI-LINK1 and DSI-LINK2 */ > + struct mipi_dsi_device *link1; > + struct mipi_dsi_device *link2; > + > + struct backlight_device *backlight; > + struct regulator *supply; > + > + bool prepared; > + bool enabled; > + > + const struct drm_display_mode *mode; > +}; > + > +static inline struct sharp_panel *to_sharp_panel(struct drm_panel *panel) > +{ > + return container_of(panel, struct sharp_panel, base); > +} > + > +static int sharp_panel_write(struct sharp_panel *sharp, u16 offset, u8 value) > +{ > + u8 payload[3] = { offset >> 8, offset & 0xff, value }; > + struct mipi_dsi_device *dsi = sharp->link1; > + ssize_t err; > + > + err = mipi_dsi_generic_write(dsi, payload, sizeof(payload)); > + if (err < 0) { > + dev_err(&dsi->dev, "failed to write %02x to %04x: %d\n", > + value, offset, err); > + return err; > + } > + > + err = mipi_dsi_dcs_nop(dsi); > + if (err < 0) { > + dev_err(&dsi->dev, "failed to send DCS nop: %d\n", err); > + return err; > + } > + > + usleep_range(10, 20); > + > + return 0; > +} > + > +static __maybe_unused int sharp_panel_read(struct sharp_panel *sharp, > + u16 offset, u8 *value) > +{ > + ssize_t err; > + > + cpu_to_be16s(&offset); > + > + err = mipi_dsi_generic_read(sharp->link1, &offset, sizeof(offset), > + value, sizeof(*value)); > + if (err < 0) > + dev_err(&sharp->link1->dev, "failed to read from %04x: %d\n", > + offset, err); > + > + return err; > +} > + > +static int sharp_panel_disable(struct drm_panel *panel) > +{ > + struct sharp_panel *sharp = to_sharp_panel(panel); > + > + if (!sharp->enabled) > + return 0; > + > + if (sharp->backlight) { > + sharp->backlight->props.power = FB_BLANK_POWERDOWN; > + backlight_update_status(sharp->backlight); > + } > + > + sharp->enabled = false; > + > + return 0; > +} > + > +static int sharp_panel_unprepare(struct drm_panel *panel) > +{ > + struct sharp_panel *sharp = to_sharp_panel(panel); > + int err; > + > + if (!sharp->prepared) > + return 0; > + > + err = mipi_dsi_dcs_set_display_off(sharp->link1); > + if (err < 0) > + dev_err(panel->dev, "failed to set display off: %d\n", err); > + > + err = mipi_dsi_dcs_enter_sleep_mode(sharp->link1); > + if (err < 0) > + dev_err(panel->dev, "failed to enter sleep mode: %d\n", err); > + > + msleep(120); > + > + regulator_disable(sharp->supply); > + > + sharp->prepared = false; > + > + return 0; > +} > + > +static int sharp_setup_symmetrical_split(struct mipi_dsi_device *left, > + struct mipi_dsi_device *right, > + const struct drm_display_mode *mode) > +{ > + int err; > + > + err = mipi_dsi_dcs_set_column_address(left, 0, mode->hdisplay / 2 - 1); > + if (err < 0) { > + dev_err(&left->dev, "failed to set column address: %d\n", err); > + return err; > + } > + > + err = mipi_dsi_dcs_set_page_address(left, 0, mode->vdisplay - 1); > + if (err < 0) { > + dev_err(&left->dev, "failed to set page address: %d\n", err); > + return err; > + } > + > + err = mipi_dsi_dcs_set_column_address(right, mode->hdisplay / 2, > + mode->hdisplay - 1); > + if (err < 0) { > + dev_err(&right->dev, "failed to set column address: %d\n", err); > + return err; > + } > + > + err = mipi_dsi_dcs_set_page_address(right, 0, mode->vdisplay - 1); > + if (err < 0) { > + dev_err(&right->dev, "failed to set page address: %d\n", err); > + return err; > + } > + > + return 0; > +} > + > +static int sharp_panel_prepare(struct drm_panel *panel) > +{ > + struct sharp_panel *sharp = to_sharp_panel(panel); > + u8 format = MIPI_DCS_PIXEL_FMT_24BIT; > + int err; > + > + if (sharp->prepared) > + return 0; > + > + err = regulator_enable(sharp->supply); > + if (err < 0) > + return err; > + > + usleep_range(10000, 20000); > + > + err = mipi_dsi_dcs_soft_reset(sharp->link1); > + if (err < 0) { > + dev_err(panel->dev, "soft reset failed: %d\n", err); > + goto poweroff; > + } > + > + msleep(120); > + > + err = mipi_dsi_dcs_exit_sleep_mode(sharp->link1); > + if (err < 0) { > + dev_err(panel->dev, "failed to exit sleep mode: %d\n", err); > + goto poweroff; > + } > + > + /* > + * The MIPI DCS specification mandates this delay only between the > + * exit_sleep_mode and enter_sleep_mode commands, so it isn't strictly > + * necessary here. > + */ > + /* > + msleep(120); > + */ > + > + /* set left-right mode */ > + err = sharp_panel_write(sharp, 0x1000, 0x2a); > + if (err < 0) { > + dev_err(panel->dev, "failed to set left-right mode: %d\n", err); > + goto poweroff; > + } > + > + /* enable command mode */ > + err = sharp_panel_write(sharp, 0x1001, 0x01); > + if (err < 0) { > + dev_err(panel->dev, "failed to enable command mode: %d\n", err); > + goto poweroff; > + } > + > + err = mipi_dsi_dcs_set_pixel_format(sharp->link1, format); > + if (err < 0) { > + dev_err(panel->dev, "failed to set pixel format: %d\n", err); > + goto poweroff; > + } > + > + /* > + * TODO: The device supports both left-right and even-odd split > + * configurations, but this driver currently supports only the left- > + * right split. To support a different mode a mechanism needs to be > + * put in place to communicate the configuration back to the DSI host > + * controller. > + */ > + err = sharp_setup_symmetrical_split(sharp->link1, sharp->link2, > + sharp->mode); > + if (err < 0) { > + dev_err(panel->dev, "failed to set up symmetrical split: %d\n", > + err); > + goto poweroff; > + } > + > + err = mipi_dsi_dcs_set_display_on(sharp->link1); > + if (err < 0) { > + dev_err(panel->dev, "failed to set display on: %d\n", err); > + goto poweroff; > + } > + > + sharp->prepared = true; > + > + return 0; > + > +poweroff: > + regulator_disable(sharp->supply); > + return err; > +} > + > +static int sharp_panel_enable(struct drm_panel *panel) > +{ > + struct sharp_panel *sharp = to_sharp_panel(panel); > + > + if (sharp->enabled) > + return 0; > + > + if (sharp->backlight) { > + sharp->backlight->props.power = FB_BLANK_UNBLANK; > + backlight_update_status(sharp->backlight); > + } > + > + sharp->enabled = true; > + > + return 0; > +} > + > +static const struct drm_display_mode default_mode = { > + .clock = 278000, > + .hdisplay = 2560, > + .hsync_start = 2560 + 128, > + .hsync_end = 2560 + 128 + 64, > + .htotal = 2560 + 128 + 64 + 64, > + .vdisplay = 1600, > + .vsync_start = 1600 + 4, > + .vsync_end = 1600 + 4 + 8, > + .vtotal = 1600 + 4 + 8 + 32, > + .vrefresh = 60, > +}; > + > +static int sharp_panel_get_modes(struct drm_panel *panel) > +{ > + struct drm_display_mode *mode; > + > + mode = drm_mode_duplicate(panel->drm, &default_mode); > + if (!mode) { > + dev_err(panel->drm->dev, "failed to add mode %ux%ux@%u\n", > + default_mode.hdisplay, default_mode.vdisplay, > + default_mode.vrefresh); > + return -ENOMEM; > + } > + > + drm_mode_set_name(mode); > + > + drm_mode_probed_add(panel->connector, mode); > + > + panel->connector->display_info.width_mm = 217; > + panel->connector->display_info.height_mm = 136; > + > + return 1; > +} > + > +static const struct drm_panel_funcs sharp_panel_funcs = { > + .disable = sharp_panel_disable, > + .unprepare = sharp_panel_unprepare, > + .prepare = sharp_panel_prepare, > + .enable = sharp_panel_enable, > + .get_modes = sharp_panel_get_modes, > +}; > + > +static const struct of_device_id sharp_of_match[] = { > + { .compatible = "sharp,lq101r1sx01", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, sharp_of_match); > + > +static int sharp_panel_add(struct sharp_panel *sharp) > +{ > + struct device_node *np; > + int err; > + > + sharp->mode = &default_mode; > + > + sharp->supply = devm_regulator_get(&sharp->link1->dev, "power"); > + if (IS_ERR(sharp->supply)) > + return PTR_ERR(sharp->supply); > + > + np = of_parse_phandle(sharp->link1->dev.of_node, "backlight", 0); > + if (np) { > + sharp->backlight = of_find_backlight_by_node(np); > + of_node_put(np); > + > + if (!sharp->backlight) > + return -EPROBE_DEFER; > + } > + > + drm_panel_init(&sharp->base); > + sharp->base.funcs = &sharp_panel_funcs; > + sharp->base.dev = &sharp->link1->dev; > + > + err = drm_panel_add(&sharp->base); > + if (err < 0) > + goto put_backlight; > + > + return 0; > + > +put_backlight: > + if (sharp->backlight) > + put_device(&sharp->backlight->dev); > + > + return err; > +} > + > +static void sharp_panel_del(struct sharp_panel *sharp) > +{ > + if (sharp->base.dev) > + drm_panel_remove(&sharp->base); > + > + if (sharp->backlight) > + put_device(&sharp->backlight->dev); > + > + if (sharp->link2) > + put_device(&sharp->link2->dev); > +} > + > +static int sharp_panel_probe(struct mipi_dsi_device *dsi) > +{ > + struct mipi_dsi_device *secondary = NULL; > + struct sharp_panel *sharp; > + struct device_node *np; > + int err; > + > + dsi->lanes = 4; > + dsi->format = MIPI_DSI_FMT_RGB888; > + dsi->mode_flags = MIPI_DSI_MODE_LPM; > + > + /* Find DSI-LINK1 */ > + np = of_parse_phandle(dsi->dev.of_node, "link2", 0); > + if (np) { > + secondary = of_find_mipi_dsi_device_by_node(np); > + of_node_put(np); > + > + if (!secondary) > + return -EPROBE_DEFER; > + } > + > + /* register a panel for only the DSI-LINK1 interface */ > + if (secondary) { > + sharp = devm_kzalloc(&dsi->dev, sizeof(*sharp), GFP_KERNEL); > + if (!sharp) { > + put_device(&secondary->dev); > + return -ENOMEM; > + } > + > + mipi_dsi_set_drvdata(dsi, sharp); > + > + sharp->link2 = secondary; > + sharp->link1 = dsi; > + > + err = sharp_panel_add(sharp); > + if (err < 0) { > + put_device(&secondary->dev); > + return err; > + } > + } > + > + err = mipi_dsi_attach(dsi); > + if (err < 0) { > + if (secondary) > + sharp_panel_del(sharp); > + > + return err; > + } > + > + return 0; > +} > + > +static int sharp_panel_remove(struct mipi_dsi_device *dsi) > +{ > + struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi); > + int err; > + > + /* only detach from host for the DSI-LINK2 interface */ > + if (!sharp) { > + mipi_dsi_detach(dsi); > + return 0; > + } > + > + err = sharp_panel_disable(&sharp->base); > + if (err < 0) > + dev_err(&dsi->dev, "failed to disable panel: %d\n", err); > + > + err = mipi_dsi_detach(dsi); > + if (err < 0) > + dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err); > + > + drm_panel_detach(&sharp->base); > + sharp_panel_del(sharp); > + > + return 0; > +} > + > +static void sharp_panel_shutdown(struct mipi_dsi_device *dsi) > +{ > + struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi); > + > + /* nothing to do for DSI-LINK2 */ > + if (!sharp) > + return; > + > + sharp_panel_disable(&sharp->base); > +} > + > +static struct mipi_dsi_driver sharp_panel_driver = { > + .driver = { > + .name = "panel-sharp-lq101r1sx01", > + .of_match_table = sharp_of_match, > + }, > + .probe = sharp_panel_probe, > + .remove = sharp_panel_remove, > + .shutdown = sharp_panel_shutdown, > +}; > +module_mipi_dsi_driver(sharp_panel_driver); > + > +MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>"); > +MODULE_DESCRIPTION("Sharp LQ101R1SX01 panel driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.1.2 >
On 11/03/2014 10:13 AM, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > This panel requires dual-channel mode. The device accepts command-mode > data on 8 lanes and will therefore need a dual-channel DSI controller. > The two interfaces that make up this device need to be instantiated in > the controllers that gang up to provide the dual-channel DSI host. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > Changes in v4: > - use low power mode since highspeed message transfers don't work > - clarify required and optional properties for both DSI links > - power off panel when .prepare() fails > - properly drop reference to DSI-LINK2 > - don't allocate memory for DSI-LINK2 > - propagate errors on failure > > .../bindings/panel/sharp,lq101r1sx01.txt | 49 +++ > drivers/gpu/drm/panel/Kconfig | 13 + > drivers/gpu/drm/panel/Makefile | 1 + > drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 464 +++++++++++++++++++++ > 4 files changed, 527 insertions(+) > create mode 100644 Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt > create mode 100644 drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c > > diff --git a/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt > new file mode 100644 > index 000000000000..f522bb8e47e1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt > @@ -0,0 +1,49 @@ > +Sharp Microelectronics 10.1" WQXGA TFT LCD panel > + > +This panel requires a dual-channel DSI host to operate. It supports two modes: > +- left-right: each channel drives the left or right half of the screen > +- even-odd: each channel drives the even or odd lines of the screen > + > +Each of the DSI channels controls a separate DSI peripheral. The peripheral > +driven by the first link (DSI-LINK1), left or even, is considered the primary > +peripheral and controls the device. The 'link2' property contains a phandle > +to the peripheral driven by the second link (DSI-LINK2, right or odd). > + > +Note that in video mode the DSI-LINK1 interface always provides the left/even > +pixels and DSI-LINK2 always provides the right/odd pixels. In command mode it > +is possible to program either link to drive the left/even or right/odd pixels > +but for the sake of consistency this binding assumes that the same assignment > +is chosen as for video mode. > + > +Required properties: > +- compatible: should be "sharp,lq101r1sx01" > +- reg: DSI virtual channel of the peripheral > + > +Required properties (for DSI-LINK1 only): > +- link2: phandle to the DSI peripheral on the secondary link. Note that the > + presence of this property marks the containing node as DSI-LINK1. > +- power-supply: phandle of the regulator that provides the supply voltage > + > +Optional properties (for DSI-LINK1 only): > +- backlight: phandle of the backlight device attached to the panel > + > +Example: > + > + dsi@54300000 { > + panel: panel@0 { > + compatible = "sharp,lq101r1sx01"; > + reg = <0>; > + > + link2 = <&secondary>; > + > + power-supply = <...>; > + backlight = <...>; > + }; > + }; > + > + dsi@54400000 { > + secondary: panel@0 { > + compatible = "sharp,lq101r1sx01"; > + reg = <0>; > + }; > + }; The bindings above and the implementation below clearly shows that the secondary node is just a dummy dsi device. Hiding this behind conditionals in both docs and code does not look good to me. On the other side having common dummy dsi driver would allow to reuse it in other dual-dsi devices. So instead of 2nd node you would have: dsi@54400000 { secondary: panel@0 { compatible = "dsi-dummy-whatever"; reg = <0>; }; }; Or just: dsi2: dsi@54400000 { } with phandle to dsi2 in 1st node, in such case 2nd dsi dev would be created dynamically like with i2c_new_dummy. But this is of course just my opinion. > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig > index bee9f72b3a93..024e98ef8e4d 100644 > --- a/drivers/gpu/drm/panel/Kconfig > +++ b/drivers/gpu/drm/panel/Kconfig > @@ -27,4 +27,17 @@ config DRM_PANEL_S6E8AA0 > select DRM_MIPI_DSI > select VIDEOMODE_HELPERS > > +config DRM_PANEL_SHARP_LQ101R1SX01 > + tristate "Sharp LQ101R1SX01 panel" > + depends on OF > + depends on DRM_MIPI_DSI > + help > + Say Y here if you want to enable support for Sharp LQ101R1SX01 > + TFT-LCD modules. The panel has a 2560x1600 resolution and uses > + 24 bit RGB per pixel. It provides a dual MIPI DSI interface to > + the host and has a built-in LED backlight. > + > + To compile this driver as a module, choose M here: the module > + will be called panel-sharp-lq101r1sx01. > + > endmenu > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile > index 8b929212fad7..4b2a0430804b 100644 > --- a/drivers/gpu/drm/panel/Makefile > +++ b/drivers/gpu/drm/panel/Makefile > @@ -1,3 +1,4 @@ > obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o > obj-$(CONFIG_DRM_PANEL_LD9040) += panel-ld9040.o > obj-$(CONFIG_DRM_PANEL_S6E8AA0) += panel-s6e8aa0.o > +obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o > diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c > new file mode 100644 > index 000000000000..ee0e7f57e4a1 > --- /dev/null > +++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c > @@ -0,0 +1,464 @@ > +/* > + * Copyright (C) 2014 NVIDIA Corporation > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/backlight.h> > +#include <linux/gpio/consumer.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/regulator/consumer.h> > + > +#include <drm/drmP.h> > +#include <drm/drm_crtc.h> > +#include <drm/drm_mipi_dsi.h> > +#include <drm/drm_panel.h> > + > +#include <video/mipi_display.h> > + > +#include <linux/host1x.h> > + > +struct sharp_panel { > + struct drm_panel base; > + /* the datasheet refers to them as DSI-LINK1 and DSI-LINK2 */ > + struct mipi_dsi_device *link1; > + struct mipi_dsi_device *link2; > + > + struct backlight_device *backlight; > + struct regulator *supply; > + > + bool prepared; > + bool enabled; Nitpick. As I wrote in review to previous version it is up to panel client (usually connector) to call panel callbacks properly, we should not add additional checks to panels. If call order is incorrect then the client should be fixed. > + > + const struct drm_display_mode *mode; > +}; > + > +static inline struct sharp_panel *to_sharp_panel(struct drm_panel *panel) > +{ > + return container_of(panel, struct sharp_panel, base); > +} > + > +static int sharp_panel_write(struct sharp_panel *sharp, u16 offset, u8 value) > +{ > + u8 payload[3] = { offset >> 8, offset & 0xff, value }; > + struct mipi_dsi_device *dsi = sharp->link1; > + ssize_t err; > + > + err = mipi_dsi_generic_write(dsi, payload, sizeof(payload)); > + if (err < 0) { > + dev_err(&dsi->dev, "failed to write %02x to %04x: %d\n", > + value, offset, err); > + return err; > + } > + > + err = mipi_dsi_dcs_nop(dsi); > + if (err < 0) { > + dev_err(&dsi->dev, "failed to send DCS nop: %d\n", err); > + return err; > + } > + > + usleep_range(10, 20); > + > + return 0; > +} > + > +static __maybe_unused int sharp_panel_read(struct sharp_panel *sharp, > + u16 offset, u8 *value) > +{ > + ssize_t err; > + > + cpu_to_be16s(&offset); > + > + err = mipi_dsi_generic_read(sharp->link1, &offset, sizeof(offset), > + value, sizeof(*value)); > + if (err < 0) > + dev_err(&sharp->link1->dev, "failed to read from %04x: %d\n", > + offset, err); > + > + return err; > +} > + > +static int sharp_panel_disable(struct drm_panel *panel) > +{ > + struct sharp_panel *sharp = to_sharp_panel(panel); > + > + if (!sharp->enabled) > + return 0; > + > + if (sharp->backlight) { > + sharp->backlight->props.power = FB_BLANK_POWERDOWN; > + backlight_update_status(sharp->backlight); > + } > + > + sharp->enabled = false; > + > + return 0; > +} > + > +static int sharp_panel_unprepare(struct drm_panel *panel) > +{ > + struct sharp_panel *sharp = to_sharp_panel(panel); > + int err; > + > + if (!sharp->prepared) > + return 0; > + > + err = mipi_dsi_dcs_set_display_off(sharp->link1); > + if (err < 0) > + dev_err(panel->dev, "failed to set display off: %d\n", err); > + > + err = mipi_dsi_dcs_enter_sleep_mode(sharp->link1); > + if (err < 0) > + dev_err(panel->dev, "failed to enter sleep mode: %d\n", err); > + > + msleep(120); > + > + regulator_disable(sharp->supply); > + > + sharp->prepared = false; > + > + return 0; > +} > + > +static int sharp_setup_symmetrical_split(struct mipi_dsi_device *left, > + struct mipi_dsi_device *right, > + const struct drm_display_mode *mode) > +{ > + int err; > + > + err = mipi_dsi_dcs_set_column_address(left, 0, mode->hdisplay / 2 - 1); > + if (err < 0) { > + dev_err(&left->dev, "failed to set column address: %d\n", err); > + return err; > + } > + > + err = mipi_dsi_dcs_set_page_address(left, 0, mode->vdisplay - 1); > + if (err < 0) { > + dev_err(&left->dev, "failed to set page address: %d\n", err); > + return err; > + } > + > + err = mipi_dsi_dcs_set_column_address(right, mode->hdisplay / 2, > + mode->hdisplay - 1); > + if (err < 0) { > + dev_err(&right->dev, "failed to set column address: %d\n", err); > + return err; > + } > + > + err = mipi_dsi_dcs_set_page_address(right, 0, mode->vdisplay - 1); > + if (err < 0) { > + dev_err(&right->dev, "failed to set page address: %d\n", err); > + return err; > + } > + > + return 0; > +} > + > +static int sharp_panel_prepare(struct drm_panel *panel) > +{ > + struct sharp_panel *sharp = to_sharp_panel(panel); > + u8 format = MIPI_DCS_PIXEL_FMT_24BIT; > + int err; > + > + if (sharp->prepared) > + return 0; > + > + err = regulator_enable(sharp->supply); > + if (err < 0) > + return err; > + > + usleep_range(10000, 20000); > + > + err = mipi_dsi_dcs_soft_reset(sharp->link1); > + if (err < 0) { > + dev_err(panel->dev, "soft reset failed: %d\n", err); > + goto poweroff; > + } > + > + msleep(120); > + > + err = mipi_dsi_dcs_exit_sleep_mode(sharp->link1); > + if (err < 0) { > + dev_err(panel->dev, "failed to exit sleep mode: %d\n", err); > + goto poweroff; > + } > + > + /* > + * The MIPI DCS specification mandates this delay only between the > + * exit_sleep_mode and enter_sleep_mode commands, so it isn't strictly > + * necessary here. > + */ > + /* > + msleep(120); > + */ > + > + /* set left-right mode */ > + err = sharp_panel_write(sharp, 0x1000, 0x2a); > + if (err < 0) { > + dev_err(panel->dev, "failed to set left-right mode: %d\n", err); > + goto poweroff; > + } > + > + /* enable command mode */ > + err = sharp_panel_write(sharp, 0x1001, 0x01); > + if (err < 0) { > + dev_err(panel->dev, "failed to enable command mode: %d\n", err); > + goto poweroff; > + } > + > + err = mipi_dsi_dcs_set_pixel_format(sharp->link1, format); > + if (err < 0) { > + dev_err(panel->dev, "failed to set pixel format: %d\n", err); > + goto poweroff; > + } > + > + /* > + * TODO: The device supports both left-right and even-odd split > + * configurations, but this driver currently supports only the left- > + * right split. To support a different mode a mechanism needs to be > + * put in place to communicate the configuration back to the DSI host > + * controller. > + */ > + err = sharp_setup_symmetrical_split(sharp->link1, sharp->link2, > + sharp->mode); > + if (err < 0) { > + dev_err(panel->dev, "failed to set up symmetrical split: %d\n", > + err); > + goto poweroff; > + } > + > + err = mipi_dsi_dcs_set_display_on(sharp->link1); > + if (err < 0) { > + dev_err(panel->dev, "failed to set display on: %d\n", err); > + goto poweroff; > + } > + > + sharp->prepared = true; > + > + return 0; > + > +poweroff: > + regulator_disable(sharp->supply); > + return err; > +} > + > +static int sharp_panel_enable(struct drm_panel *panel) > +{ > + struct sharp_panel *sharp = to_sharp_panel(panel); > + > + if (sharp->enabled) > + return 0; > + > + if (sharp->backlight) { > + sharp->backlight->props.power = FB_BLANK_UNBLANK; > + backlight_update_status(sharp->backlight); > + } > + > + sharp->enabled = true; > + > + return 0; > +} > + > +static const struct drm_display_mode default_mode = { > + .clock = 278000, > + .hdisplay = 2560, > + .hsync_start = 2560 + 128, > + .hsync_end = 2560 + 128 + 64, > + .htotal = 2560 + 128 + 64 + 64, > + .vdisplay = 1600, > + .vsync_start = 1600 + 4, > + .vsync_end = 1600 + 4 + 8, > + .vtotal = 1600 + 4 + 8 + 32, > + .vrefresh = 60, > +}; > + > +static int sharp_panel_get_modes(struct drm_panel *panel) > +{ > + struct drm_display_mode *mode; > + > + mode = drm_mode_duplicate(panel->drm, &default_mode); > + if (!mode) { > + dev_err(panel->drm->dev, "failed to add mode %ux%ux@%u\n", > + default_mode.hdisplay, default_mode.vdisplay, > + default_mode.vrefresh); > + return -ENOMEM; > + } > + > + drm_mode_set_name(mode); > + > + drm_mode_probed_add(panel->connector, mode); > + > + panel->connector->display_info.width_mm = 217; > + panel->connector->display_info.height_mm = 136; > + > + return 1; > +} > + > +static const struct drm_panel_funcs sharp_panel_funcs = { > + .disable = sharp_panel_disable, > + .unprepare = sharp_panel_unprepare, > + .prepare = sharp_panel_prepare, > + .enable = sharp_panel_enable, > + .get_modes = sharp_panel_get_modes, > +}; > + > +static const struct of_device_id sharp_of_match[] = { > + { .compatible = "sharp,lq101r1sx01", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, sharp_of_match); > + > +static int sharp_panel_add(struct sharp_panel *sharp) > +{ > + struct device_node *np; > + int err; > + > + sharp->mode = &default_mode; > + > + sharp->supply = devm_regulator_get(&sharp->link1->dev, "power"); > + if (IS_ERR(sharp->supply)) > + return PTR_ERR(sharp->supply); > + > + np = of_parse_phandle(sharp->link1->dev.of_node, "backlight", 0); > + if (np) { > + sharp->backlight = of_find_backlight_by_node(np); > + of_node_put(np); > + > + if (!sharp->backlight) > + return -EPROBE_DEFER; > + } > + > + drm_panel_init(&sharp->base); > + sharp->base.funcs = &sharp_panel_funcs; > + sharp->base.dev = &sharp->link1->dev; > + > + err = drm_panel_add(&sharp->base); > + if (err < 0) > + goto put_backlight; > + > + return 0; > + > +put_backlight: > + if (sharp->backlight) > + put_device(&sharp->backlight->dev); > + > + return err; > +} > + > +static void sharp_panel_del(struct sharp_panel *sharp) > +{ > + if (sharp->base.dev) > + drm_panel_remove(&sharp->base); > + > + if (sharp->backlight) > + put_device(&sharp->backlight->dev); > + > + if (sharp->link2) > + put_device(&sharp->link2->dev); > +} > + > +static int sharp_panel_probe(struct mipi_dsi_device *dsi) > +{ > + struct mipi_dsi_device *secondary = NULL; > + struct sharp_panel *sharp; > + struct device_node *np; > + int err; > + > + dsi->lanes = 4; > + dsi->format = MIPI_DSI_FMT_RGB888; > + dsi->mode_flags = MIPI_DSI_MODE_LPM; > + > + /* Find DSI-LINK1 */ > + np = of_parse_phandle(dsi->dev.of_node, "link2", 0); > + if (np) { > + secondary = of_find_mipi_dsi_device_by_node(np); > + of_node_put(np); > + > + if (!secondary) > + return -EPROBE_DEFER; > + } > + > + /* register a panel for only the DSI-LINK1 interface */ > + if (secondary) { > + sharp = devm_kzalloc(&dsi->dev, sizeof(*sharp), GFP_KERNEL); > + if (!sharp) { > + put_device(&secondary->dev); > + return -ENOMEM; > + } > + > + mipi_dsi_set_drvdata(dsi, sharp); > + > + sharp->link2 = secondary; > + sharp->link1 = dsi; > + > + err = sharp_panel_add(sharp); > + if (err < 0) { > + put_device(&secondary->dev); > + return err; > + } > + } > + > + err = mipi_dsi_attach(dsi); > + if (err < 0) { > + if (secondary) > + sharp_panel_del(sharp); > + > + return err; > + } > + > + return 0; > +} > + > +static int sharp_panel_remove(struct mipi_dsi_device *dsi) > +{ > + struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi); > + int err; > + > + /* only detach from host for the DSI-LINK2 interface */ > + if (!sharp) { > + mipi_dsi_detach(dsi); Quotation from previous review: >> There is no locking mechanism here, so it is possible that >> everything can crash if someone unbind driver from LINK2 and then try to >> enable the panel. > > I don't think so. Since we're not doing anything with the DSI-LINK2 > device anymore it's irrelevant whether it is bound to the driver or > not. > Please consider following scenario: 1. panel link2 is probed 2. panel link1 is probed 3. panel link2 is unbound (for example by: echo link2 >/sys/bus/dsi/drivers/*lq101*/unbind) 4. drm is bound: - during sharp_setup_symmetrical_split you will call transfer but there is no device attached to dsi2. Maybe it will not cause any troubles but it seems incorrect. I guess simple workaround is to do device_lock and check if device is bound every time you want to access link2 device. > + return 0; > + } > + > + err = sharp_panel_disable(&sharp->base); > + if (err < 0) > + dev_err(&dsi->dev, "failed to disable panel: %d\n", err); > + > + err = mipi_dsi_detach(dsi); > + if (err < 0) > + dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err); > + > + drm_panel_detach(&sharp->base); > + sharp_panel_del(sharp); You have following flow: sharp_probe: drm_panel_add dsi_attach sharp_remove: drm_panel_disable dsi_detach drm_panel_detach drm_panel_del So panel initialization is done by connector but panel de-initialization is done by the panel itself, on the other side connector can also disable/detach the panel. So it seems that drm_panel resource ownership is split between the panel and the connector. It seems racy, unless you provide additional synchronization mechanisms. And indeed there are races here, for example there are two processes: - (1) connector calls sharp_prepare, in the middle of sharp_setup_symmetrical_split process goes to sleep, - (2) sharp_panel_remove is called due to unbind, or module removal, sharp_disable is called, - (1) process wakes up, it tries to continue sharp_setup_symmetrical_split, results are unpredictable. If you leave ownership of drm_panel to connector you can reuse synchronization mechanisms of connector/drm inside connector. Otherwise you will need to add another locks in drm_panel. You can look at exynos_drm_dsi.c code, especially: - exynos_dsi_detect, - exynos_dsi_host_attach, - exynos_dsi_host_detach, and usage of dsi->panel_node and dsi->panel with drm synchronization via drm_helper_hpd_irq_event. Regards Andrzej > + > + return 0; > +} > + > +static void sharp_panel_shutdown(struct mipi_dsi_device *dsi) > +{ > + struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi); > + > + /* nothing to do for DSI-LINK2 */ > + if (!sharp) > + return; > + > + sharp_panel_disable(&sharp->base); > +} > + > +static struct mipi_dsi_driver sharp_panel_driver = { > + .driver = { > + .name = "panel-sharp-lq101r1sx01", > + .of_match_table = sharp_of_match, > + }, > + .probe = sharp_panel_probe, > + .remove = sharp_panel_remove, > + .shutdown = sharp_panel_shutdown, > +}; > +module_mipi_dsi_driver(sharp_panel_driver); > + > +MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>"); > +MODULE_DESCRIPTION("Sharp LQ101R1SX01 panel driver"); > +MODULE_LICENSE("GPL v2"); >
On Tue, Nov 04, 2014 at 11:41:15AM +0100, Andrzej Hajda wrote: > On 11/03/2014 10:13 AM, Thierry Reding wrote: [...] > > diff --git a/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt [...] > > +Example: > > + > > + dsi@54300000 { > > + panel: panel@0 { > > + compatible = "sharp,lq101r1sx01"; > > + reg = <0>; > > + > > + link2 = <&secondary>; > > + > > + power-supply = <...>; > > + backlight = <...>; > > + }; > > + }; > > + > > + dsi@54400000 { > > + secondary: panel@0 { > > + compatible = "sharp,lq101r1sx01"; > > + reg = <0>; > > + }; > > + }; > > The bindings above and the implementation below clearly shows > that the secondary node is just a dummy dsi device. It's not only a dummy device. Binding it to the device's compatible string allows the driver to properly set up the DSI device (flags, number of lanes, ...). > Hiding this behind conditionals in both docs and code does not look good > to me. On the other side having common dummy dsi driver > would allow to reuse it in other dual-dsi devices. > So instead of 2nd node you would have: > dsi@54400000 { > secondary: panel@0 { > compatible = "dsi-dummy-whatever"; > reg = <0>; > }; > }; > > Or just: > dsi2: dsi@54400000 { > } > > with phandle to dsi2 in 1st node, in such case 2nd dsi dev would be > created dynamically like with i2c_new_dummy. I don't think that's technically valid DT. Every device must have a compatible property. And the compatible property should have an entry for the specific model of the device. "dummy" doesn't qualify for that Hopefully when more dual-channel DSI devices get supported some kind of pattern will emerge. For now I'll go with what I have in this patch. It is as accurate as I can think of. > > diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c [...] > > +struct sharp_panel { > > + struct drm_panel base; > > + /* the datasheet refers to them as DSI-LINK1 and DSI-LINK2 */ > > + struct mipi_dsi_device *link1; > > + struct mipi_dsi_device *link2; > > + > > + struct backlight_device *backlight; > > + struct regulator *supply; > > + > > + bool prepared; > > + bool enabled; > > Nitpick. > As I wrote in review to previous version it is up to panel client > (usually connector) to call panel callbacks properly, we should not add > additional checks to panels. If call order is incorrect then the client > should be fixed. There are no such guarantees today. But hopefully this will change with atomic modesetting. > > +static int sharp_panel_remove(struct mipi_dsi_device *dsi) > > +{ > > + struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi); > > + int err; > > + > > + /* only detach from host for the DSI-LINK2 interface */ > > + if (!sharp) { > > + mipi_dsi_detach(dsi); > > Quotation from previous review: > > >> There is no locking mechanism here, so it is possible that > >> everything can crash if someone unbind driver from LINK2 and then try to > >> enable the panel. > > > > I don't think so. Since we're not doing anything with the DSI-LINK2 > > device anymore it's irrelevant whether it is bound to the driver or > > not. > > > > Please consider following scenario: > 1. panel link2 is probed > 2. panel link1 is probed > 3. panel link2 is unbound (for example by: echo link2 > >/sys/bus/dsi/drivers/*lq101*/unbind) > 4. drm is bound: > - during sharp_setup_symmetrical_split you will call transfer > but there is no device attached to dsi2. > > Maybe it will not cause any troubles but it seems incorrect. The device is still there. The driver may not be bound to it, but we don't need that for the transfers anyway. > I guess simple workaround is to do device_lock and check if device is > bound every time you want to access link2 device. I don't see where the problem is. We do keep a reference to the LINK2 device from the first, so it can't just disappear. The only way it could disappear is if the host controller that provides its bus goes away, but there's code at the DSI host controller level to deal with that. > > + return 0; > > + } > > + > > + err = sharp_panel_disable(&sharp->base); > > + if (err < 0) > > + dev_err(&dsi->dev, "failed to disable panel: %d\n", err); > > + > > + err = mipi_dsi_detach(dsi); > > + if (err < 0) > > + dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err); > > + > > + drm_panel_detach(&sharp->base); > > + sharp_panel_del(sharp); > > You have following flow: > > sharp_probe: > drm_panel_add > dsi_attach > > sharp_remove: > drm_panel_disable > dsi_detach > drm_panel_detach > drm_panel_del > > So panel initialization is done by connector > but panel de-initialization is done by the panel itself, No. The panel registers with the panel registry so that the connector can find it. But when the panel registers it doesn't know anything about the DRM device it is going to be attached to, so we have to attach it to the connector only when that becomes available. When the driver is unloaded, the panel unregisters from the registry. Also it needs to detach from a connector if it's still connected so that the connector can be marked unconnected. Similarly when the connector goes away it must let the panel know that it's being detached. But it looks like this infrastructure is generally somewhat immature. For example this only works relatively well for DSI panels because we have the DSI peripheral attach/detach callbacks. For RGB or LVDS panels we have no such thing, so there's no way currently for these panels to hotplug. > on the other side connector can also disable/detach the panel. > So it seems that drm_panel resource ownership is split between > the panel and the connector. It seems racy, unless you provide > additional synchronization mechanisms. > And indeed there are races here, for example there are two processes: > - (1) connector calls sharp_prepare, in the middle of > sharp_setup_symmetrical_split process goes to sleep, > - (2) sharp_panel_remove is called due to unbind, or module removal, > sharp_disable is called, > - (1) process wakes up, it tries to continue > sharp_setup_symmetrical_split, results are unpredictable. That's nothing you can solve by simply changing the calling order here. DRM panel is completely missing any infrastructure to allow you to safely deal with that kind of situation. But it's something that I've been trying to fix for the past couple of days. > If you leave ownership of drm_panel to connector you can reuse > synchronization mechanisms of connector/drm inside connector. Otherwise > you will need to add another locks in drm_panel. > > You can look at exynos_drm_dsi.c code, especially: > - exynos_dsi_detect, > - exynos_dsi_host_attach, > - exynos_dsi_host_detach, > > and usage of dsi->panel_node and dsi->panel with drm synchronization via > drm_helper_hpd_irq_event. There are a number of ways that one could possibly implement this. I do not think any of the existing ones is as good as it could be and I think we really ought to standardize on how to do this to make it easier for other drivers to adapt DRM panels (and bridges for that matter). Unfortunately before we have a good plan on how to handle panels more uniformly I don't see how we can possibly make everybody happy. It seems like currently whatever panel is written for Exynos isn't going to work on Tegra and vice-versa. So how about we merge this series and I'll look into how we can unify things? I'd appreciate any ideas on how such a unification could look. Thierry
On 11/04/2014 02:29 PM, Thierry Reding wrote: > On Tue, Nov 04, 2014 at 11:41:15AM +0100, Andrzej Hajda wrote: >> On 11/03/2014 10:13 AM, Thierry Reding wrote: > [...] >>> diff --git a/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt > [...] >>> +Example: >>> + >>> + dsi@54300000 { >>> + panel: panel@0 { >>> + compatible = "sharp,lq101r1sx01"; >>> + reg = <0>; >>> + >>> + link2 = <&secondary>; >>> + >>> + power-supply = <...>; >>> + backlight = <...>; >>> + }; >>> + }; >>> + >>> + dsi@54400000 { >>> + secondary: panel@0 { >>> + compatible = "sharp,lq101r1sx01"; >>> + reg = <0>; >>> + }; >>> + }; >> >> The bindings above and the implementation below clearly shows >> that the secondary node is just a dummy dsi device. > > It's not only a dummy device. Binding it to the device's compatible > string allows the driver to properly set up the DSI device (flags, > number of lanes, ...). This is why I called it dummy DSI device :) > >> Hiding this behind conditionals in both docs and code does not look good >> to me. On the other side having common dummy dsi driver >> would allow to reuse it in other dual-dsi devices. >> So instead of 2nd node you would have: >> dsi@54400000 { >> secondary: panel@0 { >> compatible = "dsi-dummy-whatever"; >> reg = <0>; >> }; >> }; >> >> Or just: >> dsi2: dsi@54400000 { >> } >> >> with phandle to dsi2 in 1st node, in such case 2nd dsi dev would be >> created dynamically like with i2c_new_dummy. > > I don't think that's technically valid DT. Every device must have a > compatible property. And the compatible property should have an entry > for the specific model of the device. "dummy" doesn't qualify for that One could ask if it is technically valid to represent one hw device via two separate nodes. Each choice scarifies something. > > Hopefully when more dual-channel DSI devices get supported some kind of > pattern will emerge. For now I'll go with what I have in this patch. It > is as accurate as I can think of. Agreed > >>> diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c > [...] >>> +struct sharp_panel { >>> + struct drm_panel base; >>> + /* the datasheet refers to them as DSI-LINK1 and DSI-LINK2 */ >>> + struct mipi_dsi_device *link1; >>> + struct mipi_dsi_device *link2; >>> + >>> + struct backlight_device *backlight; >>> + struct regulator *supply; >>> + >>> + bool prepared; >>> + bool enabled; >> >> Nitpick. >> As I wrote in review to previous version it is up to panel client >> (usually connector) to call panel callbacks properly, we should not add >> additional checks to panels. If call order is incorrect then the client >> should be fixed. > > There are no such guarantees today. But hopefully this will change with > atomic modesetting. I do not understand that. It is the connector code who calls these callbacks. Why cannot you force it to call them in proper order? > >>> +static int sharp_panel_remove(struct mipi_dsi_device *dsi) >>> +{ >>> + struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi); >>> + int err; >>> + >>> + /* only detach from host for the DSI-LINK2 interface */ >>> + if (!sharp) { >>> + mipi_dsi_detach(dsi); >> >> Quotation from previous review: >> >>>> There is no locking mechanism here, so it is possible that >>>> everything can crash if someone unbind driver from LINK2 and then try to >>>> enable the panel. >>> >>> I don't think so. Since we're not doing anything with the DSI-LINK2 >>> device anymore it's irrelevant whether it is bound to the driver or >>> not. >>> >> >> Please consider following scenario: >> 1. panel link2 is probed >> 2. panel link1 is probed >> 3. panel link2 is unbound (for example by: echo link2 >>> /sys/bus/dsi/drivers/*lq101*/unbind) >> 4. drm is bound: >> - during sharp_setup_symmetrical_split you will call transfer >> but there is no device attached to dsi2. >> >> Maybe it will not cause any troubles but it seems incorrect. > > The device is still there. The driver may not be bound to it, but we > don't need that for the transfers anyway. And you do not want to call it dummy device :) > >> I guess simple workaround is to do device_lock and check if device is >> bound every time you want to access link2 device. > > I don't see where the problem is. We do keep a reference to the LINK2 > device from the first, so it can't just disappear. The only way it could > disappear is if the host controller that provides its bus goes away, but > there's code at the DSI host controller level to deal with that. > As I understand it is important to you that host is set-up according to settings passed during LINK2 device attachment. IMHO assumption that the host will keep these settings after device is detached is incorrect. The host can return to its initial state or any other state. I think that it is valid if the host refuses transfers on 'non-attached' channels. >>> + return 0; >>> + } >>> + >>> + err = sharp_panel_disable(&sharp->base); >>> + if (err < 0) >>> + dev_err(&dsi->dev, "failed to disable panel: %d\n", err); >>> + >>> + err = mipi_dsi_detach(dsi); >>> + if (err < 0) >>> + dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err); >>> + >>> + drm_panel_detach(&sharp->base); >>> + sharp_panel_del(sharp); >> >> You have following flow: >> >> sharp_probe: >> drm_panel_add >> dsi_attach >> >> sharp_remove: >> drm_panel_disable >> dsi_detach >> drm_panel_detach >> drm_panel_del >> >> So panel initialization is done by connector >> but panel de-initialization is done by the panel itself, > > No. The panel registers with the panel registry so that the connector > can find it. But when the panel registers it doesn't know anything about > the DRM device it is going to be attached to, so we have to attach it to > the connector only when that becomes available. > > When the driver is unloaded, the panel unregisters from the registry. > Also it needs to detach from a connector if it's still connected so that > the connector can be marked unconnected. Similarly when the connector > goes away it must let the panel know that it's being detached. > > But it looks like this infrastructure is generally somewhat immature. > For example this only works relatively well for DSI panels because we > have the DSI peripheral attach/detach callbacks. For RGB or LVDS panels > we have no such thing, so there's no way currently for these panels to > hotplug. This is why I have proposed interface_tracker framework, we would have hotplug/hotunplug callbacks already :) > >> on the other side connector can also disable/detach the panel. >> So it seems that drm_panel resource ownership is split between >> the panel and the connector. It seems racy, unless you provide >> additional synchronization mechanisms. >> And indeed there are races here, for example there are two processes: >> - (1) connector calls sharp_prepare, in the middle of >> sharp_setup_symmetrical_split process goes to sleep, >> - (2) sharp_panel_remove is called due to unbind, or module removal, >> sharp_disable is called, >> - (1) process wakes up, it tries to continue >> sharp_setup_symmetrical_split, results are unpredictable. > > That's nothing you can solve by simply changing the calling order here. > DRM panel is completely missing any infrastructure to allow you to > safely deal with that kind of situation. This is not true for DSI panels - attach/detach callbacks allows to do it correctly. > > But it's something that I've been trying to fix for the past couple of > days. > >> If you leave ownership of drm_panel to connector you can reuse >> synchronization mechanisms of connector/drm inside connector. Otherwise >> you will need to add another locks in drm_panel. >> >> You can look at exynos_drm_dsi.c code, especially: >> - exynos_dsi_detect, >> - exynos_dsi_host_attach, >> - exynos_dsi_host_detach, >> >> and usage of dsi->panel_node and dsi->panel with drm synchronization via >> drm_helper_hpd_irq_event. > > There are a number of ways that one could possibly implement this. I do not think so. There are simple rules of synchronization, if you follow them you will usually end up with similar design. > I do > not think any of the existing ones is as good as it could be and I think > we really ought to standardize on how to do this to make it easier for > other drivers to adapt DRM panels (and bridges for that matter). > > Unfortunately before we have a good plan on how to handle panels more > uniformly I don't see how we can possibly make everybody happy. It seems > like currently whatever panel is written for Exynos isn't going to work > on Tegra and vice-versa. So how about we merge this series and I'll look > into how we can unify things? This is not about compatibility between different hosts and panels, this is just about making sharp panel driver work correctly with tegra dsi host. I think it is quite easy to fix and it would be good to do it before merging. Unification is another issue which can be solved later. Regards Andrzej > > I'd appreciate any ideas on how such a unification could look. > > Thierry > > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
diff --git a/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt new file mode 100644 index 000000000000..f522bb8e47e1 --- /dev/null +++ b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt @@ -0,0 +1,49 @@ +Sharp Microelectronics 10.1" WQXGA TFT LCD panel + +This panel requires a dual-channel DSI host to operate. It supports two modes: +- left-right: each channel drives the left or right half of the screen +- even-odd: each channel drives the even or odd lines of the screen + +Each of the DSI channels controls a separate DSI peripheral. The peripheral +driven by the first link (DSI-LINK1), left or even, is considered the primary +peripheral and controls the device. The 'link2' property contains a phandle +to the peripheral driven by the second link (DSI-LINK2, right or odd). + +Note that in video mode the DSI-LINK1 interface always provides the left/even +pixels and DSI-LINK2 always provides the right/odd pixels. In command mode it +is possible to program either link to drive the left/even or right/odd pixels +but for the sake of consistency this binding assumes that the same assignment +is chosen as for video mode. + +Required properties: +- compatible: should be "sharp,lq101r1sx01" +- reg: DSI virtual channel of the peripheral + +Required properties (for DSI-LINK1 only): +- link2: phandle to the DSI peripheral on the secondary link. Note that the + presence of this property marks the containing node as DSI-LINK1. +- power-supply: phandle of the regulator that provides the supply voltage + +Optional properties (for DSI-LINK1 only): +- backlight: phandle of the backlight device attached to the panel + +Example: + + dsi@54300000 { + panel: panel@0 { + compatible = "sharp,lq101r1sx01"; + reg = <0>; + + link2 = <&secondary>; + + power-supply = <...>; + backlight = <...>; + }; + }; + + dsi@54400000 { + secondary: panel@0 { + compatible = "sharp,lq101r1sx01"; + reg = <0>; + }; + }; diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index bee9f72b3a93..024e98ef8e4d 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -27,4 +27,17 @@ config DRM_PANEL_S6E8AA0 select DRM_MIPI_DSI select VIDEOMODE_HELPERS +config DRM_PANEL_SHARP_LQ101R1SX01 + tristate "Sharp LQ101R1SX01 panel" + depends on OF + depends on DRM_MIPI_DSI + help + Say Y here if you want to enable support for Sharp LQ101R1SX01 + TFT-LCD modules. The panel has a 2560x1600 resolution and uses + 24 bit RGB per pixel. It provides a dual MIPI DSI interface to + the host and has a built-in LED backlight. + + To compile this driver as a module, choose M here: the module + will be called panel-sharp-lq101r1sx01. + endmenu diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index 8b929212fad7..4b2a0430804b 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -1,3 +1,4 @@ obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o obj-$(CONFIG_DRM_PANEL_LD9040) += panel-ld9040.o obj-$(CONFIG_DRM_PANEL_S6E8AA0) += panel-s6e8aa0.o +obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c new file mode 100644 index 000000000000..ee0e7f57e4a1 --- /dev/null +++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c @@ -0,0 +1,464 @@ +/* + * Copyright (C) 2014 NVIDIA Corporation + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/backlight.h> +#include <linux/gpio/consumer.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/regulator/consumer.h> + +#include <drm/drmP.h> +#include <drm/drm_crtc.h> +#include <drm/drm_mipi_dsi.h> +#include <drm/drm_panel.h> + +#include <video/mipi_display.h> + +#include <linux/host1x.h> + +struct sharp_panel { + struct drm_panel base; + /* the datasheet refers to them as DSI-LINK1 and DSI-LINK2 */ + struct mipi_dsi_device *link1; + struct mipi_dsi_device *link2; + + struct backlight_device *backlight; + struct regulator *supply; + + bool prepared; + bool enabled; + + const struct drm_display_mode *mode; +}; + +static inline struct sharp_panel *to_sharp_panel(struct drm_panel *panel) +{ + return container_of(panel, struct sharp_panel, base); +} + +static int sharp_panel_write(struct sharp_panel *sharp, u16 offset, u8 value) +{ + u8 payload[3] = { offset >> 8, offset & 0xff, value }; + struct mipi_dsi_device *dsi = sharp->link1; + ssize_t err; + + err = mipi_dsi_generic_write(dsi, payload, sizeof(payload)); + if (err < 0) { + dev_err(&dsi->dev, "failed to write %02x to %04x: %d\n", + value, offset, err); + return err; + } + + err = mipi_dsi_dcs_nop(dsi); + if (err < 0) { + dev_err(&dsi->dev, "failed to send DCS nop: %d\n", err); + return err; + } + + usleep_range(10, 20); + + return 0; +} + +static __maybe_unused int sharp_panel_read(struct sharp_panel *sharp, + u16 offset, u8 *value) +{ + ssize_t err; + + cpu_to_be16s(&offset); + + err = mipi_dsi_generic_read(sharp->link1, &offset, sizeof(offset), + value, sizeof(*value)); + if (err < 0) + dev_err(&sharp->link1->dev, "failed to read from %04x: %d\n", + offset, err); + + return err; +} + +static int sharp_panel_disable(struct drm_panel *panel) +{ + struct sharp_panel *sharp = to_sharp_panel(panel); + + if (!sharp->enabled) + return 0; + + if (sharp->backlight) { + sharp->backlight->props.power = FB_BLANK_POWERDOWN; + backlight_update_status(sharp->backlight); + } + + sharp->enabled = false; + + return 0; +} + +static int sharp_panel_unprepare(struct drm_panel *panel) +{ + struct sharp_panel *sharp = to_sharp_panel(panel); + int err; + + if (!sharp->prepared) + return 0; + + err = mipi_dsi_dcs_set_display_off(sharp->link1); + if (err < 0) + dev_err(panel->dev, "failed to set display off: %d\n", err); + + err = mipi_dsi_dcs_enter_sleep_mode(sharp->link1); + if (err < 0) + dev_err(panel->dev, "failed to enter sleep mode: %d\n", err); + + msleep(120); + + regulator_disable(sharp->supply); + + sharp->prepared = false; + + return 0; +} + +static int sharp_setup_symmetrical_split(struct mipi_dsi_device *left, + struct mipi_dsi_device *right, + const struct drm_display_mode *mode) +{ + int err; + + err = mipi_dsi_dcs_set_column_address(left, 0, mode->hdisplay / 2 - 1); + if (err < 0) { + dev_err(&left->dev, "failed to set column address: %d\n", err); + return err; + } + + err = mipi_dsi_dcs_set_page_address(left, 0, mode->vdisplay - 1); + if (err < 0) { + dev_err(&left->dev, "failed to set page address: %d\n", err); + return err; + } + + err = mipi_dsi_dcs_set_column_address(right, mode->hdisplay / 2, + mode->hdisplay - 1); + if (err < 0) { + dev_err(&right->dev, "failed to set column address: %d\n", err); + return err; + } + + err = mipi_dsi_dcs_set_page_address(right, 0, mode->vdisplay - 1); + if (err < 0) { + dev_err(&right->dev, "failed to set page address: %d\n", err); + return err; + } + + return 0; +} + +static int sharp_panel_prepare(struct drm_panel *panel) +{ + struct sharp_panel *sharp = to_sharp_panel(panel); + u8 format = MIPI_DCS_PIXEL_FMT_24BIT; + int err; + + if (sharp->prepared) + return 0; + + err = regulator_enable(sharp->supply); + if (err < 0) + return err; + + usleep_range(10000, 20000); + + err = mipi_dsi_dcs_soft_reset(sharp->link1); + if (err < 0) { + dev_err(panel->dev, "soft reset failed: %d\n", err); + goto poweroff; + } + + msleep(120); + + err = mipi_dsi_dcs_exit_sleep_mode(sharp->link1); + if (err < 0) { + dev_err(panel->dev, "failed to exit sleep mode: %d\n", err); + goto poweroff; + } + + /* + * The MIPI DCS specification mandates this delay only between the + * exit_sleep_mode and enter_sleep_mode commands, so it isn't strictly + * necessary here. + */ + /* + msleep(120); + */ + + /* set left-right mode */ + err = sharp_panel_write(sharp, 0x1000, 0x2a); + if (err < 0) { + dev_err(panel->dev, "failed to set left-right mode: %d\n", err); + goto poweroff; + } + + /* enable command mode */ + err = sharp_panel_write(sharp, 0x1001, 0x01); + if (err < 0) { + dev_err(panel->dev, "failed to enable command mode: %d\n", err); + goto poweroff; + } + + err = mipi_dsi_dcs_set_pixel_format(sharp->link1, format); + if (err < 0) { + dev_err(panel->dev, "failed to set pixel format: %d\n", err); + goto poweroff; + } + + /* + * TODO: The device supports both left-right and even-odd split + * configurations, but this driver currently supports only the left- + * right split. To support a different mode a mechanism needs to be + * put in place to communicate the configuration back to the DSI host + * controller. + */ + err = sharp_setup_symmetrical_split(sharp->link1, sharp->link2, + sharp->mode); + if (err < 0) { + dev_err(panel->dev, "failed to set up symmetrical split: %d\n", + err); + goto poweroff; + } + + err = mipi_dsi_dcs_set_display_on(sharp->link1); + if (err < 0) { + dev_err(panel->dev, "failed to set display on: %d\n", err); + goto poweroff; + } + + sharp->prepared = true; + + return 0; + +poweroff: + regulator_disable(sharp->supply); + return err; +} + +static int sharp_panel_enable(struct drm_panel *panel) +{ + struct sharp_panel *sharp = to_sharp_panel(panel); + + if (sharp->enabled) + return 0; + + if (sharp->backlight) { + sharp->backlight->props.power = FB_BLANK_UNBLANK; + backlight_update_status(sharp->backlight); + } + + sharp->enabled = true; + + return 0; +} + +static const struct drm_display_mode default_mode = { + .clock = 278000, + .hdisplay = 2560, + .hsync_start = 2560 + 128, + .hsync_end = 2560 + 128 + 64, + .htotal = 2560 + 128 + 64 + 64, + .vdisplay = 1600, + .vsync_start = 1600 + 4, + .vsync_end = 1600 + 4 + 8, + .vtotal = 1600 + 4 + 8 + 32, + .vrefresh = 60, +}; + +static int sharp_panel_get_modes(struct drm_panel *panel) +{ + struct drm_display_mode *mode; + + mode = drm_mode_duplicate(panel->drm, &default_mode); + if (!mode) { + dev_err(panel->drm->dev, "failed to add mode %ux%ux@%u\n", + default_mode.hdisplay, default_mode.vdisplay, + default_mode.vrefresh); + return -ENOMEM; + } + + drm_mode_set_name(mode); + + drm_mode_probed_add(panel->connector, mode); + + panel->connector->display_info.width_mm = 217; + panel->connector->display_info.height_mm = 136; + + return 1; +} + +static const struct drm_panel_funcs sharp_panel_funcs = { + .disable = sharp_panel_disable, + .unprepare = sharp_panel_unprepare, + .prepare = sharp_panel_prepare, + .enable = sharp_panel_enable, + .get_modes = sharp_panel_get_modes, +}; + +static const struct of_device_id sharp_of_match[] = { + { .compatible = "sharp,lq101r1sx01", }, + { } +}; +MODULE_DEVICE_TABLE(of, sharp_of_match); + +static int sharp_panel_add(struct sharp_panel *sharp) +{ + struct device_node *np; + int err; + + sharp->mode = &default_mode; + + sharp->supply = devm_regulator_get(&sharp->link1->dev, "power"); + if (IS_ERR(sharp->supply)) + return PTR_ERR(sharp->supply); + + np = of_parse_phandle(sharp->link1->dev.of_node, "backlight", 0); + if (np) { + sharp->backlight = of_find_backlight_by_node(np); + of_node_put(np); + + if (!sharp->backlight) + return -EPROBE_DEFER; + } + + drm_panel_init(&sharp->base); + sharp->base.funcs = &sharp_panel_funcs; + sharp->base.dev = &sharp->link1->dev; + + err = drm_panel_add(&sharp->base); + if (err < 0) + goto put_backlight; + + return 0; + +put_backlight: + if (sharp->backlight) + put_device(&sharp->backlight->dev); + + return err; +} + +static void sharp_panel_del(struct sharp_panel *sharp) +{ + if (sharp->base.dev) + drm_panel_remove(&sharp->base); + + if (sharp->backlight) + put_device(&sharp->backlight->dev); + + if (sharp->link2) + put_device(&sharp->link2->dev); +} + +static int sharp_panel_probe(struct mipi_dsi_device *dsi) +{ + struct mipi_dsi_device *secondary = NULL; + struct sharp_panel *sharp; + struct device_node *np; + int err; + + dsi->lanes = 4; + dsi->format = MIPI_DSI_FMT_RGB888; + dsi->mode_flags = MIPI_DSI_MODE_LPM; + + /* Find DSI-LINK1 */ + np = of_parse_phandle(dsi->dev.of_node, "link2", 0); + if (np) { + secondary = of_find_mipi_dsi_device_by_node(np); + of_node_put(np); + + if (!secondary) + return -EPROBE_DEFER; + } + + /* register a panel for only the DSI-LINK1 interface */ + if (secondary) { + sharp = devm_kzalloc(&dsi->dev, sizeof(*sharp), GFP_KERNEL); + if (!sharp) { + put_device(&secondary->dev); + return -ENOMEM; + } + + mipi_dsi_set_drvdata(dsi, sharp); + + sharp->link2 = secondary; + sharp->link1 = dsi; + + err = sharp_panel_add(sharp); + if (err < 0) { + put_device(&secondary->dev); + return err; + } + } + + err = mipi_dsi_attach(dsi); + if (err < 0) { + if (secondary) + sharp_panel_del(sharp); + + return err; + } + + return 0; +} + +static int sharp_panel_remove(struct mipi_dsi_device *dsi) +{ + struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi); + int err; + + /* only detach from host for the DSI-LINK2 interface */ + if (!sharp) { + mipi_dsi_detach(dsi); + return 0; + } + + err = sharp_panel_disable(&sharp->base); + if (err < 0) + dev_err(&dsi->dev, "failed to disable panel: %d\n", err); + + err = mipi_dsi_detach(dsi); + if (err < 0) + dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err); + + drm_panel_detach(&sharp->base); + sharp_panel_del(sharp); + + return 0; +} + +static void sharp_panel_shutdown(struct mipi_dsi_device *dsi) +{ + struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi); + + /* nothing to do for DSI-LINK2 */ + if (!sharp) + return; + + sharp_panel_disable(&sharp->base); +} + +static struct mipi_dsi_driver sharp_panel_driver = { + .driver = { + .name = "panel-sharp-lq101r1sx01", + .of_match_table = sharp_of_match, + }, + .probe = sharp_panel_probe, + .remove = sharp_panel_remove, + .shutdown = sharp_panel_shutdown, +}; +module_mipi_dsi_driver(sharp_panel_driver); + +MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>"); +MODULE_DESCRIPTION("Sharp LQ101R1SX01 panel driver"); +MODULE_LICENSE("GPL v2");