diff mbox

[V7,1/4] Documentation/devicetree/bindings: b850v3_lvds_dp

Message ID 21ea1b0795bdaa30ca475d6ce675b620b2b644ed.1483301745.git.peter.senna@collabora.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Senna Tschudin Jan. 1, 2017, 8:24 p.m. UTC
Devicetree bindings documentation for the GE B850v3 LVDS/DP++
display bridge.

Cc: Martyn Welch <martyn.welch@collabora.co.uk>
Cc: Martin Donnelly <martin.donnelly@ge.com>
Cc: Javier Martinez Canillas <javier@dowhile0.org>
Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Rob Herring <robh@kernel.org>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
---
There was an Acked-by from Rob Herring <robh@kernel.org> for V6, but I changed
the bindings to use i2c_new_secondary_device() so I removed it from the commit
message.

 .../devicetree/bindings/ge/b850v3-lvds-dp.txt      | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt

Comments

Rob Herring Jan. 3, 2017, 10:51 p.m. UTC | #1
On Sun, Jan 01, 2017 at 09:24:29PM +0100, Peter Senna Tschudin wrote:
> Devicetree bindings documentation for the GE B850v3 LVDS/DP++
> display bridge.
> 
> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> Cc: Martin Donnelly <martin.donnelly@ge.com>
> Cc: Javier Martinez Canillas <javier@dowhile0.org>
> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
> ---
> There was an Acked-by from Rob Herring <robh@kernel.org> for V6, but I changed
> the bindings to use i2c_new_secondary_device() so I removed it from the commit
> message.
> 
>  .../devicetree/bindings/ge/b850v3-lvds-dp.txt      | 39 ++++++++++++++++++++++

Generally, bindings are not organized by vendor. Put in 
bindings/display/bridge/... instead.

>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> 
> diff --git a/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt b/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> new file mode 100644
> index 0000000..1bc6ebf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> @@ -0,0 +1,39 @@
> +Driver for GE B850v3 LVDS/DP++ display bridge
> +
> +Required properties:
> +  - compatible : should be "ge,b850v3-lvds-dp".

Isn't '-lvds-dp' redundant? The part# should be enough.

> +  - reg : should contain the main address which is used to ack the
> +    interrupts and address for edid.
> +  - reg-names : comma separeted list of register names. Valid values

s/separeted/separated/

> +    are "main", and "edid".
> +  - interrupt-parent : phandle of the interrupt controller that services
> +    interrupts to the device
> +  - interrupts : one interrupt should be described here, as in
> +    <0 IRQ_TYPE_LEVEL_HIGH>.
> +  - port : should describe the video signal connection between the host
> +    and the bridge.
> +
> +Example:
> +
> +&mux2_i2c2 {
> +	status = "okay";
> +	clock-frequency = <100000>;
> +
> +	b850v3-lvds-dp-bridge@73  {
> +		compatible = "ge,b850v3-lvds-dp";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		reg = <0x73 0x72>;
> +		reg-names = "main", "edid";
> +
> +		interrupt-parent = <&gpio2>;
> +		interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +
> +		port {
> +			b850v3_dp_bridge_in: endpoint {
> +				remote-endpoint = <&lvds0_out>;
> +			};
> +		};
> +	};
> +};
> -- 
> 2.5.5
>
Peter Senna Jan. 3, 2017, 11:34 p.m. UTC | #2
Hi Rob,

Thank you for the review.

On 03 January, 2017 23:51 CET, Rob Herring <robh@kernel.org> wrote: 
 
> On Sun, Jan 01, 2017 at 09:24:29PM +0100, Peter Senna Tschudin wrote:
> > Devicetree bindings documentation for the GE B850v3 LVDS/DP++
> > display bridge.
> > 
> > Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> > Cc: Martin Donnelly <martin.donnelly@ge.com>
> > Cc: Javier Martinez Canillas <javier@dowhile0.org>
> > Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
> > ---
> > There was an Acked-by from Rob Herring <robh@kernel.org> for V6, but I changed
> > the bindings to use i2c_new_secondary_device() so I removed it from the commit
> > message.
> > 
> >  .../devicetree/bindings/ge/b850v3-lvds-dp.txt      | 39 ++++++++++++++++++++++
> 
> Generally, bindings are not organized by vendor. Put in 
> bindings/display/bridge/... instead.

Will change that.

> 
> >  1 file changed, 39 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt b/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> > new file mode 100644
> > index 0000000..1bc6ebf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> > @@ -0,0 +1,39 @@
> > +Driver for GE B850v3 LVDS/DP++ display bridge
> > +
> > +Required properties:
> > +  - compatible : should be "ge,b850v3-lvds-dp".
> 
> Isn't '-lvds-dp' redundant? The part# should be enough.

b850v3 is the name of the product, this is why the proposed name. What about, b850v3-dp2 dp2 indicating the second DP output?

> 
> > +  - reg : should contain the main address which is used to ack the
> > +    interrupts and address for edid.
> > +  - reg-names : comma separeted list of register names. Valid values
> 
> s/separeted/separated/

argh, sorry for this. Will fix it.

