[PATCH v1 0/3] drm/panel: st7701: Add Anbernic RG28XX panel support

[PATCH v1 0/3] drm/panel: st7701: Add Anbernic RG28XX panel support

[Hironori KIKUCHI]

Add support for the Anbernic RG28XX display panel, as used in the RG28XX
handheld gaming device from Anbernic.

The panel is connected via an RGB parallel interface for image transmission
and an SPI interface for configuration.

Since the current panel driver for the ST7701 variants only supports MIPI
DSI as an interface for configuration, add support for SPI as well.

Hironori KIKUCHI (3):
  dt-bindings: display: st7701: Add Anbernic RG28XX panel
  drm/panel: st7701: Add support for SPI for configuration
  drm/panel: st7701: Add Anbernic RG28XX panel support

 .../display/panel/sitronix,st7701.yaml        |  36 +-
 drivers/gpu/drm/panel/Kconfig                 |   2 +
 drivers/gpu/drm/panel/panel-sitronix-st7701.c | 362 ++++++++++++++++--
 3 files changed, 357 insertions(+), 43 deletions(-)

-- 
2.45.2

[PATCH v1 1/3] dt-bindings: display: st7701: Add Anbernic RG28XX panel

[Hironori KIKUCHI]

The RG28XX panel is a panel specific to the Anbernic RG28XX.
It is 2.8 inches in size (diagonally) with a resolution of 480x640.

Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
---
 .../display/panel/sitronix,st7701.yaml        | 36 +++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
index b348f5bf0a9..04f6751ccca 100644
--- a/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
+++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
@@ -22,19 +22,21 @@ description: |
 
 allOf:
   - $ref: panel-common.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
 
 properties:
   compatible:
     items:
       - enum:
           - anbernic,rg-arc-panel
+          - anbernic,rg28xx-panel
           - densitron,dmt028vghmcmi-1a
           - elida,kd50t048a
           - techstar,ts8550b
       - const: sitronix,st7701
 
   reg:
-    description: DSI virtual channel used by that screen
+    description: DSI / SPI channel used by that screen
     maxItems: 1
 
   VCC-supply:
@@ -43,6 +45,13 @@ properties:
   IOVCC-supply:
     description: I/O system regulator
 
+  dc-gpios:
+    maxItems: 1
+    description: |
+      Controller data/command selection (D/CX) in 4-line SPI mode.
+      If not set, the controller is in 3-line SPI mode.
+      No effect for DSI.
+
   port: true
   reset-gpios: true
   rotation: true
@@ -57,7 +66,7 @@ required:
   - port
   - reset-gpios
 
-additionalProperties: false
+unevaluatedProperties: false
 
 examples:
   - |
@@ -82,3 +91,26 @@ examples:
             };
         };
     };
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        panel@0 {
+            compatible = "anbernic,rg28xx-panel", "sitronix,st7701";
+            reg = <0>;
+            spi-max-frequency = <3125000>;
+            VCC-supply = <&reg_lcd>;
+            IOVCC-supply = <&reg_lcd>;
+            reset-gpios = <&pio 8 14 GPIO_ACTIVE_HIGH>; /* LCD-RST: PI14 */
+            backlight = <&backlight>;
+
+            port {
+                panel_in_rgb: endpoint {
+                    remote-endpoint = <&tcon_lcd0_out_lcd>;
+                };
+            };
+        };
+    };
-- 
2.45.2

Re: [PATCH v1 1/3] dt-bindings: display: st7701: Add Anbernic RG28XX panel

[Krzysztof Kozlowski]

On 18/06/2024 10:15, Hironori KIKUCHI wrote:
> The RG28XX panel is a panel specific to the Anbernic RG28XX.
> It is 2.8 inches in size (diagonally) with a resolution of 480x640.
> 
> Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
> ---
>  .../display/panel/sitronix,st7701.yaml        | 36 +++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)

Nothing explains in the commit msg why rg28xx is actually st7701.
Changing interface to SPI suggests it is not.

> 
> diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
> index b348f5bf0a9..04f6751ccca 100644
> --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
> @@ -22,19 +22,21 @@ description: |
>  
>  allOf:
>    - $ref: panel-common.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>  
>  properties:
>    compatible:
>      items:
>        - enum:
>            - anbernic,rg-arc-panel
> +          - anbernic,rg28xx-panel

What is xx? Wildcards are not allowed, in general.

Can it be anything else than panel? If not, then drop "-panel".


>            - densitron,dmt028vghmcmi-1a
>            - elida,kd50t048a
>            - techstar,ts8550b
>        - const: sitronix,st7701
>  
>    reg:
> -    description: DSI virtual channel used by that screen
> +    description: DSI / SPI channel used by that screen
>      maxItems: 1
>  
>    VCC-supply:
> @@ -43,6 +45,13 @@ properties:
>    IOVCC-supply:
>      description: I/O system regulator
>  
> +  dc-gpios:
> +    maxItems: 1
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      Controller data/command selection (D/CX) in 4-line SPI mode.
> +      If not set, the controller is in 3-line SPI mode.
> +      No effect for DSI.

Which devices can be connected over SPI? It seems not all, so this
should be disallowed (": false" in allOf:if:then:; move the allOf to
bottom like in example-schema) for them.

Best regards,
Krzysztof

Re: [PATCH v1 1/3] dt-bindings: display: st7701: Add Anbernic RG28XX panel

[Hironori KIKUCHI]

Hello Krzysztof,

Thank you for your reply!

On Tue, Jun 18, 2024 at 6:17 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 18/06/2024 10:15, Hironori KIKUCHI wrote:
> > The RG28XX panel is a panel specific to the Anbernic RG28XX.
> > It is 2.8 inches in size (diagonally) with a resolution of 480x640.
> >
> > Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
> > ---
> >  .../display/panel/sitronix,st7701.yaml        | 36 +++++++++++++++++--
> >  1 file changed, 34 insertions(+), 2 deletions(-)
>
> Nothing explains in the commit msg why rg28xx is actually st7701.
> Changing interface to SPI suggests it is not.

Thanks, I'll explain like this;
---
dt-bindings: display: st7701: Add Anbernic RG28XX panel

The RG28XX panel is a panel specific to the Anbernic RG28XX
handheld device. It is 2.8 inches in size (diagonally) with a
resolution of 480x640.

This panel is driven by a variant of ST7701 driver IC internally,
confirmed by dumping and analyzing its BSP initialization sequence
by using a logic analyzer. It is very similar to the existing
densitron,dmt028vghmcmi-1a panel, but differs in some unknown
register values, so add a new entry for the panel to distinguish them.

Additionally, it is connected over SPI, instead of MIPI DSI. So
add and modify for SPI as well.
---

>
> >
> > diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
> > index b348f5bf0a9..04f6751ccca 100644
> > --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
> > +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
> > @@ -22,19 +22,21 @@ description: |
> >
> >  allOf:
> >    - $ref: panel-common.yaml#
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> >
> >  properties:
> >    compatible:
> >      items:
> >        - enum:
> >            - anbernic,rg-arc-panel
> > +          - anbernic,rg28xx-panel
>
> What is xx? Wildcards are not allowed, in general.
>
> Can it be anything else than panel? If not, then drop "-panel".

It's supprising but it actually is a product name of the handheld device...
The panel comes with the device, and part# is completely unknown.

>
>
> >            - densitron,dmt028vghmcmi-1a
> >            - elida,kd50t048a
> >            - techstar,ts8550b
> >        - const: sitronix,st7701
> >
> >    reg:
> > -    description: DSI virtual channel used by that screen
> > +    description: DSI / SPI channel used by that screen
> >      maxItems: 1
> >
> >    VCC-supply:
> > @@ -43,6 +45,13 @@ properties:
> >    IOVCC-supply:
> >      description: I/O system regulator
> >
> > +  dc-gpios:
> > +    maxItems: 1
> > +    description: |
>
> Do not need '|' unless you need to preserve formatting.

Thanks, I'll remove it.

>
> > +      Controller data/command selection (D/CX) in 4-line SPI mode.
> > +      If not set, the controller is in 3-line SPI mode.
> > +      No effect for DSI.
>
> Which devices can be connected over SPI? It seems not all, so this
> should be disallowed (": false" in allOf:if:then:; move the allOf to
> bottom like in example-schema) for them.

Hmm... That's a difficult question...

There are 3 types of connection that trying to support:
DSI, SPI with D/CX pin, and SPI without D/CX pin.

The dc-gpios is required for SPI with D/CX pin, but not for others.

DSI:
- anbernic,rg-arc-panel
- densitron,dmt028vghmcmi-1a
- elida,kd50t048a
- techstar,ts8550b

SPI without D/CX pin:
- anbernic,rg28xx-panel

But, there are no panels with D/CX pin so far.
How should I deal with this? just disallow all, perhaps?


BTW, does panel's compatible string consider to include it's interface?
ie, what if two panels use the exact same commands and timings, but
over different interface,
... are they "compatible" or not?

>
> Best regards,
> Krzysztof
>

Best regards,
kikuchan.

Re: [PATCH v1 1/3] dt-bindings: display: st7701: Add Anbernic RG28XX panel

[Krzysztof Kozlowski]

On 21/06/2024 12:59, Hironori KIKUCHI wrote:
> Hello Krzysztof,
> 
> Thank you for your reply!
> 
> On Tue, Jun 18, 2024 at 6:17 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 18/06/2024 10:15, Hironori KIKUCHI wrote:
>>> The RG28XX panel is a panel specific to the Anbernic RG28XX.
>>> It is 2.8 inches in size (diagonally) with a resolution of 480x640.
>>>
>>> Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
>>> ---
>>>  .../display/panel/sitronix,st7701.yaml        | 36 +++++++++++++++++--
>>>  1 file changed, 34 insertions(+), 2 deletions(-)
>>
>> Nothing explains in the commit msg why rg28xx is actually st7701.
>> Changing interface to SPI suggests it is not.
> 
> Thanks, I'll explain like this;
> ---
> dt-bindings: display: st7701: Add Anbernic RG28XX panel
> 
> The RG28XX panel is a panel specific to the Anbernic RG28XX
> handheld device. It is 2.8 inches in size (diagonally) with a
> resolution of 480x640.
> 
> This panel is driven by a variant of ST7701 driver IC internally,
> confirmed by dumping and analyzing its BSP initialization sequence
> by using a logic analyzer. It is very similar to the existing
> densitron,dmt028vghmcmi-1a panel, but differs in some unknown
> register values, so add a new entry for the panel to distinguish them.
> 
> Additionally, it is connected over SPI, instead of MIPI DSI. So
> add and modify for SPI as well.
> ---

OK.

> 
>>
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
>>> index b348f5bf0a9..04f6751ccca 100644
>>> --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
>>> +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
>>> @@ -22,19 +22,21 @@ description: |
>>>
>>>  allOf:
>>>    - $ref: panel-common.yaml#
>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>>
>>>  properties:
>>>    compatible:
>>>      items:
>>>        - enum:
>>>            - anbernic,rg-arc-panel
>>> +          - anbernic,rg28xx-panel
>>
>> What is xx? Wildcards are not allowed, in general.
>>
>> Can it be anything else than panel? If not, then drop "-panel".
> 
> It's supprising but it actually is a product name of the handheld device...
> The panel comes with the device, and part# is completely unknown.

OK

> 
>>
>>
>>>            - densitron,dmt028vghmcmi-1a
>>>            - elida,kd50t048a
>>>            - techstar,ts8550b
>>>        - const: sitronix,st7701
>>>
>>>    reg:
>>> -    description: DSI virtual channel used by that screen
>>> +    description: DSI / SPI channel used by that screen
>>>      maxItems: 1
>>>
>>>    VCC-supply:
>>> @@ -43,6 +45,13 @@ properties:
>>>    IOVCC-supply:
>>>      description: I/O system regulator
>>>
>>> +  dc-gpios:
>>> +    maxItems: 1
>>> +    description: |
>>
>> Do not need '|' unless you need to preserve formatting.
> 
> Thanks, I'll remove it.
> 
>>
>>> +      Controller data/command selection (D/CX) in 4-line SPI mode.
>>> +      If not set, the controller is in 3-line SPI mode.
>>> +      No effect for DSI.
>>
>> Which devices can be connected over SPI? It seems not all, so this
>> should be disallowed (": false" in allOf:if:then:; move the allOf to
>> bottom like in example-schema) for them.
> 
> Hmm... That's a difficult question...
> 
> There are 3 types of connection that trying to support:
> DSI, SPI with D/CX pin, and SPI without D/CX pin.
> 
> The dc-gpios is required for SPI with D/CX pin, but not for others.
> 
> DSI:
> - anbernic,rg-arc-panel
> - densitron,dmt028vghmcmi-1a
> - elida,kd50t048a
> - techstar,ts8550b
> 
> SPI without D/CX pin:
> - anbernic,rg28xx-panel
> 
> But, there are no panels with D/CX pin so far.
> How should I deal with this? just disallow all, perhaps?

You can disallow for all existing panels, if you are unsure.

> 
> 
> BTW, does panel's compatible string consider to include it's interface?

No, the compatible defines the device, not its wiring (bus). The parent
node defines which bus is needed.

> ie, what if two panels use the exact same commands and timings, but
> over different interface,
> ... are they "compatible" or not?


Best regards,
Krzysztof

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

[Hironori KIKUCHI]

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

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

[Krzysztof Kozlowski]

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

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

[Dan Carpenter]

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

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

[Jessica Zhang]

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
> 

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

[Hironori KIKUCHI]

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
> >

[PATCH v1 3/3] drm/panel: st7701: Add Anbernic RG28XX panel support

[Hironori KIKUCHI]

The Anbernic RG28XX is a handheld device with a 2.8 inch 480x640 display.
Add support for the display.

Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
---
 drivers/gpu/drm/panel/panel-sitronix-st7701.c | 151 ++++++++++++++++++
 1 file changed, 151 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7701.c b/drivers/gpu/drm/panel/panel-sitronix-st7701.c
index 3c4a66f2fc7..d5f8f36c835 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7701.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7701.c
@@ -471,6 +471,55 @@ static void rg_arc_gip_sequence(struct st7701 *st7701)
 	msleep(120);
 }
 
+static void rg28xx_gip_sequence(struct st7701 *st7701)
+{
+	st7701_switch_cmd_bkx(st7701, true, 3);
+	ST7701_DSI(st7701, 0xEF, 0x08);
+
+	st7701_switch_cmd_bkx(st7701, true, 0);
+	ST7701_DSI(st7701, 0xC3, 0x02, 0x10, 0x02);
+	ST7701_DSI(st7701, 0xC7, 0x04);
+	ST7701_DSI(st7701, 0xCC, 0x10);
+
+	st7701_switch_cmd_bkx(st7701, true, 1);
+	ST7701_DSI(st7701, 0xEE, 0x42);
+	ST7701_DSI(st7701, 0xE0, 0x00, 0x00, 0x02);
+
+	ST7701_DSI(st7701, 0xE1, 0x04, 0xA0, 0x06, 0xA0, 0x05, 0xA0, 0x07, 0xA0,
+		   0x00, 0x44, 0x44);
+	ST7701_DSI(st7701, 0xE2, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		   0x00, 0x00, 0x00, 0x00);
+	ST7701_DSI(st7701, 0xE3, 0x00, 0x00, 0x22, 0x22);
+	ST7701_DSI(st7701, 0xE4, 0x44, 0x44);
+	ST7701_DSI(st7701, 0xE5, 0x0C, 0x90, 0xA0, 0xA0, 0x0E, 0x92, 0xA0, 0xA0,
+		   0x08, 0x8C, 0xA0, 0xA0, 0x0A, 0x8E, 0xA0, 0xA0);
+	ST7701_DSI(st7701, 0xE6, 0x00, 0x00, 0x22, 0x22);
+	ST7701_DSI(st7701, 0xE7, 0x44, 0x44);
+	ST7701_DSI(st7701, 0xE8, 0x0D, 0x91, 0xA0, 0xA0, 0x0F, 0x93, 0xA0, 0xA0,
+		   0x09, 0x8D, 0xA0, 0xA0, 0x0B, 0x8F, 0xA0, 0xA0);
+	ST7701_DSI(st7701, 0xEB, 0x00, 0x00, 0xE4, 0xE4, 0x44, 0x00, 0x40);
+	ST7701_DSI(st7701, 0xED, 0xFF, 0xF5, 0x47, 0x6F, 0x0B, 0xA1, 0xBA, 0xFF,
+		   0xFF, 0xAB, 0x1A, 0xB0, 0xF6, 0x74, 0x5F, 0xFF);
+	ST7701_DSI(st7701, 0xEF, 0x08, 0x08, 0x08, 0x45, 0x3F, 0x54);
+
+	st7701_switch_cmd_bkx(st7701, false, 0);
+
+	st7701_switch_cmd_bkx(st7701, true, 3);
+	ST7701_DSI(st7701, 0xE6, 0x16);
+	ST7701_DSI(st7701, 0xE8, 0x00, 0x0E);
+
+	st7701_switch_cmd_bkx(st7701, false, 0);
+	ST7701_DSI(st7701, MIPI_DCS_SET_ADDRESS_MODE, 0x10);
+	ST7701_DSI(st7701, MIPI_DCS_EXIT_SLEEP_MODE);
+	msleep(120);
+
+	st7701_switch_cmd_bkx(st7701, true, 3);
+	ST7701_DSI(st7701, 0xE8, 0x00, 0x0C);
+	msleep(10);
+	ST7701_DSI(st7701, 0xE8, 0x00, 0x00);
+	st7701_switch_cmd_bkx(st7701, false, 0);
+}
+
 static int st7701_prepare(struct drm_panel *panel)
 {
 	struct st7701 *st7701 = panel_to_st7701(panel);
@@ -986,6 +1035,106 @@ static const struct st7701_panel_desc rg_arc_desc = {
 	.gip_sequence = rg_arc_gip_sequence,
 };
 
+static const struct drm_display_mode rg28xx_mode = {
+	.clock		= 22325,
+
+	.hdisplay	= 480,
+	.hsync_start	= 480 + 40,
+	.hsync_end	= 480 + 40 + 4,
+	.htotal		= 480 + 40 + 4 + 20,
+
+	.vdisplay	= 640,
+	.vsync_start	= 640 + 2,
+	.vsync_end	= 640 + 2 + 40,
+	.vtotal		= 640 + 2 + 40 + 16,
+
+	.width_mm	= 44,
+	.height_mm	= 58,
+
+	.flags		= DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+
+	.type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
+};
+
+static const struct st7701_panel_desc rg28xx_desc = {
+	.mode = &rg28xx_mode,
+
+	.panel_sleep_delay = 80,
+
+	.pv_gamma = {
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_AJ_MASK, 0) |
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC0_MASK, 0),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_AJ_MASK, 0) |
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC4_MASK, 0x10),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_AJ_MASK, 0) |
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC8_MASK, 0x17),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC16_MASK, 0xd),
+
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_AJ_MASK, 0) |
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC24_MASK, 0x11),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC52_MASK, 0x6),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC80_MASK, 0x5),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC108_MASK, 0x8),
+
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC147_MASK, 0x7),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC175_MASK, 0x1f),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC203_MASK, 0x4),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_AJ_MASK, 0) |
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC231_MASK, 0x11),
+
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC239_MASK, 0xe),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_AJ_MASK, 0) |
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC247_MASK, 0x29),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_AJ_MASK, 0) |
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC251_MASK, 0x30),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_AJ_MASK, 0) |
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC255_MASK, 0x1f)
+	},
+	.nv_gamma = {
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_AJ_MASK, 0) |
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC0_MASK, 0),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_AJ_MASK, 0) |
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC4_MASK, 0xd),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_AJ_MASK, 0) |
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC8_MASK, 0x14),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC16_MASK, 0xe),
+
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_AJ_MASK, 0) |
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC24_MASK, 0x11),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC52_MASK, 0x6),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC80_MASK, 0x4),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC108_MASK, 0x8),
+
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC147_MASK, 0x8),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC175_MASK, 0x20),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC203_MASK, 0x5),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_AJ_MASK, 0) |
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC231_MASK, 0x13),
+
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC239_MASK, 0x13),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_AJ_MASK, 0) |
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC247_MASK, 0x26),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_AJ_MASK, 0) |
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC251_MASK, 0x30),
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_AJ_MASK, 0) |
+		CFIELD_PREP(DSI_CMD2_BK0_GAMCTRL_VC255_MASK, 0x1f)
+	},
+	.nlinv = 7,
+	.vop_uv = 4800000,
+	.vcom_uv = 1512500,
+	.vgh_mv = 15000,
+	.vgl_mv = -11730,
+	.avdd_mv = 6600,
+	.avcl_mv = -4400,
+	.gamma_op_bias = OP_BIAS_MIDDLE,
+	.input_op_bias = OP_BIAS_MIN,
+	.output_op_bias = OP_BIAS_MIN,
+	.t2d_ns = 1600,
+	.t3d_ns = 10400,
+	.eot_en = true,
+	.gip_sequence = rg28xx_gip_sequence,
+};
+
 static void st7701_cleanup(void *data)
 {
 	struct st7701 *st7701 = (struct st7701 *)data;
@@ -1141,11 +1290,13 @@ static const struct of_device_id st7701_dsi_of_match[] = {
 MODULE_DEVICE_TABLE(of, st7701_dsi_of_match);
 
 static const struct of_device_id st7701_spi_of_match[] = {
+	{ .compatible = "anbernic,rg28xx-panel", .data = &rg28xx_desc },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, st7701_spi_of_match);
 
 static const struct spi_device_id st7701_spi_ids[] = {
+	{ "rg28xx-panel" },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(spi, st7701_spi_ids);
-- 
2.45.2