[v3,17/25] dt-bindings: panel: Add Bananapi S070WV20-CT16 ICN6211 MIPI-DSI to RGB bridge
diff mbox series

Message ID 20181026144344.27778-18-jagan@amarulasolutions.com
State Superseded
Headers show
Series
  • drm/sun4i: Allwinner A64 MIPI-DSI support
Related show

Commit Message

Jagan Teki Oct. 26, 2018, 2:43 p.m. UTC
Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB
bridge panel, which is available on same PCB with 24-bit RGB interface.

So, this patch adds DSI specific binding details on existing
dt-bindings file.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v3:
- Use existing binding doc and update dsi details
Changes for v2:
- none

 .../display/panel/bananapi,s070wv20-ct16.txt  | 31 +++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

Comments

Rob Herring Oct. 30, 2018, 8:23 p.m. UTC | #1
On Fri, 26 Oct 2018 20:13:36 +0530, Jagan Teki wrote:
> Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB
> bridge panel, which is available on same PCB with 24-bit RGB interface.
> 
> So, this patch adds DSI specific binding details on existing
> dt-bindings file.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v3:
> - Use existing binding doc and update dsi details
> Changes for v2:
> - none
> 
>  .../display/panel/bananapi,s070wv20-ct16.txt  | 31 +++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>
Andrzej Hajda Oct. 31, 2018, 8:53 a.m. UTC | #2
On 26.10.2018 16:43, Jagan Teki wrote:
> Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB
> bridge panel, which is available on same PCB with 24-bit RGB interface.
>
> So, this patch adds DSI specific binding details on existing
> dt-bindings file.
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v3:
> - Use existing binding doc and update dsi details
> Changes for v2:
> - none
>
>  .../display/panel/bananapi,s070wv20-ct16.txt  | 31 +++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> index 35bc0c839f49..b7855dc7c66f 100644
> --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> @@ -1,12 +1,39 @@
>  Banana Pi 7" (S070WV20-CT16) TFT LCD Panel
>  
> +S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface.
> +
> +Depending on the variant, the PCB attached to the panel module either
> +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via
> +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel
> +itself

As I understand this is display board, which contains 'pure' RGB panel
S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge.
These are separate devices, just connected by vendor to simplify its
assembly. Why don't you create then bridge driver for ICN6211 and RGB
panel driver for S070WV20-CT16 - it looks more generic.
Then you can describe both in dts and voila.
Creating drivers for every combo of devices (panel + bridge), just
because some vendor sells them together seems incorrect - we have
devicetree for it.

Regards
Andrzej


> +
>  Required properties:
> -- compatible: should be "bananapi,s070wv20-ct16"
> +- compatible:
> +  for 24-bit RGB interface, use "bananapi,s070wv20-ct16"
> +  for ICN6211 MIPI-DSI to RGB bridge, use "bananapi,s070wv20-ct16-icn6211"
> +
> +Required properties for RGB:
>  - power-supply: see ./panel-common.txt
>  
> +Required properties for MIPI-DSI to RGB:
> +- reg: for DSI virtual channel used by that screen
> +- avdd-supply: analog regulator dc1 switch
> +- dvdd-supply: 3v3 digital regulator
> +- reset-gpios: a GPIO phandle for the reset pin
> +
>  Optional properties:
> -- enable-gpios: see ./simple-panel.txt
> +- enable-gpios: see ./simple-panel.txt(not available in MIPI-DSI to RGB bridge)
>  - backlight: see ./simple-panel.txt
>  
>  This binding is compatible with the simple-panel binding, which is specified
>  in ./simple-panel.txt.
> +
> +Example:
> +panel@0 {
> +	compatible = "bananapi,s070wv20-ct16-icn6211";
> +	reg = <0>;
> +	avdd-supply = <&reg_dc1sw>;
> +	dvdd-supply = <&reg_dldo1>;
> +	reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
> +	backlight = <&backlight_dsi>;
> +};
Chen-Yu Tsai Oct. 31, 2018, 8:58 a.m. UTC | #3
On Wed, Oct 31, 2018 at 4:53 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> On 26.10.2018 16:43, Jagan Teki wrote:
> > Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB
> > bridge panel, which is available on same PCB with 24-bit RGB interface.
> >
> > So, this patch adds DSI specific binding details on existing
> > dt-bindings file.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> > Changes for v3:
> > - Use existing binding doc and update dsi details
> > Changes for v2:
> > - none
> >
> >  .../display/panel/bananapi,s070wv20-ct16.txt  | 31 +++++++++++++++++--
> >  1 file changed, 29 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> > index 35bc0c839f49..b7855dc7c66f 100644
> > --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> > +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> > @@ -1,12 +1,39 @@
> >  Banana Pi 7" (S070WV20-CT16) TFT LCD Panel
> >
> > +S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface.
> > +
> > +Depending on the variant, the PCB attached to the panel module either
> > +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via
> > +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel
> > +itself
>
> As I understand this is display board, which contains 'pure' RGB panel
> S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge.
> These are separate devices, just connected by vendor to simplify its
> assembly. Why don't you create then bridge driver for ICN6211 and RGB
> panel driver for S070WV20-CT16 - it looks more generic.
> Then you can describe both in dts and voila.
> Creating drivers for every combo of devices (panel + bridge), just
> because some vendor sells them together seems incorrect - we have
> devicetree for it.

Rob suggested this, and also the opposite: using the same
"bananapi,s070wv20-ct16"
compatible string for both types of connections, and have the driver deal with
detecting the bus type.

The thing about the bridge chip is that there's no available datasheet that
describes all the parts of the init sequence, in fact none at all. I managed
to work out some bits, but the others remain a mystery and must be hard-coded
to match the panel. That would work against having a generic bridge driver.

ChenYu

>
> Regards
> Andrzej
>
>
> > +
> >  Required properties:
> > -- compatible: should be "bananapi,s070wv20-ct16"
> > +- compatible:
> > +  for 24-bit RGB interface, use "bananapi,s070wv20-ct16"
> > +  for ICN6211 MIPI-DSI to RGB bridge, use "bananapi,s070wv20-ct16-icn6211"
> > +
> > +Required properties for RGB:
> >  - power-supply: see ./panel-common.txt
> >
> > +Required properties for MIPI-DSI to RGB:
> > +- reg: for DSI virtual channel used by that screen
> > +- avdd-supply: analog regulator dc1 switch
> > +- dvdd-supply: 3v3 digital regulator
> > +- reset-gpios: a GPIO phandle for the reset pin
> > +
> >  Optional properties:
> > -- enable-gpios: see ./simple-panel.txt
> > +- enable-gpios: see ./simple-panel.txt(not available in MIPI-DSI to RGB bridge)
> >  - backlight: see ./simple-panel.txt
> >
> >  This binding is compatible with the simple-panel binding, which is specified
> >  in ./simple-panel.txt.
> > +
> > +Example:
> > +panel@0 {
> > +     compatible = "bananapi,s070wv20-ct16-icn6211";
> > +     reg = <0>;
> > +     avdd-supply = <&reg_dc1sw>;
> > +     dvdd-supply = <&reg_dldo1>;
> > +     reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
> > +     backlight = <&backlight_dsi>;
> > +};
>
>
Andrzej Hajda Oct. 31, 2018, 9:15 a.m. UTC | #4
On 31.10.2018 09:58, Chen-Yu Tsai wrote:
> On Wed, Oct 31, 2018 at 4:53 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 26.10.2018 16:43, Jagan Teki wrote:
>>> Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB
>>> bridge panel, which is available on same PCB with 24-bit RGB interface.
>>>
>>> So, this patch adds DSI specific binding details on existing
>>> dt-bindings file.
>>>
>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>> ---
>>> Changes for v3:
>>> - Use existing binding doc and update dsi details
>>> Changes for v2:
>>> - none
>>>
>>>  .../display/panel/bananapi,s070wv20-ct16.txt  | 31 +++++++++++++++++--
>>>  1 file changed, 29 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
>>> index 35bc0c839f49..b7855dc7c66f 100644
>>> --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
>>> +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
>>> @@ -1,12 +1,39 @@
>>>  Banana Pi 7" (S070WV20-CT16) TFT LCD Panel
>>>
>>> +S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface.
>>> +
>>> +Depending on the variant, the PCB attached to the panel module either
>>> +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via
>>> +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel
>>> +itself
>> As I understand this is display board, which contains 'pure' RGB panel
>> S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge.
>> These are separate devices, just connected by vendor to simplify its
>> assembly. Why don't you create then bridge driver for ICN6211 and RGB
>> panel driver for S070WV20-CT16 - it looks more generic.
>> Then you can describe both in dts and voila.
>> Creating drivers for every combo of devices (panel + bridge), just
>> because some vendor sells them together seems incorrect - we have
>> devicetree for it.
> Rob suggested this, and also the opposite: using the same
> "bananapi,s070wv20-ct16"
> compatible string for both types of connections, and have the driver deal with
> detecting the bus type.
>
> The thing about the bridge chip is that there's no available datasheet that
> describes all the parts of the init sequence, in fact none at all. I managed
> to work out some bits, but the others remain a mystery and must be hard-coded
> to match the panel. That would work against having a generic bridge driver.


But it is common for many chips - 1st version of the driver is developed
on one platform and it supports only one configuration, if next platform
with the same cheap appears the driver is augmented if necessary.

Regards

Andrzej


>
> ChenYu
>
>> Regards
>> Andrzej
>>
>>
>>> +
>>>  Required properties:
>>> -- compatible: should be "bananapi,s070wv20-ct16"
>>> +- compatible:
>>> +  for 24-bit RGB interface, use "bananapi,s070wv20-ct16"
>>> +  for ICN6211 MIPI-DSI to RGB bridge, use "bananapi,s070wv20-ct16-icn6211"
>>> +
>>> +Required properties for RGB:
>>>  - power-supply: see ./panel-common.txt
>>>
>>> +Required properties for MIPI-DSI to RGB:
>>> +- reg: for DSI virtual channel used by that screen
>>> +- avdd-supply: analog regulator dc1 switch
>>> +- dvdd-supply: 3v3 digital regulator
>>> +- reset-gpios: a GPIO phandle for the reset pin
>>> +
>>>  Optional properties:
>>> -- enable-gpios: see ./simple-panel.txt
>>> +- enable-gpios: see ./simple-panel.txt(not available in MIPI-DSI to RGB bridge)
>>>  - backlight: see ./simple-panel.txt
>>>
>>>  This binding is compatible with the simple-panel binding, which is specified
>>>  in ./simple-panel.txt.
>>> +
>>> +Example:
>>> +panel@0 {
>>> +     compatible = "bananapi,s070wv20-ct16-icn6211";
>>> +     reg = <0>;
>>> +     avdd-supply = <&reg_dc1sw>;
>>> +     dvdd-supply = <&reg_dldo1>;
>>> +     reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
>>> +     backlight = <&backlight_dsi>;
>>> +};
>>
>
Julian Calaby Oct. 31, 2018, 9:15 a.m. UTC | #5
Hi Jagan,

On Wed, Oct 31, 2018 at 7:58 PM Chen-Yu Tsai <wens@csie.org> wrote:
>
> On Wed, Oct 31, 2018 at 4:53 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> >
> > On 26.10.2018 16:43, Jagan Teki wrote:
> > > Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB
> > > bridge panel, which is available on same PCB with 24-bit RGB interface.
> > >
> > > So, this patch adds DSI specific binding details on existing
> > > dt-bindings file.
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > ---
> > > Changes for v3:
> > > - Use existing binding doc and update dsi details
> > > Changes for v2:
> > > - none
> > >
> > >  .../display/panel/bananapi,s070wv20-ct16.txt  | 31 +++++++++++++++++--
> > >  1 file changed, 29 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> > > index 35bc0c839f49..b7855dc7c66f 100644
> > > --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> > > +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> > > @@ -1,12 +1,39 @@
> > >  Banana Pi 7" (S070WV20-CT16) TFT LCD Panel
> > >
> > > +S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface.
> > > +
> > > +Depending on the variant, the PCB attached to the panel module either
> > > +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via
> > > +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel
> > > +itself
> >
> > As I understand this is display board, which contains 'pure' RGB panel
> > S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge.
> > These are separate devices, just connected by vendor to simplify its
> > assembly. Why don't you create then bridge driver for ICN6211 and RGB
> > panel driver for S070WV20-CT16 - it looks more generic.
> > Then you can describe both in dts and voila.
> > Creating drivers for every combo of devices (panel + bridge), just
> > because some vendor sells them together seems incorrect - we have
> > devicetree for it.
>
> Rob suggested this, and also the opposite: using the same
> "bananapi,s070wv20-ct16"
> compatible string for both types of connections, and have the driver deal with
> detecting the bus type.
>
> The thing about the bridge chip is that there's no available datasheet that
> describes all the parts of the init sequence, in fact none at all. I managed
> to work out some bits, but the others remain a mystery and must be hard-coded
> to match the panel. That would work against having a generic bridge driver.

To me it seems logical that we'd model it as another step in the graph
between the DSI component and the panel. It's conceivable that some
other manufacturer will probably buy these for their panels and having
a somewhat generic driver seems vaguely future proof to me.

As I see it, the weird init process belongs to the bridge chip and the
timings belong to the panel and we shouldn't "confuse" them by giving
them the same compatible.

That said, I'm sure that these chips are already old hat and we'll
have something different and even more incomprehensible next week.

Thanks,

--
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Jagan Teki Nov. 6, 2018, 6:08 p.m. UTC | #6
On Wed, Oct 31, 2018 at 2:45 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> On 31.10.2018 09:58, Chen-Yu Tsai wrote:
> > On Wed, Oct 31, 2018 at 4:53 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> >> On 26.10.2018 16:43, Jagan Teki wrote:
> >>> Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB
> >>> bridge panel, which is available on same PCB with 24-bit RGB interface.
> >>>
> >>> So, this patch adds DSI specific binding details on existing
> >>> dt-bindings file.
> >>>
> >>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >>> ---
> >>> Changes for v3:
> >>> - Use existing binding doc and update dsi details
> >>> Changes for v2:
> >>> - none
> >>>
> >>>  .../display/panel/bananapi,s070wv20-ct16.txt  | 31 +++++++++++++++++--
> >>>  1 file changed, 29 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> >>> index 35bc0c839f49..b7855dc7c66f 100644
> >>> --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> >>> +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> >>> @@ -1,12 +1,39 @@
> >>>  Banana Pi 7" (S070WV20-CT16) TFT LCD Panel
> >>>
> >>> +S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface.
> >>> +
> >>> +Depending on the variant, the PCB attached to the panel module either
> >>> +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via
> >>> +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel
> >>> +itself
> >> As I understand this is display board, which contains 'pure' RGB panel
> >> S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge.
> >> These are separate devices, just connected by vendor to simplify its
> >> assembly. Why don't you create then bridge driver for ICN6211 and RGB
> >> panel driver for S070WV20-CT16 - it looks more generic.
> >> Then you can describe both in dts and voila.
> >> Creating drivers for every combo of devices (panel + bridge), just
> >> because some vendor sells them together seems incorrect - we have
> >> devicetree for it.
> > Rob suggested this, and also the opposite: using the same
> > "bananapi,s070wv20-ct16"
> > compatible string for both types of connections, and have the driver deal with
> > detecting the bus type.
> >
> > The thing about the bridge chip is that there's no available datasheet that
> > describes all the parts of the init sequence, in fact none at all. I managed
> > to work out some bits, but the others remain a mystery and must be hard-coded
> > to match the panel. That would work against having a generic bridge driver.
>
>
> But it is common for many chips - 1st version of the driver is developed
> on one platform and it supports only one configuration, if next platform
> with the same cheap appears the driver is augmented if necessary.

At-least few of the commands from panel initialization code, the
respective opcode data values are based on panel timings and even
clock value is different in DSI. I think it look hard to try bridge
driver for these restrictions, do you have any suggestions?
Jagan Teki Nov. 6, 2018, 6:13 p.m. UTC | #7
On Wed, Oct 31, 2018 at 2:46 PM Julian Calaby <julian.calaby@gmail.com> wrote:
>
> Hi Jagan,
>
> On Wed, Oct 31, 2018 at 7:58 PM Chen-Yu Tsai <wens@csie.org> wrote:
> >
> > On Wed, Oct 31, 2018 at 4:53 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> > >
> > > On 26.10.2018 16:43, Jagan Teki wrote:
> > > > Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB
> > > > bridge panel, which is available on same PCB with 24-bit RGB interface.
> > > >
> > > > So, this patch adds DSI specific binding details on existing
> > > > dt-bindings file.
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > ---
> > > > Changes for v3:
> > > > - Use existing binding doc and update dsi details
> > > > Changes for v2:
> > > > - none
> > > >
> > > >  .../display/panel/bananapi,s070wv20-ct16.txt  | 31 +++++++++++++++++--
> > > >  1 file changed, 29 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> > > > index 35bc0c839f49..b7855dc7c66f 100644
> > > > --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> > > > +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> > > > @@ -1,12 +1,39 @@
> > > >  Banana Pi 7" (S070WV20-CT16) TFT LCD Panel
> > > >
> > > > +S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface.
> > > > +
> > > > +Depending on the variant, the PCB attached to the panel module either
> > > > +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via
> > > > +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel
> > > > +itself
> > >
> > > As I understand this is display board, which contains 'pure' RGB panel
> > > S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge.
> > > These are separate devices, just connected by vendor to simplify its
> > > assembly. Why don't you create then bridge driver for ICN6211 and RGB
> > > panel driver for S070WV20-CT16 - it looks more generic.
> > > Then you can describe both in dts and voila.
> > > Creating drivers for every combo of devices (panel + bridge), just
> > > because some vendor sells them together seems incorrect - we have
> > > devicetree for it.
> >
> > Rob suggested this, and also the opposite: using the same
> > "bananapi,s070wv20-ct16"
> > compatible string for both types of connections, and have the driver deal with
> > detecting the bus type.
> >
> > The thing about the bridge chip is that there's no available datasheet that
> > describes all the parts of the init sequence, in fact none at all. I managed
> > to work out some bits, but the others remain a mystery and must be hard-coded
> > to match the panel. That would work against having a generic bridge driver.
>
> To me it seems logical that we'd model it as another step in the graph
> between the DSI component and the panel. It's conceivable that some
> other manufacturer will probably buy these for their panels and having
> a somewhat generic driver seems vaguely future proof to me.
>
> As I see it, the weird init process belongs to the bridge chip and the
> timings belong to the panel and we shouldn't "confuse" them by giving
> them the same compatible.

But the problem here is due to lack of information about the panel,
the init process command opcode data values seem to based on the panel
timings values. This look weird and ie reason for going into separate
panel driver with different compatible.
Andrzej Hajda Nov. 7, 2018, 9:11 a.m. UTC | #8
On 06.11.2018 19:08, Jagan Teki wrote:
> On Wed, Oct 31, 2018 at 2:45 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 31.10.2018 09:58, Chen-Yu Tsai wrote:
>>> On Wed, Oct 31, 2018 at 4:53 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>> On 26.10.2018 16:43, Jagan Teki wrote:
>>>>> Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB
>>>>> bridge panel, which is available on same PCB with 24-bit RGB interface.
>>>>>
>>>>> So, this patch adds DSI specific binding details on existing
>>>>> dt-bindings file.
>>>>>
>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>>>> ---
>>>>> Changes for v3:
>>>>> - Use existing binding doc and update dsi details
>>>>> Changes for v2:
>>>>> - none
>>>>>
>>>>>  .../display/panel/bananapi,s070wv20-ct16.txt  | 31 +++++++++++++++++--
>>>>>  1 file changed, 29 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
>>>>> index 35bc0c839f49..b7855dc7c66f 100644
>>>>> --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
>>>>> +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
>>>>> @@ -1,12 +1,39 @@
>>>>>  Banana Pi 7" (S070WV20-CT16) TFT LCD Panel
>>>>>
>>>>> +S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface.
>>>>> +
>>>>> +Depending on the variant, the PCB attached to the panel module either
>>>>> +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via
>>>>> +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel
>>>>> +itself
>>>> As I understand this is display board, which contains 'pure' RGB panel
>>>> S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge.
>>>> These are separate devices, just connected by vendor to simplify its
>>>> assembly. Why don't you create then bridge driver for ICN6211 and RGB
>>>> panel driver for S070WV20-CT16 - it looks more generic.
>>>> Then you can describe both in dts and voila.
>>>> Creating drivers for every combo of devices (panel + bridge), just
>>>> because some vendor sells them together seems incorrect - we have
>>>> devicetree for it.
>>> Rob suggested this, and also the opposite: using the same
>>> "bananapi,s070wv20-ct16"
>>> compatible string for both types of connections, and have the driver deal with
>>> detecting the bus type.
>>>
>>> The thing about the bridge chip is that there's no available datasheet that
>>> describes all the parts of the init sequence, in fact none at all. I managed
>>> to work out some bits, but the others remain a mystery and must be hard-coded
>>> to match the panel. That would work against having a generic bridge driver.
>>
>> But it is common for many chips - 1st version of the driver is developed
>> on one platform and it supports only one configuration, if next platform
>> with the same cheap appears the driver is augmented if necessary.
> At-least few of the commands from panel initialization code, the
> respective opcode data values are based on panel timings and even
> clock value is different in DSI. I think it look hard to try bridge
> driver for these restrictions, do you have any suggestions?


Where do you see an issue? Since panel is RGB it should have no
initialization sequence (beside regulator/gpio power on/off), so the
only thing to do is to figure out which regulators/gpios belongs to
which component - with publicly available specs it should be doable.

The whole initialization sequence is for the bridge, so you put it into
bridge driver, for starters it can be hardcoded.

Then you can:

1. Try to find other users of this ICN6211 chip and compare
initialization sequences to guess purpose of registers.

2. Try to get specs of the chip (ask vendor, distributor, grep Internet).

3. Do nothing - if there will be other users of the bridge they will do
this work.


Regards

Andrzej
Julian Calaby Nov. 7, 2018, 10:20 a.m. UTC | #9
Hi Jagan,

On Wed, Nov 7, 2018 at 5:13 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Wed, Oct 31, 2018 at 2:46 PM Julian Calaby <julian.calaby@gmail.com> wrote:
> >
> > Hi Jagan,
> >
> > On Wed, Oct 31, 2018 at 7:58 PM Chen-Yu Tsai <wens@csie.org> wrote:
> > >
> > > On Wed, Oct 31, 2018 at 4:53 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> > > >
> > > > On 26.10.2018 16:43, Jagan Teki wrote:
> > > > > Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB
> > > > > bridge panel, which is available on same PCB with 24-bit RGB interface.
> > > > >
> > > > > So, this patch adds DSI specific binding details on existing
> > > > > dt-bindings file.
> > > > >
> > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > ---
> > > > > Changes for v3:
> > > > > - Use existing binding doc and update dsi details
> > > > > Changes for v2:
> > > > > - none
> > > > >
> > > > >  .../display/panel/bananapi,s070wv20-ct16.txt  | 31 +++++++++++++++++--
> > > > >  1 file changed, 29 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> > > > > index 35bc0c839f49..b7855dc7c66f 100644
> > > > > --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> > > > > +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> > > > > @@ -1,12 +1,39 @@
> > > > >  Banana Pi 7" (S070WV20-CT16) TFT LCD Panel
> > > > >
> > > > > +S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface.
> > > > > +
> > > > > +Depending on the variant, the PCB attached to the panel module either
> > > > > +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via
> > > > > +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel
> > > > > +itself
> > > >
> > > > As I understand this is display board, which contains 'pure' RGB panel
> > > > S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge.
> > > > These are separate devices, just connected by vendor to simplify its
> > > > assembly. Why don't you create then bridge driver for ICN6211 and RGB
> > > > panel driver for S070WV20-CT16 - it looks more generic.
> > > > Then you can describe both in dts and voila.
> > > > Creating drivers for every combo of devices (panel + bridge), just
> > > > because some vendor sells them together seems incorrect - we have
> > > > devicetree for it.
> > >
> > > Rob suggested this, and also the opposite: using the same
> > > "bananapi,s070wv20-ct16"
> > > compatible string for both types of connections, and have the driver deal with
> > > detecting the bus type.
> > >
> > > The thing about the bridge chip is that there's no available datasheet that
> > > describes all the parts of the init sequence, in fact none at all. I managed
> > > to work out some bits, but the others remain a mystery and must be hard-coded
> > > to match the panel. That would work against having a generic bridge driver.
> >
> > To me it seems logical that we'd model it as another step in the graph
> > between the DSI component and the panel. It's conceivable that some
> > other manufacturer will probably buy these for their panels and having
> > a somewhat generic driver seems vaguely future proof to me.
> >
> > As I see it, the weird init process belongs to the bridge chip and the
> > timings belong to the panel and we shouldn't "confuse" them by giving
> > them the same compatible.
>
> But the problem here is due to lack of information about the panel,
> the init process command opcode data values seem to based on the panel
> timings values. This look weird and ie reason for going into separate
> panel driver with different compatible.

I'd missed that particular wrinkle.

In that case, it makes sense to produce a panel driver with the bridge
chip's compatible.

Thanks,
Jagan Teki Nov. 10, 2018, 7:32 a.m. UTC | #10
On Wed, Nov 7, 2018 at 2:41 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> On 06.11.2018 19:08, Jagan Teki wrote:
> > On Wed, Oct 31, 2018 at 2:45 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> >> On 31.10.2018 09:58, Chen-Yu Tsai wrote:
> >>> On Wed, Oct 31, 2018 at 4:53 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> >>>> On 26.10.2018 16:43, Jagan Teki wrote:
> >>>>> Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB
> >>>>> bridge panel, which is available on same PCB with 24-bit RGB interface.
> >>>>>
> >>>>> So, this patch adds DSI specific binding details on existing
> >>>>> dt-bindings file.
> >>>>>
> >>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >>>>> ---
> >>>>> Changes for v3:
> >>>>> - Use existing binding doc and update dsi details
> >>>>> Changes for v2:
> >>>>> - none
> >>>>>
> >>>>>  .../display/panel/bananapi,s070wv20-ct16.txt  | 31 +++++++++++++++++--
> >>>>>  1 file changed, 29 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> >>>>> index 35bc0c839f49..b7855dc7c66f 100644
> >>>>> --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> >>>>> +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> >>>>> @@ -1,12 +1,39 @@
> >>>>>  Banana Pi 7" (S070WV20-CT16) TFT LCD Panel
> >>>>>
> >>>>> +S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface.
> >>>>> +
> >>>>> +Depending on the variant, the PCB attached to the panel module either
> >>>>> +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via
> >>>>> +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel
> >>>>> +itself
> >>>> As I understand this is display board, which contains 'pure' RGB panel
> >>>> S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge.
> >>>> These are separate devices, just connected by vendor to simplify its
> >>>> assembly. Why don't you create then bridge driver for ICN6211 and RGB
> >>>> panel driver for S070WV20-CT16 - it looks more generic.
> >>>> Then you can describe both in dts and voila.
> >>>> Creating drivers for every combo of devices (panel + bridge), just
> >>>> because some vendor sells them together seems incorrect - we have
> >>>> devicetree for it.
> >>> Rob suggested this, and also the opposite: using the same
> >>> "bananapi,s070wv20-ct16"
> >>> compatible string for both types of connections, and have the driver deal with
> >>> detecting the bus type.
> >>>
> >>> The thing about the bridge chip is that there's no available datasheet that
> >>> describes all the parts of the init sequence, in fact none at all. I managed
> >>> to work out some bits, but the others remain a mystery and must be hard-coded
> >>> to match the panel. That would work against having a generic bridge driver.
> >>
> >> But it is common for many chips - 1st version of the driver is developed
> >> on one platform and it supports only one configuration, if next platform
> >> with the same cheap appears the driver is augmented if necessary.
> > At-least few of the commands from panel initialization code, the
> > respective opcode data values are based on panel timings and even
> > clock value is different in DSI. I think it look hard to try bridge
> > driver for these restrictions, do you have any suggestions?
>
>
> Where do you see an issue? Since panel is RGB it should have no
> initialization sequence (beside regulator/gpio power on/off), so the
> only thing to do is to figure out which regulators/gpios belongs to
> which component - with publicly available specs it should be doable.
>
> The whole initialization sequence is for the bridge, so you put it into
> bridge driver, for starters it can be hardcoded.

Yes, I understand we can move regulators/gpio setup separately and
though we hardcode the init sequence there is difference  in clock for
DSI(which I mentioned in previous mail). DSI panel can't work with
clock used by RGB panel-simple.

>
> Then you can:
>
> 1. Try to find other users of this ICN6211 chip and compare
> initialization sequences to guess purpose of registers.
>
> 2. Try to get specs of the chip (ask vendor, distributor, grep Internet).

As we mentioned (even Chen-Yu), we are unable to find the proper spec
for this panel, all we taken reference from AW BSP code.

>
> 3. Do nothing - if there will be other users of the bridge they will do
> this work.

Don't know how we can go with generic bridge driver irrespective of
these particular wrinkles, let me know if you have any suggestions.
Andrzej Hajda Nov. 13, 2018, 7:56 a.m. UTC | #11
On 10.11.2018 08:32, Jagan Teki wrote:
> On Wed, Nov 7, 2018 at 2:41 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 06.11.2018 19:08, Jagan Teki wrote:
>>> On Wed, Oct 31, 2018 at 2:45 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>> On 31.10.2018 09:58, Chen-Yu Tsai wrote:
>>>>> On Wed, Oct 31, 2018 at 4:53 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>>> On 26.10.2018 16:43, Jagan Teki wrote:
>>>>>>> Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB
>>>>>>> bridge panel, which is available on same PCB with 24-bit RGB interface.
>>>>>>>
>>>>>>> So, this patch adds DSI specific binding details on existing
>>>>>>> dt-bindings file.
>>>>>>>
>>>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>>>>>> ---
>>>>>>> Changes for v3:
>>>>>>> - Use existing binding doc and update dsi details
>>>>>>> Changes for v2:
>>>>>>> - none
>>>>>>>
>>>>>>>  .../display/panel/bananapi,s070wv20-ct16.txt  | 31 +++++++++++++++++--
>>>>>>>  1 file changed, 29 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
>>>>>>> index 35bc0c839f49..b7855dc7c66f 100644
>>>>>>> --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
>>>>>>> @@ -1,12 +1,39 @@
>>>>>>>  Banana Pi 7" (S070WV20-CT16) TFT LCD Panel
>>>>>>>
>>>>>>> +S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface.
>>>>>>> +
>>>>>>> +Depending on the variant, the PCB attached to the panel module either
>>>>>>> +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via
>>>>>>> +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel
>>>>>>> +itself
>>>>>> As I understand this is display board, which contains 'pure' RGB panel
>>>>>> S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge.
>>>>>> These are separate devices, just connected by vendor to simplify its
>>>>>> assembly. Why don't you create then bridge driver for ICN6211 and RGB
>>>>>> panel driver for S070WV20-CT16 - it looks more generic.
>>>>>> Then you can describe both in dts and voila.
>>>>>> Creating drivers for every combo of devices (panel + bridge), just
>>>>>> because some vendor sells them together seems incorrect - we have
>>>>>> devicetree for it.
>>>>> Rob suggested this, and also the opposite: using the same
>>>>> "bananapi,s070wv20-ct16"
>>>>> compatible string for both types of connections, and have the driver deal with
>>>>> detecting the bus type.
>>>>>
>>>>> The thing about the bridge chip is that there's no available datasheet that
>>>>> describes all the parts of the init sequence, in fact none at all. I managed
>>>>> to work out some bits, but the others remain a mystery and must be hard-coded
>>>>> to match the panel. That would work against having a generic bridge driver.
>>>> But it is common for many chips - 1st version of the driver is developed
>>>> on one platform and it supports only one configuration, if next platform
>>>> with the same cheap appears the driver is augmented if necessary.
>>> At-least few of the commands from panel initialization code, the
>>> respective opcode data values are based on panel timings and even
>>> clock value is different in DSI. I think it look hard to try bridge
>>> driver for these restrictions, do you have any suggestions?
>>
>> Where do you see an issue? Since panel is RGB it should have no
>> initialization sequence (beside regulator/gpio power on/off), so the
>> only thing to do is to figure out which regulators/gpios belongs to
>> which component - with publicly available specs it should be doable.
>>
>> The whole initialization sequence is for the bridge, so you put it into
>> bridge driver, for starters it can be hardcoded.
> Yes, I understand we can move regulators/gpio setup separately and
> though we hardcode the init sequence there is difference  in clock for
> DSI(which I mentioned in previous mail). DSI panel can't work with
> clock used by RGB panel-simple.


If you mean pixel clock from timings in next patch it seems incorrect.
Pixel clock should be always

htotal * vtotal * vrefresh, in case of drm_display_mode result should be
divided by 1000 (as .clock is in kHz).

With timings provided there you have: 928*525*60 = 29232000

So pixel clock should be 29232, if other timings are correct. DSI clock
is a different thing and it is private thing of DSI bridge/panel it
should not be exposed via drm_display_mode.


Regards

Andrzej


>
>> Then you can:
>>
>> 1. Try to find other users of this ICN6211 chip and compare
>> initialization sequences to guess purpose of registers.
>>
>> 2. Try to get specs of the chip (ask vendor, distributor, grep Internet).
> As we mentioned (even Chen-Yu), we are unable to find the proper spec
> for this panel, all we taken reference from AW BSP code.
>
>> 3. Do nothing - if there will be other users of the bridge they will do
>> this work.
> Don't know how we can go with generic bridge driver irrespective of
> these particular wrinkles, let me know if you have any suggestions.
>
>
Jagan Teki Nov. 18, 2018, 6:20 p.m. UTC | #12
On Tue, Nov 13, 2018 at 1:26 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> On 10.11.2018 08:32, Jagan Teki wrote:
> > On Wed, Nov 7, 2018 at 2:41 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> >> On 06.11.2018 19:08, Jagan Teki wrote:
> >>> On Wed, Oct 31, 2018 at 2:45 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> >>>> On 31.10.2018 09:58, Chen-Yu Tsai wrote:
> >>>>> On Wed, Oct 31, 2018 at 4:53 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
> >>>>>> On 26.10.2018 16:43, Jagan Teki wrote:
> >>>>>>> Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB
> >>>>>>> bridge panel, which is available on same PCB with 24-bit RGB interface.
> >>>>>>>
> >>>>>>> So, this patch adds DSI specific binding details on existing
> >>>>>>> dt-bindings file.
> >>>>>>>
> >>>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >>>>>>> ---
> >>>>>>> Changes for v3:
> >>>>>>> - Use existing binding doc and update dsi details
> >>>>>>> Changes for v2:
> >>>>>>> - none
> >>>>>>>
> >>>>>>>  .../display/panel/bananapi,s070wv20-ct16.txt  | 31 +++++++++++++++++--
> >>>>>>>  1 file changed, 29 insertions(+), 2 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> >>>>>>> index 35bc0c839f49..b7855dc7c66f 100644
> >>>>>>> --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> >>>>>>> +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
> >>>>>>> @@ -1,12 +1,39 @@
> >>>>>>>  Banana Pi 7" (S070WV20-CT16) TFT LCD Panel
> >>>>>>>
> >>>>>>> +S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface.
> >>>>>>> +
> >>>>>>> +Depending on the variant, the PCB attached to the panel module either
> >>>>>>> +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via
> >>>>>>> +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel
> >>>>>>> +itself
> >>>>>> As I understand this is display board, which contains 'pure' RGB panel
> >>>>>> S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge.
> >>>>>> These are separate devices, just connected by vendor to simplify its
> >>>>>> assembly. Why don't you create then bridge driver for ICN6211 and RGB
> >>>>>> panel driver for S070WV20-CT16 - it looks more generic.
> >>>>>> Then you can describe both in dts and voila.
> >>>>>> Creating drivers for every combo of devices (panel + bridge), just
> >>>>>> because some vendor sells them together seems incorrect - we have
> >>>>>> devicetree for it.
> >>>>> Rob suggested this, and also the opposite: using the same
> >>>>> "bananapi,s070wv20-ct16"
> >>>>> compatible string for both types of connections, and have the driver deal with
> >>>>> detecting the bus type.
> >>>>>
> >>>>> The thing about the bridge chip is that there's no available datasheet that
> >>>>> describes all the parts of the init sequence, in fact none at all. I managed
> >>>>> to work out some bits, but the others remain a mystery and must be hard-coded
> >>>>> to match the panel. That would work against having a generic bridge driver.
> >>>> But it is common for many chips - 1st version of the driver is developed
> >>>> on one platform and it supports only one configuration, if next platform
> >>>> with the same cheap appears the driver is augmented if necessary.
> >>> At-least few of the commands from panel initialization code, the
> >>> respective opcode data values are based on panel timings and even
> >>> clock value is different in DSI. I think it look hard to try bridge
> >>> driver for these restrictions, do you have any suggestions?
> >>
> >> Where do you see an issue? Since panel is RGB it should have no
> >> initialization sequence (beside regulator/gpio power on/off), so the
> >> only thing to do is to figure out which regulators/gpios belongs to
> >> which component - with publicly available specs it should be doable.
> >>
> >> The whole initialization sequence is for the bridge, so you put it into
> >> bridge driver, for starters it can be hardcoded.
> > Yes, I understand we can move regulators/gpio setup separately and
> > though we hardcode the init sequence there is difference  in clock for
> > DSI(which I mentioned in previous mail). DSI panel can't work with
> > clock used by RGB panel-simple.
>
>
> If you mean pixel clock from timings in next patch it seems incorrect.
> Pixel clock should be always
>
> htotal * vtotal * vrefresh, in case of drm_display_mode result should be
> divided by 1000 (as .clock is in kHz).
>
> With timings provided there you have: 928*525*60 = 29232000
>
> So pixel clock should be 29232, if other timings are correct. DSI clock
> is a different thing and it is private thing of DSI bridge/panel it
> should not be exposed via drm_display_mode.

I understand your point, but in Allwinner case DSI clock which is
named in tcon block can be computed using clock value from
drm_display_mode from panel driver.

DE -> Mixer -> tcon-> MIPI <--> panel

So, for the value
- 55MHz panel clock the computed divider for DSI clock in tcon [1] is
6 which is proper and working divider
- 30MHz panel clock the computed divider for DSI clock in tcon [2] is
11 which can't work for this specific panel.

I have verified other panels, the divider value work similar to this
computation and those panels work accordingly.  but this panel case
the divider computation look weird, so we have used working and know
clock. I'm thinking since this panel is of DSI to RGB bridge, so it
may require some higher frequency to work but I've no document to
confirm the same.

Hope this information help, let me know if you have any inputs.

[1] https://paste.ubuntu.com/p/drvzfHFMtY/
[2] https://paste.ubuntu.com/p/hz29CTJY2J/
Andrzej Hajda Nov. 19, 2018, 9:49 a.m. UTC | #13
On 18.11.2018 19:20, Jagan Teki wrote:
> On Tue, Nov 13, 2018 at 1:26 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 10.11.2018 08:32, Jagan Teki wrote:
>>> On Wed, Nov 7, 2018 at 2:41 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>> On 06.11.2018 19:08, Jagan Teki wrote:
>>>>> On Wed, Oct 31, 2018 at 2:45 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>>> On 31.10.2018 09:58, Chen-Yu Tsai wrote:
>>>>>>> On Wed, Oct 31, 2018 at 4:53 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>>>>> On 26.10.2018 16:43, Jagan Teki wrote:
>>>>>>>>> Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB
>>>>>>>>> bridge panel, which is available on same PCB with 24-bit RGB interface.
>>>>>>>>>
>>>>>>>>> So, this patch adds DSI specific binding details on existing
>>>>>>>>> dt-bindings file.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>>>>>>>> ---
>>>>>>>>> Changes for v3:
>>>>>>>>> - Use existing binding doc and update dsi details
>>>>>>>>> Changes for v2:
>>>>>>>>> - none
>>>>>>>>>
>>>>>>>>>  .../display/panel/bananapi,s070wv20-ct16.txt  | 31 +++++++++++++++++--
>>>>>>>>>  1 file changed, 29 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
>>>>>>>>> index 35bc0c839f49..b7855dc7c66f 100644
>>>>>>>>> --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
>>>>>>>>> +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
>>>>>>>>> @@ -1,12 +1,39 @@
>>>>>>>>>  Banana Pi 7" (S070WV20-CT16) TFT LCD Panel
>>>>>>>>>
>>>>>>>>> +S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface.
>>>>>>>>> +
>>>>>>>>> +Depending on the variant, the PCB attached to the panel module either
>>>>>>>>> +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via
>>>>>>>>> +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel
>>>>>>>>> +itself
>>>>>>>> As I understand this is display board, which contains 'pure' RGB panel
>>>>>>>> S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge.
>>>>>>>> These are separate devices, just connected by vendor to simplify its
>>>>>>>> assembly. Why don't you create then bridge driver for ICN6211 and RGB
>>>>>>>> panel driver for S070WV20-CT16 - it looks more generic.
>>>>>>>> Then you can describe both in dts and voila.
>>>>>>>> Creating drivers for every combo of devices (panel + bridge), just
>>>>>>>> because some vendor sells them together seems incorrect - we have
>>>>>>>> devicetree for it.
>>>>>>> Rob suggested this, and also the opposite: using the same
>>>>>>> "bananapi,s070wv20-ct16"
>>>>>>> compatible string for both types of connections, and have the driver deal with
>>>>>>> detecting the bus type.
>>>>>>>
>>>>>>> The thing about the bridge chip is that there's no available datasheet that
>>>>>>> describes all the parts of the init sequence, in fact none at all. I managed
>>>>>>> to work out some bits, but the others remain a mystery and must be hard-coded
>>>>>>> to match the panel. That would work against having a generic bridge driver.
>>>>>> But it is common for many chips - 1st version of the driver is developed
>>>>>> on one platform and it supports only one configuration, if next platform
>>>>>> with the same cheap appears the driver is augmented if necessary.
>>>>> At-least few of the commands from panel initialization code, the
>>>>> respective opcode data values are based on panel timings and even
>>>>> clock value is different in DSI. I think it look hard to try bridge
>>>>> driver for these restrictions, do you have any suggestions?
>>>> Where do you see an issue? Since panel is RGB it should have no
>>>> initialization sequence (beside regulator/gpio power on/off), so the
>>>> only thing to do is to figure out which regulators/gpios belongs to
>>>> which component - with publicly available specs it should be doable.
>>>>
>>>> The whole initialization sequence is for the bridge, so you put it into
>>>> bridge driver, for starters it can be hardcoded.
>>> Yes, I understand we can move regulators/gpio setup separately and
>>> though we hardcode the init sequence there is difference  in clock for
>>> DSI(which I mentioned in previous mail). DSI panel can't work with
>>> clock used by RGB panel-simple.
>>
>> If you mean pixel clock from timings in next patch it seems incorrect.
>> Pixel clock should be always
>>
>> htotal * vtotal * vrefresh, in case of drm_display_mode result should be
>> divided by 1000 (as .clock is in kHz).
>>
>> With timings provided there you have: 928*525*60 = 29232000
>>
>> So pixel clock should be 29232, if other timings are correct. DSI clock
>> is a different thing and it is private thing of DSI bridge/panel it
>> should not be exposed via drm_display_mode.
> I understand your point, but in Allwinner case DSI clock which is
> named in tcon block can be computed using clock value from
> drm_display_mode from panel driver.


OK, it sounds like me telling you that 2+2=4 and you answering that you
understand my point but in your case 2+2=5 fits better.

This is nothing about 'my point', this is simple fact that if you have
928*525 pixels (including hidden ones) with refresh rate 60Hz pixelclock
is 29232kHz.


>
> DE -> Mixer -> tcon-> MIPI <--> panel
>
> So, for the value
> - 55MHz panel clock the computed divider for DSI clock in tcon [1] is
> 6 which is proper and working divider
> - 30MHz panel clock the computed divider for DSI clock in tcon [2] is
> 11 which can't work for this specific panel.


Please explain what exactly means "panel clock" and the divider here.

What rate reports modetest -v ...? For this and other panels.

Maybe we can figure out what should be fixed/adjusted.


Regards

Andrzej


>
> I have verified other panels, the divider value work similar to this
> computation and those panels work accordingly.  but this panel case
> the divider computation look weird, so we have used working and know
> clock. I'm thinking since this panel is of DSI to RGB bridge, so it
> may require some higher frequency to work but I've no document to
> confirm the same.
>
> Hope this information help, let me know if you have any inputs.
>
> [1] https://paste.ubuntu.com/p/drvzfHFMtY/
> [2] https://paste.ubuntu.com/p/hz29CTJY2J/
>
>

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
index 35bc0c839f49..b7855dc7c66f 100644
--- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
+++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
@@ -1,12 +1,39 @@ 
 Banana Pi 7" (S070WV20-CT16) TFT LCD Panel
 
+S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface.
+
+Depending on the variant, the PCB attached to the panel module either
+supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via
+an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel
+itself
+
 Required properties:
-- compatible: should be "bananapi,s070wv20-ct16"
+- compatible:
+  for 24-bit RGB interface, use "bananapi,s070wv20-ct16"
+  for ICN6211 MIPI-DSI to RGB bridge, use "bananapi,s070wv20-ct16-icn6211"
+
+Required properties for RGB:
 - power-supply: see ./panel-common.txt
 
+Required properties for MIPI-DSI to RGB:
+- reg: for DSI virtual channel used by that screen
+- avdd-supply: analog regulator dc1 switch
+- dvdd-supply: 3v3 digital regulator
+- reset-gpios: a GPIO phandle for the reset pin
+
 Optional properties:
-- enable-gpios: see ./simple-panel.txt
+- enable-gpios: see ./simple-panel.txt(not available in MIPI-DSI to RGB bridge)
 - backlight: see ./simple-panel.txt
 
 This binding is compatible with the simple-panel binding, which is specified
 in ./simple-panel.txt.
+
+Example:
+panel@0 {
+	compatible = "bananapi,s070wv20-ct16-icn6211";
+	reg = <0>;
+	avdd-supply = <&reg_dc1sw>;
+	dvdd-supply = <&reg_dldo1>;
+	reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
+	backlight = <&backlight_dsi>;
+};