On Fri, Aug 7, 2015 at 9:19 AM, Thierry Reding <thierry.reding@xxxxxxxxx> wrote: > On Tue, Jul 21, 2015 at 03:36:02PM -0400, Rob Clark wrote: >> This is one of several different panels that are used on the Sony Xperia >> Z3 phone. It can operate in either command or video mode, although so >> far only video mode is implemented (since that is the mode that the >> downstream kernel version I happened to be looking at was using, and >> that is where I got the programming sequences for the panel). >> >> Signed-off-by: Rob Clark <robdclark@xxxxxxxxx> >> --- >> Couple Notes: >> 1) programming sequences and basically everything I know about the >> panel came from downstream android kernel. I've started a wiki >> page to document how to translate from downstream kernel-msm way >> of doing things to upstream panel framework, which may be useful >> for others wanting to port downstream panel drivers for snapdragon >> devices: >> >> https://github.com/freedreno/freedreno/wiki/DSI-Panel-Driver-Porting >> >> 2) The Sony phones at least (not sure if this is common) use one of >> several different panels, with runtime probing. Depending on the >> device they seem to either use a gpio (simple) or send some DSI >> commands to read back the panel-id. My rough thinking here about >> how to handle this is to implement a "panel-meta" driver (or maybe >> one each for the different probing methods), which would take a >> list of phandles to the actual candidate panels, and fwd the panel >> fxn calls to the chosen panel after probing. >> >> .../bindings/panel/auo,novatek-1080p.txt | 25 ++ >> drivers/gpu/drm/panel/Kconfig | 9 + >> drivers/gpu/drm/panel/Makefile | 1 + >> drivers/gpu/drm/panel/panel-auo-novatek-1080p.c | 470 +++++++++++++++++++++ >> 4 files changed, 505 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/panel/auo,novatek-1080p.txt >> create mode 100644 drivers/gpu/drm/panel/panel-auo-novatek-1080p.c >> >> diff --git a/Documentation/devicetree/bindings/panel/auo,novatek-1080p.txt b/Documentation/devicetree/bindings/panel/auo,novatek-1080p.txt >> new file mode 100644 >> index 0000000..8a53093 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/panel/auo,novatek-1080p.txt >> @@ -0,0 +1,25 @@ >> +AU Optronics Corporation 1080x1920 DSI panel >> + >> +This panel supports both video and command mode (although currently only video >> +mode is implemented in the driver. >> + >> +Required properties: >> +- compatible: should be "auo,novatek-1080p-vid" > > This looks a little generic for a compatible string. Can't we get at the > specific panel model number that's been used? What if AUO ever produced > some other Novatek panel with a 1080p resolution? Maybe Sony or someone else can chime in? That somewhat generic name was all I could get from downstream android kernel. I'm sure there is a better possible name, although I have no means to find that out myself. > Also, what's the -vid suffix for? the same panel seems to also work in cmd mode.. so idea was to have -vid and -cmd compat strings to choose which mode to operate in. >> +Optional properties: >> +- power-supply: phandle of the regulator that provides the supply voltage >> +- reset-gpio: phandle of gpio for reset line >> +- backlight: phandle of the backlight device attached to the panel >> + >> +Example: >> + >> + dsi@54300000 { >> + panel: panel@0 { >> + compatible = "auo,novatek-1080p-vid"; >> + reg = <0>; >> + >> + power-supply = <...>; >> + reset-gpio = <...>; >> + backlight = <...>; >> + }; >> + }; >> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig >> index 6d64c7b..89f0e8c 100644 >> --- a/drivers/gpu/drm/panel/Kconfig >> +++ b/drivers/gpu/drm/panel/Kconfig >> @@ -43,4 +43,13 @@ config DRM_PANEL_SHARP_LQ101R1SX01 >> To compile this driver as a module, choose M here: the module >> will be called panel-sharp-lq101r1sx01. >> >> +config DRM_PANEL_AUO_NOVATEK_1080P >> + tristate "AUO Novatek 1080p video mode panel" >> + depends on OF >> + depends on DRM_MIPI_DSI >> + depends on BACKLIGHT_CLASS_DEVICE >> + help >> + Say Y here if you want to enable support for AUO 1080p DSI panel >> + as found in some Sony XPERIA Z3 devices >> + > > Can we sort this alphabetically, please? > >> endmenu >> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile >> index 4b2a043..cbcfedf 100644 >> --- a/drivers/gpu/drm/panel/Makefile >> +++ b/drivers/gpu/drm/panel/Makefile >> @@ -2,3 +2,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 >> +obj-$(CONFIG_DRM_PANEL_AUO_NOVATEK_1080P) += panel-auo-novatek-1080p.o > > This too. > > Actually, nevermind. I have local patches to add vendor prefixes more > consistently so that we can actually sort properly. I can fix that up > in your patch when I apply. > >> diff --git a/drivers/gpu/drm/panel/panel-auo-novatek-1080p.c b/drivers/gpu/drm/panel/panel-auo-novatek-1080p.c >> +static int auo_panel_init(struct auo_panel *auo) >> +{ >> + struct mipi_dsi_device *dsi = auo->dsi; >> + int ret; >> + >> + dsi->mode_flags |= MIPI_DSI_MODE_LPM; >> + >> + ret = mipi_dsi_generic_write(dsi, (u8[]){ 0xb0, 0x04 }, 2); > > I find this notation hard to read. Have you considered moving this into > some sort of table that you can loop through? Or perhaps add some > helpers, say, mipi_dsi_generic_writeb() and mipi_dsi_dcs_writeb() to > help make this more readable? > Yeah, helper macro thing might be a reasonable idea. The table option makes it hard to use the helpers for things that are not non-standard, or when you need delays, etc.. >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_write(dsi, 0xff, (u8[]){ 0xe0 }, 1); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_write(dsi, 0xfb, (u8[]){ 0x01 }, 1); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_write(dsi, 0xb5, (u8[]){ 0x86 }, 1); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_write(dsi, 0xb6, (u8[]){ 0x77 }, 1); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_write(dsi, 0xb8, (u8[]){ 0xad }, 1); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_write(dsi, 0xff, (u8[]){ 0x20 }, 1); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_write(dsi, 0xfb, (u8[]){ 0x01 }, 1); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_enter_sleep_mode(dsi); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_write(dsi, 0xff, (u8[]){ 0x24 }, 1); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_write(dsi, 0xfb, (u8[]){ 0x01 }, 1); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_write(dsi, 0xc6, (u8[]){ 0x00 }, 1); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_write(dsi, 0xc5, (u8[]){ 0x32 }, 1); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_write(dsi, 0x92, (u8[]){ 0x92 }, 1); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_write(dsi, 0xff, (u8[]){ 0x10 }, 1); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_set_tear_on(dsi, MIPI_DSI_DCS_TEAR_MODE_VBLANK); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_SCANLINE, >> + (u8[]){ 0x03, 0x00 }, 2); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_write(dsi, 0x3b, (u8[]){ 0x03, 0x30, 0x06 }, 3); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_write(dsi, 0xbb, (u8[]){ 0x10 }, 1); >> + if (ret < 0) >> + return ret; >> + msleep(1); >> + >> + ret = mipi_dsi_dcs_exit_sleep_mode(dsi); >> + if (ret < 0) >> + return ret; >> + msleep(30); >> + >> + return 0; >> +} >> + >> +static int auo_panel_on(struct auo_panel *auo) >> +{ >> + struct mipi_dsi_device *dsi = auo->dsi; >> + int ret; >> + >> + dsi->mode_flags |= MIPI_DSI_MODE_LPM; > > This is weird. > >> + ret = mipi_dsi_dcs_set_display_on(dsi); >> + if (ret < 0) >> + return ret; >> + >> + msleep(40); >> + >> + return 0; >> +} >> + >> +static int auo_panel_off(struct auo_panel *auo) >> +{ >> + struct mipi_dsi_device *dsi = auo->dsi; >> + int ret; >> + >> + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; > > And this even more. Doesn't the panel work when you simply send > everything in low-power mode? I wouldn't expect low power mode to have enough bandwidth for the video signal.. but otoh it seems like I need to use lpm for power-on/off sequence. Maybe we should wrap that up in a helper to enable/disable lpm? But that seemed a bit overkill. >> +static int auo_panel_prepare(struct drm_panel *panel) >> +{ >> + struct auo_panel *auo = to_auo_panel(panel); >> + int ret; >> + >> + if (auo->prepared) >> + return 0; >> + >> + DRM_DEBUG("prepare\n"); >> + >> + if (auo->reset_gpio) { >> + gpiod_set_value(auo->reset_gpio, 0); >> + msleep(5); >> + } >> + >> + ret = regulator_enable(auo->supply); >> + if (ret < 0) >> + return ret; >> + >> + msleep(20); >> + >> + if (auo->reset_gpio) { >> + gpiod_set_value(auo->reset_gpio, 1); >> + msleep(10); >> + } >> + >> + msleep(150); >> + >> + ret = auo_panel_init(auo); >> + if (ret) { >> + dev_err(panel->dev, "failed to init panel: %d\n", ret); >> + goto poweroff; >> + } >> + >> + ret = auo_panel_on(auo); >> + if (ret) { >> + dev_err(panel->dev, "failed to set panel on: %d\n", ret); >> + goto poweroff; >> + } >> + >> + auo->prepared = true; >> + >> + return 0; >> + >> +poweroff: >> + regulator_disable(auo->supply); >> + if (auo->reset_gpio) >> + gpiod_set_value(auo->reset_gpio, 0); > > You should be able to do without the check here, because > gpiod_set_value() will simply nop if the GPIO is NULL. ok > I assume you may not want to do it above because of the additional > delays that are only relevant if you have a reset GPIO. > >> + return ret; >> +} >> + >> +static int auo_panel_enable(struct drm_panel *panel) >> +{ >> + struct auo_panel *auo = to_auo_panel(panel); >> + >> + if (auo->enabled) >> + return 0; >> + >> + DRM_DEBUG("enable\n"); >> + >> + if (auo->backlight) { >> + auo->backlight->props.power = FB_BLANK_UNBLANK; >> + backlight_update_status(auo->backlight); >> + } >> + >> + auo->enabled = true; >> + >> + return 0; >> +} >> + >> +static int auo_panel_add(struct auo_panel *auo) >> +{ >> + struct device *dev= &auo->dsi->dev; >> + struct device_node *np; >> + int ret; >> + >> + auo->mode = &default_mode; > > This seems to be unused. yeah, I think just cargo cult'd from sharp panel.. >> + >> + auo->supply = devm_regulator_get(dev, "power"); >> + if (IS_ERR(auo->supply)) >> + return PTR_ERR(auo->supply); >> + >> + auo->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); >> + if (IS_ERR(auo->reset_gpio)) { >> + dev_err(dev, "cannot get reset-gpios %ld\n", >> + PTR_ERR(auo->reset_gpio)); >> + auo->reset_gpio = NULL; >> + } else { >> + gpiod_direction_output(auo->reset_gpio, 0); > > Isn't that what GPIOD_OUT_LOW already does? > hmm, maybe? BR, -R _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
