[PATCH v2 00/10] Improvments for tc358775 with support for tc358765

[PATCH v2 00/10] Improvments for tc358775 with support for tc358765

[Tony Lindgren]

Here's v2 of these changes merged with patches from Michael.

Regards,

Tony

Changes since v1:

- After a brief offline discussion with Michael, merge series with
  Michael's patch series to make stby gpio and supplies optional as they
  may be hardwired

- Use Michael's better patch for the jeida timings change

- Parse lanes on the bridge side like other bridge devices do, and if not
  found, also parse on the DSI host side and warn

Michael Walle (3):
  dt-bindings: display: bridge: tc358775: make stby gpio and vdd
    supplies optional
  drm/bridge: tc358775: fix support for jeida-18 and jeida-24
  drm/bridge: tc358775: make standby GPIO optional

Tony Lindgren (7):
  dt-bindings: display: bridge: tc358775: Add data-lanes
  dt-bindings: display: bridge: tc358775: Add support for tc358765
  drm/bridge: tc358775: Get bridge data lanes instead of the DSI host
    lanes
  drm/bridge: tc358775: Add burst and low-power modes
  drm/bridge: tc358775: Enable pre_enable_prev_first flag
  drm/bridge: tc358775: Add support for tc358765
  drm/bridge: tc358775: Configure hs_rate and lp_rate

 .../display/bridge/toshiba,tc358775.yaml      | 30 +++++--
 drivers/gpu/drm/bridge/tc358775.c             | 90 +++++++++++--------
 2 files changed, 75 insertions(+), 45 deletions(-)

-- 
2.43.0

[PATCH v2 01/10] dt-bindings: display: bridge: tc358775: make stby gpio and vdd supplies optional

[Tony Lindgren]

From: Michael Walle <mwalle@kernel.org>

For a normal operation, the vdd supplies nor the stby GPIO is needed.
There are boards, where these voltages are statically enabled during
board power-up.

The reset pin is required because once the PPI (PHY protocol interface)
is started, it can only be stopped by asserting the reset pin.

Signed-off-by: Michael Walle <mwalle@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 .../devicetree/bindings/display/bridge/toshiba,tc358775.yaml   | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358775.yaml b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358775.yaml
--- a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358775.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358775.yaml
@@ -68,9 +68,6 @@ properties:
 required:
   - compatible
   - reg
-  - vdd-supply
-  - vddio-supply
-  - stby-gpios
   - reset-gpios
   - ports
 
-- 
2.43.0

Re: [PATCH v2 01/10] dt-bindings: display: bridge: tc358775: make stby gpio and vdd supplies optional

[Krzysztof Kozlowski]

On 02/12/2023 08:54, Tony Lindgren wrote:
> From: Michael Walle <mwalle@kernel.org>
> 
> For a normal operation, the vdd supplies nor the stby GPIO is needed.
> There are boards, where these voltages are statically enabled during
> board power-up.

This means supply is still required.

> 
> The reset pin is required because once the PPI (PHY protocol interface)
> is started, it can only be stopped by asserting the reset pin.
> 
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---


Best regards,
Krzysztof

Re: [PATCH v2 01/10] dt-bindings: display: bridge: tc358775: make stby gpio and vdd supplies optional

[Michael Walle]

>> For a normal operation, the vdd supplies nor the stby GPIO is needed.
>> There are boards, where these voltages are statically enabled during
>> board power-up.
> 
> This means supply is still required.

You mean using fixed-regulator? I didn't consider that. But yes, you're
right.

-michael

[PATCH v2 02/10] dt-bindings: display: bridge: tc358775: Add data-lanes

[Tony Lindgren]

The device uses a clock lane, and 1 to 4 DSI data lanes. Let's add the
data-lanes property starting at 1 similar to what the other bridge
bindings are doing.

Let's also drop the data-lanes properties in the example for the DSI host
controller to avoid confusion. The configuration of the DSI host depends
on the controller used and is unrelated to the bridge binding.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 .../display/bridge/toshiba,tc358775.yaml      | 21 ++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358775.yaml b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358775.yaml
--- a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358775.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358775.yaml
@@ -46,11 +46,26 @@ properties:
 
     properties:
       port@0:
-        $ref: /schemas/graph.yaml#/properties/port
+        $ref: /schemas/graph.yaml#/$defs/port-base
         description: |
           DSI Input. The remote endpoint phandle should be a
           reference to a valid mipi_dsi_host device node.
 
+        properties:
+          endpoint:
+            $ref: /schemas/media/video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              data-lanes:
+                description: array of physical DSI data lane indexes.
+                minItems: 1
+                items:
+                  - const: 1
+                  - const: 2
+                  - const: 3
+                  - const: 4
+
       port@1:
         $ref: /schemas/graph.yaml#/properties/port
         description: |
@@ -105,6 +120,7 @@ examples:
                     reg = <0>;
                     d2l_in_test: endpoint {
                         remote-endpoint = <&dsi0_out>;
+                        data-lanes = <1 2 3 4>;
                     };
                 };
 
@@ -129,7 +145,6 @@ examples:
                 reg = <1>;
                 dsi0_out: endpoint {
                     remote-endpoint = <&d2l_in_test>;
-                    data-lanes = <0 1 2 3>;
                 };
              };
          };
@@ -164,6 +179,7 @@ examples:
                     reg = <0>;
                     d2l_in_dual: endpoint {
                         remote-endpoint = <&dsi0_out_dual>;
+                        data-lanes = <1 2 3 4>;
                     };
                 };
 
@@ -195,7 +211,6 @@ examples:
                 reg = <1>;
                 dsi0_out_dual: endpoint {
                     remote-endpoint = <&d2l_in_dual>;
-                    data-lanes = <0 1 2 3>;
                 };
              };
          };
-- 
2.43.0

Re: [PATCH v2 02/10] dt-bindings: display: bridge: tc358775: Add data-lanes

[Krzysztof Kozlowski]

On 02/12/2023 08:54, Tony Lindgren wrote:
> The device uses a clock lane, and 1 to 4 DSI data lanes. Let's add the
> data-lanes property starting at 1 similar to what the other bridge
> bindings are doing.
> 
> Let's also drop the data-lanes properties in the example for the DSI host
> controller to avoid confusion. The configuration of the DSI host depends
> on the controller used and is unrelated to the bridge binding.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  .../display/bridge/toshiba,tc358775.yaml      | 21 ++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358775.yaml b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358775.yaml
> --- a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358775.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358775.yaml
> @@ -46,11 +46,26 @@ properties:
>  
>      properties:
>        port@0:
> -        $ref: /schemas/graph.yaml#/properties/port
> +        $ref: /schemas/graph.yaml#/$defs/port-base

You need to add here additionalProperties: false

Best regards,
Krzysztof

[PATCH v2 03/10] dt-bindings: display: bridge: tc358775: Add support for tc358765

[Tony Lindgren]

The tc358765 is similar to tc358775 except for the stdby-gpios.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 .../bindings/display/bridge/toshiba,tc358775.yaml           | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358775.yaml b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358775.yaml
--- a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358775.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358775.yaml
@@ -10,7 +10,7 @@ maintainers:
   - Vinay Simha BN <simhavcs@gmail.com>
 
 description: |
-  This binding supports DSI to LVDS bridge TC358775
+  This binding supports DSI to LVDS bridges TC358765 and TC358775
 
   MIPI DSI-RX Data 4-lane, CLK 1-lane with data rates up to 800 Mbps/lane.
   Video frame size:
@@ -21,7 +21,9 @@ description: |
 
 properties:
   compatible:
-    const: toshiba,tc358775
+    enum:
+      - toshiba,tc358765
+      - toshiba,tc358775
 
   reg:
     maxItems: 1
-- 
2.43.0

Re: [PATCH v2 03/10] dt-bindings: display: bridge: tc358775: Add support for tc358765

[Krzysztof Kozlowski]

On 02/12/2023 08:54, Tony Lindgren wrote:
> The tc358765 is similar to tc358775 except for the stdby-gpios.

In what aspect "except for the stby-gpios"? Does not have them? If so,
disallow them for that device (:false).



Best regards,
Krzysztof

[PATCH v2 04/10] drm/bridge: tc358775: fix support for jeida-18 and jeida-24

[Tony Lindgren]

From: Michael Walle <mwalle@kernel.org>

The bridge always uses 24bpp internally. Therefore, for jeida-18
mapping we need to discard the lowest two bits for each channel and thus
starting with LV_[RGB]2. jeida-24 has the same mapping but uses four
lanes instead of three, with the forth pair transmitting the lowest two
bits of each channel. Thus, the mapping between jeida-18 and jeida-24
is actually the same, except that one channel is turned off (by
selecting the RGB666 format in VPCTRL).

While at it, remove the bogus comment about the hardware default because
the default is overwritten in any case.

Tested with a jeida-18 display (Evervision VGG644804).

Fixes: b26975593b17 ("display/drm/bridge: TC358775 DSI/LVDS driver")
Signed-off-by: Michael Walle <mwalle@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/gpu/drm/bridge/tc358775.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
--- a/drivers/gpu/drm/bridge/tc358775.c
+++ b/drivers/gpu/drm/bridge/tc358775.c
@@ -454,10 +454,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
 	dev_dbg(tc->dev, "bus_formats %04x bpc %d\n",
 		connector->display_info.bus_formats[0],
 		tc->bpc);
-	/*
-	 * Default hardware register settings of tc358775 configured
-	 * with MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA jeida-24 format
-	 */
 	if (connector->display_info.bus_formats[0] ==
 		MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
 		/* VESA-24 */
@@ -468,14 +464,15 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
 		d2l_write(tc->i2c, LV_MX1619, LV_MX(LVI_B6, LVI_B7, LVI_B1, LVI_B2));
 		d2l_write(tc->i2c, LV_MX2023, LV_MX(LVI_B3, LVI_B4, LVI_B5, LVI_L0));
 		d2l_write(tc->i2c, LV_MX2427, LV_MX(LVI_HS, LVI_VS, LVI_DE, LVI_R6));
-	} else { /*  MEDIA_BUS_FMT_RGB666_1X7X3_SPWG - JEIDA-18 */
-		d2l_write(tc->i2c, LV_MX0003, LV_MX(LVI_R0, LVI_R1, LVI_R2, LVI_R3));
-		d2l_write(tc->i2c, LV_MX0407, LV_MX(LVI_R4, LVI_L0, LVI_R5, LVI_G0));
-		d2l_write(tc->i2c, LV_MX0811, LV_MX(LVI_G1, LVI_G2, LVI_L0, LVI_L0));
-		d2l_write(tc->i2c, LV_MX1215, LV_MX(LVI_G3, LVI_G4, LVI_G5, LVI_B0));
-		d2l_write(tc->i2c, LV_MX1619, LV_MX(LVI_L0, LVI_L0, LVI_B1, LVI_B2));
-		d2l_write(tc->i2c, LV_MX2023, LV_MX(LVI_B3, LVI_B4, LVI_B5, LVI_L0));
-		d2l_write(tc->i2c, LV_MX2427, LV_MX(LVI_HS, LVI_VS, LVI_DE, LVI_L0));
+	} else {
+		/* JEIDA-18 and JEIDA-24 */
+		d2l_write(tc->i2c, LV_MX0003, LV_MX(LVI_R2, LVI_R3, LVI_R4, LVI_R5));
+		d2l_write(tc->i2c, LV_MX0407, LV_MX(LVI_R6, LVI_R1, LVI_R7, LVI_G2));
+		d2l_write(tc->i2c, LV_MX0811, LV_MX(LVI_G3, LVI_G4, LVI_G0, LVI_G1));
+		d2l_write(tc->i2c, LV_MX1215, LV_MX(LVI_G5, LVI_G6, LVI_G7, LVI_B2));
+		d2l_write(tc->i2c, LV_MX1619, LV_MX(LVI_B0, LVI_B1, LVI_B3, LVI_B4));
+		d2l_write(tc->i2c, LV_MX2023, LV_MX(LVI_B5, LVI_B6, LVI_B7, LVI_L0));
+		d2l_write(tc->i2c, LV_MX2427, LV_MX(LVI_HS, LVI_VS, LVI_DE, LVI_R0));
 	}
 
 	d2l_write(tc->i2c, VFUEN, VFUEN_EN);
