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@gmail.com>
---
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@amarulasolutions.com>
*/
+#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...) \
+ { \
+ 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,
+ 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);
if (ret)
- goto err_attach;
+ return ret;
return 0;
+}
-err_attach:
- drm_panel_remove(&st7701->panel);
- return ret;
+static void st7701_remove(struct st7701 *st7701)
+{
+ st7701_cleanup(st7701);
+}
+
+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@amarulasolutions.com>");
+MODULE_AUTHOR("Hironori KIKUCHI <kikuchan98@gmail.com>");
MODULE_DESCRIPTION("Sitronix ST7701 LCD Panel Driver");
MODULE_LICENSE("GPL");
--
2.45.2
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@gmail.com>
> ---
> 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@amarulasolutions.com>
> */
>
> +#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@amarulasolutions.com>");
> +MODULE_AUTHOR("Hironori KIKUCHI <kikuchan98@gmail.com>");
> MODULE_DESCRIPTION("Sitronix ST7701 LCD Panel Driver");
> MODULE_LICENSE("GPL");
> --
> 2.45.2
>
Hello Jessica,
Thank you for your review and reply!
On Sat, Jun 22, 2024 at 5:31 AM Jessica Zhang <quic_jesszhan@quicinc.com> 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@gmail.com>
> > ---
> > 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@amarulasolutions.com>
> > */
> >
> > +#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@amarulasolutions.com>");
> > +MODULE_AUTHOR("Hironori KIKUCHI <kikuchan98@gmail.com>");
> > 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@quicinc.com> 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@gmail.com>
> > ---
> > 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@amarulasolutions.com>
> > */
> >
> > +#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@amarulasolutions.com>");
> > +MODULE_AUTHOR("Hironori KIKUCHI <kikuchan98@gmail.com>");
> > MODULE_DESCRIPTION("Sitronix ST7701 LCD Panel Driver");
> > MODULE_LICENSE("GPL");
> > --
> > 2.45.2
> >
Hi Hironori,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Hironori-KIKUCHI/dt-bindings-display-st7701-Add-Anbernic-RG28XX-panel/20240618-161849
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20240618081515.1215552-3-kikuchan98%40gmail.com
patch subject: [PATCH v1 2/3] drm/panel: st7701: Add support for SPI for configuration
config: i386-randconfig-141-20240621 (https://download.01.org/0day-ci/archive/20240621/202406210841.Q17G5ISz-lkp@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202406210841.Q17G5ISz-lkp@intel.com/
smatch warnings:
drivers/gpu/drm/panel/panel-sitronix-st7701.c:1076 st7701_dsi_probe() warn: passing zero to 'dev_err_probe'
vim +/dev_err_probe +1076 drivers/gpu/drm/panel/panel-sitronix-st7701.c
433b7d46874577 Hironori KIKUCHI 2024-06-18 1062 static int st7701_dsi_probe(struct mipi_dsi_device *dsi)
433b7d46874577 Hironori KIKUCHI 2024-06-18 1063 {
433b7d46874577 Hironori KIKUCHI 2024-06-18 1064 struct st7701 *st7701;
433b7d46874577 Hironori KIKUCHI 2024-06-18 1065 int err;
433b7d46874577 Hironori KIKUCHI 2024-06-18 1066
433b7d46874577 Hironori KIKUCHI 2024-06-18 1067 err = st7701_probe(&dsi->dev, DRM_MODE_CONNECTOR_DSI);
433b7d46874577 Hironori KIKUCHI 2024-06-18 1068 if (err)
433b7d46874577 Hironori KIKUCHI 2024-06-18 1069 return err;
433b7d46874577 Hironori KIKUCHI 2024-06-18 1070
433b7d46874577 Hironori KIKUCHI 2024-06-18 1071 st7701 = dev_get_drvdata(&dsi->dev);
849b2e3ff96982 Jagan Teki 2019-01-25 1072 st7701->dsi = dsi;
433b7d46874577 Hironori KIKUCHI 2024-06-18 1073 st7701->write_command = st7701_dsi_write;
849b2e3ff96982 Jagan Teki 2019-01-25 1074
433b7d46874577 Hironori KIKUCHI 2024-06-18 1075 if (!st7701->desc->lanes)
433b7d46874577 Hironori KIKUCHI 2024-06-18 @1076 return dev_err_probe(&dsi->dev, err, "This panel is not for MIPI DSI\n");
^^^
-EINVAL?
433b7d46874577 Hironori KIKUCHI 2024-06-18 1077
433b7d46874577 Hironori KIKUCHI 2024-06-18 1078 dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
433b7d46874577 Hironori KIKUCHI 2024-06-18 1079 MIPI_DSI_MODE_LPM | MIPI_DSI_CLOCK_NON_CONTINUOUS;
433b7d46874577 Hironori KIKUCHI 2024-06-18 1080 dsi->format = st7701->desc->format;
433b7d46874577 Hironori KIKUCHI 2024-06-18 1081 dsi->lanes = st7701->desc->lanes;
433b7d46874577 Hironori KIKUCHI 2024-06-18 1082
433b7d46874577 Hironori KIKUCHI 2024-06-18 1083 mipi_dsi_set_drvdata(dsi, st7701);
433b7d46874577 Hironori KIKUCHI 2024-06-18 1084
433b7d46874577 Hironori KIKUCHI 2024-06-18 1085 err = mipi_dsi_attach(dsi);
433b7d46874577 Hironori KIKUCHI 2024-06-18 1086 if (err)
433b7d46874577 Hironori KIKUCHI 2024-06-18 1087 return dev_err_probe(&dsi->dev, err, "Failed to init MIPI DSI\n");
c62102165dd792 Marek Vasut 2022-10-15 1088
c62102165dd792 Marek Vasut 2022-10-15 1089 return 0;
433b7d46874577 Hironori KIKUCHI 2024-06-18 1090 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On 18/06/2024 10:15, 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@gmail.com>
...
> +
> +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);
Nope, you cannot use GPIOD_FLAGS_BIT_NONEXCLUSIVE outside of regulators.
I don't see any code for ensuring proper simultaneus access.
Best regards,
Krzysztof
© 2016 - 2025 Red Hat, Inc.