[PATCH 0/6] drm/bridge: Add ICN6211 MIPI-DSI/RGB bridge

[PATCH 0/6] drm/bridge: Add ICN6211 MIPI-DSI/RGB bridge

[Jagan Teki]

This series support Chipone ICN6211 DSI/RGB bridge support.

This ICN6211 bridge is taking flexible configuration of MIPI DSI
signal input and produce RGB565, RGB666, RGB888 output format 
and it is present in the Bananapi s070wv20-ct16 panel.

Initially similar support is written as dsi panel driver, but 
based on the discussion from this thread [1] and input from 
"Andrzej Hajda" I would ended-up writing bridge driver.

patch 1, 2: export drm_bridge_detach

patch 3: bridge support on Allwinner DSI controller

patch 4: ICN6211 dt-bindings

patch 5: ICN6211 bridge driver

patch 6: overlay to enable bridge and panel on BPI-M64, this would
eventually depends on 'Allwinner A64 MIPI-DSI" support [2]

[2] https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=90117
[1] https://patchwork.kernel.org/patch/10657579/

Any inputs?
Jagan.

Jagan Teki (6):
  drm/bridge: Export drm_bridge_detach
  drm/exynos: dsi: Use drm_bridge_detach
  drm/sun4i: dsi: Add bridge support
  dt-bindings: display: bridge: Add ICN6211 MIPI-DSI to RGB convertor
    bridge
  drm/bridge: Add Chipone ICN6211 MIPI-DSI/RGB convertor bridge
  [DO NOT MERGE] arm64: dts: allwinner: bananapi-m64: Enable S070WV20-CT16 DSI panel

 .../display/bridge/chipone,icn6211.txt        |  36 +++
 MAINTAINERS                                   |   6 +
 .../dts/allwinner/sun50i-a64-bananapi-m64.dts |  63 ++++
 drivers/gpu/drm/bridge/Kconfig                |  10 +
 drivers/gpu/drm/bridge/Makefile               |   1 +
 drivers/gpu/drm/bridge/chipone-icn6211.c      | 275 ++++++++++++++++++
 drivers/gpu/drm/drm_bridge.c                  |   1 +
 drivers/gpu/drm/exynos/exynos_drm_dsi.c       |   3 +-
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c        |  65 +++--
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h        |   1 +
 include/drm/drm_bridge.h                      |   1 +
 11 files changed, 443 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
 create mode 100644 drivers/gpu/drm/bridge/chipone-icn6211.c

-- 
2.18.0.321.gffc6fa0e3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/6] dt-bindings: display: bridge: Add ICN6211 MIPI-DSI to RGB convertor bridge

[Jagan Teki]

ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
It has a flexible configuration of MIPI DSI signal input
and produce RGB565, RGB666, RGB888 output format.

Add dt-bingings for it.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 .../display/bridge/chipone,icn6211.txt        | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt

diff --git a/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
new file mode 100644
index 000000000000..7f13efd7ee7f
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
@@ -0,0 +1,36 @@
+Chipone ICN6211 MIPI-DSI to RGB Convertor Bridge
+
+ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
+It has a flexible configuration of MIPI DSI signal input
+and produce RGB565, RGB666, RGB888 output format.
+
+Required properties for RGB:
+- compatible: must be "chipone,icn6211" and one of:
+  * "bananapi,icn6211"
+- reg: the virtual channel number of a DSI peripheral
+- reset-gpios: a GPIO phandle for the reset pin
+
+The device node can contain following 'port' child nodes,
+according to the OF graph bindings defined in [1]:
+  0: DSI Input, not required, if the bridge is DSI controlled
+  1: RGB Output, mandatory
+
+[1]: Documentation/devicetree/bindings/media/video-interfaces.txt
+
+Example:
+
+	bridge@0 {
+		compatible = "bananapi,icn6211", "chipone,icn6211";
+		reg = <0>;
+		reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* LCD-RST: PD6 */
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@1 {
+			reg = <1>;
+
+			bridge_out_ep: endpoint {
+				remote-endpoint = <&panel_ep>;
+			};
+		};
+	};
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH 4/6] dt-bindings: display: bridge: Add ICN6211 MIPI-DSI to RGB convertor bridge

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1716 bytes --]

On Fri, Mar 15, 2019 at 06:38:23PM +0530, Jagan Teki wrote:
> ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
> It has a flexible configuration of MIPI DSI signal input
> and produce RGB565, RGB666, RGB888 output format.
> 
> Add dt-bingings for it.
> 
> Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> ---
>  .../display/bridge/chipone,icn6211.txt        | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> new file mode 100644
> index 000000000000..7f13efd7ee7f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> @@ -0,0 +1,36 @@
> +Chipone ICN6211 MIPI-DSI to RGB Convertor Bridge
> +
> +ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
> +It has a flexible configuration of MIPI DSI signal input
> +and produce RGB565, RGB666, RGB888 output format.
> +
> +Required properties for RGB:
> +- compatible: must be "chipone,icn6211" and one of:
> +  * "bananapi,icn6211"

Why is that compatible needed?

> +- reg: the virtual channel number of a DSI peripheral
> +- reset-gpios: a GPIO phandle for the reset pin
> +
> +The device node can contain following 'port' child nodes,
> +according to the OF graph bindings defined in [1]:
> +  0: DSI Input, not required, if the bridge is DSI controlled
> +  1: RGB Output, mandatory

Your example doesn't have that input port

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 4/6] dt-bindings: display: bridge: Add ICN6211 MIPI-DSI to RGB convertor bridge

[Jagan Teki]

On Fri, Mar 15, 2019 at 7:04 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Fri, Mar 15, 2019 at 06:38:23PM +0530, Jagan Teki wrote:
> > ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
> > It has a flexible configuration of MIPI DSI signal input
> > and produce RGB565, RGB666, RGB888 output format.
> >
> > Add dt-bingings for it.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >  .../display/bridge/chipone,icn6211.txt        | 36 +++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> > new file mode 100644
> > index 000000000000..7f13efd7ee7f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> > @@ -0,0 +1,36 @@
> > +Chipone ICN6211 MIPI-DSI to RGB Convertor Bridge
> > +
> > +ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
> > +It has a flexible configuration of MIPI DSI signal input
> > +and produce RGB565, RGB666, RGB888 output format.
> > +
> > +Required properties for RGB:
> > +- compatible: must be "chipone,icn6211" and one of:
> > +  * "bananapi,icn6211"
>
> Why is that compatible needed?

chipone,icn6211 - generic compatible bridge controller IC
bananapi,icn6211 -  compatible for icn6211 bridge using on bananapi panel

I hope this would be proper bindings in terms of controller IC with
associate device, anything wrong? Infact I used similar reference from
Ilitek Bananapi panel from here
Documentation/devicetree/bindings/display/panel/ilitek,ili9881c.txt

This is what I understood based on dt-binding, correct if I'm wrong.

ilitek,ili9881c - generic ilitek,ili9881c compatable
bananapi,lhr050h41 - compatible for bananapi panel associated with
this ilitek IC

>
> > +- reg: the virtual channel number of a DSI peripheral
> > +- reset-gpios: a GPIO phandle for the reset pin
> > +
> > +The device node can contain following 'port' child nodes,
> > +according to the OF graph bindings defined in [1]:
> > +  0: DSI Input, not required, if the bridge is DSI controlled
> > +  1: RGB Output, mandatory
>
> Your example doesn't have that input port

Yes, I intentionally did this by referring existing bridge binding.
Documentation/devicetree/bindings/display/bridge/toshiba,tc35876*.txt

Do we really need? since the input port can be part of panel binding.

Re: [linux-sunxi] Re: [PATCH 4/6] dt-bindings: display: bridge: Add ICN6211 MIPI-DSI to RGB convertor bridge

[Chen-Yu Tsai]

On Tue, Mar 19, 2019 at 12:58 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Fri, Mar 15, 2019 at 7:04 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Fri, Mar 15, 2019 at 06:38:23PM +0530, Jagan Teki wrote:
> > > ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
> > > It has a flexible configuration of MIPI DSI signal input
> > > and produce RGB565, RGB666, RGB888 output format.
> > >
> > > Add dt-bingings for it.
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > ---
> > >  .../display/bridge/chipone,icn6211.txt        | 36 +++++++++++++++++++
> > >  1 file changed, 36 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> > > new file mode 100644
> > > index 000000000000..7f13efd7ee7f
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> > > @@ -0,0 +1,36 @@
> > > +Chipone ICN6211 MIPI-DSI to RGB Convertor Bridge
> > > +
> > > +ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
> > > +It has a flexible configuration of MIPI DSI signal input
> > > +and produce RGB565, RGB666, RGB888 output format.
> > > +
> > > +Required properties for RGB:
> > > +- compatible: must be "chipone,icn6211" and one of:
> > > +  * "bananapi,icn6211"
> >
> > Why is that compatible needed?
>
> chipone,icn6211 - generic compatible bridge controller IC
> bananapi,icn6211 -  compatible for icn6211 bridge using on bananapi panel
>
> I hope this would be proper bindings in terms of controller IC with
> associate device, anything wrong? Infact I used similar reference from
> Ilitek Bananapi panel from here
> Documentation/devicetree/bindings/display/panel/ilitek,ili9881c.txt
>
> This is what I understood based on dt-binding, correct if I'm wrong.
>
> ilitek,ili9881c - generic ilitek,ili9881c compatable
> bananapi,lhr050h41 - compatible for bananapi panel associated with
> this ilitek IC

ili9881c is an LCD driver chip with a MIPI DSI interface. It directly
drives the LCD panel, not outputting some RGB stuff. So it is a binding
and driver for a panel, not a bridge in your case.

> >
> > > +- reg: the virtual channel number of a DSI peripheral
> > > +- reset-gpios: a GPIO phandle for the reset pin
> > > +
> > > +The device node can contain following 'port' child nodes,
> > > +according to the OF graph bindings defined in [1]:
> > > +  0: DSI Input, not required, if the bridge is DSI controlled
> > > +  1: RGB Output, mandatory
> >
> > Your example doesn't have that input port
>
> Yes, I intentionally did this by referring existing bridge binding.
> Documentation/devicetree/bindings/display/bridge/toshiba,tc35876*.txt
>
> Do we really need? since the input port can be part of panel binding.

How could the input port of _your_ _bridge_ be part of the panel binding?

ChenYu

Re: [linux-sunxi] Re: [PATCH 4/6] dt-bindings: display: bridge: Add ICN6211 MIPI-DSI to RGB convertor bridge

[Jagan Teki]

On Tue, Mar 19, 2019 at 8:29 AM Chen-Yu Tsai <wens@csie.org> wrote:
>
> On Tue, Mar 19, 2019 at 12:58 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Fri, Mar 15, 2019 at 7:04 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Fri, Mar 15, 2019 at 06:38:23PM +0530, Jagan Teki wrote:
> > > > ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
> > > > It has a flexible configuration of MIPI DSI signal input
> > > > and produce RGB565, RGB666, RGB888 output format.
> > > >
> > > > Add dt-bingings for it.
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > ---
> > > >  .../display/bridge/chipone,icn6211.txt        | 36 +++++++++++++++++++
> > > >  1 file changed, 36 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> > > > new file mode 100644
> > > > index 000000000000..7f13efd7ee7f
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> > > > @@ -0,0 +1,36 @@
> > > > +Chipone ICN6211 MIPI-DSI to RGB Convertor Bridge
> > > > +
> > > > +ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
> > > > +It has a flexible configuration of MIPI DSI signal input
> > > > +and produce RGB565, RGB666, RGB888 output format.
> > > > +
> > > > +Required properties for RGB:
> > > > +- compatible: must be "chipone,icn6211" and one of:
> > > > +  * "bananapi,icn6211"
> > >
> > > Why is that compatible needed?
> >
> > chipone,icn6211 - generic compatible bridge controller IC
> > bananapi,icn6211 -  compatible for icn6211 bridge using on bananapi panel
> >
> > I hope this would be proper bindings in terms of controller IC with
> > associate device, anything wrong? Infact I used similar reference from
> > Ilitek Bananapi panel from here
> > Documentation/devicetree/bindings/display/panel/ilitek,ili9881c.txt
> >
> > This is what I understood based on dt-binding, correct if I'm wrong.
> >
> > ilitek,ili9881c - generic ilitek,ili9881c compatable
> > bananapi,lhr050h41 - compatible for bananapi panel associated with
> > this ilitek IC
>
> ili9881c is an LCD driver chip with a MIPI DSI interface. It directly
> drives the LCD panel, not outputting some RGB stuff. So it is a binding
> and driver for a panel, not a bridge in your case.
>
> > >
> > > > +- reg: the virtual channel number of a DSI peripheral
> > > > +- reset-gpios: a GPIO phandle for the reset pin
> > > > +
> > > > +The device node can contain following 'port' child nodes,
> > > > +according to the OF graph bindings defined in [1]:
> > > > +  0: DSI Input, not required, if the bridge is DSI controlled
> > > > +  1: RGB Output, mandatory
> > >
> > > Your example doesn't have that input port
> >
> > Yes, I intentionally did this by referring existing bridge binding.
> > Documentation/devicetree/bindings/display/bridge/toshiba,tc35876*.txt
> >
> > Do we really need? since the input port can be part of panel binding.
>
> How could the input port of _your_ _bridge_ be part of the panel binding?

Here the panel is from another vendor say bananapi, so the binding of
that part could be already covered ie is what I mean.

Since I mentioned I took the reference binding from another bridge. It
has similar structure of binding / example.
arch/arm/boot/dts/exynos5250-arndale.dts
Bridge example binding:
Documentation/devicetree/bindings/display/bridge/toshiba,tc358764.txt
Panel example binding mentioned here
Documentation/devicetree/bindings/display/panel/boe,hv070wsa-100.txt
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/6] dt-bindings: display: bridge: Add ICN6211 MIPI-DSI to RGB convertor bridge

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2179 bytes --]

On Mon, Mar 18, 2019 at 10:28:10PM +0530, Jagan Teki wrote:
> On Fri, Mar 15, 2019 at 7:04 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Fri, Mar 15, 2019 at 06:38:23PM +0530, Jagan Teki wrote:
> > > ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
> > > It has a flexible configuration of MIPI DSI signal input
> > > and produce RGB565, RGB666, RGB888 output format.
> > >
> > > Add dt-bingings for it.
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > ---
> > >  .../display/bridge/chipone,icn6211.txt        | 36 +++++++++++++++++++
> > >  1 file changed, 36 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> > > new file mode 100644
> > > index 000000000000..7f13efd7ee7f
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> > > @@ -0,0 +1,36 @@
> > > +Chipone ICN6211 MIPI-DSI to RGB Convertor Bridge
> > > +
> > > +ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
> > > +It has a flexible configuration of MIPI DSI signal input
> > > +and produce RGB565, RGB666, RGB888 output format.
> > > +
> > > +Required properties for RGB:
> > > +- compatible: must be "chipone,icn6211" and one of:
> > > +  * "bananapi,icn6211"
> >
> > Why is that compatible needed?
>
> chipone,icn6211 - generic compatible bridge controller IC
> bananapi,icn6211 -  compatible for icn6211 bridge using on bananapi panel
>
> I hope this would be proper bindings in terms of controller IC with
> associate device, anything wrong? Infact I used similar reference from
> Ilitek Bananapi panel from here
> Documentation/devicetree/bindings/display/panel/ilitek,ili9881c.txt
>
> This is what I understood based on dt-binding, correct if I'm wrong.

