Re: [PATCH v5 1/2] drm/panel: Add support for Truly NT35597 panel

[Sean Paul <seanpaul@chromium.org>]

On Thu, Aug 09, 2018 at 12:53:49PM +0200, Thierry Reding wrote:
> On Fri, Aug 03, 2018 at 01:03:45PM +0200, Linus Walleij wrote:
> > On Fri, Aug 3, 2018 at 4:49 AM Abhinav Kumar <abhinavk@codeaurora.org> wrote:
> > 
> > Hi Abhinav,
> > 
> > > From: "abhinavk@codeaurora.org" <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 <architt@codeaurora.org>
> > > Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> > 
> > Overall this driver looks good to me.
> > 
> > Just a question:
> > 
> > > +struct cmd_set panel_cmds_set_1[] = {
> > > +       /* CMD2_P0 */
> > > +       { { 0xff, 0x20 } },
> > > +       { { 0xfb, 0x01 } },
> > > +       { { 0x00, 0x01 } },
> > 
> > This is what we call a jam table, I guess "magic init sequence".
> > 
> > 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.
> > 
> > This is in order to help others with hacking on the driver.
> > 
> > If you don't have more info than this it's fine, just asking.
> > 
> > > +       /* Resolution:1440x2560 */
> > > +       { { 0x72, 0x02 } },
> > 
> > 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.
> > 
> > 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.
> > 
> > > +       truly_write_buf_func(ret, truly_dcs_write_buf,
> > > +               panel, SHORT_PACKET,
> > > +               ARRAY_SIZE(panel_cmds_set_1),
> > > +               panel_cmds_set_1);
> > 
> > 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.
> > 
> > > +static const struct of_device_id truly_wqxga_of_match[] = {
> > > +       { .compatible = "truly,nt35597", },
> > 
> > If this is a panel driver that not only configurable for wqxga but actually
> > also other resolutions this is misleading.
> > 
> > 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.

+1 (fwiw)

I doubt the generic string is ever going to be useful on its own, but it could
always be added later with a refactor of the code to tease out the generic bits.
Given that the TCON vendor won't give up the docs for the magic initialization
sequance, getting to a generic implementation is going to take a bit of work
anyways.

Sean

> 
> 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



-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel