diff mbox

[v3,5/6] dt-bindings: add the rockchip, dual-channel for dw-mipi-dsi

Message ID 1508903463-7254-5-git-send-email-nickey.yang@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

Nickey Yang Oct. 25, 2017, 3:51 a.m. UTC
Configure dsi slave channel when driving a panel
which needs 2 DSI links.

Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
---
 .../devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt       | 2 ++
 1 file changed, 2 insertions(+)

Comments

Archit Taneja Oct. 26, 2017, 4:53 a.m. UTC | #1
On 10/25/2017 09:21 AM, Nickey Yang wrote:
> Configure dsi slave channel when driving a panel
> which needs 2 DSI links.
> 
> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> ---
>   .../devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt       | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt
> index 6bb59ab..a2bea22 100644
> --- a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt
> +++ b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt
> @@ -19,6 +19,8 @@ Optional properties:
>   - power-domains: a phandle to mipi dsi power domain node.
>   - resets: list of phandle + reset specifier pairs, as described in [3].
>   - reset-names: string reset name, must be "apb".
> +- rockchip,dual-channel: phandle to a 2nd DSI channel, useful as a slave
> +channel when driving a panel which needs 2 DSI links.
The example below is how dual DSI bindings could look like. Let me know what
you think of it.

If both DSI outputs drive the same device (i.e, point to the same panel DT
node), then I think it's reasonable enough to assume that the DSIs are
operating in a 'dual-channel' mode. That being said, we still need DT to
describe which of the DSIs generates the clock for both the channels. This
is done with the 'clock-master' DT binding.

Thanks,
Archit

mipi_dsi: mipi@ff960000 {
	...
	...

	clock-master;	/* implies that this DSI instance drivers the clock
			 * for both the DSIs.
			 */

	ports {
		mipi_in: port {
			...
			...
		};

		/* add extra output ports for both DSIs */
		mipi_out: port {
			mipi_panel_out: endpoint {
				remote-endpoint = <&panel_in_channel0>;
			};
		};
	};

	panel {
		...
		...
		/*
		 * panel node can describe its input ports, if both the DSIs output
		 * ports are connected to the same device (i.e, the same DSI panel),
		 * we can assume that the DSIs need to operate in dual DSI mode
		 */
		ports {
			...
			port@0 {
				panel_in_channel0: endpoint {
					remote-endpoint = <&mipi_panel_out>;
				};
			};

			port@1 {
				panel_in_channel1: endpoint {
					remote-endpoint = <&mipi1_panel_out>;
				};
	
			};
		};
	};
};


mipi_dsi1: mipi@ff968000 {
	...
	...

	ports {
		mipi1_in: port {
			...
			...
		};

		mipi1_out: port {
			mipi1_panel_out: endpoint {
				remote-endpoint = <&panel_in_channel1>;
			};
		};
	};
};

>   
>   [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>   [2] Documentation/devicetree/bindings/media/video-interfaces.txt
>
Nickey Yang Nov. 30, 2017, 5:32 p.m. UTC | #2
Hi Archit,


On 2017年10月26日 12:53, Archit Taneja wrote:
>
>
> On 10/25/2017 09:21 AM, Nickey Yang wrote:
>> Configure dsi slave channel when driving a panel
>> which needs 2 DSI links.
>>
>> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
>> ---
>> .../devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt 
>> b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt 
>>
>> index 6bb59ab..a2bea22 100644
>> --- 
>> a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt
>> +++ 
>> b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt
>> @@ -19,6 +19,8 @@ Optional properties:
>>   - power-domains: a phandle to mipi dsi power domain node.
>>   - resets: list of phandle + reset specifier pairs, as described in 
>> [3].
>>   - reset-names: string reset name, must be "apb".
>> +- rockchip,dual-channel: phandle to a 2nd DSI channel, useful as a 
>> slave
>> +channel when driving a panel which needs 2 DSI links.
> The example below is how dual DSI bindings could look like. Let me 
> know what
> you think of it.
>
> If both DSI outputs drive the same device (i.e, point to the same 
> panel DT
> node), then I think it's reasonable enough to assume that the DSIs are
> operating in a 'dual-channel' mode. That being said, we still need DT to
> describe which of the DSIs generates the clock for both the channels. 
> This
> is done with the 'clock-master' DT binding.
>
> Thanks,
> Archit
>
> mipi_dsi: mipi@ff960000 {
>     ...
>     ...
>
>     clock-master;    /* implies that this DSI instance drivers the clock
>              * for both the DSIs.
>              */
>
>     ports {
>         mipi_in: port {
>             ...
>             ...
>         };
>
>         /* add extra output ports for both DSIs */
>         mipi_out: port {
>             mipi_panel_out: endpoint {
>                 remote-endpoint = <&panel_in_channel0>;
>             };
>         };
>     };
>
>     panel {
>         ...
>         ...
>         /*
>          * panel node can describe its input ports, if both the DSIs 
> output
>          * ports are connected to the same device (i.e, the same DSI 
> panel),
>          * we can assume that the DSIs need to operate in dual DSI mode
>          */
>         ports {
>             ...
>             port@0 {
>                 panel_in_channel0: endpoint {
>                     remote-endpoint = <&mipi_panel_out>;
>                 };
>             };
>
>             port@1 {
>                 panel_in_channel1: endpoint {
>                     remote-endpoint = <&mipi1_panel_out>;
>                 };
>
>             };
>         };
>     };
> };
>
>
> mipi_dsi1: mipi@ff968000 {
>     ...
>     ...
>
>     ports {
>         mipi1_in: port {
>             ...
>             ...
>         };
>
>         mipi1_out: port {
>             mipi1_panel_out: endpoint {
>                 remote-endpoint = <&panel_in_channel1>;
>             };
>         };
>     };
> };
>
I try to follow as you suggested,use

mipi_dsi: mipi@ff960000 {
     ...
     ...
     clock-master;    /* implies that this DSI instance drivers the clock
              * for both the DSIs.
              */
     ports {
         mipi_in: port {
             ...
             ...
         };
         /* add extra output ports for both DSIs */
         mipi_out: port {
             mipi_panel_out: endpoint {
                 remote-endpoint = <&panel_in_channel0>;
             };
         };
     };
     panel {
         ...
         ...
         /*
          * panel node can describe its input ports, if both the DSIs output
          * ports are connected to the same device (i.e, the same DSI 
panel),
          * we can assume that the DSIs need to operate in dual DSI mode
          */
         ports {
             ...
             port@0 {
                 panel_in_channel0: endpoint {
                     remote-endpoint = <&mipi_panel_out>;
                 };
             };
             port@1 {
                 panel_in_channel1: endpoint {
                     remote-endpoint = <&mipi1_panel_out>;
                 };

             };
         };
     };
};

mipi_dsi1: mipi@ff968000 {
     ...
     ...
     ports {
         mipi1_in: port {
             ...
             ...
         };
         mipi1_out: port {
             mipi1_panel_out: endpoint {
                 remote-endpoint = <&panel_in_channel1>;
             };
         };
     };
}

But it seems we can not use of_drm_find_panel(like below)

/*
         port = of_graph_get_port_by_id(dev->of_node, 1);
         if (port) {
                 endpoint = of_get_child_by_name(port, "endpoint");
                 of_node_put(port);
                 if (!endpoint) {
                         dev_err(dev, "no output endpoint found\n");
                         return -EINVAL;
                 }
                 panel_node = of_graph_get_remote_port_parent(endpoint);
                 of_node_put(endpoint);
                 if (!panel_node) {
                         dev_err(dev, "no output node found\n");
                         return -EINVAL;
                 }
                 panel = of_drm_find_panel(panel_node);
                 of_node_put(panel_node);
                 if (!panel)
                         return -EPROBE_DEFER;
         }
*/
to get DSI1 outputs,because of_drm_find_panel need compare

if (panel->dev->of_node == np)

in dsi_panel driver innolux->base.dev = &innolux->link->dev;
dsi->dev

struct innolux_panel {
         struct drm_panel base;
         struct mipi_dsi_device *link;
};
It means one panel can only be found in his dsi node,(like dsi0 above).

I'm doubting about it, Or  may we follow tegra_dsi_ganged_probe
(drivers/gpu/drm/tergra/dsi.c) method.


Thanks,
Nickey

>>     [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>   [2] Documentation/devicetree/bindings/media/video-interfaces.txt
>>
>
Archit Taneja Dec. 1, 2017, 12:59 p.m. UTC | #3
On 11/30/2017 11:02 PM, Nickey Yang wrote:
> Hi Archit,
> 
> 
> On 2017年10月26日 12:53, Archit Taneja wrote:
>>
>>
>> On 10/25/2017 09:21 AM, Nickey Yang wrote:
>>> Configure dsi slave channel when driving a panel
>>> which needs 2 DSI links.
>>>
>>> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
>>> ---
>>> .../devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt 
>>> b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt
>>> index 6bb59ab..a2bea22 100644
>>> --- a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt
>>> +++ b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt
>>> @@ -19,6 +19,8 @@ Optional properties:
>>>   - power-domains: a phandle to mipi dsi power domain node.
>>>   - resets: list of phandle + reset specifier pairs, as described in [3].
>>>   - reset-names: string reset name, must be "apb".
>>> +- rockchip,dual-channel: phandle to a 2nd DSI channel, useful as a slave
>>> +channel when driving a panel which needs 2 DSI links.
>> The example below is how dual DSI bindings could look like. Let me know what
>> you think of it.
>>
>> If both DSI outputs drive the same device (i.e, point to the same panel DT
>> node), then I think it's reasonable enough to assume that the DSIs are
>> operating in a 'dual-channel' mode. That being said, we still need DT to
>> describe which of the DSIs generates the clock for both the channels. This
>> is done with the 'clock-master' DT binding.
>>
>> Thanks,
>> Archit
>>
>> mipi_dsi: mipi@ff960000 {
>>     ...
>>     ...
>>
>>     clock-master;    /* implies that this DSI instance drivers the clock
>>              * for both the DSIs.
>>              */
>>
>>     ports {
>>         mipi_in: port {
>>             ...
>>             ...
>>         };
>>
>>         /* add extra output ports for both DSIs */
>>         mipi_out: port {
>>             mipi_panel_out: endpoint {
>>                 remote-endpoint = <&panel_in_channel0>;
>>             };
>>         };
>>     };
>>
>>     panel {
>>         ...
>>         ...
>>         /*
>>          * panel node can describe its input ports, if both the DSIs output
>>          * ports are connected to the same device (i.e, the same DSI panel),
>>          * we can assume that the DSIs need to operate in dual DSI mode
>>          */
>>         ports {
>>             ...
>>             port@0 {
>>                 panel_in_channel0: endpoint {
>>                     remote-endpoint = <&mipi_panel_out>;
>>                 };
>>             };
>>
>>             port@1 {
>>                 panel_in_channel1: endpoint {
>>                     remote-endpoint = <&mipi1_panel_out>;
>>                 };
>>
>>             };
>>         };
>>     };
>> };
>>
>>
>> mipi_dsi1: mipi@ff968000 {
>>     ...
>>     ...
>>
>>     ports {
>>         mipi1_in: port {
>>             ...
>>             ...
>>         };
>>
>>         mipi1_out: port {
>>             mipi1_panel_out: endpoint {
>>                 remote-endpoint = <&panel_in_channel1>;
>>             };
>>         };
>>     };
>> };
>>
> I try to follow as you suggested,use
> 
> mipi_dsi: mipi@ff960000 {
>      ...
>      ...
>      clock-master;    /* implies that this DSI instance drivers the clock
>               * for both the DSIs.
>               */
>      ports {
>          mipi_in: port {
>              ...
>              ...
>          };
>          /* add extra output ports for both DSIs */
>          mipi_out: port {
>              mipi_panel_out: endpoint {
>                  remote-endpoint = <&panel_in_channel0>;
>              };
>          };
>      };
>      panel {
>          ...
>          ...
>          /*
>           * panel node can describe its input ports, if both the DSIs output
>           * ports are connected to the same device (i.e, the same DSI panel),
>           * we can assume that the DSIs need to operate in dual DSI mode
>           */
>          ports {
>              ...
>              port@0 {
>                  panel_in_channel0: endpoint {
>                      remote-endpoint = <&mipi_panel_out>;
>                  };
>              };
>              port@1 {
>                  panel_in_channel1: endpoint {
>                      remote-endpoint = <&mipi1_panel_out>;
>                  };
> 
>              };
>          };
>      };
> };
> 
> mipi_dsi1: mipi@ff968000 {
>      ...
>      ...
>      ports {
>          mipi1_in: port {
>              ...
>              ...
>          };
>          mipi1_out: port {
>              mipi1_panel_out: endpoint {
>                  remote-endpoint = <&panel_in_channel1>;
>              };
>          };
>      };
> }
> 
> But it seems we can not use of_drm_find_panel(like below)
> 
> /*
>          port = of_graph_get_port_by_id(dev->of_node, 1);
>          if (port) {
>                  endpoint = of_get_child_by_name(port, "endpoint");
>                  of_node_put(port);
>                  if (!endpoint) {
>                          dev_err(dev, "no output endpoint found\n");
>                          return -EINVAL;
>                  }
>                  panel_node = of_graph_get_remote_port_parent(endpoint);
>                  of_node_put(endpoint);
>                  if (!panel_node) {
>                          dev_err(dev, "no output node found\n");
>                          return -EINVAL;
>                  }
>                  panel = of_drm_find_panel(panel_node);
>                  of_node_put(panel_node);
>                  if (!panel)
>                          return -EPROBE_DEFER;
>          }
> */
> to get DSI1 outputs,because of_drm_find_panel need compare
> 
> if (panel->dev->of_node == np)
> 
> in dsi_panel driver innolux->base.dev = &innolux->link->dev;
> dsi->dev

Yes, we should only have 1 drm_panel in the global panel list.
Shouldn't it be possible to modify the dsi driver such that dsi1
doesn't care whether it has a drm_panel for it or not, if we are
in dual dsi mode?

I imagine a sequence like this:

1. dsi0 probes, parses the of-graph, finds the panel and saves its device
node.

2. dsi1 probes, parses the of-graph, find the panel's device node
   - dsi1 checks if it is the same as the panel attached to dsi0.
   - If so, it just takes the drm_panel pointer from dsi0.
   - If not, it tries a of_drm_find_panel() on the panel's device node.

A dual DSI panel driver would also be a bit different. It will be a
mipi_dsi_driver with the master DSI (dsi0) as the mipi_dsi_device. Using
the of-graph helpers, we would get the device node of dsi1 using
of_find_mipi_dsi_host_by_node(), and create another DSI device using
mipi_dsi_device_register_full(). Then, we call mipi_dsi_attach() on
both the dsi devices.

> 
> struct innolux_panel {
>          struct drm_panel base;
>          struct mipi_dsi_device *link;
> };
> It means one panel can only be found in his dsi node,(like dsi0 above).
> 
> I'm doubting about it, Or  may we follow tegra_dsi_ganged_probe
> (drivers/gpu/drm/tergra/dsi.c) method.

This method will add a new binding similar to "nvidia,ganged-mode", which
is something we don't want to do.

Archit

> 
> 
> Thanks,
> Nickey
> 
>>>     [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>   [2] Documentation/devicetree/bindings/media/video-interfaces.txt
>>>
>>
> 
>
Brian Norris Dec. 5, 2017, 1:19 a.m. UTC | #4
Hi Archit,

I'm a relative n00b here, but I'm trying to follow along and I have some
questions:

On Fri, Dec 01, 2017 at 06:29:04PM +0530, Archit Taneja wrote:
> On 11/30/2017 11:02 PM, Nickey Yang wrote:
> >I try to follow as you suggested,use
> >
> >mipi_dsi: mipi@ff960000 {
> >     ...
> >     ...
> >     clock-master;    /* implies that this DSI instance drivers the clock
> >              * for both the DSIs.
> >              */
> >     ports {
> >         mipi_in: port {
> >             ...
> >             ...
> >         };
> >         /* add extra output ports for both DSIs */
> >         mipi_out: port {
> >             mipi_panel_out: endpoint {
> >                 remote-endpoint = <&panel_in_channel0>;
> >             };
> >         };
> >     };
> >     panel {
> >         ...
> >         ...
> >         /*
> >          * panel node can describe its input ports, if both the DSIs output
> >          * ports are connected to the same device (i.e, the same DSI panel),
> >          * we can assume that the DSIs need to operate in dual DSI mode
> >          */
> >         ports {
> >             ...
> >             port@0 {
> >                 panel_in_channel0: endpoint {
> >                     remote-endpoint = <&mipi_panel_out>;
> >                 };
> >             };
> >             port@1 {
> >                 panel_in_channel1: endpoint {
> >                     remote-endpoint = <&mipi1_panel_out>;
> >                 };
> >
> >             };
> >         };
> >     };
> >};
> >
> >mipi_dsi1: mipi@ff968000 {
> >     ...
> >     ...
> >     ports {
> >         mipi1_in: port {
> >             ...
> >             ...
> >         };
> >         mipi1_out: port {
> >             mipi1_panel_out: endpoint {
> >                 remote-endpoint = <&panel_in_channel1>;
> >             };
> >         };
> >     };
> >}
> >
> >But it seems we can not use of_drm_find_panel(like below)
> >
> >/*
> >         port = of_graph_get_port_by_id(dev->of_node, 1);
> >         if (port) {
> >                 endpoint = of_get_child_by_name(port, "endpoint");
> >                 of_node_put(port);
> >                 if (!endpoint) {
> >                         dev_err(dev, "no output endpoint found\n");
> >                         return -EINVAL;
> >                 }
> >                 panel_node = of_graph_get_remote_port_parent(endpoint);
> >                 of_node_put(endpoint);
> >                 if (!panel_node) {
> >                         dev_err(dev, "no output node found\n");
> >                         return -EINVAL;
> >                 }
> >                 panel = of_drm_find_panel(panel_node);
> >                 of_node_put(panel_node);
> >                 if (!panel)
> >                         return -EPROBE_DEFER;
> >         }
> >*/
> >to get DSI1 outputs,because of_drm_find_panel need compare
> >
> >if (panel->dev->of_node == np)
> >
> >in dsi_panel driver innolux->base.dev = &innolux->link->dev;
> >dsi->dev
> 
> Yes, we should only have 1 drm_panel in the global panel list.
> Shouldn't it be possible to modify the dsi driver such that dsi1
> doesn't care whether it has a drm_panel for it or not, if we are
> in dual dsi mode?
> 
> I imagine a sequence like this:
> 
> 1. dsi0 probes, parses the of-graph, finds the panel and saves its device
> node.

Does this mean we depend on probe order? Or would we be able to
-EPROBE_DEFER or similar if dsi1 binds first?

> 2. dsi1 probes, parses the of-graph, find the panel's device node
>   - dsi1 checks if it is the same as the panel attached to dsi0.
>   - If so, it just takes the drm_panel pointer from dsi0.
>   - If not, it tries a of_drm_find_panel() on the panel's device node.

So, that all means we'd need a new variant of
drm_of_find_panel_or_bridge() for "dual" drivers like this? Or else
open-code this logic in dw-mipi-dsi.c?

> A dual DSI panel driver would also be a bit different. It will be a
> mipi_dsi_driver with the master DSI (dsi0) as the mipi_dsi_device. Using
> the of-graph helpers, we would get the device node of dsi1 using
> of_find_mipi_dsi_host_by_node(), and create another DSI device using
> mipi_dsi_device_register_full(). Then, we call mipi_dsi_attach() on
> both the dsi devices.

That seems...interesting. I guess that sounds like it could work, but
someone would have to play with that a bit more.

I assume one wouldn't want to do all this in every dual DSI driver that
needs this, right?

> >struct innolux_panel {
> >         struct drm_panel base;
> >         struct mipi_dsi_device *link;
> >};
> >It means one panel can only be found in his dsi node,(like dsi0 above).
> >
> >I'm doubting about it, Or  may we follow tegra_dsi_ganged_probe
> >(drivers/gpu/drm/tergra/dsi.c) method.
> 
> This method will add a new binding similar to "nvidia,ganged-mode", which
> is something we don't want to do.

It's unfortunate we have the anti-pattern already merged :(

Brian
Archit Taneja Dec. 5, 2017, 5:16 a.m. UTC | #5
On 12/05/2017 06:49 AM, Brian Norris wrote:
> Hi Archit,
> 
> I'm a relative n00b here, but I'm trying to follow along and I have some
> questions:
> 
> On Fri, Dec 01, 2017 at 06:29:04PM +0530, Archit Taneja wrote:
>> On 11/30/2017 11:02 PM, Nickey Yang wrote:
>>> I try to follow as you suggested,use
>>>
>>> mipi_dsi: mipi@ff960000 {
>>>      ...
>>>      ...
>>>      clock-master;    /* implies that this DSI instance drivers the clock
>>>               * for both the DSIs.
>>>               */
>>>      ports {
>>>          mipi_in: port {
>>>              ...
>>>              ...
>>>          };
>>>          /* add extra output ports for both DSIs */
>>>          mipi_out: port {
>>>              mipi_panel_out: endpoint {
>>>                  remote-endpoint = <&panel_in_channel0>;
>>>              };
>>>          };
>>>      };
>>>      panel {
>>>          ...
>>>          ...
>>>          /*
>>>           * panel node can describe its input ports, if both the DSIs output
>>>           * ports are connected to the same device (i.e, the same DSI panel),
>>>           * we can assume that the DSIs need to operate in dual DSI mode
>>>           */
>>>          ports {
>>>              ...
>>>              port@0 {
>>>                  panel_in_channel0: endpoint {
>>>                      remote-endpoint = <&mipi_panel_out>;
>>>                  };
>>>              };
>>>              port@1 {
>>>                  panel_in_channel1: endpoint {
>>>                      remote-endpoint = <&mipi1_panel_out>;
>>>                  };
>>>
>>>              };
>>>          };
>>>      };
>>> };
>>>
>>> mipi_dsi1: mipi@ff968000 {
>>>      ...
>>>      ...
>>>      ports {
>>>          mipi1_in: port {
>>>              ...
>>>              ...
>>>          };
>>>          mipi1_out: port {
>>>              mipi1_panel_out: endpoint {
>>>                  remote-endpoint = <&panel_in_channel1>;
>>>              };
>>>          };
>>>      };
>>> }
>>>
>>> But it seems we can not use of_drm_find_panel(like below)
>>>
>>> /*
>>>          port = of_graph_get_port_by_id(dev->of_node, 1);
>>>          if (port) {
>>>                  endpoint = of_get_child_by_name(port, "endpoint");
>>>                  of_node_put(port);
>>>                  if (!endpoint) {
>>>                          dev_err(dev, "no output endpoint found\n");
>>>                          return -EINVAL;
>>>                  }
>>>                  panel_node = of_graph_get_remote_port_parent(endpoint);
>>>                  of_node_put(endpoint);
>>>                  if (!panel_node) {
>>>                          dev_err(dev, "no output node found\n");
>>>                          return -EINVAL;
>>>                  }
>>>                  panel = of_drm_find_panel(panel_node);
>>>                  of_node_put(panel_node);
>>>                  if (!panel)
>>>                          return -EPROBE_DEFER;
>>>          }
>>> */
>>> to get DSI1 outputs,because of_drm_find_panel need compare
>>>
>>> if (panel->dev->of_node == np)
>>>
>>> in dsi_panel driver innolux->base.dev = &innolux->link->dev;
>>> dsi->dev
>>
>> Yes, we should only have 1 drm_panel in the global panel list.
>> Shouldn't it be possible to modify the dsi driver such that dsi1
>> doesn't care whether it has a drm_panel for it or not, if we are
>> in dual dsi mode?
>>
>> I imagine a sequence like this:
>>
>> 1. dsi0 probes, parses the of-graph, finds the panel and saves its device
>> node.
> 
> Does this mean we depend on probe order? Or would we be able to
> -EPROBE_DEFER or similar if dsi1 binds first?

I don't think the probe order should matter. However, I also don't know what
challenges it might bring up once we actually try to implement it. I can see
the the driver's of-graph parsing code getting a bit complicated. The first dsi
instance that probes/binds (say dsi1) should peek into the panels other ports
and see if it is the slave DSI instance in a dual DSI set-up. If so, it could
defer until DSI0 first probes and registers the panel.

Btw, full disclosure, I work on the drm/msm driver, and the code uses a binding
called "qcom,dual-dsi-mode" done by someone in the past, but thankfully it isn't
used in any dts file. I plan to remove these and use the bindings I've suggested
here.

Also, the bindings I've shared above are more a proof of concept, and based on
how dual DSI is implemented on the MSM chipsets. If the HW requires special
properties while operating in Dual DSI mode, then it might be okay to have
additional bindings. However, it seems strange to have a DT prop that says
"operate in dual DSI mode" if it can be inferred from the port connections.

I'll post a RFC explaining the bindings and copy all the people with kms drivers
that support DSI. Maybe we'd come up with a better consensus.

> 
>> 2. dsi1 probes, parses the of-graph, find the panel's device node
>>    - dsi1 checks if it is the same as the panel attached to dsi0.
>>    - If so, it just takes the drm_panel pointer from dsi0.
>>    - If not, it tries a of_drm_find_panel() on the panel's device node.
> 
> So, that all means we'd need a new variant of
> drm_of_find_panel_or_bridge() for "dual" drivers like this? Or else
> open-code this logic in dw-mipi-dsi.c?

Yeah. It would be nice to have these in helpers.

> 
>> A dual DSI panel driver would also be a bit different. It will be a
>> mipi_dsi_driver with the master DSI (dsi0) as the mipi_dsi_device. Using
>> the of-graph helpers, we would get the device node of dsi1 using
>> of_find_mipi_dsi_host_by_node(), and create another DSI device using
>> mipi_dsi_device_register_full(). Then, we call mipi_dsi_attach() on
>> both the dsi devices.
> 
> That seems...interesting. I guess that sounds like it could work, but
> someone would have to play with that a bit more.
> 
> I assume one wouldn't want to do all this in every dual DSI driver that
> needs this, right?

Yes. I agree.

> 
>>> struct innolux_panel {
>>>          struct drm_panel base;
>>>          struct mipi_dsi_device *link;
>>> };
>>> It means one panel can only be found in his dsi node,(like dsi0 above).
>>>
>>> I'm doubting about it, Or  may we follow tegra_dsi_ganged_probe
>>> (drivers/gpu/drm/tergra/dsi.c) method.
>>
>> This method will add a new binding similar to "nvidia,ganged-mode", which
>> is something we don't want to do.

Btw, we already have a dual DSI panel driver, which has a special phandle called
"link2":

Documentation/devicetree/bindings/display/panel/sharp,lq101r1sx01.txt

I don't think we should continue using a prop like link2, it seems like something
that should be replaced by of-graph usage.

Thanks,
Archit

> 
> It's unfortunate we have the anti-pattern already merged :(
> 
> Brian
>
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt
index 6bb59ab..a2bea22 100644
--- a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt
+++ b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt
@@ -19,6 +19,8 @@  Optional properties:
 - power-domains: a phandle to mipi dsi power domain node.
 - resets: list of phandle + reset specifier pairs, as described in [3].
 - reset-names: string reset name, must be "apb".
+- rockchip,dual-channel: phandle to a 2nd DSI channel, useful as a slave
+channel when driving a panel which needs 2 DSI links.
 
 [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
 [2] Documentation/devicetree/bindings/media/video-interfaces.txt