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

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

 



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

On Tue, Oct 10, 2023 at 4:44 AM Doug Anderson <dianders@xxxxxxxxxx> wrote:
>
> Hi,
>
> On Fri, Oct 6, 2023 at 11:07 PM Cong Yang
> <yangcong5@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > From: Linus Walleij <linus.walleij@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Cong Yang <yangcong5@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxx>");
> > +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".




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux