From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH v5 1/2] drm/panel: Add support for Truly NT35597 panel Date: Thu, 9 Aug 2018 12:53:49 +0200 Message-ID: <20180809105349.GD21639@ulmo> References: <1533264572-6364-1-git-send-email-abhinavk@codeaurora.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1015382072==" Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Linus Walleij Cc: linux-arm-msm@vger.kernel.org, "open list:DRM PANEL DRIVERS" , hoegsberg@google.com, abhinavk@codeaurora.org, chandanu@codeaurora.org List-Id: linux-arm-msm@vger.kernel.org --===============1015382072== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="SO98HVl1bnMOfKZd" Content-Disposition: inline --SO98HVl1bnMOfKZd Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Aug 03, 2018 at 01:03:45PM +0200, Linus Walleij wrote: > On Fri, Aug 3, 2018 at 4:49 AM Abhinav Kumar wr= ote: >=20 > Hi Abhinav, >=20 > > From: "abhinavk@codeaurora.org" > > > > Add support for Truly NT35597 panel used > > in MSM reference platforms. > > > > This panel supports both single DSI and dual DSI > > modes. > > > > However, this patch series adds support only for > > dual DSI mode. > > > > Changes in v5: > > - Added comments for the delays > > - Fix error messages and return code > > - Start using backlight_enable/disable helpers > > - Start using ARRAY_SIZE everywhere > > - Split the panel commands into three sets to > > remove redundant structure fields and simplify > > the DCS command sending method > > - Use of_get_drm_display_mode() and simplify > > get_modes function > > - Remove truly_wqxga_panel_del and do necessary > > cleanup > > - Replace dev_err with DRM_DEV_ERROR > > > > Signed-off-by: Archit Taneja > > Signed-off-by: Abhinav Kumar >=20 > Overall this driver looks good to me. >=20 > Just a question: >=20 > > +struct cmd_set panel_cmds_set_1[] =3D { > > + /* CMD2_P0 */ > > + { { 0xff, 0x20 } }, > > + { { 0xfb, 0x01 } }, > > + { { 0x00, 0x01 } }, >=20 > This is what we call a jam table, I guess "magic init sequence". >=20 > There are some comments on what the different sections do, but in > cases like this where there is no public datasheet, it would be nice > to use some #defines rather than opaque hex codes, if you know what > the different commands actually mean. >=20 > This is in order to help others with hacking on the driver. >=20 > If you don't have more info than this it's fine, just asking. >=20 > > + /* Resolution:1440x2560 */ > > + { { 0x72, 0x02 } }, >=20 > This is for example quite hard-coded. One gets the idea that the > resolution is dynamic and that this is not really a panel per se but > a panel driver, so the Truly NT35597 is not a panel but a panel driver > that can be configured to be used with several physical panels. >=20 > Compare to other panel drivers such as Ilitek ILI9322 that is in this > driver dir. There I make it a bit more transparent what the panel driver > is actually doing on the inside, so if we find it is used with other > physical panels we can reuse the code more easily. >=20 > > + truly_write_buf_func(ret, truly_dcs_write_buf, > > + panel, SHORT_PACKET, > > + ARRAY_SIZE(panel_cmds_set_1), > > + panel_cmds_set_1); >=20 > Instead of calling these "cmd_set_1" name them after what the > command set actually does so we can follow the init flow. > If you don't know what the commands do you could as well > call it "magic 1", "magic 2" etc so we know it is magic. >=20 > > +static const struct of_device_id truly_wqxga_of_match[] =3D { > > + { .compatible =3D "truly,nt35597", }, >=20 > If this is a panel driver that not only configurable for wqxga but actual= ly > also other resolutions this is misleading. >=20 > I suspect this is indeed a panel driver and not a panel with integrated > driver. I think the best is to define two compatible strings like > we do for ILI9322: > "truly,nt35597", "qcom,reference-design-name-display"; I don't understand why we need the two compatible strings for this. Having "truly,nt35597" isn't quite correct in that case, because in itself that doesn't contain enough information for any programming. If that chip can indeed be used to drive different panels, what we really need is the a compatible string that describes the complete assembly. In the driver we could then rely on parameterized common code that the panel driver can call into in order to program the driver chip. Note that a driver is assumed to know what to do with a device that it gets bound to based on the compatible string. If some OS implements a driver for "truly,nt35597" but not "qcom,reference-design-name-display", then what is it supposed to do if it encounters the above list of compatible strings? It can't really program a mode because its missing essential information such as resolution or timings. Thierry --SO98HVl1bnMOfKZd Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAltsHT0ACgkQ3SOs138+ s6ERihAApeZrRhzckYnw8HQwmwJXfYNh40RUs22HcXaKp+TPpE9ZTLm2VkcofRCZ yMIVeYhMuc4md31Qtcj1DGJEoc8vY/BgQjKgtEt62KtrUinuZNorew7MmBX9/7Ju CkeU7RCJV8L5fnT96Y7ibuXCwucjwIzYtYOziw5M1mQGUWsQNLlCgFS3fke1Jr1n k98LdCgcxrwzI5yrfvn/aharmwz0MSp/2746dwQuKJPDrAhq8ZG3ni8H8OaTXh8h 0XvtetKl9i8mbmgAZtMVp2NBJGgDqxWrGPmFa+94kSZb9Pg07auXWQ/6ZKegk8ll Vx3Jt+EkQOZJGw4CENP6E2UvFeahtbiDH7PYIsZsq6TDNDhHr17YqpgU9CJqgYZV dvCFUjl0t1IBXCX2HJRlO1UUxOIyeAwnJtWs3LrSb9PlJL5821q4K2GzOEVS4H3R zV38uNiDehXJyQeq8liYaLGROx1Q8/xKEc1N+OZfBS1kjQ7Ke8GzGfT/Zi2wV2JZ nJq9t5vv5+L8kxYN2P5Sl5saiOUHS/jL+d7k40E8uWGdlx9aeSCI6bm2R3v8FZBT POp5AxVeRRIEoIg+UB/Y56VsK5RqPhpACbZfxw5hHGNgE9CfoVGNOQXeFDWeffv0 JX8lFjqatXsZmrD2jLAO2b3vWTyKRju81Ja7c23sgwSR1wy7eMU= =hNKR -----END PGP SIGNATURE----- --SO98HVl1bnMOfKZd-- --===============1015382072== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1015382072==--