[PATCH v4 0/4] drm: sun4i: dsi: Convert drm bridge

[PATCH v4 0/4] drm: sun4i: dsi: Convert drm bridge

[Jagan Teki]

This series convert Allwinner DSI controller to full functional 
drm bridge driver for supporting slave panel, bridges.

Here, are the previous version changes[1].

Patch 1: use drm_of_find_panel_or_bridge API

Patch 2: Adding DRM Bridge support

Patch 3: Convert to bridge driver, that indeed drop
	 encoder API's and support bridge API's

Patch 4: Overlay patch for bridge enablement in BPI-M2M

Note: Only nit on this series is kms hotplug, added Samuel Holland
for reviews and comments as he is authorized the code before.

[1] https://lkml.org/lkml/2021/2/14/173

Any inputs on this would be appreciated!
Jagan.

Jagan Teki (4):
  drm: sun4i: dsi: Use drm_of_find_panel_or_bridge
  drm: sun4i: dsi: Add bridge support
  drm: sun4i: dsi: Convert to bridge driver
  [DO NOT MERGE] ARM: dts: sun8i: bananapi-m2m: Enable S070WV20-CT16 panel

 arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts |  85 ++++++++++++++++
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c       | 100 +++++++++++++------
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h       |   8 +-
 3 files changed, 160 insertions(+), 33 deletions(-)

-- 
2.25.1

[PATCH v4 1/4] drm: sun4i: dsi: Use drm_of_find_panel_or_bridge

[Jagan Teki]

Replace of_drm_find_panel with drm_of_find_panel_or_bridge
for finding panel, this indeed help to find the bridge if
bridge support added.

Added NULL in bridge argument, same will replace with bridge
parameter once bridge supported.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v4, v3:
- none

 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 4f5efcace68e..2e9e7b2d4145 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -21,6 +21,7 @@
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_mipi_dsi.h>
+#include <drm/drm_of.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
@@ -963,10 +964,14 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
 			    struct mipi_dsi_device *device)
 {
 	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
-	struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
+	struct drm_panel *panel;
+	int ret;
+
+	ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0,
+					  &panel, NULL);
+	if (ret)
+		return ret;
 
-	if (IS_ERR(panel))
-		return PTR_ERR(panel);
 	if (!dsi->drm || !dsi->drm->registered)
 		return -EPROBE_DEFER;
 
-- 
2.25.1

Re: [PATCH v4 1/4] drm: sun4i: dsi: Use drm_of_find_panel_or_bridge

[Laurent Pinchart]

Hi Jagan,

Thank you for the patch.

On Mon, Mar 22, 2021 at 07:31:49PM +0530, Jagan Teki wrote:
> Replace of_drm_find_panel with drm_of_find_panel_or_bridge
> for finding panel, this indeed help to find the bridge if
> bridge support added.
> 
> Added NULL in bridge argument, same will replace with bridge
> parameter once bridge supported.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>

Looks good, there should be no functional change.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> Changes for v4, v3:
> - none
> 
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 4f5efcace68e..2e9e7b2d4145 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -21,6 +21,7 @@
>  
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_of.h>
>  #include <drm/drm_panel.h>
>  #include <drm/drm_print.h>
>  #include <drm/drm_probe_helper.h>
> @@ -963,10 +964,14 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
>  			    struct mipi_dsi_device *device)
>  {
>  	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> -	struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
> +	struct drm_panel *panel;
> +	int ret;
> +
> +	ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0,
> +					  &panel, NULL);
> +	if (ret)
> +		return ret;
>  
> -	if (IS_ERR(panel))
> -		return PTR_ERR(panel);
>  	if (!dsi->drm || !dsi->drm->registered)
>  		return -EPROBE_DEFER;
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 1/4] drm: sun4i: dsi: Use drm_of_find_panel_or_bridge

[Samuel Holland]

On 3/23/21 5:53 PM, Laurent Pinchart wrote:
> Hi Jagan,
> 
> Thank you for the patch.
> 
> On Mon, Mar 22, 2021 at 07:31:49PM +0530, Jagan Teki wrote:
>> Replace of_drm_find_panel with drm_of_find_panel_or_bridge
>> for finding panel, this indeed help to find the bridge if
>> bridge support added.
>>
>> Added NULL in bridge argument, same will replace with bridge
>> parameter once bridge supported.
>>
>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> 
> Looks good, there should be no functional change.

Actually this breaks all existing users of this driver, see below.

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> ---
>> Changes for v4, v3:
>> - none
>>
>>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> index 4f5efcace68e..2e9e7b2d4145 100644
>> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> @@ -21,6 +21,7 @@
>>  
>>  #include <drm/drm_atomic_helper.h>
>>  #include <drm/drm_mipi_dsi.h>
>> +#include <drm/drm_of.h>
>>  #include <drm/drm_panel.h>
>>  #include <drm/drm_print.h>
>>  #include <drm/drm_probe_helper.h>
>> @@ -963,10 +964,14 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
>>  			    struct mipi_dsi_device *device)
>>  {
>>  	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
>> -	struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);

This is using the OF node of the DSI device, which is a direct child of
the DSI host's OF node. There is no OF graph involved.

>> +	struct drm_panel *panel;
>> +	int ret;
>> +
>> +	ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0,
>> +					  &panel, NULL);

However, this function expects to find the panel using OF graph. This
does not work with existing device trees (PinePhone, PineTab) which do
not use OF graph to connect the panel. And it cannot work, because the
DSI host's binding specifies a single port: the input port from the
display engine.

Regards,
Samuel

>> +	if (ret)
>> +		return ret;
>>  
>> -	if (IS_ERR(panel))
>> -		return PTR_ERR(panel);
>>  	if (!dsi->drm || !dsi->drm->registered)
>>  		return -EPROBE_DEFER;
>>  
> 

Re: [PATCH v4 1/4] drm: sun4i: dsi: Use drm_of_find_panel_or_bridge

[Jagan Teki]

On Wed, Mar 24, 2021 at 8:18 AM Samuel Holland <samuel@sholland.org> wrote:
>
> On 3/23/21 5:53 PM, Laurent Pinchart wrote:
> > Hi Jagan,
> >
> > Thank you for the patch.
> >
> > On Mon, Mar 22, 2021 at 07:31:49PM +0530, Jagan Teki wrote:
> >> Replace of_drm_find_panel with drm_of_find_panel_or_bridge
> >> for finding panel, this indeed help to find the bridge if
> >> bridge support added.
> >>
> >> Added NULL in bridge argument, same will replace with bridge
> >> parameter once bridge supported.
> >>
> >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >
> > Looks good, there should be no functional change.
>
> Actually this breaks all existing users of this driver, see below.
>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> >> ---
> >> Changes for v4, v3:
> >> - none
> >>
> >>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 11 ++++++++---
> >>  1 file changed, 8 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> >> index 4f5efcace68e..2e9e7b2d4145 100644
> >> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> >> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> >> @@ -21,6 +21,7 @@
> >>
> >>  #include <drm/drm_atomic_helper.h>
> >>  #include <drm/drm_mipi_dsi.h>
> >> +#include <drm/drm_of.h>
> >>  #include <drm/drm_panel.h>
> >>  #include <drm/drm_print.h>
> >>  #include <drm/drm_probe_helper.h>
> >> @@ -963,10 +964,14 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> >>                          struct mipi_dsi_device *device)
> >>  {
> >>      struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> >> -    struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
>
> This is using the OF node of the DSI device, which is a direct child of
> the DSI host's OF node. There is no OF graph involved.
>
> >> +    struct drm_panel *panel;
> >> +    int ret;
> >> +
> >> +    ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0,
> >> +                                      &panel, NULL);
>
> However, this function expects to find the panel using OF graph. This
> does not work with existing device trees (PinePhone, PineTab) which do
> not use OF graph to connect the panel. And it cannot work, because the
> DSI host's binding specifies a single port: the input port from the
> display engine.

Thanks for noticing this. I did understand your point and yes, I did
mention the updated pipeline in previous versions and forgot to add it
to this series.

Here is the updated pipeline to make it work:

https://patchwork.kernel.org/project/dri-devel/patch/20190524104252.20236-1-jagan@amarulasolutions.com/

Let me know your comments on this, so I will add a patch for the
above-affected DTS files.

Jagan.

Re: [PATCH v4 1/4] drm: sun4i: dsi: Use drm_of_find_panel_or_bridge

[Laurent Pinchart]

Hi Jagan,

On Wed, Mar 24, 2021 at 02:44:57PM +0530, Jagan Teki wrote:
> On Wed, Mar 24, 2021 at 8:18 AM Samuel Holland wrote:
> > On 3/23/21 5:53 PM, Laurent Pinchart wrote:
> > > On Mon, Mar 22, 2021 at 07:31:49PM +0530, Jagan Teki wrote:
> > >> Replace of_drm_find_panel with drm_of_find_panel_or_bridge
> > >> for finding panel, this indeed help to find the bridge if
> > >> bridge support added.
> > >>
> > >> Added NULL in bridge argument, same will replace with bridge
> > >> parameter once bridge supported.
> > >>
> > >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > >
> > > Looks good, there should be no functional change.
> >
> > Actually this breaks all existing users of this driver, see below.
> >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > >> ---
> > >> Changes for v4, v3:
> > >> - none
> > >>
> > >>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 11 ++++++++---
> > >>  1 file changed, 8 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > >> index 4f5efcace68e..2e9e7b2d4145 100644
> > >> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > >> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > >> @@ -21,6 +21,7 @@
> > >>
> > >>  #include <drm/drm_atomic_helper.h>
> > >>  #include <drm/drm_mipi_dsi.h>
> > >> +#include <drm/drm_of.h>
> > >>  #include <drm/drm_panel.h>
> > >>  #include <drm/drm_print.h>
> > >>  #include <drm/drm_probe_helper.h>
> > >> @@ -963,10 +964,14 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> > >>                          struct mipi_dsi_device *device)
> > >>  {
> > >>      struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > >> -    struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
> >
> > This is using the OF node of the DSI device, which is a direct child of
> > the DSI host's OF node. There is no OF graph involved.
> >
> > >> +    struct drm_panel *panel;
> > >> +    int ret;
> > >> +
> > >> +    ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0,
> > >> +                                      &panel, NULL);
> >
> > However, this function expects to find the panel using OF graph. This
> > does not work with existing device trees (PinePhone, PineTab) which do
> > not use OF graph to connect the panel. And it cannot work, because the
> > DSI host's binding specifies a single port: the input port from the
> > display engine.
> 
> Thanks for noticing this. I did understand your point and yes, I did
> mention the updated pipeline in previous versions and forgot to add it
> to this series.
> 
> Here is the updated pipeline to make it work:
> 
> https://patchwork.kernel.org/project/dri-devel/patch/20190524104252.20236-1-jagan@amarulasolutions.com/
> 
> Let me know your comments on this, so I will add a patch for the
> above-affected DTS files.

DT is an ABI, we need to ensure backward compatibility. Changes in
kernel drivers can't break devices that have an old DT.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 1/4] drm: sun4i: dsi: Use drm_of_find_panel_or_bridge

[Jagan Teki]

Hi Laurent,

On Wed, Mar 24, 2021 at 3:09 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Jagan,
>
> On Wed, Mar 24, 2021 at 02:44:57PM +0530, Jagan Teki wrote:
> > On Wed, Mar 24, 2021 at 8:18 AM Samuel Holland wrote:
> > > On 3/23/21 5:53 PM, Laurent Pinchart wrote:
> > > > On Mon, Mar 22, 2021 at 07:31:49PM +0530, Jagan Teki wrote:
> > > >> Replace of_drm_find_panel with drm_of_find_panel_or_bridge
> > > >> for finding panel, this indeed help to find the bridge if
> > > >> bridge support added.
> > > >>
> > > >> Added NULL in bridge argument, same will replace with bridge
> > > >> parameter once bridge supported.
> > > >>
> > > >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > >
> > > > Looks good, there should be no functional change.
> > >
> > > Actually this breaks all existing users of this driver, see below.
> > >
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >
> > > >> ---
> > > >> Changes for v4, v3:
> > > >> - none
> > > >>
> > > >>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 11 ++++++++---
> > > >>  1 file changed, 8 insertions(+), 3 deletions(-)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > >> index 4f5efcace68e..2e9e7b2d4145 100644
> > > >> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > >> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > >> @@ -21,6 +21,7 @@
> > > >>
> > > >>  #include <drm/drm_atomic_helper.h>
> > > >>  #include <drm/drm_mipi_dsi.h>
> > > >> +#include <drm/drm_of.h>
> > > >>  #include <drm/drm_panel.h>
> > > >>  #include <drm/drm_print.h>
> > > >>  #include <drm/drm_probe_helper.h>
> > > >> @@ -963,10 +964,14 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> > > >>                          struct mipi_dsi_device *device)
> > > >>  {
> > > >>      struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > > >> -    struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
> > >
> > > This is using the OF node of the DSI device, which is a direct child of
> > > the DSI host's OF node. There is no OF graph involved.
> > >
> > > >> +    struct drm_panel *panel;
> > > >> +    int ret;
> > > >> +
> > > >> +    ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0,
> > > >> +                                      &panel, NULL);
> > >
> > > However, this function expects to find the panel using OF graph. This
> > > does not work with existing device trees (PinePhone, PineTab) which do
> > > not use OF graph to connect the panel. And it cannot work, because the
> > > DSI host's binding specifies a single port: the input port from the
> > > display engine.
> >
> > Thanks for noticing this. I did understand your point and yes, I did
> > mention the updated pipeline in previous versions and forgot to add it
> > to this series.
> >
> > Here is the updated pipeline to make it work:
> >
> > https://patchwork.kernel.org/project/dri-devel/patch/20190524104252.20236-1-jagan@amarulasolutions.com/
> >
> > Let me know your comments on this, so I will add a patch for the
> > above-affected DTS files.
>
> DT is an ABI, we need to ensure backward compatibility. Changes in
> kernel drivers can't break devices that have an old DT.

Thanks for your point.

So, we need to choose APIs that would compatible with the old DT and
new DT changes. Am I correct?

Jagan.

Re: [PATCH v4 1/4] drm: sun4i: dsi: Use drm_of_find_panel_or_bridge

[Laurent Pinchart]

Hi Jagan,

On Wed, Mar 24, 2021 at 03:19:10PM +0530, Jagan Teki wrote:
> On Wed, Mar 24, 2021 at 3:09 PM Laurent Pinchart wrote:
> > On Wed, Mar 24, 2021 at 02:44:57PM +0530, Jagan Teki wrote:
> > > On Wed, Mar 24, 2021 at 8:18 AM Samuel Holland wrote:
> > > > On 3/23/21 5:53 PM, Laurent Pinchart wrote:
> > > > > On Mon, Mar 22, 2021 at 07:31:49PM +0530, Jagan Teki wrote:
> > > > >> Replace of_drm_find_panel with drm_of_find_panel_or_bridge
> > > > >> for finding panel, this indeed help to find the bridge if
> > > > >> bridge support added.
> > > > >>
> > > > >> Added NULL in bridge argument, same will replace with bridge
> > > > >> parameter once bridge supported.
> > > > >>
> > > > >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > >
> > > > > Looks good, there should be no functional change.
> > > >
> > > > Actually this breaks all existing users of this driver, see below.
> > > >
> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > >
> > > > >> ---
> > > > >> Changes for v4, v3:
> > > > >> - none
> > > > >>
> > > > >>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 11 ++++++++---
> > > > >>  1 file changed, 8 insertions(+), 3 deletions(-)
> > > > >>
> > > > >> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > >> index 4f5efcace68e..2e9e7b2d4145 100644
> > > > >> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > >> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > >> @@ -21,6 +21,7 @@
> > > > >>
> > > > >>  #include <drm/drm_atomic_helper.h>
> > > > >>  #include <drm/drm_mipi_dsi.h>
> > > > >> +#include <drm/drm_of.h>
> > > > >>  #include <drm/drm_panel.h>
> > > > >>  #include <drm/drm_print.h>
> > > > >>  #include <drm/drm_probe_helper.h>
> > > > >> @@ -963,10 +964,14 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> > > > >>                          struct mipi_dsi_device *device)
> > > > >>  {
> > > > >>      struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > > > >> -    struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
> > > >
> > > > This is using the OF node of the DSI device, which is a direct child of
> > > > the DSI host's OF node. There is no OF graph involved.
> > > >
> > > > >> +    struct drm_panel *panel;
> > > > >> +    int ret;
> > > > >> +
> > > > >> +    ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0,
> > > > >> +                                      &panel, NULL);
> > > >
> > > > However, this function expects to find the panel using OF graph. This
> > > > does not work with existing device trees (PinePhone, PineTab) which do
> > > > not use OF graph to connect the panel. And it cannot work, because the
> > > > DSI host's binding specifies a single port: the input port from the
> > > > display engine.
> > >
> > > Thanks for noticing this. I did understand your point and yes, I did
> > > mention the updated pipeline in previous versions and forgot to add it
> > > to this series.
> > >
> > > Here is the updated pipeline to make it work:
> > >
> > > https://patchwork.kernel.org/project/dri-devel/patch/20190524104252.20236-1-jagan@amarulasolutions.com/
> > >
> > > Let me know your comments on this, so I will add a patch for the
> > > above-affected DTS files.
> >
> > DT is an ABI, we need to ensure backward compatibility. Changes in
> > kernel drivers can't break devices that have an old DT.
> 
> Thanks for your point.
> 
> So, we need to choose APIs that would compatible with the old DT and
> new DT changes. Am I correct?

Yes, that's correct.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 1/4] drm: sun4i: dsi: Use drm_of_find_panel_or_bridge

[Maxime Ripard]

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

On Wed, Mar 24, 2021 at 11:55:35AM +0200, Laurent Pinchart wrote:
> Hi Jagan,
> 
> On Wed, Mar 24, 2021 at 03:19:10PM +0530, Jagan Teki wrote:
> > On Wed, Mar 24, 2021 at 3:09 PM Laurent Pinchart wrote:
> > > On Wed, Mar 24, 2021 at 02:44:57PM +0530, Jagan Teki wrote:
> > > > On Wed, Mar 24, 2021 at 8:18 AM Samuel Holland wrote:
> > > > > On 3/23/21 5:53 PM, Laurent Pinchart wrote:
> > > > > > On Mon, Mar 22, 2021 at 07:31:49PM +0530, Jagan Teki wrote:
> > > > > >> Replace of_drm_find_panel with drm_of_find_panel_or_bridge
> > > > > >> for finding panel, this indeed help to find the bridge if
> > > > > >> bridge support added.
> > > > > >>
> > > > > >> Added NULL in bridge argument, same will replace with bridge
> > > > > >> parameter once bridge supported.
> > > > > >>
> > > > > >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > >
> > > > > > Looks good, there should be no functional change.
> > > > >
> > > > > Actually this breaks all existing users of this driver, see below.
> > > > >
> > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > >
> > > > > >> ---
> > > > > >> Changes for v4, v3:
> > > > > >> - none
> > > > > >>
> > > > > >>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 11 ++++++++---
> > > > > >>  1 file changed, 8 insertions(+), 3 deletions(-)
> > > > > >>
> > > > > >> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > > >> index 4f5efcace68e..2e9e7b2d4145 100644
> > > > > >> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > > >> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > > >> @@ -21,6 +21,7 @@
> > > > > >>
> > > > > >>  #include <drm/drm_atomic_helper.h>
> > > > > >>  #include <drm/drm_mipi_dsi.h>
> > > > > >> +#include <drm/drm_of.h>
> > > > > >>  #include <drm/drm_panel.h>
> > > > > >>  #include <drm/drm_print.h>
> > > > > >>  #include <drm/drm_probe_helper.h>
> > > > > >> @@ -963,10 +964,14 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> > > > > >>                          struct mipi_dsi_device *device)
> > > > > >>  {
> > > > > >>      struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > > > > >> -    struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
> > > > >
> > > > > This is using the OF node of the DSI device, which is a direct child of
> > > > > the DSI host's OF node. There is no OF graph involved.
> > > > >
> > > > > >> +    struct drm_panel *panel;
> > > > > >> +    int ret;
> > > > > >> +
> > > > > >> +    ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0,
> > > > > >> +                                      &panel, NULL);
> > > > >
> > > > > However, this function expects to find the panel using OF graph. This
> > > > > does not work with existing device trees (PinePhone, PineTab) which do
> > > > > not use OF graph to connect the panel. And it cannot work, because the
> > > > > DSI host's binding specifies a single port: the input port from the
> > > > > display engine.
> > > >
> > > > Thanks for noticing this. I did understand your point and yes, I did
> > > > mention the updated pipeline in previous versions and forgot to add it
> > > > to this series.
> > > >
> > > > Here is the updated pipeline to make it work:
> > > >
> > > > https://patchwork.kernel.org/project/dri-devel/patch/20190524104252.20236-1-jagan@amarulasolutions.com/
> > > >
> > > > Let me know your comments on this, so I will add a patch for the
> > > > above-affected DTS files.
> > >
> > > DT is an ABI, we need to ensure backward compatibility. Changes in
> > > kernel drivers can't break devices that have an old DT.
> > 
> > Thanks for your point.
> > 
> > So, we need to choose APIs that would compatible with the old DT and
> > new DT changes. Am I correct?
> 
> Yes, that's correct.

However, I see no particular reason to change the DT binding in this
case. The DSI devices are supposed to be described through a subnode of
their DSI controller, that's the generic binding and except for very odd
devices (and a bridge like this one is certainly not one), I see no
reason to deviate from that.

Maxime

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

[PATCH v4 2/4] 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
converter.

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

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

Cc: Samuel Holland <samuel@sholland.org>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Note: 
Samuel Holland, The existing kms hotplug dropped in order to 
attach the bridge properly. 

However, I did try several ways to support hotplug with the 
bridge but it's resulting in a deadlock where bind never attach 
bridge until bridge pointer found and bridge pointer cannot 
found until bind finishes. Any inputs on this would be appreciated.

Changes for v4:
- none
Changes for v3:
- updated with new API's 

 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 34 +++++++++++++++++---------
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  2 +-
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 2e9e7b2d4145..39321299dc27 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -773,6 +773,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
 	if (dsi->panel)
 		drm_panel_prepare(dsi->panel);
 
+	if (dsi->panel_bridge)
+		dsi->panel_bridge->funcs->pre_enable(dsi->panel_bridge);
+
 	/*
 	 * FIXME: This should be moved after the switch to HS mode.
 	 *
@@ -788,6 +791,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
 	if (dsi->panel)
 		drm_panel_enable(dsi->panel);
 
+	if (dsi->panel_bridge)
+		dsi->panel_bridge->funcs->enable(dsi->panel_bridge);
+
 	sun6i_dsi_start(dsi, DSI_START_HSC);
 
 	udelay(1000);
@@ -804,6 +810,9 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
 	if (dsi->panel) {
 		drm_panel_disable(dsi->panel);
 		drm_panel_unprepare(dsi->panel);
+	} else if (dsi->panel_bridge) {
+		dsi->panel_bridge->funcs->disable(dsi->panel_bridge);
+		dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge);
 	}
 
 	phy_power_off(dsi->dphy);
@@ -964,23 +973,17 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
 			    struct mipi_dsi_device *device)
 {
 	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
-	struct drm_panel *panel;
 	int ret;
 
 	ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0,
-					  &panel, NULL);
+					  &dsi->panel, &dsi->panel_bridge);
 	if (ret)
 		return ret;
 
-	if (!dsi->drm || !dsi->drm->registered)
-		return -EPROBE_DEFER;
-
-	dsi->panel = panel;
 	dsi->device = device;
 
-	drm_kms_helper_hotplug_event(dsi->drm);
-
-	dev_info(host->dev, "Attached device %s\n", device->name);
+	dev_info(host->dev, "Attached %s %s\n",
+		 device->name, dsi->panel ? "panel" : "bridge");
 
 	return 0;
 }
@@ -991,9 +994,10 @@ static int sun6i_dsi_detach(struct mipi_dsi_host *host,
 	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
 
 	dsi->panel = NULL;
+	dsi->panel_bridge = NULL;
 	dsi->device = NULL;
 
-	drm_kms_helper_hotplug_event(dsi->drm);
+	drm_of_panel_bridge_remove(dsi->dev->of_node, 0, 0);
 
 	return 0;
 }
@@ -1082,7 +1086,13 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
 
 	drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
 
-	dsi->drm = drm;
+	if (dsi->panel_bridge) {
+		ret = drm_bridge_attach(&dsi->encoder, dsi->panel_bridge, NULL, 0);
+		if (ret) {
+			dev_err(dsi->dev, "Couldn't attach drm bridge\n");
+			goto err_cleanup_connector;
+		}
+	}
 
 	return 0;
 
@@ -1096,7 +1106,7 @@ static void sun6i_dsi_unbind(struct device *dev, struct device *master,
 {
 	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
 
-	dsi->drm = NULL;
+	drm_encoder_cleanup(&dsi->encoder);
 }
 
 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 c863900ae3b4..370ecb356a63 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
@@ -29,8 +29,8 @@ struct sun6i_dsi {
 
 	struct device		*dev;
 	struct mipi_dsi_device	*device;
-	struct drm_device	*drm;
 	struct drm_panel	*panel;
+	struct drm_bridge	*panel_bridge;
 };
 
 static inline struct sun6i_dsi *host_to_sun6i_dsi(struct mipi_dsi_host *host)
-- 
2.25.1

Re: [PATCH v4 2/4] drm: sun4i: dsi: Add bridge support

[Laurent Pinchart]

Hi Jagan,

Thank you for the patch.

On Mon, Mar 22, 2021 at 07:31:50PM +0530, Jagan Teki wrote:
> Some display panels would come up with a non-DSI output which

Did you mean input instead of output ?

> can have an option to connect DSI interface by means of bridge
> converter.
> 
> This DSI to non-DSI bridge converter would require a bridge
> driver that would communicate the DSI controller for bridge
> functionalities.
> 
> So, add support for bridge functionalities in Allwinner DSI
> controller.
> 
> Cc: Samuel Holland <samuel@sholland.org>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Note: 
> Samuel Holland, The existing kms hotplug dropped in order to 
> attach the bridge properly. 
> 
> However, I did try several ways to support hotplug with the 
> bridge but it's resulting in a deadlock where bind never attach 
> bridge until bridge pointer found and bridge pointer cannot 
> found until bind finishes. Any inputs on this would be appreciated.
> 
> Changes for v4:
> - none
> Changes for v3:
> - updated with new API's 
> 
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 34 +++++++++++++++++---------
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  2 +-
>  2 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 2e9e7b2d4145..39321299dc27 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -773,6 +773,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
>  	if (dsi->panel)
>  		drm_panel_prepare(dsi->panel);
>  
> +	if (dsi->panel_bridge)
> +		dsi->panel_bridge->funcs->pre_enable(dsi->panel_bridge);
> +
>  	/*
>  	 * FIXME: This should be moved after the switch to HS mode.
>  	 *
> @@ -788,6 +791,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
>  	if (dsi->panel)
>  		drm_panel_enable(dsi->panel);
>  
> +	if (dsi->panel_bridge)
> +		dsi->panel_bridge->funcs->enable(dsi->panel_bridge);
> +
>  	sun6i_dsi_start(dsi, DSI_START_HSC);
>  
>  	udelay(1000);
> @@ -804,6 +810,9 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
>  	if (dsi->panel) {
>  		drm_panel_disable(dsi->panel);
>  		drm_panel_unprepare(dsi->panel);
> +	} else if (dsi->panel_bridge) {
> +		dsi->panel_bridge->funcs->disable(dsi->panel_bridge);
> +		dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge);
>  	}

Instead of having code paths that depend on whether you have a panel or
a bridge, it would be better to wrap the panel a bridge (using
drivers/gpu/drm/bridge/panel.c). The dsi->panel_bridge pointer should be
renamed to next_bridge, and all the code (except in probe) can the use
next_bridge without caring if it's a direct connection to a panel or
another bridge.

Furthermore, the encoder should call bridge functions explicitly, this
should be handled by the DRM core.

>  
>  	phy_power_off(dsi->dphy);
> @@ -964,23 +973,17 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
>  			    struct mipi_dsi_device *device)
>  {
>  	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> -	struct drm_panel *panel;
>  	int ret;
>  
>  	ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0,
> -					  &panel, NULL);
> +					  &dsi->panel, &dsi->panel_bridge);
>  	if (ret)
>  		return ret;
>  
> -	if (!dsi->drm || !dsi->drm->registered)
> -		return -EPROBE_DEFER;
> -
> -	dsi->panel = panel;
>  	dsi->device = device;
>  
> -	drm_kms_helper_hotplug_event(dsi->drm);
> -
> -	dev_info(host->dev, "Attached device %s\n", device->name);
> +	dev_info(host->dev, "Attached %s %s\n",
> +		 device->name, dsi->panel ? "panel" : "bridge");
>  
>  	return 0;
>  }
> @@ -991,9 +994,10 @@ static int sun6i_dsi_detach(struct mipi_dsi_host *host,
>  	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
>  
>  	dsi->panel = NULL;
> +	dsi->panel_bridge = NULL;
>  	dsi->device = NULL;
>  
> -	drm_kms_helper_hotplug_event(dsi->drm);
> +	drm_of_panel_bridge_remove(dsi->dev->of_node, 0, 0);
>  
>  	return 0;
>  }
> @@ -1082,7 +1086,13 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
>  
>  	drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
>  
> -	dsi->drm = drm;
> +	if (dsi->panel_bridge) {
> +		ret = drm_bridge_attach(&dsi->encoder, dsi->panel_bridge, NULL, 0);
> +		if (ret) {
> +			dev_err(dsi->dev, "Couldn't attach drm bridge\n");
> +			goto err_cleanup_connector;
> +		}
> +	}
>  
>  	return 0;
>  
> @@ -1096,7 +1106,7 @@ static void sun6i_dsi_unbind(struct device *dev, struct device *master,
>  {
>  	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
>  
> -	dsi->drm = NULL;
> +	drm_encoder_cleanup(&dsi->encoder);
>  }
>  
>  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 c863900ae3b4..370ecb356a63 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> @@ -29,8 +29,8 @@ struct sun6i_dsi {
>  
>  	struct device		*dev;
>  	struct mipi_dsi_device	*device;
> -	struct drm_device	*drm;
>  	struct drm_panel	*panel;
> +	struct drm_bridge	*panel_bridge;
>  };
>  
>  static inline struct sun6i_dsi *host_to_sun6i_dsi(struct mipi_dsi_host *host)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/4] drm: sun4i: dsi: Add bridge support

[Samuel Holland]

On 3/22/21 9:01 AM, 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
> converter.
> 
> This DSI to non-DSI bridge converter would require a bridge
> driver that would communicate the DSI controller for bridge
> functionalities.
> 
> So, add support for bridge functionalities in Allwinner DSI
> controller.
> 
> Cc: Samuel Holland <samuel@sholland.org>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Note: 
> Samuel Holland, The existing kms hotplug dropped in order to 
> attach the bridge properly. 
> 
> However, I did try several ways to support hotplug with the 
> bridge but it's resulting in a deadlock where bind never attach 
> bridge until bridge pointer found and bridge pointer cannot 
> found until bind finishes. Any inputs on this would be appreciated.

The intended behavior is that sun6i_dsi_bind() is independent of any DSI
device. And sun6i_dsi_attach() must only be called after bind completes
and the DRM device is registered. This design allows the rest of the
display engine (such as the HDMI output) to work even if no panel is
listed in the device tree, or if a panel driver is missing.

> Changes for v4:
> - none
> Changes for v3:
> - updated with new API's 
> 
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 34 +++++++++++++++++---------
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  2 +-
>  2 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 2e9e7b2d4145..39321299dc27 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -773,6 +773,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
>  	if (dsi->panel)
>  		drm_panel_prepare(dsi->panel);
>  
> +	if (dsi->panel_bridge)
> +		dsi->panel_bridge->funcs->pre_enable(dsi->panel_bridge);
> +
>  	/*
>  	 * FIXME: This should be moved after the switch to HS mode.
>  	 *
> @@ -788,6 +791,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
>  	if (dsi->panel)
>  		drm_panel_enable(dsi->panel);
>  
> +	if (dsi->panel_bridge)
> +		dsi->panel_bridge->funcs->enable(dsi->panel_bridge);
> +
>  	sun6i_dsi_start(dsi, DSI_START_HSC);
>  
>  	udelay(1000);
> @@ -804,6 +810,9 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
>  	if (dsi->panel) {
>  		drm_panel_disable(dsi->panel);
>  		drm_panel_unprepare(dsi->panel);
> +	} else if (dsi->panel_bridge) {
> +		dsi->panel_bridge->funcs->disable(dsi->panel_bridge);
> +		dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge);
>  	}
>  
>  	phy_power_off(dsi->dphy);
> @@ -964,23 +973,17 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
>  			    struct mipi_dsi_device *device)
>  {
>  	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> -	struct drm_panel *panel;
>  	int ret;
>  
>  	ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0,
> -					  &panel, NULL);
> +					  &dsi->panel, &dsi->panel_bridge);
>  	if (ret)
>  		return ret;
>  
> -	if (!dsi->drm || !dsi->drm->registered)
> -		return -EPROBE_DEFER;
> -
> -	dsi->panel = panel;
>  	dsi->device = device;
>  
> -	drm_kms_helper_hotplug_event(dsi->drm);
> -
> -	dev_info(host->dev, "Attached device %s\n", device->name);
> +	dev_info(host->dev, "Attached %s %s\n",
> +		 device->name, dsi->panel ? "panel" : "bridge");
>  
>  	return 0;
>  }
> @@ -991,9 +994,10 @@ static int sun6i_dsi_detach(struct mipi_dsi_host *host,
>  	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
>  
>  	dsi->panel = NULL;
> +	dsi->panel_bridge = NULL;
>  	dsi->device = NULL;
>  
> -	drm_kms_helper_hotplug_event(dsi->drm);
> +	drm_of_panel_bridge_remove(dsi->dev->of_node, 0, 0);
>  
>  	return 0;
>  }
> @@ -1082,7 +1086,13 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
>  
>  	drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
>  
> -	dsi->drm = drm;
> +	if (dsi->panel_bridge) {
> +		ret = drm_bridge_attach(&dsi->encoder, dsi->panel_bridge, NULL, 0);
> +		if (ret) {
> +			dev_err(dsi->dev, "Couldn't attach drm bridge\n");
> +			goto err_cleanup_connector;
> +		}
> +	}

You initialize dsi->panel_bridge in sun6i_dsi_attach() above, but that
function may run long after sun6i_dsi_bind() has completed -- for
example if this driver is built in, but the panel/bridge driver is built
as a module. So you cannot reference dsi->panel_bridge from this function.

Regards,
Samuel

>  
>  	return 0;
>  
> @@ -1096,7 +1106,7 @@ static void sun6i_dsi_unbind(struct device *dev, struct device *master,
>  {
>  	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
>  
> -	dsi->drm = NULL;
> +	drm_encoder_cleanup(&dsi->encoder);
>  }
>  
>  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 c863900ae3b4..370ecb356a63 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> @@ -29,8 +29,8 @@ struct sun6i_dsi {
>  
>  	struct device		*dev;
>  	struct mipi_dsi_device	*device;
> -	struct drm_device	*drm;
>  	struct drm_panel	*panel;
> +	struct drm_bridge	*panel_bridge;
>  };
>  
>  static inline struct sun6i_dsi *host_to_sun6i_dsi(struct mipi_dsi_host *host)
> 

[PATCH v4 3/4] drm: sun4i: dsi: Convert to bridge driver

[Jagan Teki]

DRM bridge drivers have build-in handling of treating all display
pipeline components as bridges.

So, convert the existing to a drm bridge driver with a built-in
encoder support for compatibility with existing component drivers.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v4:
- none
Changes for v3:
- new patch

 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 75 ++++++++++++++++----------
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  6 +++
 2 files changed, 54 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 39321299dc27..6f3c5330a468 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -714,10 +714,10 @@ static int sun6i_dsi_start(struct sun6i_dsi *dsi,
 	return 0;
 }
 
-static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
+static void sun6i_dsi_bridge_enable(struct drm_bridge *bridge)
 {
-	struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
-	struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
+	struct drm_display_mode *mode = &bridge->encoder->crtc->state->adjusted_mode;
+	struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
 	struct mipi_dsi_device *device = dsi->device;
 	union phy_configure_opts opts = { };
 	struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
@@ -801,9 +801,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
 	sun6i_dsi_start(dsi, DSI_START_HSD);
 }
 
-static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
+static void sun6i_dsi_bridge_disable(struct drm_bridge *bridge)
 {
-	struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
+	struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
 
 	DRM_DEBUG_DRIVER("Disabling DSI output\n");
 
@@ -852,9 +852,40 @@ static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
 	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
 };
 
-static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = {
-	.disable	= sun6i_dsi_encoder_disable,
-	.enable		= sun6i_dsi_encoder_enable,
+static int sun6i_dsi_bridge_attach(struct drm_bridge *bridge,
+				   enum drm_bridge_attach_flags flags)
+{
+	struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
+	int ret;
+
+	if (dsi->panel_bridge)
+		return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, NULL, 0);
+
+	if (dsi->panel) {
+		drm_connector_helper_add(&dsi->connector,
+					 &sun6i_dsi_connector_helper_funcs);
+		ret = drm_connector_init(bridge->dev, &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);
+	}
+
+	return 0;
+
+err_cleanup_connector:
+	drm_encoder_cleanup(&dsi->encoder);
+	return ret;
+}
+
+static const struct drm_bridge_funcs sun6i_dsi_bridge_funcs = {
+	.enable		= sun6i_dsi_bridge_enable,
+	.disable	= sun6i_dsi_bridge_disable,
+	.attach		= sun6i_dsi_bridge_attach,
 };
 
 static u32 sun6i_dsi_dcs_build_pkt_hdr(struct sun6i_dsi *dsi,
@@ -1063,8 +1094,6 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
 	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
 	int ret;
 
-	drm_encoder_helper_add(&dsi->encoder,
-			       &sun6i_dsi_enc_helper_funcs);
 	ret = drm_simple_encoder_init(drm, &dsi->encoder,
 				      DRM_MODE_ENCODER_DSI);
 	if (ret) {
@@ -1073,27 +1102,12 @@ 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);
+	ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, 0);
 	if (ret) {
-		dev_err(dsi->dev,
-			"Couldn't initialise the DSI connector\n");
+		dev_err(dsi->dev, "Couldn't attach drm bridge\n");
 		goto err_cleanup_connector;
 	}
 
-	drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
-
-	if (dsi->panel_bridge) {
-		ret = drm_bridge_attach(&dsi->encoder, dsi->panel_bridge, NULL, 0);
-		if (ret) {
-			dev_err(dsi->dev, "Couldn't attach drm bridge\n");
-			goto err_cleanup_connector;
-		}
-	}
-
 	return 0;
 
 err_cleanup_connector:
@@ -1199,6 +1213,12 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
 		goto err_unprotect_clk;
 	}
 
+	dsi->bridge.funcs = &sun6i_dsi_bridge_funcs;
+	dsi->bridge.of_node = dev->of_node;
+	dsi->bridge.type = DRM_MODE_CONNECTOR_DSI;
+
+	drm_bridge_add(&dsi->bridge);
+
 	ret = component_add(&pdev->dev, &sun6i_dsi_ops);
 	if (ret) {
 		dev_err(dev, "Couldn't register our component\n");
@@ -1222,6 +1242,7 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
 
+	drm_bridge_remove(&dsi->bridge);
 	component_del(&pdev->dev, &sun6i_dsi_ops);
 	mipi_dsi_host_unregister(&dsi->host);
 	clk_rate_exclusive_put(dsi->mod_clk);
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
index 370ecb356a63..5e70666089ad 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
@@ -16,6 +16,7 @@
 #define SUN6I_DSI_TCON_DIV	4
 
 struct sun6i_dsi {
+	struct drm_bridge	bridge;
 	struct drm_connector	connector;
 	struct drm_encoder	encoder;
 	struct mipi_dsi_host	host;
@@ -38,6 +39,11 @@ static inline struct sun6i_dsi *host_to_sun6i_dsi(struct mipi_dsi_host *host)
 	return container_of(host, struct sun6i_dsi, host);
 };
 
+static inline struct sun6i_dsi *bridge_to_sun6i_dsi(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct sun6i_dsi, bridge);
+}
+
 static inline struct sun6i_dsi *connector_to_sun6i_dsi(struct drm_connector *connector)
 {
 	return container_of(connector, struct sun6i_dsi, connector);
-- 
2.25.1

Re: [PATCH v4 3/4] drm: sun4i: dsi: Convert to bridge driver

[Laurent Pinchart]

Hi Jagan,

Thank you for the patch.

On Mon, Mar 22, 2021 at 07:31:51PM +0530, Jagan Teki wrote:
> DRM bridge drivers have build-in handling of treating all display
> pipeline components as bridges.
> 
> So, convert the existing to a drm bridge driver with a built-in
> encoder support for compatibility with existing component drivers.

It would be best if possible to move this patch before 2/4, to first
convert to the bridge model, and then build on top of it.

> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v4:
> - none
> Changes for v3:
> - new patch
> 
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 75 ++++++++++++++++----------
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  6 +++
>  2 files changed, 54 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 39321299dc27..6f3c5330a468 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -714,10 +714,10 @@ static int sun6i_dsi_start(struct sun6i_dsi *dsi,
>  	return 0;
>  }
>  
> -static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
> +static void sun6i_dsi_bridge_enable(struct drm_bridge *bridge)
>  {
> -	struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
> -	struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
> +	struct drm_display_mode *mode = &bridge->encoder->crtc->state->adjusted_mode;
> +	struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
>  	struct mipi_dsi_device *device = dsi->device;
>  	union phy_configure_opts opts = { };
>  	struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
> @@ -801,9 +801,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
>  	sun6i_dsi_start(dsi, DSI_START_HSD);
>  }
>  
> -static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
> +static void sun6i_dsi_bridge_disable(struct drm_bridge *bridge)
>  {
> -	struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
> +	struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
>  
>  	DRM_DEBUG_DRIVER("Disabling DSI output\n");
>  
> @@ -852,9 +852,40 @@ static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
>  	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
>  };
>  
> -static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = {
> -	.disable	= sun6i_dsi_encoder_disable,
> -	.enable		= sun6i_dsi_encoder_enable,
> +static int sun6i_dsi_bridge_attach(struct drm_bridge *bridge,
> +				   enum drm_bridge_attach_flags flags)
> +{
> +	struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
> +	int ret;
> +
> +	if (dsi->panel_bridge)
> +		return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, NULL, 0);
> +
> +	if (dsi->panel) {
> +		drm_connector_helper_add(&dsi->connector,
> +					 &sun6i_dsi_connector_helper_funcs);
> +		ret = drm_connector_init(bridge->dev, &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);
> +	}
> +
> +	return 0;
> +
> +err_cleanup_connector:
> +	drm_encoder_cleanup(&dsi->encoder);
> +	return ret;
> +}
> +
> +static const struct drm_bridge_funcs sun6i_dsi_bridge_funcs = {
> +	.enable		= sun6i_dsi_bridge_enable,
> +	.disable	= sun6i_dsi_bridge_disable,
> +	.attach		= sun6i_dsi_bridge_attach,
>  };
>  
>  static u32 sun6i_dsi_dcs_build_pkt_hdr(struct sun6i_dsi *dsi,
> @@ -1063,8 +1094,6 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
>  	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
>  	int ret;
>  
> -	drm_encoder_helper_add(&dsi->encoder,
> -			       &sun6i_dsi_enc_helper_funcs);
>  	ret = drm_simple_encoder_init(drm, &dsi->encoder,
>  				      DRM_MODE_ENCODER_DSI);
>  	if (ret) {
> @@ -1073,27 +1102,12 @@ 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);
> +	ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, 0);
>  	if (ret) {
> -		dev_err(dsi->dev,
> -			"Couldn't initialise the DSI connector\n");
> +		dev_err(dsi->dev, "Couldn't attach drm bridge\n");
>  		goto err_cleanup_connector;
>  	}
>  
> -	drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
> -
> -	if (dsi->panel_bridge) {
> -		ret = drm_bridge_attach(&dsi->encoder, dsi->panel_bridge, NULL, 0);
> -		if (ret) {
> -			dev_err(dsi->dev, "Couldn't attach drm bridge\n");
> -			goto err_cleanup_connector;
> -		}
> -	}
> -
>  	return 0;
>  
>  err_cleanup_connector:
> @@ -1199,6 +1213,12 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
>  		goto err_unprotect_clk;
>  	}
>  
> +	dsi->bridge.funcs = &sun6i_dsi_bridge_funcs;
> +	dsi->bridge.of_node = dev->of_node;
> +	dsi->bridge.type = DRM_MODE_CONNECTOR_DSI;
> +
> +	drm_bridge_add(&dsi->bridge);
> +
>  	ret = component_add(&pdev->dev, &sun6i_dsi_ops);
>  	if (ret) {
>  		dev_err(dev, "Couldn't register our component\n");
> @@ -1222,6 +1242,7 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
>  
> +	drm_bridge_remove(&dsi->bridge);
>  	component_del(&pdev->dev, &sun6i_dsi_ops);
>  	mipi_dsi_host_unregister(&dsi->host);
>  	clk_rate_exclusive_put(dsi->mod_clk);
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> index 370ecb356a63..5e70666089ad 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> @@ -16,6 +16,7 @@
>  #define SUN6I_DSI_TCON_DIV	4
>  
>  struct sun6i_dsi {
> +	struct drm_bridge	bridge;
>  	struct drm_connector	connector;
>  	struct drm_encoder	encoder;

The drm_encoder should be dropped from this driver, the encoder should
be created by the main display driver.

>  	struct mipi_dsi_host	host;
> @@ -38,6 +39,11 @@ static inline struct sun6i_dsi *host_to_sun6i_dsi(struct mipi_dsi_host *host)
>  	return container_of(host, struct sun6i_dsi, host);
>  };
>  
> +static inline struct sun6i_dsi *bridge_to_sun6i_dsi(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct sun6i_dsi, bridge);
> +}
> +
>  static inline struct sun6i_dsi *connector_to_sun6i_dsi(struct drm_connector *connector)
>  {
>  	return container_of(connector, struct sun6i_dsi, connector);

-- 
Regards,

Laurent Pinchart

[DO NOT MERGE] [PATCH v4 4/4] ARM: dts: sun8i: bananapi-m2m: Enable S070WV20-CT16 panel

[Jagan Teki]

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

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

DSI panel connected via board DSI port with,
- DCDC1 as VCC-DSI supply
- PL5 gpio for bridge enable gpio pin
- PB7 gpio for lcd enable gpio pin
- PL4 gpio for backlight enable pin

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v4:
- replace reset with enable-gpios
Changes for v3:
- none 

 arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts | 85 ++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts b/arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts
index 293016d081cd..6f33e1ae8ffc 100644
--- a/arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts
+++ b/arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts
@@ -44,6 +44,7 @@
 #include "sun8i-a33.dtsi"
 
 #include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/pwm/pwm.h>
 
 / {
 	model = "BananaPi M2 Magic";
@@ -55,12 +56,21 @@ aliases {
 		i2c2 = &i2c2;
 		serial0 = &uart0;
 		serial1 = &uart1;
+		mmc0 = &mmc0;
 	};
 
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
 
+	backlight: backlight {
+		compatible = "pwm-backlight";
+		pwms = <&pwm 0 50000 PWM_POLARITY_INVERTED>;
+		brightness-levels = <1 2 4 8 16 32 64 128 255>;
+		default-brightness-level = <8>;
+		enable-gpios = <&r_pio 0 4 GPIO_ACTIVE_HIGH>; /* LCD-BL-EN: PL4 */
+	};
+
 	leds {
 		compatible = "gpio-leds";
 
@@ -81,6 +91,18 @@ led-2 {
 		};
 	};
 
+	panel {
+		compatible = "bananapi,s070wv20-ct16";
+		enable-gpios = <&pio 1 7 GPIO_ACTIVE_HIGH>; /* LCD-PWR-EN: PB7 */
+		backlight = <&backlight>;
+
+		port {
+			panel_out_bridge: endpoint {
+				remote-endpoint = <&bridge_out_panel>;
+			};
+		};
+	};
+
 	reg_vcc5v0: vcc5v0 {
 		compatible = "regulator-fixed";
 		regulator-name = "vcc5v0";
@@ -122,6 +144,59 @@ &dai {
 	status = "okay";
 };
 
+&de {
+	status = "okay";
+};
+
+&dphy {
+	status = "okay";
+};
+
+&dsi {
+	vcc-dsi-supply = <&reg_dcdc1>;		/* VCC-DSI */
+	status = "okay";
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		dsi_out: port@0 {
+			reg = <0>;
+
+			dsi_out_bridge: endpoint {
+				remote-endpoint = <&bridge_out_dsi>;
+			};
+		};
+	};
+
+	bridge@0 {
+		compatible = "chipone,icn6211";
+		reg = <0>;
+		enable-gpios = <&r_pio 0 5 GPIO_ACTIVE_HIGH>; /* LCD-RST: PL5 */
+
+		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>;
+				};
+			};
+		};
+	};
+};
+
 &ehci0 {
 	status = "okay";
 };
@@ -157,6 +232,12 @@ &ohci0 {
 	status = "okay";
 };
 
+&pwm {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pwm0_pin>;
+	status = "okay";
+};
+
 &r_rsb {
 	status = "okay";
 
@@ -269,6 +350,10 @@ &sound {
 	status = "okay";
 };
 
+&tcon0 {
+	status = "okay";
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_pb_pins>;
-- 
2.25.1