[2/4] dt-bindings: Document the Raspberry Pi Touchscreen nodes.

Commit Message

Eric Anholt May 11, 2017, 11:56 p.m. UTC

The Raspberry Pi 7" Touchscreen is a DPI touchscreen panel with
DSI->DPI bridge and touchscreen controller integrated, that connects
to the Raspberry Pi through its 15-pin "DSI" connector (some lines are
DSI, some lines are I2C).

This device is represented in the DT as three nodes (DSI device, I2C
device, panel).  Input will be left to a separate binding later, as it
will be a basic I2C client device.

Signed-off-by: Eric Anholt <[e-mail]>
---
 .../raspberrypi,7inch-touchscreen-bridge.txt       | 68 ++++++++++++++++++++++
 .../panel/raspberrypi,7inch-touchscreen-panel.txt  |  7 +++
 2 files changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchscreen-bridge.txt
 create mode 100644 Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen-panel.txt

Comments

Rob Herring (Arm) May 15, 2017, 8:44 p.m. UTC | #1

On Thu, May 11, 2017 at 04:56:23PM -0700, Eric Anholt wrote:
> The Raspberry Pi 7" Touchscreen is a DPI touchscreen panel with
> DSI->DPI bridge and touchscreen controller integrated, that connects
> to the Raspberry Pi through its 15-pin "DSI" connector (some lines are
> DSI, some lines are I2C).
> 
> This device is represented in the DT as three nodes (DSI device, I2C
> device, panel).  Input will be left to a separate binding later, as it
> will be a basic I2C client device.
> 
> Signed-off-by: Eric Anholt <[e-mail]>
> ---
>  .../raspberrypi,7inch-touchscreen-bridge.txt       | 68 ++++++++++++++++++++++
>  .../panel/raspberrypi,7inch-touchscreen-panel.txt  |  7 +++
>  2 files changed, 75 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchscreen-bridge.txt
>  create mode 100644 Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen-panel.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchscreen-bridge.txt b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchscreen-bridge.txt
> new file mode 100644
> index 000000000000..a5669beaf68f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchscreen-bridge.txt
> @@ -0,0 +1,68 @@
> +Official 7" (800x480) Raspberry Pi touchscreen panel's bridge.
> +
> +This DSI panel contains:
> +
> +- TC358762 DSI->DPI bridge
> +- Atmel microcontroller on I2C for power sequencing the DSI bridge and
> +  controlling backlight
> +- Touchscreen controller on I2C for touch input

This is 1 uC or 2?

> +
> +and this covers the TC358762 bridge and Atmel microcontroller, while
> +../panel/raspberrypi,7inch-touchscreen-panel.txt covers the panel.
> +
> +Required properties:
> +- compatible:	Must be "raspberrypi,7inch-touchscreen-bridge"
> +- raspberrypi,touchscreen-bridge:
> +		Handle to the I2C device for Atmel microcontroller
> +
> +Example:
> +
> +dsi1: dsi@7e700000 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	<...>
> +
> +	lcd-bridge@0 {
> +		compatible = "raspberrypi,7inch-touchscreen-bridge";
> +		reg = <0>;
> +
> +		raspberrypi,touchscreen-bridge = <&pitouchscreen_bridge>;

I think this should be a port with a graph connection from the DSI 
node to the i2c bridge device (and then to the panel).

It's also how other DSI bridges like the tc358767 are done.

> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			port@0 {
> +				reg = <0>;

BTW, you don't need this when there is only 1.

> +				pitouchscreen_bridge_port: endpoint {
> +					remote-endpoint = <&pitouchscreen_panel_port>;
> +				};
> +			};
> +		};
> +	};
> +};
> +
> +i2c_dsi: i2c {
> +	compatible = "i2c-gpio";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	gpios = <&gpio 28 0
> +		 &gpio 29 0>;
> +
> +	pitouchscreen_bridge: bridge@45 {
> +		compatible = "raspberrypi,touchscreen-bridge-i2c";
> +		reg = <0x45>;
> +	};
> +};
> +
> +lcd {
> +	compatible = "raspberrypi,7inch-touchscreen-panel";
> +	ports {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		port@0 {
> +			reg = <0>;
> +			pitouchscreen_panel_port: endpoint {
> +				remote-endpoint = <&pitouchscreen_bridge_port>;
> +			};
> +		};
> +	};
> +};
> diff --git a/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen-panel.txt b/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen-panel.txt
> new file mode 100644
> index 000000000000..1e84f97b3b20
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen-panel.txt
> @@ -0,0 +1,7 @@
> +Official 7" (800x480) Raspberry Pi touchscreen panel's panel.
> +
> +This binding is compatible with the simple-panel binding, which is specified
> +in simple-panel.txt in this directory.
> +
> +Required properties:
> +- compatible:	Must be "raspberrypi,7inch-touchscreen-panel"
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Laurent Pinchart May 15, 2017, 9:53 p.m. UTC | #2

Hi Eric,

Thank you for the patch.

On Thursday 11 May 2017 16:56:23 Eric Anholt wrote:
> The Raspberry Pi 7" Touchscreen is a DPI touchscreen panel with
> DSI->DPI bridge and touchscreen controller integrated, that connects
> to the Raspberry Pi through its 15-pin "DSI" connector (some lines are
> DSI, some lines are I2C).
> 
> This device is represented in the DT as three nodes (DSI device, I2C
> device, panel).  Input will be left to a separate binding later, as it
> will be a basic I2C client device.
> 
> Signed-off-by: Eric Anholt <[e-mail]>
> ---
>  .../raspberrypi,7inch-touchscreen-bridge.txt       | 68 +++++++++++++++++++
>  .../panel/raspberrypi,7inch-touchscreen-panel.txt  |  7 +++
>  2 files changed, 75 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchscr
> een-bridge.txt create mode 100644
> Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscre
> en-panel.txt
> 
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchs
> creen-bridge.txt
> b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchs
> creen-bridge.txt new file mode 100644
> index 000000000000..a5669beaf68f
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchs
> creen-bridge.txt @@ -0,0 +1,68 @@
> +Official 7" (800x480) Raspberry Pi touchscreen panel's bridge.
> +
> +This DSI panel contains:
> +
> +- TC358762 DSI->DPI bridge
> +- Atmel microcontroller on I2C for power sequencing the DSI bridge and
> +  controlling backlight
> +- Touchscreen controller on I2C for touch input
> +
> +and this covers the TC358762 bridge and Atmel microcontroller, while
> +../panel/raspberrypi,7inch-touchscreen-panel.txt covers the panel.

The TC358762 is a standalone bridge that doesn't depend on the ATTiny 
microcontroller used by the RPI. As it's usable standalone, I believe this 
binding should be split in two.

> +Required properties:
> +- compatible:	Must be "raspberrypi,7inch-touchscreen-bridge"
> +- raspberrypi,touchscreen-bridge:
> +		Handle to the I2C device for Atmel microcontroller
> +
> +Example:
> +
> +dsi1: dsi@7e700000 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	<...>
> +
> +	lcd-bridge@0 {
> +		compatible = "raspberrypi,7inch-touchscreen-bridge";
> +		reg = <0>;
> +
> +		raspberrypi,touchscreen-bridge = <&pitouchscreen_bridge>;
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			port@0 {
> +				reg = <0>;
> +				pitouchscreen_bridge_port: endpoint {
> +					remote-endpoint = 
<&pitouchscreen_panel_port>;
> +				};
> +			};
> +		};
> +	};
> +};
> +
> +i2c_dsi: i2c {
> +	compatible = "i2c-gpio";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	gpios = <&gpio 28 0
> +		 &gpio 29 0>;
> +
> +	pitouchscreen_bridge: bridge@45 {
> +		compatible = "raspberrypi,touchscreen-bridge-i2c";
> +		reg = <0x45>;
> +	};
> +};
> +
> +lcd {
> +	compatible = "raspberrypi,7inch-touchscreen-panel";
> +	ports {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		port@0 {
> +			reg = <0>;
> +			pitouchscreen_panel_port: endpoint {
> +				remote-endpoint = 
<&pitouchscreen_bridge_port>;
> +			};
> +		};
> +	};
> +};
> diff --git
> a/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchsc
> reen-panel.txt
> b/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchsc
> reen-panel.txt new file mode 100644
> index 000000000000..1e84f97b3b20
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchsc
> reen-panel.txt @@ -0,0 +1,7 @@
> +Official 7" (800x480) Raspberry Pi touchscreen panel's panel.
> +
> +This binding is compatible with the simple-panel binding, which is
> specified +in simple-panel.txt in this directory.
> +
> +Required properties:
> +- compatible:	Must be "raspberrypi,7inch-touchscreen-panel"

Laurent Pinchart May 15, 2017, 9:56 p.m. UTC | #3

Hi Rob,

On Monday 15 May 2017 15:44:57 Rob Herring wrote:
> On Thu, May 11, 2017 at 04:56:23PM -0700, Eric Anholt wrote:
> > The Raspberry Pi 7" Touchscreen is a DPI touchscreen panel with
> > DSI->DPI bridge and touchscreen controller integrated, that connects
> > to the Raspberry Pi through its 15-pin "DSI" connector (some lines are
> > DSI, some lines are I2C).
> > 
> > This device is represented in the DT as three nodes (DSI device, I2C
> > device, panel).  Input will be left to a separate binding later, as it
> > will be a basic I2C client device.
> > 
> > Signed-off-by: Eric Anholt <[e-mail]>
> > ---
> > 
> >  .../raspberrypi,7inch-touchscreen-bridge.txt       | 68
> >  ++++++++++++++++++++++ .../panel/raspberrypi,7inch-touchscreen-panel.txt
> >   |  7 +++
> >  2 files changed, 75 insertions(+)
> >  create mode 100644
> >  Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touch
> >  screen-bridge.txt create mode 100644
> >  Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchs
> >  creen-panel.txt> 
> > diff --git
> > a/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touc
> > hscreen-bridge.txt
> > b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touc
> > hscreen-bridge.txt new file mode 100644
> > index 000000000000..a5669beaf68f
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touc
> > hscreen-bridge.txt @@ -0,0 +1,68 @@
> > +Official 7" (800x480) Raspberry Pi touchscreen panel's bridge.
> > +
> > +This DSI panel contains:
> > +
> > +- TC358762 DSI->DPI bridge
> > +- Atmel microcontroller on I2C for power sequencing the DSI bridge and
> > +  controlling backlight
> > +- Touchscreen controller on I2C for touch input
> 
> This is 1 uC or 2?
> 
> > +
> > +and this covers the TC358762 bridge and Atmel microcontroller, while
> > +../panel/raspberrypi,7inch-touchscreen-panel.txt covers the panel.
> > +
> > +Required properties:
> > +- compatible:	Must be "raspberrypi,7inch-touchscreen-bridge"
> > +- raspberrypi,touchscreen-bridge:
> > +		Handle to the I2C device for Atmel microcontroller
> > +
> > +Example:
> > +
> > +dsi1: dsi@7e700000 {
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +	<...>
> > +
> > +	lcd-bridge@0 {
> > +		compatible = "raspberrypi,7inch-touchscreen-bridge";
> > +		reg = <0>;
> > +
> > +		raspberrypi,touchscreen-bridge = <&pitouchscreen_bridge>;
> 
> I think this should be a port with a graph connection from the DSI
> node to the i2c bridge device (and then to the panel).

No, this should actually not exist :-) The property references the I2C device 
DT node corresponding to the microcontroller that controls the backlight (and, 
if I understand correctly, handles power sequencing). The DSI driver (in patch 
3/4) then grabs the I2C device and talks to it. I don't think this is right, 
as commented separately on this patch, the bindings should be split, with a 
standalone binding for the TC358762 (and a separate standalone driver too).

> It's also how other DSI bridges like the tc358767 are done.
> 
> > +		ports {
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +			port@0 {
> > +				reg = <0>;
> 
> BTW, you don't need this when there is only 1.
> 
> > +				pitouchscreen_bridge_port: endpoint {
> > +					remote-endpoint = 
<&pitouchscreen_panel_port>;
> > +				};
> > +			};
> > +		};
> > +	};
> > +};
> > +
> > +i2c_dsi: i2c {
> > +	compatible = "i2c-gpio";
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +	gpios = <&gpio 28 0
> > +		 &gpio 29 0>;
> > +
> > +	pitouchscreen_bridge: bridge@45 {
> > +		compatible = "raspberrypi,touchscreen-bridge-i2c";
> > +		reg = <0x45>;
> > +	};
> > +};
> > +
> > +lcd {
> > +	compatible = "raspberrypi,7inch-touchscreen-panel";
> > +	ports {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		port@0 {
> > +			reg = <0>;
> > +			pitouchscreen_panel_port: endpoint {
> > +				remote-endpoint = 
<&pitouchscreen_bridge_port>;
> > +			};
> > +		};
> > +	};
> > +};
> > diff --git
> > a/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touch
> > screen-panel.txt
> > b/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touch
> > screen-panel.txt new file mode 100644
> > index 000000000000..1e84f97b3b20
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touch
> > screen-panel.txt @@ -0,0 +1,7 @@
> > +Official 7" (800x480) Raspberry Pi touchscreen panel's panel.
> > +
> > +This binding is compatible with the simple-panel binding, which is
> > specified +in simple-panel.txt in this directory.
> > +
> > +Required properties:
> > +- compatible:	Must be "raspberrypi,7inch-touchscreen-panel"

Eric Anholt May 16, 2017, 12:03 a.m. UTC | #4

Laurent Pinchart <[e-mail]> writes:

> Hi Eric,
>
> Thank you for the patch.
>
> On Thursday 11 May 2017 16:56:23 Eric Anholt wrote:
>> The Raspberry Pi 7" Touchscreen is a DPI touchscreen panel with
>> DSI->DPI bridge and touchscreen controller integrated, that connects
>> to the Raspberry Pi through its 15-pin "DSI" connector (some lines are
>> DSI, some lines are I2C).
>> 
>> This device is represented in the DT as three nodes (DSI device, I2C
>> device, panel).  Input will be left to a separate binding later, as it
>> will be a basic I2C client device.
>> 
>> Signed-off-by: Eric Anholt <[e-mail]>
>> ---
>>  .../raspberrypi,7inch-touchscreen-bridge.txt       | 68 +++++++++++++++++++
>>  .../panel/raspberrypi,7inch-touchscreen-panel.txt  |  7 +++
>>  2 files changed, 75 insertions(+)
>>  create mode 100644
>> Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchscr
>> een-bridge.txt create mode 100644
>> Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscre
>> en-panel.txt
>> 
>> diff --git
>> a/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchs
>> creen-bridge.txt
>> b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchs
>> creen-bridge.txt new file mode 100644
>> index 000000000000..a5669beaf68f
>> --- /dev/null
>> +++
>> b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchs
>> creen-bridge.txt @@ -0,0 +1,68 @@
>> +Official 7" (800x480) Raspberry Pi touchscreen panel's bridge.
>> +
>> +This DSI panel contains:
>> +
>> +- TC358762 DSI->DPI bridge
>> +- Atmel microcontroller on I2C for power sequencing the DSI bridge and
>> +  controlling backlight
>> +- Touchscreen controller on I2C for touch input
>> +
>> +and this covers the TC358762 bridge and Atmel microcontroller, while
>> +../panel/raspberrypi,7inch-touchscreen-panel.txt covers the panel.
>
> The TC358762 is a standalone bridge that doesn't depend on the ATTiny 
> microcontroller used by the RPI. As it's usable standalone, I believe this 
> binding should be split in two.

Do you have a plan for how I would implement a driver on top of that
binding change, though?  Note that we don't program the Toshiba
directly, we only send commands to the Atmel.

Rob Herring May 16, 2017, 12:11 a.m. UTC | #5

On Mon, May 15, 2017 at 7:03 PM, Eric Anholt <[e-mail]> wrote:
> Laurent Pinchart <[e-mail]> writes:
>
>> Hi Eric,
>>
>> Thank you for the patch.
>>
>> On Thursday 11 May 2017 16:56:23 Eric Anholt wrote:
>>> The Raspberry Pi 7" Touchscreen is a DPI touchscreen panel with
>>> DSI->DPI bridge and touchscreen controller integrated, that connects
>>> to the Raspberry Pi through its 15-pin "DSI" connector (some lines are
>>> DSI, some lines are I2C).
>>>
>>> This device is represented in the DT as three nodes (DSI device, I2C
>>> device, panel).  Input will be left to a separate binding later, as it
>>> will be a basic I2C client device.
>>>
>>> Signed-off-by: Eric Anholt <[e-mail]>
>>> ---
>>>  .../raspberrypi,7inch-touchscreen-bridge.txt       | 68 +++++++++++++++++++
>>>  .../panel/raspberrypi,7inch-touchscreen-panel.txt  |  7 +++
>>>  2 files changed, 75 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchscr
>>> een-bridge.txt create mode 100644
>>> Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscre
>>> en-panel.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchs
>>> creen-bridge.txt
>>> b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchs
>>> creen-bridge.txt new file mode 100644
>>> index 000000000000..a5669beaf68f
>>> --- /dev/null
>>> +++
>>> b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchs
>>> creen-bridge.txt @@ -0,0 +1,68 @@
>>> +Official 7" (800x480) Raspberry Pi touchscreen panel's bridge.
>>> +
>>> +This DSI panel contains:
>>> +
>>> +- TC358762 DSI->DPI bridge
>>> +- Atmel microcontroller on I2C for power sequencing the DSI bridge and
>>> +  controlling backlight
>>> +- Touchscreen controller on I2C for touch input
>>> +
>>> +and this covers the TC358762 bridge and Atmel microcontroller, while
>>> +../panel/raspberrypi,7inch-touchscreen-panel.txt covers the panel.
>>
>> The TC358762 is a standalone bridge that doesn't depend on the ATTiny
>> microcontroller used by the RPI. As it's usable standalone, I believe this
>> binding should be split in two.
>
> Do you have a plan for how I would implement a driver on top of that
> binding change, though?  Note that we don't program the Toshiba
> directly, we only send commands to the Atmel.

I agree. If it is a black box and the interface to the host is defined
by the Atmel uC firmware, then that's what the DT should describe.
Perhaps a diagram here or pointer to one would help and remove
mentioning what kind of bridge chip it is.

Rob

Laurent Pinchart May 16, 2017, 7:20 a.m. UTC | #6

Hi Eric,

On Monday 15 May 2017 17:03:29 Eric Anholt wrote:
> Laurent Pinchart <[e-mail]> writes:
> > On Thursday 11 May 2017 16:56:23 Eric Anholt wrote:
> >> The Raspberry Pi 7" Touchscreen is a DPI touchscreen panel with
> >> DSI->DPI bridge and touchscreen controller integrated, that connects
> >> to the Raspberry Pi through its 15-pin "DSI" connector (some lines are
> >> DSI, some lines are I2C).
> >> 
> >> This device is represented in the DT as three nodes (DSI device, I2C
> >> device, panel).  Input will be left to a separate binding later, as it
> >> will be a basic I2C client device.
> >> 
> >> Signed-off-by: Eric Anholt <[e-mail]>
> >> ---
> >> 
> >>  .../raspberrypi,7inch-touchscreen-bridge.txt       | 68 +++++++++++++++
> >>  .../panel/raspberrypi,7inch-touchscreen-panel.txt  |  7 +++
> >>  2 files changed, 75 insertions(+)
> >>  create mode 100644
> >> 
> >> Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchs
> >> cr
> >> een-bridge.txt create mode 100644
> >> Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchsc
> >> re
> >> en-panel.txt
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touc
> >> hs
> >> creen-bridge.txt
> >> b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touc
> >> hs
> >> creen-bridge.txt new file mode 100644
> >> index 000000000000..a5669beaf68f
> >> --- /dev/null
> >> +++
> >> b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touc
> >> hs
> >> creen-bridge.txt @@ -0,0 +1,68 @@
> >> +Official 7" (800x480) Raspberry Pi touchscreen panel's bridge.
> >> +
> >> +This DSI panel contains:
> >> +
> >> +- TC358762 DSI->DPI bridge
> >> +- Atmel microcontroller on I2C for power sequencing the DSI bridge and
> >> +  controlling backlight
> >> +- Touchscreen controller on I2C for touch input
> >> +
> >> +and this covers the TC358762 bridge and Atmel microcontroller, while
> >> +../panel/raspberrypi,7inch-touchscreen-panel.txt covers the panel.
> > 
> > The TC358762 is a standalone bridge that doesn't depend on the ATTiny
> > microcontroller used by the RPI. As it's usable standalone, I believe this
> > binding should be split in two.
> 
> Do you have a plan for how I would implement a driver on top of that
> binding change, though?  Note that we don't program the Toshiba
> directly, we only send commands to the Atmel.

I'm not too familiar with the hardware architecture, let me have a look. Is 
the schematics publicly available somewhere ?

Eric Anholt May 16, 2017, 4:47 p.m. UTC | #7

Rob Herring <[e-mail]> writes:

> On Mon, May 15, 2017 at 7:03 PM, Eric Anholt <[e-mail]> wrote:
>> Laurent Pinchart <[e-mail]> writes:
>>
>>> Hi Eric,
>>>
>>> Thank you for the patch.
>>>
>>> On Thursday 11 May 2017 16:56:23 Eric Anholt wrote:
>>>> The Raspberry Pi 7" Touchscreen is a DPI touchscreen panel with
>>>> DSI->DPI bridge and touchscreen controller integrated, that connects
>>>> to the Raspberry Pi through its 15-pin "DSI" connector (some lines are
>>>> DSI, some lines are I2C).
>>>>
>>>> This device is represented in the DT as three nodes (DSI device, I2C
>>>> device, panel).  Input will be left to a separate binding later, as it
>>>> will be a basic I2C client device.
>>>>
>>>> Signed-off-by: Eric Anholt <[e-mail]>
>>>> ---
>>>>  .../raspberrypi,7inch-touchscreen-bridge.txt       | 68 +++++++++++++++++++
>>>>  .../panel/raspberrypi,7inch-touchscreen-panel.txt  |  7 +++
>>>>  2 files changed, 75 insertions(+)
>>>>  create mode 100644
>>>> Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchscr
>>>> een-bridge.txt create mode 100644
>>>> Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscre
>>>> en-panel.txt
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchs
>>>> creen-bridge.txt
>>>> b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchs
>>>> creen-bridge.txt new file mode 100644
>>>> index 000000000000..a5669beaf68f
>>>> --- /dev/null
>>>> +++
>>>> b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchs
>>>> creen-bridge.txt @@ -0,0 +1,68 @@
>>>> +Official 7" (800x480) Raspberry Pi touchscreen panel's bridge.
>>>> +
>>>> +This DSI panel contains:
>>>> +
>>>> +- TC358762 DSI->DPI bridge
>>>> +- Atmel microcontroller on I2C for power sequencing the DSI bridge and
>>>> +  controlling backlight
>>>> +- Touchscreen controller on I2C for touch input
>>>> +
>>>> +and this covers the TC358762 bridge and Atmel microcontroller, while
>>>> +../panel/raspberrypi,7inch-touchscreen-panel.txt covers the panel.
>>>
>>> The TC358762 is a standalone bridge that doesn't depend on the ATTiny
>>> microcontroller used by the RPI. As it's usable standalone, I believe this
>>> binding should be split in two.
>>
>> Do you have a plan for how I would implement a driver on top of that
>> binding change, though?  Note that we don't program the Toshiba
>> directly, we only send commands to the Atmel.
>
> I agree. If it is a black box and the interface to the host is defined
> by the Atmel uC firmware, then that's what the DT should describe.
> Perhaps a diagram here or pointer to one would help and remove
> mentioning what kind of bridge chip it is.

It's a *very* black box.  I have some non-public schematics that don't
even say what panel is involved, and no documentation of the uc
interface.  The driver code is just replicating the firmware's
programming sequence.

I would certainly love to be building a generic TC358762 driver, which
would be a lot more satisfying.  I just don't think it's doable for this
panel.  Given that, what do I need to do to the DT?  Should I just drop
mention of the Toshiba and talk about this being a bridge with a custom
microcontroller firmware?

Laurent Pinchart May 16, 2017, 4:54 p.m. UTC | #8

Hi Eric,

On Tuesday 16 May 2017 09:47:49 Eric Anholt wrote:
> Rob Herring <[e-mail]> writes:
> > On Mon, May 15, 2017 at 7:03 PM, Eric Anholt <[e-mail]> wrote:
> >> Laurent Pinchart <[e-mail]> writes:
> >>> Hi Eric,
> >>> 
> >>> Thank you for the patch.
> >>> 
> >>> On Thursday 11 May 2017 16:56:23 Eric Anholt wrote:
> >>>> The Raspberry Pi 7" Touchscreen is a DPI touchscreen panel with
> >>>> DSI->DPI bridge and touchscreen controller integrated, that connects
> >>>> to the Raspberry Pi through its 15-pin "DSI" connector (some lines are
> >>>> DSI, some lines are I2C).
> >>>> 
> >>>> This device is represented in the DT as three nodes (DSI device, I2C
> >>>> device, panel).  Input will be left to a separate binding later, as it
> >>>> will be a basic I2C client device.
> >>>> 
> >>>> Signed-off-by: Eric Anholt <[e-mail]>
> >>>> ---
> >>>> 
> >>>>  .../raspberrypi,7inch-touchscreen-bridge.txt       | 68 ++++++++++++++
> >>>>  .../panel/raspberrypi,7inch-touchscreen-panel.txt  |  7 +++
> >>>>  2 files changed, 75 insertions(+)
> >>>>  create mode 100644
> >>>> 
> >>>> Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touc
> >>>> hscreen-bridge.txt create mode 100644
> >>>> Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touch
> >>>> screen-panel.txt
> >>>> 
> >>>> diff --git
> >>>> a/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-to
> >>>> uchscreen-bridge.txt
> >>>> b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-to
> >>>> uchscreen-bridge.txt new file mode 100644
> >>>> index 000000000000..a5669beaf68f
> >>>> --- /dev/null
> >>>> +++
> >>>> b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-to
> >>>> uchscreen-bridge.txt
> >>>> @@ -0,0 +1,68 @@
> >>>> +Official 7" (800x480) Raspberry Pi touchscreen panel's bridge.
> >>>> +
> >>>> +This DSI panel contains:
> >>>> +
> >>>> +- TC358762 DSI->DPI bridge
> >>>> +- Atmel microcontroller on I2C for power sequencing the DSI bridge and
> >>>> +  controlling backlight
> >>>> +- Touchscreen controller on I2C for touch input
> >>>> +
> >>>> +and this covers the TC358762 bridge and Atmel microcontroller, while
> >>>> +../panel/raspberrypi,7inch-touchscreen-panel.txt covers the panel.
> >>> 
> >>> The TC358762 is a standalone bridge that doesn't depend on the ATTiny
> >>> microcontroller used by the RPI. As it's usable standalone, I believe
> >>> this binding should be split in two.
> >> 
> >> Do you have a plan for how I would implement a driver on top of that
> >> binding change, though?  Note that we don't program the Toshiba
> >> directly, we only send commands to the Atmel.
> > 
> > I agree. If it is a black box and the interface to the host is defined
> > by the Atmel uC firmware, then that's what the DT should describe.
> > Perhaps a diagram here or pointer to one would help and remove
> > mentioning what kind of bridge chip it is.
> 
> It's a *very* black box.  I have some non-public schematics that don't
> even say what panel is involved, and no documentation of the uc
> interface.  The driver code is just replicating the firmware's
> programming sequence.
> 
> I would certainly love to be building a generic TC358762 driver, which
> would be a lot more satisfying.  I just don't think it's doable for this
> panel.  Given that, what do I need to do to the DT?  Should I just drop
> mention of the Toshiba and talk about this being a bridge with a custom
> microcontroller firmware?

I think that would be best, yes. Could you share a simple block-diagram of the 
hardware ? It would help turning my random advices into semi-random advices 
:-)

Eric Anholt May 16, 2017, 6:46 p.m. UTC | #9

Laurent Pinchart <[e-mail]> writes:

> Hi Eric,
>
> On Tuesday 16 May 2017 09:47:49 Eric Anholt wrote:
>> Rob Herring <[e-mail]> writes:
>> > On Mon, May 15, 2017 at 7:03 PM, Eric Anholt <[e-mail]> wrote:
>> >> Laurent Pinchart <[e-mail]> writes:
>> >>> Hi Eric,
>> >>> 
>> >>> Thank you for the patch.
>> >>> 
>> >>> On Thursday 11 May 2017 16:56:23 Eric Anholt wrote:
>> >>>> The Raspberry Pi 7" Touchscreen is a DPI touchscreen panel with
>> >>>> DSI->DPI bridge and touchscreen controller integrated, that connects
>> >>>> to the Raspberry Pi through its 15-pin "DSI" connector (some lines are
>> >>>> DSI, some lines are I2C).
>> >>>> 
>> >>>> This device is represented in the DT as three nodes (DSI device, I2C
>> >>>> device, panel).  Input will be left to a separate binding later, as it
>> >>>> will be a basic I2C client device.
>> >>>> 
>> >>>> Signed-off-by: Eric Anholt <[e-mail]>
>> >>>> ---
>> >>>> 
>> >>>>  .../raspberrypi,7inch-touchscreen-bridge.txt       | 68 ++++++++++++++
>> >>>>  .../panel/raspberrypi,7inch-touchscreen-panel.txt  |  7 +++
>> >>>>  2 files changed, 75 insertions(+)
>> >>>>  create mode 100644
>> >>>> 
>> >>>> Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touc
>> >>>> hscreen-bridge.txt create mode 100644
>> >>>> Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touch
>> >>>> screen-panel.txt
>> >>>> 
>> >>>> diff --git
>> >>>> a/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-to
>> >>>> uchscreen-bridge.txt
>> >>>> b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-to
>> >>>> uchscreen-bridge.txt new file mode 100644
>> >>>> index 000000000000..a5669beaf68f
>> >>>> --- /dev/null
>> >>>> +++
>> >>>> b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-to
>> >>>> uchscreen-bridge.txt
>> >>>> @@ -0,0 +1,68 @@
>> >>>> +Official 7" (800x480) Raspberry Pi touchscreen panel's bridge.
>> >>>> +
>> >>>> +This DSI panel contains:
>> >>>> +
>> >>>> +- TC358762 DSI->DPI bridge
>> >>>> +- Atmel microcontroller on I2C for power sequencing the DSI bridge and
>> >>>> +  controlling backlight
>> >>>> +- Touchscreen controller on I2C for touch input
>> >>>> +
>> >>>> +and this covers the TC358762 bridge and Atmel microcontroller, while
>> >>>> +../panel/raspberrypi,7inch-touchscreen-panel.txt covers the panel.
>> >>> 
>> >>> The TC358762 is a standalone bridge that doesn't depend on the ATTiny
>> >>> microcontroller used by the RPI. As it's usable standalone, I believe
>> >>> this binding should be split in two.
>> >> 
>> >> Do you have a plan for how I would implement a driver on top of that
>> >> binding change, though?  Note that we don't program the Toshiba
>> >> directly, we only send commands to the Atmel.
>> > 
>> > I agree. If it is a black box and the interface to the host is defined
>> > by the Atmel uC firmware, then that's what the DT should describe.
>> > Perhaps a diagram here or pointer to one would help and remove
>> > mentioning what kind of bridge chip it is.
>> 
>> It's a *very* black box.  I have some non-public schematics that don't
>> even say what panel is involved, and no documentation of the uc
>> interface.  The driver code is just replicating the firmware's
>> programming sequence.
>> 
>> I would certainly love to be building a generic TC358762 driver, which
>> would be a lot more satisfying.  I just don't think it's doable for this
>> panel.  Given that, what do I need to do to the DT?  Should I just drop
>> mention of the Toshiba and talk about this being a bridge with a custom
>> microcontroller firmware?
>
> I think that would be best, yes. Could you share a simple block-diagram of the 
> hardware ? It would help turning my random advices into semi-random advices 
> :-)

