[PATCH V7 0/4] Add driver for GE B850v3 LVDS/DP++ Bridge

[PATCH V7 0/4] Add driver for GE B850v3 LVDS/DP++ Bridge

[Peter Senna Tschudin]

The series adds a driver that creates a drm_bridge and a drm_connector for the
LVDS to DP++ display bridge of the GE B850v3.

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

This series depends on commit dc80d7038883 ("drm/imx-ldb: Add support to
drm-bridge") which is already on linux-next.

The patches from the series:
 [1/4] Devicetree documentation for the GE B850v3 LVDS/DP++ Bridge

 [2/4] Update the MAINTAINERS file

 [3/4] Add the driver, make changes to Kconfig and Makefile

 [4/4] Make the changes to the B850v3 dts file to enable the GE B850v3
       LVDS/DP++ Bridge.

Tested on next20161224.

Changes from V6:
 - Removed check for pixel clock as the bridge supports up to 330Mhz while the
   host is limited to 264 MHz in very specific conditions.
 - Added a second mutex to avoid concurrency issues while acking interrupts
   from threaded interrupt handlers.
 - Renamed the edid mutex to be more descriptive.
 - Added a .detach() function that disables the interrupts.
 - Adopted i2c_new_secondary_device() and updated the dts and documentation to
   match the new method.
 - Removed useless test to drm_bridge_add()
 - Did some refactoring around devm_request_threaded_irq()
 - Added a missing call to i2c_unregister_device() on the i2c_driver.remove()
   function.

Changes from V5:
 - Change to MAINTAINERS in a separate patch
 - Reworked interrupt handler initialization
 - Removed useless calls to: drm_connector_register(),
   drm_helper_hpd_irq_event(), and drm_bridge_enable()

Changes from V4:
 - Renamed the i2c_driver.name from "ge,b850v3-lvds-dp" to "b850v3-lvds-dp" to
   remove the comma from the driver name

Changes from V3:
 - Removed the patch that was configuring the mapping between IPUs and external
   displays on the dts file

Peter Senna Tschudin (4):
  Documentation/devicetree/bindings: b850v3_lvds_dp
  MAINTAINERS: Add entry for GE B850v3 LVDS/DP++ Bridge
  drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge
  dts/imx6q-b850v3: Use GE B850v3 LVDS/DP++ Bridge

 .../devicetree/bindings/ge/b850v3-lvds-dp.txt      |  39 +++
 MAINTAINERS                                        |   8 +
 arch/arm/boot/dts/imx6q-b850v3.dts                 |  30 ++
 drivers/gpu/drm/bridge/Kconfig                     |  11 +
 drivers/gpu/drm/bridge/Makefile                    |   1 +
 drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c         | 384 +++++++++++++++++++++
 6 files changed, 473 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ge/b850v3-lvds-dp.txt
 create mode 100644 drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c

-- 
2.5.5

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

[Peter Senna Tschudin]

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.
+
+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

Re: [PATCH V7 1/4] Documentation/devicetree/bindings: b850v3_lvds_dp

[Rob Herring]

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
> 

Re: [PATCH V7 1/4] Documentation/devicetree/bindings: b850v3_lvds_dp

[Peter Senna Tschudin]

 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
> > 
 
 
 
 

Re: [PATCH V7 1/4] Documentation/devicetree/bindings: b850v3_lvds_dp

[Rob Herring]

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

Re: [PATCH V7 1/4] Documentation/devicetree/bindings: b850v3_lvds_dp

[Peter Senna Tschudin]

 
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
-- // --

Re: [PATCH V7 1/4] Documentation/devicetree/bindings: b850v3_lvds_dp

[Laurent Pinchart]

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 ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH V7 1/4] Documentation/devicetree/bindings: b850v3_lvds_dp

[Peter Senna Tschudin]

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
> 

Re: [PATCH V7 1/4] Documentation/devicetree/bindings: b850v3_lvds_dp

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH V7 1/4] Documentation/devicetree/bindings: b850v3_lvds_dp

[Peter Senna Tschudin]

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
> 

Re: [PATCH V7 1/4] Documentation/devicetree/bindings: b850v3_lvds_dp

[Laurent Pinchart]

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 ? :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH V7 1/4] Documentation/devicetree/bindings: b850v3_lvds_dp

[Peter Senna Tschudin]

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
> 

Re: [PATCH V7 1/4] Documentation/devicetree/bindings: b850v3_lvds_dp

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH V7 1/4] Documentation/devicetree/bindings: b850v3_lvds_dp

[Laurent Pinchart]

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>;
> +			};
> +		};
> +	};
> +};

-- 
Regards,

Laurent Pinchart

[PATCH V7 2/4] MAINTAINERS: Add entry for GE B850v3 LVDS/DP++ Bridge

[Peter Senna Tschudin]

Update the MAINTAINERS file for the GE B850v3 LVDS/DP++ Bridge.

Cc: Martyn Welch <martyn.welch@collabora.co.uk>
Cc: Martin Donnelly <martin.donnelly@ge.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
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>
CC: David Airlie <airlied@linux.ie>
CC: Thierry Reding <treding@nvidia.com>
CC: Thierry Reding <thierry.reding@gmail.com>
CC: Archit Taneja <architt@codeaurora.org>
Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index fdd9d5e3..1d3f17a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5371,6 +5371,14 @@ W:	https://linuxtv.org
 S:	Maintained
 F:	drivers/media/radio/radio-gemtek*
 
+GENERAL ELECTRIC B850V3 LVDS/DP++ BRIDGE
+M:	Peter Senna Tschudin <peter.senna@collabora.com>
+M:	Martin Donnelly <martin.donnelly@ge.com>
+M:	Martyn Welch <martyn.welch@collabora.co.uk>
+S:	Maintained
+F:	drivers/gpu/drm/bridge/ge_b850v3_dp2.c
+F:	Documentation/devicetree/bindings/ge/b850v3_dp2_bridge.txt
+
 GENERIC GPIO I2C DRIVER
 M:	Haavard Skinnemoen <hskinnemoen@gmail.com>
 S:	Supported
-- 
2.5.5

[PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Peter Senna Tschudin]

Add a driver that create a drm_bridge and a drm_connector for the LVDS
to DP++ display bridge of the GE B850v3.

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

Cc: Martyn Welch <martyn.welch@collabora.co.uk>
Cc: Martin Donnelly <martin.donnelly@ge.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
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>
CC: David Airlie <airlied@linux.ie>
CC: Thierry Reding <treding@nvidia.com>
CC: Thierry Reding <thierry.reding@gmail.com>
CC: Archit Taneja <architt@codeaurora.org>
Reviewed-by: Enric Balletbo <enric.balletbo@collabora.com>
Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
---
 drivers/gpu/drm/bridge/Kconfig             |  11 +
 drivers/gpu/drm/bridge/Makefile            |   1 +
 drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c | 384 +++++++++++++++++++++++++++++
 3 files changed, 396 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index eb8688e..e3e1f3b 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -48,6 +48,17 @@ config DRM_DW_HDMI_I2S_AUDIO
 	  Support the I2S Audio interface which is part of the Synopsis
 	  Designware HDMI block.
 
+config DRM_GE_B850V3_LVDS_DP
+	tristate "GE B850v3 LVDS to DP++ display bridge"
+	depends on OF
+	select DRM_KMS_HELPER
+	select DRM_PANEL
+	---help---
+          This is a driver for the display bridge of
+          GE B850v3 that convert dual channel LVDS
+          to DP++. This is used with the i.MX6 imx-ldb
+          driver.
+
 config DRM_NXP_PTN3460
 	tristate "NXP PTN3460 DP/LVDS bridge"
 	depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 2e83a785..886d0fd 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
 obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o
 obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o
 obj-$(CONFIG_DRM_DW_HDMI_I2S_AUDIO) += dw-hdmi-i2s-audio.o
+obj-$(CONFIG_DRM_GE_B850V3_LVDS_DP) += ge_b850v3_lvds_dp.o
 obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
 obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
 obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
diff --git a/drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c b/drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c
new file mode 100644
index 0000000..4574f6e
--- /dev/null
+++ b/drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c
@@ -0,0 +1,384 @@
+/*
+ * Driver for GE B850v3 DP display bridge
+
+ * Copyright (c) 2016, Collabora Ltd.
+ * Copyright (c) 2016, General Electric Company
+
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+ * This driver creates a drm_bridge and a drm_connector for the LVDS to DP++
+ * display bridge of the GE B850v3. There are two physical bridges on the video
+ * signal pipeline: a STDP4028(LVDS to DP) and a STDP2690(DP to DP++). However
+ * the physical bridges are automatically configured by the input video signal,
+ * and the driver has no access to the video processing pipeline. The driver is
+ * only needed to read EDID from the STDP2690 and to handle HPD events from the
+ * STDP4028. The driver communicates with both bridges over i2c. The video
+ * signal pipeline is as follows:
+ *
+ *   Host -> LVDS|--(STDP4028)--|DP -> DP|--(STDP2690)--|DP++ -> Video output
+ *
+ */
+
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_edid.h>
+#include <drm/drmP.h>
+
+#define DEFAULT_EDID_REG 0x72
+#define DEFAULT_EDID_REG_NAME "edid"
+
+#define EDID_EXT_BLOCK_CNT 0x7E
+
+#define STDP4028_IRQ_OUT_CONF_REG 0x02
+#define STDP4028_DPTX_IRQ_EN_REG 0x3C
+#define STDP4028_DPTX_IRQ_STS_REG 0x3D
+#define STDP4028_DPTX_STS_REG 0x3E
+
+#define STDP4028_DPTX_DP_IRQ_EN 0x1000
+
+#define STDP4028_DPTX_HOTPLUG_IRQ_EN 0x0400
+#define STDP4028_DPTX_LINK_CH_IRQ_EN 0x2000
+#define STDP4028_DPTX_IRQ_CONFIG \
+		(STDP4028_DPTX_LINK_CH_IRQ_EN | STDP4028_DPTX_HOTPLUG_IRQ_EN)
+
+#define STDP4028_DPTX_HOTPLUG_STS 0x0200
+#define STDP4028_DPTX_LINK_STS 0x1000
+#define STDP4028_CON_STATE_CONNECTED \
+		(STDP4028_DPTX_HOTPLUG_STS | STDP4028_DPTX_LINK_STS)
+
+#define STDP4028_DPTX_HOTPLUG_CH_STS 0x0400
+#define STDP4028_DPTX_LINK_CH_STS 0x2000
+#define STDP4028_DPTX_IRQ_CLEAR \
+		(STDP4028_DPTX_LINK_CH_STS | STDP4028_DPTX_HOTPLUG_CH_STS)
+
+struct ge_b850v3_lvds_dp {
+	struct drm_connector connector;
+	struct drm_bridge bridge;
+	struct i2c_client *ge_b850v3_lvds_dp_i2c;
+	struct i2c_client *edid_i2c;
+	struct edid *edid;
+	struct mutex edid_mutex;
+	struct mutex irq_reg_mutex;
+};
+
+static inline struct ge_b850v3_lvds_dp *
+		bridge_to_ge_b850v3_lvds_dp(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct ge_b850v3_lvds_dp, bridge);
+}
+
+static inline struct ge_b850v3_lvds_dp *
+		connector_to_ge_b850v3_lvds_dp(struct drm_connector *connector)
+{
+	return container_of(connector, struct ge_b850v3_lvds_dp, connector);
+}
+
+u8 *stdp2690_get_edid(struct i2c_client *client)
+{
+	struct i2c_adapter *adapter = client->adapter;
+	unsigned char start = 0x00;
+	unsigned int total_size;
+	u8 *block = kmalloc(EDID_LENGTH, GFP_KERNEL);
+
+	struct i2c_msg msgs[] = {
+		{
+			.addr	= client->addr,
+			.flags	= 0,
+			.len	= 1,
+			.buf	= &start,
+		}, {
+			.addr	= client->addr,
+			.flags	= I2C_M_RD,
+			.len	= EDID_LENGTH,
+			.buf	= block,
+		}
+	};
+
+	if (!block)
+		return NULL;
+
+	if (i2c_transfer(adapter, msgs, 2) != 2) {
+		DRM_ERROR("Unable to read EDID.\n");
+		goto err;
+	}
+
+	if (!drm_edid_block_valid(block, 0, false, NULL)) {
+		DRM_ERROR("Invalid EDID block\n");
+		goto err;
+	}
+
+	total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
+	if (total_size > EDID_LENGTH) {
+		kfree(block);
+		block = kmalloc(total_size, GFP_KERNEL);
+		if (!block)
+			return NULL;
+
+		/* Yes, read the entire buffer, and do not skip the first
+		 * EDID_LENGTH bytes.
+		 */
+		start = 0x00;
+		msgs[1].len = total_size;
+		msgs[1].buf = block;
+
+		if (i2c_transfer(adapter, msgs, 2) != 2) {
+			DRM_ERROR("Unable to read EDID extension blocks.\n");
+			goto err;
+		}
+	}
+
+	return block;
+
+err:
+	kfree(block);
+	return NULL;
+}
+
+static int ge_b850v3_lvds_dp_get_modes(struct drm_connector *connector)
+{
+	struct ge_b850v3_lvds_dp *ptn_bridge;
+	struct i2c_client *client;
+	int num_modes = 0;
+
+	ptn_bridge = connector_to_ge_b850v3_lvds_dp(connector);
+	client = ptn_bridge->edid_i2c;
+
+	mutex_lock(&ptn_bridge->edid_mutex);
+
+	kfree(ptn_bridge->edid);
+	ptn_bridge->edid = (struct edid *) stdp2690_get_edid(client);
+
+	if (ptn_bridge->edid) {
+		drm_mode_connector_update_edid_property(connector,
+				ptn_bridge->edid);
+		num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
+	}
+
+	mutex_unlock(&ptn_bridge->edid_mutex);
+
+	return num_modes;
+}
+
+
+static enum drm_mode_status ge_b850v3_lvds_dp_mode_valid(
+		struct drm_connector *connector, struct drm_display_mode *mode)
+{
+	return MODE_OK;
+}
+
+static const struct
+drm_connector_helper_funcs ge_b850v3_lvds_dp_connector_helper_funcs = {
+	.get_modes = ge_b850v3_lvds_dp_get_modes,
+	.mode_valid = ge_b850v3_lvds_dp_mode_valid,
+};
+
+static enum drm_connector_status ge_b850v3_lvds_dp_detect(
+		struct drm_connector *connector, bool force)
+{
+	struct ge_b850v3_lvds_dp *ptn_bridge =
+			connector_to_ge_b850v3_lvds_dp(connector);
+	struct i2c_client *ge_b850v3_lvds_dp_i2c =
+			ptn_bridge->ge_b850v3_lvds_dp_i2c;
+	s32 link_state;
+
+	link_state = i2c_smbus_read_word_data(ge_b850v3_lvds_dp_i2c,
+			STDP4028_DPTX_STS_REG);
+
+	if (link_state == STDP4028_CON_STATE_CONNECTED)
+		return connector_status_connected;
+
+	if (link_state == 0)
+		return connector_status_disconnected;
+
+	return connector_status_unknown;
+}
+
+static const struct drm_connector_funcs ge_b850v3_lvds_dp_connector_funcs = {
+	.dpms = drm_atomic_helper_connector_dpms,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.detect = ge_b850v3_lvds_dp_detect,
+	.destroy = drm_connector_cleanup,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static irqreturn_t ge_b850v3_lvds_dp_irq_handler(int irq, void *dev_id)
+{
+	struct ge_b850v3_lvds_dp *ptn_bridge = dev_id;
+	struct i2c_client *ge_b850v3_lvds_dp_i2c
+			= ptn_bridge->ge_b850v3_lvds_dp_i2c;
+
+	mutex_lock(&ptn_bridge->irq_reg_mutex);
+
+	i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
+			STDP4028_DPTX_IRQ_STS_REG, STDP4028_DPTX_IRQ_CLEAR);
+
+	mutex_unlock(&ptn_bridge->irq_reg_mutex);
+
+	if (ptn_bridge->connector.dev)
+		drm_kms_helper_hotplug_event(ptn_bridge->connector.dev);
+
+	return IRQ_HANDLED;
+}
+
+static int ge_b850v3_lvds_dp_attach(struct drm_bridge *bridge)
+{
+	struct ge_b850v3_lvds_dp *ptn_bridge
+			= bridge_to_ge_b850v3_lvds_dp(bridge);
+	struct drm_connector *connector = &ptn_bridge->connector;
+	struct i2c_client *ge_b850v3_lvds_dp_i2c
+			= ptn_bridge->ge_b850v3_lvds_dp_i2c;
+	int ret;
+
+	if (!bridge->encoder) {
+		DRM_ERROR("Parent encoder object not found");
+		return -ENODEV;
+	}
+
+	connector->polled = DRM_CONNECTOR_POLL_HPD;
+
+	drm_connector_helper_add(connector,
+			&ge_b850v3_lvds_dp_connector_helper_funcs);
+
+	ret = drm_connector_init(bridge->dev, connector,
+			&ge_b850v3_lvds_dp_connector_funcs,
+			DRM_MODE_CONNECTOR_DisplayPort);
+	if (ret) {
+		DRM_ERROR("Failed to initialize connector with drm\n");
+		return ret;
+	}
+
+	ret = drm_mode_connector_attach_encoder(connector, bridge->encoder);
+	if (ret)
+		return ret;
+
+	drm_helper_hpd_irq_event(connector->dev);
+
+	/* Configures the bridge to re-enable interrupts after each ack. */
+	i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
+			STDP4028_IRQ_OUT_CONF_REG, STDP4028_DPTX_DP_IRQ_EN);
+
+	/* Enable interrupts */
+	i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
+			STDP4028_DPTX_IRQ_EN_REG, STDP4028_DPTX_IRQ_CONFIG);
+
+	return 0;
+}
+
+static void ge_b850v3_lvds_dp_detach(struct drm_bridge *bridge)
+{
+	struct ge_b850v3_lvds_dp *ptn_bridge
+			= bridge_to_ge_b850v3_lvds_dp(bridge);
+	struct i2c_client *ge_b850v3_lvds_dp_i2c
+			= ptn_bridge->ge_b850v3_lvds_dp_i2c;
+
+	/* Disable interrupts */
+	i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
+			STDP4028_DPTX_IRQ_EN_REG, ~STDP4028_DPTX_IRQ_CONFIG);
+}
+
+static const struct drm_bridge_funcs ge_b850v3_lvds_dp_funcs = {
+	.attach = ge_b850v3_lvds_dp_attach,
+	.detach = ge_b850v3_lvds_dp_detach,
+};
+
+static int ge_b850v3_lvds_dp_probe(struct i2c_client *ge_b850v3_lvds_dp_i2c,
+				const struct i2c_device_id *id)
+{
+	struct device *dev = &ge_b850v3_lvds_dp_i2c->dev;
+	struct ge_b850v3_lvds_dp *ptn_bridge;
+
+	ptn_bridge = devm_kzalloc(dev, sizeof(*ptn_bridge), GFP_KERNEL);
+	if (!ptn_bridge)
+		return -ENOMEM;
+
+	mutex_init(&ptn_bridge->edid_mutex);
+	mutex_init(&ptn_bridge->irq_reg_mutex);
+
+	ptn_bridge->ge_b850v3_lvds_dp_i2c = ge_b850v3_lvds_dp_i2c;
+	ptn_bridge->bridge.driver_private = ptn_bridge;
+	i2c_set_clientdata(ge_b850v3_lvds_dp_i2c, ptn_bridge);
+
+	ptn_bridge->edid_i2c = i2c_new_secondary_device(ge_b850v3_lvds_dp_i2c,
+			DEFAULT_EDID_REG_NAME, DEFAULT_EDID_REG);
+
+	if (!ptn_bridge->edid_i2c) {
+		dev_err(dev, "Error registering edid i2c_client, aborting...\n");
+		return -ENODEV;
+	}
+
+	ptn_bridge->bridge.funcs = &ge_b850v3_lvds_dp_funcs;
+	ptn_bridge->bridge.of_node = dev->of_node;
+	drm_bridge_add(&ptn_bridge->bridge);
+
+	/* Clear pending interrupts since power up. */
+	i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
+			STDP4028_DPTX_IRQ_STS_REG, STDP4028_DPTX_IRQ_CLEAR);
+
+	if (!ge_b850v3_lvds_dp_i2c->irq)
+		return 0;
+
+	return devm_request_threaded_irq(&ge_b850v3_lvds_dp_i2c->dev,
+			ge_b850v3_lvds_dp_i2c->irq, NULL,
+			ge_b850v3_lvds_dp_irq_handler,
+			IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+			"ge-b850v3-lvds-dp", ptn_bridge);
+}
+
+static int ge_b850v3_lvds_dp_remove(struct i2c_client *ge_b850v3_lvds_dp_i2c)
+{
+	struct ge_b850v3_lvds_dp *ptn_bridge =
+		i2c_get_clientdata(ge_b850v3_lvds_dp_i2c);
+
+	i2c_unregister_device(ptn_bridge->edid_i2c);
+
+	drm_bridge_remove(&ptn_bridge->bridge);
+
+	kfree(ptn_bridge->edid);
+
+	return 0;
+}
+
+static const struct i2c_device_id ge_b850v3_lvds_dp_i2c_table[] = {
+	{"b850v3-lvds-dp", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, ge_b850v3_lvds_dp_i2c_table);
+
+static const struct of_device_id ge_b850v3_lvds_dp_match[] = {
+	{ .compatible = "ge,b850v3-lvds-dp" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ge_b850v3_lvds_dp_match);
+
+static struct i2c_driver ge_b850v3_lvds_dp_driver = {
+	.id_table	= ge_b850v3_lvds_dp_i2c_table,
+	.probe		= ge_b850v3_lvds_dp_probe,
+	.remove		= ge_b850v3_lvds_dp_remove,
+	.driver		= {
+		.name		= "b850v3-lvds-dp",
+		.of_match_table = ge_b850v3_lvds_dp_match,
+	},
+};
+module_i2c_driver(ge_b850v3_lvds_dp_driver);
+
+MODULE_AUTHOR("Peter Senna Tschudin <peter.senna@collabora.com>");
+MODULE_AUTHOR("Martyn Welch <martyn.welch@collabora.co.uk>");
+MODULE_DESCRIPTION("GE LVDS to DP++ display bridge)");
+MODULE_LICENSE("GPL v2");
-- 
2.5.5

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Peter Senna Tschudin]

 
On 01 January, 2017 21:24 CET, Peter Senna Tschudin <peter.senna@collabora.com> wrote: 

[ ... ] 

> +static void ge_b850v3_lvds_dp_detach(struct drm_bridge *bridge)
> +{
> +	struct ge_b850v3_lvds_dp *ptn_bridge
> +			= bridge_to_ge_b850v3_lvds_dp(bridge);
> +	struct i2c_client *ge_b850v3_lvds_dp_i2c
> +			= ptn_bridge->ge_b850v3_lvds_dp_i2c;
> +
> +	/* Disable interrupts */
> +	i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
> +			STDP4028_DPTX_IRQ_EN_REG, ~STDP4028_DPTX_IRQ_CONFIG);

 ~STDP4028_DPTX_IRQ_CONFIG? Argh! Will fix this and resend after reviews...

[ ... ] 
 
 

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Archit Taneja]

Hi,

Some comments below.

On 01/02/2017 01:54 AM, Peter Senna Tschudin wrote:
> Add a driver that create a drm_bridge and a drm_connector for the LVDS
> to DP++ display bridge of the GE B850v3.
>
> 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
>
> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> Cc: Martin Donnelly <martin.donnelly@ge.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> 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>
> CC: David Airlie <airlied@linux.ie>
> CC: Thierry Reding <treding@nvidia.com>
> CC: Thierry Reding <thierry.reding@gmail.com>
> CC: Archit Taneja <architt@codeaurora.org>
> Reviewed-by: Enric Balletbo <enric.balletbo@collabora.com>
> Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com>
> ---
>  drivers/gpu/drm/bridge/Kconfig             |  11 +
>  drivers/gpu/drm/bridge/Makefile            |   1 +
>  drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c | 384 +++++++++++++++++++++++++++++
>  3 files changed, 396 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index eb8688e..e3e1f3b 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -48,6 +48,17 @@ config DRM_DW_HDMI_I2S_AUDIO
>  	  Support the I2S Audio interface which is part of the Synopsis
>  	  Designware HDMI block.
>
> +config DRM_GE_B850V3_LVDS_DP
> +	tristate "GE B850v3 LVDS to DP++ display bridge"
> +	depends on OF
> +	select DRM_KMS_HELPER
> +	select DRM_PANEL
> +	---help---
> +          This is a driver for the display bridge of
> +          GE B850v3 that convert dual channel LVDS
> +          to DP++. This is used with the i.MX6 imx-ldb
> +          driver.
> +
>  config DRM_NXP_PTN3460
>  	tristate "NXP PTN3460 DP/LVDS bridge"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 2e83a785..886d0fd 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
>  obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o
>  obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o
>  obj-$(CONFIG_DRM_DW_HDMI_I2S_AUDIO) += dw-hdmi-i2s-audio.o
> +obj-$(CONFIG_DRM_GE_B850V3_LVDS_DP) += ge_b850v3_lvds_dp.o
>  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
>  obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
> diff --git a/drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c b/drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c
> new file mode 100644
> index 0000000..4574f6e
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c
> @@ -0,0 +1,384 @@
> +/*
> + * Driver for GE B850v3 DP display bridge

Mentioning LVDS to DP++ here would be nice.

> +
> + * Copyright (c) 2016, Collabora Ltd.
> + * Copyright (c) 2016, General Electric Company
> +
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> +
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> +
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> + * This driver creates a drm_bridge and a drm_connector for the LVDS to DP++
> + * display bridge of the GE B850v3. There are two physical bridges on the video
> + * signal pipeline: a STDP4028(LVDS to DP) and a STDP2690(DP to DP++). However
> + * the physical bridges are automatically configured by the input video signal,
> + * and the driver has no access to the video processing pipeline. The driver is
> + * only needed to read EDID from the STDP2690 and to handle HPD events from the
> + * STDP4028. The driver communicates with both bridges over i2c. The video
> + * signal pipeline is as follows:
> + *
> + *   Host -> LVDS|--(STDP4028)--|DP -> DP|--(STDP2690)--|DP++ -> Video output
> + *
> + */
> +
> +#include <linux/gpio.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_edid.h>
> +#include <drm/drmP.h>
> +
> +#define DEFAULT_EDID_REG 0x72
> +#define DEFAULT_EDID_REG_NAME "edid"
> +
> +#define EDID_EXT_BLOCK_CNT 0x7E
> +
> +#define STDP4028_IRQ_OUT_CONF_REG 0x02
> +#define STDP4028_DPTX_IRQ_EN_REG 0x3C
> +#define STDP4028_DPTX_IRQ_STS_REG 0x3D
> +#define STDP4028_DPTX_STS_REG 0x3E
> +
> +#define STDP4028_DPTX_DP_IRQ_EN 0x1000
> +
> +#define STDP4028_DPTX_HOTPLUG_IRQ_EN 0x0400
> +#define STDP4028_DPTX_LINK_CH_IRQ_EN 0x2000
> +#define STDP4028_DPTX_IRQ_CONFIG \
> +		(STDP4028_DPTX_LINK_CH_IRQ_EN | STDP4028_DPTX_HOTPLUG_IRQ_EN)
> +
> +#define STDP4028_DPTX_HOTPLUG_STS 0x0200
> +#define STDP4028_DPTX_LINK_STS 0x1000
> +#define STDP4028_CON_STATE_CONNECTED \
> +		(STDP4028_DPTX_HOTPLUG_STS | STDP4028_DPTX_LINK_STS)
> +
> +#define STDP4028_DPTX_HOTPLUG_CH_STS 0x0400
> +#define STDP4028_DPTX_LINK_CH_STS 0x2000
> +#define STDP4028_DPTX_IRQ_CLEAR \
> +		(STDP4028_DPTX_LINK_CH_STS | STDP4028_DPTX_HOTPLUG_CH_STS)
> +
> +struct ge_b850v3_lvds_dp {
> +	struct drm_connector connector;
> +	struct drm_bridge bridge;
> +	struct i2c_client *ge_b850v3_lvds_dp_i2c;
> +	struct i2c_client *edid_i2c;
> +	struct edid *edid;
> +	struct mutex edid_mutex;
> +	struct mutex irq_reg_mutex;
> +};
> +
> +static inline struct ge_b850v3_lvds_dp *
> +		bridge_to_ge_b850v3_lvds_dp(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct ge_b850v3_lvds_dp, bridge);
> +}
> +
> +static inline struct ge_b850v3_lvds_dp *
> +		connector_to_ge_b850v3_lvds_dp(struct drm_connector *connector)
> +{
> +	return container_of(connector, struct ge_b850v3_lvds_dp, connector);
> +}
> +
> +u8 *stdp2690_get_edid(struct i2c_client *client)
> +{
> +	struct i2c_adapter *adapter = client->adapter;
> +	unsigned char start = 0x00;
> +	unsigned int total_size;
> +	u8 *block = kmalloc(EDID_LENGTH, GFP_KERNEL);
> +
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr	= client->addr,
> +			.flags	= 0,
> +			.len	= 1,
> +			.buf	= &start,
> +		}, {
> +			.addr	= client->addr,
> +			.flags	= I2C_M_RD,
> +			.len	= EDID_LENGTH,
> +			.buf	= block,
> +		}
> +	};
> +
> +	if (!block)
> +		return NULL;
> +
> +	if (i2c_transfer(adapter, msgs, 2) != 2) {
> +		DRM_ERROR("Unable to read EDID.\n");
> +		goto err;
> +	}
> +
> +	if (!drm_edid_block_valid(block, 0, false, NULL)) {
> +		DRM_ERROR("Invalid EDID block\n");
> +		goto err;
> +	}
> +
> +	total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
> +	if (total_size > EDID_LENGTH) {
> +		kfree(block);
> +		block = kmalloc(total_size, GFP_KERNEL);
> +		if (!block)
> +			return NULL;
> +
> +		/* Yes, read the entire buffer, and do not skip the first
> +		 * EDID_LENGTH bytes.
> +		 */

Is this the reason why you aren't using drm_do_get_edid()?

> +		start = 0x00;
> +		msgs[1].len = total_size;
> +		msgs[1].buf = block;
> +
> +		if (i2c_transfer(adapter, msgs, 2) != 2) {
> +			DRM_ERROR("Unable to read EDID extension blocks.\n");
> +			goto err;
> +		}

We should ideally check if the extension blocks are valid too.

> +	}
> +
> +	return block;
> +
> +err:
> +	kfree(block);
> +	return NULL;
> +}
> +
> +static int ge_b850v3_lvds_dp_get_modes(struct drm_connector *connector)
> +{
> +	struct ge_b850v3_lvds_dp *ptn_bridge;

Why are the bridge pointers named ptn_bridge? I'm guessing it's because you
used nxp-ptn3460 bridge driver as reference. You should use something relevant
to your device.

> +	struct i2c_client *client;
> +	int num_modes = 0;
> +
> +	ptn_bridge = connector_to_ge_b850v3_lvds_dp(connector);
> +	client = ptn_bridge->edid_i2c;
> +
> +	mutex_lock(&ptn_bridge->edid_mutex);

Do we really need this mutex? All the paths that call a connector's get_modes
hold the drm device's dev->mode_config.mutex lock anyway.

> +
> +	kfree(ptn_bridge->edid);
> +	ptn_bridge->edid = (struct edid *) stdp2690_get_edid(client);
> +
> +	if (ptn_bridge->edid) {
> +		drm_mode_connector_update_edid_property(connector,
> +				ptn_bridge->edid);
> +		num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
> +	}
> +
> +	mutex_unlock(&ptn_bridge->edid_mutex);
> +
> +	return num_modes;
> +}
> +
> +
> +static enum drm_mode_status ge_b850v3_lvds_dp_mode_valid(
> +		struct drm_connector *connector, struct drm_display_mode *mode)
> +{
> +	return MODE_OK;
> +}
> +
> +static const struct
> +drm_connector_helper_funcs ge_b850v3_lvds_dp_connector_helper_funcs = {
> +	.get_modes = ge_b850v3_lvds_dp_get_modes,
> +	.mode_valid = ge_b850v3_lvds_dp_mode_valid,
> +};
> +
> +static enum drm_connector_status ge_b850v3_lvds_dp_detect(
> +		struct drm_connector *connector, bool force)
> +{
> +	struct ge_b850v3_lvds_dp *ptn_bridge =
> +			connector_to_ge_b850v3_lvds_dp(connector);
> +	struct i2c_client *ge_b850v3_lvds_dp_i2c =
> +			ptn_bridge->ge_b850v3_lvds_dp_i2c;
> +	s32 link_state;
> +
> +	link_state = i2c_smbus_read_word_data(ge_b850v3_lvds_dp_i2c,
> +			STDP4028_DPTX_STS_REG);
> +
> +	if (link_state == STDP4028_CON_STATE_CONNECTED)
> +		return connector_status_connected;
> +
> +	if (link_state == 0)
> +		return connector_status_disconnected;
> +
> +	return connector_status_unknown;
> +}
> +
> +static const struct drm_connector_funcs ge_b850v3_lvds_dp_connector_funcs = {
> +	.dpms = drm_atomic_helper_connector_dpms,
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.detect = ge_b850v3_lvds_dp_detect,
> +	.destroy = drm_connector_cleanup,
> +	.reset = drm_atomic_helper_connector_reset,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static irqreturn_t ge_b850v3_lvds_dp_irq_handler(int irq, void *dev_id)
> +{
> +	struct ge_b850v3_lvds_dp *ptn_bridge = dev_id;
> +	struct i2c_client *ge_b850v3_lvds_dp_i2c
> +			= ptn_bridge->ge_b850v3_lvds_dp_i2c;
> +
> +	mutex_lock(&ptn_bridge->irq_reg_mutex);

Do we need this mutex? The handler is registered with the IRQF_ONESHOT
flag, so we won't get another interrupt until this handler returns.

> +
> +	i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
> +			STDP4028_DPTX_IRQ_STS_REG, STDP4028_DPTX_IRQ_CLEAR);
> +
> +	mutex_unlock(&ptn_bridge->irq_reg_mutex);
> +
> +	if (ptn_bridge->connector.dev)
> +		drm_kms_helper_hotplug_event(ptn_bridge->connector.dev);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ge_b850v3_lvds_dp_attach(struct drm_bridge *bridge)
> +{
> +	struct ge_b850v3_lvds_dp *ptn_bridge
> +			= bridge_to_ge_b850v3_lvds_dp(bridge);
> +	struct drm_connector *connector = &ptn_bridge->connector;
> +	struct i2c_client *ge_b850v3_lvds_dp_i2c
> +			= ptn_bridge->ge_b850v3_lvds_dp_i2c;
> +	int ret;
> +
> +	if (!bridge->encoder) {
> +		DRM_ERROR("Parent encoder object not found");
> +		return -ENODEV;
> +	}
> +
> +	connector->polled = DRM_CONNECTOR_POLL_HPD;
> +
> +	drm_connector_helper_add(connector,
> +			&ge_b850v3_lvds_dp_connector_helper_funcs);
> +
> +	ret = drm_connector_init(bridge->dev, connector,
> +			&ge_b850v3_lvds_dp_connector_funcs,
> +			DRM_MODE_CONNECTOR_DisplayPort);
> +	if (ret) {
> +		DRM_ERROR("Failed to initialize connector with drm\n");
> +		return ret;
> +	}
> +
> +	ret = drm_mode_connector_attach_encoder(connector, bridge->encoder);
> +	if (ret)
> +		return ret;
> +
> +	drm_helper_hpd_irq_event(connector->dev);

This call doesn't serve any purpose for a connector until it is registered.
You can drop this.

> +
> +	/* Configures the bridge to re-enable interrupts after each ack. */
> +	i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
> +			STDP4028_IRQ_OUT_CONF_REG, STDP4028_DPTX_DP_IRQ_EN);
> +
> +	/* Enable interrupts */
> +	i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
> +			STDP4028_DPTX_IRQ_EN_REG, STDP4028_DPTX_IRQ_CONFIG);
> +
> +	return 0;
> +}
> +
> +static void ge_b850v3_lvds_dp_detach(struct drm_bridge *bridge)
> +{
> +	struct ge_b850v3_lvds_dp *ptn_bridge
> +			= bridge_to_ge_b850v3_lvds_dp(bridge);
> +	struct i2c_client *ge_b850v3_lvds_dp_i2c
> +			= ptn_bridge->ge_b850v3_lvds_dp_i2c;
> +
> +	/* Disable interrupts */
> +	i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
> +			STDP4028_DPTX_IRQ_EN_REG, ~STDP4028_DPTX_IRQ_CONFIG);
> +}
> +
> +static const struct drm_bridge_funcs ge_b850v3_lvds_dp_funcs = {
> +	.attach = ge_b850v3_lvds_dp_attach,
> +	.detach = ge_b850v3_lvds_dp_detach,
> +};
> +
> +static int ge_b850v3_lvds_dp_probe(struct i2c_client *ge_b850v3_lvds_dp_i2c,
> +				const struct i2c_device_id *id)
> +{
> +	struct device *dev = &ge_b850v3_lvds_dp_i2c->dev;
> +	struct ge_b850v3_lvds_dp *ptn_bridge;
> +
> +	ptn_bridge = devm_kzalloc(dev, sizeof(*ptn_bridge), GFP_KERNEL);
> +	if (!ptn_bridge)
> +		return -ENOMEM;
> +
> +	mutex_init(&ptn_bridge->edid_mutex);
> +	mutex_init(&ptn_bridge->irq_reg_mutex);
> +
> +	ptn_bridge->ge_b850v3_lvds_dp_i2c = ge_b850v3_lvds_dp_i2c;
> +	ptn_bridge->bridge.driver_private = ptn_bridge;

bridge->driver_private isn't used by the driver anywhere. It's probably
better to drop it until it is used.

> +	i2c_set_clientdata(ge_b850v3_lvds_dp_i2c, ptn_bridge);
> +
> +	ptn_bridge->edid_i2c = i2c_new_secondary_device(ge_b850v3_lvds_dp_i2c,
> +			DEFAULT_EDID_REG_NAME, DEFAULT_EDID_REG);
> +
> +	if (!ptn_bridge->edid_i2c) {
> +		dev_err(dev, "Error registering edid i2c_client, aborting...\n");
> +		return -ENODEV;
> +	}
> +
> +	ptn_bridge->bridge.funcs = &ge_b850v3_lvds_dp_funcs;
> +	ptn_bridge->bridge.of_node = dev->of_node;
> +	drm_bridge_add(&ptn_bridge->bridge);
> +
> +	/* Clear pending interrupts since power up. */
> +	i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
> +			STDP4028_DPTX_IRQ_STS_REG, STDP4028_DPTX_IRQ_CLEAR);
> +
> +	if (!ge_b850v3_lvds_dp_i2c->irq)
> +		return 0;
> +
> +	return devm_request_threaded_irq(&ge_b850v3_lvds_dp_i2c->dev,
> +			ge_b850v3_lvds_dp_i2c->irq, NULL,
> +			ge_b850v3_lvds_dp_irq_handler,
> +			IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> +			"ge-b850v3-lvds-dp", ptn_bridge);
> +}
> +
> +static int ge_b850v3_lvds_dp_remove(struct i2c_client *ge_b850v3_lvds_dp_i2c)
> +{
> +	struct ge_b850v3_lvds_dp *ptn_bridge =
> +		i2c_get_clientdata(ge_b850v3_lvds_dp_i2c);
> +
> +	i2c_unregister_device(ptn_bridge->edid_i2c);
> +
> +	drm_bridge_remove(&ptn_bridge->bridge);
> +
> +	kfree(ptn_bridge->edid);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id ge_b850v3_lvds_dp_i2c_table[] = {
> +	{"b850v3-lvds-dp", 0},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i2c, ge_b850v3_lvds_dp_i2c_table);
> +
> +static const struct of_device_id ge_b850v3_lvds_dp_match[] = {
> +	{ .compatible = "ge,b850v3-lvds-dp" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ge_b850v3_lvds_dp_match);
> +
> +static struct i2c_driver ge_b850v3_lvds_dp_driver = {
> +	.id_table	= ge_b850v3_lvds_dp_i2c_table,
> +	.probe		= ge_b850v3_lvds_dp_probe,
> +	.remove		= ge_b850v3_lvds_dp_remove,
> +	.driver		= {
> +		.name		= "b850v3-lvds-dp",
> +		.of_match_table = ge_b850v3_lvds_dp_match,
> +	},
> +};
> +module_i2c_driver(ge_b850v3_lvds_dp_driver);
> +
> +MODULE_AUTHOR("Peter Senna Tschudin <peter.senna@collabora.com>");
> +MODULE_AUTHOR("Martyn Welch <martyn.welch@collabora.co.uk>");
> +MODULE_DESCRIPTION("GE LVDS to DP++ display bridge)");
> +MODULE_LICENSE("GPL v2");
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Peter Senna Tschudin]

On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
Hi Archit,

Thank you for the comments!

[...]
> > +	total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
> > +	if (total_size > EDID_LENGTH) {
> > +		kfree(block);
> > +		block = kmalloc(total_size, GFP_KERNEL);
> > +		if (!block)
> > +			return NULL;
> > +
> > +		/* Yes, read the entire buffer, and do not skip the first
> > +		 * EDID_LENGTH bytes.
> > +		 */
> 
> Is this the reason why you aren't using drm_do_get_edid()?

Yes, for some hw specific reason, it is necessary to read the entire
EDID buffer starting from 0, not block by block.

[...]

I fixed all your other suggestions. Thank you!

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Jani Nikula]

On Sat, 28 Jan 2017, Peter Senna Tschudin <peter.senna@collabora.com> wrote:
> On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
> Hi Archit,
>
> Thank you for the comments!
>
> [...]
>> > +	total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
>> > +	if (total_size > EDID_LENGTH) {
>> > +		kfree(block);
>> > +		block = kmalloc(total_size, GFP_KERNEL);
>> > +		if (!block)
>> > +			return NULL;
>> > +
>> > +		/* Yes, read the entire buffer, and do not skip the first
>> > +		 * EDID_LENGTH bytes.
>> > +		 */
>> 
>> Is this the reason why you aren't using drm_do_get_edid()?
>
> Yes, for some hw specific reason, it is necessary to read the entire
> EDID buffer starting from 0, not block by block.

Hrmh, I'm planning on moving the edid override and firmware edid
mechanisms at the drm_do_get_edid() level to be able to truly and
transparently use a different edid. Currently, they're only used for
modes, really, and lead to some info retrieved from overrides, some from
the real edid. This kind of hacks will bypass the override/firmware edid
mechanisms then too. :(

BR,
Jani.


>
> [...]
>
> I fixed all your other suggestions. Thank you!
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Archit Taneja]

On 01/30/2017 10:35 PM, Jani Nikula wrote:
> On Sat, 28 Jan 2017, Peter Senna Tschudin <peter.senna@collabora.com> wrote:
>> On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
>> Hi Archit,
>>
>> Thank you for the comments!
>>
>> [...]
>>>> +	total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
>>>> +	if (total_size > EDID_LENGTH) {
>>>> +		kfree(block);
>>>> +		block = kmalloc(total_size, GFP_KERNEL);
>>>> +		if (!block)
>>>> +			return NULL;
>>>> +
>>>> +		/* Yes, read the entire buffer, and do not skip the first
>>>> +		 * EDID_LENGTH bytes.
>>>> +		 */
>>>
>>> Is this the reason why you aren't using drm_do_get_edid()?
>>
>> Yes, for some hw specific reason, it is necessary to read the entire
>> EDID buffer starting from 0, not block by block.
>
> Hrmh, I'm planning on moving the edid override and firmware edid
> mechanisms at the drm_do_get_edid() level to be able to truly and
> transparently use a different edid. Currently, they're only used for
> modes, really, and lead to some info retrieved from overrides, some from
> the real edid. This kind of hacks will bypass the override/firmware edid
> mechanisms then too. :(

It seems like there is a HW issue which prevents them from reading EDID
from an offset. So, I'm not sure if it is a hack or a HW limitation.

One way around this would be to hide the HW requirement in the
get_edid_block func pointer passed to drm_do_get_edid(). This
would, however, result in more i2c reads (equal to # of extension
blocks) than what the patch currently does.

Peter, if you think doing extra EDID reads isn't too costly on your
platform, you could consider using drm_do_get_edid(). If not, I guess
you'll miss out on the additional functionality Jani is going to add
in the future.

Thanks,
Archit


>
> BR,
> Jani.
>
>
>>
>> [...]
>>
>> I fixed all your other suggestions. Thank you!
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Peter Senna Tschudin]

Hi Archit,
 
On 01 February, 2017 10:44 CET, Archit Taneja <architt@codeaurora.org> wrote: 

> 
> 
> On 01/30/2017 10:35 PM, Jani Nikula wrote:
> > On Sat, 28 Jan 2017, Peter Senna Tschudin <peter.senna@collabora.com> wrote:
> >> On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
> >> Hi Archit,
> >>
> >> Thank you for the comments!
> >>
> >> [...]
> >>>> +	total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
> >>>> +	if (total_size > EDID_LENGTH) {
> >>>> +		kfree(block);
> >>>> +		block = kmalloc(total_size, GFP_KERNEL);
> >>>> +		if (!block)
> >>>> +			return NULL;
> >>>> +
> >>>> +		/* Yes, read the entire buffer, and do not skip the first
> >>>> +		 * EDID_LENGTH bytes.
> >>>> +		 */
> >>>
> >>> Is this the reason why you aren't using drm_do_get_edid()?

> >>
> >> Yes, for some hw specific reason, it is necessary to read the entire
> >> EDID buffer starting from 0, not block by block.
> >
> > Hrmh, I'm planning on moving the edid override and firmware edid
> > mechanisms at the drm_do_get_edid() level to be able to truly and
> > transparently use a different edid. Currently, they're only used for
> > modes, really, and lead to some info retrieved from overrides, some from
> > the real edid. This kind of hacks will bypass the override/firmware edid
> > mechanisms then too. :(
> 
> It seems like there is a HW issue which prevents them from reading EDID
> from an offset. So, I'm not sure if it is a hack or a HW limitation.

> 
> One way around this would be to hide the HW requirement in the
> get_edid_block func pointer passed to drm_do_get_edid(). This
> would, however, result in more i2c reads (equal to # of extension
> blocks) than what the patch currently does.
> 
> Peter, if you think doing extra EDID reads isn't too costly on your
> platform, you could consider using drm_do_get_edid(). If not, I guess
> you'll miss out on the additional functionality Jani is going to add

> in the future.

My concern is that for almost one year now, every time I fix something one or two new requests are made. I'm happy to fix the driver, but I want a list of the changes that are required to get it upstream, before I make more changes. Can we agree on exactly what is preventing this driver to get upstream? Then I'll fix it.

> 
> Thanks,
> Archit
> 
> 
> >
> > BR,
> > Jani.
> >
> >
> >>
> >> [...]
> >>
> >> I fixed all your other suggestions. Thank you!
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
 
 
 
 

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Daniel Vetter]

On Wed, Feb 01, 2017 at 10:58:43AM +0000, Peter Senna Tschudin wrote:
> Hi Archit,
> 
> On 01 February, 2017 10:44 CET, Archit Taneja <architt@codeaurora.org> wrote:
> 
> >
> >
> > On 01/30/2017 10:35 PM, Jani Nikula wrote:
> > > On Sat, 28 Jan 2017, Peter Senna Tschudin <peter.senna@collabora.com> wrote:
> > >> On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
> > >> Hi Archit,
> > >>
> > >> Thank you for the comments!
> > >>
> > >> [...]
> > >>>> +	total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
> > >>>> +	if (total_size > EDID_LENGTH) {
> > >>>> +		kfree(block);
> > >>>> +		block = kmalloc(total_size, GFP_KERNEL);
> > >>>> +		if (!block)
> > >>>> +			return NULL;
> > >>>> +
> > >>>> +		/* Yes, read the entire buffer, and do not skip the first
> > >>>> +		 * EDID_LENGTH bytes.
> > >>>> +		 */
> > >>>
> > >>> Is this the reason why you aren't using drm_do_get_edid()?
> 
> > >>
> > >> Yes, for some hw specific reason, it is necessary to read the entire
> > >> EDID buffer starting from 0, not block by block.
> > >
> > > Hrmh, I'm planning on moving the edid override and firmware edid
> > > mechanisms at the drm_do_get_edid() level to be able to truly and
> > > transparently use a different edid. Currently, they're only used for
> > > modes, really, and lead to some info retrieved from overrides, some from
> > > the real edid. This kind of hacks will bypass the override/firmware edid
> > > mechanisms then too. :(
> >
> > It seems like there is a HW issue which prevents them from reading EDID
> > from an offset. So, I'm not sure if it is a hack or a HW limitation.
> 
> >
> > One way around this would be to hide the HW requirement in the
> > get_edid_block func pointer passed to drm_do_get_edid(). This
> > would, however, result in more i2c reads (equal to # of extension
> > blocks) than what the patch currently does.
> >
> > Peter, if you think doing extra EDID reads isn't too costly on your
> > platform, you could consider using drm_do_get_edid(). If not, I guess
> > you'll miss out on the additional functionality Jani is going to add
> 
> > in the future.
> 
> My concern is that for almost one year now, every time I fix something
> one or two new requests are made. I'm happy to fix the driver, but I
> want a list of the changes that are required to get it upstream, before
> I make more changes. Can we agree on exactly what is preventing this
> driver to get upstream? Then I'll fix it.

I think addressing this edid reading question post-merge is perfectly
fine. Aside, want to keep maintaing your stuff as part of the drm-misc
group, with the drivers-in-misc experiment?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Peter Senna Tschudin]

 
On 01 February, 2017 12:35 CET, Daniel Vetter <daniel@ffwll.ch> wrote: 
 
> On Wed, Feb 01, 2017 at 10:58:43AM +0000, Peter Senna Tschudin wrote:
> > Hi Archit,
> > 
> > On 01 February, 2017 10:44 CET, Archit Taneja <architt@codeaurora.org> wrote:
> > 
> > >
> > >
> > > On 01/30/2017 10:35 PM, Jani Nikula wrote:
> > > > On Sat, 28 Jan 2017, Peter Senna Tschudin <peter.senna@collabora.com> wrote:
> > > >> On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
> > > >> Hi Archit,
> > > >>
> > > >> Thank you for the comments!
> > > >>
> > > >> [...]
> > > >>>> +	total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
> > > >>>> +	if (total_size > EDID_LENGTH) {
> > > >>>> +		kfree(block);
> > > >>>> +		block = kmalloc(total_size, GFP_KERNEL);
> > > >>>> +		if (!block)
> > > >>>> +			return NULL;
> > > >>>> +
> > > >>>> +		/* Yes, read the entire buffer, and do not skip the first
> > > >>>> +		 * EDID_LENGTH bytes.
> > > >>>> +		 */
> > > >>>
> > > >>> Is this the reason why you aren't using drm_do_get_edid()?
> > 
> > > >>
> > > >> Yes, for some hw specific reason, it is necessary to read the entire
> > > >> EDID buffer starting from 0, not block by block.
> > > >
> > > > Hrmh, I'm planning on moving the edid override and firmware edid
> > > > mechanisms at the drm_do_get_edid() level to be able to truly and
> > > > transparently use a different edid. Currently, they're only used for
> > > > modes, really, and lead to some info retrieved from overrides, some from
> > > > the real edid. This kind of hacks will bypass the override/firmware edid
> > > > mechanisms then too. :(
> > >
> > > It seems like there is a HW issue which prevents them from reading EDID
> > > from an offset. So, I'm not sure if it is a hack or a HW limitation.
> > 
> > >
> > > One way around this would be to hide the HW requirement in the
> > > get_edid_block func pointer passed to drm_do_get_edid(). This
> > > would, however, result in more i2c reads (equal to # of extension
> > > blocks) than what the patch currently does.
> > >
> > > Peter, if you think doing extra EDID reads isn't too costly on your
> > > platform, you could consider using drm_do_get_edid(). If not, I guess
> > > you'll miss out on the additional functionality Jani is going to add
> > 
> > > in the future.
> > 
> > My concern is that for almost one year now, every time I fix something
> > one or two new requests are made. I'm happy to fix the driver, but I
> > want a list of the changes that are required to get it upstream, before
> > I make more changes. Can we agree on exactly what is preventing this
> > driver to get upstream? Then I'll fix it.
> 
> I think addressing this edid reading question post-merge is perfectly
> fine. Aside, want to keep maintaing your stuff as part of the drm-misc
> group, with the drivers-in-misc experiment?

Yes, sure!

> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
 
 
 
 

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Archit Taneja]

On 02/01/2017 05:51 PM, Peter Senna Tschudin wrote:
>
> On 01 February, 2017 12:35 CET, Daniel Vetter <daniel@ffwll.ch> wrote:
>
>> On Wed, Feb 01, 2017 at 10:58:43AM +0000, Peter Senna Tschudin wrote:
>>> Hi Archit,
>>>
>>> On 01 February, 2017 10:44 CET, Archit Taneja <architt@codeaurora.org> wrote:
>>>
>>>>
>>>>
>>>> On 01/30/2017 10:35 PM, Jani Nikula wrote:
>>>>> On Sat, 28 Jan 2017, Peter Senna Tschudin <peter.senna@collabora.com> wrote:
>>>>>> On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
>>>>>> Hi Archit,
>>>>>>
>>>>>> Thank you for the comments!
>>>>>>
>>>>>> [...]
>>>>>>>> +	total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
>>>>>>>> +	if (total_size > EDID_LENGTH) {
>>>>>>>> +		kfree(block);
>>>>>>>> +		block = kmalloc(total_size, GFP_KERNEL);
>>>>>>>> +		if (!block)
>>>>>>>> +			return NULL;
>>>>>>>> +
>>>>>>>> +		/* Yes, read the entire buffer, and do not skip the first
>>>>>>>> +		 * EDID_LENGTH bytes.
>>>>>>>> +		 */
>>>>>>>
>>>>>>> Is this the reason why you aren't using drm_do_get_edid()?
>>>
>>>>>>
>>>>>> Yes, for some hw specific reason, it is necessary to read the entire
>>>>>> EDID buffer starting from 0, not block by block.
>>>>>
>>>>> Hrmh, I'm planning on moving the edid override and firmware edid
>>>>> mechanisms at the drm_do_get_edid() level to be able to truly and
>>>>> transparently use a different edid. Currently, they're only used for
>>>>> modes, really, and lead to some info retrieved from overrides, some from
>>>>> the real edid. This kind of hacks will bypass the override/firmware edid
>>>>> mechanisms then too. :(
>>>>
>>>> It seems like there is a HW issue which prevents them from reading EDID
>>>> from an offset. So, I'm not sure if it is a hack or a HW limitation.
>>>
>>>>
>>>> One way around this would be to hide the HW requirement in the
>>>> get_edid_block func pointer passed to drm_do_get_edid(). This
>>>> would, however, result in more i2c reads (equal to # of extension
>>>> blocks) than what the patch currently does.
>>>>
>>>> Peter, if you think doing extra EDID reads isn't too costly on your
>>>> platform, you could consider using drm_do_get_edid(). If not, I guess
>>>> you'll miss out on the additional functionality Jani is going to add
>>>
>>>> in the future.
>>>
>>> My concern is that for almost one year now, every time I fix something
>>> one or two new requests are made. I'm happy to fix the driver, but I
>>> want a list of the changes that are required to get it upstream, before
>>> I make more changes. Can we agree on exactly what is preventing this
>>> driver to get upstream? Then I'll fix it.
>>
>> I think addressing this edid reading question post-merge is perfectly
>> fine. Aside, want to keep maintaing your stuff as part of the drm-misc
>> group, with the drivers-in-misc experiment?

The edid thing was only a suggestion. As Daniel said, it's okay to work on
it post merge.

Please do fix the minor comments I mentioned in the latest patch set. I'll
pull in the first 3 patches once Rob H gives an Ack on the DT bindings
patch. The 4th patch needs to go through the SoC maintainer.

Thanks,
Archit

>
> Yes, sure!
>
>> -Daniel
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>
>
>
>
>
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Emil Velikov]

On 1 February 2017 at 11:35, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Feb 01, 2017 at 10:58:43AM +0000, Peter Senna Tschudin wrote:
>> Hi Archit,
>>
>> On 01 February, 2017 10:44 CET, Archit Taneja <architt@codeaurora.org> wrote:
>>
>> >
>> >
>> > On 01/30/2017 10:35 PM, Jani Nikula wrote:
>> > > On Sat, 28 Jan 2017, Peter Senna Tschudin <peter.senna@collabora.com> wrote:
>> > >> On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
>> > >> Hi Archit,
>> > >>
>> > >> Thank you for the comments!
>> > >>
>> > >> [...]
>> > >>>> +      total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
>> > >>>> +      if (total_size > EDID_LENGTH) {
>> > >>>> +              kfree(block);
>> > >>>> +              block = kmalloc(total_size, GFP_KERNEL);
>> > >>>> +              if (!block)
>> > >>>> +                      return NULL;
>> > >>>> +
>> > >>>> +              /* Yes, read the entire buffer, and do not skip the first
>> > >>>> +               * EDID_LENGTH bytes.
>> > >>>> +               */
>> > >>>
>> > >>> Is this the reason why you aren't using drm_do_get_edid()?
>>
>> > >>
>> > >> Yes, for some hw specific reason, it is necessary to read the entire
>> > >> EDID buffer starting from 0, not block by block.
>> > >
>> > > Hrmh, I'm planning on moving the edid override and firmware edid
>> > > mechanisms at the drm_do_get_edid() level to be able to truly and
>> > > transparently use a different edid. Currently, they're only used for
>> > > modes, really, and lead to some info retrieved from overrides, some from
>> > > the real edid. This kind of hacks will bypass the override/firmware edid
>> > > mechanisms then too. :(
>> >
>> > It seems like there is a HW issue which prevents them from reading EDID
>> > from an offset. So, I'm not sure if it is a hack or a HW limitation.
>>
>> >
>> > One way around this would be to hide the HW requirement in the
>> > get_edid_block func pointer passed to drm_do_get_edid(). This
>> > would, however, result in more i2c reads (equal to # of extension
>> > blocks) than what the patch currently does.
>> >
>> > Peter, if you think doing extra EDID reads isn't too costly on your
>> > platform, you could consider using drm_do_get_edid(). If not, I guess
>> > you'll miss out on the additional functionality Jani is going to add
>>
>> > in the future.
>>
>> My concern is that for almost one year now, every time I fix something
>> one or two new requests are made. I'm happy to fix the driver, but I
>> want a list of the changes that are required to get it upstream, before
>> I make more changes. Can we agree on exactly what is preventing this
>> driver to get upstream? Then I'll fix it.
>
> I think addressing this edid reading question post-merge is perfectly
> fine. Aside, want to keep maintaing your stuff as part of the drm-misc
> group, with the drivers-in-misc experiment?

Wasn't there some entry level for people ? It's not like Peter is
thick or anything, just that he has not reviewed or replied (afaict)
to any patches, even on ones where he was explicitly cc'd.
Although he's got dozens of contributions elsewhere, there's only a
single patch of his in DRM.

-Emil

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Peter Senna Tschudin]

 
On 02 February, 2017 02:46 CET, Emil Velikov <emil.l.velikov@gmail.com> wrote: 
 
> On 1 February 2017 at 11:35, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Feb 01, 2017 at 10:58:43AM +0000, Peter Senna Tschudin wrote:
> >> Hi Archit,
> >>
> >> On 01 February, 2017 10:44 CET, Archit Taneja <architt@codeaurora.org> wrote:
> >>
> >> >
> >> >
> >> > On 01/30/2017 10:35 PM, Jani Nikula wrote:
> >> > > On Sat, 28 Jan 2017, Peter Senna Tschudin <peter.senna@collabora.com> wrote:
> >> > >> On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
> >> > >> Hi Archit,
> >> > >>
> >> > >> Thank you for the comments!
> >> > >>
> >> > >> [...]
> >> > >>>> +      total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
> >> > >>>> +      if (total_size > EDID_LENGTH) {
> >> > >>>> +              kfree(block);
> >> > >>>> +              block = kmalloc(total_size, GFP_KERNEL);
> >> > >>>> +              if (!block)
> >> > >>>> +                      return NULL;
> >> > >>>> +
> >> > >>>> +              /* Yes, read the entire buffer, and do not skip the first
> >> > >>>> +               * EDID_LENGTH bytes.
> >> > >>>> +               */
> >> > >>>
> >> > >>> Is this the reason why you aren't using drm_do_get_edid()?
> >>
> >> > >>
> >> > >> Yes, for some hw specific reason, it is necessary to read the entire
> >> > >> EDID buffer starting from 0, not block by block.
> >> > >
> >> > > Hrmh, I'm planning on moving the edid override and firmware edid
> >> > > mechanisms at the drm_do_get_edid() level to be able to truly and
> >> > > transparently use a different edid. Currently, they're only used for
> >> > > modes, really, and lead to some info retrieved from overrides, some from
> >> > > the real edid. This kind of hacks will bypass the override/firmware edid
> >> > > mechanisms then too. :(
> >> >
> >> > It seems like there is a HW issue which prevents them from reading EDID
> >> > from an offset. So, I'm not sure if it is a hack or a HW limitation.
> >>
> >> >
> >> > One way around this would be to hide the HW requirement in the
> >> > get_edid_block func pointer passed to drm_do_get_edid(). This
> >> > would, however, result in more i2c reads (equal to # of extension
> >> > blocks) than what the patch currently does.
> >> >
> >> > Peter, if you think doing extra EDID reads isn't too costly on your
> >> > platform, you could consider using drm_do_get_edid(). If not, I guess
> >> > you'll miss out on the additional functionality Jani is going to add
> >>
> >> > in the future.
> >>
> >> My concern is that for almost one year now, every time I fix something
> >> one or two new requests are made. I'm happy to fix the driver, but I
> >> want a list of the changes that are required to get it upstream, before
> >> I make more changes. Can we agree on exactly what is preventing this
> >> driver to get upstream? Then I'll fix it.
> >
> > I think addressing this edid reading question post-merge is perfectly
> > fine. Aside, want to keep maintaing your stuff as part of the drm-misc
> > group, with the drivers-in-misc experiment?
> 
> Wasn't there some entry level for people ? It's not like Peter is
> thick or anything, just that he has not reviewed or replied (afaict)

> to any patches, even on ones where he was explicitly cc'd.
> Although he's got dozens of contributions elsewhere, there's only a
> single patch of his in DRM.

"Any" is an exaggeration. See:

c6c1f9bc798bee7cf
...
Tested-by: Peter Senna Tschudin <peter.senna@gmail.com>

Even if my involvement with drm started with my driver last year, I took my personal time to report this issue, and to test multiple iterations of the fix. Then my "single patch in DRM" involved some work to get it right, and it is something, that made imx-ldb better by adding a feature it did not support before.

You made me curious. Can you point to the explicit cases where Peter "has not reviewed or replied to any patches, even on ones where he was explicitly cc'd"? And can you list case by case your take on why I should have done differently?

Also it is not clear to me what the problem really is here. I'm assuming is not me, but if I was qualified from your perspective then would be no problem, right?

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Emil Velikov]

On 2 February 2017 at 11:53, Peter Senna Tschudin
<peter.senna@collabora.co.uk> wrote:
>
> On 02 February, 2017 02:46 CET, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
>> On 1 February 2017 at 11:35, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Wed, Feb 01, 2017 at 10:58:43AM +0000, Peter Senna Tschudin wrote:
>> >> Hi Archit,
>> >>
>> >> On 01 February, 2017 10:44 CET, Archit Taneja <architt@codeaurora.org> wrote:
>> >>
>> >> >
>> >> >
>> >> > On 01/30/2017 10:35 PM, Jani Nikula wrote:
>> >> > > On Sat, 28 Jan 2017, Peter Senna Tschudin <peter.senna@collabora.com> wrote:
>> >> > >> On Thu, Jan 05, 2017 at 01:18:47PM +0530, Archit Taneja wrote:
>> >> > >> Hi Archit,
>> >> > >>
>> >> > >> Thank you for the comments!
>> >> > >>
>> >> > >> [...]
>> >> > >>>> +      total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
>> >> > >>>> +      if (total_size > EDID_LENGTH) {
>> >> > >>>> +              kfree(block);
>> >> > >>>> +              block = kmalloc(total_size, GFP_KERNEL);
>> >> > >>>> +              if (!block)
>> >> > >>>> +                      return NULL;
>> >> > >>>> +
>> >> > >>>> +              /* Yes, read the entire buffer, and do not skip the first
>> >> > >>>> +               * EDID_LENGTH bytes.
>> >> > >>>> +               */
>> >> > >>>
>> >> > >>> Is this the reason why you aren't using drm_do_get_edid()?
>> >>
>> >> > >>
>> >> > >> Yes, for some hw specific reason, it is necessary to read the entire
>> >> > >> EDID buffer starting from 0, not block by block.
>> >> > >
>> >> > > Hrmh, I'm planning on moving the edid override and firmware edid
>> >> > > mechanisms at the drm_do_get_edid() level to be able to truly and
>> >> > > transparently use a different edid. Currently, they're only used for
>> >> > > modes, really, and lead to some info retrieved from overrides, some from
>> >> > > the real edid. This kind of hacks will bypass the override/firmware edid
>> >> > > mechanisms then too. :(
>> >> >
>> >> > It seems like there is a HW issue which prevents them from reading EDID
>> >> > from an offset. So, I'm not sure if it is a hack or a HW limitation.
>> >>
>> >> >
>> >> > One way around this would be to hide the HW requirement in the
>> >> > get_edid_block func pointer passed to drm_do_get_edid(). This
>> >> > would, however, result in more i2c reads (equal to # of extension
>> >> > blocks) than what the patch currently does.
>> >> >
>> >> > Peter, if you think doing extra EDID reads isn't too costly on your
>> >> > platform, you could consider using drm_do_get_edid(). If not, I guess
>> >> > you'll miss out on the additional functionality Jani is going to add
>> >>
>> >> > in the future.
>> >>
>> >> My concern is that for almost one year now, every time I fix something
>> >> one or two new requests are made. I'm happy to fix the driver, but I
>> >> want a list of the changes that are required to get it upstream, before
>> >> I make more changes. Can we agree on exactly what is preventing this
>> >> driver to get upstream? Then I'll fix it.
>> >
>> > I think addressing this edid reading question post-merge is perfectly
>> > fine. Aside, want to keep maintaing your stuff as part of the drm-misc
>> > group, with the drivers-in-misc experiment?
>>
>> Wasn't there some entry level for people ? It's not like Peter is
>> thick or anything, just that he has not reviewed or replied (afaict)
>
>> to any patches, even on ones where he was explicitly cc'd.
>> Although he's got dozens of contributions elsewhere, there's only a
>> single patch of his in DRM.
>
> "Any" is an exaggeration. See:
>
> c6c1f9bc798bee7cf
> ...
> Tested-by: Peter Senna Tschudin <peter.senna@gmail.com>
>
> Even if my involvement with drm started with my driver last year, I took my personal time to report this issue, and to test multiple iterations of the fix. Then my "single patch in DRM" involved some work to get it right, and it is something, that made imx-ldb better by adding a feature it did not support before.
>
> You made me curious. Can you point to the explicit cases where Peter "has not reviewed or replied to any patches, even on ones where he was explicitly cc'd"? And can you list case by case your take on why I should have done differently?
>
> Also it is not clear to me what the problem really is here. I'm assuming is not me, but if I was qualified from your perspective then would be no problem, right?
>
Ok seems like my wording came out, rather off.

Peter, as mentioned on IRC my messages is not aimed to be picking on
you in the slightest way. Pardon if it came as such.

It had two main points:
 - Daniel, gents - is drm-misc aimed at devs with limited (no?)
review/commit history in the area and/or the kernel in general ?
In this case, Peter have quite noticeable experience [in kernel
development] with little-to no in DRM.
Should one draw a line in the sand somewhere ?

 - You patch has been on a long [bit rocky road] for a while, with
devs giving you [what seems like] a partial reviews.
Have you considered reviewing others' patches in exchange for a [more
complete one] of this one ? According to git log people have poked you
a handful of times, but seemingly you were busy.

Note that neither of the above is supposed to belittle individuals or
the group maintainer model of drm-misc, but to point out what may not
seem obvious.

Regards,
Emil

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Daniel Vetter]

On Thu, Feb 02, 2017 at 12:37:21PM +0000, Emil Velikov wrote:
>  - Daniel, gents - is drm-misc aimed at devs with limited (no?)
> review/commit history in the area and/or the kernel in general ?
> In this case, Peter have quite noticeable experience [in kernel
> development] with little-to no in DRM.
> Should one draw a line in the sand somewhere ?

We're fairly lenient with accepting new drivers already, and for drivers
in drm-misc we require some checks and peer review to make sure new folks
learn faster. But it's an expirement, I fully expect some fallout and then
some learnign and fine tuning from that, and maybe we even need to crank
requirements back up to a much higher level of experience.

But for drivers I'm honestly not too concerned: As long as there's some
process in place to foster learning&collaboration (which I think will be
better with this) I really don't see much harm in merging non-perfect
driver code.

>  - You patch has been on a long [bit rocky road] for a while, with
> devs giving you [what seems like] a partial reviews.
> Have you considered reviewing others' patches in exchange for a [more
> complete one] of this one ? According to git log people have poked you
> a handful of times, but seemingly you were busy.

Just a clarification: When I review drivers I don't do a fairly cursor
look, hunting for areas where some refactoring and align with drm best
practices would be good. And I think that's perfectly fine and enough to
get a driver landed, but we're definitely not 100% consistent on this
within drm-misc. Probably will take some time to figure this out.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Emil Velikov]

On 3 February 2017 at 08:00, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Feb 02, 2017 at 12:37:21PM +0000, Emil Velikov wrote:
>>  - Daniel, gents - is drm-misc aimed at devs with limited (no?)
>> review/commit history in the area and/or the kernel in general ?
>> In this case, Peter have quite noticeable experience [in kernel
>> development] with little-to no in DRM.
>> Should one draw a line in the sand somewhere ?
>
> We're fairly lenient with accepting new drivers already, and for drivers
> in drm-misc we require some checks and peer review to make sure new folks
> learn faster. But it's an expirement, I fully expect some fallout and then
> some learnign and fine tuning from that, and maybe we even need to crank
> requirements back up to a much higher level of experience.
>
> But for drivers I'm honestly not too concerned: As long as there's some
> process in place to foster learning&collaboration (which I think will be
> better with this) I really don't see much harm in merging non-perfect
> driver code.
>
Thank you Daniel, this is very well said. Being lenient to begin with
and adjusting where/if needed is the more welcoming approach, indeed.
Doubt there'll be (m)any cases of (ab|mis)use but even if so, nobody
can see the future.

>>  - You patch has been on a long [bit rocky road] for a while, with
>> devs giving you [what seems like] a partial reviews.
>> Have you considered reviewing others' patches in exchange for a [more
>> complete one] of this one ? According to git log people have poked you
>> a handful of times, but seemingly you were busy.
>
> Just a clarification: When I review drivers I don't do a fairly cursor
> look, hunting for areas where some refactoring and align with drm best
> practices would be good. And I think that's perfectly fine and enough to
> get a driver landed, but we're definitely not 100% consistent on this
> within drm-misc. Probably will take some time to figure this out.
Seems like I've missed "Peter, your patch..." above, silly me.

Thanks for answering my [what may seem silly] questions ;-)
Emil

Re: [PATCH V7 3/4] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Daniel Vetter]

On Fri, Feb 03, 2017 at 12:25:24PM +0000, Emil Velikov wrote:
> On 3 February 2017 at 08:00, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Feb 02, 2017 at 12:37:21PM +0000, Emil Velikov wrote:
> >>  - Daniel, gents - is drm-misc aimed at devs with limited (no?)
> >> review/commit history in the area and/or the kernel in general ?
> >> In this case, Peter have quite noticeable experience [in kernel
> >> development] with little-to no in DRM.
> >> Should one draw a line in the sand somewhere ?
> >
> > We're fairly lenient with accepting new drivers already, and for drivers
> > in drm-misc we require some checks and peer review to make sure new folks
> > learn faster. But it's an expirement, I fully expect some fallout and then
> > some learnign and fine tuning from that, and maybe we even need to crank
> > requirements back up to a much higher level of experience.
> >
> > But for drivers I'm honestly not too concerned: As long as there's some
> > process in place to foster learning&collaboration (which I think will be
> > better with this) I really don't see much harm in merging non-perfect
> > driver code.
> >
> Thank you Daniel, this is very well said. Being lenient to begin with
> and adjusting where/if needed is the more welcoming approach, indeed.
> Doubt there'll be (m)any cases of (ab|mis)use but even if so, nobody
> can see the future.

Yeah, the big idea behind drm-misc is also to make drm more welcoming for
new folks, and I think starting with a small driver or a few driver
patches for hw you have lying around for hacking is a great way. And every
once in a while we can "trick" one of these newbies into doing some cool
core refactoring or cleanup, and maybe even to stick around for a while.

All for world domination and other evil purposes ofc :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH V7 4/4] dts/imx6q-b850v3: Use GE B850v3 LVDS/DP++ Bridge

[Peter Senna Tschudin]

Configures the GE B850v3 LVDS/DP++ bridge on the dts file.

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>
---
 arch/arm/boot/dts/imx6q-b850v3.dts | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/arm/boot/dts/imx6q-b850v3.dts b/arch/arm/boot/dts/imx6q-b850v3.dts
index 167f744..ce0ca3a 100644
--- a/arch/arm/boot/dts/imx6q-b850v3.dts
+++ b/arch/arm/boot/dts/imx6q-b850v3.dts
@@ -72,6 +72,13 @@
 		fsl,data-mapping = "spwg";
 		fsl,data-width = <24>;
 		status = "okay";
+
+		port@4 {
+			reg = <4>;
+			lvds0_out: endpoint {
+				remote-endpoint = <&b850v3_lvds_dp_bridge_in>;
+			};
+		};
 	};
 };
 
@@ -142,3 +149,26 @@
 		reg = <0x4a>;
 	};
 };
+
+&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_lvds_dp_bridge_in: endpoint {
+				remote-endpoint = <&lvds0_out>;
+			};
+		};
+	};
+};
-- 
2.5.5