[v3,4/4] drm/panel: ili9882t: Break out function for switching page

  The ILI9882t has similarities with other Ilitek panels, such
as the characteristic internal page switching code that uses
the model number (0x98, 0x82) as parameter.

We can clearly abstract out the page switching sequence from
the initialization code.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpu/drm/panel/panel-ilitek-ili9882t.c | 54 ++++++++++++++++++---------
 1 file changed, 37 insertions(+), 17 deletions(-)
  

Hi,

On Mon, Jul 3, 2023 at 9:22 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> The ILI9882t has similarities with other Ilitek panels, such
> as the characteristic internal page switching code that uses
> the model number (0x98, 0x82) as parameter.
>
> We can clearly abstract out the page switching sequence from
> the initialization code.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpu/drm/panel/panel-ilitek-ili9882t.c | 54 ++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> index 20f3cc37fa83..c1a0f10fbaf7 100644
> --- a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> @@ -55,13 +55,33 @@ struct ili9882t {
>         struct gpio_desc *enable_gpio;
>  };
>
> +/* ILI9882-specific commands, add new commands as you decode them */
> +#define ILI9882T_DCS_SWITCH_PAGE       0xFF
> +
> +static int ili9882t_switch_page(struct mipi_dsi_device *dsi, u8 page)
> +{
> +       u8 switch_cmd[] = {0x98, 0x82, 0x00};
> +       int ret;
> +
> +       switch_cmd[2] = page;
> +
> +       ret = mipi_dsi_dcs_write(dsi, ILI9882T_DCS_SWITCH_PAGE, switch_cmd, 3);
> +       if (ret) {
> +               dev_err(&dsi->dev,
> +                       "error switching panel controller page (%d)\n", ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
>  static int starry_ili9882t_init(struct mipi_dsi_device *dsi)
>  {
>         int ret;
>
>         msleep(5);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x01);
> +       ili9882t_switch_page(dsi, 0x01);
>         mipi_dsi_dcs_write_seq(dsi, 0x00, 0x42);
>         mipi_dsi_dcs_write_seq(dsi, 0x01, 0x11);
>         mipi_dsi_dcs_write_seq(dsi, 0x02, 0x00);
> @@ -192,7 +212,7 @@ static int starry_ili9882t_init(struct mipi_dsi_device *dsi)
>         mipi_dsi_dcs_write_seq(dsi, 0x8B, 0x07);
>         mipi_dsi_dcs_write_seq(dsi, 0x8C, 0x07);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x02);
> +       ili9882t_switch_page(dsi, 0x02);
>         mipi_dsi_dcs_write_seq(dsi, 0x29, 0x3A);
>         mipi_dsi_dcs_write_seq(dsi, 0x2A, 0x3B);
>
> @@ -211,12 +231,12 @@ static int starry_ili9882t_init(struct mipi_dsi_device *dsi)
>         mipi_dsi_dcs_write_seq(dsi, 0x5E, 0x40);
>         mipi_dsi_dcs_write_seq(dsi, 0x84, 0x00);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x03);
> +       ili9882t_switch_page(dsi, 0x03);
>         mipi_dsi_dcs_write_seq(dsi, 0x20, 0x01);
>         mipi_dsi_dcs_write_seq(dsi, 0x21, 0x3C);
>         mipi_dsi_dcs_write_seq(dsi, 0x22, 0xFA);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x0A);
> +       ili9882t_switch_page(dsi, 0x0a);
>         mipi_dsi_dcs_write_seq(dsi, 0xE0, 0x01);
>         mipi_dsi_dcs_write_seq(dsi, 0xE2, 0x01);
>         mipi_dsi_dcs_write_seq(dsi, 0xE5, 0x91);
> @@ -224,10 +244,10 @@ static int starry_ili9882t_init(struct mipi_dsi_device *dsi)
>         mipi_dsi_dcs_write_seq(dsi, 0xE7, 0x00);
>         mipi_dsi_dcs_write_seq(dsi, 0xE8, 0xFA);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x12);
> +       ili9882t_switch_page(dsi, 0x12);
>         mipi_dsi_dcs_write_seq(dsi, 0x87, 0x2C);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x05);
> +       ili9882t_switch_page(dsi, 0x05);
>         mipi_dsi_dcs_write_seq(dsi, 0x73, 0xE5);
>         mipi_dsi_dcs_write_seq(dsi, 0x7F, 0x6B);
>         mipi_dsi_dcs_write_seq(dsi, 0x6D, 0xA4);
> @@ -242,7 +262,7 @@ static int starry_ili9882t_init(struct mipi_dsi_device *dsi)
>         mipi_dsi_dcs_write_seq(dsi, 0x1D, 0x90);
>         mipi_dsi_dcs_write_seq(dsi, 0x86, 0x87);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x06);
> +       ili9882t_switch_page(dsi, 0x06);
>         mipi_dsi_dcs_write_seq(dsi, 0xC0, 0x80);
>         mipi_dsi_dcs_write_seq(dsi, 0xC1, 0x07);
>         mipi_dsi_dcs_write_seq(dsi, 0xCA, 0x58);
> @@ -256,7 +276,7 @@ static int starry_ili9882t_init(struct mipi_dsi_device *dsi)
>         mipi_dsi_dcs_write_seq(dsi, 0xD6, 0x55);
>         mipi_dsi_dcs_write_seq(dsi, 0xDC, 0x38);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x08);
> +       ili9882t_switch_page(dsi, 0x08);
>         mipi_dsi_dcs_write_seq(dsi, 0xE0, 0x00, 0x10, 0x2A, 0x4D, 0x61, 0x56, 0x6A, 0x6E, 0x79,
>                                0x76, 0x8F, 0x95, 0x98, 0xAE, 0xAA, 0xB2, 0xBB, 0xCE, 0xC6, 0xBD,
>                                0xD5, 0xE2, 0xE8);
> @@ -264,10 +284,10 @@ static int starry_ili9882t_init(struct mipi_dsi_device *dsi)
>                                0x76, 0x8F, 0x95, 0x98, 0xAE, 0xAA, 0xB2, 0xBB, 0xCE, 0xC6, 0xBD,
>                                0xD5, 0xE2, 0xE8);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x04);
> +       ili9882t_switch_page(dsi, 0x04);
>         mipi_dsi_dcs_write_seq(dsi, 0xBA, 0x81);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x0C);
> +       ili9882t_switch_page(dsi, 0x0c);
>         mipi_dsi_dcs_write_seq(dsi, 0x00, 0x02);
>         mipi_dsi_dcs_write_seq(dsi, 0x01, 0x00);
>         mipi_dsi_dcs_write_seq(dsi, 0x02, 0x03);
> @@ -333,10 +353,10 @@ static int starry_ili9882t_init(struct mipi_dsi_device *dsi)
>         mipi_dsi_dcs_write_seq(dsi, 0x3E, 0x0A);
>         mipi_dsi_dcs_write_seq(dsi, 0x3F, 0x1F);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x04);
> +       ili9882t_switch_page(dsi, 0x04);
>         mipi_dsi_dcs_write_seq(dsi, 0xBA, 0x01);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x0E);
> +       ili9882t_switch_page(dsi, 0x0e);
>         mipi_dsi_dcs_write_seq(dsi, 0x02, 0x0C);
>         mipi_dsi_dcs_write_seq(dsi, 0x20, 0x10);
>         mipi_dsi_dcs_write_seq(dsi, 0x25, 0x16);
> @@ -373,12 +393,12 @@ static int starry_ili9882t_init(struct mipi_dsi_device *dsi)
>         mipi_dsi_dcs_write_seq(dsi, 0x07, 0x21);
>         mipi_dsi_dcs_write_seq(dsi, 0x04, 0x10);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x1E);
> +       ili9882t_switch_page(dsi, 0x1e);
>         mipi_dsi_dcs_write_seq(dsi, 0x60, 0x00);
>         mipi_dsi_dcs_write_seq(dsi, 0x64, 0x00);
>         mipi_dsi_dcs_write_seq(dsi, 0x6D, 0x00);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x0B);
> +       ili9882t_switch_page(dsi, 0x0b);
>         mipi_dsi_dcs_write_seq(dsi, 0xA6, 0x44);
>         mipi_dsi_dcs_write_seq(dsi, 0xA7, 0xB6);
>         mipi_dsi_dcs_write_seq(dsi, 0xA8, 0x03);
> @@ -389,13 +409,13 @@ static int starry_ili9882t_init(struct mipi_dsi_device *dsi)
>         mipi_dsi_dcs_write_seq(dsi, 0xBD, 0x92);
>         mipi_dsi_dcs_write_seq(dsi, 0xBE, 0xA1);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x05);
> +       ili9882t_switch_page(dsi, 0x05);
>         mipi_dsi_dcs_write_seq(dsi, 0x86, 0x87);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x06);
> +       ili9882t_switch_page(dsi, 0x06);
>         mipi_dsi_dcs_write_seq(dsi, 0x92, 0x22);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x00);
> +       ili9882t_switch_page(dsi, 0x00);
>
>         ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>         if (ret)
>
> --
> 2.34.1
>

Tested-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
  

Hi,

On Mon, Jul 3, 2023 at 6:22 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> The ILI9882t has similarities with other Ilitek panels, such
> as the characteristic internal page switching code that uses
> the model number (0x98, 0x82) as parameter.
>
> We can clearly abstract out the page switching sequence from
> the initialization code.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpu/drm/panel/panel-ilitek-ili9882t.c | 54 ++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 17 deletions(-)

This is nice, thanks!


> diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> index 20f3cc37fa83..c1a0f10fbaf7 100644
> --- a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> @@ -55,13 +55,33 @@ struct ili9882t {
>         struct gpio_desc *enable_gpio;
>  };
>
> +/* ILI9882-specific commands, add new commands as you decode them */
> +#define ILI9882T_DCS_SWITCH_PAGE       0xFF
> +
> +static int ili9882t_switch_page(struct mipi_dsi_device *dsi, u8 page)
> +{
> +       u8 switch_cmd[] = {0x98, 0x82, 0x00};
> +       int ret;
> +
> +       switch_cmd[2] = page;
> +
> +       ret = mipi_dsi_dcs_write(dsi, ILI9882T_DCS_SWITCH_PAGE, switch_cmd, 3);

nit: Instead of "3", "ARRAY_SIZE(switch_cmd)" would be more documenting.


> +       if (ret) {
> +               dev_err(&dsi->dev,
> +                       "error switching panel controller page (%d)\n", ret);
> +               return ret;
> +       }
> +
> +       return 0;

This is a static function and you don't check the error code anywhere.
Why bother returning it?

...although in patch #1 I'm suggesting adding to the core the ability
to have a "sequence of sequences". If that makes sense then perhaps
the code below that uses this will be short enough that it won't look
bad to error check each call?


>  static int starry_ili9882t_init(struct mipi_dsi_device *dsi)
>  {
>         int ret;
>
>         msleep(5);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x01);
> +       ili9882t_switch_page(dsi, 0x01);
>         mipi_dsi_dcs_write_seq(dsi, 0x00, 0x42);
>         mipi_dsi_dcs_write_seq(dsi, 0x01, 0x11);
>         mipi_dsi_dcs_write_seq(dsi, 0x02, 0x00);
> @@ -192,7 +212,7 @@ static int starry_ili9882t_init(struct mipi_dsi_device *dsi)
>         mipi_dsi_dcs_write_seq(dsi, 0x8B, 0x07);
>         mipi_dsi_dcs_write_seq(dsi, 0x8C, 0x07);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x02);
> +       ili9882t_switch_page(dsi, 0x02);
>         mipi_dsi_dcs_write_seq(dsi, 0x29, 0x3A);
>         mipi_dsi_dcs_write_seq(dsi, 0x2A, 0x3B);
>
> @@ -211,12 +231,12 @@ static int starry_ili9882t_init(struct mipi_dsi_device *dsi)
>         mipi_dsi_dcs_write_seq(dsi, 0x5E, 0x40);
>         mipi_dsi_dcs_write_seq(dsi, 0x84, 0x00);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x03);
> +       ili9882t_switch_page(dsi, 0x03);
>         mipi_dsi_dcs_write_seq(dsi, 0x20, 0x01);
>         mipi_dsi_dcs_write_seq(dsi, 0x21, 0x3C);
>         mipi_dsi_dcs_write_seq(dsi, 0x22, 0xFA);
>
> -       mipi_dsi_dcs_write_seq(dsi, 0xFF, 0x98, 0x82, 0x0A);
> +       ili9882t_switch_page(dsi, 0x0a);

Super nitty: Even though Linux in general has hex constants in
lowercase, this driver seems to consistently use uppercase. You've
introduced a few inconsistencies here (and a few more below). Can you
make this consistent? Maybe do the upper-to-lower conversion as part
of your efforts to break the driver out?

-Doug