> 
> > +    are "main", and "edid".
> > +  - interrupt-parent : phandle of the interrupt controller that services
> > +    interrupts to the device
> > +  - interrupts : one interrupt should be described here, as in
> > +    <0 IRQ_TYPE_LEVEL_HIGH>.
> > +  - port : should describe the video signal connection between the host
> > +    and the bridge.
> > +
> > +Example:
> > +
> > +&mux2_i2c2 {
> > +	status = "okay";
> > +	clock-frequency = <100000>;
> > +
> > +	b850v3-lvds-dp-bridge@73  {
> > +		compatible = "ge,b850v3-lvds-dp";
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		reg = <0x73 0x72>;
> > +		reg-names = "main", "edid";
> > +
> > +		interrupt-parent = <&gpio2>;
> > +		interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> > +
> > +		port {
> > +			b850v3_dp_bridge_in: endpoint {
> > +				remote-endpoint = <&lvds0_out>;
> > +			};
> > +		};
> > +	};
> > +};
> > -- 
> > 2.5.5
> >
Rob Herring Jan. 4, 2017, 8:39 p.m. UTC | #3
On Tue, Jan 3, 2017 at 5:34 PM, Peter Senna Tschudin
<peter.senna@collabora.co.uk> wrote:
>  Hi Rob,
>
> Thank you for the review.
>
> On 03 January, 2017 23:51 CET, Rob Herring <robh@kernel.org> wrote:
>
>> On Sun, Jan 01, 2017 at 09:24:29PM +0100, Peter Senna Tschudin wrote:
>> > Devicetree bindings documentation for the GE B850v3 LVDS/DP++
>> > display bridge.
>> >
>> > Cc: Martyn Welch <martyn.welch@collabora.co.uk>
>> > Cc: Martin Donnelly <martin.donnelly@ge.com>
>> > Cc: Javier Martinez Canillas <javier@dowhile0.org>
>> > Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
>> > Cc: Rob Herring <robh@kernel.org>
>> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
>> > Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
>> > ---
>> > There was an Acked-by from Rob Herring <robh@kernel.org> for V6, but I changed
>> > the bindings to use i2c_new_secondary_device() so I removed it from the commit
>> > message.
>> >
>> >  .../devicetree/bindings/ge/b850v3-lvds-dp.txt      | 39 ++++++++++++++++++++++
>>
>> Generally, bindings are not organized by vendor. Put in
>> bindings/display/bridge/... instead.
>
> Will change that.
>
>>
>> >  1 file changed, 39 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt b/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
>> > new file mode 100644
>> > index 0000000..1bc6ebf
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
>> > @@ -0,0 +1,39 @@
>> > +Driver for GE B850v3 LVDS/DP++ display bridge
>> > +
>> > +Required properties:
>> > +  - compatible : should be "ge,b850v3-lvds-dp".
>>
>> Isn't '-lvds-dp' redundant? The part# should be enough.
>
> b850v3 is the name of the product, this is why the proposed name. What about, b850v3-dp2 dp2 indicating the second DP output?

Humm, b850v3 is the board name? This node should be the name of the bridge chip.

Rob
Peter Senna Jan. 7, 2017, 1:29 a.m. UTC | #4
On 04 January, 2017 21:39 CET, Rob Herring <robh@kernel.org> wrote: 
 
> On Tue, Jan 3, 2017 at 5:34 PM, Peter Senna Tschudin
> <peter.senna@collabora.co.uk> wrote:
> >  Hi Rob,
> >
> > Thank you for the review.
> >
> > On 03 January, 2017 23:51 CET, Rob Herring <robh@kernel.org> wrote:
> >
> >> On Sun, Jan 01, 2017 at 09:24:29PM +0100, Peter Senna Tschudin wrote:
> >> > Devicetree bindings documentation for the GE B850v3 LVDS/DP++
> >> > display bridge.
> >> >
> >> > Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> >> > Cc: Martin Donnelly <martin.donnelly@ge.com>
> >> > Cc: Javier Martinez Canillas <javier@dowhile0.org>
> >> > Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> >> > Cc: Rob Herring <robh@kernel.org>
> >> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> >> > Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>

> >> > ---
> >> > There was an Acked-by from Rob Herring <robh@kernel.org> for V6, but I changed
> >> > the bindings to use i2c_new_secondary_device() so I removed it from the commit
> >> > message.
> >> >
> >> >  .../devicetree/bindings/ge/b850v3-lvds-dp.txt      | 39 ++++++++++++++++++++++
> >>
> >> Generally, bindings are not organized by vendor. Put in
> >> bindings/display/bridge/... instead.
> >
> > Will change that.
> >
> >>
> >> >  1 file changed, 39 insertions(+)
> >> >  create mode 100644 Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt b/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> >> > new file mode 100644
> >> > index 0000000..1bc6ebf
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> >> > @@ -0,0 +1,39 @@
> >> > +Driver for GE B850v3 LVDS/DP++ display bridge
> >> > +
> >> > +Required properties:
> >> > +  - compatible : should be "ge,b850v3-lvds-dp".
> >>
> >> Isn't '-lvds-dp' redundant? The part# should be enough.
> >
> > b850v3 is the name of the product, this is why the proposed name. What about, b850v3-dp2 dp2 indicating the second DP output?
> 
> Humm, b850v3 is the board name? This node should be the name of the bridge chip.

From the cover letter:

-- // --
There are two physical bridges on the video signal pipeline: a STDP4028(LVDS to
DP) and a STDP2690(DP to DP++).  The hardware and firmware made it complicated
for this binding to comprise two device tree nodes, as the design goal is to
configure both bridges based on the LVDS signal, which leave the driver
powerless to control the video processing pipeline. The two bridges behaves as
a single bridge, and the driver is only needed for telling the host about EDID /
HPD, and for giving the host powers to ack interrupts. The video signal
pipeline is as follows:

  Host -> LVDS|--(STDP4028)--|DP -> DP|--(STDP2690)--|DP++ -> Video output
-- // --
Laurent Pinchart Jan. 10, 2017, 9:04 p.m. UTC | #5
Hi Peter,

