On Mon, May 16, 2022 at 2:56 PM Joel Selvaraj jo@jsfamily.in wrote:
On 13/05/22 03:21, Linus Walleij wrote:
On Fri, May 6, 2022 at 2:18 PM Joel Selvaraj jo@jsfamily.in wrote:
+#define dsi_dcs_write_seq(dsi, seq...) do { \
static const u8 d[] = { seq }; \int ret; \ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \if (ret < 0) \return ret; \} while (0)First I don't see what the do {} while (0) buys you, just use a basic block {}.
The do {} while (0) in macro ensures there is a semicolon when it's used. With normal blocking, it would have issues with if conditions? I read about them here: https://stackoverflow.com/a/2381339
Hm that seems true, it enforces the semicolon ; at the end of the statement which is nice. I suppose we should fix the other macro as well.
Noralf added this ({}) form in 02dd95fe31693, so maybe he wants to chip in.
Second look at mipi_dbi_command() in include/drm/drm_mipi_dbi.h this is very similar.
Does the ({..}) style blocking used in mipi_dbi_command help workaround the semicolon issue I mentioned above?
Nope. But add the rate limited error print please!
It's dubious that you always have dsi_dcs_write_seq() followed by dsi_generic_write_seq().
That means mipi_dsi_generic_write() followed by mipi_dsi_dcs_write_buffer(). But if you look at these commands in drivers/gpu/drm/drm_mipi_dsi.c you see that they do the same thing!
They almost do the same thing except for the msg.type values? Mostly the msg.type value is used to just check whether it's a long or short write in the msm dsi_host code. However, in mipi_dsi_create_packet function, the msg->type value is used to calculate packet->header[0] as follows:
packet->header[0] = ((msg->channel & 0x3) << 6) | (msg->type & 0x3f);
Wouldn't the difference between the mipi_dsi_dcs_write_buffer's and mipi_dsi_generic_write's msg.type values cause issue here?
I tried using mipi_dsi_dcs_write_buffer for all commands and the panel worked fine, but I am not sure if it's correct to do so?
I think it's fine? The only issue would be if there is a DSI host controller that only supports short writes, and in that case it should emulate long writes by breaking long messages apart. (My amateur view at least.)
Lots of magic numbers. You don't have a datasheet do you? So you could #define some of the magic?
Unfortunately, I don't have a datasheet and the power on sequence is taken from downstream android dts. It works pretty well though. So I don't think I can #define any of these magic.
If you know which display controller the display is using (usually Novatek nnnnn, Ilitek nnnn etc someting like that) there is often a datasheet for the display controller available but the display per se often obscures the display controller.
Doesn't it work to combine them into one call for each pair?
dsi_dcs_write_seq(dsi, );dsi_generic_write_seq(dsi, 0xff, 0x87, 0x19);By using a macro? We can... but I am not sure what (0x00, 0x80), (0x00, 0xa0),etc type of commands signify without the datasheet, so I am not sure what to name them in the macro and make any sensible meaning out of it.
I meant just sending dsi_generic_write_seq() with everything in it:
dsi_generic_write_seq(dsi, 0x00, 0x80, 0xff, 0x87, 0x19);
Instead of two writes. Doesn't this work?
Yours, Linus Walleij