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@xxxxxxxxxxxxxx>
wrote:
Hi Abhinav,
From: "abhinavk@xxxxxxxxxxxxxx" <abhinavk@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxx>
Signed-off-by: Abhinav Kumar <abhinavk@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
