Re: [PATCH 3/3] drm/panel: introduce ebbg,ft8719 panel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux