Re: [v1 0/2] Break out as separate driver from boe-tv101wum-nl6 panel driver

Re: [v1 0/2] Break out as separate driver from boe-tv101wum-nl6 panel driver

[Linus Walleij]

On Sat, Oct 7, 2023 at 8:06 AM Cong Yang
<yangcong5@huaqin.corp-partner.google.com> wrote:

> Linus series proposed to break out ili9882t as separate driver,
> but he didn't have time for that extensive rework of the driver.
> As discussed by Linus and Doug [1], keep macro using the "struct panel_init_cmd"
> until we get some resolution about the binary size issue.

OK works for me:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Dough, if it looks OK to you too, can you apply the patches?

Yours,
Linus Walleij

Re: [v1 0/2] Break out as separate driver from boe-tv101wum-nl6 panel driver

[Doug Anderson]

Hi,

On Sun, Oct 8, 2023 at 12:33 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Sat, Oct 7, 2023 at 8:06 AM Cong Yang
> <yangcong5@huaqin.corp-partner.google.com> wrote:
>
> > Linus series proposed to break out ili9882t as separate driver,
> > but he didn't have time for that extensive rework of the driver.
> > As discussed by Linus and Doug [1], keep macro using the "struct panel_init_cmd"
> > until we get some resolution about the binary size issue.
>
> OK works for me:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Dough, if it looks OK to you too, can you apply the patches?

Thanks for the review. I had a few small comments so I'd expect a v2.

I suspect it would look a little weird to add your Reviewed-by to
patch #1 since the way Cong has it you're the direct patch author. :-P
Cong, I suspect you may want to change the tagging on patch #1. I'd
suggest setting yourself as the patch author (git commit --amend
--reset-author), then tag the first patch like this (I put "x-" first
to make it obvious to any bots reading this that these are not tags to
actually apply--remove that when you tag your patch):

x-Co-developed-by: Linus Walleij <linus.walleij@linaro.org>
x-Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
x-Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
x-Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>

Also: just as a heads up, Hsin-Yi measured the impact of removing the
"command table" for init and replacing it with a whole pile of direct
function calls. She found that it added over 100K to the driver (!!!).
I believe it went from a 45K driver to a 152K driver. Something to
keep in mind. ;-)

-Doug

Re: [v1 0/2] Break out as separate driver from boe-tv101wum-nl6 panel driver

[Linus Walleij]

On Mon, Oct 9, 2023 at 10:53 PM Doug Anderson <dianders@google.com> wrote:

> Also: just as a heads up, Hsin-Yi measured the impact of removing the
> "command table" for init and replacing it with a whole pile of direct
> function calls. She found that it added over 100K to the driver (!!!).
> I believe it went from a 45K driver to a 152K driver. Something to
> keep in mind. ;-)

Sounds like Aarch64 code. I would love a comparison of the same
driver compiled to ARMv7t thumb code. Just for the academic
interest. Because I have heard about people running ARM32
kernels on Aarch64 hardware for this exact reason: so they can
have thumb, which is compact.

OK OK we definitely need command sequence tables in the core,
what we have now is each driver rolling its own which is looking bad.

Yours,
Linus Walleij

Re: [v1 0/2] Break out as separate driver from boe-tv101wum-nl6 panel driver

[Doug Anderson]

Hi,

On Mon, Oct 9, 2023 at 2:02 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Mon, Oct 9, 2023 at 10:53 PM Doug Anderson <dianders@google.com> wrote:
>
> > Also: just as a heads up, Hsin-Yi measured the impact of removing the
> > "command table" for init and replacing it with a whole pile of direct
> > function calls. She found that it added over 100K to the driver (!!!).
> > I believe it went from a 45K driver to a 152K driver. Something to
> > keep in mind. ;-)
>
> Sounds like Aarch64 code. I would love a comparison of the same
> driver compiled to ARMv7t thumb code. Just for the academic
> interest. Because I have heard about people running ARM32
> kernels on Aarch64 hardware for this exact reason: so they can
> have thumb, which is compact.

Yeah, thumb2 was the best.

I suspect that in addition to the aarch64 vs thumb2 part of the
problem is that mipi_dsi_dcs_write_seq() is a macro, so this wasn't
just a whole ton of function calls, but a whole ton of inline function
calls. ;-) Still, even if we fixed that, I'm not sure it we'll ever be
able to beat the space efficiency of command sequence tables.


> OK OK we definitely need command sequence tables in the core,
> what we have now is each driver rolling its own which is looking bad.

Agreed. I'd love to see someone tackle this (though not blocking
Cong's series on it). Hsin-Yi took a quick look at it and noticed that
some drivers have slightly different cases for how they handle command
sequences, which is a bit annoying. For instance, at least one driver
had an extra NOP between commands and said it was important not to
remove that. ...so we'd have to figure out how to abstract some of
these differences without it getting too ugly...

-Doug

Re: [v1 1/2] drm/panel: ili9882t: Break out as separate driver

[Doug Anderson]

Hi,

On Fri, Oct 6, 2023 at 11:07 PM Cong Yang
<yangcong5@huaqin.corp-partner.google.com> wrote:
>
> From: Linus Walleij <linus.walleij@linaro.org>
>
> The Starry ILI9882t-based panel should never have been part of the boe
> tv101wum driver, it is clearly based on the Ilitek ILI9882t display
> controller and if you look at the custom command sequences for the
> panel these clearly contain the signature Ilitek page switch (0xff)
> commands. The hardware has nothing in common with the other panels
> supported by this driver.
>
> Break this out into a separate driver and config symbol instead.
>
> If the placement here is out of convenience for using similar code,
> we should consider creating a helper library instead.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> ---
>  drivers/gpu/drm/panel/Kconfig                 |   9 +
>  drivers/gpu/drm/panel/Makefile                |   1 +
>  .../gpu/drm/panel/panel-boe-tv101wum-nl6.c    | 371 ---------
>  drivers/gpu/drm/panel/panel-ilitek-ili9882t.c | 752 ++++++++++++++++++
>  4 files changed, 762 insertions(+), 371 deletions(-)
>  create mode 100644 drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index ecb22ea326cb..99e14dc212ec 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -203,6 +203,15 @@ config DRM_PANEL_ILITEK_ILI9881C
>           Say Y if you want to enable support for panels based on the
>           Ilitek ILI9881c controller.
>
> +config DRM_PANEL_ILITEK_ILI9882T
> +       tristate "Ilitek ILI9882t-based panels"
> +       depends on OF
> +       depends on DRM_MIPI_DSI
> +       depends on BACKLIGHT_CLASS_DEVICE
> +       help
> +         Say Y if you want to enable support for panels based on the
> +         Ilitek ILI9882t controller.

We'll of course run into the same problem we always run into when
Kconfig symbols get renamed or broken apart: people will have to know
to update their configs to include this. Not much we can do about it,
though. :-/ optional: I guess you could theoretically also include an
extra patch in your series to 'arch/arm64/configs/defconfig' enabling
this new config, since the old panel was enabled there...


> diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> new file mode 100644
> index 000000000000..bbfcffe65623
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> @@ -0,0 +1,752 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Panels based on the Ilitek ILI9882T display controller.
> + */
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drm_connector.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +
> +#include <video/mipi_display.h>
> +
> +/*
> + * Use this descriptor struct to describe different panels using the
> + * Ilitek ILI9882T display controller.
> + */
> +struct panel_desc {
> +       const struct drm_display_mode *modes;
> +       unsigned int bpc;
> +
> +       /**
> +        * @width_mm: width of the panel's active display area
> +        * @height_mm: height of the panel's active display area
> +        */
> +       struct {
> +               unsigned int width_mm;
> +               unsigned int height_mm;
> +       } size;
> +
> +       unsigned long mode_flags;
> +       enum mipi_dsi_pixel_format format;
> +       const struct panel_init_cmd *init_cmds;
> +       unsigned int init_cmd_length;

Why do you need 'init_cmd_length'? It seems like an arbitrary
difference between the two drivers. Your 'panel_init_cmd' in the new
driver still ends with a 0-length command so just use that so you
don't need to store the length.


> +/* ILI9882-specific commands, add new commands as you decode them */
> +#define ILI9882T_DCS_SWITCH_PAGE       0xFF
> +
> +static const struct panel_init_cmd starry_ili9882t_init_cmd[] = {
> +       _INIT_DELAY_CMD(5),
> +       _INIT_DCS_CMD(ILI9882T_DCS_SWITCH_PAGE, 0x98, 0x82, 0x01),

Slightly cleaner, can you do:

#define _INIT_SWITCH_PAGE_CMD(page) \
  _INIT_DCS_CMD(ILI9882T_DCS_SWITCH_PAGE, 0x98, 0x82, (page))

Then in your array you can use stuff like

_INIT_SWITCH_PAGE_CMD(0x01);


> +static int ili9882t_prepare(struct drm_panel *panel)
> +{
> +       struct ili9882t *ili = to_ili9882t(panel);
> +       struct mipi_dsi_device *dsi = ili->dsi;
> +       int i, ret;
> +
> +       gpiod_set_value(ili->enable_gpio, 0);
> +       usleep_range(1000, 1500);
> +
> +       ret = regulator_enable(ili->pp3300);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = regulator_enable(ili->pp1800);
> +       if (ret < 0)
> +               return ret;
> +
> +       usleep_range(3000, 5000);
> +
> +       ret = regulator_enable(ili->avdd);
> +       if (ret < 0)
> +               goto poweroff1v8;
> +       ret = regulator_enable(ili->avee);
> +       if (ret < 0)
> +               goto poweroffavdd;
> +
> +       usleep_range(10000, 11000);
> +
> +       // MIPI needs to keep the LP11 state before the lcm_reset pin is pulled high
> +       mipi_dsi_dcs_nop(ili->dsi);
> +       usleep_range(1000, 2000);
> +
> +       gpiod_set_value(ili->enable_gpio, 1);
> +       usleep_range(1000, 2000);
> +       gpiod_set_value(ili->enable_gpio, 0);
> +       usleep_range(1000, 2000);
> +       gpiod_set_value(ili->enable_gpio, 1);
> +       usleep_range(6000, 10000);
> +
> +       for(i = 0; i < ili->desc->init_cmd_length; i++) {
> +               const struct panel_init_cmd *cmd = &ili->desc->init_cmds[i];
> +               switch (cmd->type) {
> +               case DELAY_CMD:
> +                       msleep(cmd->data[0]);
> +                       ret = 0;
> +                       break;
> +
> +               case INIT_DCS_CMD:
> +                       ret = mipi_dsi_dcs_write(dsi, cmd->data[0],
> +                                                       cmd->len <= 1 ? NULL :
> +                                                       &cmd->data[1],
> +                                                       cmd->len - 1);
> +                       break;
> +
> +               default:
> +                       ret = -EINVAL;
> +               }
> +
> +               if (ret < 0) {
> +                       dev_err(panel->dev,
> +                               "failed to write command %u\n", i);
> +                  goto poweroff;
> +               }
> +       }

In the boe driver the above is in a sub-function
boe_panel_init_dcs_cmd(). Can you create a similar sub-function for
the ili9882t driver? It seems like a nice logical thing to break out
and nice not to have arbitrary differences between the two drivers
since they're so similar...


> +static const struct panel_desc starry_ili9882t_desc = {
> +       .modes = &starry_ili9882t_default_mode,
> +       .bpc = 8,
> +       .size = {
> +               .width_mm = 141,
> +               .height_mm = 226,
> +       },
> +       .lanes = 4,
> +       .format = MIPI_DSI_FMT_RGB888,
> +       .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> +                         MIPI_DSI_MODE_LPM,

nit: please fix indentation of the line above.


> +       .init_cmds = starry_ili9882t_init_cmd,
> +       .init_cmd_length = ARRAY_SIZE(starry_ili9882t_init_cmd),
> +};
> +
> +static int ili9882t_get_modes(struct drm_panel *panel,
> +                                  struct drm_connector *connector)

nit: please fix indentation of the line above.


> +static int ili9882t_add(struct ili9882t *ili)
> +{
> +       struct device *dev = &ili->dsi->dev;
> +       int err;
> +
> +       ili->avdd = devm_regulator_get(dev, "avdd");
> +       if (IS_ERR(ili->avdd))
> +               return PTR_ERR(ili->avdd);
> +
> +       ili->avee = devm_regulator_get(dev, "avee");
> +       if (IS_ERR(ili->avee))
> +               return PTR_ERR(ili->avee);
> +
> +       ili->pp3300 = devm_regulator_get(dev, "pp3300");
> +       if (IS_ERR(ili->pp3300))
> +               return PTR_ERR(ili->pp3300);
> +
> +       ili->pp1800 = devm_regulator_get(dev, "pp1800");
> +       if (IS_ERR(ili->pp1800))
> +               return PTR_ERR(ili->pp1800);
> +
> +       ili->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> +       if (IS_ERR(ili->enable_gpio)) {
> +               dev_err(dev, "cannot get reset-gpios %ld\n",
> +                       PTR_ERR(ili->enable_gpio));
> +               return PTR_ERR(ili->enable_gpio);
> +       }
> +
> +       gpiod_set_value(ili->enable_gpio, 0);
> +
> +       drm_panel_init(&ili->base, dev, &ili9882t_funcs,
> +                          DRM_MODE_CONNECTOR_DSI);

nit: the indentation of the above line isn't quite right. Just put the
whole drm_panel_init() on one line even if it's slightly over 80
characters long.


> +static void ili9882t_shutdown(struct mipi_dsi_device *dsi)
> +{
> +       struct ili9882t *ili = mipi_dsi_get_drvdata(dsi);
> +
> +       drm_panel_disable(&ili->base);
> +       drm_panel_unprepare(&ili->base);
> +}

Please remove the "shutdown()" function. The above two calls to
drm_panel_disable() and drm_panel_unprepare() require that the panel
driver is tracking the "prepared" / "enabled" state and will trigger
warnings if you try shutting down while the panel was off.

We shouldn't need the shutdown functionality because all of the DRM
drivers that this panel is used together with should properly call
drm_atomic_helper_shutdown(). For details, see the long discussion in
reply to my patch at:

https://lore.kernel.org/r/20230804140605.RFC.4.I930069a32baab6faf46d6b234f89613b5cec0f14@changeid


> +static void ili9882t_remove(struct mipi_dsi_device *dsi)
> +{
> +       struct ili9882t *ili = mipi_dsi_get_drvdata(dsi);
> +       int ret;
> +
> +       ili9882t_shutdown(dsi);
> +
> +       ret = mipi_dsi_detach(dsi);
> +       if (ret < 0)
> +               dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
> +
> +       if (ili->base.dev)
> +               drm_panel_remove(&ili->base);
> +}
> +
> +static const struct of_device_id ili9882t_of_match[] = {
> +       { .compatible = "starry,ili9882t",
> +         .data = &starry_ili9882t_desc
> +       },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ili9882t_of_match);
> +
> +static struct mipi_dsi_driver ili9882t_driver = {
> +       .driver = {
> +               .name = "panel-ili9882t",
> +               .of_match_table = ili9882t_of_match,
> +       },
> +       .probe = ili9882t_probe,
> +       .remove = ili9882t_remove,
> +       .shutdown = ili9882t_shutdown,
> +};
> +module_mipi_dsi_driver(ili9882t_driver);
> +
> +MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
> +MODULE_DESCRIPTION("Ilitek ILI9882T-based panels driver");
> +MODULE_LICENSE("GPL");
> \ No newline at end of file

Please make sure there's a newline at the end of the file so you don't
have the "No newline at end of file".

Re: [v1 1/2] drm/panel: ili9882t: Break out as separate driver

[cong yang]

Thanks for the review,I will modify these in V2.

On Tue, Oct 10, 2023 at 4:44 AM Doug Anderson <dianders@google.com> wrote:
>
> Hi,
>
> On Fri, Oct 6, 2023 at 11:07 PM Cong Yang
> <yangcong5@huaqin.corp-partner.google.com> wrote:
> >
> > From: Linus Walleij <linus.walleij@linaro.org>
> >
> > The Starry ILI9882t-based panel should never have been part of the boe
> > tv101wum driver, it is clearly based on the Ilitek ILI9882t display
> > controller and if you look at the custom command sequences for the
> > panel these clearly contain the signature Ilitek page switch (0xff)
> > commands. The hardware has nothing in common with the other panels
> > supported by this driver.
> >
> > Break this out into a separate driver and config symbol instead.
> >
> > If the placement here is out of convenience for using similar code,
> > we should consider creating a helper library instead.
> >
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> > ---
> >  drivers/gpu/drm/panel/Kconfig                 |   9 +
> >  drivers/gpu/drm/panel/Makefile                |   1 +
> >  .../gpu/drm/panel/panel-boe-tv101wum-nl6.c    | 371 ---------
> >  drivers/gpu/drm/panel/panel-ilitek-ili9882t.c | 752 ++++++++++++++++++
> >  4 files changed, 762 insertions(+), 371 deletions(-)
> >  create mode 100644 drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> >
> > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> > index ecb22ea326cb..99e14dc212ec 100644
> > --- a/drivers/gpu/drm/panel/Kconfig
> > +++ b/drivers/gpu/drm/panel/Kconfig
> > @@ -203,6 +203,15 @@ config DRM_PANEL_ILITEK_ILI9881C
> >           Say Y if you want to enable support for panels based on the
> >           Ilitek ILI9881c controller.
> >
> > +config DRM_PANEL_ILITEK_ILI9882T
> > +       tristate "Ilitek ILI9882t-based panels"
> > +       depends on OF
> > +       depends on DRM_MIPI_DSI
> > +       depends on BACKLIGHT_CLASS_DEVICE
> > +       help
> > +         Say Y if you want to enable support for panels based on the
> > +         Ilitek ILI9882t controller.
>
> We'll of course run into the same problem we always run into when
> Kconfig symbols get renamed or broken apart: people will have to know
> to update their configs to include this. Not much we can do about it,
> though. :-/ optional: I guess you could theoretically also include an
> extra patch in your series to 'arch/arm64/configs/defconfig' enabling
> this new config, since the old panel was enabled there...
>
>
> > diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> > new file mode 100644
> > index 000000000000..bbfcffe65623
> > --- /dev/null
> > +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> > @@ -0,0 +1,752 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Panels based on the Ilitek ILI9882T display controller.
> > + */
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#include <drm/drm_connector.h>
> > +#include <drm/drm_crtc.h>
> > +#include <drm/drm_mipi_dsi.h>
> > +#include <drm/drm_panel.h>
> > +
> > +#include <video/mipi_display.h>
> > +
> > +/*
> > + * Use this descriptor struct to describe different panels using the
> > + * Ilitek ILI9882T display controller.
> > + */
> > +struct panel_desc {
> > +       const struct drm_display_mode *modes;
> > +       unsigned int bpc;
> > +
> > +       /**
> > +        * @width_mm: width of the panel's active display area
> > +        * @height_mm: height of the panel's active display area
> > +        */
> > +       struct {
> > +               unsigned int width_mm;
> > +               unsigned int height_mm;
> > +       } size;
> > +
> > +       unsigned long mode_flags;
> > +       enum mipi_dsi_pixel_format format;
> > +       const struct panel_init_cmd *init_cmds;
> > +       unsigned int init_cmd_length;
>
> Why do you need 'init_cmd_length'? It seems like an arbitrary
> difference between the two drivers. Your 'panel_init_cmd' in the new
> driver still ends with a 0-length command so just use that so you
> don't need to store the length.
>
>
> > +/* ILI9882-specific commands, add new commands as you decode them */
> > +#define ILI9882T_DCS_SWITCH_PAGE       0xFF
> > +
> > +static const struct panel_init_cmd starry_ili9882t_init_cmd[] = {
> > +       _INIT_DELAY_CMD(5),
> > +       _INIT_DCS_CMD(ILI9882T_DCS_SWITCH_PAGE, 0x98, 0x82, 0x01),
>
> Slightly cleaner, can you do:
>
> #define _INIT_SWITCH_PAGE_CMD(page) \
>   _INIT_DCS_CMD(ILI9882T_DCS_SWITCH_PAGE, 0x98, 0x82, (page))
>
> Then in your array you can use stuff like
>
> _INIT_SWITCH_PAGE_CMD(0x01);
>
>
> > +static int ili9882t_prepare(struct drm_panel *panel)
> > +{
> > +       struct ili9882t *ili = to_ili9882t(panel);
> > +       struct mipi_dsi_device *dsi = ili->dsi;
> > +       int i, ret;
> > +
> > +       gpiod_set_value(ili->enable_gpio, 0);
> > +       usleep_range(1000, 1500);
> > +
> > +       ret = regulator_enable(ili->pp3300);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       ret = regulator_enable(ili->pp1800);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       usleep_range(3000, 5000);
> > +
> > +       ret = regulator_enable(ili->avdd);
> > +       if (ret < 0)
> > +               goto poweroff1v8;
> > +       ret = regulator_enable(ili->avee);
> > +       if (ret < 0)
> > +               goto poweroffavdd;
> > +
> > +       usleep_range(10000, 11000);
> > +
> > +       // MIPI needs to keep the LP11 state before the lcm_reset pin is pulled high
> > +       mipi_dsi_dcs_nop(ili->dsi);
> > +       usleep_range(1000, 2000);
> > +
> > +       gpiod_set_value(ili->enable_gpio, 1);
> > +       usleep_range(1000, 2000);
> > +       gpiod_set_value(ili->enable_gpio, 0);
> > +       usleep_range(1000, 2000);
> > +       gpiod_set_value(ili->enable_gpio, 1);
> > +       usleep_range(6000, 10000);
> > +
> > +       for(i = 0; i < ili->desc->init_cmd_length; i++) {
> > +               const struct panel_init_cmd *cmd = &ili->desc->init_cmds[i];
> > +               switch (cmd->type) {
> > +               case DELAY_CMD:
> > +                       msleep(cmd->data[0]);
> > +                       ret = 0;
> > +                       break;
> > +
> > +               case INIT_DCS_CMD:
> > +                       ret = mipi_dsi_dcs_write(dsi, cmd->data[0],
> > +                                                       cmd->len <= 1 ? NULL :
> > +                                                       &cmd->data[1],
> > +                                                       cmd->len - 1);
> > +                       break;
> > +
> > +               default:
> > +                       ret = -EINVAL;
> > +               }
> > +
> > +               if (ret < 0) {
> > +                       dev_err(panel->dev,
> > +                               "failed to write command %u\n", i);
> > +                  goto poweroff;
> > +               }
> > +       }
>
> In the boe driver the above is in a sub-function
> boe_panel_init_dcs_cmd(). Can you create a similar sub-function for
> the ili9882t driver? It seems like a nice logical thing to break out
> and nice not to have arbitrary differences between the two drivers
> since they're so similar...
>
>
> > +static const struct panel_desc starry_ili9882t_desc = {
> > +       .modes = &starry_ili9882t_default_mode,
> > +       .bpc = 8,
> > +       .size = {
> > +               .width_mm = 141,
> > +               .height_mm = 226,
> > +       },
> > +       .lanes = 4,
> > +       .format = MIPI_DSI_FMT_RGB888,
> > +       .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> > +                         MIPI_DSI_MODE_LPM,
>
> nit: please fix indentation of the line above.
>
>
> > +       .init_cmds = starry_ili9882t_init_cmd,
> > +       .init_cmd_length = ARRAY_SIZE(starry_ili9882t_init_cmd),
> > +};
> > +
> > +static int ili9882t_get_modes(struct drm_panel *panel,
> > +                                  struct drm_connector *connector)
>
> nit: please fix indentation of the line above.
>
>
> > +static int ili9882t_add(struct ili9882t *ili)
> > +{
> > +       struct device *dev = &ili->dsi->dev;
> > +       int err;
> > +
> > +       ili->avdd = devm_regulator_get(dev, "avdd");
> > +       if (IS_ERR(ili->avdd))
> > +               return PTR_ERR(ili->avdd);
> > +
> > +       ili->avee = devm_regulator_get(dev, "avee");
> > +       if (IS_ERR(ili->avee))
> > +               return PTR_ERR(ili->avee);
> > +
> > +       ili->pp3300 = devm_regulator_get(dev, "pp3300");
> > +       if (IS_ERR(ili->pp3300))
> > +               return PTR_ERR(ili->pp3300);
> > +
> > +       ili->pp1800 = devm_regulator_get(dev, "pp1800");
> > +       if (IS_ERR(ili->pp1800))
> > +               return PTR_ERR(ili->pp1800);
> > +
> > +       ili->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> > +       if (IS_ERR(ili->enable_gpio)) {
> > +               dev_err(dev, "cannot get reset-gpios %ld\n",
> > +                       PTR_ERR(ili->enable_gpio));
> > +               return PTR_ERR(ili->enable_gpio);
> > +       }
> > +
> > +       gpiod_set_value(ili->enable_gpio, 0);
> > +
> > +       drm_panel_init(&ili->base, dev, &ili9882t_funcs,
> > +                          DRM_MODE_CONNECTOR_DSI);
>
> nit: the indentation of the above line isn't quite right. Just put the
> whole drm_panel_init() on one line even if it's slightly over 80
> characters long.
>
>
> > +static void ili9882t_shutdown(struct mipi_dsi_device *dsi)
> > +{
> > +       struct ili9882t *ili = mipi_dsi_get_drvdata(dsi);
> > +
> > +       drm_panel_disable(&ili->base);
> > +       drm_panel_unprepare(&ili->base);
> > +}
>
> Please remove the "shutdown()" function. The above two calls to
> drm_panel_disable() and drm_panel_unprepare() require that the panel
> driver is tracking the "prepared" / "enabled" state and will trigger
> warnings if you try shutting down while the panel was off.
>
> We shouldn't need the shutdown functionality because all of the DRM
> drivers that this panel is used together with should properly call
> drm_atomic_helper_shutdown(). For details, see the long discussion in
> reply to my patch at:
>
> https://lore.kernel.org/r/20230804140605.RFC.4.I930069a32baab6faf46d6b234f89613b5cec0f14@changeid
>
>
> > +static void ili9882t_remove(struct mipi_dsi_device *dsi)
> > +{
> > +       struct ili9882t *ili = mipi_dsi_get_drvdata(dsi);
> > +       int ret;
> > +
> > +       ili9882t_shutdown(dsi);
> > +
> > +       ret = mipi_dsi_detach(dsi);
> > +       if (ret < 0)
> > +               dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
> > +
> > +       if (ili->base.dev)
> > +               drm_panel_remove(&ili->base);
> > +}
> > +
> > +static const struct of_device_id ili9882t_of_match[] = {
> > +       { .compatible = "starry,ili9882t",
> > +         .data = &starry_ili9882t_desc
> > +       },
> > +       { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ili9882t_of_match);
> > +
> > +static struct mipi_dsi_driver ili9882t_driver = {
> > +       .driver = {
> > +               .name = "panel-ili9882t",
> > +               .of_match_table = ili9882t_of_match,
> > +       },
> > +       .probe = ili9882t_probe,
> > +       .remove = ili9882t_remove,
> > +       .shutdown = ili9882t_shutdown,
> > +};
> > +module_mipi_dsi_driver(ili9882t_driver);
> > +
> > +MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
> > +MODULE_DESCRIPTION("Ilitek ILI9882T-based panels driver");
> > +MODULE_LICENSE("GPL");
> > \ No newline at end of file
>
> Please make sure there's a newline at the end of the file so you don't
> have the "No newline at end of file".

Re: [v1 2/2] drm/panel: ili9882t: Avoid blurred screen from fast sleep

[Doug Anderson]

Hi,

On Fri, Oct 6, 2023 at 11:07 PM Cong Yang
<yangcong5@huaqin.corp-partner.google.com> wrote:
>
> At present, we have found that there may be a problem of blurred
> screen during fast sleep/resume. The direct cause of the blurred
> screen is that the IC does not receive 0x28/0x10. Because of the
> particularity of the IC, before the panel enters sleep hid must
> stop scanning, i2c_hid_core_suspend before ili9882t_disable.
> This doesn't look very spec-compliant.

Presumably you could be more spec compliant if we used
"panel_follower" in this case? Would that be a better solution?


> So in order to solve this
> problem, the IC can handle it through the exception mechanism when
> it cannot receive 0X28/0X10 command. Handling exceptions requires a reset
> 50ms delay. Refer to vendor detailed analysis [1].
>
> Ilitek vendor also suggested switching the page before entering sleep to
> avoid panel IC not receiving 0x28/0x10 command.
>
> Note: 0x28 is display off, 0x10 is sleep in.
>
> [1]: https://github.com/ILITEK-LoganLin/Document/tree/main/ILITEK_Power_Sequence
>
> Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> ---
>  drivers/gpu/drm/panel/panel-ilitek-ili9882t.c | 21 ++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> index bbfcffe65623..0a1dd987b204 100644
> --- a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> @@ -423,6 +423,23 @@ static inline struct ili9882t *to_ili9882t(struct drm_panel *panel)
>         return container_of(panel, struct ili9882t, base);
>  }
>
> +static int ili9882t_switch_page(struct mipi_dsi_device *dsi, u8 page)
> +{
> +       u8 switch_cmd[] = {0x98, 0x82, 0x00};

Can't you just replace the last 0x00 above with "page" and get rid of
the manual assignment below?


> +       int ret;
> +
> +       switch_cmd[2] = page;
> +
> +       ret = mipi_dsi_dcs_write(dsi, ILI9882T_DCS_SWITCH_PAGE, switch_cmd, 3);

Instead of hardcoding 3, should use ARRAY_SIZE().


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

optional: It feels like it would be nice to somehow use the
"_INIT_SWITCH_PAGE_CMD" macro I suggested in patch #1 instead of
having to hardcode 0x98, 0x82 again. In patch #1 I already suggested
breaking out the function to send a sequence of commands. If you had
that function take a pointer instead of hardcoding it to look at
->init_cmds then you could probably use the same function that you do
at init time?


>  static int ili9882t_enter_sleep_mode(struct ili9882t *ili)
>  {
>         struct mipi_dsi_device *dsi = ili->dsi;
> @@ -444,8 +461,10 @@ static int ili9882t_enter_sleep_mode(struct ili9882t *ili)
>  static int ili9882t_disable(struct drm_panel *panel)
>  {
>         struct ili9882t *ili = to_ili9882t(panel);
> +       struct mipi_dsi_device *dsi = ili->dsi;
>         int ret;
>
> +       ili9882t_switch_page(dsi, 0x00);
>         ret = ili9882t_enter_sleep_mode(ili);
>         if (ret < 0) {
>                 dev_err(panel->dev, "failed to set panel off: %d\n", ret);
> @@ -507,7 +526,7 @@ static int ili9882t_prepare(struct drm_panel *panel)
>         gpiod_set_value(ili->enable_gpio, 1);
>         usleep_range(1000, 2000);
>         gpiod_set_value(ili->enable_gpio, 0);
> -       usleep_range(1000, 2000);
> +       usleep_range(40000, 50000);

nit: use 40000, 41000 instead of 40000, 50000. Linux almost always
uses the longer delay, so that'll save ~9 ms. The only reason for the
range is to optimize kernel wakeups which is really not a concern
here.

Re: [v1 2/2] drm/panel: ili9882t: Avoid blurred screen from fast sleep

[cong yang]

Hi,

On Tue, Oct 10, 2023 at 4:44 AM Doug Anderson <dianders@google.com> wrote:
>
> Hi,
>
> On Fri, Oct 6, 2023 at 11:07 PM Cong Yang
> <yangcong5@huaqin.corp-partner.google.com> wrote:
> >
> > At present, we have found that there may be a problem of blurred
> > screen during fast sleep/resume. The direct cause of the blurred
> > screen is that the IC does not receive 0x28/0x10. Because of the
> > particularity of the IC, before the panel enters sleep hid must
> > stop scanning, i2c_hid_core_suspend before ili9882t_disable.
> > This doesn't look very spec-compliant.
>
> Presumably you could be more spec compliant if we used
> "panel_follower" in this case? Would that be a better solution?

In the "panel_follower" solution, the phenomenon is the same.
The current order is
ili9882t_disable=>i2c_hid_core_suspend=>elan_i2c_hid_power_down=>ili9882t_unprepare,
ili9882t need touchpanel stop scanning,i2c_hid_core_suspend before
ili9882t_disable.
>
>
> > So in order to solve this
> > problem, the IC can handle it through the exception mechanism when
> > it cannot receive 0X28/0X10 command. Handling exceptions requires a reset
> > 50ms delay. Refer to vendor detailed analysis [1].
> >
> > Ilitek vendor also suggested switching the page before entering sleep to
> > avoid panel IC not receiving 0x28/0x10 command.
> >
> > Note: 0x28 is display off, 0x10 is sleep in.
> >
> > [1]: https://github.com/ILITEK-LoganLin/Document/tree/main/ILITEK_Power_Sequence
> >
> > Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> > ---
> >  drivers/gpu/drm/panel/panel-ilitek-ili9882t.c | 21 ++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> > index bbfcffe65623..0a1dd987b204 100644
> > --- a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> > +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> > @@ -423,6 +423,23 @@ static inline struct ili9882t *to_ili9882t(struct drm_panel *panel)
> >         return container_of(panel, struct ili9882t, base);
> >  }
> >
> > +static int ili9882t_switch_page(struct mipi_dsi_device *dsi, u8 page)
> > +{
> > +       u8 switch_cmd[] = {0x98, 0x82, 0x00};
>
> Can't you just replace the last 0x00 above with "page" and get rid of
> the manual assignment below?
>
>
> > +       int ret;
> > +
> > +       switch_cmd[2] = page;
> > +
> > +       ret = mipi_dsi_dcs_write(dsi, ILI9882T_DCS_SWITCH_PAGE, switch_cmd, 3);
>
> Instead of hardcoding 3, should use ARRAY_SIZE().
>
>
> > +       if (ret) {
> > +               dev_err(&dsi->dev,
> > +                       "error switching panel controller page (%d)\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
>
> optional: It feels like it would be nice to somehow use the
> "_INIT_SWITCH_PAGE_CMD" macro I suggested in patch #1 instead of
> having to hardcode 0x98, 0x82 again. In patch #1 I already suggested
> breaking out the function to send a sequence of commands. If you had
> that function take a pointer instead of hardcoding it to look at
> ->init_cmds then you could probably use the same function that you do
> at init time?
>
>
> >  static int ili9882t_enter_sleep_mode(struct ili9882t *ili)
> >  {
> >         struct mipi_dsi_device *dsi = ili->dsi;
> > @@ -444,8 +461,10 @@ static int ili9882t_enter_sleep_mode(struct ili9882t *ili)
> >  static int ili9882t_disable(struct drm_panel *panel)
> >  {
> >         struct ili9882t *ili = to_ili9882t(panel);
> > +       struct mipi_dsi_device *dsi = ili->dsi;
> >         int ret;
> >
> > +       ili9882t_switch_page(dsi, 0x00);
> >         ret = ili9882t_enter_sleep_mode(ili);
> >         if (ret < 0) {
> >                 dev_err(panel->dev, "failed to set panel off: %d\n", ret);
> > @@ -507,7 +526,7 @@ static int ili9882t_prepare(struct drm_panel *panel)
> >         gpiod_set_value(ili->enable_gpio, 1);
> >         usleep_range(1000, 2000);
> >         gpiod_set_value(ili->enable_gpio, 0);
> > -       usleep_range(1000, 2000);
> > +       usleep_range(40000, 50000);
>
> nit: use 40000, 41000 instead of 40000, 50000. Linux almost always
> uses the longer delay, so that'll save ~9 ms. The only reason for the
> range is to optimize kernel wakeups which is really not a concern
> here.

We need 50ms delay to meet the requirement.

Re: [v1 2/2] drm/panel: ili9882t: Avoid blurred screen from fast sleep

[Doug Anderson]

Hi,

On Tue, Oct 10, 2023 at 4:36 AM cong yang
<yangcong5@huaqin.corp-partner.google.com> wrote:
>
> Hi,
>
> On Tue, Oct 10, 2023 at 4:44 AM Doug Anderson <dianders@google.com> wrote:
> >
> > Hi,
> >
> > On Fri, Oct 6, 2023 at 11:07 PM Cong Yang
> > <yangcong5@huaqin.corp-partner.google.com> wrote:
> > >
> > > At present, we have found that there may be a problem of blurred
> > > screen during fast sleep/resume. The direct cause of the blurred
> > > screen is that the IC does not receive 0x28/0x10. Because of the
> > > particularity of the IC, before the panel enters sleep hid must
> > > stop scanning, i2c_hid_core_suspend before ili9882t_disable.
> > > This doesn't look very spec-compliant.
> >
> > Presumably you could be more spec compliant if we used
> > "panel_follower" in this case? Would that be a better solution?
>
> In the "panel_follower" solution, the phenomenon is the same.
> The current order is
> ili9882t_disable=>i2c_hid_core_suspend=>elan_i2c_hid_power_down=>ili9882t_unprepare,
> ili9882t need touchpanel stop scanning,i2c_hid_core_suspend before
> ili9882t_disable.

Ugh, that's unfortunate. Though is there a reason why you couldn't
just move the `ili9882t_enter_sleep_mode()` to `ili9882t_unprepare()`?
That seems like it should be OK and even perhaps makes it more
symmetric with thue enable?


> > > @@ -507,7 +526,7 @@ static int ili9882t_prepare(struct drm_panel *panel)
> > >         gpiod_set_value(ili->enable_gpio, 1);
> > >         usleep_range(1000, 2000);
> > >         gpiod_set_value(ili->enable_gpio, 0);
> > > -       usleep_range(1000, 2000);
> > > +       usleep_range(40000, 50000);
> >
> > nit: use 40000, 41000 instead of 40000, 50000. Linux almost always
> > uses the longer delay, so that'll save ~9 ms. The only reason for the
> > range is to optimize kernel wakeups which is really not a concern
> > here.
>
> We need 50ms delay to meet the requirement.

I'll respond to your v2, but if you need 50 ms then your current delay is wrong.


-Doug

Re: [v1 2/2] drm/panel: ili9882t: Avoid blurred screen from fast sleep

[cong yang]

Hi,

On Wed, Oct 11, 2023 at 3:11 AM Doug Anderson <dianders@google.com> wrote:
>
> Hi,
>
> On Tue, Oct 10, 2023 at 4:36 AM cong yang
> <yangcong5@huaqin.corp-partner.google.com> wrote:
> >
> > Hi,
> >
> > On Tue, Oct 10, 2023 at 4:44 AM Doug Anderson <dianders@google.com> wrote:
> > >
> > > Hi,
> > >
> > > On Fri, Oct 6, 2023 at 11:07 PM Cong Yang
> > > <yangcong5@huaqin.corp-partner.google.com> wrote:
> > > >
> > > > At present, we have found that there may be a problem of blurred
> > > > screen during fast sleep/resume. The direct cause of the blurred
> > > > screen is that the IC does not receive 0x28/0x10. Because of the
> > > > particularity of the IC, before the panel enters sleep hid must
> > > > stop scanning, i2c_hid_core_suspend before ili9882t_disable.
> > > > This doesn't look very spec-compliant.
> > >
> > > Presumably you could be more spec compliant if we used
> > > "panel_follower" in this case? Would that be a better solution?
> >
> > In the "panel_follower" solution, the phenomenon is the same.
> > The current order is
> > ili9882t_disable=>i2c_hid_core_suspend=>elan_i2c_hid_power_down=>ili9882t_unprepare,
> > ili9882t need touchpanel stop scanning,i2c_hid_core_suspend before
> > ili9882t_disable.
>
> Ugh, that's unfortunate. Though is there a reason why you couldn't
> just move the `ili9882t_enter_sleep_mode()` to `ili9882t_unprepare()`?
> That seems like it should be OK and even perhaps makes it more
> symmetric with thue enable?

Thank you for your suggestion. If the timing is met and there is no problem,
I will implement it in V3.

>
>
> > > > @@ -507,7 +526,7 @@ static int ili9882t_prepare(struct drm_panel *panel)
> > > >         gpiod_set_value(ili->enable_gpio, 1);
> > > >         usleep_range(1000, 2000);
> > > >         gpiod_set_value(ili->enable_gpio, 0);
> > > > -       usleep_range(1000, 2000);
> > > > +       usleep_range(40000, 50000);
> > >
> > > nit: use 40000, 41000 instead of 40000, 50000. Linux almost always
> > > uses the longer delay, so that'll save ~9 ms. The only reason for the
> > > range is to optimize kernel wakeups which is really not a concern
> > > here.
> >
> > We need 50ms delay to meet the requirement.
>
> I'll respond to your v2, but if you need 50 ms then your current delay is wrong.
>
>
> -Doug

Re: [v1 2/2] drm/panel: ili9882t: Avoid blurred screen from fast sleep

[cong yang]

Hi,

On Wed, Oct 11, 2023 at 3:11 AM Doug Anderson <dianders@google.com> wrote:
>
> Hi,
>
> On Tue, Oct 10, 2023 at 4:36 AM cong yang
> <yangcong5@huaqin.corp-partner.google.com> wrote:
> >
> > Hi,
> >
> > On Tue, Oct 10, 2023 at 4:44 AM Doug Anderson <dianders@google.com> wrote:
> > >
> > > Hi,
> > >
> > > On Fri, Oct 6, 2023 at 11:07 PM Cong Yang
> > > <yangcong5@huaqin.corp-partner.google.com> wrote:
> > > >
> > > > At present, we have found that there may be a problem of blurred
> > > > screen during fast sleep/resume. The direct cause of the blurred
> > > > screen is that the IC does not receive 0x28/0x10. Because of the
> > > > particularity of the IC, before the panel enters sleep hid must
> > > > stop scanning, i2c_hid_core_suspend before ili9882t_disable.
> > > > This doesn't look very spec-compliant.
> > >
> > > Presumably you could be more spec compliant if we used
> > > "panel_follower" in this case? Would that be a better solution?
> >
> > In the "panel_follower" solution, the phenomenon is the same.
> > The current order is
> > ili9882t_disable=>i2c_hid_core_suspend=>elan_i2c_hid_power_down=>ili9882t_unprepare,
> > ili9882t need touchpanel stop scanning,i2c_hid_core_suspend before
> > ili9882t_disable.
>
> Ugh, that's unfortunate. Though is there a reason why you couldn't
> just move the `ili9882t_enter_sleep_mode()` to `ili9882t_unprepare()`?
> That seems like it should be OK and even perhaps makes it more
> symmetric with thue enable?

It looks like the test will still fail, because the touchpanel reset was
already pulled low before the panel enter sleep, which did not seem
to meet the timing requirements. I will explain this in the V3  cover letter.
Thanks.

>
>
> > > > @@ -507,7 +526,7 @@ static int ili9882t_prepare(struct drm_panel *panel)
> > > >         gpiod_set_value(ili->enable_gpio, 1);
> > > >         usleep_range(1000, 2000);
> > > >         gpiod_set_value(ili->enable_gpio, 0);
> > > > -       usleep_range(1000, 2000);
> > > > +       usleep_range(40000, 50000);
> > >
> > > nit: use 40000, 41000 instead of 40000, 50000. Linux almost always
> > > uses the longer delay, so that'll save ~9 ms. The only reason for the
> > > range is to optimize kernel wakeups which is really not a concern
> > > here.
> >
> > We need 50ms delay to meet the requirement.
>
> I'll respond to your v2, but if you need 50 ms then your current delay is wrong.
>
>
> -Doug