In terms of physical connections:

   [15-pin "DSI" connector on 2835]
    |                   |
    | I2C               | DSI
    |                   |
   / \        SPI       |
[TS]  [Atmel]------[TC358762]
       \                |
        \PWM            |
         \              | DPI
[some backlight]------[some unknown panel]

The binding I'm trying to create is to expose what's necessary for a
driver that talks I2C to the Atmel, which then controls the PWM and does
the command sequence over SPI to the Toshiba that sets up its end of the
DSI link.

Archit Taneja May 18, 2017, 8:26 a.m. UTC | #10

Hi,

On 05/17/2017 12:16 AM, Eric Anholt wrote:
> Laurent Pinchart <[e-mail]> writes:
>
>> Hi Eric,
>>
>> On Tuesday 16 May 2017 09:47:49 Eric Anholt wrote:
>>> Rob Herring <[e-mail]> writes:
>>>> On Mon, May 15, 2017 at 7:03 PM, Eric Anholt <[e-mail]> wrote:
>>>>> Laurent Pinchart <[e-mail]> writes:
>>>>>> Hi Eric,
>>>>>>
>>>>>> Thank you for the patch.
>>>>>>
>>>>>> On Thursday 11 May 2017 16:56:23 Eric Anholt wrote:
>>>>>>> The Raspberry Pi 7" Touchscreen is a DPI touchscreen panel with
>>>>>>> DSI->DPI bridge and touchscreen controller integrated, that connects
>>>>>>> to the Raspberry Pi through its 15-pin "DSI" connector (some lines are
>>>>>>> DSI, some lines are I2C).
>>>>>>>
>>>>>>> This device is represented in the DT as three nodes (DSI device, I2C
>>>>>>> device, panel).  Input will be left to a separate binding later, as it
>>>>>>> will be a basic I2C client device.
>>>>>>>
>>>>>>> Signed-off-by: Eric Anholt <[e-mail]>
>>>>>>> ---
>>>>>>>
>>>>>>>  .../raspberrypi,7inch-touchscreen-bridge.txt       | 68 ++++++++++++++
>>>>>>>  .../panel/raspberrypi,7inch-touchscreen-panel.txt  |  7 +++
>>>>>>>  2 files changed, 75 insertions(+)
>>>>>>>  create mode 100644
>>>>>>>
>>>>>>> Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touc
>>>>>>> hscreen-bridge.txt create mode 100644
>>>>>>> Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touch
>>>>>>> screen-panel.txt
>>>>>>>
>>>>>>> diff --git
>>>>>>> a/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-to
>>>>>>> uchscreen-bridge.txt
>>>>>>> b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-to
>>>>>>> uchscreen-bridge.txt new file mode 100644
>>>>>>> index 000000000000..a5669beaf68f
>>>>>>> --- /dev/null
>>>>>>> +++
>>>>>>> b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-to
>>>>>>> uchscreen-bridge.txt
>>>>>>> @@ -0,0 +1,68 @@
>>>>>>> +Official 7" (800x480) Raspberry Pi touchscreen panel's bridge.
>>>>>>> +
>>>>>>> +This DSI panel contains:
>>>>>>> +
>>>>>>> +- TC358762 DSI->DPI bridge
>>>>>>> +- Atmel microcontroller on I2C for power sequencing the DSI bridge and
>>>>>>> +  controlling backlight
>>>>>>> +- Touchscreen controller on I2C for touch input
>>>>>>> +
>>>>>>> +and this covers the TC358762 bridge and Atmel microcontroller, while
>>>>>>> +../panel/raspberrypi,7inch-touchscreen-panel.txt covers the panel.
>>>>>>
>>>>>> The TC358762 is a standalone bridge that doesn't depend on the ATTiny
>>>>>> microcontroller used by the RPI. As it's usable standalone, I believe
>>>>>> this binding should be split in two.
>>>>>
>>>>> Do you have a plan for how I would implement a driver on top of that
>>>>> binding change, though?  Note that we don't program the Toshiba
>>>>> directly, we only send commands to the Atmel.
>>>>
>>>> I agree. If it is a black box and the interface to the host is defined
>>>> by the Atmel uC firmware, then that's what the DT should describe.
>>>> Perhaps a diagram here or pointer to one would help and remove
>>>> mentioning what kind of bridge chip it is.
>>>
>>> It's a *very* black box.  I have some non-public schematics that don't
>>> even say what panel is involved, and no documentation of the uc
>>> interface.  The driver code is just replicating the firmware's
>>> programming sequence.
>>>
>>> I would certainly love to be building a generic TC358762 driver, which
>>> would be a lot more satisfying.  I just don't think it's doable for this
>>> panel.  Given that, what do I need to do to the DT?  Should I just drop
>>> mention of the Toshiba and talk about this being a bridge with a custom
>>> microcontroller firmware?
>>
>> I think that would be best, yes. Could you share a simple block-diagram of the
>> hardware ? It would help turning my random advices into semi-random advices
>> :-)
>
> In terms of physical connections:
>
>    [15-pin "DSI" connector on 2835]
>     |                   |
>     | I2C               | DSI
>     |                   |
>    / \        SPI       |
> [TS]  [Atmel]------[TC358762]
>        \                |
>         \PWM            |
>          \              | DPI
> [some backlight]------[some unknown panel]
>
> The binding I'm trying to create is to expose what's necessary for a
> driver that talks I2C to the Atmel, which then controls the PWM and does
> the command sequence over SPI to the Toshiba that sets up its end of the
> DSI link.
>

The bridge (Atmel + TC358762 combination) here looks like it's primarily
an i2c device (i.e, the control bus is i2c). Therefore, the drm-bridge
driver here should be an i2c driver instead of a mipi_dsi_driver.

We have the facility to create a mipi DSI device without the need to have
a corresponding node in DT. The ADV7533 and TC358767 drivers are examples
of that.

The following is what the binding could look like, it's same as what Rob
also mentioned previously in the thread.

Thanks,
Archit

dsi1: dsi@7e700000 {
	#address-cells = <1>;
	#size-cells = <0>;
	<...>

	/* The SoC's DSI input/output port */
	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		/* port@0 if needed */

		port@1 {
			dsi_out_port: endpoint {
				reg = <1>;
				remote-endpoint = <&bridge_dsi_port>;
			};
		};
	};
};

i2c_dsi: i2c {
	compatible = "i2c-gpio";
	#address-cells = <1>;
	#size-cells = <0>;
	gpios = <&gpio 28 0
		 &gpio 29 0>;

	/* the Atmel + TC35872 bridge */
	pitouchscreen_bridge: bridge@45 {
		compatible = "raspberrypi,touchscreen-bridge";
		reg = <0x45>;

		ports {
			#address-cells = <1>;
			#size-cells = <0>;

			port@0 {
				reg = <0>;
				bridge_dsi_port: endpoint {
					remote-endpoint = <&dsi_out_port>;
				};
			};
			port@1 {
				reg = <1>;
				bridge_dpi_port: endpoint {
					remote-endpoint = <&pitouchscreen_panel_port>;
				};
			};
		};
	};
};

lcd {
	compatible = "raspberrypi,7inch-touchscreen-panel";
	ports {
		#address-cells = <1>;
		#size-cells = <0>;
		port@0 {
			reg = <0>;
			pitouchscreen_panel_port: endpoint {
				remote-endpoint = <&bridge_dpi_port>;
			};
		};
	};
};

Laurent Pinchart May 18, 2017, 2:45 p.m. UTC | #11

Hi Eric,

On Tuesday 16 May 2017 11:46:36 Eric Anholt wrote:

[snip]

> In terms of physical connections:
> 
>    [15-pin "DSI" connector on 2835]
> 
>     | I2C               | DSI
>    / \        SPI       |
> [TS]  [Atmel]------[TC358762]
>        \                |
>         \PWM            |
>          \              | DPI
> [some backlight]------[some unknown panel]
> 
> The binding I'm trying to create is to expose what's necessary for a
> driver that talks I2C to the Atmel, which then controls the PWM and does
> the command sequence over SPI to the Toshiba that sets up its end of the
> DSI link.

According to the documentation I've been able to find, the TC358762 has an SPI 
master port through which it can output the commands DCS received from the DSI 
port, and an I2C slave port through which it can be configured by an external 
device. If the connection between the microcontroller and the TC358762 is 
indeed SPI and not I2C, I assume it's used by the microcontroller to receive 
the DCS commands and perform control of the backlight (and possibly other 
components) accordingly. By the way, is there any place where I can find a 
leaked version of the non-public panel schematics ? ;-)

As far as I can tell from your patch series, you don't need to send any 
command to the TC358762 over DSI. In that case I would model the panel in DT 
as an I2C device, as all control goes through the I2C bus. The DSI video data 
connection should then be modelled using the OF graph DT bindings. The result 
will be a black box panel with a custom black box panel driver, using a single 
DT node. There's no need for a separate bridge instance. That's the cleanest 
option I can come up with so far, and I agree that splitting TC358762 support 
into a standalone bridge driver makes no sense in this case.

Laurent Pinchart May 18, 2017, 2:55 p.m. UTC | #12

Hi Archit,

On Thursday 18 May 2017 13:56:19 Archit Taneja wrote:
> On 05/17/2017 12:16 AM, Eric Anholt wrote:

[snip]

> > In terms of physical connections:
> >    [15-pin "DSI" connector on 2835]
> >    
> >     | I2C               | DSI
> >    
> >    / \        SPI       |
> > 
> > [TS]  [Atmel]------[TC358762]
> > 
> >        \                |
> >        
> >         \PWM            |
> >         
> >          \              | DPI
> > 
> > [some backlight]------[some unknown panel]
> > 
> > The binding I'm trying to create is to expose what's necessary for a
> > driver that talks I2C to the Atmel, which then controls the PWM and does
> > the command sequence over SPI to the Toshiba that sets up its end of the
> > DSI link.
> 
> The bridge (Atmel + TC358762 combination) here looks like it's primarily
> an i2c device (i.e, the control bus is i2c). Therefore, the drm-bridge
> driver here should be an i2c driver instead of a mipi_dsi_driver.

Glad to see we agree, that's what I've proposed in a separate answer :-) I'd 
go one step further though, there should be no DRM bridge, just a DRM panel.

> We have the facility to create a mipi DSI device without the need to have
> a corresponding node in DT. The ADV7533 and TC358767 drivers are examples
> of that.
> 
> The following is what the binding could look like, it's same as what Rob
> also mentioned previously in the thread.
> 
> Thanks,
> Archit
> 
> dsi1: dsi@7e700000 {
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 	<...>
> 
> 	/* The SoC's DSI input/output port */
> 	ports {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		/* port@0 if needed */
> 
> 		port@1 {
> 			dsi_out_port: endpoint {
> 				reg = <1>;
> 				remote-endpoint = <&bridge_dsi_port>;
> 			};
> 		};
> 	};
> };
> 
> i2c_dsi: i2c {
> 	compatible = "i2c-gpio";
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 	gpios = <&gpio 28 0
> 		 &gpio 29 0>;
> 
> 	/* the Atmel + TC35872 bridge */
> 	pitouchscreen_bridge: bridge@45 {

This should thus be lcd@45.

> 		compatible = "raspberrypi,touchscreen-bridge";

And this raspberrypi,7inch-touchscreen-panel. Shame we haven't standardized 
the vendor name prefix to rpi :-/

> 		reg = <0x45>;
> 
> 		ports {
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> 			port@0 {
> 				reg = <0>;
> 				bridge_dsi_port: endpoint {

This should be named panel_dsi_port.

> 					remote-endpoint = <&dsi_out_port>;
> 				};
> 			};
> 			port@1 {
> 				reg = <1>;
> 				bridge_dpi_port: endpoint {
> 					remote-endpoint = 
<&pitouchscreen_panel_port>;
> 				};
> 			};

The second port is thus not needed.

> 		};

So we can simplify this to

		port {
			panel_dsi_port: endpoint {
				remote-endpoint = <&dsi_out_port>;
			};
		};

(no need for a ports node when there's a single port)

> 	};
> };
> 
> lcd {
> 	compatible = "raspberrypi,7inch-touchscreen-panel";
> 	ports {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 		port@0 {
> 			reg = <0>;
> 			pitouchscreen_panel_port: endpoint {
> 				remote-endpoint = <&bridge_dpi_port>;
> 			};
> 		};
> 	};
> };

And this node can go away.

Archit Taneja May 19, 2017, 8:54 a.m. UTC | #13

On 05/18/2017 08:25 PM, Laurent Pinchart wrote:
> Hi Archit,
>
> On Thursday 18 May 2017 13:56:19 Archit Taneja wrote:
>> On 05/17/2017 12:16 AM, Eric Anholt wrote:
>
> [snip]
>
>>> In terms of physical connections:
>>>    [15-pin "DSI" connector on 2835]
>>>
>>>     | I2C               | DSI
>>>
>>>    / \        SPI       |
>>>
>>> [TS]  [Atmel]------[TC358762]
>>>
>>>        \                |
>>>
>>>         \PWM            |
>>>
>>>          \              | DPI
>>>
>>> [some backlight]------[some unknown panel]
>>>
>>> The binding I'm trying to create is to expose what's necessary for a
>>> driver that talks I2C to the Atmel, which then controls the PWM and does
>>> the command sequence over SPI to the Toshiba that sets up its end of the
>>> DSI link.
>>
>> The bridge (Atmel + TC358762 combination) here looks like it's primarily
>> an i2c device (i.e, the control bus is i2c). Therefore, the drm-bridge
>> driver here should be an i2c driver instead of a mipi_dsi_driver.
>
> Glad to see we agree, that's what I've proposed in a separate answer :-) I'd
> go one step further though, there should be no DRM bridge, just a DRM panel.

If the PCB containing the controller chips and the panel are part of a single
casing, and the set up won't work with another panel, then yeah, I agree. If the
bridge chips are on a separate adapter board, and there is a possibility to connect
other panels, then maybe a separate DRM bridge and a DRM panel might be a safer bet.

Thanks,
Archit

>
>> We have the facility to create a mipi DSI device without the need to have
>> a corresponding node in DT. The ADV7533 and TC358767 drivers are examples
>> of that.
>>
>> The following is what the binding could look like, it's same as what Rob
>> also mentioned previously in the thread.
>>
>> Thanks,
>> Archit
>>
>> dsi1: dsi@7e700000 {
>> 	#address-cells = <1>;
>> 	#size-cells = <0>;
>> 	<...>
>>
>> 	/* The SoC's DSI input/output port */
>> 	ports {
>> 		#address-cells = <1>;
>> 		#size-cells = <0>;
>>
>> 		/* port@0 if needed */
>>
>> 		port@1 {
>> 			dsi_out_port: endpoint {
>> 				reg = <1>;
>> 				remote-endpoint = <&bridge_dsi_port>;
>> 			};
>> 		};
>> 	};
>> };
>>
>> i2c_dsi: i2c {
>> 	compatible = "i2c-gpio";
>> 	#address-cells = <1>;
>> 	#size-cells = <0>;
>> 	gpios = <&gpio 28 0
>> 		 &gpio 29 0>;
>>
>> 	/* the Atmel + TC35872 bridge */
>> 	pitouchscreen_bridge: bridge@45 {
>
> This should thus be lcd@45.
>
>> 		compatible = "raspberrypi,touchscreen-bridge";
>
> And this raspberrypi,7inch-touchscreen-panel. Shame we haven't standardized
> the vendor name prefix to rpi :-/
>
>> 		reg = <0x45>;
>>
>> 		ports {
>> 			#address-cells = <1>;
>> 			#size-cells = <0>;
>>
>> 			port@0 {
>> 				reg = <0>;
>> 				bridge_dsi_port: endpoint {
>
> This should be named panel_dsi_port.
>
>> 					remote-endpoint = <&dsi_out_port>;
>> 				};
>> 			};
>> 			port@1 {
>> 				reg = <1>;
>> 				bridge_dpi_port: endpoint {
>> 					remote-endpoint =
> <&pitouchscreen_panel_port>;
>> 				};
>> 			};
>
> The second port is thus not needed.
>
>> 		};
>
> So we can simplify this to
>
> 		port {
> 			panel_dsi_port: endpoint {
> 				remote-endpoint = <&dsi_out_port>;
> 			};
> 		};
>
> (no need for a ports node when there's a single port)
>
>> 	};
>> };
>>
>> lcd {
>> 	compatible = "raspberrypi,7inch-touchscreen-panel";
>> 	ports {
>> 		#address-cells = <1>;
>> 		#size-cells = <0>;
>> 		port@0 {
>> 			reg = <0>;
>> 			pitouchscreen_panel_port: endpoint {
>> 				remote-endpoint = <&bridge_dpi_port>;
>> 			};
>> 		};
>> 	};
>> };
>
> And this node can go away.
>

Laurent Pinchart May 19, 2017, 9:32 a.m. UTC | #14

Hi Archit,

On Friday 19 May 2017 14:24:36 Archit Taneja wrote:
> On 05/18/2017 08:25 PM, Laurent Pinchart wrote:
> > On Thursday 18 May 2017 13:56:19 Archit Taneja wrote:
> >> On 05/17/2017 12:16 AM, Eric Anholt wrote:
> >
> > [snip]
> > 
> >>> In terms of physical connections:
> >>>    [15-pin "DSI" connector on 2835]
> >>>     | I2C               | DSI
> >>>    / \        SPI       |
> >>> [TS]  [Atmel]------[TC358762]
> >>>        \                |
> >>>         \PWM            |
> >>>          \              | DPI
> >>> 
> >>> [some backlight]------[some unknown panel]
> >>> 
> >>> The binding I'm trying to create is to expose what's necessary for a
> >>> driver that talks I2C to the Atmel, which then controls the PWM and does
> >>> the command sequence over SPI to the Toshiba that sets up its end of the
> >>> DSI link.
> >> 
> >> The bridge (Atmel + TC358762 combination) here looks like it's primarily
> >> an i2c device (i.e, the control bus is i2c). Therefore, the drm-bridge
> >> driver here should be an i2c driver instead of a mipi_dsi_driver.
> > 
> > Glad to see we agree, that's what I've proposed in a separate answer :-)
> > I'd go one step further though, there should be no DRM bridge, just a DRM
> > panel.
>
> If the PCB containing the controller chips and the panel are part of a
> single casing, and the set up won't work with another panel, then yeah, I
> agree. If the bridge chips are on a separate adapter board, and there is a
> possibility to connect other panels, then maybe a separate DRM bridge and a
> DRM panel might be a safer bet.

I thought it was a single black box, but upon closer inspection there's a 
separate PCB with the Microcontroller and TC358762.

Eric, do you know if it's possible to exchange the panel for another one (and 
not just an model with identical features from another vendor, but another 
panel with a different mode for instance) without reprogramming the 
microcontroller, or is the bridge board tied to the panel model ?

> >> We have the facility to create a mipi DSI device without the need to have
> >> a corresponding node in DT. The ADV7533 and TC358767 drivers are examples
> >> of that.
> >> 
> >> The following is what the binding could look like, it's same as what Rob
> >> also mentioned previously in the thread.
> >> 
> >> Thanks,
> >> Archit
> >> 
> >> dsi1: dsi@7e700000 {
> >> 
> >> 	#address-cells = <1>;
> >> 	#size-cells = <0>;
> >> 	<...>
> >> 	
> >> 	/* The SoC's DSI input/output port */
> >> 	ports {
> >> 		#address-cells = <1>;
> >> 		#size-cells = <0>;
> >> 		
> >> 		/* port@0 if needed */
> >> 		port@1 {
> >> 			dsi_out_port: endpoint {
> >> 				reg = <1>;
> >> 				remote-endpoint = <&bridge_dsi_port>;
> >> 			};
> >> 		};
> >> 	};
> >> };
> >> 
> >> i2c_dsi: i2c {
> >> 	compatible = "i2c-gpio";
> >> 	#address-cells = <1>;
> >> 	#size-cells = <0>;
> >> 	gpios = <&gpio 28 0
> >> 		 &gpio 29 0>;
> >> 	
> >> 	/* the Atmel + TC35872 bridge */
> >> 	pitouchscreen_bridge: bridge@45 {
> > 
> > This should thus be lcd@45.
> > 
> >> 		compatible = "raspberrypi,touchscreen-bridge";
> > 
> > And this raspberrypi,7inch-touchscreen-panel. Shame we haven't
> > standardized
> > the vendor name prefix to rpi :-/
> > 
> >> 		reg = <0x45>;
> >> 		
> >> 		ports {
> >> 			#address-cells = <1>;
> >> 			#size-cells = <0>;
> >> 			port@0 {
> >> 				reg = <0>;
> >> 				bridge_dsi_port: endpoint {
> > 
> > This should be named panel_dsi_port.
> > 
> >> 					remote-endpoint = <&dsi_out_port>;
> >> 				};
> >> 			};
> >> 			port@1 {
> >> 				reg = <1>;
> >> 				bridge_dpi_port: endpoint {
> >> 					remote-endpoint =
> > <&pitouchscreen_panel_port>;
> >> 				};
> >> 			};
> > 
> > The second port is thus not needed.
> > 
> >> 		};
> > 
> > So we can simplify this to
> > 
> > 		port {
> > 			panel_dsi_port: endpoint {
> > 				remote-endpoint = <&dsi_out_port>;
> > 			};
> > 		};
> > 
> > (no need for a ports node when there's a single port)
> > 
> >> 	};
> >> };
> >> 
> >> lcd {
> >> 	compatible = "raspberrypi,7inch-touchscreen-panel";
> >> 	ports {
> >> 		#address-cells = <1>;
> >> 		#size-cells = <0>;
> >> 		port@0 {
> >> 			reg = <0>;
> >> 			pitouchscreen_panel_port: endpoint {
> >> 				remote-endpoint = <&bridge_dpi_port>;
> >> 			};
> >> 		};
> >> 	};
> >> };
> > 
> > And this node can go away.

Eric Anholt May 22, 2017, 8:50 p.m. UTC | #15

Laurent Pinchart <[e-mail]> writes:

> Hi Eric,
>
> On Tuesday 16 May 2017 11:46:36 Eric Anholt wrote:
>
> [snip]
>
>> In terms of physical connections:
>> 
>>    [15-pin "DSI" connector on 2835]
>> 
>>     | I2C               | DSI
>>    / \        SPI       |
>> [TS]  [Atmel]------[TC358762]
>>        \                |
>>         \PWM            |
>>          \              | DPI
>> [some backlight]------[some unknown panel]
>> 
>> The binding I'm trying to create is to expose what's necessary for a
>> driver that talks I2C to the Atmel, which then controls the PWM and does
>> the command sequence over SPI to the Toshiba that sets up its end of the
>> DSI link.
>
> According to the documentation I've been able to find, the TC358762 has an SPI 
> master port through which it can output the commands DCS received from the DSI 
> port, and an I2C slave port through which it can be configured by an external 
> device. If the connection between the microcontroller and the TC358762 is 
> indeed SPI and not I2C, I assume it's used by the microcontroller to receive 
> the DCS commands and perform control of the backlight (and possibly other 
> components) accordingly. By the way, is there any place where I can find a 
> leaked version of the non-public panel schematics ? ;-)

Not that I know of.

I don't know that you can control the backlight over DCS, given that I
have no docs.  We only send commands from Atmel to TC, not the other way
around.

> As far as I can tell from your patch series, you don't need to send any 
> command to the TC358762 over DSI. In that case I would model the panel in DT 
> as an I2C device, as all control goes through the I2C bus. The DSI video data 
> connection should then be modelled using the OF graph DT bindings. The result 
> will be a black box panel with a custom black box panel driver, using a single 
> DT node. There's no need for a separate bridge instance. That's the cleanest 
> option I can come up with so far, and I agree that splitting TC358762 support 
> into a standalone bridge driver makes no sense in this case.

I agree, it's just that when I submitted to drm-panel I was told that it
didn't make sense as a single driver, so I went to all of this work
instead.

Eric Anholt May 22, 2017, 8:51 p.m. UTC | #16

Laurent Pinchart <[e-mail]> writes:

> Hi Archit,
>
> On Friday 19 May 2017 14:24:36 Archit Taneja wrote:
>> On 05/18/2017 08:25 PM, Laurent Pinchart wrote:
>> > On Thursday 18 May 2017 13:56:19 Archit Taneja wrote:
>> >> On 05/17/2017 12:16 AM, Eric Anholt wrote:
>> >
>> > [snip]
>> > 
>> >>> In terms of physical connections:
>> >>>    [15-pin "DSI" connector on 2835]
>> >>>     | I2C               | DSI
>> >>>    / \        SPI       |
>> >>> [TS]  [Atmel]------[TC358762]
>> >>>        \                |
>> >>>         \PWM            |
>> >>>          \              | DPI
>> >>> 
>> >>> [some backlight]------[some unknown panel]
>> >>> 
>> >>> The binding I'm trying to create is to expose what's necessary for a
>> >>> driver that talks I2C to the Atmel, which then controls the PWM and does
>> >>> the command sequence over SPI to the Toshiba that sets up its end of the
>> >>> DSI link.
>> >> 
>> >> The bridge (Atmel + TC358762 combination) here looks like it's primarily
>> >> an i2c device (i.e, the control bus is i2c). Therefore, the drm-bridge
>> >> driver here should be an i2c driver instead of a mipi_dsi_driver.
>> > 
>> > Glad to see we agree, that's what I've proposed in a separate answer :-)
>> > I'd go one step further though, there should be no DRM bridge, just a DRM
>> > panel.
>>
>> If the PCB containing the controller chips and the panel are part of a
>> single casing, and the set up won't work with another panel, then yeah, I
>> agree. If the bridge chips are on a separate adapter board, and there is a
>> possibility to connect other panels, then maybe a separate DRM bridge and a
>> DRM panel might be a safer bet.
>
> I thought it was a single black box, but upon closer inspection there's a 
> separate PCB with the Microcontroller and TC358762.
>
> Eric, do you know if it's possible to exchange the panel for another one (and 
> not just an model with identical features from another vendor, but another 
> panel with a different mode for instance) without reprogramming the 
> microcontroller, or is the bridge board tied to the panel model ?

Not without finding some other panel with equivalent non-standard
connectors / doing your own soldering, at a minimum.  And we don't know
what kind of programming the microcontroller does, since it's a black
box.

Patch

diff --git a/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchscreen-bridge.txt b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchscreen-bridge.txt
new file mode 100644
index 000000000000..a5669beaf68f
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/raspberrypi,7inch-touchscreen-bridge.txt
@@ -0,0 +1,68 @@ 
+Official 7" (800x480) Raspberry Pi touchscreen panel's bridge.
+
+This DSI panel contains:
+
+- TC358762 DSI->DPI bridge
+- Atmel microcontroller on I2C for power sequencing the DSI bridge and
+  controlling backlight
+- Touchscreen controller on I2C for touch input
+
+and this covers the TC358762 bridge and Atmel microcontroller, while
+../panel/raspberrypi,7inch-touchscreen-panel.txt covers the panel.
+
+Required properties:
+- compatible:	Must be "raspberrypi,7inch-touchscreen-bridge"
+- raspberrypi,touchscreen-bridge:
+		Handle to the I2C device for Atmel microcontroller
+
+Example:
+
+dsi1: dsi@7e700000 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	<...>
+
+	lcd-bridge@0 {
+		compatible = "raspberrypi,7inch-touchscreen-bridge";
+		reg = <0>;
+
+		raspberrypi,touchscreen-bridge = <&pitouchscreen_bridge>;
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			port@0 {
+				reg = <0>;
+				pitouchscreen_bridge_port: endpoint {
+					remote-endpoint = <&pitouchscreen_panel_port>;
+				};
+			};
+		};
+	};
+};
+
+i2c_dsi: i2c {
+	compatible = "i2c-gpio";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	gpios = <&gpio 28 0
+		 &gpio 29 0>;
+
+	pitouchscreen_bridge: bridge@45 {
+		compatible = "raspberrypi,touchscreen-bridge-i2c";
+		reg = <0x45>;
+	};
+};
+
+lcd {
+	compatible = "raspberrypi,7inch-touchscreen-panel";
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		port@0 {
+			reg = <0>;
+			pitouchscreen_panel_port: endpoint {
+				remote-endpoint = <&pitouchscreen_bridge_port>;
+			};
+		};
+	};
+};
diff --git a/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen-panel.txt b/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen-panel.txt
new file mode 100644
index 000000000000..1e84f97b3b20
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen-panel.txt
@@ -0,0 +1,7 @@ 
+Official 7" (800x480) Raspberry Pi touchscreen panel's panel.
+
+This binding is compatible with the simple-panel binding, which is specified
+in simple-panel.txt in this directory.
+
+Required properties:
+- compatible:	Must be "raspberrypi,7inch-touchscreen-panel"