Re: [PATCH v1 2/3] drm/panel: st7701: Add support for SPI for configuration

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

 



Hello Jessica,

Thank you for your review and reply!

On Sat, Jun 22, 2024 at 5:31 AM Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> wrote:
>
>
>
> On 6/18/2024 1:15 AM, Hironori KIKUCHI wrote:
> > The ST7701 supports not only MIPI DSI, but also SPI as an interface
> > for configuration. To support a panel connected via RGB parallel
> > interface, add support for SPI using MIPI DBI helpers.
> >
> > Signed-off-by: Hironori KIKUCHI <kikuchan98@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/panel/Kconfig                 |   2 +
> >   drivers/gpu/drm/panel/panel-sitronix-st7701.c | 211 ++++++++++++++----
> >   2 files changed, 172 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> > index 2ae0eb0638f..1831544400d 100644
> > --- a/drivers/gpu/drm/panel/Kconfig
> > +++ b/drivers/gpu/drm/panel/Kconfig
> > @@ -755,7 +755,9 @@ config DRM_PANEL_SHARP_LS060T1SX01
> >   config DRM_PANEL_SITRONIX_ST7701
> >       tristate "Sitronix ST7701 panel driver"
> >       depends on OF
> > +     depends on SPI
> >       depends on DRM_MIPI_DSI
> > +     select DRM_MIPI_DBI
> >       depends on BACKLIGHT_CLASS_DEVICE
> >       help
> >         Say Y here if you want to enable support for the Sitronix
> > diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7701.c b/drivers/gpu/drm/panel/panel-sitronix-st7701.c
> > index 421eb4592b6..3c4a66f2fc7 100644
> > --- a/drivers/gpu/drm/panel/panel-sitronix-st7701.c
> > +++ b/drivers/gpu/drm/panel/panel-sitronix-st7701.c
> > @@ -4,6 +4,7 @@
> >    * Author: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>
> >    */
> >
> > +#include <drm/drm_mipi_dbi.h>
> >   #include <drm/drm_mipi_dsi.h>
> >   #include <drm/drm_modes.h>
> >   #include <drm/drm_panel.h>
> > @@ -14,6 +15,7 @@
> >   #include <linux/module.h>
> >   #include <linux/of.h>
> >   #include <linux/regulator/consumer.h>
> > +#include <linux/spi/spi.h>
> >
> >   #include <video/mipi_display.h>
> >
> > @@ -130,12 +132,16 @@ struct st7701_panel_desc {
> >   struct st7701 {
> >       struct drm_panel panel;
> >       struct mipi_dsi_device *dsi;
> > +     struct mipi_dbi dbi;
> >       const struct st7701_panel_desc *desc;
> >
> >       struct regulator_bulk_data supplies[2];
> >       struct gpio_desc *reset;
> >       unsigned int sleep_delay;
> >       enum drm_panel_orientation orientation;
> > +
> > +     int (*write_command)(struct st7701 *st7701, u8 cmd, const u8 *seq,
> > +                          size_t len);
> >   };
> >
> >   static inline struct st7701 *panel_to_st7701(struct drm_panel *panel)
> > @@ -143,16 +149,22 @@ static inline struct st7701 *panel_to_st7701(struct drm_panel *panel)
> >       return container_of(panel, struct st7701, panel);
> >   }
> >
> > -static inline int st7701_dsi_write(struct st7701 *st7701, const void *seq,
> > -                                size_t len)
> > +static int st7701_dsi_write(struct st7701 *st7701, u8 cmd, const u8 *seq,
> > +                         size_t len)
> >   {
> > -     return mipi_dsi_dcs_write_buffer(st7701->dsi, seq, len);
> > +     return mipi_dsi_dcs_write(st7701->dsi, cmd, seq, len);
> >   }
> >
> > -#define ST7701_DSI(st7701, seq...)                           \
> > -     {                                                       \
> > -             const u8 d[] = { seq };                         \
> > -             st7701_dsi_write(st7701, d, ARRAY_SIZE(d));     \
> > +static int st7701_dbi_write(struct st7701 *st7701, u8 cmd, const u8 *seq,
> > +                         size_t len)
> > +{
> > +     return mipi_dbi_command_stackbuf(&st7701->dbi, cmd, seq, len);
> > +}
> > +
> > +#define ST7701_DSI(st7701, cmd, seq...)
>
> Hi Hironori,
>
> Is it really correct to keep this as *_DSI? Since the macro can
> theoretically support either DBI or DSI write commands, maybe this
> should be renamed to something more generic.
>
>                                 \
> > +     {                                                               \
> > +             const u8 d[] = { seq };                                 \
> > +             st7701->write_command(st7701, cmd, d, ARRAY_SIZE(d));   \
> >       }
> >
> >   static u8 st7701_vgls_map(struct st7701 *st7701)
> > @@ -211,10 +223,10 @@ static void st7701_init_sequence(struct st7701 *st7701)
> >       /* Command2, BK0 */
> >       st7701_switch_cmd_bkx(st7701, true, 0);
> >
> > -     mipi_dsi_dcs_write(st7701->dsi, DSI_CMD2_BK0_PVGAMCTRL,
> > -                        desc->pv_gamma, ARRAY_SIZE(desc->pv_gamma));
> > -     mipi_dsi_dcs_write(st7701->dsi, DSI_CMD2_BK0_NVGAMCTRL,
> > -                        desc->nv_gamma, ARRAY_SIZE(desc->nv_gamma));
> > +     st7701->write_command(st7701, DSI_CMD2_BK0_PVGAMCTRL, desc->pv_gamma,
>
> Same here for the DSI_CMD2_* macros.
>

Right...
I'll change them to ST7701_WIRTE() and ST7701_CMD* respectively.
Thanks.


> > +                           ARRAY_SIZE(desc->pv_gamma));
> > +     st7701->write_command(st7701, DSI_CMD2_BK0_NVGAMCTRL, desc->nv_gamma,
> > +                           ARRAY_SIZE(desc->nv_gamma));
> >       /*
> >        * Vertical line count configuration:
> >        * Line[6:0]: select number of vertical lines of the TFT matrix in
> > @@ -974,42 +986,47 @@ static const struct st7701_panel_desc rg_arc_desc = {
> >       .gip_sequence = rg_arc_gip_sequence,
> >   };
> >
> > -static int st7701_dsi_probe(struct mipi_dsi_device *dsi)
> > +static void st7701_cleanup(void *data)
> > +{
> > +     struct st7701 *st7701 = (struct st7701 *)data;
> > +
> > +     drm_panel_remove(&st7701->panel);
> > +}
> > +
> > +static int st7701_probe(struct device *dev, int connector_type)
> >   {
> >       const struct st7701_panel_desc *desc;
> >       struct st7701 *st7701;
> >       int ret;
> >
> > -     st7701 = devm_kzalloc(&dsi->dev, sizeof(*st7701), GFP_KERNEL);
> > +     desc = of_device_get_match_data(dev);
> > +     if (!desc)
> > +             return -ENODEV;
> > +
> > +     st7701 = devm_kzalloc(dev, sizeof(*st7701), GFP_KERNEL);
> >       if (!st7701)
> >               return -ENOMEM;
> >
> > -     desc = of_device_get_match_data(&dsi->dev);
> > -     dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
> > -                       MIPI_DSI_MODE_LPM | MIPI_DSI_CLOCK_NON_CONTINUOUS;
> > -     dsi->format = desc->format;
> > -     dsi->lanes = desc->lanes;
> > -
> > +     st7701->desc = desc;
> >       st7701->supplies[0].supply = "VCC";
> >       st7701->supplies[1].supply = "IOVCC";
> >
> > -     ret = devm_regulator_bulk_get(&dsi->dev, ARRAY_SIZE(st7701->supplies),
> > +     ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st7701->supplies),
> >                                     st7701->supplies);
> >       if (ret < 0)
> >               return ret;
> >
> > -     st7701->reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
> > +     st7701->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> >       if (IS_ERR(st7701->reset)) {
> > -             dev_err(&dsi->dev, "Couldn't get our reset GPIO\n");
> > +             dev_err(dev, "Couldn't get our reset GPIO\n");
> >               return PTR_ERR(st7701->reset);
> >       }
> >
> > -     ret = of_drm_get_panel_orientation(dsi->dev.of_node, &st7701->orientation);
> > +     ret = of_drm_get_panel_orientation(dev->of_node, &st7701->orientation);
> >       if (ret < 0)
> > -             return dev_err_probe(&dsi->dev, ret, "Failed to get orientation\n");
> > +             return dev_err_probe(dev, ret, "Failed to get orientation\n");
> >
> > -     drm_panel_init(&st7701->panel, &dsi->dev, &st7701_funcs,
> > -                    DRM_MODE_CONNECTOR_DSI);
> > +     drm_panel_init(&st7701->panel, dev, &st7701_funcs, connector_type);
> >
> >       /**
> >        * Once sleep out has been issued, ST7701 IC required to wait 120ms
> > @@ -1026,21 +1043,77 @@ static int st7701_dsi_probe(struct mipi_dsi_device *dsi)
> >       if (ret)
> >               return ret;
> >
> > +     dev_set_drvdata(dev, st7701);
> > +
> >       drm_panel_add(&st7701->panel);
> >
> > -     mipi_dsi_set_drvdata(dsi, st7701);
> > -     st7701->dsi = dsi;
> > -     st7701->desc = desc;
> > -
> > -     ret = mipi_dsi_attach(dsi);
> > +     ret = devm_add_action_or_reset(dev, st7701_cleanup, st7701);
>
> Is the intention here to move to using the devm framework? Also, just
> wondering, what advantages does this implementation have over the
> original implementation?

Uh, maybe no?
Actually not intended, but I just want to avoid the need to call
drm_panel_remove() manually and carefully in both st7701_dsi_probe()
and st7701_spi_probe() after calling st7701_probe().

Oh, in this way, I've just noticed that drm_panel_remove() might no
longer need to be explicitly called in st7701_remove(), perhaps?

>
> >       if (ret)
> > -             goto err_attach;
> > +             return ret;
>
> If you're removing the `goto` here, you could probably drop this entire
> check and just `return ret` at the end of the function.

Right, thanks.

>
> >
> >       return 0;
> > +}
> >
> > -err_attach:
> > -     drm_panel_remove(&st7701->panel);
> > -     return ret;
> > +static void st7701_remove(struct st7701 *st7701)
> > +{
> > +     st7701_cleanup(st7701);
>
> Why add an extra helper that's essentially just a wrapper for another
> helper? Why can't we directly call st7701_cleanup()?

To avoid a cast, because I'm unsure which is preferred in kernel...
Would it be better like this?

return devm_add_action_or_reset(dev, (void (*)(void *))st7701_remove, st7701);

or, more directly

return devm_add_action_or_reset(dev, (void (*)(void
*))drm_panel_remove, &st7701->panel);

This emphasizes a relation to the preceding drm_panel_add().

>
> Thanks,
>
> Jessica Zhang
>
> > +}
> > +
> > +static int st7701_dsi_probe(struct mipi_dsi_device *dsi)
> > +{
> > +     struct st7701 *st7701;
> > +     int err;
> > +
> > +     err = st7701_probe(&dsi->dev, DRM_MODE_CONNECTOR_DSI);
> > +     if (err)
> > +             return err;
> > +
> > +     st7701 = dev_get_drvdata(&dsi->dev);
> > +     st7701->dsi = dsi;
> > +     st7701->write_command = st7701_dsi_write;
> > +
> > +     if (!st7701->desc->lanes)
> > +             return dev_err_probe(&dsi->dev, err, "This panel is not for MIPI DSI\n");
> > +
> > +     dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
> > +                       MIPI_DSI_MODE_LPM | MIPI_DSI_CLOCK_NON_CONTINUOUS;
> > +     dsi->format = st7701->desc->format;
> > +     dsi->lanes = st7701->desc->lanes;
> > +
> > +     mipi_dsi_set_drvdata(dsi, st7701);
> > +
> > +     err = mipi_dsi_attach(dsi);
> > +     if (err)
> > +             return dev_err_probe(&dsi->dev, err, "Failed to init MIPI DSI\n");
> > +
> > +     return 0;
> > +}
> > +
> > +static int st7701_spi_probe(struct spi_device *spi)
> > +{
> > +     struct st7701 *st7701;
> > +     struct gpio_desc *dc;
> > +     int err;
> > +
> > +     err = st7701_probe(&spi->dev, DRM_MODE_CONNECTOR_DPI);
> > +     if (err)
> > +             return err;
> > +
> > +     st7701 = dev_get_drvdata(&spi->dev);
> > +     st7701->write_command = st7701_dbi_write;
> > +
> > +     dc = devm_gpiod_get_optional(&spi->dev, "dc", GPIOD_OUT_LOW | GPIOD_FLAGS_BIT_NONEXCLUSIVE);
> > +     if (IS_ERR(dc))
> > +             return dev_err_probe(&spi->dev, PTR_ERR(dc), "Failed to get GPIO for D/CX\n");
> > +
> > +     spi_set_drvdata(spi, st7701);
> > +
> > +     err = mipi_dbi_spi_init(spi, &st7701->dbi, dc);
> > +     if (err)
> > +             return dev_err_probe(&spi->dev, err, "Failed to init MIPI DBI\n");
> > +     st7701->dbi.read_commands = NULL;
> > +
> > +     return 0;
> >   }
> >
> >   static void st7701_dsi_remove(struct mipi_dsi_device *dsi)
> > @@ -1048,28 +1121,84 @@ static void st7701_dsi_remove(struct mipi_dsi_device *dsi)
> >       struct st7701 *st7701 = mipi_dsi_get_drvdata(dsi);
> >
> >       mipi_dsi_detach(dsi);
> > -     drm_panel_remove(&st7701->panel);
> > +     st7701_remove(st7701);
> >   }
> >
> > -static const struct of_device_id st7701_of_match[] = {
> > +static void st7701_spi_remove(struct spi_device *spi)
> > +{
> > +     struct st7701 *st7701 = spi_get_drvdata(spi);
> > +
> > +     st7701_remove(st7701);
> > +}
> > +
> > +static const struct of_device_id st7701_dsi_of_match[] = {
> >       { .compatible = "anbernic,rg-arc-panel", .data = &rg_arc_desc },
> >       { .compatible = "densitron,dmt028vghmcmi-1a", .data = &dmt028vghmcmi_1a_desc },
> >       { .compatible = "elida,kd50t048a", .data = &kd50t048a_desc },
> >       { .compatible = "techstar,ts8550b", .data = &ts8550b_desc },
> > -     { }
> > +     { /* sentinel */ }
> >   };
> > -MODULE_DEVICE_TABLE(of, st7701_of_match);
> > +MODULE_DEVICE_TABLE(of, st7701_dsi_of_match);
> > +
> > +static const struct of_device_id st7701_spi_of_match[] = {
> > +     { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, st7701_spi_of_match);
> > +
> > +static const struct spi_device_id st7701_spi_ids[] = {
> > +     { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(spi, st7701_spi_ids);
> >
> >   static struct mipi_dsi_driver st7701_dsi_driver = {
> >       .probe          = st7701_dsi_probe,
> >       .remove         = st7701_dsi_remove,
> >       .driver = {
> >               .name           = "st7701",
> > -             .of_match_table = st7701_of_match,
> > +             .of_match_table = st7701_dsi_of_match,
> >       },
> >   };
> > -module_mipi_dsi_driver(st7701_dsi_driver);
> > +
> > +static struct spi_driver st7701_spi_driver = {
> > +     .probe          = st7701_spi_probe,
> > +     .remove         = st7701_spi_remove,
> > +     .id_table       = st7701_spi_ids,
> > +     .driver = {
> > +             .name           = "st7701",
> > +             .of_match_table = st7701_spi_of_match,
> > +     },
> > +};
> > +
> > +static int __init st7701_driver_init(void)
> > +{
> > +     int err;
> > +
> > +     err = spi_register_driver(&st7701_spi_driver);
> > +     if (err)
> > +             return err;
> > +
> > +     if (IS_ENABLED(CONFIG_DRM_MIPI_DSI)) {
> > +             err = mipi_dsi_driver_register(&st7701_dsi_driver);
> > +             if (err) {
> > +                     spi_unregister_driver(&st7701_spi_driver);
> > +                     return err;
> > +             }
> > +
> > +             return 0;
> > +     }
> > +}
> > +module_init(st7701_driver_init);
> > +
> > +static void __exit st7701_driver_exit(void)
> > +{
> > +     if (IS_ENABLED(CONFIG_DRM_MIPI_DSI))
> > +             mipi_dsi_driver_unregister(&st7701_dsi_driver);
> > +
> > +     spi_unregister_driver(&st7701_spi_driver);
> > +}
> > +module_exit(st7701_driver_exit);
> >
> >   MODULE_AUTHOR("Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>");
> > +MODULE_AUTHOR("Hironori KIKUCHI <kikuchan98@xxxxxxxxx>");
> >   MODULE_DESCRIPTION("Sitronix ST7701 LCD Panel Driver");
> >   MODULE_LICENSE("GPL");
> > --
> > 2.45.2
> >

Best regards,
kikuchan.

On Sat, Jun 22, 2024 at 5:31 AM Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> wrote:
>
>
>
> On 6/18/2024 1:15 AM, Hironori KIKUCHI wrote:
> > The ST7701 supports not only MIPI DSI, but also SPI as an interface
> > for configuration. To support a panel connected via RGB parallel
> > interface, add support for SPI using MIPI DBI helpers.
> >
> > Signed-off-by: Hironori KIKUCHI <kikuchan98@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/panel/Kconfig                 |   2 +
> >   drivers/gpu/drm/panel/panel-sitronix-st7701.c | 211 ++++++++++++++----
> >   2 files changed, 172 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> > index 2ae0eb0638f..1831544400d 100644
> > --- a/drivers/gpu/drm/panel/Kconfig
> > +++ b/drivers/gpu/drm/panel/Kconfig
> > @@ -755,7 +755,9 @@ config DRM_PANEL_SHARP_LS060T1SX01
> >   config DRM_PANEL_SITRONIX_ST7701
> >       tristate "Sitronix ST7701 panel driver"
> >       depends on OF
> > +     depends on SPI
> >       depends on DRM_MIPI_DSI
> > +     select DRM_MIPI_DBI
> >       depends on BACKLIGHT_CLASS_DEVICE
> >       help
> >         Say Y here if you want to enable support for the Sitronix
> > diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7701.c b/drivers/gpu/drm/panel/panel-sitronix-st7701.c
> > index 421eb4592b6..3c4a66f2fc7 100644
> > --- a/drivers/gpu/drm/panel/panel-sitronix-st7701.c
> > +++ b/drivers/gpu/drm/panel/panel-sitronix-st7701.c
> > @@ -4,6 +4,7 @@
> >    * Author: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>
> >    */
> >
> > +#include <drm/drm_mipi_dbi.h>
> >   #include <drm/drm_mipi_dsi.h>
> >   #include <drm/drm_modes.h>
> >   #include <drm/drm_panel.h>
> > @@ -14,6 +15,7 @@
> >   #include <linux/module.h>
> >   #include <linux/of.h>
> >   #include <linux/regulator/consumer.h>
> > +#include <linux/spi/spi.h>
> >
> >   #include <video/mipi_display.h>
> >
> > @@ -130,12 +132,16 @@ struct st7701_panel_desc {
> >   struct st7701 {
> >       struct drm_panel panel;
> >       struct mipi_dsi_device *dsi;
> > +     struct mipi_dbi dbi;
> >       const struct st7701_panel_desc *desc;
> >
> >       struct regulator_bulk_data supplies[2];
> >       struct gpio_desc *reset;
> >       unsigned int sleep_delay;
> >       enum drm_panel_orientation orientation;
> > +
> > +     int (*write_command)(struct st7701 *st7701, u8 cmd, const u8 *seq,
> > +                          size_t len);
> >   };
> >
> >   static inline struct st7701 *panel_to_st7701(struct drm_panel *panel)
> > @@ -143,16 +149,22 @@ static inline struct st7701 *panel_to_st7701(struct drm_panel *panel)
> >       return container_of(panel, struct st7701, panel);
> >   }
> >
> > -static inline int st7701_dsi_write(struct st7701 *st7701, const void *seq,
> > -                                size_t len)
> > +static int st7701_dsi_write(struct st7701 *st7701, u8 cmd, const u8 *seq,
> > +                         size_t len)
> >   {
> > -     return mipi_dsi_dcs_write_buffer(st7701->dsi, seq, len);
> > +     return mipi_dsi_dcs_write(st7701->dsi, cmd, seq, len);
> >   }
> >
> > -#define ST7701_DSI(st7701, seq...)                           \
> > -     {                                                       \
> > -             const u8 d[] = { seq };                         \
> > -             st7701_dsi_write(st7701, d, ARRAY_SIZE(d));     \
> > +static int st7701_dbi_write(struct st7701 *st7701, u8 cmd, const u8 *seq,
> > +                         size_t len)
> > +{
> > +     return mipi_dbi_command_stackbuf(&st7701->dbi, cmd, seq, len);
> > +}
> > +
> > +#define ST7701_DSI(st7701, cmd, seq...)
>
> Hi Hironori,
>
> Is it really correct to keep this as *_DSI? Since the macro can
> theoretically support either DBI or DSI write commands, maybe this
> should be renamed to something more generic.
>
>                                 \
> > +     {                                                               \
> > +             const u8 d[] = { seq };                                 \
> > +             st7701->write_command(st7701, cmd, d, ARRAY_SIZE(d));   \
> >       }
> >
> >   static u8 st7701_vgls_map(struct st7701 *st7701)
> > @@ -211,10 +223,10 @@ static void st7701_init_sequence(struct st7701 *st7701)
> >       /* Command2, BK0 */
> >       st7701_switch_cmd_bkx(st7701, true, 0);
> >
> > -     mipi_dsi_dcs_write(st7701->dsi, DSI_CMD2_BK0_PVGAMCTRL,
> > -                        desc->pv_gamma, ARRAY_SIZE(desc->pv_gamma));
> > -     mipi_dsi_dcs_write(st7701->dsi, DSI_CMD2_BK0_NVGAMCTRL,
> > -                        desc->nv_gamma, ARRAY_SIZE(desc->nv_gamma));
> > +     st7701->write_command(st7701, DSI_CMD2_BK0_PVGAMCTRL, desc->pv_gamma,
>
> Same here for the DSI_CMD2_* macros.
>
> > +                           ARRAY_SIZE(desc->pv_gamma));
> > +     st7701->write_command(st7701, DSI_CMD2_BK0_NVGAMCTRL, desc->nv_gamma,
> > +                           ARRAY_SIZE(desc->nv_gamma));
> >       /*
> >        * Vertical line count configuration:
> >        * Line[6:0]: select number of vertical lines of the TFT matrix in
> > @@ -974,42 +986,47 @@ static const struct st7701_panel_desc rg_arc_desc = {
> >       .gip_sequence = rg_arc_gip_sequence,
> >   };
> >
> > -static int st7701_dsi_probe(struct mipi_dsi_device *dsi)
> > +static void st7701_cleanup(void *data)
> > +{
> > +     struct st7701 *st7701 = (struct st7701 *)data;
> > +
> > +     drm_panel_remove(&st7701->panel);
> > +}
> > +
> > +static int st7701_probe(struct device *dev, int connector_type)
> >   {
> >       const struct st7701_panel_desc *desc;
> >       struct st7701 *st7701;
> >       int ret;
> >
> > -     st7701 = devm_kzalloc(&dsi->dev, sizeof(*st7701), GFP_KERNEL);
> > +     desc = of_device_get_match_data(dev);
> > +     if (!desc)
> > +             return -ENODEV;
> > +
> > +     st7701 = devm_kzalloc(dev, sizeof(*st7701), GFP_KERNEL);
> >       if (!st7701)
> >               return -ENOMEM;
> >
> > -     desc = of_device_get_match_data(&dsi->dev);
> > -     dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
> > -                       MIPI_DSI_MODE_LPM | MIPI_DSI_CLOCK_NON_CONTINUOUS;
> > -     dsi->format = desc->format;
> > -     dsi->lanes = desc->lanes;
> > -
> > +     st7701->desc = desc;
> >       st7701->supplies[0].supply = "VCC";
> >       st7701->supplies[1].supply = "IOVCC";
> >
> > -     ret = devm_regulator_bulk_get(&dsi->dev, ARRAY_SIZE(st7701->supplies),
> > +     ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st7701->supplies),
> >                                     st7701->supplies);
> >       if (ret < 0)
> >               return ret;
> >
> > -     st7701->reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
> > +     st7701->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> >       if (IS_ERR(st7701->reset)) {
> > -             dev_err(&dsi->dev, "Couldn't get our reset GPIO\n");
> > +             dev_err(dev, "Couldn't get our reset GPIO\n");
> >               return PTR_ERR(st7701->reset);
> >       }
> >
> > -     ret = of_drm_get_panel_orientation(dsi->dev.of_node, &st7701->orientation);
> > +     ret = of_drm_get_panel_orientation(dev->of_node, &st7701->orientation);
> >       if (ret < 0)
> > -             return dev_err_probe(&dsi->dev, ret, "Failed to get orientation\n");
> > +             return dev_err_probe(dev, ret, "Failed to get orientation\n");
> >
> > -     drm_panel_init(&st7701->panel, &dsi->dev, &st7701_funcs,
> > -                    DRM_MODE_CONNECTOR_DSI);
> > +     drm_panel_init(&st7701->panel, dev, &st7701_funcs, connector_type);
> >
> >       /**
> >        * Once sleep out has been issued, ST7701 IC required to wait 120ms
> > @@ -1026,21 +1043,77 @@ static int st7701_dsi_probe(struct mipi_dsi_device *dsi)
> >       if (ret)
> >               return ret;
> >
> > +     dev_set_drvdata(dev, st7701);
> > +
> >       drm_panel_add(&st7701->panel);
> >
> > -     mipi_dsi_set_drvdata(dsi, st7701);
> > -     st7701->dsi = dsi;
> > -     st7701->desc = desc;
> > -
> > -     ret = mipi_dsi_attach(dsi);
> > +     ret = devm_add_action_or_reset(dev, st7701_cleanup, st7701);
>
> Is the intention here to move to using the devm framework? Also, just
> wondering, what advantages does this implementation have over the
> original implementation?
>
> >       if (ret)
> > -             goto err_attach;
> > +             return ret;
>
> If you're removing the `goto` here, you could probably drop this entire
> check and just `return ret` at the end of the function.
>
> >
> >       return 0;
> > +}
> >
> > -err_attach:
> > -     drm_panel_remove(&st7701->panel);
> > -     return ret;
> > +static void st7701_remove(struct st7701 *st7701)
> > +{
> > +     st7701_cleanup(st7701);
>
> Why add an extra helper that's essentially just a wrapper for another
> helper? Why can't we directly call st7701_cleanup()?
>
> Thanks,
>
> Jessica Zhang
>
> > +}
> > +
> > +static int st7701_dsi_probe(struct mipi_dsi_device *dsi)
> > +{
> > +     struct st7701 *st7701;
> > +     int err;
> > +
> > +     err = st7701_probe(&dsi->dev, DRM_MODE_CONNECTOR_DSI);
> > +     if (err)
> > +             return err;
> > +
> > +     st7701 = dev_get_drvdata(&dsi->dev);
> > +     st7701->dsi = dsi;
> > +     st7701->write_command = st7701_dsi_write;
> > +
> > +     if (!st7701->desc->lanes)
> > +             return dev_err_probe(&dsi->dev, err, "This panel is not for MIPI DSI\n");
> > +
> > +     dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
> > +                       MIPI_DSI_MODE_LPM | MIPI_DSI_CLOCK_NON_CONTINUOUS;
> > +     dsi->format = st7701->desc->format;
> > +     dsi->lanes = st7701->desc->lanes;
> > +
> > +     mipi_dsi_set_drvdata(dsi, st7701);
> > +
> > +     err = mipi_dsi_attach(dsi);
> > +     if (err)
> > +             return dev_err_probe(&dsi->dev, err, "Failed to init MIPI DSI\n");
> > +
> > +     return 0;
> > +}
> > +
> > +static int st7701_spi_probe(struct spi_device *spi)
> > +{
> > +     struct st7701 *st7701;
> > +     struct gpio_desc *dc;
> > +     int err;
> > +
> > +     err = st7701_probe(&spi->dev, DRM_MODE_CONNECTOR_DPI);
> > +     if (err)
> > +             return err;
> > +
> > +     st7701 = dev_get_drvdata(&spi->dev);
> > +     st7701->write_command = st7701_dbi_write;
> > +
> > +     dc = devm_gpiod_get_optional(&spi->dev, "dc", GPIOD_OUT_LOW | GPIOD_FLAGS_BIT_NONEXCLUSIVE);
> > +     if (IS_ERR(dc))
> > +             return dev_err_probe(&spi->dev, PTR_ERR(dc), "Failed to get GPIO for D/CX\n");
> > +
> > +     spi_set_drvdata(spi, st7701);
> > +
> > +     err = mipi_dbi_spi_init(spi, &st7701->dbi, dc);
> > +     if (err)
> > +             return dev_err_probe(&spi->dev, err, "Failed to init MIPI DBI\n");
> > +     st7701->dbi.read_commands = NULL;
> > +
> > +     return 0;
> >   }
> >
> >   static void st7701_dsi_remove(struct mipi_dsi_device *dsi)
> > @@ -1048,28 +1121,84 @@ static void st7701_dsi_remove(struct mipi_dsi_device *dsi)
> >       struct st7701 *st7701 = mipi_dsi_get_drvdata(dsi);
> >
> >       mipi_dsi_detach(dsi);
> > -     drm_panel_remove(&st7701->panel);
> > +     st7701_remove(st7701);
> >   }
> >
> > -static const struct of_device_id st7701_of_match[] = {
> > +static void st7701_spi_remove(struct spi_device *spi)
> > +{
> > +     struct st7701 *st7701 = spi_get_drvdata(spi);
> > +
> > +     st7701_remove(st7701);
> > +}
> > +
> > +static const struct of_device_id st7701_dsi_of_match[] = {
> >       { .compatible = "anbernic,rg-arc-panel", .data = &rg_arc_desc },
> >       { .compatible = "densitron,dmt028vghmcmi-1a", .data = &dmt028vghmcmi_1a_desc },
> >       { .compatible = "elida,kd50t048a", .data = &kd50t048a_desc },
> >       { .compatible = "techstar,ts8550b", .data = &ts8550b_desc },
> > -     { }
> > +     { /* sentinel */ }
> >   };
> > -MODULE_DEVICE_TABLE(of, st7701_of_match);
> > +MODULE_DEVICE_TABLE(of, st7701_dsi_of_match);
> > +
> > +static const struct of_device_id st7701_spi_of_match[] = {
> > +     { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, st7701_spi_of_match);
> > +
> > +static const struct spi_device_id st7701_spi_ids[] = {
> > +     { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(spi, st7701_spi_ids);
> >
> >   static struct mipi_dsi_driver st7701_dsi_driver = {
> >       .probe          = st7701_dsi_probe,
> >       .remove         = st7701_dsi_remove,
> >       .driver = {
> >               .name           = "st7701",
> > -             .of_match_table = st7701_of_match,
> > +             .of_match_table = st7701_dsi_of_match,
> >       },
> >   };
> > -module_mipi_dsi_driver(st7701_dsi_driver);
> > +
> > +static struct spi_driver st7701_spi_driver = {
> > +     .probe          = st7701_spi_probe,
> > +     .remove         = st7701_spi_remove,
> > +     .id_table       = st7701_spi_ids,
> > +     .driver = {
> > +             .name           = "st7701",
> > +             .of_match_table = st7701_spi_of_match,
> > +     },
> > +};
> > +
> > +static int __init st7701_driver_init(void)
> > +{
> > +     int err;
> > +
> > +     err = spi_register_driver(&st7701_spi_driver);
> > +     if (err)
> > +             return err;
> > +
> > +     if (IS_ENABLED(CONFIG_DRM_MIPI_DSI)) {
> > +             err = mipi_dsi_driver_register(&st7701_dsi_driver);
> > +             if (err) {
> > +                     spi_unregister_driver(&st7701_spi_driver);
> > +                     return err;
> > +             }
> > +
> > +             return 0;
> > +     }
> > +}
> > +module_init(st7701_driver_init);
> > +
> > +static void __exit st7701_driver_exit(void)
> > +{
> > +     if (IS_ENABLED(CONFIG_DRM_MIPI_DSI))
> > +             mipi_dsi_driver_unregister(&st7701_dsi_driver);
> > +
> > +     spi_unregister_driver(&st7701_spi_driver);
> > +}
> > +module_exit(st7701_driver_exit);
> >
> >   MODULE_AUTHOR("Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>");
> > +MODULE_AUTHOR("Hironori KIKUCHI <kikuchan98@xxxxxxxxx>");
> >   MODULE_DESCRIPTION("Sitronix ST7701 LCD Panel Driver");
> >   MODULE_LICENSE("GPL");
> > --
> > 2.45.2
> >





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux