From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH v3] drm/panel: add s6e3ha2 AMOLED panel driver Date: Tue, 3 Feb 2015 13:51:52 +0100 Message-ID: <20150203125150.GD15068@ulmo.nvidia.com> References: <1422323312-13306-1-git-send-email-human.hwang@samsung.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0798041375==" Return-path: Received: from mail-pa0-f43.google.com (mail-pa0-f43.google.com [209.85.220.43]) by gabe.freedesktop.org (Postfix) with ESMTP id B82096E051 for ; Tue, 3 Feb 2015 04:51:56 -0800 (PST) Received: by mail-pa0-f43.google.com with SMTP id eu11so95927225pac.2 for ; Tue, 03 Feb 2015 04:51:56 -0800 (PST) In-Reply-To: <1422323312-13306-1-git-send-email-human.hwang@samsung.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Hyungwon Hwang Cc: Sangbae Lee , Donghwa Lee , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0798041375== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="0/kgSOzhNoDC5T3a" Content-Disposition: inline --0/kgSOzhNoDC5T3a Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jan 27, 2015 at 10:48:32AM +0900, Hyungwon Hwang wrote: > From: Donghwa Lee >=20 > This patch adds MIPI-DSI based S6E3HA2 panel driver. This panel has > 1440x2560 resolution in 5.7-inch physical panel. >=20 > Signed-off-by: Donghwa Lee > Signed-off-by: Hyungwon Hwang > Cc: Inki Dae > Cc: Sangbae Lee > --- > Changes for v2: > - Fix errata in documentation and source code comments > Changes for v3: > - Remove the term LCD to clarify the sort of this panel > - Rename lcd-en-gpios to panel-en-gpios to clarify the sort of this panel > - Fix errata in documentation and source code comments > .../devicetree/bindings/panel/samsung,s6e3ha2.txt | 49 ++ > drivers/gpu/drm/panel/Kconfig | 6 + > drivers/gpu/drm/panel/Makefile | 1 + > drivers/gpu/drm/panel/panel-s6e3ha2.c | 513 +++++++++++++++= ++++++ > 4 files changed, 569 insertions(+) > create mode 100644 Documentation/devicetree/bindings/panel/samsung,s6e3h= a2.txt > create mode 100644 drivers/gpu/drm/panel/panel-s6e3ha2.c >=20 > diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3ha2.txt = b/Documentation/devicetree/bindings/panel/samsung,s6e3ha2.txt > new file mode 100644 > index 0000000..5210926 > --- /dev/null > +++ b/Documentation/devicetree/bindings/panel/samsung,s6e3ha2.txt > @@ -0,0 +1,49 @@ > +Samsung S6E3HA2 5.7" 1440x2560 AMOLED panel > + > +Required properties: > + - compatible: "samsung,s6e3ha2" > + - reg: the virtual channel number of a DSI peripheral > + - vdd3-supply: core voltage supply > + - vci-supply: voltage supply for analog circuits > + - reset-gpios: a GPIO spec for the reset pin > + - panel-en-gpios: a GPIO spec for the panel enable pin Why not "enable-gpios"? That would be much more in line with the reset-gpios property. > + - te-gpios: a GPIO spec for the tearing effect synchronization signal = gpio pin s/gpio/GPIO/ > + > +Optional properties: > + - display-timings: timings for the connected panel as described by [1] > + - panel-width-mm: physical panel width [mm] > + - panel-height-mm: physical panel height [mm] If these are optional, what happens if they aren't there? The panel is not likely to work in that case. Also I think these should be hard-coded in the driver, because they are implied by the "compatible" string. > +The device node can contain one 'port' child node with one child > +'endpoint' node, according to the bindings defined in [2]. This > +node should describe panel's video bus. The example doesn't show how to use that, why is it necessary? > diff --git a/drivers/gpu/drm/panel/panel-s6e3ha2.c b/drivers/gpu/drm/pane= l/panel-s6e3ha2.c [...] > +struct s6e3ha2 { > + struct device *dev; > + struct drm_panel panel; > + > + struct regulator_bulk_data supplies[2]; > + struct gpio_desc *reset_gpio; > + struct gpio_desc *panel_en_gpio; > + struct videomode vm; > + u32 width_mm; > + u32 height_mm; > + > + /* This field is tested by functions directly accessing DSI bus before > + * transfer, transfer is skipped if it is set. In case of transfer > + * failure or unexpected response the field is set to error value. > + * Such construct allows to eliminate many checks in higher level > + * functions. > + */ > + int error; I hate myself for not NAK'ing the first patch that introduced this idiom stronger. I think it's a really bad concept and you're not doing yourself any favours by using it. > +static void s6e3ha2_te_start_setting(struct s6e3ha2 *ctx) > +{ > + s6e3ha2_dcs_write_seq_static(ctx, 0xb9, 0x10, 0x09, 0xff, 0x00, 0x09); > +} > + > + Gratuituous blank line. > +static void s6e3ha2_panel_init(struct s6e3ha2 *ctx) > +{ > + s6e3ha2_dcs_write_seq_static(ctx, MIPI_DCS_EXIT_SLEEP_MODE); We have a standard function for this, please use it. > + /* common setting */ > + s6e3ha2_dcs_write_seq_static(ctx, MIPI_DCS_SET_TEAR_ON); Same here. > + s6e3ha2_test_key_off_f0(ctx); > + s6e3ha2_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_ON); And here. > +static int s6e3ha2_unprepare(struct drm_panel *panel) > +{ > + struct s6e3ha2 *ctx =3D panel_to_s6e3ha2(panel); > + > + s6e3ha2_dcs_write_seq_static(ctx, MIPI_DCS_ENTER_SLEEP_MODE); > + s6e3ha2_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_OFF); More standard functions. > + msleep(40); > + > + s6e3ha2_clear_error(ctx); > + > + return s6e3ha2_power_off(ctx); > +} > + > +static int s6e3ha2_power_on(struct s6e3ha2 *ctx) > +{ > + int ret; > + > + ret =3D regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies); > + if (ret < 0) > + return ret; > + > + msleep(25); > + > + gpiod_set_value(ctx->panel_en_gpio, 0); > + usleep_range(5000, 6000); > + gpiod_set_value(ctx->panel_en_gpio, 1); > + > + gpiod_set_value(ctx->reset_gpio, 1); > + usleep_range(5000, 6000); > + gpiod_set_value(ctx->reset_gpio, 0); > + usleep_range(5000, 6000); > + gpiod_set_value(ctx->reset_gpio, 1); I would expect the value after power on to be 0 for reset. Perhaps you need GPIO_ACTIVE_LOW in your device tree? Also why do you toggle thrice? I would assume that putting the peripheral into reset and taking it out again would be enough. > +static int s6e3ha2_prepare(struct drm_panel *panel) > +{ > + struct s6e3ha2 *ctx =3D panel_to_s6e3ha2(panel); > + int ret; > + > + ret =3D s6e3ha2_power_on(ctx); > + if (ret < 0) > + return ret; > + > + s6e3ha2_panel_init(ctx); > + if (ctx->error < 0) This is one of the reasons why ctx->error is a bad idea. It's completely unintuitive to check ctx->error here because nobody's actually setting it in this context. > + s6e3ha2_unprepare(panel); This is somewhat asymmetric. I would expect the s6e3ha2_panel_init() to undo what it did on failure, so that you would only have to call s6e3ha2_power_off() here and undo what you did in *this* function. > +static int s6e3ha2_enable(struct drm_panel *panel) > +{ > + return 0; > +} This is really where you're supposed to turn on the backlight or similar. Where does that happen? > +static int s6e3ha2_get_modes(struct drm_panel *panel) > +{ > + struct drm_connector *connector =3D panel->connector; > + struct s6e3ha2 *ctx =3D panel_to_s6e3ha2(panel); > + struct drm_display_mode *mode; > + > + mode =3D drm_mode_create(connector->dev); > + if (!mode) { > + DRM_ERROR("failed to create a new display mode\n"); > + return 0; > + } > + > + drm_display_mode_from_videomode(&ctx->vm, mode); > + mode->width_mm =3D ctx->width_mm; > + mode->height_mm =3D ctx->height_mm; > + connector->display_info.width_mm =3D mode->width_mm; > + connector->display_info.height_mm =3D mode->height_mm; > + > + mode->type =3D DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; > + drm_mode_probed_add(connector, mode); > + > + return 1; > +} Like I said before, the mode is implied by the compatible value, so no need to parse it from device tree. > +static int s6e3ha2_probe(struct mipi_dsi_device *dsi) > +{ > + struct device *dev =3D &dsi->dev; > + struct s6e3ha2 *ctx; > + int ret; > + > + ctx =3D devm_kzalloc(dev, sizeof(struct s6e3ha2), GFP_KERNEL); sizeof(*ctx) is much shorter. > + ctx->reset_gpio =3D devm_gpiod_get(dev, "reset"); > + if (IS_ERR(ctx->reset_gpio)) { > + dev_err(dev, "cannot get reset-gpios %ld\n", > + PTR_ERR(ctx->reset_gpio)); > + return PTR_ERR(ctx->reset_gpio); > + } > + ret =3D gpiod_direction_output(ctx->reset_gpio, 1); For consistency the above two lines should be separated by a blank line. > + if (ret < 0) { > + dev_err(dev, "cannot configure reset-gpios %d\n", ret); > + return ret; > + } > + > + ctx->panel_en_gpio =3D devm_gpiod_get(dev, "panel-en"); > + if (IS_ERR(ctx->panel_en_gpio)) { > + dev_err(dev, "cannot get panel-en-gpios %ld\n", > + PTR_ERR(ctx->panel_en_gpio)); > + return PTR_ERR(ctx->panel_en_gpio); > + } > + ret =3D gpiod_direction_output(ctx->panel_en_gpio, 1); Same here. > + if (ret < 0) { > + dev_err(dev, "cannot configure panel-en-gpios %d\n", ret); > + return ret; > + } You seem to be turning on the panel here. That's wrong because you're supposed to wait for the display driver to tell you to turn it on via ->prepare() and ->enable(). > + > + drm_panel_init(&ctx->panel); > + ctx->panel.dev =3D dev; > + ctx->panel.funcs =3D &s6e3ha2_drm_funcs; I don't see a use for the _drm in here. > +static struct of_device_id s6e3ha2_of_match[] =3D { static const, please. > + { .compatible =3D "samsung,s6e3ha2" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, s6e3ha2_of_match); > + > +static struct mipi_dsi_driver s6e3ha2_driver =3D { > + .probe =3D s6e3ha2_probe, > + .remove =3D s6e3ha2_remove, > + .driver =3D { > + .name =3D "panel_s6e3ha2", Please use a - instead of an _ here, for consistency with other drivers. > + .owner =3D THIS_MODULE, No need for this anymore. Thierry --0/kgSOzhNoDC5T3a Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJU0MRmAAoJEN0jrNd/PrOhtFMP/1i5sOVM80wUUxfcXrcOHz1v +YztKkA5U4h9u1/dma2Zul2obYEvsa6kWYWdvEHNtk61lFrqbtYBXne577SeSlut Siw53CT2Y5DWw5k5MciDdZHPPtNX9NSNlTenEH0NapFL6lZOXaxFfEdz58y9rwTH ODlFGRi8on9yJlKLSICi+Me2VMA1t75Qb+EHVaWMCRyZ7a4YSFXjIQcdDlHDDKXq EVRTrHgMNydOxTXufCgil2ZsKR9PFBviH4M6LTzwO6opUeWkn8v/J/8hTSXxmKq6 43jIF+aytImOdCj+M4d0fEb9Rxpe6Zmd92Ujtq3prPfftBv8ZfTG8TQc/cTPvFcg NMdm7Q2+FlNYAyntA4NF3f/wo4tKZDsXKkFlDs7mHPAMrUn1wzHpNdY0UQz2FPZI T1/guN/y2tqXGeMRdsBAIrhuVitLUBWkWkO6Y1Jc2Lu6jR/vrZHIPySotb8XxSel htNslKkk/hOIEqDPl36bBK+CTeZSEYt8d1YFkKQTvD9ltLux2d2itMpo5Ts6g14J KmRo0/tVC3CSNWQATy9es0JmeEYKyZtKILnZKfqFP0CULBfFjk0BYNBLIlzIoBq3 6/PXcA7B0PcJxBvLlLitjde3XfxTOHR5PCslTk6scVQJZW0OZ7Khn+hMCiksdDXa Dz6K03e6M2QfdFwJ9aw+ =Ey1l -----END PGP SIGNATURE----- --0/kgSOzhNoDC5T3a-- --===============0798041375== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============0798041375==--