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

[abhinavk@codeaurora.org]

Hi Linus

Thanks for your valuable comments.

We agree with your ideas and will address them.

Some details below on how we will address them.

Thanks

Abhinav
On 2018-08-03 04:03, 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.
> 
Unfortunately, I do not have more info than this on each of them.
The documentation I have does not speak more in detail about what
each command does.

>> +       /* 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.
> 
After going through the documentation I have, Yes i agree that
this is a panel driver and can support other panels/resolutions.

Yes, since I dont have more documentation to add here will change
these sets to magic_set_1, magic_set_2 etc.

>> +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";
> 
> The command sequence is probably just applicable for these Qcom
> reference designs with a certain physical display, unless the display
> controller can self-describe, e.g. by electric straps.
> 
> So only run these command sets for wqxga etc for this specific
> qcom reference design, bail out if subsystem (reference design)
> compatible string is not defined for now and match on the reference
> design-display compatible.
> 
Yes, we agree to have two compatible strings, and shall bail out for now
if it doesnt match our reference design one similar to what is present 
here:

https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/panel/panel-ilitek-ili9322.c#L757

> Yours,
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel