Hi Linus Walleij,
On 13/05/22 03:21, Linus Walleij wrote:
On Fri, May 6, 2022 at 2:18 PM Joel Selvaraj <jo@xxxxxxxxxxx> 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
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?
So this utility macro should be in a generic file such as
include/drm/drm_mipi_dsi.h. (Can be added in a separate
patch.)
I agree. Could be done in another patch.
Third I think you need only one macro (see below).
+static int ebbg_ft8719_on(struct ebbg_ft8719 *ctx)
+{
+ struct mipi_dsi_device *dsi = ctx->dsi;
+ struct device *dev = &dsi->dev;
+ int ret;
+
+ dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
+ dsi_dcs_write_seq(dsi, 0x00, 0x00);
+ dsi_generic_write_seq(dsi, 0xff, 0x87, 0x19, 0x01);
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?
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.
> Doesn't it work to combine them into one call for each
> pair?
>> + dsi_dcs_write_seq(dsi, 0x00, 0x80);
>> + 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.
+ if (ctx->prepared)
+ return 0;
(...)
+ ctx->prepared = true;
+ return 0;
(...)
+ if (!ctx->prepared)
+ return 0;
(...)
+ ctx->prepared = false;
+ return 0;
Drop this state variable it is a reimplementation of something
that the core will track for you.
ok. Will drop them in the next version.
The rest looks nice!
Thanks for your review!
Yours,
Linus Walleij
Best Regards,
Joel Selvaraj
