On 11/03/2014 10:13 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("Sharp LQ101R1SX01 panel driver");
> +MODULE_LICENSE("GPL v2");
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