-- 
2.43.0

[PATCH v2 05/10] drm/bridge: tc358775: make standby GPIO optional

[Tony Lindgren]

From: Michael Walle <mwalle@kernel.org>

The stby pin is optional. It is only needed for power-up and down
sequencing. It is not needed, if the power rails cannot by dynamically
enabled.

Because the GPIO is not optional, remove the error message.

Signed-off-by: Michael Walle <mwalle@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/gpu/drm/bridge/tc358775.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
--- a/drivers/gpu/drm/bridge/tc358775.c
+++ b/drivers/gpu/drm/bridge/tc358775.c
@@ -669,12 +669,9 @@ static int tc_probe(struct i2c_client *client)
 		return ret;
 	}
 
-	tc->stby_gpio = devm_gpiod_get(dev, "stby", GPIOD_OUT_HIGH);
-	if (IS_ERR(tc->stby_gpio)) {
-		ret = PTR_ERR(tc->stby_gpio);
-		dev_err(dev, "cannot get stby-gpio %d\n", ret);
-		return ret;
-	}
+	tc->stby_gpio = devm_gpiod_get_optional(dev, "stby", GPIOD_OUT_HIGH);
+	if (IS_ERR(tc->stby_gpio))
+		return PTR_ERR(tc->stby_gpio);
 
 	tc->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(tc->reset_gpio)) {
-- 
2.43.0

Re: [PATCH v2 05/10] drm/bridge: tc358775: make standby GPIO optional

[Dmitry Baryshkov]

On Sat, 2 Dec 2023 at 10:01, Tony Lindgren <tony@atomide.com> wrote:
>
> From: Michael Walle <mwalle@kernel.org>
>
> The stby pin is optional. It is only needed for power-up and down
> sequencing. It is not needed, if the power rails cannot by dynamically
> enabled.
>
> Because the GPIO is not optional, remove the error message.
>
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/gpu/drm/bridge/tc358775.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


-- 
With best wishes
Dmitry

Re: [PATCH v2 05/10] drm/bridge: tc358775: make standby GPIO optional

[Michael Walle]

> The stby pin is optional. It is only needed for power-up and down
> sequencing. It is not needed, if the power rails cannot by dynamically
> enabled.
> 
> Because the GPIO is not optional, remove the error message.

I just noticed a typo: "is *now* optional.

-michael

[PATCH v2 06/10] drm/bridge: tc358775: Get bridge data lanes instead of the DSI host lanes

[Tony Lindgren]

The current code assumes the data-lanes property is configured on the
DSI host side instead of the bridge side, and assumes DSI host endpoint 1.

Let's standardize on what the other bridge drivers are doing and parse the
data-lanes property for the bridge. Only if data-lanes property is not found,
let's be nice and also check the DSI host for old dtb in use and warn.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/gpu/drm/bridge/tc358775.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
--- a/drivers/gpu/drm/bridge/tc358775.c
+++ b/drivers/gpu/drm/bridge/tc358775.c
@@ -525,27 +525,24 @@ tc_mode_valid(struct drm_bridge *bridge,
 static int tc358775_parse_dt(struct device_node *np, struct tc_data *tc)
 {
 	struct device_node *endpoint;
-	struct device_node *parent;
 	struct device_node *remote;
 	int dsi_lanes = -1;
 
-	/*
-	 * To get the data-lanes of dsi, we need to access the dsi0_out of port1
-	 *  of dsi0 endpoint from bridge port0 of d2l_in
-	 */
 	endpoint = of_graph_get_endpoint_by_regs(tc->dev->of_node,
 						 TC358775_DSI_IN, -1);
-	if (endpoint) {
-		/* dsi0_out node */
-		parent = of_graph_get_remote_port_parent(endpoint);
-		of_node_put(endpoint);
-		if (parent) {
-			/* dsi0 port 1 */
-			dsi_lanes = drm_of_get_data_lanes_count_ep(parent, 1, -1, 1, 4);
-			of_node_put(parent);
-		}
+	dsi_lanes = drm_of_get_data_lanes_count(endpoint, 1, 4);
+
+	/* Quirk old dtb: Use data lanes from the DSI host side instead of bridge */
+	if (dsi_lanes == -EINVAL || dsi_lanes == -ENODEV) {
+		remote = of_graph_get_remote_endpoint(endpoint);
+		dsi_lanes = drm_of_get_data_lanes_count(remote, 1, 4);
+		of_node_put(remote);
+		if (dsi_lanes >= 1)
+			dev_warn(tc->dev, "missing dsi-lanes property for the bridge\n");
 	}
 
+	of_node_put(endpoint);
+
 	if (dsi_lanes < 0)
 		return dsi_lanes;
 
-- 
2.43.0

Re: [PATCH v2 06/10] drm/bridge: tc358775: Get bridge data lanes instead of the DSI host lanes

[Dmitry Baryshkov]

On Sat, 2 Dec 2023 at 10:01, Tony Lindgren <tony@atomide.com> wrote:
>
> The current code assumes the data-lanes property is configured on the
> DSI host side instead of the bridge side, and assumes DSI host endpoint 1.
>
> Let's standardize on what the other bridge drivers are doing and parse the
> data-lanes property for the bridge. Only if data-lanes property is not found,
> let's be nice and also check the DSI host for old dtb in use and warn.

It might be worth adding that lanes configuration for the host and for
the bridge might be different (e.g. the lanes might be swapped on the
host side).

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/gpu/drm/bridge/tc358775.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)

-- 
With best wishes
Dmitry

Re: [PATCH v2 06/10] drm/bridge: tc358775: Get bridge data lanes instead of the DSI host lanes

[Michael Walle]

> The current code assumes the data-lanes property is configured on the
> DSI host side instead of the bridge side, and assumes DSI host endpoint 1.
> 
> Let's standardize on what the other bridge drivers are doing and parse the
> data-lanes property for the bridge. Only if data-lanes property is not found,
> let's be nice and also check the DSI host for old dtb in use and warn.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/gpu/drm/bridge/tc358775.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
> --- a/drivers/gpu/drm/bridge/tc358775.c
> +++ b/drivers/gpu/drm/bridge/tc358775.c
> @@ -525,27 +525,24 @@ tc_mode_valid(struct drm_bridge *bridge,
>  static int tc358775_parse_dt(struct device_node *np, struct tc_data *tc)
>  {
>  	struct device_node *endpoint;
> -	struct device_node *parent;
>  	struct device_node *remote;
>  	int dsi_lanes = -1;
>  
> -	/*
> -	 * To get the data-lanes of dsi, we need to access the dsi0_out of port1
> -	 *  of dsi0 endpoint from bridge port0 of d2l_in
> -	 */
>  	endpoint = of_graph_get_endpoint_by_regs(tc->dev->of_node,
>  						 TC358775_DSI_IN, -1);
> -	if (endpoint) {
> -		/* dsi0_out node */
> -		parent = of_graph_get_remote_port_parent(endpoint);
> -		of_node_put(endpoint);
> -		if (parent) {
> -			/* dsi0 port 1 */
> -			dsi_lanes = drm_of_get_data_lanes_count_ep(parent, 1, -1, 1, 4);
> -			of_node_put(parent);
> -		}
> +	dsi_lanes = drm_of_get_data_lanes_count(endpoint, 1, 4);
> +
> +	/* Quirk old dtb: Use data lanes from the DSI host side instead of bridge */
> +	if (dsi_lanes == -EINVAL || dsi_lanes == -ENODEV) {
> +		remote = of_graph_get_remote_endpoint(endpoint);
> +		dsi_lanes = drm_of_get_data_lanes_count(remote, 1, 4);
> +		of_node_put(remote);
> +		if (dsi_lanes >= 1)
> +			dev_warn(tc->dev, "missing dsi-lanes property for the bridge\n");

It wasn't obvious what this warning should tell me at first. Maybe
add something like ". Falling back to the property of the remote
endpoint". A little verbose, maybe you could come up with a more
dense wording.

In any case,

Reviewed-by: Michael Walle <mwalle@kernel.org>

-michael

[PATCH v2 07/10] drm/bridge: tc358775: Add burst and low-power modes

[Tony Lindgren]

Burst and low-power modes are supported both for tc358765 and tc358775.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/gpu/drm/bridge/tc358775.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
--- a/drivers/gpu/drm/bridge/tc358775.c
+++ b/drivers/gpu/drm/bridge/tc358775.c
@@ -619,7 +619,8 @@ static int tc_attach_host(struct tc_data *tc)
 
 	dsi->lanes = tc->num_dsi_lanes;
 	dsi->format = MIPI_DSI_FMT_RGB888;
-	dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
+		MIPI_DSI_MODE_LPM;
 
 	ret = devm_mipi_dsi_attach(dev, dsi);
 	if (ret < 0) {
-- 
2.43.0

Re: [PATCH v2 07/10] drm/bridge: tc358775: Add burst and low-power modes

[Michael Walle]

> Burst and low-power modes are supported both for tc358765 and tc358775.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/gpu/drm/bridge/tc358775.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
> --- a/drivers/gpu/drm/bridge/tc358775.c
> +++ b/drivers/gpu/drm/bridge/tc358775.c
> @@ -619,7 +619,8 @@ static int tc_attach_host(struct tc_data *tc)
>  
>  	dsi->lanes = tc->num_dsi_lanes;
>  	dsi->format = MIPI_DSI_FMT_RGB888;
> -	dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
> +		MIPI_DSI_MODE_LPM;

Could you align it with the equal sign of the former line?

Reviewed-by: Michael Walle <mwalle@kernel.org>
Tested-by: Michael Walle <mwalle@kernel.org>

-michael

[PATCH v2 08/10] drm/bridge: tc358775: Enable pre_enable_prev_first flag

[Tony Lindgren]

Set pre_enable_prev_first to ensure the previous bridge is enabled
first.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/gpu/drm/bridge/tc358775.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
--- a/drivers/gpu/drm/bridge/tc358775.c
+++ b/drivers/gpu/drm/bridge/tc358775.c
@@ -680,6 +680,7 @@ static int tc_probe(struct i2c_client *client)
 
 	tc->bridge.funcs = &tc_bridge_funcs;
 	tc->bridge.of_node = dev->of_node;
+	tc->bridge.pre_enable_prev_first = true;
 	drm_bridge_add(&tc->bridge);
 
 	i2c_set_clientdata(client, tc);
-- 
2.43.0

Re: [PATCH v2 08/10] drm/bridge: tc358775: Enable pre_enable_prev_first flag

[Dmitry Baryshkov]

On Sat, 2 Dec 2023 at 10:03, Tony Lindgren <tony@atomide.com> wrote:
>
> Set pre_enable_prev_first to ensure the previous bridge is enabled
> first.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/gpu/drm/bridge/tc358775.c | 1 +
>  1 file changed, 1 insertion(+)
>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


-- 
With best wishes
Dmitry

Re: [PATCH v2 08/10] drm/bridge: tc358775: Enable pre_enable_prev_first flag

[Michael Walle]

> Set pre_enable_prev_first to ensure the previous bridge is enabled
> first.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Reviewed-by: Michael Walle <mwalle@kernel.org>
Tested-by: Michael Walle <mwalle@kernel.org>

[PATCH v2 09/10] drm/bridge: tc358775: Add support for tc358765

[Tony Lindgren]

The tc358775 bridge is pin compatible with earlier tc358765 according to
the tc358774xbg_datasheet_en_20190118.pdf documentation. Compared to the
tc358765, the tc358775 supports a STBY GPIO and higher data rates.

The tc358765 has a register bit for video event mode vs video pulse mode.
We must set it to video event mode for the LCD output to work, and on the
tc358775, this bit no longer exists.

Looks like the registers seem to match otherwise based on a quick glance
comparing the defines to the earlier Android kernel tc358765 driver.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/gpu/drm/bridge/tc358775.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
--- a/drivers/gpu/drm/bridge/tc358775.c
+++ b/drivers/gpu/drm/bridge/tc358775.c
@@ -15,6 +15,7 @@
 #include <linux/kernel.h>
 #include <linux/media-bus-format.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
@@ -107,6 +108,7 @@
 #define RDPKTLN         0x0404  /* Command Read Packet Length */
 
 #define VPCTRL          0x0450  /* Video Path Control */
+#define EVTMODE		BIT(5)  /* Video event mode enable, tc35876x only */
 #define HTIM1           0x0454  /* Horizontal Timing Control 1 */
 #define HTIM2           0x0458  /* Horizontal Timing Control 2 */
 #define VTIM1           0x045C  /* Vertical Timing Control 1 */
@@ -254,6 +256,11 @@ enum tc358775_ports {
 	TC358775_LVDS_OUT1,
 };
 
+enum tc3587x5_type {
+	TC358765,
+	TC358775,
+};
+
 struct tc_data {
 	struct i2c_client	*i2c;
 	struct device		*dev;
@@ -271,6 +278,8 @@ struct tc_data {
 	struct gpio_desc	*stby_gpio;
 	u8			lvds_link; /* single-link or dual-link */
 	u8			bpc;
+
+	enum tc3587x5_type	type;
 };
 
 static inline struct tc_data *bridge_to_tc(struct drm_bridge *b)
@@ -424,10 +433,16 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
 	d2l_write(tc->i2c, PPI_STARTPPI, PPI_START_FUNCTION);
 	d2l_write(tc->i2c, DSI_STARTDSI, DSI_RX_START);
 
+	/* Video event mode vs pulse mode bit, does not exist for tc358775 */
+	if (tc->type == TC358765)
+		val = EVTMODE;
+	else
+		val = 0;
+
 	if (tc->bpc == 8)
-		val = TC358775_VPCTRL_OPXLFMT(1);
+		val |= TC358775_VPCTRL_OPXLFMT(1);
 	else /* bpc = 6; */
-		val = TC358775_VPCTRL_MSF(1);
+		val |= TC358775_VPCTRL_MSF(1);
 
 	dsiclk = mode->crtc_clock * 3 * tc->bpc / tc->num_dsi_lanes / 1000;
 	clkdiv = dsiclk / (tc->lvds_link == DUAL_LINK ? DIVIDE_BY_6 : DIVIDE_BY_3);
@@ -643,6 +658,7 @@ static int tc_probe(struct i2c_client *client)
 
 	tc->dev = dev;
 	tc->i2c = client;
+	tc->type = (enum tc3587x5_type)of_device_get_match_data(dev);
 
 	tc->panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node,
 						  TC358775_LVDS_OUT0, 0);
@@ -704,13 +720,15 @@ static void tc_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id tc358775_i2c_ids[] = {
-	{ "tc358775", 0 },
+	{ "tc358765", TC358765, },
+	{ "tc358775", TC358775, },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, tc358775_i2c_ids);
 
 static const struct of_device_id tc358775_of_ids[] = {
-	{ .compatible = "toshiba,tc358775", },
+	{ .compatible = "toshiba,tc358765", .data = (void *)TC358765, },
+	{ .compatible = "toshiba,tc358775", .data = (void *)TC358775, },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, tc358775_of_ids);
-- 
2.43.0

Re: [PATCH v2 09/10] drm/bridge: tc358775: Add support for tc358765

[Dmitry Baryshkov]

On Sat, 2 Dec 2023 at 10:04, Tony Lindgren <tony@atomide.com> wrote:
>
> The tc358775 bridge is pin compatible with earlier tc358765 according to
> the tc358774xbg_datasheet_en_20190118.pdf documentation. Compared to the
> tc358765, the tc358775 supports a STBY GPIO and higher data rates.
>
> The tc358765 has a register bit for video event mode vs video pulse mode.
> We must set it to video event mode for the LCD output to work, and on the
> tc358775, this bit no longer exists.
>
> Looks like the registers seem to match otherwise based on a quick glance
> comparing the defines to the earlier Android kernel tc358765 driver.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/gpu/drm/bridge/tc358775.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
> --- a/drivers/gpu/drm/bridge/tc358775.c
> +++ b/drivers/gpu/drm/bridge/tc358775.c
> @@ -15,6 +15,7 @@
>  #include <linux/kernel.h>
>  #include <linux/media-bus-format.h>
>  #include <linux/module.h>
> +#include <linux/of_device.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>
> @@ -107,6 +108,7 @@
>  #define RDPKTLN         0x0404  /* Command Read Packet Length */
>
>  #define VPCTRL          0x0450  /* Video Path Control */
> +#define EVTMODE                BIT(5)  /* Video event mode enable, tc35876x only */
>  #define HTIM1           0x0454  /* Horizontal Timing Control 1 */
>  #define HTIM2           0x0458  /* Horizontal Timing Control 2 */
>  #define VTIM1           0x045C  /* Vertical Timing Control 1 */
> @@ -254,6 +256,11 @@ enum tc358775_ports {
>         TC358775_LVDS_OUT1,
>  };
>
> +enum tc3587x5_type {
> +       TC358765,

I'd suggest adding = 1 or =0x65 so that the specified type differs
from 'no data' = 0 / NULL.

> +       TC358775,
> +};
> +
>  struct tc_data {
>         struct i2c_client       *i2c;
>         struct device           *dev;
> @@ -271,6 +278,8 @@ struct tc_data {
>         struct gpio_desc        *stby_gpio;
>         u8                      lvds_link; /* single-link or dual-link */
>         u8                      bpc;
> +
> +       enum tc3587x5_type      type;
>  };
>
>  static inline struct tc_data *bridge_to_tc(struct drm_bridge *b)
> @@ -424,10 +433,16 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>         d2l_write(tc->i2c, PPI_STARTPPI, PPI_START_FUNCTION);
>         d2l_write(tc->i2c, DSI_STARTDSI, DSI_RX_START);
>
> +       /* Video event mode vs pulse mode bit, does not exist for tc358775 */
> +       if (tc->type == TC358765)
> +               val = EVTMODE;
> +       else
> +               val = 0;
> +
>         if (tc->bpc == 8)
> -               val = TC358775_VPCTRL_OPXLFMT(1);
> +               val |= TC358775_VPCTRL_OPXLFMT(1);
>         else /* bpc = 6; */
> -               val = TC358775_VPCTRL_MSF(1);
> +               val |= TC358775_VPCTRL_MSF(1);
>
>         dsiclk = mode->crtc_clock * 3 * tc->bpc / tc->num_dsi_lanes / 1000;
>         clkdiv = dsiclk / (tc->lvds_link == DUAL_LINK ? DIVIDE_BY_6 : DIVIDE_BY_3);
> @@ -643,6 +658,7 @@ static int tc_probe(struct i2c_client *client)
>
>         tc->dev = dev;
>         tc->i2c = client;
> +       tc->type = (enum tc3587x5_type)of_device_get_match_data(dev);

Would it make sense to use i2c_get_match_data() instead?

>
>         tc->panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node,
>                                                   TC358775_LVDS_OUT0, 0);
> @@ -704,13 +720,15 @@ static void tc_remove(struct i2c_client *client)
>  }
>
>  static const struct i2c_device_id tc358775_i2c_ids[] = {
> -       { "tc358775", 0 },
> +       { "tc358765", TC358765, },
> +       { "tc358775", TC358775, },
>         { }
>  };
>  MODULE_DEVICE_TABLE(i2c, tc358775_i2c_ids);
>
>  static const struct of_device_id tc358775_of_ids[] = {
> -       { .compatible = "toshiba,tc358775", },
> +       { .compatible = "toshiba,tc358765", .data = (void *)TC358765, },
> +       { .compatible = "toshiba,tc358775", .data = (void *)TC358775, },
>         { }
>  };
>  MODULE_DEVICE_TABLE(of, tc358775_of_ids);
> --
> 2.43.0



-- 
With best wishes
Dmitry

Re: [PATCH v2 09/10] drm/bridge: tc358775: Add support for tc358765

[Michael Walle]

>> The tc358775 bridge is pin compatible with earlier tc358765 according to
>> the tc358774xbg_datasheet_en_20190118.pdf documentation. Compared to the
>> tc358765, the tc358775 supports a STBY GPIO and higher data rates.
>>
>> The tc358765 has a register bit for video event mode vs video pulse mode.
>> We must set it to video event mode for the LCD output to work, and on the
>> tc358775, this bit no longer exists.
>>
>> Looks like the registers seem to match otherwise based on a quick glance
>> comparing the defines to the earlier Android kernel tc358765 driver.
>>
>> Signed-off-by: Tony Lindgren <tony@atomide.com>
>> ---
>>  drivers/gpu/drm/bridge/tc358775.c | 26 ++++++++++++++++++++++----
>>  1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
>> --- a/drivers/gpu/drm/bridge/tc358775.c
>> +++ b/drivers/gpu/drm/bridge/tc358775.c
>> @@ -15,6 +15,7 @@
>>  #include <linux/kernel.h>
>>  #include <linux/media-bus-format.h>
>>  #include <linux/module.h>
>> +#include <linux/of_device.h>
>>  #include <linux/regulator/consumer.h>
>>  #include <linux/slab.h>
>>
>> @@ -107,6 +108,7 @@
>>  #define RDPKTLN         0x0404  /* Command Read Packet Length */
>>
>>  #define VPCTRL          0x0450  /* Video Path Control */
>> +#define EVTMODE                BIT(5)  /* Video event mode enable, tc35876x only */
>>  #define HTIM1           0x0454  /* Horizontal Timing Control 1 */
>>  #define HTIM2           0x0458  /* Horizontal Timing Control 2 */
>>  #define VTIM1           0x045C  /* Vertical Timing Control 1 */
>> @@ -254,6 +256,11 @@ enum tc358775_ports {
>>         TC358775_LVDS_OUT1,
>>  };
>>
>> +enum tc3587x5_type {
>> +       TC358765,
>
> I'd suggest adding = 1 or =0x65 so that the specified type differs
> from 'no data' = 0 / NULL.
>
>> +       TC358775,
>> +};
>> +
>>  struct tc_data {
>>         struct i2c_client       *i2c;
>>         struct device           *dev;
>> @@ -271,6 +278,8 @@ struct tc_data {
>>         struct gpio_desc        *stby_gpio;
>>         u8                      lvds_link; /* single-link or dual-link */
>>         u8                      bpc;
>> +
>> +       enum tc3587x5_type      type;
>>  };
>>
>>  static inline struct tc_data *bridge_to_tc(struct drm_bridge *b)
>> @@ -424,10 +433,16 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>>         d2l_write(tc->i2c, PPI_STARTPPI, PPI_START_FUNCTION);
>>         d2l_write(tc->i2c, DSI_STARTDSI, DSI_RX_START);
>>
>> +       /* Video event mode vs pulse mode bit, does not exist for tc358775 */
>> +       if (tc->type == TC358765)
>> +               val = EVTMODE;
>> +       else
>> +               val = 0;
>> +
>>         if (tc->bpc == 8)
>> -               val = TC358775_VPCTRL_OPXLFMT(1);
>> +               val |= TC358775_VPCTRL_OPXLFMT(1);
>>         else /* bpc = 6; */
>> -               val = TC358775_VPCTRL_MSF(1);
>> +               val |= TC358775_VPCTRL_MSF(1);
>>
>>         dsiclk = mode->crtc_clock * 3 * tc->bpc / tc->num_dsi_lanes / 1000;
>>         clkdiv = dsiclk / (tc->lvds_link == DUAL_LINK ? DIVIDE_BY_6 : DIVIDE_BY_3);
>> @@ -643,6 +658,7 @@ static int tc_probe(struct i2c_client *client)
>>
>>         tc->dev = dev;
>>         tc->i2c = client;
>> +       tc->type = (enum tc3587x5_type)of_device_get_match_data(dev);
>
> Would it make sense to use i2c_get_match_data() instead?

FWIW, I' planning to add a dsi binding for this driver. So I'd
suggest either the of_ or the device_ variant. Not sure though,
if the new device supports the DSI commands.

Otherwise, the patch looks good:

Reviewed-by: Michael Walle <mwalle@kernel.org>

-michael

>
>>
>>         tc->panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node,
>>                                                   TC358775_LVDS_OUT0, 0);
>> @@ -704,13 +720,15 @@ static void tc_remove(struct i2c_client *client)
>>  }
>>
>>  static const struct i2c_device_id tc358775_i2c_ids[] = {
>> -       { "tc358775", 0 },
>> +       { "tc358765", TC358765, },
>> +       { "tc358775", TC358775, },
>>         { }
>>  };
>>  MODULE_DEVICE_TABLE(i2c, tc358775_i2c_ids);
>>
>>  static const struct of_device_id tc358775_of_ids[] = {
>> -       { .compatible = "toshiba,tc358775", },
>> +       { .compatible = "toshiba,tc358765", .data = (void *)TC358765, },
>> +       { .compatible = "toshiba,tc358775", .data = (void *)TC358775, },
>>         { }
>>  };
>>  MODULE_DEVICE_TABLE(of, tc358775_of_ids);
>> --
>> 2.43.0



-- 
With best wishes
Dmitry

Re: [PATCH v2 09/10] drm/bridge: tc358775: Add support for tc358765

[Tony Lindgren]

* Michael Walle <mwalle@kernel.org> [231204 09:52]:
> >> @@ -643,6 +658,7 @@ static int tc_probe(struct i2c_client *client)
> >>
> >>         tc->dev = dev;
> >>         tc->i2c = client;
> >> +       tc->type = (enum tc3587x5_type)of_device_get_match_data(dev);
> >
> > Would it make sense to use i2c_get_match_data() instead?
> 
> FWIW, I' planning to add a dsi binding for this driver. So I'd
> suggest either the of_ or the device_ variant. Not sure though,
> if the new device supports the DSI commands.

Yeah good point as some hardware may not have i2c wired at all. Let's keep
this as of_device_get_match_data() for now as the driver is currently
completely dependant on devicetree.

I'll update the enumeration to use the hardware id numbering like Dmitry
suggested though.

Regards,

Tony

Re: [PATCH v2 09/10] drm/bridge: tc358775: Add support for tc358765

[kernel test robot]

Hi Tony,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.7-rc4 next-20231205]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tony-Lindgren/dt-bindings-display-bridge-tc358775-make-stby-gpio-and-vdd-supplies-optional/20231202-160622
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20231202075514.44474-10-tony%40atomide.com
patch subject: [PATCH v2 09/10] drm/bridge: tc358775: Add support for tc358765
config: x86_64-randconfig-121-20231203 (https://download.01.org/0day-ci/archive/20231206/202312060419.B8qmgrsh-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231206/202312060419.B8qmgrsh-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312060419.B8qmgrsh-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/bridge/tc358775.c:661:13: warning: cast to smaller integer type 'enum tc3587x5_type' from 'const void *' [-Wvoid-pointer-to-enum-cast]
           tc->type = (enum tc3587x5_type)of_device_get_match_data(dev);
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning generated.


vim +661 drivers/gpu/drm/bridge/tc358775.c

   648	
   649	static int tc_probe(struct i2c_client *client)
   650	{
   651		struct device *dev = &client->dev;
   652		struct tc_data *tc;
   653		int ret;
   654	
   655		tc = devm_kzalloc(dev, sizeof(*tc), GFP_KERNEL);
   656		if (!tc)
   657			return -ENOMEM;
   658	
   659		tc->dev = dev;
   660		tc->i2c = client;
 > 661		tc->type = (enum tc3587x5_type)of_device_get_match_data(dev);
   662	
   663		tc->panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node,
   664							  TC358775_LVDS_OUT0, 0);
   665		if (IS_ERR(tc->panel_bridge))
   666			return PTR_ERR(tc->panel_bridge);
   667	
   668		ret = tc358775_parse_dt(dev->of_node, tc);
   669		if (ret)
   670			return ret;
   671	
   672		tc->vddio = devm_regulator_get(dev, "vddio-supply");
   673		if (IS_ERR(tc->vddio)) {
   674			ret = PTR_ERR(tc->vddio);
   675			dev_err(dev, "vddio-supply not found\n");
   676			return ret;
   677		}
   678	
   679		tc->vdd = devm_regulator_get(dev, "vdd-supply");
   680		if (IS_ERR(tc->vdd)) {
   681			ret = PTR_ERR(tc->vdd);
   682			dev_err(dev, "vdd-supply not found\n");
   683			return ret;
   684		}
   685	
   686		tc->stby_gpio = devm_gpiod_get_optional(dev, "stby", GPIOD_OUT_HIGH);
   687		if (IS_ERR(tc->stby_gpio))
   688			return PTR_ERR(tc->stby_gpio);
   689	
   690		tc->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
   691		if (IS_ERR(tc->reset_gpio)) {
   692			ret = PTR_ERR(tc->reset_gpio);
   693			dev_err(dev, "cannot get reset-gpios %d\n", ret);
   694			return ret;
   695		}
   696	
   697		tc->bridge.funcs = &tc_bridge_funcs;
   698		tc->bridge.of_node = dev->of_node;
   699		tc->bridge.pre_enable_prev_first = true;
   700		drm_bridge_add(&tc->bridge);
   701	
   702		i2c_set_clientdata(client, tc);
   703	
   704		ret = tc_attach_host(tc);
   705		if (ret)
   706			goto err_bridge_remove;
   707	
   708		return 0;
   709	
   710	err_bridge_remove:
   711		drm_bridge_remove(&tc->bridge);
   712		return ret;
   713	}
   714	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2 10/10] drm/bridge: tc358775: Configure hs_rate and lp_rate

[Tony Lindgren]

The hs_rate and lp_rate may be used by the dsi host for timing
calculations. The tc358775 has a maximum bit rate of 1 Gbps/lane,
tc358765 has maximurate of 800 Mbps per lane.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/gpu/drm/bridge/tc358775.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
--- a/drivers/gpu/drm/bridge/tc358775.c
+++ b/drivers/gpu/drm/bridge/tc358775.c
@@ -636,6 +636,11 @@ static int tc_attach_host(struct tc_data *tc)
 	dsi->format = MIPI_DSI_FMT_RGB888;
 	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
 		MIPI_DSI_MODE_LPM;
+	if (tc->type == TC358765)
+		dsi->hs_rate = 800000000;
+	else
+		dsi->hs_rate = 1000000000;
+	dsi->lp_rate = 10000000;
 
 	ret = devm_mipi_dsi_attach(dev, dsi);
 	if (ret < 0) {
-- 
2.43.0

Re: [PATCH v2 10/10] drm/bridge: tc358775: Configure hs_rate and lp_rate

[Dmitry Baryshkov]

On Sat, 2 Dec 2023 at 10:06, Tony Lindgren <tony@atomide.com> wrote:
>
> The hs_rate and lp_rate may be used by the dsi host for timing
> calculations. The tc358775 has a maximum bit rate of 1 Gbps/lane,
> tc358765 has maximurate of 800 Mbps per lane.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/gpu/drm/bridge/tc358775.c | 5 +++++
>  1 file changed, 5 insertions(+)


Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v2 10/10] drm/bridge: tc358775: Configure hs_rate and lp_rate

[Michael Walle]

> The hs_rate and lp_rate may be used by the dsi host for timing
> calculations. The tc358775 has a maximum bit rate of 1 Gbps/lane,
> tc358765 has maximurate of 800 Mbps per lane.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/gpu/drm/bridge/tc358775.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
> --- a/drivers/gpu/drm/bridge/tc358775.c
> +++ b/drivers/gpu/drm/bridge/tc358775.c
> @@ -636,6 +636,11 @@ static int tc_attach_host(struct tc_data *tc)
>  	dsi->format = MIPI_DSI_FMT_RGB888;
>  	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
>  		MIPI_DSI_MODE_LPM;
> +	if (tc->type == TC358765)
> +		dsi->hs_rate = 800000000;

It's not clear to me whether this is the data rate or the frequency. From
the kernel doc:

 * @hs_rate: maximum lane frequency for high speed mode in hertz, this should
 * be set to the real limits of the hardware, zero is only accepted for
 * legacy drivers

The tc358775 datasheet lists 1Gbps per lane, which corresponds to a 500MHz DSI
clock frequency. Not sure how that would correspond to the "maximum lane
frequency" above. I guess the wording of the comment is just misleading and
the value is the data rate of the lane.

> +	else
> +		dsi->hs_rate = 1000000000;
> +	dsi->lp_rate = 10000000;

That I didn't found in the datasheet. Just a T_min_rx (minimum pulse width
response) which is 20ns. But there are no more details on this.

-michael

Re: [PATCH v2 10/10] drm/bridge: tc358775: Configure hs_rate and lp_rate

[Tony Lindgren]

* Michael Walle <mwalle@kernel.org> [231207 16:14]:
> > The hs_rate and lp_rate may be used by the dsi host for timing
> > calculations. The tc358775 has a maximum bit rate of 1 Gbps/lane,
> > tc358765 has maximurate of 800 Mbps per lane.
> > 
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > ---
> >  drivers/gpu/drm/bridge/tc358775.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
> > --- a/drivers/gpu/drm/bridge/tc358775.c
> > +++ b/drivers/gpu/drm/bridge/tc358775.c
> > @@ -636,6 +636,11 @@ static int tc_attach_host(struct tc_data *tc)
> >  	dsi->format = MIPI_DSI_FMT_RGB888;
> >  	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
> >  		MIPI_DSI_MODE_LPM;
> > +	if (tc->type == TC358765)
> > +		dsi->hs_rate = 800000000;
> 
> It's not clear to me whether this is the data rate or the frequency. From
> the kernel doc:
> 
>  * @hs_rate: maximum lane frequency for high speed mode in hertz, this should
>  * be set to the real limits of the hardware, zero is only accepted for
>  * legacy drivers
> 
> The tc358775 datasheet lists 1Gbps per lane, which corresponds to a 500MHz DSI
> clock frequency. Not sure how that would correspond to the "maximum lane
> frequency" above. I guess the wording of the comment is just misleading and
> the value is the data rate of the lane.

Yeah seems we're using the data rate of a lane in in hertz and then the
host drivers adapt for the double data rate. Or at least that's my
understanding.. Hopefully we don't have different assumptions in the
host drivers.

> > +	else
> > +		dsi->hs_rate = 1000000000;
> > +	dsi->lp_rate = 10000000;
> 
> That I didn't found in the datasheet. Just a T_min_rx (minimum pulse width
> response) which is 20ns. But there are no more details on this.

I think the low power data rate might be specified in the mipi dsi spec.
Maybe somebody familiar with the spec can confirm it.

Regards,

Tony