On Saturday 07 Jan 2017 01:29:52 Peter Senna Tschudin wrote:
> On 04 January, 2017 21:39 CET, Rob Herring wrote:
> > On Tue, Jan 3, 2017 at 5:34 PM, Peter Senna Tschudin wrote:
> >> On 03 January, 2017 23:51 CET, Rob Herring <robh@kernel.org> wrote:
> >>> On Sun, Jan 01, 2017 at 09:24:29PM +0100, Peter Senna Tschudin wrote:
> >>>> Devicetree bindings documentation for the GE B850v3 LVDS/DP++
> >>>> display bridge.
> >>>> 
> >>>> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> >>>> Cc: Martin Donnelly <martin.donnelly@ge.com>
> >>>> Cc: Javier Martinez Canillas <javier@dowhile0.org>
> >>>> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >>>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> >>>> Cc: Rob Herring <robh@kernel.org>
> >>>> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> >>>> Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
> >>>> ---
> >>>> There was an Acked-by from Rob Herring <robh@kernel.org> for V6, but
> >>>> I changed the bindings to use i2c_new_secondary_device() so I
> >>>> removed it from the commit message.
> >>>> 
> >>>>  .../devicetree/bindings/ge/b850v3-lvds-dp.txt      | 39 ++++++++++++++
> >>> Generally, bindings are not organized by vendor. Put in
> >>> bindings/display/bridge/... instead.
> >> 
> >> Will change that.
> >> 
> >>>>  1 file changed, 39 insertions(+)
> >>>>  create mode 100644
> >>>>  Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> >>>> 
> >>>> diff --git a/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> >>>> b/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt new file
> >>>> mode 100644
> >>>> index 0000000..1bc6ebf
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> >>>> @@ -0,0 +1,39 @@
> >>>> +Driver for GE B850v3 LVDS/DP++ display bridge
> >>>> +
> >>>> +Required properties:
> >>>> +  - compatible : should be "ge,b850v3-lvds-dp".
> >>> 
> >>> Isn't '-lvds-dp' redundant? The part# should be enough.
> >> 
> >> b850v3 is the name of the product, this is why the proposed name. What
> >> about, b850v3-dp2 dp2 indicating the second DP output?
> >
> > Humm, b850v3 is the board name? This node should be the name of the bridge
> > chip.
>
> From the cover letter:
> 
> -- // --
> There are two physical bridges on the video signal pipeline: a STDP4028(LVDS
> to DP) and a STDP2690(DP to DP++).  The hardware and firmware made it
> complicated for this binding to comprise two device tree nodes, as the
> design goal is to configure both bridges based on the LVDS signal, which
> leave the driver powerless to control the video processing pipeline. The
> two bridges behaves as a single bridge, and the driver is only needed for
> telling the host about EDID / HPD, and for giving the host powers to ack
> interrupts. The video signal pipeline is as follows:
> 
>   Host -> LVDS|--(STDP4028)--|DP -> DP|--(STDP2690)--|DP++ -> Video output
> -- // --

You forgot to prefix your patch series with [HACK] ;-)

How about fixing the issues that make the two DT nodes solution difficult ? 
What are they ?
Laurent Pinchart Jan. 10, 2017, 9:06 p.m. UTC | #6
Hi Peter,

Thank you for the patch.

On Sunday 01 Jan 2017 21:24:29 Peter Senna Tschudin wrote:
> Devicetree bindings documentation for the GE B850v3 LVDS/DP++
> display bridge.
> 
> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> Cc: Martin Donnelly <martin.donnelly@ge.com>
> Cc: Javier Martinez Canillas <javier@dowhile0.org>
> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
> ---
> There was an Acked-by from Rob Herring <robh@kernel.org> for V6, but I
> changed the bindings to use i2c_new_secondary_device() so I removed it from
> the commit message.
> 
>  .../devicetree/bindings/ge/b850v3-lvds-dp.txt      | 39 +++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> 
> diff --git a/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> b/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt new file mode
> 100644
> index 0000000..1bc6ebf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> @@ -0,0 +1,39 @@
> +Driver for GE B850v3 LVDS/DP++ display bridge
> +
> +Required properties:
> +  - compatible : should be "ge,b850v3-lvds-dp".
> +  - reg : should contain the main address which is used to ack the
> +    interrupts and address for edid.
> +  - reg-names : comma separeted list of register names. Valid values
> +    are "main", and "edid".
> +  - interrupt-parent : phandle of the interrupt controller that services
> +    interrupts to the device
> +  - interrupts : one interrupt should be described here, as in
> +    <0 IRQ_TYPE_LEVEL_HIGH>.
> +  - port : should describe the video signal connection between the host
> +    and the bridge.

A bridge has, by definition, (at least) one input and (at least) one output. 
You should thus have (at least) two ports.

> +Example:
> +
> +&mux2_i2c2 {
> +	status = "okay";
> +	clock-frequency = <100000>;
> +
> +	b850v3-lvds-dp-bridge@73  {
> +		compatible = "ge,b850v3-lvds-dp";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		reg = <0x73 0x72>;
> +		reg-names = "main", "edid";
> +
> +		interrupt-parent = <&gpio2>;
> +		interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +
> +		port {
> +			b850v3_dp_bridge_in: endpoint {
> +				remote-endpoint = <&lvds0_out>;
> +			};
> +		};
> +	};
> +};
Peter Senna Tschudin Jan. 16, 2017, 8:37 a.m. UTC | #7
On Tue, Jan 10, 2017 at 11:04:58PM +0200, Laurent Pinchart wrote:
> Hi Peter,

Laurent!

> 
> On Saturday 07 Jan 2017 01:29:52 Peter Senna Tschudin wrote:
> > On 04 January, 2017 21:39 CET, Rob Herring wrote:
> > > On Tue, Jan 3, 2017 at 5:34 PM, Peter Senna Tschudin wrote:
> > >> On 03 January, 2017 23:51 CET, Rob Herring <robh@kernel.org> wrote:
> > >>> On Sun, Jan 01, 2017 at 09:24:29PM +0100, Peter Senna Tschudin wrote:
> > >>>> Devicetree bindings documentation for the GE B850v3 LVDS/DP++
> > >>>> display bridge.
> > >>>> 
> > >>>> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> > >>>> Cc: Martin Donnelly <martin.donnelly@ge.com>
> > >>>> Cc: Javier Martinez Canillas <javier@dowhile0.org>
> > >>>> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > >>>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > >>>> Cc: Rob Herring <robh@kernel.org>
> > >>>> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > >>>> Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
> > >>>> ---
> > >>>> There was an Acked-by from Rob Herring <robh@kernel.org> for V6, but
> > >>>> I changed the bindings to use i2c_new_secondary_device() so I
> > >>>> removed it from the commit message.
> > >>>> 
> > >>>>  .../devicetree/bindings/ge/b850v3-lvds-dp.txt      | 39 ++++++++++++++
> > >>> Generally, bindings are not organized by vendor. Put in
> > >>> bindings/display/bridge/... instead.
> > >> 
> > >> Will change that.
> > >> 
> > >>>>  1 file changed, 39 insertions(+)
> > >>>>  create mode 100644
> > >>>>  Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> > >>>> 
> > >>>> diff --git a/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> > >>>> b/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt new file
> > >>>> mode 100644
> > >>>> index 0000000..1bc6ebf
> > >>>> --- /dev/null
> > >>>> +++ b/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> > >>>> @@ -0,0 +1,39 @@
> > >>>> +Driver for GE B850v3 LVDS/DP++ display bridge
> > >>>> +
> > >>>> +Required properties:
> > >>>> +  - compatible : should be "ge,b850v3-lvds-dp".
> > >>> 
> > >>> Isn't '-lvds-dp' redundant? The part# should be enough.
> > >> 
> > >> b850v3 is the name of the product, this is why the proposed name. What
> > >> about, b850v3-dp2 dp2 indicating the second DP output?
> > >
> > > Humm, b850v3 is the board name? This node should be the name of the bridge
> > > chip.
> >
> > From the cover letter:
> > 
> > -- // --
> > There are two physical bridges on the video signal pipeline: a STDP4028(LVDS
> > to DP) and a STDP2690(DP to DP++).  The hardware and firmware made it
> > complicated for this binding to comprise two device tree nodes, as the
> > design goal is to configure both bridges based on the LVDS signal, which
> > leave the driver powerless to control the video processing pipeline. The
> > two bridges behaves as a single bridge, and the driver is only needed for
> > telling the host about EDID / HPD, and for giving the host powers to ack
> > interrupts. The video signal pipeline is as follows:
> > 
> >   Host -> LVDS|--(STDP4028)--|DP -> DP|--(STDP2690)--|DP++ -> Video output
> > -- // --
> 
> You forgot to prefix your patch series with [HACK] ;-)
> 
> How about fixing the issues that make the two DT nodes solution difficult ? 
> What are they ?

The Firmware and the hardware design. Both bridges, with stock firmware,
are fully capable of providig EDID information and handling interrupts.
But on this specific design, with this specific firmware, I need to read
EDID from one bridge, and handle interrupts on the other. Back when I
was starting the development I could not come up with a proper way to
split EDID and interrupts between two bridges in a way that would result
in a fully functional connector. Did I miss something?


> 
> -- 
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart Jan. 18, 2017, 9:10 p.m. UTC | #8
Hi Peter,

On Monday 16 Jan 2017 09:37:11 Peter Senna Tschudin wrote:
> On Tue, Jan 10, 2017 at 11:04:58PM +0200, Laurent Pinchart wrote:
> > On Saturday 07 Jan 2017 01:29:52 Peter Senna Tschudin wrote:
> >> On 04 January, 2017 21:39 CET, Rob Herring wrote:
> >>> On Tue, Jan 3, 2017 at 5:34 PM, Peter Senna Tschudin wrote:
> >>>> On 03 January, 2017 23:51 CET, Rob Herring <robh@kernel.org> wrote:
> >>>>> On Sun, Jan 01, 2017 at 09:24:29PM +0100, Peter Senna Tschudin wrote:
> >>>>>> Devicetree bindings documentation for the GE B850v3 LVDS/DP++
> >>>>>> display bridge.
> >>>>>> 
> >>>>>> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> >>>>>> Cc: Martin Donnelly <martin.donnelly@ge.com>
> >>>>>> Cc: Javier Martinez Canillas <javier@dowhile0.org>
> >>>>>> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >>>>>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> >>>>>> Cc: Rob Herring <robh@kernel.org>
> >>>>>> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> >>>>>> Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
> >>>>>> ---
> >>>>>> There was an Acked-by from Rob Herring <robh@kernel.org> for V6,
> >>>>>> but I changed the bindings to use i2c_new_secondary_device() so I
> >>>>>> removed it from the commit message.
> >>>>>> 
> >>>>>>  .../devicetree/bindings/ge/b850v3-lvds-dp.txt      | 39 +++++++++++
> >>>>> 
> >>>>> Generally, bindings are not organized by vendor. Put in
> >>>>> bindings/display/bridge/... instead.
> >>>> 
> >>>> Will change that.
> >>>> 
> >>>>>>  1 file changed, 39 insertions(+)
> >>>>>>  create mode 100644
> >>>>>>  Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> >>>>>> 
> >>>>>> diff --git
> >>>>>> a/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> >>>>>> b/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt new file
> >>>>>> mode 100644
> >>>>>> index 0000000..1bc6ebf
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
> >>>>>> @@ -0,0 +1,39 @@
> >>>>>> +Driver for GE B850v3 LVDS/DP++ display bridge
> >>>>>> +
> >>>>>> +Required properties:
> >>>>>> +  - compatible : should be "ge,b850v3-lvds-dp".
> >>>>> 
> >>>>> Isn't '-lvds-dp' redundant? The part# should be enough.
> >>>> 
> >>>> b850v3 is the name of the product, this is why the proposed name.
> >>>> What about, b850v3-dp2 dp2 indicating the second DP output?
> >>> 
> >>> Humm, b850v3 is the board name? This node should be the name of the
> >>> bridge chip.
> >> 
> >> From the cover letter:
> >> 
> >> -- // --
> >> There are two physical bridges on the video signal pipeline: a
> >> STDP4028(LVDS to DP) and a STDP2690(DP to DP++).  The hardware and
> >> firmware made it complicated for this binding to comprise two device
> >> tree nodes, as the design goal is to configure both bridges based on
> >> the LVDS signal, which leave the driver powerless to control the video
> >> processing pipeline. The two bridges behaves as a single bridge, and
> >> the driver is only needed for telling the host about EDID / HPD, and
> >> for giving the host powers to ack interrupts. The video signal pipeline
> >> is as follows:
> >>   Host -> LVDS|--(STDP4028)--|DP -> DP|--(STDP2690)--|DP++ -> Video
> >>   output
> >> -- // --
> > 
> > You forgot to prefix your patch series with [HACK] ;-)
> > 
> > How about fixing the issues that make the two DT nodes solution difficult
> > ? What are they ?
> 
> The Firmware and the hardware design. Both bridges, with stock firmware,
> are fully capable of providig EDID information and handling interrupts.
> But on this specific design, with this specific firmware, I need to read
> EDID from one bridge, and handle interrupts on the other.

Which firmware are you talking about ? Firmware running on the bridges, or 
somewhere else ?

> Back when I was starting the development I could not come up with a proper
> way to split EDID and interrupts between two bridges in a way that would
> result in a fully functional connector. Did I miss something?

You didn't, we did :-) I've been telling for quite some time now that we must 
decouple bridges from connectors, and this is another example of why we have 
such a need. Bridges should expose additional functions needed to implement 
connector operations, and the connector should be instantiated by the display 
driver with the help of bridge operations. You could then create a connector 
that relies on one bridge to read the EDID and on the other bridge to handle 
HPD.
Peter Senna Tschudin Jan. 19, 2017, 8:12 a.m. UTC | #9
On Wed, Jan 18, 2017 at 11:10:58PM +0200, Laurent Pinchart wrote:
> Hi Peter,
> 
> On Monday 16 Jan 2017 09:37:11 Peter Senna Tschudin wrote:
> > On Tue, Jan 10, 2017 at 11:04:58PM +0200, Laurent Pinchart wrote:
> > > On Saturday 07 Jan 2017 01:29:52 Peter Senna Tschudin wrote:
> > >> On 04 January, 2017 21:39 CET, Rob Herring wrote:
> > >>> On Tue, Jan 3, 2017 at 5:34 PM, Peter Senna Tschudin wrote:
> > >>>> On 03 January, 2017 23:51 CET, Rob Herring <robh@kernel.org> wrote:
> > >>>>> On Sun, Jan 01, 2017 at 09:24:29PM +0100, Peter Senna Tschudin wrote:
> > >>>>>> Devicetree bindings documentation for the GE B850v3 LVDS/DP++
> > >>>>>> display bridge.
> > >>>>>> 
> > >>>>>> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> > >>>>>> Cc: Martin Donnelly <martin.donnelly@ge.com>
> > >>>>>> Cc: Javier Martinez Canillas <javier@dowhile0.org>
> > >>>>>> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > >>>>>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > >>>>>> Cc: Rob Herring <robh@kernel.org>
> > >>>>>> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > >>>>>> Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
> > >>>>>> ---
> > >>>>>> There was an Acked-by from Rob Herring <robh@kernel.org> for V6,
> > >>>>>> but I changed the bindings to use i2c_new_secondary_device() so I
> > >>>>>> removed it from the commit message.
> > >>>>>> 

[...]

> > >>>>>>  .../devicetree/bindings/ge/b850v3-lvds-dp.txt      | 39 +++++++++++
> > >>>>> 
> > >>>>> Isn't '-lvds-dp' redundant? The part# should be enough.
> > >>>> 
> > >>>> b850v3 is the name of the product, this is why the proposed name.
> > >>>> What about, b850v3-dp2 dp2 indicating the second DP output?
> > >>> 
> > >>> Humm, b850v3 is the board name? This node should be the name of the
> > >>> bridge chip.
> > >> 
> > >> From the cover letter:
> > >> 
> > >> -- // --
> > >> There are two physical bridges on the video signal pipeline: a
> > >> STDP4028(LVDS to DP) and a STDP2690(DP to DP++).  The hardware and
> > >> firmware made it complicated for this binding to comprise two device
> > >> tree nodes, as the design goal is to configure both bridges based on
> > >> the LVDS signal, which leave the driver powerless to control the video
> > >> processing pipeline. The two bridges behaves as a single bridge, and
> > >> the driver is only needed for telling the host about EDID / HPD, and
> > >> for giving the host powers to ack interrupts. The video signal pipeline
> > >> is as follows:
> > >>   Host -> LVDS|--(STDP4028)--|DP -> DP|--(STDP2690)--|DP++ -> Video
> > >>   output
> > >> -- // --
> > > 
> > > You forgot to prefix your patch series with [HACK] ;-)
> > > 
> > > How about fixing the issues that make the two DT nodes solution difficult
> > > ? What are they ?
> > 
> > The Firmware and the hardware design. Both bridges, with stock firmware,
> > are fully capable of providig EDID information and handling interrupts.
> > But on this specific design, with this specific firmware, I need to read
> > EDID from one bridge, and handle interrupts on the other.
> 
> Which firmware are you talking about ? Firmware running on the bridges, or 
> somewhere else ?

Each bridge has it's own external flash containing a binary firmware.
The goal of the firmware is to configure the output end based on the
input end. This is part of what makes handling EDID and HPD challenging.

> 
> > Back when I was starting the development I could not come up with a proper
> > way to split EDID and interrupts between two bridges in a way that would
> > result in a fully functional connector. Did I miss something?
> 
> You didn't, we did :-) I've been telling for quite some time now that we must 
> decouple bridges from connectors, and this is another example of why we have 
> such a need. Bridges should expose additional functions needed to implement 
> connector operations, and the connector should be instantiated by the display 
> driver with the help of bridge operations. You could then create a connector 
> that relies on one bridge to read the EDID and on the other bridge to handle 
> HPD.

Ah thanks. So for now the single DT node approach is acceptable, right?
The problem is that even if the driver is getting better on each
iteration, the single DT node for two chips issue comes back often and I
believe is _the_ issue preventing the driver from getting upstream. V1
was sent ~ 8 months ago...

Can I have some blessing on the single DT node approach for now? I'm one
of the 3 proposed maintainers for the driver, and I'm willing to
maintain the driver on the long run, as is the same with the other two
proposed maintainers. So when the time to split the node in two comes,
we will be around, and willing to do it ourselves.

Thank you!

Peter
> 
> -- 
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart Jan. 19, 2017, 8:17 a.m. UTC | #10
Hi Peter,

On Thursday 19 Jan 2017 09:12:14 Peter Senna Tschudin wrote:
> On Wed, Jan 18, 2017 at 11:10:58PM +0200, Laurent Pinchart wrote:
> > On Monday 16 Jan 2017 09:37:11 Peter Senna Tschudin wrote:
> >> On Tue, Jan 10, 2017 at 11:04:58PM +0200, Laurent Pinchart wrote:
> >>> On Saturday 07 Jan 2017 01:29:52 Peter Senna Tschudin wrote:
> >>>> On 04 January, 2017 21:39 CET, Rob Herring wrote:
> >>>>> On Tue, Jan 3, 2017 at 5:34 PM, Peter Senna Tschudin wrote:
> >>>>>> On 03 January, 2017 23:51 CET, Rob Herring <robh@kernel.org> wrote:
> >>>>>>> On Sun, Jan 01, 2017 at 09:24:29PM +0100, Peter Senna Tschudin 
wrote:
> >>>>>>>> Devicetree bindings documentation for the GE B850v3 LVDS/DP++
> >>>>>>>> display bridge.
> >>>>>>>> 
> >>>>>>>> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> >>>>>>>> Cc: Martin Donnelly <martin.donnelly@ge.com>
> >>>>>>>> Cc: Javier Martinez Canillas <javier@dowhile0.org>
> >>>>>>>> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >>>>>>>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> >>>>>>>> Cc: Rob Herring <robh@kernel.org>
> >>>>>>>> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> >>>>>>>> Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
> >>>>>>>> ---
> >>>>>>>> There was an Acked-by from Rob Herring <robh@kernel.org> for V6,
> >>>>>>>> but I changed the bindings to use i2c_new_secondary_device() so I
> >>>>>>>> removed it from the commit message.
> 
> [...]
> 
> >>>>>>>>  .../devicetree/bindings/ge/b850v3-lvds-dp.txt      | 39 +++++++++
> >>>>>>> 
> >>>>>>> Isn't '-lvds-dp' redundant? The part# should be enough.
> >>>>>> 
> >>>>>> b850v3 is the name of the product, this is why the proposed name.
> >>>>>> What about, b850v3-dp2 dp2 indicating the second DP output?
> >>>>> 
> >>>>> Humm, b850v3 is the board name? This node should be the name of the
> >>>>> bridge chip.
> >>>> 
> >>>> From the cover letter:
> >>>> 
> >>>> -- // --
> >>>> There are two physical bridges on the video signal pipeline: a
> >>>> STDP4028(LVDS to DP) and a STDP2690(DP to DP++).  The hardware and
> >>>> firmware made it complicated for this binding to comprise two device
> >>>> tree nodes, as the design goal is to configure both bridges based on
> >>>> the LVDS signal, which leave the driver powerless to control the
> >>>> video processing pipeline. The two bridges behaves as a single bridge,
> >>>> and the driver is only needed for telling the host about EDID / HPD,
> >>>> and for giving the host powers to ack interrupts. The video signal
> >>>> pipeline
> >>>> 
> >>>> is as follows:
> >>>>   Host -> LVDS|--(STDP4028)--|DP -> DP|--(STDP2690)--|DP++ -> Video
> >>>>   output
> >>>> 
> >>>> -- // --
> >>> 
> >>> You forgot to prefix your patch series with [HACK] ;-)
> >>> 
> >>> How about fixing the issues that make the two DT nodes solution
> >>> difficult ? What are they ?
> >> 
> >> The Firmware and the hardware design. Both bridges, with stock firmware,
> >> are fully capable of providig EDID information and handling interrupts.
> >> But on this specific design, with this specific firmware, I need to read
> >> EDID from one bridge, and handle interrupts on the other.
> > 
> > Which firmware are you talking about ? Firmware running on the bridges, or
> > somewhere else ?
> 
> Each bridge has it's own external flash containing a binary firmware.
> The goal of the firmware is to configure the output end based on the
> input end. This is part of what makes handling EDID and HPD challenging.
> 
> >> Back when I was starting the development I could not come up with a
> >> proper way to split EDID and interrupts between two bridges in a way
> >> that would result in a fully functional connector. Did I miss something?
> > 
> > You didn't, we did :-) I've been telling for quite some time now that we
> > must decouple bridges from connectors, and this is another example of why
> > we have such a need. Bridges should expose additional functions needed to
> > implement connector operations, and the connector should be instantiated
> > by the display driver with the help of bridge operations. You could then
> > create a connector that relies on one bridge to read the EDID and on the
> > other bridge to handle HPD.
> 
> Ah thanks. So for now the single DT node approach is acceptable, right?
> The problem is that even if the driver is getting better on each
> iteration, the single DT node for two chips issue comes back often and I
> believe is _the_ issue preventing the driver from getting upstream. V1
> was sent ~ 8 months ago...
> 
> Can I have some blessing on the single DT node approach for now?

With the "DT as an ABI" approach, I'm afraid not. Temporary hacks are 
acceptable on the driver side, but you need two nodes in DT.

> I'm one of the 3 proposed maintainers for the driver, and I'm willing to
> maintain the driver on the long run, as is the same with the other two
> proposed maintainers. So when the time to split the node in two comes,
> we will be around, and willing to do it ourselves.

How about putting that team of 3 maintainers to work on fixing the problem in 
the bridge API ? :-)
Peter Senna Tschudin Jan. 19, 2017, 9:25 a.m. UTC | #11
On Thu, Jan 19, 2017 at 10:17:45AM +0200, Laurent Pinchart wrote:
> Hi Peter,
> 
> On Thursday 19 Jan 2017 09:12:14 Peter Senna Tschudin wrote:
> > On Wed, Jan 18, 2017 at 11:10:58PM +0200, Laurent Pinchart wrote:
> > > On Monday 16 Jan 2017 09:37:11 Peter Senna Tschudin wrote:
> > >> On Tue, Jan 10, 2017 at 11:04:58PM +0200, Laurent Pinchart wrote:
> > >>> On Saturday 07 Jan 2017 01:29:52 Peter Senna Tschudin wrote:
> > >>>> On 04 January, 2017 21:39 CET, Rob Herring wrote:
> > >>>>> On Tue, Jan 3, 2017 at 5:34 PM, Peter Senna Tschudin wrote:
> > >>>>>> On 03 January, 2017 23:51 CET, Rob Herring <robh@kernel.org> wrote:
> > >>>>>>> On Sun, Jan 01, 2017 at 09:24:29PM +0100, Peter Senna Tschudin 
> wrote:
> > >>>>>>>> Devicetree bindings documentation for the GE B850v3 LVDS/DP++
> > >>>>>>>> display bridge.
> > >>>>>>>> 
> > >>>>>>>> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> > >>>>>>>> Cc: Martin Donnelly <martin.donnelly@ge.com>
> > >>>>>>>> Cc: Javier Martinez Canillas <javier@dowhile0.org>
> > >>>>>>>> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > >>>>>>>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > >>>>>>>> Cc: Rob Herring <robh@kernel.org>
> > >>>>>>>> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > >>>>>>>> Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
> > >>>>>>>> ---
> > >>>>>>>> There was an Acked-by from Rob Herring <robh@kernel.org> for V6,
> > >>>>>>>> but I changed the bindings to use i2c_new_secondary_device() so I
> > >>>>>>>> removed it from the commit message.
> > 
> > [...]
> > 
> > >>>>>>>>  .../devicetree/bindings/ge/b850v3-lvds-dp.txt      | 39 +++++++++
> > >>>>>>> 
> > >>>>>>> Isn't '-lvds-dp' redundant? The part# should be enough.
> > >>>>>> 
> > >>>>>> b850v3 is the name of the product, this is why the proposed name.
> > >>>>>> What about, b850v3-dp2 dp2 indicating the second DP output?
> > >>>>> 
> > >>>>> Humm, b850v3 is the board name? This node should be the name of the
> > >>>>> bridge chip.
> > >>>> 
> > >>>> From the cover letter:
> > >>>> 
> > >>>> -- // --
> > >>>> There are two physical bridges on the video signal pipeline: a
> > >>>> STDP4028(LVDS to DP) and a STDP2690(DP to DP++).  The hardware and
> > >>>> firmware made it complicated for this binding to comprise two device
> > >>>> tree nodes, as the design goal is to configure both bridges based on
> > >>>> the LVDS signal, which leave the driver powerless to control the
> > >>>> video processing pipeline. The two bridges behaves as a single bridge,
> > >>>> and the driver is only needed for telling the host about EDID / HPD,
> > >>>> and for giving the host powers to ack interrupts. The video signal
> > >>>> pipeline
> > >>>> 
> > >>>> is as follows:
> > >>>>   Host -> LVDS|--(STDP4028)--|DP -> DP|--(STDP2690)--|DP++ -> Video
> > >>>>   output
> > >>>> 
> > >>>> -- // --
> > >>> 
> > >>> You forgot to prefix your patch series with [HACK] ;-)
> > >>> 
> > >>> How about fixing the issues that make the two DT nodes solution
> > >>> difficult ? What are they ?
> > >> 
> > >> The Firmware and the hardware design. Both bridges, with stock firmware,
> > >> are fully capable of providig EDID information and handling interrupts.
> > >> But on this specific design, with this specific firmware, I need to read
> > >> EDID from one bridge, and handle interrupts on the other.
> > > 
> > > Which firmware are you talking about ? Firmware running on the bridges, or
> > > somewhere else ?
> > 
> > Each bridge has it's own external flash containing a binary firmware.
> > The goal of the firmware is to configure the output end based on the
> > input end. This is part of what makes handling EDID and HPD challenging.
> > 
> > >> Back when I was starting the development I could not come up with a
> > >> proper way to split EDID and interrupts between two bridges in a way
> > >> that would result in a fully functional connector. Did I miss something?
> > > 
> > > You didn't, we did :-) I've been telling for quite some time now that we
> > > must decouple bridges from connectors, and this is another example of why
> > > we have such a need. Bridges should expose additional functions needed to
> > > implement connector operations, and the connector should be instantiated
> > > by the display driver with the help of bridge operations. You could then
> > > create a connector that relies on one bridge to read the EDID and on the
> > > other bridge to handle HPD.
> > 
> > Ah thanks. So for now the single DT node approach is acceptable, right?
> > The problem is that even if the driver is getting better on each
> > iteration, the single DT node for two chips issue comes back often and I
> > believe is _the_ issue preventing the driver from getting upstream. V1
> > was sent ~ 8 months ago...
> > 
> > Can I have some blessing on the single DT node approach for now?
> 
> With the "DT as an ABI" approach, I'm afraid not. Temporary hacks are 
> acceptable on the driver side, but you need two nodes in DT.

So can I make two node DT "in the right way" and work around current
connectors vs. bridge limitations on the driver side? This seems to be
doable.

Then I could fix bridge API, with my own driver and update API clients
affected by the change...

> 
> > I'm one of the 3 proposed maintainers for the driver, and I'm willing to
> > maintain the driver on the long run, as is the same with the other two
> > proposed maintainers. So when the time to split the node in two comes,
> > we will be around, and willing to do it ourselves.
> 
> How about putting that team of 3 maintainers to work on fixing the problem in 
> the bridge API ? :-)

Guess you would be a good lawyer! My point was not exactly that we could
work in parallel. Point was that there is redundancy in case one or two
of us loose interest. But nice try! :-)

Chances of having resources to fix bridge API and clients were better 6
months ago, but let me see what I can get.  Last blocking issue was the
migration to atomic, now this. I'm going to need to answer what the next
blocking issue is going to be.

Actually in these ~8 months one bit of the required changes was
accepted: dc80d7038883, but this was generic and not related to our
specific use case.

Thanks!

Peter

> 
> -- 
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart Jan. 19, 2017, 11:49 a.m. UTC | #12
Hi Peter,

On Thursday 19 Jan 2017 10:25:32 Peter Senna Tschudin wrote:
> On Thu, Jan 19, 2017 at 10:17:45AM +0200, Laurent Pinchart wrote:
> > On Thursday 19 Jan 2017 09:12:14 Peter Senna Tschudin wrote:
> >> On Wed, Jan 18, 2017 at 11:10:58PM +0200, Laurent Pinchart wrote:
> >>> On Monday 16 Jan 2017 09:37:11 Peter Senna Tschudin wrote:
> >>>> On Tue, Jan 10, 2017 at 11:04:58PM +0200, Laurent Pinchart wrote:
> >>>>> On Saturday 07 Jan 2017 01:29:52 Peter Senna Tschudin wrote:
> >>>>>> On 04 January, 2017 21:39 CET, Rob Herring wrote:
> >>>>>>> On Tue, Jan 3, 2017 at 5:34 PM, Peter Senna Tschudin wrote:
> >>>>>>>> On 03 January, 2017 23:51 CET, Rob Herring <robh@kernel.org> wrote:
> >>>>>>>>> On Sun, Jan 01, 2017 at 09:24:29PM +0100, Peter Senna Tschudin 
> > wrote:
> >>>>>>>>>> Devicetree bindings documentation for the GE B850v3 LVDS/DP++
> >>>>>>>>>> display bridge.
> >>>>>>>>>> 
> >>>>>>>>>> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> >>>>>>>>>> Cc: Martin Donnelly <martin.donnelly@ge.com>
> >>>>>>>>>> Cc: Javier Martinez Canillas <javier@dowhile0.org>
> >>>>>>>>>> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >>>>>>>>>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> >>>>>>>>>> Cc: Rob Herring <robh@kernel.org>
> >>>>>>>>>> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> >>>>>>>>>> Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
> >>>>>>>>> ---
> >>>>>>>>>> There was an Acked-by from Rob Herring <robh@kernel.org> for
> >>>>>>>>>> V6, but I changed the bindings to use i2c_new_secondary_device()
> >>>>>>>>>> so I removed it from the commit message.
> >> 
> >> [...]
> >> 
> >>>>>>>>>> .../devicetree/bindings/ge/b850v3-lvds-dp.txt      | 39 +++++++++
> >>>>>>>>> 
> >>>>>>>>> Isn't '-lvds-dp' redundant? The part# should be enough.
> >>>>>>>> 
> >>>>>>>> b850v3 is the name of the product, this is why the proposed name.
> >>>>>>>> What about, b850v3-dp2 dp2 indicating the second DP output?
> >>>>>>> 
> >>>>>> Humm, b850v3 is the board name? This node should be the name of
> >>>>>>> the bridge chip.
> >>>>>> 
> >>>>>> From the cover letter:
> >>>>>> 
> >>>>>> -- // --
> >>>>>> There are two physical bridges on the video signal pipeline: a
> >>>>>> STDP4028(LVDS to DP) and a STDP2690(DP to DP++).  The hardware and
> >>>>>> firmware made it complicated for this binding to comprise two
> >>>>>> device tree nodes, as the design goal is to configure both bridges
> >>>>>> based on the LVDS signal, which leave the driver powerless to control
> >>>>>> the video processing pipeline. The two bridges behaves as a single
> >>>>>> bridge, and the driver is only needed for telling the host about EDID
> >>>>>> / HPD, and for giving the host powers to ack interrupts. The video
> >>>>>> signal pipeline
> >>>>>> 
> >>>>>> is as follows:
> >>>>>>   Host -> LVDS|--(STDP4028)--|DP -> DP|--(STDP2690)--|DP++ -> Video
> >>>>>>   output
> >>>>>> 
> >>>>>> -- // --
> >>>>> 
> >>>>> You forgot to prefix your patch series with [HACK] ;-)
> >>>>> 
> >>>>> How about fixing the issues that make the two DT nodes solution
> >>>>> difficult ? What are they ?
> >>>> 
> >>>> The Firmware and the hardware design. Both bridges, with stock
> >>>> firmware, are fully capable of providig EDID information and handling
> >>>> interrupts. But on this specific design, with this specific firmware, I
> >>>> need to read EDID from one bridge, and handle interrupts on the other.
> >>> 
> >>> Which firmware are you talking about ? Firmware running on the
> >>> bridges, or somewhere else ?
> >> 
> >> Each bridge has it's own external flash containing a binary firmware.
> >> The goal of the firmware is to configure the output end based on the
> >> input end. This is part of what makes handling EDID and HPD challenging.
> >> 
> >>>> Back when I was starting the development I could not come up with a
> >>>> proper way to split EDID and interrupts between two bridges in a way
> >>>> that would result in a fully functional connector. Did I miss
> >>>> something?
> >>> 
> >>> You didn't, we did :-) I've been telling for quite some time now that
> >>> we must decouple bridges from connectors, and this is another example of
> >>> why we have such a need. Bridges should expose additional functions
> >>> needed to implement connector operations, and the connector should be
> >>> instantiated by the display driver with the help of bridge operations.
> >>> You could then create a connector that relies on one bridge to read the
> >>> EDID and on the other bridge to handle HPD.
> >> 
> >> Ah thanks. So for now the single DT node approach is acceptable, right?
> >> The problem is that even if the driver is getting better on each
> >> iteration, the single DT node for two chips issue comes back often and I
> >> believe is _the_ issue preventing the driver from getting upstream. V1
> >> was sent ~ 8 months ago...
> >> 
> >> Can I have some blessing on the single DT node approach for now?
> > 
> > With the "DT as an ABI" approach, I'm afraid not. Temporary hacks are
> > acceptable on the driver side, but you need two nodes in DT.
> 
> So can I make two node DT "in the right way" and work around current
> connectors vs. bridge limitations on the driver side? This seems to be
> doable.
> 
> Then I could fix bridge API, with my own driver and update API clients
> affected by the change...

I'm willing to discuss that as long as the DT bindings are correct, yes.

> >> I'm one of the 3 proposed maintainers for the driver, and I'm willing to
> >> maintain the driver on the long run, as is the same with the other two
> >> proposed maintainers. So when the time to split the node in two comes,
> >> we will be around, and willing to do it ourselves.
> > 
> > How about putting that team of 3 maintainers to work on fixing the problem
> > in the bridge API ? :-)
> 
> Guess you would be a good lawyer! My point was not exactly that we could
> work in parallel. Point was that there is redundancy in case one or two
> of us loose interest. But nice try! :-)
> 
> Chances of having resources to fix bridge API and clients were better 6
> months ago, but let me see what I can get.  Last blocking issue was the
> migration to atomic, now this. I'm going to need to answer what the next
> blocking issue is going to be.
> 
> Actually in these ~8 months one bit of the required changes was
> accepted: dc80d7038883, but this was generic and not related to our
> specific use case.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt b/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
new file mode 100644
index 0000000..1bc6ebf
--- /dev/null
+++ b/Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
@@ -0,0 +1,39 @@ 
+Driver for GE B850v3 LVDS/DP++ display bridge
+
+Required properties:
+  - compatible : should be "ge,b850v3-lvds-dp".
+  - reg : should contain the main address which is used to ack the
+    interrupts and address for edid.
+  - reg-names : comma separeted list of register names. Valid values
+    are "main", and "edid".
+  - interrupt-parent : phandle of the interrupt controller that services
+    interrupts to the device
+  - interrupts : one interrupt should be described here, as in
+    <0 IRQ_TYPE_LEVEL_HIGH>.
+  - port : should describe the video signal connection between the host
+    and the bridge.
+
+Example:
+
+&mux2_i2c2 {
+	status = "okay";
+	clock-frequency = <100000>;
+
+	b850v3-lvds-dp-bridge@73  {
+		compatible = "ge,b850v3-lvds-dp";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		reg = <0x73 0x72>;
+		reg-names = "main", "edid";
+
+		interrupt-parent = <&gpio2>;
+		interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+
+		port {
+			b850v3_dp_bridge_in: endpoint {
+				remote-endpoint = <&lvds0_out>;
+			};
+		};
+	};
+};