From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH] drm/panel: auo novatek 1080p video mode panel Date: Fri, 7 Aug 2015 15:19:33 +0200 Message-ID: <20150807131931.GA27639@ulmo> References: <1437507362-20162-1-git-send-email-robdclark@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1469532065==" Return-path: In-Reply-To: <1437507362-20162-1-git-send-email-robdclark@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Rob Clark Cc: linux-arm-msm@vger.kernel.org, Bjorn Andersson , dri-devel@lists.freedesktop.org List-Id: linux-arm-msm@vger.kernel.org --===============1469532065== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ZGiS0Q5IWpPtfppv" Content-Disposition: inline --ZGiS0Q5IWpPtfppv Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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). >=20 > Signed-off-by: Rob Clark > --- > 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: >=20 > https://github.com/freedreno/freedreno/wiki/DSI-Panel-Driver-Porting >=20 > 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. >=20 > .../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-1= 080p.txt > create mode 100644 drivers/gpu/drm/panel/panel-auo-novatek-1080p.c >=20 > diff --git a/Documentation/devicetree/bindings/panel/auo,novatek-1080p.tx= t 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? Also, what's the -vid suffix for? > +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 =3D "auo,novatek-1080p-vid"; > + reg =3D <0>; > + > + power-supply =3D <...>; > + reset-gpio =3D <...>; > + backlight =3D <...>; > + }; > + }; > 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. > =20 > +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/Makef= ile > 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) +=3D panel-simple.o > obj-$(CONFIG_DRM_PANEL_LD9040) +=3D panel-ld9040.o > obj-$(CONFIG_DRM_PANEL_S6E8AA0) +=3D panel-s6e8aa0.o > obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) +=3D panel-sharp-lq101r1sx01.o > +obj-$(CONFIG_DRM_PANEL_AUO_NOVATEK_1080P) +=3D 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/gp= u/drm/panel/panel-auo-novatek-1080p.c > +static int auo_panel_init(struct auo_panel *auo) > +{ > + struct mipi_dsi_device *dsi =3D auo->dsi; > + int ret; > + > + dsi->mode_flags |=3D MIPI_DSI_MODE_LPM; > + > + ret =3D 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? > + if (ret < 0) > + return ret; > + > + ret =3D mipi_dsi_dcs_write(dsi, 0xff, (u8[]){ 0xe0 }, 1); > + if (ret < 0) > + return ret; > + > + ret =3D mipi_dsi_dcs_write(dsi, 0xfb, (u8[]){ 0x01 }, 1); > + if (ret < 0) > + return ret; > + > + ret =3D mipi_dsi_dcs_write(dsi, 0xb5, (u8[]){ 0x86 }, 1); > + if (ret < 0) > + return ret; > + > + ret =3D mipi_dsi_dcs_write(dsi, 0xb6, (u8[]){ 0x77 }, 1); > + if (ret < 0) > + return ret; > + > + ret =3D mipi_dsi_dcs_write(dsi, 0xb8, (u8[]){ 0xad }, 1); > + if (ret < 0) > + return ret; > + > + ret =3D mipi_dsi_dcs_write(dsi, 0xff, (u8[]){ 0x20 }, 1); > + if (ret < 0) > + return ret; > + > + ret =3D mipi_dsi_dcs_write(dsi, 0xfb, (u8[]){ 0x01 }, 1); > + if (ret < 0) > + return ret; > + > + ret =3D mipi_dsi_dcs_enter_sleep_mode(dsi); > + if (ret < 0) > + return ret; > + > + ret =3D mipi_dsi_dcs_write(dsi, 0xff, (u8[]){ 0x24 }, 1); > + if (ret < 0) > + return ret; > + > + ret =3D mipi_dsi_dcs_write(dsi, 0xfb, (u8[]){ 0x01 }, 1); > + if (ret < 0) > + return ret; > + > + ret =3D mipi_dsi_dcs_write(dsi, 0xc6, (u8[]){ 0x00 }, 1); > + if (ret < 0) > + return ret; > + > + ret =3D mipi_dsi_dcs_write(dsi, 0xc5, (u8[]){ 0x32 }, 1); > + if (ret < 0) > + return ret; > + > + ret =3D mipi_dsi_dcs_write(dsi, 0x92, (u8[]){ 0x92 }, 1); > + if (ret < 0) > + return ret; > + > + ret =3D mipi_dsi_dcs_write(dsi, 0xff, (u8[]){ 0x10 }, 1); > + if (ret < 0) > + return ret; > + > + ret =3D mipi_dsi_dcs_set_tear_on(dsi, MIPI_DSI_DCS_TEAR_MODE_VBLANK); > + if (ret < 0) > + return ret; > + > + ret =3D mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_SCANLINE, > + (u8[]){ 0x03, 0x00 }, 2); > + if (ret < 0) > + return ret; > + > + ret =3D mipi_dsi_dcs_write(dsi, 0x3b, (u8[]){ 0x03, 0x30, 0x06 }, 3); > + if (ret < 0) > + return ret; > + > + ret =3D mipi_dsi_dcs_write(dsi, 0xbb, (u8[]){ 0x10 }, 1); > + if (ret < 0) > + return ret; > + msleep(1); > + > + ret =3D 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 =3D auo->dsi; > + int ret; > + > + dsi->mode_flags |=3D MIPI_DSI_MODE_LPM; This is weird. > + ret =3D 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 =3D auo->dsi; > + int ret; > + > + dsi->mode_flags &=3D ~MIPI_DSI_MODE_LPM; And this even more. Doesn't the panel work when you simply send everything in low-power mode? > +static int auo_panel_prepare(struct drm_panel *panel) > +{ > + struct auo_panel *auo =3D 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 =3D 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 =3D auo_panel_init(auo); > + if (ret) { > + dev_err(panel->dev, "failed to init panel: %d\n", ret); > + goto poweroff; > + } > + > + ret =3D auo_panel_on(auo); > + if (ret) { > + dev_err(panel->dev, "failed to set panel on: %d\n", ret); > + goto poweroff; > + } > + > + auo->prepared =3D 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. 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 =3D to_auo_panel(panel); > + > + if (auo->enabled) > + return 0; > + > + DRM_DEBUG("enable\n"); > + > + if (auo->backlight) { > + auo->backlight->props.power =3D FB_BLANK_UNBLANK; > + backlight_update_status(auo->backlight); > + } > + > + auo->enabled =3D true; > + > + return 0; > +} > + > +static int auo_panel_add(struct auo_panel *auo) > +{ > + struct device *dev=3D &auo->dsi->dev; > + struct device_node *np; > + int ret; > + > + auo->mode =3D &default_mode; This seems to be unused. > + > + auo->supply =3D devm_regulator_get(dev, "power"); > + if (IS_ERR(auo->supply)) > + return PTR_ERR(auo->supply); > + > + auo->reset_gpio =3D 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 =3D NULL; > + } else { > + gpiod_direction_output(auo->reset_gpio, 0); Isn't that what GPIOD_OUT_LOW already does? --ZGiS0Q5IWpPtfppv Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJVxLBgAAoJEN0jrNd/PrOhZzUP/RZ2q46APA8P9OF9QEXKec7v dASsw/gc+LejwRdTJC4UgEzERRwoENjrLQH8YiW4J69v9oMGDUit24/lEqYMkmPV OOJN/3qxl0KVnsaFYygcDt0DyUzEv+5h9EGhNow4ISKTYwTvdBK+H57bVOxYEDFC sCpYLqFHqB7PhS6zC6cvnhnwd+HD2AnHiVisEBtlkch8LhPNkuJqifARtxmycp2w NNuHrl5SF8GbpimUZI0G6+uDf1Wyg3I8qAvIvx1mKnm06w17Hn5+pKJSanR7rE+S nGHU9YLA/EtminEjJjik/U2bD/jWKAaTL46yJBe4ai8svfwGw5ZLKK7zcxAKa7Wn Tn2KWIfU10e8kqEXUrjfMtje4lnp7oR6liiZKy/CQMtHemxdwimXxlR6Ix6QAH1F TkYTa5N/qZ9jOqZSDhcgG3ZTQgGdlMBZgMjqhLINIMDWS8CPyQN1G6NK+n5q8Uun n3QcKnMk7FBgx9LYMciMjZeMOPJi2uUmtB7p5MXacxE/WCuJe9tjSmI+iSWejZAo rElgn74TZpltTumoImKnnp1G89oEnPZdmPtgkW78vzqbAyNb2gVyQmhxHmNloI+m Fk6BzTCcMN9bc5d66lVa02TeAMwcXPdysV0KCChJgEJiHHvDXi+HQFDlCmb6PsEr 9dpWl/TphoYbcuG8BEY6 =JeQT -----END PGP SIGNATURE----- --ZGiS0Q5IWpPtfppv-- --===============1469532065== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============1469532065==--