dt-bindings: vendor-prefixes: Add prefix for EBBG

Add a prefix for EBBG. They manufacture displays which are used in some Xiaomi phones, but I could not find much details about the company.

Signed-off-by Joel Selvaraj <[email protected]>

Add bindings for the EBBG FT8719 6.18" 2246x1080 DSI video mode panel, which can be found on some Xiaomi Poco F1 phones. The backlight is managed through the QCOM WLED driver.

Signed-off-by Joel Selvaraj <[email protected]>

---
 .../bindings/display/panel/ebbg,ft8719.yaml   | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/ebbg,ft8719.yaml

Documentation/devicetree/bindings/display/panel/ebbg,ft8719.yaml new file +15 -0

new file mode 100644
index 000000000000..fac6c9692c55
@@ -0,0 +1,78 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/ebbg,ft8719.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: EBBG FT8719 MIPI-DSI LCD panel
+
+maintainers:
+  - Joel Selvaraj <[email protected]>
+
+description: |
+  The FT8719 panel from EBBG is a FHD+ LCD display panel with a resolution
+  of 1080x2246. It is a video mode DSI panel. The backlight is managed
+  through the QCOM WLED driver.

Documentation/devicetree/bindings/display/panel/ebbg,ft8719.yaml new file +4 -0

+
+required:
+  - compatible
+  - reg

Documentation/devicetree/bindings/display/panel/ebbg,ft8719.yaml line 40

Quoted 2 lines

Documentation/devicetree/bindings/display/panel/ebbg,ft8719.yaml new file +2 -0

+  backlight: true
+  port: true

Both should not be needed - they come from panel-common.yaml. They might stay in list

Hi Krzysztof Kozlowski,

A quick question.

On 07/05/22 21:02, Krzysztof Kozlowski wrote:

Quoted 5 lines

Documentation/devicetree/bindings/display/panel/ebbg,ft8719.yaml line 40

Quoted 2 lines

Documentation/devicetree/bindings/display/panel/ebbg,ft8719.yaml new file +2 -0

+  backlight: true
+  port: true

Both should not be needed - they come from panel-common.yaml. They might stay in list

I see that almost 54 panels mention "port: true" and 46 panels mention "backlight: true". Almost all panels refer the panel-common.yaml too.

So I think specifying them as true is just for extra clarity that these properties are usually used by this panel? But I am not very sure.

Should I leave them be? or it's still recommended to remove them?

Add DRM panel driver for EBBG FT8719 6.18" 2246x1080 DSI video mode panel, which can be found on some Xiaomi Poco F1 phones. The panel's backlight is managed through QCOM WLED driver.

Signed-off-by Joel Selvaraj <[email protected]>

drivers/gpu/drm/panel/panel-ebbg-ft8719.c new file +5 -0

+#define dsi_generic_write_seq(dsi, seq...) do {                                \
+               static const u8 d[] = { seq };                          \
+               int ret;                                                \
+               ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d));    \
+               if (ret < 0)                                            \

(quoted diff) +2 -0

+       if (ctx->prepared)
+               return 0;

(quoted diff) +2 -0

+       ctx->prepared = true;
+       return 0;

(quoted diff) +2 -0

+       if (!ctx->prepared)
+               return 0;

drivers/gpu/drm/panel/panel-ebbg-ft8719.c line 62

Quoted 7 lines · Joel Selvaraj

drivers/gpu/drm/panel/panel-ebbg-ft8719.c new file +7 -0

+#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 {}.

Second look at mipi_dbi_command() in include/drm/drm_mipi_dbi.h this is very similar.

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.)

Third I think you need only one macro (see below).

drivers/gpu/drm/panel/panel-ebbg-ft8719.c line 83

Quoted 10 lines

drivers/gpu/drm/panel/panel-ebbg-ft8719.c new file +10 -0

+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!

Lots of magic numbers. You don't have a datasheet do you? So you could #define some of the magic?

Doesn't it work to combine them into one call for each pair?

Quoted 2 lines

drivers/gpu/drm/panel/panel-ebbg-ft8719.c new file +2 -0

+       dsi_dcs_write_seq(dsi, 0x00, 0x80);
+       dsi_generic_write_seq(dsi, 0xff, 0x87, 0x19);

Quoted 3 lines

(quoted diff) +2 -0

+       if (ctx->prepared)
+               return 0;

(...)

Quoted 2 lines

(quoted diff) +2 -0

+       ctx->prepared = true;
+       return 0;

(...)

Quoted 2 lines

(quoted diff) +2 -0

+       if (!ctx->prepared)
+               return 0;

(...)

drivers/gpu/drm/panel/panel-ebbg-ft8719.c line 230

Quoted 2 lines

drivers/gpu/drm/panel/panel-ebbg-ft8719.c new file +2 -0

+       ctx->prepared = false;
+       return 0;

Drop this state variable it is a reimplementation of something that the core will track for you.

The rest looks nice!

Yours,
Linus Walleij

Quoted 9 lines · Linus Walleij

Quoted 7 lines · Joel Selvaraj

drivers/gpu/drm/panel/panel-ebbg-ft8719.c new file +7 -0

+#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)

Quoted 2 lines

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

Quoted 2 lines

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?

Quoted 7 lines

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?

Quoted 2 lines

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.

Quoted 4 lines

Doesn't it work to combine them into one call for each pair?

Quoted 2 lines

(quoted diff) +2 -0

+       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.

Nope. But add the rate limited error print please!

Quoted 6 lines

Quoted 2 lines

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.

Quoted 8 lines

Quoted 4 lines

Doesn't it work to combine them into one call for each pair?

Quoted 2 lines

(quoted diff) +2 -0

+       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