The ILI9881c is an LCD controller, and it has some panel specific
configuration.

Does your bridge have board specific configuration?

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 1/6] drm/bridge: Export drm_bridge_detach

[Jagan Teki]

Export drm_bridge_detach from drm bridge core so-that it
can use on respective interface or bridge driver while
detaching the bridge.

Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
---
 drivers/gpu/drm/drm_bridge.c | 1 +
 include/drm/drm_bridge.h     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 138b2711d389..569d4f345429 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -159,6 +159,7 @@ void drm_bridge_detach(struct drm_bridge *bridge)
 
 	bridge->dev = NULL;
 }
+EXPORT_SYMBOL(drm_bridge_detach);
 
 /**
  * DOC: bridge callbacks
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 9da8c93f7976..4955e3e50fa4 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -301,6 +301,7 @@ void drm_bridge_remove(struct drm_bridge *bridge);
 struct drm_bridge *of_drm_find_bridge(struct device_node *np);
 int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 		      struct drm_bridge *previous);
+void drm_bridge_detach(struct drm_bridge *bridge);
 
 bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
 			   const struct drm_display_mode *mode,
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH 1/6] drm/bridge: Export drm_bridge_detach

[Paul Kocialkowski]

Hi Jakan,

On Fri, 2019-03-15 at 18:38 +0530, Jagan Teki wrote:
> Export drm_bridge_detach from drm bridge core so-that it
> can use on respective interface or bridge driver while
> detaching the bridge.

I don't see why this change is required based on the commit log. The
DRM bridge code clearly indicates that drm_bridge_attach should *not*
be balanced with a drm_bridge_detach call in the driver, so this seems
quite wrong.

The DRM core itself should handle detaching the bridge, not the driver.
Is there any reason why you need to do things differently for DSI?

Cheers,

Paul

> Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> ---
>  drivers/gpu/drm/drm_bridge.c | 1 +
>  include/drm/drm_bridge.h     | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 138b2711d389..569d4f345429 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -159,6 +159,7 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>  
>  	bridge->dev = NULL;
>  }
> +EXPORT_SYMBOL(drm_bridge_detach);
>  
>  /**
>   * DOC: bridge callbacks
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 9da8c93f7976..4955e3e50fa4 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -301,6 +301,7 @@ void drm_bridge_remove(struct drm_bridge *bridge);
>  struct drm_bridge *of_drm_find_bridge(struct device_node *np);
>  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  		      struct drm_bridge *previous);
> +void drm_bridge_detach(struct drm_bridge *bridge);
>  
>  bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
>  			   const struct drm_display_mode *mode,
> -- 
> 2.18.0.321.gffc6fa0e3
> 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [linux-sunxi] [PATCH 1/6] drm/bridge: Export drm_bridge_detach

[Jagan Teki]

Hi Paul,

On Fri, Mar 15, 2019 at 6:58 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi Jakan,
>
> On Fri, 2019-03-15 at 18:38 +0530, Jagan Teki wrote:
> > Export drm_bridge_detach from drm bridge core so-that it
> > can use on respective interface or bridge driver while
> > detaching the bridge.
>
> I don't see why this change is required based on the commit log. The
> DRM bridge code clearly indicates that drm_bridge_attach should *not*
> be balanced with a drm_bridge_detach call in the driver, so this seems
> quite wrong.
>
> The DRM core itself should handle detaching the bridge, not the driver.
> Is there any reason why you need to do things differently for DSI?

Yes, you are correct the detach of bridge is being taking care via
drm_encoder_cleanup. This patch exported explicitly, since we need to
taken care bridge detach during unbind even exynos_drm_dsi in other
patch seems using detach by explicitly pointing. so I think the better
approach is to use drm_encoder_cleanup in unbind, what do you say?

Jagan.

Re: [linux-sunxi] [PATCH 1/6] drm/bridge: Export drm_bridge_detach

[Paul Kocialkowski]

Hi,

Le lundi 18 mars 2019 à 22:18 +0530, Jagan Teki a écrit :
> Hi Paul,
> 
> On Fri, Mar 15, 2019 at 6:58 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > Hi Jakan,
> > 
> > On Fri, 2019-03-15 at 18:38 +0530, Jagan Teki wrote:
> > > Export drm_bridge_detach from drm bridge core so-that it
> > > can use on respective interface or bridge driver while
> > > detaching the bridge.
> > 
> > I don't see why this change is required based on the commit log. The
> > DRM bridge code clearly indicates that drm_bridge_attach should *not*
> > be balanced with a drm_bridge_detach call in the driver, so this seems
> > quite wrong.
> > 
> > The DRM core itself should handle detaching the bridge, not the driver.
> > Is there any reason why you need to do things differently for DSI?
> 
> Yes, you are correct the detach of bridge is being taking care via
> drm_encoder_cleanup. This patch exported explicitly, since we need to
> taken care bridge detach during unbind even exynos_drm_dsi in other
> patch seems using detach by explicitly pointing.

I can see that from your patches, but you are not explaining why you
need the change. And if the framework doesn't work for your case, you
should certainly try and fix the framework instead of working around
the issue.

Anyway, you should probably look into using drm_panel_bridge_add, it
might fix the underlying issue on its own.

> so I think the better approach is to use drm_encoder_cleanup in
> unbind, what do you say?

I any case, you need to state what your problem is (in the commit log,
and not even in subsequent discussions) so that we can have a chance to
understand it and provide some feedback about what is an appropriate
fix and what is not. We can't understand the fix if we can't understand
the underlying issue.

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [linux-sunxi] [PATCH 1/6] drm/bridge: Export drm_bridge_detach

[Jagan Teki]

On Mon, Mar 18, 2019 at 10:27 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> Le lundi 18 mars 2019 à 22:18 +0530, Jagan Teki a écrit :
> > Hi Paul,
> >
> > On Fri, Mar 15, 2019 at 6:58 PM Paul Kocialkowski
> > <paul.kocialkowski@bootlin.com> wrote:
> > > Hi Jakan,
> > >
> > > On Fri, 2019-03-15 at 18:38 +0530, Jagan Teki wrote:
> > > > Export drm_bridge_detach from drm bridge core so-that it
> > > > can use on respective interface or bridge driver while
> > > > detaching the bridge.
> > >
> > > I don't see why this change is required based on the commit log. The
> > > DRM bridge code clearly indicates that drm_bridge_attach should *not*
> > > be balanced with a drm_bridge_detach call in the driver, so this seems
> > > quite wrong.
> > >
> > > The DRM core itself should handle detaching the bridge, not the driver.
> > > Is there any reason why you need to do things differently for DSI?
> >
> > Yes, you are correct the detach of bridge is being taking care via
> > drm_encoder_cleanup. This patch exported explicitly, since we need to
> > taken care bridge detach during unbind even exynos_drm_dsi in other
> > patch seems using detach by explicitly pointing.
>
> I can see that from your patches, but you are not explaining why you
> need the change. And if the framework doesn't work for your case, you
> should certainly try and fix the framework instead of working around
> the issue.

Please see below comments.

>
> Anyway, you should probably look into using drm_panel_bridge_add, it
> might fix the underlying issue on its own.
>
> > so I think the better approach is to use drm_encoder_cleanup in
> > unbind, what do you say?
>
> I any case, you need to state what your problem is (in the commit log,
> and not even in subsequent discussions) so that we can have a chance to
> understand it and provide some feedback about what is an appropriate
> fix and what is not. We can't understand the fix if we can't understand
> the underlying issue.

True, if my intentions is trying to fix some issue, but as I said in
previous mail since the other dsi (exynos_drm_dsi) driver is
explicitly detaching I presume it require or missed the export, not
thought about fix and neither I mentioned on the commit head.  Anyway
thanks for the review, will update the code accordingly in next
version.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/6] drm/exynos: dsi: Use drm_bridge_detach

[Jagan Teki]

drm_bridge_detach now available to use while detaching
bridge, use this core wrapper instead of explicitly
pointing the bridge funcs.

Cc: Dae <inki.dae-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Joonyoung Shim <jy0922.shim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Seung-Woo Kim <sw0312.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index a4253dd55f86..5daf43d02768 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1583,8 +1583,7 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
 		dsi->connector.status = connector_status_disconnected;
 		mutex_unlock(&drm->mode_config.mutex);
 	} else {
-		if (dsi->out_bridge->funcs->detach)
-			dsi->out_bridge->funcs->detach(dsi->out_bridge);
+		drm_bridge_detach(dsi->out_bridge);
 		dsi->out_bridge = NULL;
 	}
 
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH 2/6] drm/exynos: dsi: Use drm_bridge_detach

[Inki Dae]

19. 3. 15. 오후 10:08에 Jagan Teki 이(가) 쓴 글:
> drm_bridge_detach now available to use while detaching
> bridge, use this core wrapper instead of explicitly
> pointing the bridge funcs.
> 
> Cc: Dae <inki.dae@samsung.com>
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>

Acked-by: Inki Dae <inki.dae@samsung.com>

Thanks,
Inki Dae

> ---
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index a4253dd55f86..5daf43d02768 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -1583,8 +1583,7 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>  		dsi->connector.status = connector_status_disconnected;
>  		mutex_unlock(&drm->mode_config.mutex);
>  	} else {
> -		if (dsi->out_bridge->funcs->detach)
> -			dsi->out_bridge->funcs->detach(dsi->out_bridge);
> +		drm_bridge_detach(dsi->out_bridge);
>  		dsi->out_bridge = NULL;
>  	}
>  
> 

[PATCH 3/6] drm/sun4i: dsi: Add bridge support

[Jagan Teki]

Some display panels would come up with a non-DSI output which
can have an option to connect DSI interface by means of bridge
convertor.

This DSI to non-DSI bridge convertor would require a bridge
driver that would communicate the DSI controller for bridge
functionalities.

So, add support for bridge functionalities in Allwinner DSI
controller.

Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 65 +++++++++++++++++++-------
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  1 +
 2 files changed, 49 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 0960b96b62cc..64d74313b842 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -781,6 +781,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
 	if (!IS_ERR(dsi->panel))
 		drm_panel_prepare(dsi->panel);
 
+	if (!IS_ERR(dsi->bridge))
+		drm_bridge_pre_enable(dsi->bridge);
+
 	/*
 	 * FIXME: This should be moved after the switch to HS mode.
 	 *
@@ -796,6 +799,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
 	if (!IS_ERR(dsi->panel))
 		drm_panel_enable(dsi->panel);
 
+	if (!IS_ERR(dsi->bridge))
+		drm_bridge_enable(dsi->bridge);
+
 	sun6i_dsi_start(dsi, DSI_START_HSC);
 
 	udelay(1000);
@@ -812,6 +818,9 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
 	if (!IS_ERR(dsi->panel)) {
 		drm_panel_disable(dsi->panel);
 		drm_panel_unprepare(dsi->panel);
+	} else if (!IS_ERR(dsi->bridge)) {
+		drm_bridge_disable(dsi->bridge);
+		drm_bridge_post_disable(dsi->bridge);
 	}
 
 	phy_power_off(dsi->dphy);
@@ -973,11 +982,16 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
 	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
 
 	dsi->device = device;
-	dsi->panel = of_drm_find_panel(device->dev.of_node);
-	if (IS_ERR(dsi->panel))
-		return PTR_ERR(dsi->panel);
 
-	dev_info(host->dev, "Attached device %s\n", device->name);
+	dsi->bridge = of_drm_find_bridge(device->dev.of_node);
+	if (!dsi->bridge) {
+		dsi->panel = of_drm_find_panel(device->dev.of_node);
+		if (IS_ERR(dsi->panel))
+			return PTR_ERR(dsi->panel);
+	}
+
+	dev_info(host->dev, "Attached %s %s\n",
+		 dsi->bridge ? "bridge" : "panel", device->name);
 
 	return 0;
 }
@@ -1055,8 +1069,10 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
 	struct sun4i_tcon *tcon0 = sun4i_get_tcon0(drm);
 	int ret;
 
-	if (!dsi->panel)
+	if (!(dsi->panel || dsi->bridge)) {
+		dev_info(drm->dev, "No panel or bridge found... DSI output disabled\n");
 		return -EPROBE_DEFER;
+	}
 
 	dsi->drv = drv;
 
@@ -1078,19 +1094,29 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
 	}
 	dsi->encoder.possible_crtcs = BIT(0);
 
-	drm_connector_helper_add(&dsi->connector,
-				 &sun6i_dsi_connector_helper_funcs);
-	ret = drm_connector_init(drm, &dsi->connector,
-				 &sun6i_dsi_connector_funcs,
-				 DRM_MODE_CONNECTOR_DSI);
-	if (ret) {
-		dev_err(dsi->dev,
-			"Couldn't initialise the DSI connector\n");
-		goto err_cleanup_connector;
+	if (dsi->panel) {
+		drm_connector_helper_add(&dsi->connector,
+					 &sun6i_dsi_connector_helper_funcs);
+		ret = drm_connector_init(drm, &dsi->connector,
+					 &sun6i_dsi_connector_funcs,
+					 DRM_MODE_CONNECTOR_DSI);
+		if (ret) {
+			dev_err(dsi->dev,
+				"Couldn't initialise the DSI connector\n");
+			goto err_cleanup_connector;
+		}
+
+		drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
+		drm_panel_attach(dsi->panel, &dsi->connector);
 	}
 
-	drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
-	drm_panel_attach(dsi->panel, &dsi->connector);
+	if (dsi->bridge) {
+		ret = drm_bridge_attach(&dsi->encoder, dsi->bridge, NULL);
+		if (ret) {
+			dev_err(dsi->dev, "Couldn't attach the DSI bridge\n");
+			goto err_cleanup_connector;
+		}
+	}
 
 	return 0;
 
@@ -1104,7 +1130,12 @@ static void sun6i_dsi_unbind(struct device *dev, struct device *master,
 {
 	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
 
-	drm_panel_detach(dsi->panel);
+	if (dsi->panel)
+		drm_panel_detach(dsi->panel);
+
+	if (dsi->bridge)
+		drm_bridge_detach(dsi->bridge);
+
 }
 
 static const struct component_ops sun6i_dsi_ops = {
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
index 5c4983212f89..76874ff8e3ef 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
@@ -36,6 +36,7 @@ struct sun6i_dsi {
 	struct sun4i_tcon	*tcon;
 	struct mipi_dsi_device	*device;
 	struct drm_panel	*panel;
+	struct drm_bridge	*bridge;
 	const struct sun6i_dsi_variant	*variant;
 };
 
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH 3/6] drm/sun4i: dsi: Add bridge support

[Paul Kocialkowski]

Hi,

On Fri, 2019-03-15 at 18:38 +0530, Jagan Teki wrote:
> Some display panels would come up with a non-DSI output which
> can have an option to connect DSI interface by means of bridge
> convertor.
> 
> This DSI to non-DSI bridge convertor would require a bridge
> driver that would communicate the DSI controller for bridge
> functionalities.
> 
> So, add support for bridge functionalities in Allwinner DSI
> controller.

See a few comments below.

> Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> ---
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 65 +++++++++++++++++++-------
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  1 +
>  2 files changed, 49 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 0960b96b62cc..64d74313b842 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -781,6 +781,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
>  	if (!IS_ERR(dsi->panel))
>  		drm_panel_prepare(dsi->panel);
>  
> +	if (!IS_ERR(dsi->bridge))
> +		drm_bridge_pre_enable(dsi->bridge);
> +
>  	/*
>  	 * FIXME: This should be moved after the switch to HS mode.
>  	 *
> @@ -796,6 +799,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
>  	if (!IS_ERR(dsi->panel))
>  		drm_panel_enable(dsi->panel);
>  
> +	if (!IS_ERR(dsi->bridge))
> +		drm_bridge_enable(dsi->bridge);
> +
>  	sun6i_dsi_start(dsi, DSI_START_HSC);
>  
>  	udelay(1000);
> @@ -812,6 +818,9 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
>  	if (!IS_ERR(dsi->panel)) {
>  		drm_panel_disable(dsi->panel);
>  		drm_panel_unprepare(dsi->panel);
> +	} else if (!IS_ERR(dsi->bridge)) {
> +		drm_bridge_disable(dsi->bridge);
> +		drm_bridge_post_disable(dsi->bridge);
>  	}
>  
>  	phy_power_off(dsi->dphy);
> @@ -973,11 +982,16 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
>  	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
>  
>  	dsi->device = device;
> -	dsi->panel = of_drm_find_panel(device->dev.of_node);
> -	if (IS_ERR(dsi->panel))
> -		return PTR_ERR(dsi->panel);
>  
> -	dev_info(host->dev, "Attached device %s\n", device->name);
> +	dsi->bridge = of_drm_find_bridge(device->dev.of_node);
> +	if (!dsi->bridge) {

You are using IS_ERR to check that the bridge is alive in the changes
above, but switch to checking that it's non-NULL at this point.

Are both guaranteed to be interchangeable?

> +		dsi->panel = of_drm_find_panel(device->dev.of_node);
> +		if (IS_ERR(dsi->panel))
> +			return PTR_ERR(dsi->panel);
> +	}

You should probably use drm_of_find_panel_or_bridge instead of
duplicating the logic here.

> +	dev_info(host->dev, "Attached %s %s\n",
> +		 dsi->bridge ? "bridge" : "panel", device->name);
>  
>  	return 0;
>  }
> @@ -1055,8 +1069,10 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
>  	struct sun4i_tcon *tcon0 = sun4i_get_tcon0(drm);
>  	int ret;
>  
> -	if (!dsi->panel)
> +	if (!(dsi->panel || dsi->bridge)) {
> +		dev_info(drm->dev, "No panel or bridge found... DSI output disabled\n");
>  		return -EPROBE_DEFER;
> +	}
>  
>  	dsi->drv = drv;
>  
> @@ -1078,19 +1094,29 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
>  	}
>  	dsi->encoder.possible_crtcs = BIT(0);
>  
> -	drm_connector_helper_add(&dsi->connector,
> -				 &sun6i_dsi_connector_helper_funcs);
> -	ret = drm_connector_init(drm, &dsi->connector,
> -				 &sun6i_dsi_connector_funcs,
> -				 DRM_MODE_CONNECTOR_DSI);
> -	if (ret) {
> -		dev_err(dsi->dev,
> -			"Couldn't initialise the DSI connector\n");
> -		goto err_cleanup_connector;
> +	if (dsi->panel) {
> +		drm_connector_helper_add(&dsi->connector,
> +					 &sun6i_dsi_connector_helper_funcs);
> +		ret = drm_connector_init(drm, &dsi->connector,
> +					 &sun6i_dsi_connector_funcs,
> +					 DRM_MODE_CONNECTOR_DSI);
> +		if (ret) {
> +			dev_err(dsi->dev,
> +				"Couldn't initialise the DSI connector\n");
> +			goto err_cleanup_connector;
> +		}
> +
> +		drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
> +		drm_panel_attach(dsi->panel, &dsi->connector);
>  	}
>  
> -	drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
> -	drm_panel_attach(dsi->panel, &dsi->connector);
> +	if (dsi->bridge) {
> +		ret = drm_bridge_attach(&dsi->encoder, dsi->bridge, NULL);
> +		if (ret) {
> +			dev_err(dsi->dev, "Couldn't attach the DSI bridge\n");
> +			goto err_cleanup_connector;
> +		}
> +	}
>  
>  	return 0;
>  
> @@ -1104,7 +1130,12 @@ static void sun6i_dsi_unbind(struct device *dev, struct device *master,
>  {
>  	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
>  
> -	drm_panel_detach(dsi->panel);
> +	if (dsi->panel)
> +		drm_panel_detach(dsi->panel);
> +
> +	if (dsi->bridge)
> +		drm_bridge_detach(dsi->bridge);

As I mentionned in the first patch, this is quite suspicious since the
DRM core should be in charge of detaching the bridge, not the driver.

Cheers,

Paul

> +
>  }
>  
>  static const struct component_ops sun6i_dsi_ops = {
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> index 5c4983212f89..76874ff8e3ef 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> @@ -36,6 +36,7 @@ struct sun6i_dsi {
>  	struct sun4i_tcon	*tcon;
>  	struct mipi_dsi_device	*device;
>  	struct drm_panel	*panel;
> +	struct drm_bridge	*bridge;
>  	const struct sun6i_dsi_variant	*variant;
>  };
>  
> -- 
> 2.18.0.321.gffc6fa0e3
> 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH 3/6] drm/sun4i: dsi: Add bridge support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3163 bytes --]

On Fri, Mar 15, 2019 at 02:32:55PM +0100, Paul Kocialkowski wrote:
> Hi,
> 
> On Fri, 2019-03-15 at 18:38 +0530, Jagan Teki wrote:
> > Some display panels would come up with a non-DSI output which
> > can have an option to connect DSI interface by means of bridge
> > convertor.
> > 
> > This DSI to non-DSI bridge convertor would require a bridge
> > driver that would communicate the DSI controller for bridge
> > functionalities.
> > 
> > So, add support for bridge functionalities in Allwinner DSI
> > controller.
> 
> See a few comments below.
> 
> > Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> > ---
> >  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 65 +++++++++++++++++++-------
> >  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  1 +
> >  2 files changed, 49 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > index 0960b96b62cc..64d74313b842 100644
> > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > @@ -781,6 +781,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
> >  	if (!IS_ERR(dsi->panel))
> >  		drm_panel_prepare(dsi->panel);
> >  
> > +	if (!IS_ERR(dsi->bridge))
> > +		drm_bridge_pre_enable(dsi->bridge);
> > +
> >  	/*
> >  	 * FIXME: This should be moved after the switch to HS mode.
> >  	 *
> > @@ -796,6 +799,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
> >  	if (!IS_ERR(dsi->panel))
> >  		drm_panel_enable(dsi->panel);
> >  
> > +	if (!IS_ERR(dsi->bridge))
> > +		drm_bridge_enable(dsi->bridge);
> > +
> >  	sun6i_dsi_start(dsi, DSI_START_HSC);
> >  
> >  	udelay(1000);
> > @@ -812,6 +818,9 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
> >  	if (!IS_ERR(dsi->panel)) {
> >  		drm_panel_disable(dsi->panel);
> >  		drm_panel_unprepare(dsi->panel);
> > +	} else if (!IS_ERR(dsi->bridge)) {
> > +		drm_bridge_disable(dsi->bridge);
> > +		drm_bridge_post_disable(dsi->bridge);
> >  	}
> >  
> >  	phy_power_off(dsi->dphy);
> > @@ -973,11 +982,16 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> >  	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> >  
> >  	dsi->device = device;
> > -	dsi->panel = of_drm_find_panel(device->dev.of_node);
> > -	if (IS_ERR(dsi->panel))
> > -		return PTR_ERR(dsi->panel);
> >  
> > -	dev_info(host->dev, "Attached device %s\n", device->name);
> > +	dsi->bridge = of_drm_find_bridge(device->dev.of_node);
> > +	if (!dsi->bridge) {
> 
> You are using IS_ERR to check that the bridge is alive in the changes
> above, but switch to checking that it's non-NULL at this point.
> 
> Are both guaranteed to be interchangeable?

They aren't. Any ERR_PTR will be !NULL

> > +		dsi->panel = of_drm_find_panel(device->dev.of_node);
> > +		if (IS_ERR(dsi->panel))
> > +			return PTR_ERR(dsi->panel);
> > +	}
> 
> You should probably use drm_of_find_panel_or_bridge instead of
> duplicating the logic here.

Or we can even use the drm_panel_bridge_add to simplify things.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 3/6] drm/sun4i: dsi: Add bridge support

[Paul Kocialkowski]

Hi,

On Fri, 2019-03-15 at 14:45 +0100, Maxime Ripard wrote:
> On Fri, Mar 15, 2019 at 02:32:55PM +0100, Paul Kocialkowski wrote:
> > Hi,
> > 
> > On Fri, 2019-03-15 at 18:38 +0530, Jagan Teki wrote:
> > > Some display panels would come up with a non-DSI output which
> > > can have an option to connect DSI interface by means of bridge
> > > convertor.
> > > 
> > > This DSI to non-DSI bridge convertor would require a bridge
> > > driver that would communicate the DSI controller for bridge
> > > functionalities.
> > > 
> > > So, add support for bridge functionalities in Allwinner DSI
> > > controller.
> > 
> > See a few comments below.
> > 
> > > Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> > > ---
> > >  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 65 +++++++++++++++++++-------
> > >  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  1 +
> > >  2 files changed, 49 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > index 0960b96b62cc..64d74313b842 100644
> > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > @@ -781,6 +781,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
> > >  	if (!IS_ERR(dsi->panel))
> > >  		drm_panel_prepare(dsi->panel);
> > >  
> > > +	if (!IS_ERR(dsi->bridge))
> > > +		drm_bridge_pre_enable(dsi->bridge);
> > > +
> > >  	/*
> > >  	 * FIXME: This should be moved after the switch to HS mode.
> > >  	 *
> > > @@ -796,6 +799,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
> > >  	if (!IS_ERR(dsi->panel))
> > >  		drm_panel_enable(dsi->panel);
> > >  
> > > +	if (!IS_ERR(dsi->bridge))
> > > +		drm_bridge_enable(dsi->bridge);
> > > +
> > >  	sun6i_dsi_start(dsi, DSI_START_HSC);
> > >  
> > >  	udelay(1000);
> > > @@ -812,6 +818,9 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
> > >  	if (!IS_ERR(dsi->panel)) {
> > >  		drm_panel_disable(dsi->panel);
> > >  		drm_panel_unprepare(dsi->panel);
> > > +	} else if (!IS_ERR(dsi->bridge)) {
> > > +		drm_bridge_disable(dsi->bridge);
> > > +		drm_bridge_post_disable(dsi->bridge);
> > >  	}
> > >  
> > >  	phy_power_off(dsi->dphy);
> > > @@ -973,11 +982,16 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> > >  	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > >  
> > >  	dsi->device = device;
> > > -	dsi->panel = of_drm_find_panel(device->dev.of_node);
> > > -	if (IS_ERR(dsi->panel))
> > > -		return PTR_ERR(dsi->panel);
> > >  
> > > -	dev_info(host->dev, "Attached device %s\n", device->name);
> > > +	dsi->bridge = of_drm_find_bridge(device->dev.of_node);
> > > +	if (!dsi->bridge) {
> > 
> > You are using IS_ERR to check that the bridge is alive in the changes
> > above, but switch to checking that it's non-NULL at this point.
> > 
> > Are both guaranteed to be interchangeable?
> 
> They aren't. Any ERR_PTR will be !NULL
> 
> > > +		dsi->panel = of_drm_find_panel(device->dev.of_node);
> > > +		if (IS_ERR(dsi->panel))
> > > +			return PTR_ERR(dsi->panel);
> > > +	}
> > 
> > You should probably use drm_of_find_panel_or_bridge instead of
> > duplicating the logic here.
> 
> Or we can even use the drm_panel_bridge_add to simplify things.

Indeed, I was just looking at that and it seems to be specifically
tailored for this use case.

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [linux-sunxi] [PATCH 3/6] drm/sun4i: dsi: Add bridge support

[Sergey Suloev]

[-- Attachment #1.1: Type: text/plain, Size: 3400 bytes --]

Hi, Jagan,

On 3/15/19 4:45 PM, Maxime Ripard wrote:
> On Fri, Mar 15, 2019 at 02:32:55PM +0100, Paul Kocialkowski wrote:
>> Hi,
>>
>> On Fri, 2019-03-15 at 18:38 +0530, Jagan Teki wrote:
>>> Some display panels would come up with a non-DSI output which
>>> can have an option to connect DSI interface by means of bridge
>>> convertor.
>>>
>>> This DSI to non-DSI bridge convertor would require a bridge
>>> driver that would communicate the DSI controller for bridge
>>> functionalities.
>>>
>>> So, add support for bridge functionalities in Allwinner DSI
>>> controller.
>> See a few comments below.
>>
>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>> ---
>>>   drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 65 +++++++++++++++++++-------
>>>   drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  1 +
>>>   2 files changed, 49 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>>> index 0960b96b62cc..64d74313b842 100644
>>> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>>> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>>> @@ -781,6 +781,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
>>>   	if (!IS_ERR(dsi->panel))
>>>   		drm_panel_prepare(dsi->panel);
>>>   
>>> +	if (!IS_ERR(dsi->bridge))
>>> +		drm_bridge_pre_enable(dsi->bridge);
>>> +
>>>   	/*
>>>   	 * FIXME: This should be moved after the switch to HS mode.
>>>   	 *
>>> @@ -796,6 +799,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
>>>   	if (!IS_ERR(dsi->panel))
>>>   		drm_panel_enable(dsi->panel);
>>>   
>>> +	if (!IS_ERR(dsi->bridge))
>>> +		drm_bridge_enable(dsi->bridge);
>>> +
>>>   	sun6i_dsi_start(dsi, DSI_START_HSC);
>>>   
>>>   	udelay(1000);
>>> @@ -812,6 +818,9 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
>>>   	if (!IS_ERR(dsi->panel)) {
>>>   		drm_panel_disable(dsi->panel);
>>>   		drm_panel_unprepare(dsi->panel);
>>> +	} else if (!IS_ERR(dsi->bridge)) {
>>> +		drm_bridge_disable(dsi->bridge);
>>> +		drm_bridge_post_disable(dsi->bridge);
>>>   	}
>>>   
>>>   	phy_power_off(dsi->dphy);
>>> @@ -973,11 +982,16 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
>>>   	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
>>>   
>>>   	dsi->device = device;
>>> -	dsi->panel = of_drm_find_panel(device->dev.of_node);
>>> -	if (IS_ERR(dsi->panel))
>>> -		return PTR_ERR(dsi->panel);
>>>   
>>> -	dev_info(host->dev, "Attached device %s\n", device->name);
>>> +	dsi->bridge = of_drm_find_bridge(device->dev.of_node);
>>> +	if (!dsi->bridge) {
>> You are using IS_ERR to check that the bridge is alive in the changes
>> above, but switch to checking that it's non-NULL at this point.
>>
>> Are both guaranteed to be interchangeable?
> They aren't. Any ERR_PTR will be !NULL
>
>>> +		dsi->panel = of_drm_find_panel(device->dev.of_node);
>>> +		if (IS_ERR(dsi->panel))
>>> +			return PTR_ERR(dsi->panel);
>>> +	}
>> You should probably use drm_of_find_panel_or_bridge instead of
>> duplicating the logic here.
> Or we can even use the drm_panel_bridge_add to simplify things.

this approach is implemented by E.Anholt in VC4 dsi driver and it looks 
nice.

>
> Maxime
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[-- Attachment #1.2: Type: text/html, Size: 4839 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [linux-sunxi] [PATCH 3/6] drm/sun4i: dsi: Add bridge support

[Jagan Teki]

Hi Paul and Maxime,

On Fri, Mar 15, 2019 at 7:03 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> On Fri, 2019-03-15 at 18:38 +0530, Jagan Teki wrote:
> > Some display panels would come up with a non-DSI output which
> > can have an option to connect DSI interface by means of bridge
> > convertor.
> >
> > This DSI to non-DSI bridge convertor would require a bridge
> > driver that would communicate the DSI controller for bridge
> > functionalities.
> >
> > So, add support for bridge functionalities in Allwinner DSI
> > controller.
>
> See a few comments below.
>
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 65 +++++++++++++++++++-------
> >  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  1 +
> >  2 files changed, 49 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > index 0960b96b62cc..64d74313b842 100644
> > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > @@ -781,6 +781,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
> >       if (!IS_ERR(dsi->panel))
> >               drm_panel_prepare(dsi->panel);
> >
> > +     if (!IS_ERR(dsi->bridge))
> > +             drm_bridge_pre_enable(dsi->bridge);
> > +
> >       /*
> >        * FIXME: This should be moved after the switch to HS mode.
> >        *
> > @@ -796,6 +799,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
> >       if (!IS_ERR(dsi->panel))
> >               drm_panel_enable(dsi->panel);
> >
> > +     if (!IS_ERR(dsi->bridge))
> > +             drm_bridge_enable(dsi->bridge);
> > +
> >       sun6i_dsi_start(dsi, DSI_START_HSC);
> >
> >       udelay(1000);
> > @@ -812,6 +818,9 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
> >       if (!IS_ERR(dsi->panel)) {
> >               drm_panel_disable(dsi->panel);
> >               drm_panel_unprepare(dsi->panel);
> > +     } else if (!IS_ERR(dsi->bridge)) {
> > +             drm_bridge_disable(dsi->bridge);
> > +             drm_bridge_post_disable(dsi->bridge);
> >       }
> >
> >       phy_power_off(dsi->dphy);
> > @@ -973,11 +982,16 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> >       struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> >
> >       dsi->device = device;
> > -     dsi->panel = of_drm_find_panel(device->dev.of_node);
> > -     if (IS_ERR(dsi->panel))
> > -             return PTR_ERR(dsi->panel);
> >
> > -     dev_info(host->dev, "Attached device %s\n", device->name);
> > +     dsi->bridge = of_drm_find_bridge(device->dev.of_node);
> > +     if (!dsi->bridge) {
>
> You are using IS_ERR to check that the bridge is alive in the changes
> above, but switch to checking that it's non-NULL at this point.
>
> Are both guaranteed to be interchangeable?
>
> > +             dsi->panel = of_drm_find_panel(device->dev.of_node);
> > +             if (IS_ERR(dsi->panel))
> > +                     return PTR_ERR(dsi->panel);
> > +     }
>
> You should probably use drm_of_find_panel_or_bridge instead of
> duplicating the logic here.

True, In-fact I did try this API. but pipeline were unable to bound.
Usually the panel and bridge were attached first and then the pipeline
bound would start from front-end (in A33) But in my below cases I have
seen only panel or bridge attached but no pipeline bound at all.

And I'm using drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0,
&dsi->panel, &dsi->bridge); in dsi attach API.

Case-1, panel:

&dsi {
    vcc-dsi-supply = <&reg_dcdc1>;        /* VCC3V3-DSI */
    status = "okay";

    ports {
        dsi_out: port@1 {
            reg = <1>;

            dsi_out_panel: endpoint {
                remote-endpoint = <&panel_out_dsi>;
            };
        };
    };

    panel@0 {
        compatible = "bananapi,s070wv20-ct16-icn6211";
        reg = <0>;
        enable-gpios = <&pio 1 7 GPIO_ACTIVE_HIGH>; /* LCD-PWR-EN: PB7 */
        reset-gpios = <&r_pio 0 5 GPIO_ACTIVE_HIGH>; /* LCD-RST: PL5 */
        backlight = <&backlight>;

        port {
            panel_out_dsi: endpoint {
                remote-endpoint = <&dsi_out_panel>;
            };
        };
    };
};

Case-2, bridge:

    panel {
        compatible = "bananapi,s070wv20-ct16", "simple-panel";
        enable-gpios = <&pio 1 7 GPIO_ACTIVE_HIGH>; /* LCD-PWR-EN: PB7 */
        backlight = <&backlight>;

        port {

            panel_out_bridge: endpoint {
                remote-endpoint = <&bridge_out_panel>;
            };
        };
    };

&dsi {
    vcc-dsi-supply = <&reg_dcdc1>;        /* VCC-DSI */
    status = "okay";

    ports {
        dsi_out: port@1 {
            reg = <1>;

            dsi_out_bridge: endpoint {
                remote-endpoint = <&bridge_out_dsi>;
            };
        };
    };

    bridge@0 {
        reg = <0>;
        compatible = "bananapi,icn6211", "chipone,icn6211";
        reset-gpios = <&r_pio 0 5 GPIO_ACTIVE_HIGH>; /* LCD-RST: PL5 */
        #address-cells = <1>;
        #size-cells = <0>;

        ports {
            #address-cells = <1>;
            #size-cells = <0>;

            bridge_in: port@0 {
                reg = <0>;

                bridge_out_dsi: endpoint {
                    remote-endpoint = <&dsi_out_bridge>;
                };
            };

            bridge_out: port@1 {
                reg = <1>;

                bridge_out_panel: endpoint {
                    remote-endpoint = <&panel_out_bridge>;
                };
            };
        };
    };
};

I think, I'm sure about the pipeline connections as per my
understanding. but something loosely missed here or in the code.
Please do let me know for any suggestions.

Jagan.

[PATCH 5/6] drm/bridge: Add Chipone ICN6211 MIPI-DSI/RGB convertor bridge

[Jagan Teki]

ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
It has a flexible configuration of MIPI DSI signal input
and produce RGB565, RGB666, RGB888 output format.

Add bridge driver for it.

Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
---
 MAINTAINERS                              |   6 +
 drivers/gpu/drm/bridge/Kconfig           |  10 +
 drivers/gpu/drm/bridge/Makefile          |   1 +
 drivers/gpu/drm/bridge/chipone-icn6211.c | 275 +++++++++++++++++++++++
 4 files changed, 292 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/chipone-icn6211.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4de80222cffb..e529addd30f5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4897,6 +4897,12 @@ T:	git git://anongit.freedesktop.org/drm/drm-misc
 S:	Maintained
 F:	drivers/gpu/drm/bochs/
 
+DRM DRIVER FOR CHIPONE ICN6211 MIPI-DSI to RGB CONVERTOR BRIDGE
+M:	Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
+S:	Maintained
+F:	drivers/gpu/drm/bridge/chipone-icn6211.c
+F:	Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
+
 DRM DRIVER FOR FARADAY TVE200 TV ENCODER
 M:	Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
 T:	git git://anongit.freedesktop.org/drm/drm-misc
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 8840f396a7b6..cd314572e4ed 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -36,6 +36,16 @@ config DRM_CDNS_DSI
 	  Support Cadence DPI to DSI bridge. This is an internal
 	  bridge and is meant to be directly embedded in a SoC.
 
+config DRM_CHIPONE_ICN6211
+	tristate "Chipone ICN6211 MIPI-DSI/RGB converter bridge"
+	depends on DRM && DRM_PANEL
+	depends on OF
+	select DRM_MIPI_DSI
+	help
+	  ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
+	  It has a flexible configuration of MIPI DSI signal input
+	  and produce RGB565, RGB666, RGB888 output format.
+
 config DRM_DUMB_VGA_DAC
 	tristate "Dumb VGA DAC Bridge support"
 	depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 4934fcf5a6f8..541fdccad10b 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
 obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
+obj-$(CONFIG_DRM_CHIPONE_ICN6211) += chipone-icn6211.o
 obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
 obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
 obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
new file mode 100644
index 000000000000..cd2f3505f845
--- /dev/null
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -0,0 +1,275 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2018 Amarula Solutions
+ * Author: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
+ */
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_of.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_print.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_mipi_dsi.h>
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of_graph.h>
+
+#define ICN6211_INIT_CMD_LEN		2
+
+struct chipone {
+	struct device *dev;
+	struct drm_bridge bridge;
+	struct drm_connector connector;
+	struct drm_panel *panel;
+
+	struct gpio_desc *reset;
+};
+
+static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct chipone, bridge);
+}
+
+static inline
+struct chipone *connector_to_chipone(struct drm_connector *connector)
+{
+	return container_of(connector, struct chipone, connector);
+}
+
+struct icn6211_init_cmd {
+	u8 data[ICN6211_INIT_CMD_LEN];
+};
+
+static const struct icn6211_init_cmd icn6211_init_cmds[] = {
+	{ .data = { 0x7A, 0xC1 } },
+	{ .data = { 0x20, 0x20 } },
+	{ .data = { 0x21, 0xE0 } },
+	{ .data = { 0x22, 0x13 } },
+	{ .data = { 0x23, 0x28 } },
+	{ .data = { 0x24, 0x30 } },
+	{ .data = { 0x25, 0x28 } },
+	{ .data = { 0x26, 0x00 } },
+	{ .data = { 0x27, 0x0D } },
+	{ .data = { 0x28, 0x03 } },
+	{ .data = { 0x29, 0x1D } },
+	{ .data = { 0x34, 0x80 } },
+	{ .data = { 0x36, 0x28 } },
+	{ .data = { 0xB5, 0xA0 } },
+	{ .data = { 0x5C, 0xFF } },
+	{ .data = { 0x2A, 0x01 } },
+	{ .data = { 0x56, 0x92 } },
+	{ .data = { 0x6B, 0x71 } },
+	{ .data = { 0x69, 0x2B } },
+	{ .data = { 0x10, 0x40 } },
+	{ .data = { 0x11, 0x98 } },
+	{ .data = { 0xB6, 0x20 } },
+	{ .data = { 0x51, 0x20 } },
+	{ .data = { 0x09, 0x10 } },
+};
+
+static int chipone_get_modes(struct drm_connector *connector)
+{
+	struct chipone *icn = connector_to_chipone(connector);
+
+	return drm_panel_get_modes(icn->panel);
+}
+
+static const
+struct drm_connector_helper_funcs chipone_connector_helper_funcs = {
+	.get_modes = chipone_get_modes,
+};
+
+static const struct drm_connector_funcs chipone_connector_funcs = {
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static void chipone_disable(struct drm_bridge *bridge)
+{
+	struct chipone *icn = bridge_to_chipone(bridge);
+	int ret = drm_panel_disable(bridge_to_chipone(bridge)->panel);
+
+	if (ret < 0)
+		DRM_DEV_ERROR(icn->dev, "error disabling panel (%d)\n", ret);
+}
+
+static void chipone_post_disable(struct drm_bridge *bridge)
+{
+	struct chipone *icn = bridge_to_chipone(bridge);
+	int ret;
+
+	ret = drm_panel_unprepare(icn->panel);
+	if (ret < 0)
+		DRM_DEV_ERROR(icn->dev, "error unpreparing panel (%d)\n", ret);
+
+	msleep(50);
+
+	gpiod_set_value(icn->reset, 0);
+}
+
+static void chipone_pre_enable(struct drm_bridge *bridge)
+{
+	struct chipone *icn = bridge_to_chipone(bridge);
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(icn->dev);
+	unsigned int i;
+	int ret;
+
+	gpiod_set_value(icn->reset, 0);
+	msleep(20);
+
+	gpiod_set_value(icn->reset, 1);
+	msleep(50);
+
+	for (i = 0; i < ARRAY_SIZE(icn6211_init_cmds); i++) {
+		const struct icn6211_init_cmd *cmd = &icn6211_init_cmds[i];
+
+		ret = mipi_dsi_generic_write(dsi, cmd->data,
+					     ICN6211_INIT_CMD_LEN);
+		if (ret < 0) {
+			DRM_DEV_ERROR(icn->dev,
+				      "failed to write cmd %d: %d\n", i, ret);
+			return;
+		}
+	}
+
+	ret = drm_panel_prepare(icn->panel);
+	if (ret < 0)
+		DRM_DEV_ERROR(icn->dev, "error preparing panel (%d)\n", ret);
+}
+
+static void chipone_enable(struct drm_bridge *bridge)
+{
+	struct chipone *icn = bridge_to_chipone(bridge);
+	int ret = drm_panel_enable(icn->panel);
+
+	if (ret < 0)
+		DRM_DEV_ERROR(icn->dev, "error enabling panel (%d)\n", ret);
+}
+
+static int chipone_attach(struct drm_bridge *bridge)
+{
+	struct chipone *icn = bridge_to_chipone(bridge);
+	struct drm_device *drm = bridge->dev;
+	int ret;
+
+	icn->connector.polled = DRM_CONNECTOR_POLL_HPD;
+	ret = drm_connector_init(drm, &icn->connector,
+				 &chipone_connector_funcs,
+				 DRM_MODE_CONNECTOR_Unknown);
+	if (ret) {
+		DRM_ERROR("Failed to initialize connector\n");
+		return ret;
+	}
+
+	drm_connector_helper_add(&icn->connector,
+				 &chipone_connector_helper_funcs);
+	drm_connector_attach_encoder(&icn->connector, bridge->encoder);
+	drm_panel_attach(icn->panel, &icn->connector);
+	icn->connector.funcs->reset(&icn->connector);
+	drm_fb_helper_add_one_connector(drm->fb_helper, &icn->connector);
+	drm_connector_register(&icn->connector);
+
+	return 0;
+}
+
+static void chipone_detach(struct drm_bridge *bridge)
+{
+	struct chipone *icn = bridge_to_chipone(bridge);
+	struct drm_device *drm = bridge->dev;
+
+	drm_connector_unregister(&icn->connector);
+	drm_fb_helper_remove_one_connector(drm->fb_helper, &icn->connector);
+	drm_panel_detach(icn->panel);
+	icn->panel = NULL;
+	drm_connector_put(&icn->connector);
+}
+
+static const struct drm_bridge_funcs chipone_bridge_funcs = {
+	.disable = chipone_disable,
+	.post_disable = chipone_post_disable,
+	.enable = chipone_enable,
+	.pre_enable = chipone_pre_enable,
+	.attach = chipone_attach,
+	.detach = chipone_detach,
+};
+
+static int chipone_probe(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	struct chipone *icn;
+	int ret;
+
+	icn = devm_kzalloc(dev, sizeof(struct chipone), GFP_KERNEL);
+	if (!icn)
+		return -ENOMEM;
+
+	mipi_dsi_set_drvdata(dsi, icn);
+
+	icn->dev = dev;
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
+
+	icn->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(icn->reset)) {
+		DRM_DEV_ERROR(dev, "no reset GPIO pin provided\n");
+		return PTR_ERR(icn->reset);
+	}
+
+	ret = drm_of_find_panel_or_bridge(icn->dev->of_node, 1, 0,
+					  &icn->panel, NULL);
+	if (ret && ret != -EPROBE_DEFER) {
+		DRM_DEV_ERROR(dev, "failed to find panel (ret = %d)\n", ret);
+		return ret;
+	}
+
+	icn->bridge.funcs = &chipone_bridge_funcs;
+	icn->bridge.of_node = dev->of_node;
+
+	drm_bridge_add(&icn->bridge);
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		drm_bridge_remove(&icn->bridge);
+		DRM_DEV_ERROR(dev, "failed to attach dsi (ret = %d)\n", ret);
+	}
+
+	return ret;
+}
+
+static int chipone_remove(struct mipi_dsi_device *dsi)
+{
+	struct chipone *icn = mipi_dsi_get_drvdata(dsi);
+
+	mipi_dsi_detach(dsi);
+	drm_bridge_remove(&icn->bridge);
+
+	return 0;
+}
+
+static const struct of_device_id chipone_of_match[] = {
+	{ .compatible = "bananapi,icn6211" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, chipone_of_match);
+
+static struct mipi_dsi_driver chipone_driver = {
+	.probe = chipone_probe,
+	.remove = chipone_remove,
+	.driver = {
+		.name = "chipone-icn6211",
+		.owner = THIS_MODULE,
+		.of_match_table = chipone_of_match,
+	},
+};
+module_mipi_dsi_driver(chipone_driver);
+
+MODULE_AUTHOR("Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>");
+MODULE_DESCRIPTION("Chipone ICN6211 MIPI-DSI to RGB Convertor Bridge");
+MODULE_LICENSE("GPL v2");
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH 5/6] drm/bridge: Add Chipone ICN6211 MIPI-DSI/RGB convertor bridge

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 10806 bytes --]

On Fri, Mar 15, 2019 at 06:38:24PM +0530, Jagan Teki wrote:
> ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
> It has a flexible configuration of MIPI DSI signal input
> and produce RGB565, RGB666, RGB888 output format.
> 
> Add bridge driver for it.
> 
> Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> ---
>  MAINTAINERS                              |   6 +
>  drivers/gpu/drm/bridge/Kconfig           |  10 +
>  drivers/gpu/drm/bridge/Makefile          |   1 +
>  drivers/gpu/drm/bridge/chipone-icn6211.c | 275 +++++++++++++++++++++++
>  4 files changed, 292 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/chipone-icn6211.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4de80222cffb..e529addd30f5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4897,6 +4897,12 @@ T:	git git://anongit.freedesktop.org/drm/drm-misc
>  S:	Maintained
>  F:	drivers/gpu/drm/bochs/
>  
> +DRM DRIVER FOR CHIPONE ICN6211 MIPI-DSI to RGB CONVERTOR BRIDGE
> +M:	Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> +S:	Maintained
> +F:	drivers/gpu/drm/bridge/chipone-icn6211.c
> +F:	Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> +
>  DRM DRIVER FOR FARADAY TVE200 TV ENCODER
>  M:	Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>  T:	git git://anongit.freedesktop.org/drm/drm-misc
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 8840f396a7b6..cd314572e4ed 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -36,6 +36,16 @@ config DRM_CDNS_DSI
>  	  Support Cadence DPI to DSI bridge. This is an internal
>  	  bridge and is meant to be directly embedded in a SoC.
>  
> +config DRM_CHIPONE_ICN6211
> +	tristate "Chipone ICN6211 MIPI-DSI/RGB converter bridge"
> +	depends on DRM && DRM_PANEL
> +	depends on OF
> +	select DRM_MIPI_DSI
> +	help
> +	  ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
> +	  It has a flexible configuration of MIPI DSI signal input
> +	  and produce RGB565, RGB666, RGB888 output format.
> +
>  config DRM_DUMB_VGA_DAC
>  	tristate "Dumb VGA DAC Bridge support"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 4934fcf5a6f8..541fdccad10b 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
>  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
> +obj-$(CONFIG_DRM_CHIPONE_ICN6211) += chipone-icn6211.o
>  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
>  obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
>  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
> new file mode 100644
> index 000000000000..cd2f3505f845
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> @@ -0,0 +1,275 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018 Amarula Solutions
> + * Author: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> + */
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_mipi_dsi.h>
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of_graph.h>
> +
> +#define ICN6211_INIT_CMD_LEN		2
> +
> +struct chipone {
> +	struct device *dev;
> +	struct drm_bridge bridge;
> +	struct drm_connector connector;
> +	struct drm_panel *panel;
> +
> +	struct gpio_desc *reset;
> +};
> +
> +static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct chipone, bridge);
> +}
> +
> +static inline
> +struct chipone *connector_to_chipone(struct drm_connector *connector)
> +{
> +	return container_of(connector, struct chipone, connector);
> +}
> +
> +struct icn6211_init_cmd {
> +	u8 data[ICN6211_INIT_CMD_LEN];
> +};
> +
> +static const struct icn6211_init_cmd icn6211_init_cmds[] = {
> +	{ .data = { 0x7A, 0xC1 } },
> +	{ .data = { 0x20, 0x20 } },
> +	{ .data = { 0x21, 0xE0 } },
> +	{ .data = { 0x22, 0x13 } },
> +	{ .data = { 0x23, 0x28 } },
> +	{ .data = { 0x24, 0x30 } },
> +	{ .data = { 0x25, 0x28 } },
> +	{ .data = { 0x26, 0x00 } },
> +	{ .data = { 0x27, 0x0D } },
> +	{ .data = { 0x28, 0x03 } },
> +	{ .data = { 0x29, 0x1D } },
> +	{ .data = { 0x34, 0x80 } },
> +	{ .data = { 0x36, 0x28 } },
> +	{ .data = { 0xB5, 0xA0 } },
> +	{ .data = { 0x5C, 0xFF } },
> +	{ .data = { 0x2A, 0x01 } },
> +	{ .data = { 0x56, 0x92 } },
> +	{ .data = { 0x6B, 0x71 } },
> +	{ .data = { 0x69, 0x2B } },
> +	{ .data = { 0x10, 0x40 } },
> +	{ .data = { 0x11, 0x98 } },
> +	{ .data = { 0xB6, 0x20 } },
> +	{ .data = { 0x51, 0x20 } },
> +	{ .data = { 0x09, 0x10 } },
> +};

What are those commands supposed to be? Some of them at least look
pretty standard (and have proper functions):
MIPI_DCS_EXIT_INVERT_MODE, _SET_DISPLAY_ON, _SET_TEAR_OFF, etc.

> +static int chipone_get_modes(struct drm_connector *connector)
> +{
> +	struct chipone *icn = connector_to_chipone(connector);
> +
> +	return drm_panel_get_modes(icn->panel);
> +}
> +
> +static const
> +struct drm_connector_helper_funcs chipone_connector_helper_funcs = {
> +	.get_modes = chipone_get_modes,
> +};
> +
> +static const struct drm_connector_funcs chipone_connector_funcs = {
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.destroy = drm_connector_cleanup,
> +	.reset = drm_atomic_helper_connector_reset,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static void chipone_disable(struct drm_bridge *bridge)
> +{
> +	struct chipone *icn = bridge_to_chipone(bridge);
> +	int ret = drm_panel_disable(bridge_to_chipone(bridge)->panel);
> +
> +	if (ret < 0)
> +		DRM_DEV_ERROR(icn->dev, "error disabling panel (%d)\n", ret);
> +}
> +
> +static void chipone_post_disable(struct drm_bridge *bridge)
> +{
> +	struct chipone *icn = bridge_to_chipone(bridge);
> +	int ret;
> +
> +	ret = drm_panel_unprepare(icn->panel);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(icn->dev, "error unpreparing panel (%d)\n", ret);
> +
> +	msleep(50);
> +
> +	gpiod_set_value(icn->reset, 0);
> +}
> +
> +static void chipone_pre_enable(struct drm_bridge *bridge)
> +{
> +	struct chipone *icn = bridge_to_chipone(bridge);
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(icn->dev);
> +	unsigned int i;
> +	int ret;
> +
> +	gpiod_set_value(icn->reset, 0);
> +	msleep(20);
> +
> +	gpiod_set_value(icn->reset, 1);
> +	msleep(50);
> +
> +	for (i = 0; i < ARRAY_SIZE(icn6211_init_cmds); i++) {
> +		const struct icn6211_init_cmd *cmd = &icn6211_init_cmds[i];
> +
> +		ret = mipi_dsi_generic_write(dsi, cmd->data,
> +					     ICN6211_INIT_CMD_LEN);
> +		if (ret < 0) {
> +			DRM_DEV_ERROR(icn->dev,
> +				      "failed to write cmd %d: %d\n", i, ret);
> +			return;
> +		}
> +	}
> +
> +	ret = drm_panel_prepare(icn->panel);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(icn->dev, "error preparing panel (%d)\n", ret);
> +}
> +
> +static void chipone_enable(struct drm_bridge *bridge)
> +{
> +	struct chipone *icn = bridge_to_chipone(bridge);
> +	int ret = drm_panel_enable(icn->panel);
> +
> +	if (ret < 0)
> +		DRM_DEV_ERROR(icn->dev, "error enabling panel (%d)\n", ret);
> +}
> +
> +static int chipone_attach(struct drm_bridge *bridge)
> +{
> +	struct chipone *icn = bridge_to_chipone(bridge);
> +	struct drm_device *drm = bridge->dev;
> +	int ret;
> +
> +	icn->connector.polled = DRM_CONNECTOR_POLL_HPD;
> +	ret = drm_connector_init(drm, &icn->connector,
> +				 &chipone_connector_funcs,
> +				 DRM_MODE_CONNECTOR_Unknown);

It should be a DRM_MODE_CONNECTOR_DPI connector.

> +	if (ret) {
> +		DRM_ERROR("Failed to initialize connector\n");
> +		return ret;
> +	}
> +
> +	drm_connector_helper_add(&icn->connector,
> +				 &chipone_connector_helper_funcs);
> +	drm_connector_attach_encoder(&icn->connector, bridge->encoder);
> +	drm_panel_attach(icn->panel, &icn->connector);
> +	icn->connector.funcs->reset(&icn->connector);
> +	drm_fb_helper_add_one_connector(drm->fb_helper, &icn->connector);

What do you need that for?

> +	drm_connector_register(&icn->connector);
> +
> +	return 0;
> +}
> +
> +static void chipone_detach(struct drm_bridge *bridge)
> +{
> +	struct chipone *icn = bridge_to_chipone(bridge);
> +	struct drm_device *drm = bridge->dev;
> +
> +	drm_connector_unregister(&icn->connector);
> +	drm_fb_helper_remove_one_connector(drm->fb_helper, &icn->connector);
> +	drm_panel_detach(icn->panel);
> +	icn->panel = NULL;
> +	drm_connector_put(&icn->connector);
> +}
> +
> +static const struct drm_bridge_funcs chipone_bridge_funcs = {
> +	.disable = chipone_disable,
> +	.post_disable = chipone_post_disable,
> +	.enable = chipone_enable,
> +	.pre_enable = chipone_pre_enable,
> +	.attach = chipone_attach,
> +	.detach = chipone_detach,
> +};
> +
> +static int chipone_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct chipone *icn;
> +	int ret;
> +
> +	icn = devm_kzalloc(dev, sizeof(struct chipone), GFP_KERNEL);
> +	if (!icn)
> +		return -ENOMEM;
> +
> +	mipi_dsi_set_drvdata(dsi, icn);
> +
> +	icn->dev = dev;
> +	dsi->lanes = 4;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> +
> +	icn->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(icn->reset)) {
> +		DRM_DEV_ERROR(dev, "no reset GPIO pin provided\n");
> +		return PTR_ERR(icn->reset);
> +	}
> +
> +	ret = drm_of_find_panel_or_bridge(icn->dev->of_node, 1, 0,
> +					  &icn->panel, NULL);
> +	if (ret && ret != -EPROBE_DEFER) {
> +		DRM_DEV_ERROR(dev, "failed to find panel (ret = %d)\n", ret);
> +		return ret;
> +	}
> +
> +	icn->bridge.funcs = &chipone_bridge_funcs;
> +	icn->bridge.of_node = dev->of_node;
> +
> +	drm_bridge_add(&icn->bridge);
> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0) {
> +		drm_bridge_remove(&icn->bridge);
> +		DRM_DEV_ERROR(dev, "failed to attach dsi (ret = %d)\n", ret);
> +	}
> +
> +	return ret;
> +}
> +
> +static int chipone_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct chipone *icn = mipi_dsi_get_drvdata(dsi);
> +
> +	mipi_dsi_detach(dsi);
> +	drm_bridge_remove(&icn->bridge);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id chipone_of_match[] = {
> +	{ .compatible = "bananapi,icn6211" },

This should have your generic compatible listed

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 5/6] drm/bridge: Add Chipone ICN6211 MIPI-DSI/RGB convertor bridge

[Laurent Pinchart]

Hello,

On Fri, Mar 15, 2019 at 02:33:04PM +0100, Maxime Ripard wrote:
> On Fri, Mar 15, 2019 at 06:38:24PM +0530, Jagan Teki wrote:
> > ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
> > It has a flexible configuration of MIPI DSI signal input
> > and produce RGB565, RGB666, RGB888 output format.
> > 
> > Add bridge driver for it.
> > 
> > Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> > ---
> >  MAINTAINERS                              |   6 +
> >  drivers/gpu/drm/bridge/Kconfig           |  10 +
> >  drivers/gpu/drm/bridge/Makefile          |   1 +
> >  drivers/gpu/drm/bridge/chipone-icn6211.c | 275 +++++++++++++++++++++++
> >  4 files changed, 292 insertions(+)
> >  create mode 100644 drivers/gpu/drm/bridge/chipone-icn6211.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 4de80222cffb..e529addd30f5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4897,6 +4897,12 @@ T:	git git://anongit.freedesktop.org/drm/drm-misc
> >  S:	Maintained
> >  F:	drivers/gpu/drm/bochs/
> >  
> > +DRM DRIVER FOR CHIPONE ICN6211 MIPI-DSI to RGB CONVERTOR BRIDGE
> > +M:	Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> > +S:	Maintained
> > +F:	drivers/gpu/drm/bridge/chipone-icn6211.c
> > +F:	Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> > +
> >  DRM DRIVER FOR FARADAY TVE200 TV ENCODER
> >  M:	Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >  T:	git git://anongit.freedesktop.org/drm/drm-misc
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index 8840f396a7b6..cd314572e4ed 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -36,6 +36,16 @@ config DRM_CDNS_DSI
> >  	  Support Cadence DPI to DSI bridge. This is an internal
> >  	  bridge and is meant to be directly embedded in a SoC.
> >  
> > +config DRM_CHIPONE_ICN6211
> > +	tristate "Chipone ICN6211 MIPI-DSI/RGB converter bridge"
> > +	depends on DRM && DRM_PANEL
> > +	depends on OF
> > +	select DRM_MIPI_DSI
> > +	help
> > +	  ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
> > +	  It has a flexible configuration of MIPI DSI signal input
> > +	  and produce RGB565, RGB666, RGB888 output format.
> > +
> >  config DRM_DUMB_VGA_DAC
> >  	tristate "Dumb VGA DAC Bridge support"
> >  	depends on OF
> > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > index 4934fcf5a6f8..541fdccad10b 100644
> > --- a/drivers/gpu/drm/bridge/Makefile
> > +++ b/drivers/gpu/drm/bridge/Makefile
> > @@ -1,6 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
> >  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
> > +obj-$(CONFIG_DRM_CHIPONE_ICN6211) += chipone-icn6211.o
> >  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> >  obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
> >  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
> > diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
> > new file mode 100644
> > index 000000000000..cd2f3505f845
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> > @@ -0,0 +1,275 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2018 Amarula Solutions
> > + * Author: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> > + */
> > +
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_fb_helper.h>
> > +#include <drm/drm_of.h>
> > +#include <drm/drm_panel.h>
> > +#include <drm/drm_print.h>
> > +#include <drm/drm_probe_helper.h>
> > +#include <drm/drm_mipi_dsi.h>
> > +
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/module.h>
> > +#include <linux/of_graph.h>
> > +
> > +#define ICN6211_INIT_CMD_LEN		2
> > +
> > +struct chipone {
> > +	struct device *dev;
> > +	struct drm_bridge bridge;
> > +	struct drm_connector connector;
> > +	struct drm_panel *panel;
> > +
> > +	struct gpio_desc *reset;
> > +};
> > +
> > +static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
> > +{
> > +	return container_of(bridge, struct chipone, bridge);
> > +}
> > +
> > +static inline
> > +struct chipone *connector_to_chipone(struct drm_connector *connector)
> > +{
> > +	return container_of(connector, struct chipone, connector);
> > +}
> > +
> > +struct icn6211_init_cmd {
> > +	u8 data[ICN6211_INIT_CMD_LEN];
> > +};
> > +
> > +static const struct icn6211_init_cmd icn6211_init_cmds[] = {
> > +	{ .data = { 0x7A, 0xC1 } },
> > +	{ .data = { 0x20, 0x20 } },
> > +	{ .data = { 0x21, 0xE0 } },
> > +	{ .data = { 0x22, 0x13 } },
> > +	{ .data = { 0x23, 0x28 } },
> > +	{ .data = { 0x24, 0x30 } },
> > +	{ .data = { 0x25, 0x28 } },
> > +	{ .data = { 0x26, 0x00 } },
> > +	{ .data = { 0x27, 0x0D } },
> > +	{ .data = { 0x28, 0x03 } },
> > +	{ .data = { 0x29, 0x1D } },
> > +	{ .data = { 0x34, 0x80 } },
> > +	{ .data = { 0x36, 0x28 } },
> > +	{ .data = { 0xB5, 0xA0 } },
> > +	{ .data = { 0x5C, 0xFF } },
> > +	{ .data = { 0x2A, 0x01 } },
> > +	{ .data = { 0x56, 0x92 } },
> > +	{ .data = { 0x6B, 0x71 } },
> > +	{ .data = { 0x69, 0x2B } },
> > +	{ .data = { 0x10, 0x40 } },
> > +	{ .data = { 0x11, 0x98 } },
> > +	{ .data = { 0xB6, 0x20 } },
> > +	{ .data = { 0x51, 0x20 } },
> > +	{ .data = { 0x09, 0x10 } },
> > +};
> 
> What are those commands supposed to be? Some of them at least look
> pretty standard (and have proper functions):
> MIPI_DCS_EXIT_INVERT_MODE, _SET_DISPLAY_ON, _SET_TEAR_OFF, etc.
> 
> > +static int chipone_get_modes(struct drm_connector *connector)
> > +{
> > +	struct chipone *icn = connector_to_chipone(connector);
> > +
> > +	return drm_panel_get_modes(icn->panel);
> > +}
> > +
> > +static const
> > +struct drm_connector_helper_funcs chipone_connector_helper_funcs = {
> > +	.get_modes = chipone_get_modes,
> > +};
> > +
> > +static const struct drm_connector_funcs chipone_connector_funcs = {
> > +	.fill_modes = drm_helper_probe_single_connector_modes,
> > +	.destroy = drm_connector_cleanup,
> > +	.reset = drm_atomic_helper_connector_reset,
> > +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > +};
> > +
> > +static void chipone_disable(struct drm_bridge *bridge)
> > +{
> > +	struct chipone *icn = bridge_to_chipone(bridge);
> > +	int ret = drm_panel_disable(bridge_to_chipone(bridge)->panel);
> > +
> > +	if (ret < 0)
> > +		DRM_DEV_ERROR(icn->dev, "error disabling panel (%d)\n", ret);
> > +}
> > +
> > +static void chipone_post_disable(struct drm_bridge *bridge)
> > +{
> > +	struct chipone *icn = bridge_to_chipone(bridge);
> > +	int ret;
> > +
> > +	ret = drm_panel_unprepare(icn->panel);
> > +	if (ret < 0)
> > +		DRM_DEV_ERROR(icn->dev, "error unpreparing panel (%d)\n", ret);
> > +
> > +	msleep(50);
> > +
> > +	gpiod_set_value(icn->reset, 0);
> > +}
> > +
> > +static void chipone_pre_enable(struct drm_bridge *bridge)
> > +{
> > +	struct chipone *icn = bridge_to_chipone(bridge);
> > +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(icn->dev);
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	gpiod_set_value(icn->reset, 0);
> > +	msleep(20);
> > +
> > +	gpiod_set_value(icn->reset, 1);
> > +	msleep(50);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(icn6211_init_cmds); i++) {
> > +		const struct icn6211_init_cmd *cmd = &icn6211_init_cmds[i];
> > +
> > +		ret = mipi_dsi_generic_write(dsi, cmd->data,
> > +					     ICN6211_INIT_CMD_LEN);
> > +		if (ret < 0) {
> > +			DRM_DEV_ERROR(icn->dev,
> > +				      "failed to write cmd %d: %d\n", i, ret);
> > +			return;
> > +		}
> > +	}
> > +
> > +	ret = drm_panel_prepare(icn->panel);
> > +	if (ret < 0)
> > +		DRM_DEV_ERROR(icn->dev, "error preparing panel (%d)\n", ret);
> > +}
> > +
> > +static void chipone_enable(struct drm_bridge *bridge)
> > +{
> > +	struct chipone *icn = bridge_to_chipone(bridge);
> > +	int ret = drm_panel_enable(icn->panel);
> > +
> > +	if (ret < 0)
> > +		DRM_DEV_ERROR(icn->dev, "error enabling panel (%d)\n", ret);
> > +}
> > +
> > +static int chipone_attach(struct drm_bridge *bridge)
> > +{
> > +	struct chipone *icn = bridge_to_chipone(bridge);
> > +	struct drm_device *drm = bridge->dev;
> > +	int ret;
> > +
> > +	icn->connector.polled = DRM_CONNECTOR_POLL_HPD;
> > +	ret = drm_connector_init(drm, &icn->connector,
> > +				 &chipone_connector_funcs,
> > +				 DRM_MODE_CONNECTOR_Unknown);
> 
> It should be a DRM_MODE_CONNECTOR_DPI connector.

I'd go one step further, the driver should use drm_panel_bridge_add()
and not create a connector itself. It should also support the case where
the RGB output is connected to a bridge instead of a panel.

> > +	if (ret) {
> > +		DRM_ERROR("Failed to initialize connector\n");
> > +		return ret;
> > +	}
> > +
> > +	drm_connector_helper_add(&icn->connector,
> > +				 &chipone_connector_helper_funcs);
> > +	drm_connector_attach_encoder(&icn->connector, bridge->encoder);
> > +	drm_panel_attach(icn->panel, &icn->connector);
> > +	icn->connector.funcs->reset(&icn->connector);
> > +	drm_fb_helper_add_one_connector(drm->fb_helper, &icn->connector);
> 
> What do you need that for?
> 
> > +	drm_connector_register(&icn->connector);
> > +
> > +	return 0;
> > +}
> > +
> > +static void chipone_detach(struct drm_bridge *bridge)
> > +{
> > +	struct chipone *icn = bridge_to_chipone(bridge);
> > +	struct drm_device *drm = bridge->dev;
> > +
> > +	drm_connector_unregister(&icn->connector);
> > +	drm_fb_helper_remove_one_connector(drm->fb_helper, &icn->connector);
> > +	drm_panel_detach(icn->panel);
> > +	icn->panel = NULL;
> > +	drm_connector_put(&icn->connector);
> > +}
> > +
> > +static const struct drm_bridge_funcs chipone_bridge_funcs = {
> > +	.disable = chipone_disable,
> > +	.post_disable = chipone_post_disable,
> > +	.enable = chipone_enable,
> > +	.pre_enable = chipone_pre_enable,
> > +	.attach = chipone_attach,
> > +	.detach = chipone_detach,
> > +};
> > +
> > +static int chipone_probe(struct mipi_dsi_device *dsi)
> > +{
> > +	struct device *dev = &dsi->dev;
> > +	struct chipone *icn;
> > +	int ret;
> > +
> > +	icn = devm_kzalloc(dev, sizeof(struct chipone), GFP_KERNEL);
> > +	if (!icn)
> > +		return -ENOMEM;
> > +
> > +	mipi_dsi_set_drvdata(dsi, icn);
> > +
> > +	icn->dev = dev;
> > +	dsi->lanes = 4;
> > +	dsi->format = MIPI_DSI_FMT_RGB888;
> > +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> > +
> > +	icn->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> > +	if (IS_ERR(icn->reset)) {
> > +		DRM_DEV_ERROR(dev, "no reset GPIO pin provided\n");
> > +		return PTR_ERR(icn->reset);
> > +	}
> > +
> > +	ret = drm_of_find_panel_or_bridge(icn->dev->of_node, 1, 0,
> > +					  &icn->panel, NULL);
> > +	if (ret && ret != -EPROBE_DEFER) {
> > +		DRM_DEV_ERROR(dev, "failed to find panel (ret = %d)\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	icn->bridge.funcs = &chipone_bridge_funcs;
> > +	icn->bridge.of_node = dev->of_node;
> > +
> > +	drm_bridge_add(&icn->bridge);
> > +
> > +	ret = mipi_dsi_attach(dsi);
> > +	if (ret < 0) {
> > +		drm_bridge_remove(&icn->bridge);
> > +		DRM_DEV_ERROR(dev, "failed to attach dsi (ret = %d)\n", ret);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int chipone_remove(struct mipi_dsi_device *dsi)
> > +{
> > +	struct chipone *icn = mipi_dsi_get_drvdata(dsi);
> > +
> > +	mipi_dsi_detach(dsi);
> > +	drm_bridge_remove(&icn->bridge);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id chipone_of_match[] = {
> > +	{ .compatible = "bananapi,icn6211" },
> 
> This should have your generic compatible listed

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/6] drm/bridge: Add Chipone ICN6211 MIPI-DSI/RGB convertor bridge

[Jagan Teki]

On Fri, Mar 15, 2019 at 7:03 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Fri, Mar 15, 2019 at 06:38:24PM +0530, Jagan Teki wrote:
> > ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
> > It has a flexible configuration of MIPI DSI signal input
> > and produce RGB565, RGB666, RGB888 output format.
> >
> > Add bridge driver for it.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >  MAINTAINERS                              |   6 +
> >  drivers/gpu/drm/bridge/Kconfig           |  10 +
> >  drivers/gpu/drm/bridge/Makefile          |   1 +
> >  drivers/gpu/drm/bridge/chipone-icn6211.c | 275 +++++++++++++++++++++++
> >  4 files changed, 292 insertions(+)
> >  create mode 100644 drivers/gpu/drm/bridge/chipone-icn6211.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 4de80222cffb..e529addd30f5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4897,6 +4897,12 @@ T:     git git://anongit.freedesktop.org/drm/drm-misc
> >  S:   Maintained
> >  F:   drivers/gpu/drm/bochs/
> >
> > +DRM DRIVER FOR CHIPONE ICN6211 MIPI-DSI to RGB CONVERTOR BRIDGE
> > +M:   Jagan Teki <jagan@amarulasolutions.com>
> > +S:   Maintained
> > +F:   drivers/gpu/drm/bridge/chipone-icn6211.c
> > +F:   Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> > +
> >  DRM DRIVER FOR FARADAY TVE200 TV ENCODER
> >  M:   Linus Walleij <linus.walleij@linaro.org>
> >  T:   git git://anongit.freedesktop.org/drm/drm-misc
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index 8840f396a7b6..cd314572e4ed 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -36,6 +36,16 @@ config DRM_CDNS_DSI
> >         Support Cadence DPI to DSI bridge. This is an internal
> >         bridge and is meant to be directly embedded in a SoC.
> >
> > +config DRM_CHIPONE_ICN6211
> > +     tristate "Chipone ICN6211 MIPI-DSI/RGB converter bridge"
> > +     depends on DRM && DRM_PANEL
> > +     depends on OF
> > +     select DRM_MIPI_DSI
> > +     help
> > +       ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
> > +       It has a flexible configuration of MIPI DSI signal input
> > +       and produce RGB565, RGB666, RGB888 output format.
> > +
> >  config DRM_DUMB_VGA_DAC
> >       tristate "Dumb VGA DAC Bridge support"
> >       depends on OF
> > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > index 4934fcf5a6f8..541fdccad10b 100644
> > --- a/drivers/gpu/drm/bridge/Makefile
> > +++ b/drivers/gpu/drm/bridge/Makefile
> > @@ -1,6 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
> >  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
> > +obj-$(CONFIG_DRM_CHIPONE_ICN6211) += chipone-icn6211.o
> >  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> >  obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
> >  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
> > diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
> > new file mode 100644
> > index 000000000000..cd2f3505f845
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> > @@ -0,0 +1,275 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2018 Amarula Solutions
> > + * Author: Jagan Teki <jagan@amarulasolutions.com>
> > + */
> > +
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_fb_helper.h>
> > +#include <drm/drm_of.h>
> > +#include <drm/drm_panel.h>
> > +#include <drm/drm_print.h>
> > +#include <drm/drm_probe_helper.h>
> > +#include <drm/drm_mipi_dsi.h>
> > +
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/module.h>
> > +#include <linux/of_graph.h>
> > +
> > +#define ICN6211_INIT_CMD_LEN         2
> > +
> > +struct chipone {
> > +     struct device *dev;
> > +     struct drm_bridge bridge;
> > +     struct drm_connector connector;
> > +     struct drm_panel *panel;
> > +
> > +     struct gpio_desc *reset;
> > +};
> > +
> > +static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
> > +{
> > +     return container_of(bridge, struct chipone, bridge);
> > +}
> > +
> > +static inline
> > +struct chipone *connector_to_chipone(struct drm_connector *connector)
> > +{
> > +     return container_of(connector, struct chipone, connector);
> > +}
> > +
> > +struct icn6211_init_cmd {
> > +     u8 data[ICN6211_INIT_CMD_LEN];
> > +};
> > +
> > +static const struct icn6211_init_cmd icn6211_init_cmds[] = {
> > +     { .data = { 0x7A, 0xC1 } },
> > +     { .data = { 0x20, 0x20 } },
> > +     { .data = { 0x21, 0xE0 } },
> > +     { .data = { 0x22, 0x13 } },
> > +     { .data = { 0x23, 0x28 } },
> > +     { .data = { 0x24, 0x30 } },
> > +     { .data = { 0x25, 0x28 } },
> > +     { .data = { 0x26, 0x00 } },
> > +     { .data = { 0x27, 0x0D } },
> > +     { .data = { 0x28, 0x03 } },
> > +     { .data = { 0x29, 0x1D } },
> > +     { .data = { 0x34, 0x80 } },
> > +     { .data = { 0x36, 0x28 } },
> > +     { .data = { 0xB5, 0xA0 } },
> > +     { .data = { 0x5C, 0xFF } },
> > +     { .data = { 0x2A, 0x01 } },
> > +     { .data = { 0x56, 0x92 } },
> > +     { .data = { 0x6B, 0x71 } },
> > +     { .data = { 0x69, 0x2B } },
> > +     { .data = { 0x10, 0x40 } },
> > +     { .data = { 0x11, 0x98 } },
> > +     { .data = { 0xB6, 0x20 } },
> > +     { .data = { 0x51, 0x20 } },
> > +     { .data = { 0x09, 0x10 } },
> > +};
>
> What are those commands supposed to be? Some of them at least look
> pretty standard (and have proper functions):
> MIPI_DCS_EXIT_INVERT_MODE, _SET_DISPLAY_ON, _SET_TEAR_OFF, etc.

As I said in previous Allwinner MIPI-DSI fixes and new code support
series. For this bridge we don't have programming guide or any
datasheet. The initialization sequence is taken from BSP panel driver
and analyzed based on other references mentioned here [1]. Some of the
command sequence value are based on the timings other based on
standard MIPI commands as you said. Do you need to add comments to
describe the same or some other changes?

>
> > +static int chipone_get_modes(struct drm_connector *connector)
> > +{
> > +     struct chipone *icn = connector_to_chipone(connector);
> > +
> > +     return drm_panel_get_modes(icn->panel);
> > +}
> > +
> > +static const
> > +struct drm_connector_helper_funcs chipone_connector_helper_funcs = {
> > +     .get_modes = chipone_get_modes,
> > +};
> > +
> > +static const struct drm_connector_funcs chipone_connector_funcs = {
> > +     .fill_modes = drm_helper_probe_single_connector_modes,
> > +     .destroy = drm_connector_cleanup,
> > +     .reset = drm_atomic_helper_connector_reset,
> > +     .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > +     .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > +};
> > +
> > +static void chipone_disable(struct drm_bridge *bridge)
> > +{
> > +     struct chipone *icn = bridge_to_chipone(bridge);
> > +     int ret = drm_panel_disable(bridge_to_chipone(bridge)->panel);
> > +
> > +     if (ret < 0)
> > +             DRM_DEV_ERROR(icn->dev, "error disabling panel (%d)\n", ret);
> > +}
> > +
> > +static void chipone_post_disable(struct drm_bridge *bridge)
> > +{
> > +     struct chipone *icn = bridge_to_chipone(bridge);
> > +     int ret;
> > +
> > +     ret = drm_panel_unprepare(icn->panel);
> > +     if (ret < 0)
> > +             DRM_DEV_ERROR(icn->dev, "error unpreparing panel (%d)\n", ret);
> > +
> > +     msleep(50);
> > +
> > +     gpiod_set_value(icn->reset, 0);
> > +}
> > +
> > +static void chipone_pre_enable(struct drm_bridge *bridge)
> > +{
> > +     struct chipone *icn = bridge_to_chipone(bridge);
> > +     struct mipi_dsi_device *dsi = to_mipi_dsi_device(icn->dev);
> > +     unsigned int i;
> > +     int ret;
> > +
> > +     gpiod_set_value(icn->reset, 0);
> > +     msleep(20);
> > +
> > +     gpiod_set_value(icn->reset, 1);
> > +     msleep(50);
> > +
> > +     for (i = 0; i < ARRAY_SIZE(icn6211_init_cmds); i++) {
> > +             const struct icn6211_init_cmd *cmd = &icn6211_init_cmds[i];
> > +
> > +             ret = mipi_dsi_generic_write(dsi, cmd->data,
> > +                                          ICN6211_INIT_CMD_LEN);
> > +             if (ret < 0) {
> > +                     DRM_DEV_ERROR(icn->dev,
> > +                                   "failed to write cmd %d: %d\n", i, ret);
> > +                     return;
> > +             }
> > +     }
> > +
> > +     ret = drm_panel_prepare(icn->panel);
> > +     if (ret < 0)
> > +             DRM_DEV_ERROR(icn->dev, "error preparing panel (%d)\n", ret);
> > +}
> > +
> > +static void chipone_enable(struct drm_bridge *bridge)
> > +{
> > +     struct chipone *icn = bridge_to_chipone(bridge);
> > +     int ret = drm_panel_enable(icn->panel);
> > +
> > +     if (ret < 0)
> > +             DRM_DEV_ERROR(icn->dev, "error enabling panel (%d)\n", ret);
> > +}
> > +
> > +static int chipone_attach(struct drm_bridge *bridge)
> > +{
> > +     struct chipone *icn = bridge_to_chipone(bridge);
> > +     struct drm_device *drm = bridge->dev;
> > +     int ret;
> > +
> > +     icn->connector.polled = DRM_CONNECTOR_POLL_HPD;
> > +     ret = drm_connector_init(drm, &icn->connector,
> > +                              &chipone_connector_funcs,
> > +                              DRM_MODE_CONNECTOR_Unknown);
>
> It should be a DRM_MODE_CONNECTOR_DPI connector.

Since the output connector is RGB, I have used 'Unknown' since we
follow similar connector type in sun4i_rgb.c So these RGB comes under
parallel display interface we can have to use this DPI connector is
it?

>
> > +     if (ret) {
> > +             DRM_ERROR("Failed to initialize connector\n");
> > +             return ret;
> > +     }
> > +
> > +     drm_connector_helper_add(&icn->connector,
> > +                              &chipone_connector_helper_funcs);
> > +     drm_connector_attach_encoder(&icn->connector, bridge->encoder);
> > +     drm_panel_attach(icn->panel, &icn->connector);
> > +     icn->connector.funcs->reset(&icn->connector);
> > +     drm_fb_helper_add_one_connector(drm->fb_helper, &icn->connector);
>
> What do you need that for?

I took the reference code, sorry it is accidentally present will
wind-up in next version.

>
> > +     drm_connector_register(&icn->connector);
> > +
> > +     return 0;
> > +}
> > +
> > +static void chipone_detach(struct drm_bridge *bridge)
> > +{
> > +     struct chipone *icn = bridge_to_chipone(bridge);
> > +     struct drm_device *drm = bridge->dev;
> > +
> > +     drm_connector_unregister(&icn->connector);
> > +     drm_fb_helper_remove_one_connector(drm->fb_helper, &icn->connector);
> > +     drm_panel_detach(icn->panel);
> > +     icn->panel = NULL;
> > +     drm_connector_put(&icn->connector);
> > +}
> > +
> > +static const struct drm_bridge_funcs chipone_bridge_funcs = {
> > +     .disable = chipone_disable,
> > +     .post_disable = chipone_post_disable,
> > +     .enable = chipone_enable,
> > +     .pre_enable = chipone_pre_enable,
> > +     .attach = chipone_attach,
> > +     .detach = chipone_detach,
> > +};
> > +
> > +static int chipone_probe(struct mipi_dsi_device *dsi)
> > +{
> > +     struct device *dev = &dsi->dev;
> > +     struct chipone *icn;
> > +     int ret;
> > +
> > +     icn = devm_kzalloc(dev, sizeof(struct chipone), GFP_KERNEL);
> > +     if (!icn)
> > +             return -ENOMEM;
> > +
> > +     mipi_dsi_set_drvdata(dsi, icn);
> > +
> > +     icn->dev = dev;
> > +     dsi->lanes = 4;
> > +     dsi->format = MIPI_DSI_FMT_RGB888;
> > +     dsi->mode_flags = MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> > +
> > +     icn->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> > +     if (IS_ERR(icn->reset)) {
> > +             DRM_DEV_ERROR(dev, "no reset GPIO pin provided\n");
> > +             return PTR_ERR(icn->reset);
> > +     }
> > +
> > +     ret = drm_of_find_panel_or_bridge(icn->dev->of_node, 1, 0,
> > +                                       &icn->panel, NULL);
> > +     if (ret && ret != -EPROBE_DEFER) {
> > +             DRM_DEV_ERROR(dev, "failed to find panel (ret = %d)\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     icn->bridge.funcs = &chipone_bridge_funcs;
> > +     icn->bridge.of_node = dev->of_node;
> > +
> > +     drm_bridge_add(&icn->bridge);
> > +
> > +     ret = mipi_dsi_attach(dsi);
> > +     if (ret < 0) {
> > +             drm_bridge_remove(&icn->bridge);
> > +             DRM_DEV_ERROR(dev, "failed to attach dsi (ret = %d)\n", ret);
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static int chipone_remove(struct mipi_dsi_device *dsi)
> > +{
> > +     struct chipone *icn = mipi_dsi_get_drvdata(dsi);
> > +
> > +     mipi_dsi_detach(dsi);
> > +     drm_bridge_remove(&icn->bridge);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id chipone_of_match[] = {
> > +     { .compatible = "bananapi,icn6211" },
>
> This should have your generic compatible listed

Any idea why? usually it can be know attached device associated IC
chip isn't it? I can see this similar approach in some panel driver
which I commented in 4/5
https://patchwork.freedesktop.org/patch/292416/

[1] https://patchwork.kernel.org/patch/10617921/

Re: Re: [PATCH 5/6] drm/bridge: Add Chipone ICN6211 MIPI-DSI/RGB convertor bridge

[Chen-Yu Tsai]

On Tue, Mar 19, 2019 at 1:59 AM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
>
> On Fri, Mar 15, 2019 at 7:03 PM Maxime Ripard <maxime.ripard-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org> wrote:
> >
> > On Fri, Mar 15, 2019 at 06:38:24PM +0530, Jagan Teki wrote:
> > > ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
> > > It has a flexible configuration of MIPI DSI signal input
> > > and produce RGB565, RGB666, RGB888 output format.
> > >
> > > Add bridge driver for it.
> > >
> > > Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> > > ---
> > >  MAINTAINERS                              |   6 +
> > >  drivers/gpu/drm/bridge/Kconfig           |  10 +
> > >  drivers/gpu/drm/bridge/Makefile          |   1 +
> > >  drivers/gpu/drm/bridge/chipone-icn6211.c | 275 +++++++++++++++++++++++
> > >  4 files changed, 292 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/bridge/chipone-icn6211.c
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 4de80222cffb..e529addd30f5 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -4897,6 +4897,12 @@ T:     git git://anongit.freedesktop.org/drm/drm-misc
> > >  S:   Maintained
> > >  F:   drivers/gpu/drm/bochs/
> > >
> > > +DRM DRIVER FOR CHIPONE ICN6211 MIPI-DSI to RGB CONVERTOR BRIDGE
> > > +M:   Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> > > +S:   Maintained
> > > +F:   drivers/gpu/drm/bridge/chipone-icn6211.c
> > > +F:   Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt
> > > +
> > >  DRM DRIVER FOR FARADAY TVE200 TV ENCODER
> > >  M:   Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > >  T:   git git://anongit.freedesktop.org/drm/drm-misc
> > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > > index 8840f396a7b6..cd314572e4ed 100644
> > > --- a/drivers/gpu/drm/bridge/Kconfig
> > > +++ b/drivers/gpu/drm/bridge/Kconfig
> > > @@ -36,6 +36,16 @@ config DRM_CDNS_DSI
> > >         Support Cadence DPI to DSI bridge. This is an internal
> > >         bridge and is meant to be directly embedded in a SoC.
> > >
> > > +config DRM_CHIPONE_ICN6211
> > > +     tristate "Chipone ICN6211 MIPI-DSI/RGB converter bridge"
> > > +     depends on DRM && DRM_PANEL
> > > +     depends on OF
> > > +     select DRM_MIPI_DSI
> > > +     help
> > > +       ICN6211 is MIPI-DSI/RGB converter bridge from chipone.
> > > +       It has a flexible configuration of MIPI DSI signal input
> > > +       and produce RGB565, RGB666, RGB888 output format.
> > > +
> > >  config DRM_DUMB_VGA_DAC
> > >       tristate "Dumb VGA DAC Bridge support"
> > >       depends on OF
> > > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > > index 4934fcf5a6f8..541fdccad10b 100644
> > > --- a/drivers/gpu/drm/bridge/Makefile
> > > +++ b/drivers/gpu/drm/bridge/Makefile
> > > @@ -1,6 +1,7 @@
> > >  # SPDX-License-Identifier: GPL-2.0
> > >  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
> > >  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
> > > +obj-$(CONFIG_DRM_CHIPONE_ICN6211) += chipone-icn6211.o
> > >  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> > >  obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
> > >  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
> > > diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
> > > new file mode 100644
> > > index 000000000000..cd2f3505f845
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> > > @@ -0,0 +1,275 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright (C) 2018 Amarula Solutions
> > > + * Author: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> > > + */
> > > +
> > > +#include <drm/drm_atomic_helper.h>
> > > +#include <drm/drm_fb_helper.h>
> > > +#include <drm/drm_of.h>
> > > +#include <drm/drm_panel.h>
> > > +#include <drm/drm_print.h>
> > > +#include <drm/drm_probe_helper.h>
> > > +#include <drm/drm_mipi_dsi.h>
> > > +
> > > +#include <linux/delay.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_graph.h>
> > > +
> > > +#define ICN6211_INIT_CMD_LEN         2
> > > +
> > > +struct chipone {
> > > +     struct device *dev;
> > > +     struct drm_bridge bridge;
> > > +     struct drm_connector connector;
> > > +     struct drm_panel *panel;
> > > +
> > > +     struct gpio_desc *reset;
> > > +};
> > > +
> > > +static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
> > > +{
> > > +     return container_of(bridge, struct chipone, bridge);
> > > +}
> > > +
> > > +static inline
> > > +struct chipone *connector_to_chipone(struct drm_connector *connector)
> > > +{
> > > +     return container_of(connector, struct chipone, connector);
> > > +}
> > > +
> > > +struct icn6211_init_cmd {
> > > +     u8 data[ICN6211_INIT_CMD_LEN];
> > > +};
> > > +
> > > +static const struct icn6211_init_cmd icn6211_init_cmds[] = {
> > > +     { .data = { 0x7A, 0xC1 } },
> > > +     { .data = { 0x20, 0x20 } },
> > > +     { .data = { 0x21, 0xE0 } },
> > > +     { .data = { 0x22, 0x13 } },
> > > +     { .data = { 0x23, 0x28 } },
> > > +     { .data = { 0x24, 0x30 } },
> > > +     { .data = { 0x25, 0x28 } },
> > > +     { .data = { 0x26, 0x00 } },
> > > +     { .data = { 0x27, 0x0D } },
> > > +     { .data = { 0x28, 0x03 } },
> > > +     { .data = { 0x29, 0x1D } },
> > > +     { .data = { 0x34, 0x80 } },
> > > +     { .data = { 0x36, 0x28 } },
> > > +     { .data = { 0xB5, 0xA0 } },
> > > +     { .data = { 0x5C, 0xFF } },
> > > +     { .data = { 0x2A, 0x01 } },
> > > +     { .data = { 0x56, 0x92 } },
> > > +     { .data = { 0x6B, 0x71 } },
> > > +     { .data = { 0x69, 0x2B } },
> > > +     { .data = { 0x10, 0x40 } },
> > > +     { .data = { 0x11, 0x98 } },
> > > +     { .data = { 0xB6, 0x20 } },
> > > +     { .data = { 0x51, 0x20 } },
> > > +     { .data = { 0x09, 0x10 } },
> > > +};
> >
> > What are those commands supposed to be? Some of them at least look
> > pretty standard (and have proper functions):
> > MIPI_DCS_EXIT_INVERT_MODE, _SET_DISPLAY_ON, _SET_TEAR_OFF, etc.
>
> As I said in previous Allwinner MIPI-DSI fixes and new code support
> series. For this bridge we don't have programming guide or any
> datasheet. The initialization sequence is taken from BSP panel driver
> and analyzed based on other references mentioned here [1]. Some of the
> command sequence value are based on the timings other based on
> standard MIPI commands as you said. Do you need to add comments to
> describe the same or some other changes?

Please at least put in proper macros or even calculated values for
the stuff that we do know, such as the standard commands and basic
timings that I already worked out for you previously.

For a good reason why, see the slightly cleaned up but still awful
table of init values in the ov5640 camera sensor driver.

> >
> > > +static int chipone_get_modes(struct drm_connector *connector)
> > > +{
> > > +     struct chipone *icn = connector_to_chipone(connector);
> > > +
> > > +     return drm_panel_get_modes(icn->panel);
> > > +}
> > > +
> > > +static const
> > > +struct drm_connector_helper_funcs chipone_connector_helper_funcs = {
> > > +     .get_modes = chipone_get_modes,
> > > +};
> > > +
> > > +static const struct drm_connector_funcs chipone_connector_funcs = {
> > > +     .fill_modes = drm_helper_probe_single_connector_modes,
> > > +     .destroy = drm_connector_cleanup,
> > > +     .reset = drm_atomic_helper_connector_reset,
> > > +     .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > > +     .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > > +};
> > > +
> > > +static void chipone_disable(struct drm_bridge *bridge)
> > > +{
> > > +     struct chipone *icn = bridge_to_chipone(bridge);
> > > +     int ret = drm_panel_disable(bridge_to_chipone(bridge)->panel);
> > > +
> > > +     if (ret < 0)
> > > +             DRM_DEV_ERROR(icn->dev, "error disabling panel (%d)\n", ret);
> > > +}
> > > +
> > > +static void chipone_post_disable(struct drm_bridge *bridge)
> > > +{
> > > +     struct chipone *icn = bridge_to_chipone(bridge);
> > > +     int ret;
> > > +
> > > +     ret = drm_panel_unprepare(icn->panel);
> > > +     if (ret < 0)
> > > +             DRM_DEV_ERROR(icn->dev, "error unpreparing panel (%d)\n", ret);
> > > +
> > > +     msleep(50);
> > > +
> > > +     gpiod_set_value(icn->reset, 0);
> > > +}
> > > +
> > > +static void chipone_pre_enable(struct drm_bridge *bridge)
> > > +{
> > > +     struct chipone *icn = bridge_to_chipone(bridge);
> > > +     struct mipi_dsi_device *dsi = to_mipi_dsi_device(icn->dev);
> > > +     unsigned int i;
> > > +     int ret;
> > > +
> > > +     gpiod_set_value(icn->reset, 0);
> > > +     msleep(20);
> > > +
> > > +     gpiod_set_value(icn->reset, 1);
> > > +     msleep(50);
> > > +
> > > +     for (i = 0; i < ARRAY_SIZE(icn6211_init_cmds); i++) {
> > > +             const struct icn6211_init_cmd *cmd = &icn6211_init_cmds[i];
> > > +
> > > +             ret = mipi_dsi_generic_write(dsi, cmd->data,
> > > +                                          ICN6211_INIT_CMD_LEN);
> > > +             if (ret < 0) {
> > > +                     DRM_DEV_ERROR(icn->dev,
> > > +                                   "failed to write cmd %d: %d\n", i, ret);
> > > +                     return;
> > > +             }
> > > +     }
> > > +
> > > +     ret = drm_panel_prepare(icn->panel);
> > > +     if (ret < 0)
> > > +             DRM_DEV_ERROR(icn->dev, "error preparing panel (%d)\n", ret);
> > > +}
> > > +
> > > +static void chipone_enable(struct drm_bridge *bridge)
> > > +{
> > > +     struct chipone *icn = bridge_to_chipone(bridge);
> > > +     int ret = drm_panel_enable(icn->panel);
> > > +
> > > +     if (ret < 0)
> > > +             DRM_DEV_ERROR(icn->dev, "error enabling panel (%d)\n", ret);
> > > +}
> > > +
> > > +static int chipone_attach(struct drm_bridge *bridge)
> > > +{
> > > +     struct chipone *icn = bridge_to_chipone(bridge);
> > > +     struct drm_device *drm = bridge->dev;
> > > +     int ret;
> > > +
> > > +     icn->connector.polled = DRM_CONNECTOR_POLL_HPD;
> > > +     ret = drm_connector_init(drm, &icn->connector,
> > > +                              &chipone_connector_funcs,
> > > +                              DRM_MODE_CONNECTOR_Unknown);
> >
> > It should be a DRM_MODE_CONNECTOR_DPI connector.
>
> Since the output connector is RGB, I have used 'Unknown' since we
> follow similar connector type in sun4i_rgb.c So these RGB comes under
> parallel display interface we can have to use this DPI connector is
> it?

It should be the DPI connector. We used the wrong connector to begin
with and now we can't fix it, since the connector type is exported to
userspace.

> >
> > > +     if (ret) {
> > > +             DRM_ERROR("Failed to initialize connector\n");
> > > +             return ret;
> > > +     }
> > > +
> > > +     drm_connector_helper_add(&icn->connector,
> > > +                              &chipone_connector_helper_funcs);
> > > +     drm_connector_attach_encoder(&icn->connector, bridge->encoder);
> > > +     drm_panel_attach(icn->panel, &icn->connector);
> > > +     icn->connector.funcs->reset(&icn->connector);
> > > +     drm_fb_helper_add_one_connector(drm->fb_helper, &icn->connector);
> >
> > What do you need that for?
>
> I took the reference code, sorry it is accidentally present will
> wind-up in next version.
>
> >
> > > +     drm_connector_register(&icn->connector);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static void chipone_detach(struct drm_bridge *bridge)
> > > +{
> > > +     struct chipone *icn = bridge_to_chipone(bridge);
> > > +     struct drm_device *drm = bridge->dev;
> > > +
> > > +     drm_connector_unregister(&icn->connector);
> > > +     drm_fb_helper_remove_one_connector(drm->fb_helper, &icn->connector);
> > > +     drm_panel_detach(icn->panel);
> > > +     icn->panel = NULL;
> > > +     drm_connector_put(&icn->connector);
> > > +}
> > > +
> > > +static const struct drm_bridge_funcs chipone_bridge_funcs = {
> > > +     .disable = chipone_disable,
> > > +     .post_disable = chipone_post_disable,
> > > +     .enable = chipone_enable,
> > > +     .pre_enable = chipone_pre_enable,
> > > +     .attach = chipone_attach,
> > > +     .detach = chipone_detach,
> > > +};
> > > +
> > > +static int chipone_probe(struct mipi_dsi_device *dsi)
> > > +{
> > > +     struct device *dev = &dsi->dev;
> > > +     struct chipone *icn;
> > > +     int ret;
> > > +
> > > +     icn = devm_kzalloc(dev, sizeof(struct chipone), GFP_KERNEL);
> > > +     if (!icn)
> > > +             return -ENOMEM;
> > > +
> > > +     mipi_dsi_set_drvdata(dsi, icn);
> > > +
> > > +     icn->dev = dev;
> > > +     dsi->lanes = 4;
> > > +     dsi->format = MIPI_DSI_FMT_RGB888;
> > > +     dsi->mode_flags = MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> > > +
> > > +     icn->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> > > +     if (IS_ERR(icn->reset)) {
> > > +             DRM_DEV_ERROR(dev, "no reset GPIO pin provided\n");
> > > +             return PTR_ERR(icn->reset);
> > > +     }
> > > +
> > > +     ret = drm_of_find_panel_or_bridge(icn->dev->of_node, 1, 0,
> > > +                                       &icn->panel, NULL);
> > > +     if (ret && ret != -EPROBE_DEFER) {
> > > +             DRM_DEV_ERROR(dev, "failed to find panel (ret = %d)\n", ret);
> > > +             return ret;
> > > +     }
> > > +
> > > +     icn->bridge.funcs = &chipone_bridge_funcs;
> > > +     icn->bridge.of_node = dev->of_node;
> > > +
> > > +     drm_bridge_add(&icn->bridge);
> > > +
> > > +     ret = mipi_dsi_attach(dsi);
> > > +     if (ret < 0) {
> > > +             drm_bridge_remove(&icn->bridge);
> > > +             DRM_DEV_ERROR(dev, "failed to attach dsi (ret = %d)\n", ret);
> > > +     }
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static int chipone_remove(struct mipi_dsi_device *dsi)
> > > +{
> > > +     struct chipone *icn = mipi_dsi_get_drvdata(dsi);
> > > +
> > > +     mipi_dsi_detach(dsi);
> > > +     drm_bridge_remove(&icn->bridge);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static const struct of_device_id chipone_of_match[] = {
> > > +     { .compatible = "bananapi,icn6211" },
> >
> > This should have your generic compatible listed
>
> Any idea why? usually it can be know attached device associated IC
> chip isn't it? I can see this similar approach in some panel driver
> which I commented in 4/5
> https://patchwork.freedesktop.org/patch/292416/

A panel driver is tied to a specific set of timings, as required by
the actual LCD panel. A bridge on the other hand is generic. Whatever
timings needed are a property of the downstream end device. So you
really need to use the generic compatible string here.

You can add a big fat warning in this driver saying it currently only
supports one panel, and even peek at the downstream panel and bail out
if it's not the expected one. If other people encounter this chip, they
can come in and either add their own set of init commands, or even help
to generalize this driver.

ChenYu

> [1] https://patchwork.kernel.org/patch/10617921/
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
> For more options, visit https://groups.google.com/d/optout.

[PATCH 6/6] arm64: dts: allwinner: bananapi-m64: Enable S070WV20-CT16 DSI panel

[Jagan Teki]

This patch add support for Bananapi S070WV20-CT16 DSI panel to
BPI-M64 board.

Bananapi S070WV20-CT16 is a pure RGB output panel with ICN6211 DSI/RGB
convertor bridge, so enable bridge along with associated panel.

DSI panel connected via board DSI port with,
- DLDO1 as VCC-DSI supply
- PD6 gpio for reset pin
- PD5 gpio for backlight enable pin
- PD7 gpio for backlight vdd supply

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 .../dts/allwinner/sun50i-a64-bananapi-m64.dts | 63 +++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
index 7793ebb5d2b8..f31083aa4521 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
@@ -45,6 +45,7 @@
 #include "sun50i-a64.dtsi"
 
 #include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/pwm/pwm.h>
 
 / {
 	model = "BananaPi-M64";
@@ -56,6 +57,15 @@
 		serial1 = &uart1;
 	};
 
+	backlight: backlight {
+		compatible = "pwm-backlight";
+		pwms = <&r_pwm 0 50000 PWM_POLARITY_INVERTED>;
+		brightness-levels = <1 2 4 8 16 32 64 128 512>;
+		default-brightness-level = <2>;
+		enable-gpios = <&pio 3 5 GPIO_ACTIVE_HIGH>; /* LCD-BL-EN: PD5 */
+		power-supply = <&reg_vdd_backlight>;
+	};
+
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
@@ -91,6 +101,26 @@
 		};
 	};
 
+	panel-connector {
+		compatible = "bananapi,s070wv20-ct16", "simple-panel";
+
+		port {
+			backlight = <&backlight>;
+			panel_ep: endpoint {
+				remote-endpoint = <&bridge_out_ep>;
+			};
+		};
+	};
+
+	reg_vdd_backlight: vdd-backlight {
+		compatible = "regulator-fixed";
+		regulator-name = "vdd-backlight";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		gpio = <&pio 3 7 GPIO_ACTIVE_HIGH>; /* LCD-PWR-EN: PD7 */
+		enable-active-high;
+	};
+
 	wifi_pwrseq: wifi_pwrseq {
 		compatible = "mmc-pwrseq-simple";
 		reset-gpios = <&r_pio 0 2 GPIO_ACTIVE_LOW>; /* PL2 */
@@ -116,6 +146,33 @@
 	status = "okay";
 };
 
+&dphy {
+	status = "okay";
+};
+
+&dsi {
+	vcc-dsi-supply = <&reg_dldo1>;		/* VCC3V3-DSI */
+	status = "okay";
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	bridge@0 {
+		compatible = "bananapi,icn6211", "chipone, icn6211";
+		reg = <0>;
+		reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* LCD-RST: PD6 */
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@1 {
+			reg = <1>;
+
+			bridge_out_ep: endpoint {
+				remote-endpoint = <&panel_ep>;
+			};
+		};
+	};
+};
+
 &ehci0 {
 	status = "okay";
 };
@@ -208,6 +265,12 @@
 	status = "okay";
 };
 
+&r_pwm {
+	pinctrl-names = "default";
+	pinctrl-0 = <&r_pwm_pin>;
+	status = "okay";
+};
+
 &r_rsb {
 	status = "okay";
 
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH 6/6] arm64: dts: allwinner: bananapi-m64: Enable S070WV20-CT16 DSI panel

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3092 bytes --]

On Fri, Mar 15, 2019 at 06:38:25PM +0530, Jagan Teki wrote:
> This patch add support for Bananapi S070WV20-CT16 DSI panel to
> BPI-M64 board.
> 
> Bananapi S070WV20-CT16 is a pure RGB output panel with ICN6211 DSI/RGB
> convertor bridge, so enable bridge along with associated panel.
> 
> DSI panel connected via board DSI port with,
> - DLDO1 as VCC-DSI supply
> - PD6 gpio for reset pin
> - PD5 gpio for backlight enable pin
> - PD7 gpio for backlight vdd supply
> 
> Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> ---
>  .../dts/allwinner/sun50i-a64-bananapi-m64.dts | 63 +++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
> index 7793ebb5d2b8..f31083aa4521 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
> @@ -45,6 +45,7 @@
>  #include "sun50i-a64.dtsi"
>  
>  #include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/pwm/pwm.h>
>  
>  / {
>  	model = "BananaPi-M64";
> @@ -56,6 +57,15 @@
>  		serial1 = &uart1;
>  	};
>  
> +	backlight: backlight {
> +		compatible = "pwm-backlight";
> +		pwms = <&r_pwm 0 50000 PWM_POLARITY_INVERTED>;
> +		brightness-levels = <1 2 4 8 16 32 64 128 512>;
> +		default-brightness-level = <2>;
> +		enable-gpios = <&pio 3 5 GPIO_ACTIVE_HIGH>; /* LCD-BL-EN: PD5 */
> +		power-supply = <&reg_vdd_backlight>;
> +	};
> +
>  	chosen {
>  		stdout-path = "serial0:115200n8";
>  	};
> @@ -91,6 +101,26 @@
>  		};
>  	};
>  
> +	panel-connector {
> +		compatible = "bananapi,s070wv20-ct16", "simple-panel";
> +
> +		port {
> +			backlight = <&backlight>;
> +			panel_ep: endpoint {
> +				remote-endpoint = <&bridge_out_ep>;
> +			};
> +		};
> +	};
> +
> +	reg_vdd_backlight: vdd-backlight {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vdd-backlight";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		gpio = <&pio 3 7 GPIO_ACTIVE_HIGH>; /* LCD-PWR-EN: PD7 */
> +		enable-active-high;
> +	};
> +
>  	wifi_pwrseq: wifi_pwrseq {
>  		compatible = "mmc-pwrseq-simple";
>  		reset-gpios = <&r_pio 0 2 GPIO_ACTIVE_LOW>; /* PL2 */
> @@ -116,6 +146,33 @@
>  	status = "okay";
>  };
>  
> +&dphy {
> +	status = "okay";
> +};
> +
> +&dsi {
> +	vcc-dsi-supply = <&reg_dldo1>;		/* VCC3V3-DSI */
> +	status = "okay";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	bridge@0 {
> +		compatible = "bananapi,icn6211", "chipone, icn6211";

There's a typo in the compatible

> +		reg = <0>;
> +		reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* LCD-RST: PD6 */
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		port@1 {
> +			reg = <1>;
> +
> +			bridge_out_ep: endpoint {
> +				remote-endpoint = <&panel_ep>;
> +			};
> +		};

The port here should be under a node called ports, with two ports,
which is what you documented in your binding.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com