Re: [PATCH v5 0/3] drm/panel: Support Rocktech jh057n00900 DSI panel
From: Sam Ravnborg
Date: Wed Apr 03 2019 - 17:07:50 EST
Hi Joe.
Thanks for your patch.
> ---
> drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c | 210 +++++++++++++--------
> 1 file changed, 136 insertions(+), 74 deletions(-)
Hmmm, add more lines than it deletes.
>
> diff --git a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> index 158a6d548068..7862863db5f7 100644
> --- a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> +++ b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> @@ -20,27 +20,6 @@
>
> #define DRV_NAME "panel-rocktech-jh057n00900"
>
> -/* Manufacturer specific Commands send via DSI */
> -#define ST7703_CMD_ALL_PIXEL_OFF 0x22
> -#define ST7703_CMD_ALL_PIXEL_ON 0x23
> -#define ST7703_CMD_SETDISP 0xB2
> -#define ST7703_CMD_SETRGBIF 0xB3
> -#define ST7703_CMD_SETCYC 0xB4
> -#define ST7703_CMD_SETBGP 0xB5
> -#define ST7703_CMD_SETVCOM 0xB6
> -#define ST7703_CMD_SETOTP 0xB7
> -#define ST7703_CMD_SETPOWER_EXT 0xB8
> -#define ST7703_CMD_SETEXTC 0xB9
> -#define ST7703_CMD_SETMIPI 0xBA
> -#define ST7703_CMD_SETVDC 0xBC
> -#define ST7703_CMD_SETSCR 0xC0
> -#define ST7703_CMD_SETPOWER 0xC1
> -#define ST7703_CMD_SETPANEL 0xCC
> -#define ST7703_CMD_SETGAMMA 0xE0
> -#define ST7703_CMD_SETEQ 0xE3
> -#define ST7703_CMD_SETGIP1 0xE9
> -#define ST7703_CMD_SETGIP2 0xEA
> -
> struct jh057n {
> struct device *dev;
> struct drm_panel panel;
> @@ -51,75 +30,153 @@ struct jh057n {
> struct dentry *debugfs;
> };
>
> +struct st7703_cmd {
> + const size_t size;
> + const u8 data[];
> +};
> +
> +#define st7703_cmd_data(cmd, ...) \
> + .size = 1 + (sizeof((u8[]){__VA_ARGS__})/sizeof(u8)), \
> + .data = {cmd, __VA_ARGS__}
> +
> +/* Manufacturer specific Commands send via DSI */
> +static const struct st7703_cmd ST7703_CMD_ALL_PIXEL_OFF = {
> + st7703_cmd_data(0x22)
> +};
> +static const struct st7703_cmd ST7703_CMD_ALL_PIXEL_ON = {
> + st7703_cmd_data(0x23)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETDISP = {
> + st7703_cmd_data(0xB2,
> + 0xF0, 0x12, 0x30)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETRGBIF = {
> + st7703_cmd_data(0xB3,
> + 0x10, 0x10, 0x05, 0x05, 0x03, 0xFF, 0x00, 0x00,
> + 0x00, 0x00)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETCYC = {
> + st7703_cmd_data(0xB4,
> + 0x80)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETBGP = {
> + st7703_cmd_data(0xB5,
> + 0x08, 0x08)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETVCOM = {
> + st7703_cmd_data(0xB6,
> + 0x3F, 0x3F)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETOTP = {
> + st7703_cmd_data(0xB7)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETPOWER_EXT = {
> + st7703_cmd_data(0xB8)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETEXTC = {
> + st7703_cmd_data(0xB9,
> + 0xF1, 0x12, 0x83)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETMIPI = {
> + st7703_cmd_data(0xBA)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETVDC = {
> + st7703_cmd_data(0xBC,
> + 0x4E)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETSCR = {
> + st7703_cmd_data(0xC0,
> + 0x73, 0x73, 0x50, 0x50, 0x00, 0x00, 0x08, 0x70,
> + 0x00)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETPOWER = {
> + st7703_cmd_data(0xC1)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETPANEL = {
> + st7703_cmd_data(0xCC,
> + 0x0B)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETGAMMA = {
> + st7703_cmd_data(0xE0,
> + 0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41, 0x37,
> + 0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10, 0x11,
> + 0x18, 0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41,
> + 0x37, 0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10,
> + 0x11, 0x18)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETEQ = {
> + st7703_cmd_data(0xE3,
> + 0x07, 0x07, 0x0B, 0x0B, 0x03, 0x0B, 0x00, 0x00,
> + 0x00, 0x00, 0xFF, 0x00, 0xC0, 0x10)
> +};
> +static const struct st7703_cmd ST7703_CMD_SETGIP1 = {
> + st7703_cmd_data(0xE9,
> + 0x82, 0x10, 0x06, 0x05, 0x9E, 0x0A, 0xA5, 0x12,
> + 0x31, 0x23, 0x37, 0x83, 0x04, 0xBC, 0x27, 0x38,
> + 0x0C, 0x00, 0x03, 0x00, 0x00, 0x00, 0x0C, 0x00,
> + 0x03, 0x00, 0x00, 0x00, 0x75, 0x75, 0x31, 0x88,
> + 0x88, 0x88, 0x88, 0x88, 0x88, 0x13, 0x88, 0x64,
> + 0x64, 0x20, 0x88, 0x88, 0x88, 0x88, 0x88, 0x88,
> + 0x02, 0x88, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00)
> +};
> +
> +static const struct st7703_cmd ST7703_CMD_SETGIP2 = {
> + st7703_cmd_data(0xEA,
> + 0x02, 0x21, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0x00, 0x00, 0x00, 0x02, 0x46, 0x02, 0x88,
> + 0x88, 0x88, 0x88, 0x88, 0x88, 0x64, 0x88, 0x13,
> + 0x57, 0x13, 0x88, 0x88, 0x88, 0x88, 0x88, 0x88,
> + 0x75, 0x88, 0x23, 0x14, 0x00, 0x00, 0x02, 0x00,
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x30, 0x0A,
> + 0xA5, 0x00, 0x00, 0x00, 0x00)
> +};
> +
> static inline struct jh057n *panel_to_jh057n(struct drm_panel *panel)
> {
> return container_of(panel, struct jh057n, panel);
> }
>
> -#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) \
> - return ret; \
> - } while (0)
The above macro was the one triggering this patch.
And frankly it looks nice and simple.
The old code is IMO more readable.
- We have all the commands listed in the order they
are used and in a rahter compatch format.
- It is obvious when we need delays.
- We have traditional #defines for the constants we know
This macro:
> +#define st7703_cmd_data(cmd, ...) \
> + .size = 1 + (sizeof((u8[]){__VA_ARGS__})/sizeof(u8)), \
> + .data = {cmd, __VA_ARGS__}
is again IMO not easier to follow than the above.
This is all to some extent bikeshedding, but I suggest
to keep the current code.
It is simple and it is tested.
Thanks for trying to come up with a better solution.
Sam