[PATCH V3 00/13] drm: bridge: icn6211: Fix hard-coded panel settings and add I2C support

[PATCH V3 00/13] drm: bridge: icn6211: Fix hard-coded panel settings and add I2C support

[Marek Vasut]

This series fixes multiple problems with the ICN6211 driver and adds
support for configuration of the chip via I2C bus.

First, in the current state, the ICN6211 driver hard-codes DPI timing
and clock settings specific to some unknown panel. The settings provided
by panel driver are ignored. Using any other panel than the one for which
this driver is currently hard-coded can lead to permanent damage of the
panel (per display supplier warning, and it sure did in my case. The
damage looks like multiple rows of dead pixels at the bottom of the
panel, and this is not going away even after long power off time).

Much of this series thus fixes incorrect register layout, DPI timing
programming, clock generation by adding actual PLL configuration code.
This series also adds lane count decoding instead of using hard-coded
value, and fills in a couple of registers with likely correct default
values.

Second, this series adds support for I2C configuration of the ICN6211.
The device can be configured either via DSI command mode or via I2C,
the register layout is the same in both cases.

Since the datasheet for this device is very hard to come by, a lot of
information has been salvaged from [1] and [2].

[1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/gpu/drm/bridge/icn6211.c
[2] https://github.com/tdjastrzebski/ICN6211-Configurator

Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org

Marek Vasut (13):
  dt-bindings: display: bridge: icn6211: Document DSI data-lanes
    property
  drm: bridge: icn6211: Fix register layout
  drm: bridge: icn6211: Fix HFP_HSW_HBP_HI and HFP_MIN handling
  drm: bridge: icn6211: Add HS/VS/DE polarity handling
  drm: bridge: icn6211: Add DSI lane count DT property parsing
  drm: bridge: icn6211: Add generic DSI-to-DPI PLL configuration
  drm: bridge: icn6211: Use DSI burst mode without EoT and with LP
    command mode
  drm: bridge: icn6211: Disable DPI color swap
  drm: bridge: icn6211: Set SYS_CTRL_1 to value used in examples
  drm: bridge: icn6211: Implement atomic_get_input_bus_fmts
  drm: bridge: icn6211: Add I2C configuration support
  drm: bridge: icn6211: Rework ICN6211_DSI to chipone_writeb()
  drm: bridge: icn6211: Read and validate chip IDs before configuration

 .../display/bridge/chipone,icn6211.yaml       |  18 +-
 drivers/gpu/drm/bridge/chipone-icn6211.c      | 534 ++++++++++++++++--
 2 files changed, 496 insertions(+), 56 deletions(-)

-- 
2.34.1

[PATCH V3 01/13] dt-bindings: display: bridge: icn6211: Document DSI data-lanes property

[Marek Vasut]

It is necessary to specify the number of connected/used DSI data lanes when
using the DSI input port of this bridge. Document the 'data-lanes' property
of the DSI input port.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: devicetree@vger.kernel.org
To: dri-devel@lists.freedesktop.org
---
V3: New patch
---
 .../display/bridge/chipone,icn6211.yaml        | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml
index 62c3bd4cb28d8..f8cac721a7330 100644
--- a/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml
@@ -41,10 +41,26 @@ properties:
 
     properties:
       port@0:
-        $ref: /schemas/graph.yaml#/properties/port
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
         description:
           Video port for MIPI DSI input
 
+        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:
-- 
2.34.1

[PATCH V3 01/13] dt-bindings: display: bridge: icn6211: Document DSI data-lanes property

[Marek Vasut]

It is necessary to specify the number of connected/used DSI data lanes when
using the DSI input port of this bridge. Document the 'data-lanes' property
of the DSI input port.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: devicetree@vger.kernel.org
To: dri-devel@lists.freedesktop.org
---
V3: New patch
---
 .../display/bridge/chipone,icn6211.yaml        | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml
index 62c3bd4cb28d8..f8cac721a7330 100644
--- a/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml
@@ -41,10 +41,26 @@ properties:
 
     properties:
       port@0:
-        $ref: /schemas/graph.yaml#/properties/port
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
         description:
           Video port for MIPI DSI input
 
+        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:
-- 
2.34.1

Re: [PATCH V3 01/13] dt-bindings: display: bridge: icn6211: Document DSI data-lanes property

[Rob Herring]

On Fri, 04 Mar 2022 01:24:56 +0100, Marek Vasut wrote:
> It is necessary to specify the number of connected/used DSI data lanes when
> using the DSI input port of this bridge. Document the 'data-lanes' property
> of the DSI input port.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: devicetree@vger.kernel.org
> To: dri-devel@lists.freedesktop.org
> ---
> V3: New patch
> ---
>  .../display/bridge/chipone,icn6211.yaml        | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH V3 01/13] dt-bindings: display: bridge: icn6211: Document DSI data-lanes property

[Rob Herring]

On Fri, 04 Mar 2022 01:24:56 +0100, Marek Vasut wrote:
> It is necessary to specify the number of connected/used DSI data lanes when
> using the DSI input port of this bridge. Document the 'data-lanes' property
> of the DSI input port.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: devicetree@vger.kernel.org
> To: dri-devel@lists.freedesktop.org
> ---
> V3: New patch
> ---
>  .../display/bridge/chipone,icn6211.yaml        | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH V3 01/13] dt-bindings: display: bridge: icn6211: Document DSI data-lanes property

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 684 bytes --]

On Fri, Mar 04, 2022 at 01:24:56AM +0100, Marek Vasut wrote:
> It is necessary to specify the number of connected/used DSI data lanes when
> using the DSI input port of this bridge. Document the 'data-lanes' property
> of the DSI input port.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: devicetree@vger.kernel.org
> To: dri-devel@lists.freedesktop.org

Acked-by: Maxime Ripard <maxime@cerno.tech>

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH V3 01/13] dt-bindings: display: bridge: icn6211: Document DSI data-lanes property

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 684 bytes --]

On Fri, Mar 04, 2022 at 01:24:56AM +0100, Marek Vasut wrote:
> It is necessary to specify the number of connected/used DSI data lanes when
> using the DSI input port of this bridge. Document the 'data-lanes' property
> of the DSI input port.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: devicetree@vger.kernel.org
> To: dri-devel@lists.freedesktop.org

Acked-by: Maxime Ripard <maxime@cerno.tech>

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH V3 02/13] drm: bridge: icn6211: Fix register layout

[Marek Vasut]

The chip register layout has nothing to do with MIPI DCS, the registers
incorrectly marked as MIPI DCS in the driver are regular chip registers
often with completely different function.

Fill in the actual register names and bits from [1] and [2] and add the
entire register layout, since the documentation for this chip is hard to
come by.

[1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/gpu/drm/bridge/icn6211.c
[2] https://github.com/tdjastrzebski/ICN6211-Configurator

Acked-by: Maxime Ripard <maxime@cerno.tech>
Fixes: ce517f18944e3 ("drm: bridge: Add Chipone ICN6211 MIPI-DSI to RGB bridge")
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
V2: Rebase on next-20220214
V3: Add AB from Maxime
---
 drivers/gpu/drm/bridge/chipone-icn6211.c | 134 ++++++++++++++++++++---
 1 file changed, 117 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index e8f36dca56b33..4b8d1a5a50302 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -15,8 +15,19 @@
 #include <linux/of_device.h>
 #include <linux/regulator/consumer.h>
 
-#include <video/mipi_display.h>
-
+#define VENDOR_ID		0x00
+#define DEVICE_ID_H		0x01
+#define DEVICE_ID_L		0x02
+#define VERSION_ID		0x03
+#define FIRMWARE_VERSION	0x08
+#define CONFIG_FINISH		0x09
+#define PD_CTRL(n)		(0x0a + ((n) & 0x3)) /* 0..3 */
+#define RST_CTRL(n)		(0x0e + ((n) & 0x1)) /* 0..1 */
+#define SYS_CTRL(n)		(0x10 + ((n) & 0x7)) /* 0..4 */
+#define RGB_DRV(n)		(0x18 + ((n) & 0x3)) /* 0..3 */
+#define RGB_DLY(n)		(0x1c + ((n) & 0x1)) /* 0..1 */
+#define RGB_TEST_CTRL		0x1e
+#define ATE_PLL_EN		0x1f
 #define HACTIVE_LI		0x20
 #define VACTIVE_LI		0x21
 #define VACTIVE_HACTIVE_HI	0x22
@@ -27,6 +38,95 @@
 #define VFP			0x27
 #define VSYNC			0x28
 #define VBP			0x29
+#define BIST_POL		0x2a
+#define BIST_POL_BIST_MODE(n)		(((n) & 0xf) << 4)
+#define BIST_POL_BIST_GEN		BIT(3)
+#define BIST_POL_HSYNC_POL		BIT(2)
+#define BIST_POL_VSYNC_POL		BIT(1)
+#define BIST_POL_DE_POL			BIT(0)
+#define BIST_RED		0x2b
+#define BIST_GREEN		0x2c
+#define BIST_BLUE		0x2d
+#define BIST_CHESS_X		0x2e
+#define BIST_CHESS_Y		0x2f
+#define BIST_CHESS_XY_H		0x30
+#define BIST_FRAME_TIME_L	0x31
+#define BIST_FRAME_TIME_H	0x32
+#define FIFO_MAX_ADDR_LOW	0x33
+#define SYNC_EVENT_DLY		0x34
+#define HSW_MIN			0x35
+#define HFP_MIN			0x36
+#define LOGIC_RST_NUM		0x37
+#define OSC_CTRL(n)		(0x48 + ((n) & 0x7)) /* 0..5 */
+#define BG_CTRL			0x4e
+#define LDO_PLL			0x4f
+#define PLL_CTRL(n)		(0x50 + ((n) & 0xf)) /* 0..15 */
+#define PLL_CTRL_6_EXTERNAL		0x90
+#define PLL_CTRL_6_MIPI_CLK		0x92
+#define PLL_CTRL_6_INTERNAL		0x93
+#define PLL_REM(n)		(0x60 + ((n) & 0x3)) /* 0..2 */
+#define PLL_DIV(n)		(0x63 + ((n) & 0x3)) /* 0..2 */
+#define PLL_FRAC(n)		(0x66 + ((n) & 0x3)) /* 0..2 */
+#define PLL_INT(n)		(0x69 + ((n) & 0x1)) /* 0..1 */
+#define PLL_REF_DIV		0x6b
+#define PLL_REF_DIV_P(n)		((n) & 0xf)
+#define PLL_REF_DIV_Pe			BIT(4)
+#define PLL_REF_DIV_S(n)		(((n) & 0x7) << 5)
+#define PLL_SSC_P(n)		(0x6c + ((n) & 0x3)) /* 0..2 */
+#define PLL_SSC_STEP(n)		(0x6f + ((n) & 0x3)) /* 0..2 */
+#define PLL_SSC_OFFSET(n)	(0x72 + ((n) & 0x3)) /* 0..3 */
+#define GPIO_OEN		0x79
+#define MIPI_CFG_PW		0x7a
+#define MIPI_CFG_PW_CONFIG_DSI		0xc1
+#define MIPI_CFG_PW_CONFIG_I2C		0x3e
+#define GPIO_SEL(n)		(0x7b + ((n) & 0x1)) /* 0..1 */
+#define IRQ_SEL			0x7d
+#define DBG_SEL			0x7e
+#define DBG_SIGNAL		0x7f
+#define MIPI_ERR_VECTOR_L	0x80
+#define MIPI_ERR_VECTOR_H	0x81
+#define MIPI_ERR_VECTOR_EN_L	0x82
+#define MIPI_ERR_VECTOR_EN_H	0x83
+#define MIPI_MAX_SIZE_L		0x84
+#define MIPI_MAX_SIZE_H		0x85
+#define DSI_CTRL		0x86
+#define DSI_CTRL_UNKNOWN		0x28
+#define DSI_CTRL_DSI_LANES(n)		((n) & 0x3)
+#define MIPI_PN_SWAP		0x87
+#define MIPI_PN_SWAP_CLK		BIT(4)
+#define MIPI_PN_SWAP_D(n)		BIT((n) & 0x3)
+#define MIPI_SOT_SYNC_BIT_(n)	(0x88 + ((n) & 0x1)) /* 0..1 */
+#define MIPI_ULPS_CTRL		0x8a
+#define MIPI_CLK_CHK_VAR	0x8e
+#define MIPI_CLK_CHK_INI	0x8f
+#define MIPI_T_TERM_EN		0x90
+#define MIPI_T_HS_SETTLE	0x91
+#define MIPI_T_TA_SURE_PRE	0x92
+#define MIPI_T_LPX_SET		0x94
+#define MIPI_T_CLK_MISS		0x95
+#define MIPI_INIT_TIME_L	0x96
+#define MIPI_INIT_TIME_H	0x97
+#define MIPI_T_CLK_TERM_EN	0x99
+#define MIPI_T_CLK_SETTLE	0x9a
+#define MIPI_TO_HS_RX_L		0x9e
+#define MIPI_TO_HS_RX_H		0x9f
+#define MIPI_PHY_(n)		(0xa0 + ((n) & 0x7)) /* 0..5 */
+#define MIPI_PD_RX		0xb0
+#define MIPI_PD_TERM		0xb1
+#define MIPI_PD_HSRX		0xb2
+#define MIPI_PD_LPTX		0xb3
+#define MIPI_PD_LPRX		0xb4
+#define MIPI_PD_CK_LANE		0xb5
+#define MIPI_FORCE_0		0xb6
+#define MIPI_RST_CTRL		0xb7
+#define MIPI_RST_NUM		0xb8
+#define MIPI_DBG_SET_(n)	(0xc0 + ((n) & 0xf)) /* 0..9 */
+#define MIPI_DBG_SEL		0xe0
+#define MIPI_DBG_DATA		0xe1
+#define MIPI_ATE_TEST_SEL	0xe2
+#define MIPI_ATE_STATUS_(n)	(0xe3 + ((n) & 0x1)) /* 0..1 */
+#define MIPI_ATE_STATUS_1	0xe4
+#define ICN6211_MAX_REGISTER	MIPI_ATE_STATUS(1)
 
 struct chipone {
 	struct device *dev;
@@ -64,13 +164,13 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 	struct chipone *icn = bridge_to_chipone(bridge);
 	struct drm_display_mode *mode = &icn->mode;
 
-	ICN6211_DSI(icn, 0x7a, 0xc1);
+	ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_DSI);
 
 	ICN6211_DSI(icn, HACTIVE_LI, mode->hdisplay & 0xff);
 
 	ICN6211_DSI(icn, VACTIVE_LI, mode->vdisplay & 0xff);
 
-	/**
+	/*
 	 * lsb nibble: 2nd nibble of hdisplay
 	 * msb nibble: 2nd nibble of vdisplay
 	 */
@@ -93,21 +193,21 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 	ICN6211_DSI(icn, VBP, mode->vtotal - mode->vsync_end);
 
 	/* dsi specific sequence */
-	ICN6211_DSI(icn, MIPI_DCS_SET_TEAR_OFF, 0x80);
-	ICN6211_DSI(icn, MIPI_DCS_SET_ADDRESS_MODE, 0x28);
-	ICN6211_DSI(icn, 0xb5, 0xa0);
-	ICN6211_DSI(icn, 0x5c, 0xff);
-	ICN6211_DSI(icn, MIPI_DCS_SET_COLUMN_ADDRESS, 0x01);
-	ICN6211_DSI(icn, MIPI_DCS_GET_POWER_SAVE, 0x92);
-	ICN6211_DSI(icn, 0x6b, 0x71);
-	ICN6211_DSI(icn, 0x69, 0x2b);
-	ICN6211_DSI(icn, MIPI_DCS_ENTER_SLEEP_MODE, 0x40);
-	ICN6211_DSI(icn, MIPI_DCS_EXIT_SLEEP_MODE, 0x98);
+	ICN6211_DSI(icn, SYNC_EVENT_DLY, 0x80);
+	ICN6211_DSI(icn, HFP_MIN, 0x28);
+	ICN6211_DSI(icn, MIPI_PD_CK_LANE, 0xa0);
+	ICN6211_DSI(icn, PLL_CTRL(12), 0xff);
+	ICN6211_DSI(icn, BIST_POL, BIST_POL_DE_POL);
+	ICN6211_DSI(icn, PLL_CTRL(6), PLL_CTRL_6_MIPI_CLK);
+	ICN6211_DSI(icn, PLL_REF_DIV, 0x71);
+	ICN6211_DSI(icn, PLL_INT(0), 0x2b);
+	ICN6211_DSI(icn, SYS_CTRL(0), 0x40);
+	ICN6211_DSI(icn, SYS_CTRL(1), 0x98);
 
 	/* icn6211 specific sequence */
-	ICN6211_DSI(icn, 0xb6, 0x20);
-	ICN6211_DSI(icn, 0x51, 0x20);
-	ICN6211_DSI(icn, 0x09, 0x10);
+	ICN6211_DSI(icn, MIPI_FORCE_0, 0x20);
+	ICN6211_DSI(icn, PLL_CTRL(1), 0x20);
+	ICN6211_DSI(icn, CONFIG_FINISH, 0x10);
 
 	usleep_range(10000, 11000);
 }
-- 
2.34.1

[PATCH V3 03/13] drm: bridge: icn6211: Fix HFP_HSW_HBP_HI and HFP_MIN handling

[Marek Vasut]

The HFP_HSW_HBP_HI register must be programmed with 2 LSbits of each
Horizontal Front Porch/Sync/Back Porch. Currently the driver programs
this register to 0, which breaks displays with either value above 255.

The HFP_MIN register must be set to the same value as HFP_LI, otherwise
there is visible image distortion, usually in the form of missing lines
at the bottom of the panel.

Fix this by correctly programming the HFP_HSW_HBP_HI and HFP_MIN registers.

Acked-by: Maxime Ripard <maxime@cerno.tech>
Fixes: ce517f18944e3 ("drm: bridge: Add Chipone ICN6211 MIPI-DSI to RGB bridge")
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
V2: Rebase on next-20220214
V3: Add AB from Maxime
---
 drivers/gpu/drm/bridge/chipone-icn6211.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 4b8d1a5a50302..e29e6a84c39a6 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -35,6 +35,9 @@
 #define HSYNC_LI		0x24
 #define HBP_LI			0x25
 #define HFP_HSW_HBP_HI		0x26
+#define HFP_HSW_HBP_HI_HFP(n)		(((n) & 0x300) >> 4)
+#define HFP_HSW_HBP_HI_HS(n)		(((n) & 0x300) >> 6)
+#define HFP_HSW_HBP_HI_HBP(n)		(((n) & 0x300) >> 8)
 #define VFP			0x27
 #define VSYNC			0x28
 #define VBP			0x29
@@ -163,6 +166,7 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 {
 	struct chipone *icn = bridge_to_chipone(bridge);
 	struct drm_display_mode *mode = &icn->mode;
+	u16 hfp, hbp, hsync;
 
 	ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_DSI);
 
@@ -178,13 +182,18 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 		    ((mode->hdisplay >> 8) & 0xf) |
 		    (((mode->vdisplay >> 8) & 0xf) << 4));
 
-	ICN6211_DSI(icn, HFP_LI, mode->hsync_start - mode->hdisplay);
+	hfp = mode->hsync_start - mode->hdisplay;
+	hsync = mode->hsync_end - mode->hsync_start;
+	hbp = mode->htotal - mode->hsync_end;
 
-	ICN6211_DSI(icn, HSYNC_LI, mode->hsync_end - mode->hsync_start);
-
-	ICN6211_DSI(icn, HBP_LI, mode->htotal - mode->hsync_end);
-
-	ICN6211_DSI(icn, HFP_HSW_HBP_HI, 0x00);
+	ICN6211_DSI(icn, HFP_LI, hfp & 0xff);
+	ICN6211_DSI(icn, HSYNC_LI, hsync & 0xff);
+	ICN6211_DSI(icn, HBP_LI, hbp & 0xff);
+	/* Top two bits of Horizontal Front porch/Sync/Back porch */
+	ICN6211_DSI(icn, HFP_HSW_HBP_HI,
+		    HFP_HSW_HBP_HI_HFP(hfp) |
+		    HFP_HSW_HBP_HI_HS(hsync) |
+		    HFP_HSW_HBP_HI_HBP(hbp));
 
 	ICN6211_DSI(icn, VFP, mode->vsync_start - mode->vdisplay);
 
@@ -194,7 +203,7 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 
 	/* dsi specific sequence */
 	ICN6211_DSI(icn, SYNC_EVENT_DLY, 0x80);
-	ICN6211_DSI(icn, HFP_MIN, 0x28);
+	ICN6211_DSI(icn, HFP_MIN, hfp & 0xff);
 	ICN6211_DSI(icn, MIPI_PD_CK_LANE, 0xa0);
 	ICN6211_DSI(icn, PLL_CTRL(12), 0xff);
 	ICN6211_DSI(icn, BIST_POL, BIST_POL_DE_POL);
-- 
2.34.1

[PATCH V3 04/13] drm: bridge: icn6211: Add HS/VS/DE polarity handling

[Marek Vasut]

The driver currently hard-codes HS/VS polarity to active-low and DE to
active-high, which is not correct for a lot of supported DPI panels.
Add the missing mode flag handling for HS/VS/DE polarity.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
V2: Rebase on next-20220214
V3: No change
---
 drivers/gpu/drm/bridge/chipone-icn6211.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index e29e6a84c39a6..2ac8eb7e25f52 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -165,8 +165,16 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 				  struct drm_bridge_state *old_bridge_state)
 {
 	struct chipone *icn = bridge_to_chipone(bridge);
+	struct drm_atomic_state *state = old_bridge_state->base.state;
 	struct drm_display_mode *mode = &icn->mode;
+	const struct drm_bridge_state *bridge_state;
 	u16 hfp, hbp, hsync;
+	u32 bus_flags;
+	u8 pol;
+
+	/* Get the DPI flags from the bridge state. */
+	bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
+	bus_flags = bridge_state->output_bus_cfg.flags;
 
 	ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_DSI);
 
@@ -206,7 +214,13 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 	ICN6211_DSI(icn, HFP_MIN, hfp & 0xff);
 	ICN6211_DSI(icn, MIPI_PD_CK_LANE, 0xa0);
 	ICN6211_DSI(icn, PLL_CTRL(12), 0xff);
-	ICN6211_DSI(icn, BIST_POL, BIST_POL_DE_POL);
+
+	/* DPI HS/VS/DE polarity */
+	pol = ((mode->flags & DRM_MODE_FLAG_PHSYNC) ? BIST_POL_HSYNC_POL : 0) |
+	      ((mode->flags & DRM_MODE_FLAG_PVSYNC) ? BIST_POL_VSYNC_POL : 0) |
+	      ((bus_flags & DRM_BUS_FLAG_DE_HIGH) ? BIST_POL_DE_POL : 0);
+	ICN6211_DSI(icn, BIST_POL, pol);
+
 	ICN6211_DSI(icn, PLL_CTRL(6), PLL_CTRL_6_MIPI_CLK);
 	ICN6211_DSI(icn, PLL_REF_DIV, 0x71);
 	ICN6211_DSI(icn, PLL_INT(0), 0x2b);
-- 
2.34.1

Re: [PATCH V3 04/13] drm: bridge: icn6211: Add HS/VS/DE polarity handling

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 633 bytes --]

On Fri, Mar 04, 2022 at 01:24:59AM +0100, Marek Vasut wrote:
> The driver currently hard-codes HS/VS polarity to active-low and DE to
> active-high, which is not correct for a lot of supported DPI panels.
> Add the missing mode flag handling for HS/VS/DE polarity.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> To: dri-devel@lists.freedesktop.org

Acked-by: Maxime Ripard <maxime@cerno.tech>

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Marek Vasut]

The driver currently hard-codes DSI lane count to two, however the chip
is capable of operating in 1..4 DSI lanes mode. Parse 'data-lanes' DT
property and program the result into DSI_CTRL register.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
V2: Rebase on next-20220214
V3: Default to 4 data lanes unless specified otherwise
---
 drivers/gpu/drm/bridge/chipone-icn6211.c | 45 +++++++++++++++++++++---
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 2ac8eb7e25f52..df8e75a068ad0 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -136,10 +136,12 @@ struct chipone {
 	struct drm_bridge bridge;
 	struct drm_display_mode mode;
 	struct drm_bridge *panel_bridge;
+	struct device_node *host_node;
 	struct gpio_desc *enable_gpio;
 	struct regulator *vdd1;
 	struct regulator *vdd2;
 	struct regulator *vdd3;
+	int dsi_lanes;
 };
 
 static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
@@ -212,6 +214,11 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 	/* dsi specific sequence */
 	ICN6211_DSI(icn, SYNC_EVENT_DLY, 0x80);
 	ICN6211_DSI(icn, HFP_MIN, hfp & 0xff);
+
+	/* DSI data lane count */
+	ICN6211_DSI(icn, DSI_CTRL,
+		    DSI_CTRL_UNKNOWN | DSI_CTRL_DSI_LANES(icn->dsi_lanes - 1));
+
 	ICN6211_DSI(icn, MIPI_PD_CK_LANE, 0xa0);
 	ICN6211_DSI(icn, PLL_CTRL(12), 0xff);
 
@@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
 static int chipone_parse_dt(struct chipone *icn)
 {
 	struct device *dev = icn->dev;
+	struct device_node *endpoint;
 	struct drm_panel *panel;
+	int dsi_lanes;
 	int ret;
 
 	icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
@@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
 		return PTR_ERR(icn->enable_gpio);
 	}
 
+	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
+	dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
+	icn->host_node = of_graph_get_remote_port_parent(endpoint);
+	of_node_put(endpoint);
+
+	if (!icn->host_node)
+		return -ENODEV;
+
+	/*
+	 * If the 'data-lanes' property does not exist in DT or is invalid,
+	 * default to previously hard-coded behavior, which was 4 data lanes.
+	 */
+	if (dsi_lanes < 0) {
+		icn->dsi_lanes = 4;
+	} else if (dsi_lanes > 4) {
+		ret = -EINVAL;
+		goto err_data_lanes;
+	} else {
+		icn->dsi_lanes = dsi_lanes;
+	}
+
 	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
 	if (ret)
-		return ret;
+		goto err_data_lanes;
 
 	icn->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
-	if (IS_ERR(icn->panel_bridge))
-		return PTR_ERR(icn->panel_bridge);
+	if (IS_ERR(icn->panel_bridge)) {
+		ret = PTR_ERR(icn->panel_bridge);
+		goto err_data_lanes;
+	}
 
 	return 0;
+
+err_data_lanes:
+	of_node_put(icn->host_node);
+	return ret;
 }
 
 static int chipone_probe(struct mipi_dsi_device *dsi)
@@ -384,7 +420,7 @@ static int chipone_probe(struct mipi_dsi_device *dsi)
 
 	drm_bridge_add(&icn->bridge);
 
-	dsi->lanes = 4;
+	dsi->lanes = icn->dsi_lanes;
 	dsi->format = MIPI_DSI_FMT_RGB888;
 	dsi->mode_flags = MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
 
@@ -403,6 +439,7 @@ static int chipone_remove(struct mipi_dsi_device *dsi)
 
 	mipi_dsi_detach(dsi);
 	drm_bridge_remove(&icn->bridge);
+	of_node_put(icn->host_node);
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Jagan Teki]

Hi Marek,

On Fri, Mar 4, 2022 at 5:55 AM Marek Vasut <marex@denx.de> wrote:
>
> The driver currently hard-codes DSI lane count to two, however the chip
> is capable of operating in 1..4 DSI lanes mode. Parse 'data-lanes' DT
> property and program the result into DSI_CTRL register.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> To: dri-devel@lists.freedesktop.org
> ---
> V2: Rebase on next-20220214
> V3: Default to 4 data lanes unless specified otherwise
> ---
>  drivers/gpu/drm/bridge/chipone-icn6211.c | 45 +++++++++++++++++++++---
>  1 file changed, 41 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
> index 2ac8eb7e25f52..df8e75a068ad0 100644
> --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> @@ -136,10 +136,12 @@ struct chipone {
>         struct drm_bridge bridge;
>         struct drm_display_mode mode;
>         struct drm_bridge *panel_bridge;
> +       struct device_node *host_node;
>         struct gpio_desc *enable_gpio;
>         struct regulator *vdd1;
>         struct regulator *vdd2;
>         struct regulator *vdd3;
> +       int dsi_lanes;
>  };
>
>  static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
> @@ -212,6 +214,11 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
>         /* dsi specific sequence */
>         ICN6211_DSI(icn, SYNC_EVENT_DLY, 0x80);
>         ICN6211_DSI(icn, HFP_MIN, hfp & 0xff);
> +
> +       /* DSI data lane count */
> +       ICN6211_DSI(icn, DSI_CTRL,
> +                   DSI_CTRL_UNKNOWN | DSI_CTRL_DSI_LANES(icn->dsi_lanes - 1));
> +
>         ICN6211_DSI(icn, MIPI_PD_CK_LANE, 0xa0);
>         ICN6211_DSI(icn, PLL_CTRL(12), 0xff);
>
> @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
>  static int chipone_parse_dt(struct chipone *icn)
>  {
>         struct device *dev = icn->dev;
> +       struct device_node *endpoint;
>         struct drm_panel *panel;
> +       int dsi_lanes;
>         int ret;
>
>         icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
> @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
>                 return PTR_ERR(icn->enable_gpio);
>         }
>
> +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
> +       of_node_put(endpoint);
> +
> +       if (!icn->host_node)
> +               return -ENODEV;

The non-ports-based OF graph returns a -19 example on the Allwinner
Display pipeline in R16 [1].

We need to have a helper to return host_node for non-ports as I have
done it for drm_of_find_bridge.

[1] https://patchwork.amarulasolutions.com/patch/1805/

Thanks,
Jagan.

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Marek Vasut]

On 3/8/22 09:03, Jagan Teki wrote:

Hi,

[...]

>> @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
>>   static int chipone_parse_dt(struct chipone *icn)
>>   {
>>          struct device *dev = icn->dev;
>> +       struct device_node *endpoint;
>>          struct drm_panel *panel;
>> +       int dsi_lanes;
>>          int ret;
>>
>>          icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
>> @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
>>                  return PTR_ERR(icn->enable_gpio);
>>          }
>>
>> +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
>> +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
>> +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
>> +       of_node_put(endpoint);
>> +
>> +       if (!icn->host_node)
>> +               return -ENODEV;
> 
> The non-ports-based OF graph returns a -19 example on the Allwinner
> Display pipeline in R16 [1].
> 
> We need to have a helper to return host_node for non-ports as I have
> done it for drm_of_find_bridge.
> 
> [1] https://patchwork.amarulasolutions.com/patch/1805/

The link points to a patch marked "DO NOT MERGE", maybe that patch is 
missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are 
required, see:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53

What is "non-ports-based OF graph" ?

I don't see drm_of_find_bridge() in linux-next , what is that ?

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Jagan Teki]

On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
>
> On 3/8/22 09:03, Jagan Teki wrote:
>
> Hi,
>
> [...]
>
> >> @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
> >>   static int chipone_parse_dt(struct chipone *icn)
> >>   {
> >>          struct device *dev = icn->dev;
> >> +       struct device_node *endpoint;
> >>          struct drm_panel *panel;
> >> +       int dsi_lanes;
> >>          int ret;
> >>
> >>          icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
> >> @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
> >>                  return PTR_ERR(icn->enable_gpio);
> >>          }
> >>
> >> +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> >> +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> >> +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
> >> +       of_node_put(endpoint);
> >> +
> >> +       if (!icn->host_node)
> >> +               return -ENODEV;
> >
> > The non-ports-based OF graph returns a -19 example on the Allwinner
> > Display pipeline in R16 [1].
> >
> > We need to have a helper to return host_node for non-ports as I have
> > done it for drm_of_find_bridge.
> >
> > [1] https://patchwork.amarulasolutions.com/patch/1805/
>
> The link points to a patch marked "DO NOT MERGE", maybe that patch is
> missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
> required, see:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
>
> What is "non-ports-based OF graph" ?
>
> I don't see drm_of_find_bridge() in linux-next , what is that ?

port@0 is optional as some of the DSI host OF-graph represent the
bridge or panel as child nodes instead of ports. (i think dt-binding
has to fix it to make port@0 optional)

Example OF-graph is on the commit message.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/gpu/drm/drm_of.c?id=80253168dbfd256bca97cf7f13312863c5a7f2e5

Jagan.

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Marek Vasut]

On 3/8/22 11:07, Jagan Teki wrote:
> On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 3/8/22 09:03, Jagan Teki wrote:
>>
>> Hi,
>>
>> [...]
>>
>>>> @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
>>>>    static int chipone_parse_dt(struct chipone *icn)
>>>>    {
>>>>           struct device *dev = icn->dev;
>>>> +       struct device_node *endpoint;
>>>>           struct drm_panel *panel;
>>>> +       int dsi_lanes;
>>>>           int ret;
>>>>
>>>>           icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
>>>> @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
>>>>                   return PTR_ERR(icn->enable_gpio);
>>>>           }
>>>>
>>>> +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
>>>> +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
>>>> +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
>>>> +       of_node_put(endpoint);
>>>> +
>>>> +       if (!icn->host_node)
>>>> +               return -ENODEV;
>>>
>>> The non-ports-based OF graph returns a -19 example on the Allwinner
>>> Display pipeline in R16 [1].
>>>
>>> We need to have a helper to return host_node for non-ports as I have
>>> done it for drm_of_find_bridge.
>>>
>>> [1] https://patchwork.amarulasolutions.com/patch/1805/
>>
>> The link points to a patch marked "DO NOT MERGE", maybe that patch is
>> missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
>> required, see:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
>>
>> What is "non-ports-based OF graph" ?
>>
>> I don't see drm_of_find_bridge() in linux-next , what is that ?
> 
> port@0 is optional as some of the DSI host OF-graph represent the
> bridge or panel as child nodes instead of ports. (i think dt-binding
> has to fix it to make port@0 optional)

The current upstream DT binding document says:

     required:
       - port@0
       - port@1

So port@0 is mandatory.

So I don't think any change is needed to this patch ?

> Example OF-graph is on the commit message.
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/gpu/drm/drm_of.c?id=80253168dbfd256bca97cf7f13312863c5a7f2e5

It seems the current upstream DT bindings only support DSI hosts which 
do provide an endpoint, because port@0 is required per DT binding 
document. If you want to support the other options as listed in the 
aforementioned commit, then you would have to extend this driver and its 
bindings, but that is out of scope of this series.

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Jagan Teki]

Hi Marek,

On Tue, Mar 8, 2022 at 4:00 PM Marek Vasut <marex@denx.de> wrote:
>
> On 3/8/22 11:07, Jagan Teki wrote:
> > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 3/8/22 09:03, Jagan Teki wrote:
> >>
> >> Hi,
> >>
> >> [...]
> >>
> >>>> @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
> >>>>    static int chipone_parse_dt(struct chipone *icn)
> >>>>    {
> >>>>           struct device *dev = icn->dev;
> >>>> +       struct device_node *endpoint;
> >>>>           struct drm_panel *panel;
> >>>> +       int dsi_lanes;
> >>>>           int ret;
> >>>>
> >>>>           icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
> >>>> @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
> >>>>                   return PTR_ERR(icn->enable_gpio);
> >>>>           }
> >>>>
> >>>> +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> >>>> +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> >>>> +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
> >>>> +       of_node_put(endpoint);
> >>>> +
> >>>> +       if (!icn->host_node)
> >>>> +               return -ENODEV;
> >>>
> >>> The non-ports-based OF graph returns a -19 example on the Allwinner
> >>> Display pipeline in R16 [1].
> >>>
> >>> We need to have a helper to return host_node for non-ports as I have
> >>> done it for drm_of_find_bridge.
> >>>
> >>> [1] https://patchwork.amarulasolutions.com/patch/1805/
> >>
> >> The link points to a patch marked "DO NOT MERGE", maybe that patch is
> >> missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
> >> required, see:
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
> >>
> >> What is "non-ports-based OF graph" ?
> >>
> >> I don't see drm_of_find_bridge() in linux-next , what is that ?
> >
> > port@0 is optional as some of the DSI host OF-graph represent the
> > bridge or panel as child nodes instead of ports. (i think dt-binding
> > has to fix it to make port@0 optional)
>
> The current upstream DT binding document says:
>
>      required:
>        - port@0
>        - port@1
>
> So port@0 is mandatory.
>
> So I don't think any change is needed to this patch ?
>
> > Example OF-graph is on the commit message.
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/gpu/drm/drm_of.c?id=80253168dbfd256bca97cf7f13312863c5a7f2e5
>
> It seems the current upstream DT bindings only support DSI hosts which
> do provide an endpoint, because port@0 is required per DT binding
> document. If you want to support the other options as listed in the
> aforementioned commit, then you would have to extend this driver and its
> bindings, but that is out of scope of this series.

Your comments seem to be valid, thanks.

Jagan.

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2872 bytes --]

On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
> On 3/8/22 11:07, Jagan Teki wrote:
> > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
> > > 
> > > On 3/8/22 09:03, Jagan Teki wrote:
> > > 
> > > Hi,
> > > 
> > > [...]
> > > 
> > > > > @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
> > > > >    static int chipone_parse_dt(struct chipone *icn)
> > > > >    {
> > > > >           struct device *dev = icn->dev;
> > > > > +       struct device_node *endpoint;
> > > > >           struct drm_panel *panel;
> > > > > +       int dsi_lanes;
> > > > >           int ret;
> > > > > 
> > > > >           icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
> > > > > @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
> > > > >                   return PTR_ERR(icn->enable_gpio);
> > > > >           }
> > > > > 
> > > > > +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> > > > > +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> > > > > +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
> > > > > +       of_node_put(endpoint);
> > > > > +
> > > > > +       if (!icn->host_node)
> > > > > +               return -ENODEV;
> > > > 
> > > > The non-ports-based OF graph returns a -19 example on the Allwinner
> > > > Display pipeline in R16 [1].
> > > > 
> > > > We need to have a helper to return host_node for non-ports as I have
> > > > done it for drm_of_find_bridge.
> > > > 
> > > > [1] https://patchwork.amarulasolutions.com/patch/1805/
> > > 
> > > The link points to a patch marked "DO NOT MERGE", maybe that patch is
> > > missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
> > > required, see:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
> > > 
> > > What is "non-ports-based OF graph" ?
> > > 
> > > I don't see drm_of_find_bridge() in linux-next , what is that ?
> > 
> > port@0 is optional as some of the DSI host OF-graph represent the
> > bridge or panel as child nodes instead of ports. (i think dt-binding
> > has to fix it to make port@0 optional)
> 
> The current upstream DT binding document says:
> 
>     required:
>       - port@0
>       - port@1
> 
> So port@0 is mandatory.

In the binding, sure, but fundamentally the DT excerpt Jagan provided is
correct. If the bridge supports DCS, there's no reason to use the OF
graph in the first place: the bridge node will be a child node of the
MIPI-DSI controller (and there's no obligation to use the OF-graph for a
MIPI-DSI controller).

I believe port@0 should be made optional (or downright removed if
MIPI-DCS in the only control bus).

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Marek Vasut]

On 3/8/22 13:51, Maxime Ripard wrote:
> On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
>> On 3/8/22 11:07, Jagan Teki wrote:
>>> On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 3/8/22 09:03, Jagan Teki wrote:
>>>>
>>>> Hi,
>>>>
>>>> [...]
>>>>
>>>>>> @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
>>>>>>     static int chipone_parse_dt(struct chipone *icn)
>>>>>>     {
>>>>>>            struct device *dev = icn->dev;
>>>>>> +       struct device_node *endpoint;
>>>>>>            struct drm_panel *panel;
>>>>>> +       int dsi_lanes;
>>>>>>            int ret;
>>>>>>
>>>>>>            icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
>>>>>> @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
>>>>>>                    return PTR_ERR(icn->enable_gpio);
>>>>>>            }
>>>>>>
>>>>>> +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
>>>>>> +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
>>>>>> +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
>>>>>> +       of_node_put(endpoint);
>>>>>> +
>>>>>> +       if (!icn->host_node)
>>>>>> +               return -ENODEV;
>>>>>
>>>>> The non-ports-based OF graph returns a -19 example on the Allwinner
>>>>> Display pipeline in R16 [1].
>>>>>
>>>>> We need to have a helper to return host_node for non-ports as I have
>>>>> done it for drm_of_find_bridge.
>>>>>
>>>>> [1] https://patchwork.amarulasolutions.com/patch/1805/
>>>>
>>>> The link points to a patch marked "DO NOT MERGE", maybe that patch is
>>>> missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
>>>> required, see:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
>>>>
>>>> What is "non-ports-based OF graph" ?
>>>>
>>>> I don't see drm_of_find_bridge() in linux-next , what is that ?
>>>
>>> port@0 is optional as some of the DSI host OF-graph represent the
>>> bridge or panel as child nodes instead of ports. (i think dt-binding
>>> has to fix it to make port@0 optional)
>>
>> The current upstream DT binding document says:
>>
>>      required:
>>        - port@0
>>        - port@1
>>
>> So port@0 is mandatory.
> 
> In the binding, sure, but fundamentally the DT excerpt Jagan provided is
> correct. If the bridge supports DCS, there's no reason to use the OF
> graph in the first place: the bridge node will be a child node of the
> MIPI-DSI controller (and there's no obligation to use the OF-graph for a
> MIPI-DSI controller).
> 
> I believe port@0 should be made optional (or downright removed if
> MIPI-DCS in the only control bus).

That's out of scope of this series anyway, so Jagan can implement 
patches for that mode if needed.

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3554 bytes --]

On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote:
> On 3/8/22 13:51, Maxime Ripard wrote:
> > On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
> > > On 3/8/22 11:07, Jagan Teki wrote:
> > > > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
> > > > > 
> > > > > On 3/8/22 09:03, Jagan Teki wrote:
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > > @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
> > > > > > >     static int chipone_parse_dt(struct chipone *icn)
> > > > > > >     {
> > > > > > >            struct device *dev = icn->dev;
> > > > > > > +       struct device_node *endpoint;
> > > > > > >            struct drm_panel *panel;
> > > > > > > +       int dsi_lanes;
> > > > > > >            int ret;
> > > > > > > 
> > > > > > >            icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
> > > > > > > @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
> > > > > > >                    return PTR_ERR(icn->enable_gpio);
> > > > > > >            }
> > > > > > > 
> > > > > > > +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> > > > > > > +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> > > > > > > +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
> > > > > > > +       of_node_put(endpoint);
> > > > > > > +
> > > > > > > +       if (!icn->host_node)
> > > > > > > +               return -ENODEV;
> > > > > > 
> > > > > > The non-ports-based OF graph returns a -19 example on the Allwinner
> > > > > > Display pipeline in R16 [1].
> > > > > > 
> > > > > > We need to have a helper to return host_node for non-ports as I have
> > > > > > done it for drm_of_find_bridge.
> > > > > > 
> > > > > > [1] https://patchwork.amarulasolutions.com/patch/1805/
> > > > > 
> > > > > The link points to a patch marked "DO NOT MERGE", maybe that patch is
> > > > > missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
> > > > > required, see:
> > > > > 
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
> > > > > 
> > > > > What is "non-ports-based OF graph" ?
> > > > > 
> > > > > I don't see drm_of_find_bridge() in linux-next , what is that ?
> > > > 
> > > > port@0 is optional as some of the DSI host OF-graph represent the
> > > > bridge or panel as child nodes instead of ports. (i think dt-binding
> > > > has to fix it to make port@0 optional)
> > > 
> > > The current upstream DT binding document says:
> > > 
> > >      required:
> > >        - port@0
> > >        - port@1
> > > 
> > > So port@0 is mandatory.
> > 
> > In the binding, sure, but fundamentally the DT excerpt Jagan provided is
> > correct. If the bridge supports DCS, there's no reason to use the OF
> > graph in the first place: the bridge node will be a child node of the
> > MIPI-DSI controller (and there's no obligation to use the OF-graph for a
> > MIPI-DSI controller).
> > 
> > I believe port@0 should be made optional (or downright removed if
> > MIPI-DCS in the only control bus).
> 
> That's out of scope of this series anyway, so Jagan can implement patches
> for that mode if needed.

Not really? You can't count on the port@0 being there generally
speaking, so you can't count on data-lanes being there either, which
exactly what you're doing in this patch.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Marek Vasut]

On 3/8/22 14:49, Maxime Ripard wrote:
> On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote:
>> On 3/8/22 13:51, Maxime Ripard wrote:
>>> On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
>>>> On 3/8/22 11:07, Jagan Teki wrote:
>>>>> On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 3/8/22 09:03, Jagan Teki wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>> @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
>>>>>>>>      static int chipone_parse_dt(struct chipone *icn)
>>>>>>>>      {
>>>>>>>>             struct device *dev = icn->dev;
>>>>>>>> +       struct device_node *endpoint;
>>>>>>>>             struct drm_panel *panel;
>>>>>>>> +       int dsi_lanes;
>>>>>>>>             int ret;
>>>>>>>>
>>>>>>>>             icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
>>>>>>>> @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
>>>>>>>>                     return PTR_ERR(icn->enable_gpio);
>>>>>>>>             }
>>>>>>>>
>>>>>>>> +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
>>>>>>>> +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
>>>>>>>> +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
>>>>>>>> +       of_node_put(endpoint);
>>>>>>>> +
>>>>>>>> +       if (!icn->host_node)
>>>>>>>> +               return -ENODEV;
>>>>>>>
>>>>>>> The non-ports-based OF graph returns a -19 example on the Allwinner
>>>>>>> Display pipeline in R16 [1].
>>>>>>>
>>>>>>> We need to have a helper to return host_node for non-ports as I have
>>>>>>> done it for drm_of_find_bridge.
>>>>>>>
>>>>>>> [1] https://patchwork.amarulasolutions.com/patch/1805/
>>>>>>
>>>>>> The link points to a patch marked "DO NOT MERGE", maybe that patch is
>>>>>> missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
>>>>>> required, see:
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
>>>>>>
>>>>>> What is "non-ports-based OF graph" ?
>>>>>>
>>>>>> I don't see drm_of_find_bridge() in linux-next , what is that ?
>>>>>
>>>>> port@0 is optional as some of the DSI host OF-graph represent the
>>>>> bridge or panel as child nodes instead of ports. (i think dt-binding
>>>>> has to fix it to make port@0 optional)
>>>>
>>>> The current upstream DT binding document says:
>>>>
>>>>       required:
>>>>         - port@0
>>>>         - port@1
>>>>
>>>> So port@0 is mandatory.
>>>
>>> In the binding, sure, but fundamentally the DT excerpt Jagan provided is
>>> correct. If the bridge supports DCS, there's no reason to use the OF
>>> graph in the first place: the bridge node will be a child node of the
>>> MIPI-DSI controller (and there's no obligation to use the OF-graph for a
>>> MIPI-DSI controller).
>>>
>>> I believe port@0 should be made optional (or downright removed if
>>> MIPI-DCS in the only control bus).
>>
>> That's out of scope of this series anyway, so Jagan can implement patches
>> for that mode if needed.
> 
> Not really? You can't count on the port@0 being there generally
> speaking, so you can't count on data-lanes being there either, which
> exactly what you're doing in this patch.

I can because the upstream DT bindings currently say port@0 must be 
present, see above. If that requirement should be relaxed, sure, but 
that's a separate series.

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4559 bytes --]

On Tue, Mar 08, 2022 at 03:47:22PM +0100, Marek Vasut wrote:
> On 3/8/22 14:49, Maxime Ripard wrote:
> > On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote:
> > > On 3/8/22 13:51, Maxime Ripard wrote:
> > > > On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
> > > > > On 3/8/22 11:07, Jagan Teki wrote:
> > > > > > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
> > > > > > > 
> > > > > > > On 3/8/22 09:03, Jagan Teki wrote:
> > > > > > > 
> > > > > > > Hi,
> > > > > > > 
> > > > > > > [...]
> > > > > > > 
> > > > > > > > > @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
> > > > > > > > >      static int chipone_parse_dt(struct chipone *icn)
> > > > > > > > >      {
> > > > > > > > >             struct device *dev = icn->dev;
> > > > > > > > > +       struct device_node *endpoint;
> > > > > > > > >             struct drm_panel *panel;
> > > > > > > > > +       int dsi_lanes;
> > > > > > > > >             int ret;
> > > > > > > > > 
> > > > > > > > >             icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
> > > > > > > > > @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
> > > > > > > > >                     return PTR_ERR(icn->enable_gpio);
> > > > > > > > >             }
> > > > > > > > > 
> > > > > > > > > +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> > > > > > > > > +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> > > > > > > > > +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
> > > > > > > > > +       of_node_put(endpoint);
> > > > > > > > > +
> > > > > > > > > +       if (!icn->host_node)
> > > > > > > > > +               return -ENODEV;
> > > > > > > > 
> > > > > > > > The non-ports-based OF graph returns a -19 example on the Allwinner
> > > > > > > > Display pipeline in R16 [1].
> > > > > > > > 
> > > > > > > > We need to have a helper to return host_node for non-ports as I have
> > > > > > > > done it for drm_of_find_bridge.
> > > > > > > > 
> > > > > > > > [1] https://patchwork.amarulasolutions.com/patch/1805/
> > > > > > > 
> > > > > > > The link points to a patch marked "DO NOT MERGE", maybe that patch is
> > > > > > > missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
> > > > > > > required, see:
> > > > > > > 
> > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
> > > > > > > 
> > > > > > > What is "non-ports-based OF graph" ?
> > > > > > > 
> > > > > > > I don't see drm_of_find_bridge() in linux-next , what is that ?
> > > > > > 
> > > > > > port@0 is optional as some of the DSI host OF-graph represent the
> > > > > > bridge or panel as child nodes instead of ports. (i think dt-binding
> > > > > > has to fix it to make port@0 optional)
> > > > > 
> > > > > The current upstream DT binding document says:
> > > > > 
> > > > >       required:
> > > > >         - port@0
> > > > >         - port@1
> > > > > 
> > > > > So port@0 is mandatory.
> > > > 
> > > > In the binding, sure, but fundamentally the DT excerpt Jagan provided is
> > > > correct. If the bridge supports DCS, there's no reason to use the OF
> > > > graph in the first place: the bridge node will be a child node of the
> > > > MIPI-DSI controller (and there's no obligation to use the OF-graph for a
> > > > MIPI-DSI controller).
> > > > 
> > > > I believe port@0 should be made optional (or downright removed if
> > > > MIPI-DCS in the only control bus).
> > > 
> > > That's out of scope of this series anyway, so Jagan can implement patches
> > > for that mode if needed.
> > 
> > Not really? You can't count on the port@0 being there generally
> > speaking, so you can't count on data-lanes being there either, which
> > exactly what you're doing in this patch.
> 
> I can because the upstream DT bindings currently say port@0 must be present,
> see above. If that requirement should be relaxed, sure, but that's a
> separate series.

And another upstream DT bindings say that you don't need them at all.
Yes, there's a conflict. Yes, it's unfortunate. But the generic DSI
binding is far more relevant than a single bridge driver.

So figuring it out is very much a prerequisite to that series,
especially since those patches effectively make the OF-graph mandatory
in some situations, while it was purely aesthetics before.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Marek Vasut]

On 3/8/22 17:21, Maxime Ripard wrote:
> On Tue, Mar 08, 2022 at 03:47:22PM +0100, Marek Vasut wrote:
>> On 3/8/22 14:49, Maxime Ripard wrote:
>>> On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote:
>>>> On 3/8/22 13:51, Maxime Ripard wrote:
>>>>> On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
>>>>>> On 3/8/22 11:07, Jagan Teki wrote:
>>>>>>> On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>
>>>>>>>> On 3/8/22 09:03, Jagan Teki wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>>> @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
>>>>>>>>>>       static int chipone_parse_dt(struct chipone *icn)
>>>>>>>>>>       {
>>>>>>>>>>              struct device *dev = icn->dev;
>>>>>>>>>> +       struct device_node *endpoint;
>>>>>>>>>>              struct drm_panel *panel;
>>>>>>>>>> +       int dsi_lanes;
>>>>>>>>>>              int ret;
>>>>>>>>>>
>>>>>>>>>>              icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
>>>>>>>>>> @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
>>>>>>>>>>                      return PTR_ERR(icn->enable_gpio);
>>>>>>>>>>              }
>>>>>>>>>>
>>>>>>>>>> +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
>>>>>>>>>> +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
>>>>>>>>>> +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
>>>>>>>>>> +       of_node_put(endpoint);
>>>>>>>>>> +
>>>>>>>>>> +       if (!icn->host_node)
>>>>>>>>>> +               return -ENODEV;
>>>>>>>>>
>>>>>>>>> The non-ports-based OF graph returns a -19 example on the Allwinner
>>>>>>>>> Display pipeline in R16 [1].
>>>>>>>>>
>>>>>>>>> We need to have a helper to return host_node for non-ports as I have
>>>>>>>>> done it for drm_of_find_bridge.
>>>>>>>>>
>>>>>>>>> [1] https://patchwork.amarulasolutions.com/patch/1805/
>>>>>>>>
>>>>>>>> The link points to a patch marked "DO NOT MERGE", maybe that patch is
>>>>>>>> missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
>>>>>>>> required, see:
>>>>>>>>
>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
>>>>>>>>
>>>>>>>> What is "non-ports-based OF graph" ?
>>>>>>>>
>>>>>>>> I don't see drm_of_find_bridge() in linux-next , what is that ?
>>>>>>>
>>>>>>> port@0 is optional as some of the DSI host OF-graph represent the
>>>>>>> bridge or panel as child nodes instead of ports. (i think dt-binding
>>>>>>> has to fix it to make port@0 optional)
>>>>>>
>>>>>> The current upstream DT binding document says:
>>>>>>
>>>>>>        required:
>>>>>>          - port@0
>>>>>>          - port@1
>>>>>>
>>>>>> So port@0 is mandatory.
>>>>>
>>>>> In the binding, sure, but fundamentally the DT excerpt Jagan provided is
>>>>> correct. If the bridge supports DCS, there's no reason to use the OF
>>>>> graph in the first place: the bridge node will be a child node of the
>>>>> MIPI-DSI controller (and there's no obligation to use the OF-graph for a
>>>>> MIPI-DSI controller).
>>>>>
>>>>> I believe port@0 should be made optional (or downright removed if
>>>>> MIPI-DCS in the only control bus).
>>>>
>>>> That's out of scope of this series anyway, so Jagan can implement patches
>>>> for that mode if needed.
>>>
>>> Not really? You can't count on the port@0 being there generally
>>> speaking, so you can't count on data-lanes being there either, which
>>> exactly what you're doing in this patch.
>>
>> I can because the upstream DT bindings currently say port@0 must be present,
>> see above. If that requirement should be relaxed, sure, but that's a
>> separate series.
> 
> And another upstream DT bindings say that you don't need them at all.

Which "another upstream DT bindings" do you refer to ?

> Yes, there's a conflict. Yes, it's unfortunate. But the generic DSI
> binding is far more relevant than a single bridge driver.

That seems to be the wrong way around, how can generic subsystem-wide 
binding take precedence over specific driver binding ?

> So figuring it out is very much a prerequisite to that series,
> especially since those patches effectively make the OF-graph mandatory
> in some situations, while it was purely aesthetics before.

The OF-graph is mandatory per the DT bindings of this driver. If you 
implement invalid DT which does not contain port@0, it will fail DT 
validation.

If this requirement should be relaxed, sure, it can and I don't think it 
would be hard to do, but I don't see why that should be part of this 
series, which follows the upstream DT binding document for this driver.

If I cannot trust the driver DT bindings to indicate what is and is not 
mandatory, what other document can I trust then ...

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6627 bytes --]

On Tue, Mar 08, 2022 at 10:41:05PM +0100, Marek Vasut wrote:
> On 3/8/22 17:21, Maxime Ripard wrote:
> > On Tue, Mar 08, 2022 at 03:47:22PM +0100, Marek Vasut wrote:
> > > On 3/8/22 14:49, Maxime Ripard wrote:
> > > > On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote:
> > > > > On 3/8/22 13:51, Maxime Ripard wrote:
> > > > > > On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
> > > > > > > On 3/8/22 11:07, Jagan Teki wrote:
> > > > > > > > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
> > > > > > > > > 
> > > > > > > > > On 3/8/22 09:03, Jagan Teki wrote:
> > > > > > > > > 
> > > > > > > > > Hi,
> > > > > > > > > 
> > > > > > > > > [...]
> > > > > > > > > 
> > > > > > > > > > > @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
> > > > > > > > > > >       static int chipone_parse_dt(struct chipone *icn)
> > > > > > > > > > >       {
> > > > > > > > > > >              struct device *dev = icn->dev;
> > > > > > > > > > > +       struct device_node *endpoint;
> > > > > > > > > > >              struct drm_panel *panel;
> > > > > > > > > > > +       int dsi_lanes;
> > > > > > > > > > >              int ret;
> > > > > > > > > > > 
> > > > > > > > > > >              icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
> > > > > > > > > > > @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
> > > > > > > > > > >                      return PTR_ERR(icn->enable_gpio);
> > > > > > > > > > >              }
> > > > > > > > > > > 
> > > > > > > > > > > +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> > > > > > > > > > > +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> > > > > > > > > > > +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
> > > > > > > > > > > +       of_node_put(endpoint);
> > > > > > > > > > > +
> > > > > > > > > > > +       if (!icn->host_node)
> > > > > > > > > > > +               return -ENODEV;
> > > > > > > > > > 
> > > > > > > > > > The non-ports-based OF graph returns a -19 example on the Allwinner
> > > > > > > > > > Display pipeline in R16 [1].
> > > > > > > > > > 
> > > > > > > > > > We need to have a helper to return host_node for non-ports as I have
> > > > > > > > > > done it for drm_of_find_bridge.
> > > > > > > > > > 
> > > > > > > > > > [1] https://patchwork.amarulasolutions.com/patch/1805/
> > > > > > > > > 
> > > > > > > > > The link points to a patch marked "DO NOT MERGE", maybe that patch is
> > > > > > > > > missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
> > > > > > > > > required, see:
> > > > > > > > > 
> > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
> > > > > > > > > 
> > > > > > > > > What is "non-ports-based OF graph" ?
> > > > > > > > > 
> > > > > > > > > I don't see drm_of_find_bridge() in linux-next , what is that ?
> > > > > > > > 
> > > > > > > > port@0 is optional as some of the DSI host OF-graph represent the
> > > > > > > > bridge or panel as child nodes instead of ports. (i think dt-binding
> > > > > > > > has to fix it to make port@0 optional)
> > > > > > > 
> > > > > > > The current upstream DT binding document says:
> > > > > > > 
> > > > > > >        required:
> > > > > > >          - port@0
> > > > > > >          - port@1
> > > > > > > 
> > > > > > > So port@0 is mandatory.
> > > > > > 
> > > > > > In the binding, sure, but fundamentally the DT excerpt Jagan provided is
> > > > > > correct. If the bridge supports DCS, there's no reason to use the OF
> > > > > > graph in the first place: the bridge node will be a child node of the
> > > > > > MIPI-DSI controller (and there's no obligation to use the OF-graph for a
> > > > > > MIPI-DSI controller).
> > > > > > 
> > > > > > I believe port@0 should be made optional (or downright removed if
> > > > > > MIPI-DCS in the only control bus).
> > > > > 
> > > > > That's out of scope of this series anyway, so Jagan can implement patches
> > > > > for that mode if needed.
> > > > 
> > > > Not really? You can't count on the port@0 being there generally
> > > > speaking, so you can't count on data-lanes being there either, which
> > > > exactly what you're doing in this patch.
> > > 
> > > I can because the upstream DT bindings currently say port@0 must be present,
> > > see above. If that requirement should be relaxed, sure, but that's a
> > > separate series.
> > 
> > And another upstream DT bindings say that you don't need them at all.
> 
> Which "another upstream DT bindings" do you refer to ?

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt

> > Yes, there's a conflict. Yes, it's unfortunate. But the generic DSI
> > binding is far more relevant than a single bridge driver.
> 
> That seems to be the wrong way around, how can generic subsystem-wide
> binding take precedence over specific driver binding ?

This is the binding of the bus. You're part of that bus. You're a child
node of that bus, but nothing ever mandates that your parent node uses
the same convention. And some don't. And since your bridge can be
connected to pretty much any DSI controller, you have to use the lowest
common denominator, not make up some new constraints that not all
controller will be able to comply with.

> > So figuring it out is very much a prerequisite to that series,
> > especially since those patches effectively make the OF-graph mandatory
> > in some situations, while it was purely aesthetics before.
> 
> The OF-graph is mandatory per the DT bindings of this driver. If you
> implement invalid DT which does not contain port@0, it will fail DT
> validation.
>
> If this requirement should be relaxed, sure, it can and I don't think it
> would be hard to do, but I don't see why that should be part of this series,
> which follows the upstream DT binding document for this driver.
> 
> If I cannot trust the driver DT bindings to indicate what is and is not
> mandatory, what other document can I trust then ...

Oh, come on. Doing that, you also require OF-Graph support for the DSI
controller you attach to, and you can't require that. This is very
different from just requiring a property that doesn't have any impact on
any other device, and you know that very well.

Anyway, you wanted my review. You have it. Do what you want with it.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Marek Vasut]

On 3/10/22 11:53, Maxime Ripard wrote:
> On Tue, Mar 08, 2022 at 10:41:05PM +0100, Marek Vasut wrote:
>> On 3/8/22 17:21, Maxime Ripard wrote:
>>> On Tue, Mar 08, 2022 at 03:47:22PM +0100, Marek Vasut wrote:
>>>> On 3/8/22 14:49, Maxime Ripard wrote:
>>>>> On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote:
>>>>>> On 3/8/22 13:51, Maxime Ripard wrote:
>>>>>>> On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
>>>>>>>> On 3/8/22 11:07, Jagan Teki wrote:
>>>>>>>>> On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>
>>>>>>>>>> On 3/8/22 09:03, Jagan Teki wrote:
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> [...]
>>>>>>>>>>
>>>>>>>>>>>> @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
>>>>>>>>>>>>        static int chipone_parse_dt(struct chipone *icn)
>>>>>>>>>>>>        {
>>>>>>>>>>>>               struct device *dev = icn->dev;
>>>>>>>>>>>> +       struct device_node *endpoint;
>>>>>>>>>>>>               struct drm_panel *panel;
>>>>>>>>>>>> +       int dsi_lanes;
>>>>>>>>>>>>               int ret;
>>>>>>>>>>>>
>>>>>>>>>>>>               icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
>>>>>>>>>>>> @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
>>>>>>>>>>>>                       return PTR_ERR(icn->enable_gpio);
>>>>>>>>>>>>               }
>>>>>>>>>>>>
>>>>>>>>>>>> +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
>>>>>>>>>>>> +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
>>>>>>>>>>>> +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
>>>>>>>>>>>> +       of_node_put(endpoint);
>>>>>>>>>>>> +
>>>>>>>>>>>> +       if (!icn->host_node)
>>>>>>>>>>>> +               return -ENODEV;
>>>>>>>>>>>
>>>>>>>>>>> The non-ports-based OF graph returns a -19 example on the Allwinner
>>>>>>>>>>> Display pipeline in R16 [1].
>>>>>>>>>>>
>>>>>>>>>>> We need to have a helper to return host_node for non-ports as I have
>>>>>>>>>>> done it for drm_of_find_bridge.
>>>>>>>>>>>
>>>>>>>>>>> [1] https://patchwork.amarulasolutions.com/patch/1805/
>>>>>>>>>>
>>>>>>>>>> The link points to a patch marked "DO NOT MERGE", maybe that patch is
>>>>>>>>>> missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
>>>>>>>>>> required, see:
>>>>>>>>>>
>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
>>>>>>>>>>
>>>>>>>>>> What is "non-ports-based OF graph" ?
>>>>>>>>>>
>>>>>>>>>> I don't see drm_of_find_bridge() in linux-next , what is that ?
>>>>>>>>>
>>>>>>>>> port@0 is optional as some of the DSI host OF-graph represent the
>>>>>>>>> bridge or panel as child nodes instead of ports. (i think dt-binding
>>>>>>>>> has to fix it to make port@0 optional)
>>>>>>>>
>>>>>>>> The current upstream DT binding document says:
>>>>>>>>
>>>>>>>>         required:
>>>>>>>>           - port@0
>>>>>>>>           - port@1
>>>>>>>>
>>>>>>>> So port@0 is mandatory.
>>>>>>>
>>>>>>> In the binding, sure, but fundamentally the DT excerpt Jagan provided is
>>>>>>> correct. If the bridge supports DCS, there's no reason to use the OF
>>>>>>> graph in the first place: the bridge node will be a child node of the
>>>>>>> MIPI-DSI controller (and there's no obligation to use the OF-graph for a
>>>>>>> MIPI-DSI controller).
>>>>>>>
>>>>>>> I believe port@0 should be made optional (or downright removed if
>>>>>>> MIPI-DCS in the only control bus).
>>>>>>
>>>>>> That's out of scope of this series anyway, so Jagan can implement patches
>>>>>> for that mode if needed.
>>>>>
>>>>> Not really? You can't count on the port@0 being there generally
>>>>> speaking, so you can't count on data-lanes being there either, which
>>>>> exactly what you're doing in this patch.
>>>>
>>>> I can because the upstream DT bindings currently say port@0 must be present,
>>>> see above. If that requirement should be relaxed, sure, but that's a
>>>> separate series.
>>>
>>> And another upstream DT bindings say that you don't need them at all.
>>
>> Which "another upstream DT bindings" do you refer to ?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
> 
>>> Yes, there's a conflict. Yes, it's unfortunate. But the generic DSI
>>> binding is far more relevant than a single bridge driver.
>>
>> That seems to be the wrong way around, how can generic subsystem-wide
>> binding take precedence over specific driver binding ?
> 
> This is the binding of the bus. You're part of that bus. You're a child
> node of that bus, but nothing ever mandates that your parent node uses
> the same convention. And some don't. And since your bridge can be
> connected to pretty much any DSI controller, you have to use the lowest
> common denominator, not make up some new constraints that not all
> controller will be able to comply with.

It seems to me the ICN6211 DT bindings currently further constraint the 
generic DSI bus bindings, and that seems OK to me.

Let me reiterate this again -- if someone wants to relax the 
requirements currently imposed by the ICN6211 DT bindings, fine, but 
that can be done in a separate patchset AND that needs DT bindings 
update. Furthermore, there are no users of this ICN6211 bridge in 
upstream DTs, so there is currently no bridge which would operate 
without OF graph using this driver.

>>> So figuring it out is very much a prerequisite to that series,
>>> especially since those patches effectively make the OF-graph mandatory
>>> in some situations, while it was purely aesthetics before.
>>
>> The OF-graph is mandatory per the DT bindings of this driver. If you
>> implement invalid DT which does not contain port@0, it will fail DT
>> validation.
>>
>> If this requirement should be relaxed, sure, it can and I don't think it
>> would be hard to do, but I don't see why that should be part of this series,
>> which follows the upstream DT binding document for this driver.
>>
>> If I cannot trust the driver DT bindings to indicate what is and is not
>> mandatory, what other document can I trust then ...
> 
> Oh, come on. Doing that, you also require OF-Graph support for the DSI
> controller you attach to, and you can't require that. This is very
> different from just requiring a property that doesn't have any impact on
> any other device, and you know that very well.

Currently the ICN6211 DT bindings DO require that kind of bridge.

The new ad-hoc requirement to support operation with invalid DT bindings 
is something I don't agree with, sorry. But there is a way to cater for 
this new requirement, see above. It just won't be in this series, 
because I have no such bridge and I cannot test it in any way, so 
someone else has to do that.

> Anyway, you wanted my review. You have it. Do what you want with it.

I see.

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 8196 bytes --]

On Thu, Mar 10, 2022 at 01:47:13PM +0100, Marek Vasut wrote:
> On 3/10/22 11:53, Maxime Ripard wrote:
> > On Tue, Mar 08, 2022 at 10:41:05PM +0100, Marek Vasut wrote:
> > > On 3/8/22 17:21, Maxime Ripard wrote:
> > > > On Tue, Mar 08, 2022 at 03:47:22PM +0100, Marek Vasut wrote:
> > > > > On 3/8/22 14:49, Maxime Ripard wrote:
> > > > > > On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote:
> > > > > > > On 3/8/22 13:51, Maxime Ripard wrote:
> > > > > > > > On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
> > > > > > > > > On 3/8/22 11:07, Jagan Teki wrote:
> > > > > > > > > > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
> > > > > > > > > > > 
> > > > > > > > > > > On 3/8/22 09:03, Jagan Teki wrote:
> > > > > > > > > > > 
> > > > > > > > > > > Hi,
> > > > > > > > > > > 
> > > > > > > > > > > [...]
> > > > > > > > > > > 
> > > > > > > > > > > > > @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
> > > > > > > > > > > > >        static int chipone_parse_dt(struct chipone *icn)
> > > > > > > > > > > > >        {
> > > > > > > > > > > > >               struct device *dev = icn->dev;
> > > > > > > > > > > > > +       struct device_node *endpoint;
> > > > > > > > > > > > >               struct drm_panel *panel;
> > > > > > > > > > > > > +       int dsi_lanes;
> > > > > > > > > > > > >               int ret;
> > > > > > > > > > > > > 
> > > > > > > > > > > > >               icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
> > > > > > > > > > > > > @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
> > > > > > > > > > > > >                       return PTR_ERR(icn->enable_gpio);
> > > > > > > > > > > > >               }
> > > > > > > > > > > > > 
> > > > > > > > > > > > > +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> > > > > > > > > > > > > +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> > > > > > > > > > > > > +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
> > > > > > > > > > > > > +       of_node_put(endpoint);
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +       if (!icn->host_node)
> > > > > > > > > > > > > +               return -ENODEV;
> > > > > > > > > > > > 
> > > > > > > > > > > > The non-ports-based OF graph returns a -19 example on the Allwinner
> > > > > > > > > > > > Display pipeline in R16 [1].
> > > > > > > > > > > > 
> > > > > > > > > > > > We need to have a helper to return host_node for non-ports as I have
> > > > > > > > > > > > done it for drm_of_find_bridge.
> > > > > > > > > > > > 
> > > > > > > > > > > > [1] https://patchwork.amarulasolutions.com/patch/1805/
> > > > > > > > > > > 
> > > > > > > > > > > The link points to a patch marked "DO NOT MERGE", maybe that patch is
> > > > > > > > > > > missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
> > > > > > > > > > > required, see:
> > > > > > > > > > > 
> > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
> > > > > > > > > > > 
> > > > > > > > > > > What is "non-ports-based OF graph" ?
> > > > > > > > > > > 
> > > > > > > > > > > I don't see drm_of_find_bridge() in linux-next , what is that ?
> > > > > > > > > > 
> > > > > > > > > > port@0 is optional as some of the DSI host OF-graph represent the
> > > > > > > > > > bridge or panel as child nodes instead of ports. (i think dt-binding
> > > > > > > > > > has to fix it to make port@0 optional)
> > > > > > > > > 
> > > > > > > > > The current upstream DT binding document says:
> > > > > > > > > 
> > > > > > > > >         required:
> > > > > > > > >           - port@0
> > > > > > > > >           - port@1
> > > > > > > > > 
> > > > > > > > > So port@0 is mandatory.
> > > > > > > > 
> > > > > > > > In the binding, sure, but fundamentally the DT excerpt Jagan provided is
> > > > > > > > correct. If the bridge supports DCS, there's no reason to use the OF
> > > > > > > > graph in the first place: the bridge node will be a child node of the
> > > > > > > > MIPI-DSI controller (and there's no obligation to use the OF-graph for a
> > > > > > > > MIPI-DSI controller).
> > > > > > > > 
> > > > > > > > I believe port@0 should be made optional (or downright removed if
> > > > > > > > MIPI-DCS in the only control bus).
> > > > > > > 
> > > > > > > That's out of scope of this series anyway, so Jagan can implement patches
> > > > > > > for that mode if needed.
> > > > > > 
> > > > > > Not really? You can't count on the port@0 being there generally
> > > > > > speaking, so you can't count on data-lanes being there either, which
> > > > > > exactly what you're doing in this patch.
> > > > > 
> > > > > I can because the upstream DT bindings currently say port@0 must be present,
> > > > > see above. If that requirement should be relaxed, sure, but that's a
> > > > > separate series.
> > > > 
> > > > And another upstream DT bindings say that you don't need them at all.
> > > 
> > > Which "another upstream DT bindings" do you refer to ?
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
> > 
> > > > Yes, there's a conflict. Yes, it's unfortunate. But the generic DSI
> > > > binding is far more relevant than a single bridge driver.
> > > 
> > > That seems to be the wrong way around, how can generic subsystem-wide
> > > binding take precedence over specific driver binding ?
> > 
> > This is the binding of the bus. You're part of that bus. You're a child
> > node of that bus, but nothing ever mandates that your parent node uses
> > the same convention. And some don't. And since your bridge can be
> > connected to pretty much any DSI controller, you have to use the lowest
> > common denominator, not make up some new constraints that not all
> > controller will be able to comply with.
> 
> It seems to me the ICN6211 DT bindings currently further constraint the
> generic DSI bus bindings, and that seems OK to me.
> 
> Let me reiterate this again -- if someone wants to relax the requirements
> currently imposed by the ICN6211 DT bindings, fine, but that can be done in
> a separate patchset AND that needs DT bindings update. Furthermore, there
> are no users of this ICN6211 bridge in upstream DTs, so there is currently
> no bridge which would operate without OF graph using this driver.

And let me reiterate this again: something that used to work for a user
doesn't anymore when your series is applied. This is a textbook
regression. I suggested a way forward, that you don't like for some
reason, fine. But pushing through a regression is just not acceptable.

> > > > So figuring it out is very much a prerequisite to that series,
> > > > especially since those patches effectively make the OF-graph mandatory
> > > > in some situations, while it was purely aesthetics before.
> > > 
> > > The OF-graph is mandatory per the DT bindings of this driver. If you
> > > implement invalid DT which does not contain port@0, it will fail DT
> > > validation.
> > > 
> > > If this requirement should be relaxed, sure, it can and I don't think it
> > > would be hard to do, but I don't see why that should be part of this series,
> > > which follows the upstream DT binding document for this driver.
> > > 
> > > If I cannot trust the driver DT bindings to indicate what is and is not
> > > mandatory, what other document can I trust then ...
> > 
> > Oh, come on. Doing that, you also require OF-Graph support for the DSI
> > controller you attach to, and you can't require that. This is very
> > different from just requiring a property that doesn't have any impact on
> > any other device, and you know that very well.
> 
> Currently the ICN6211 DT bindings DO require that kind of bridge.

And while this wasn't enforced before, you make it a hard requirement
with this series. This is what changed, and what caused this whole
discussion.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Marek Vasut]

On 3/10/22 15:18, Maxime Ripard wrote:
> On Thu, Mar 10, 2022 at 01:47:13PM +0100, Marek Vasut wrote:
>> On 3/10/22 11:53, Maxime Ripard wrote:
>>> On Tue, Mar 08, 2022 at 10:41:05PM +0100, Marek Vasut wrote:
>>>> On 3/8/22 17:21, Maxime Ripard wrote:
>>>>> On Tue, Mar 08, 2022 at 03:47:22PM +0100, Marek Vasut wrote:
>>>>>> On 3/8/22 14:49, Maxime Ripard wrote:
>>>>>>> On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote:
>>>>>>>> On 3/8/22 13:51, Maxime Ripard wrote:
>>>>>>>>> On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
>>>>>>>>>> On 3/8/22 11:07, Jagan Teki wrote:
>>>>>>>>>>> On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On 3/8/22 09:03, Jagan Teki wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> [...]
>>>>>>>>>>>>
>>>>>>>>>>>>>> @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
>>>>>>>>>>>>>>         static int chipone_parse_dt(struct chipone *icn)
>>>>>>>>>>>>>>         {
>>>>>>>>>>>>>>                struct device *dev = icn->dev;
>>>>>>>>>>>>>> +       struct device_node *endpoint;
>>>>>>>>>>>>>>                struct drm_panel *panel;
>>>>>>>>>>>>>> +       int dsi_lanes;
>>>>>>>>>>>>>>                int ret;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>                icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
>>>>>>>>>>>>>> @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
>>>>>>>>>>>>>>                        return PTR_ERR(icn->enable_gpio);
>>>>>>>>>>>>>>                }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
>>>>>>>>>>>>>> +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
>>>>>>>>>>>>>> +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
>>>>>>>>>>>>>> +       of_node_put(endpoint);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +       if (!icn->host_node)
>>>>>>>>>>>>>> +               return -ENODEV;
>>>>>>>>>>>>>
>>>>>>>>>>>>> The non-ports-based OF graph returns a -19 example on the Allwinner
>>>>>>>>>>>>> Display pipeline in R16 [1].
>>>>>>>>>>>>>
>>>>>>>>>>>>> We need to have a helper to return host_node for non-ports as I have
>>>>>>>>>>>>> done it for drm_of_find_bridge.
>>>>>>>>>>>>>
>>>>>>>>>>>>> [1] https://patchwork.amarulasolutions.com/patch/1805/
>>>>>>>>>>>>
>>>>>>>>>>>> The link points to a patch marked "DO NOT MERGE", maybe that patch is
>>>>>>>>>>>> missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
>>>>>>>>>>>> required, see:
>>>>>>>>>>>>
>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
>>>>>>>>>>>>
>>>>>>>>>>>> What is "non-ports-based OF graph" ?
>>>>>>>>>>>>
>>>>>>>>>>>> I don't see drm_of_find_bridge() in linux-next , what is that ?
>>>>>>>>>>>
>>>>>>>>>>> port@0 is optional as some of the DSI host OF-graph represent the
>>>>>>>>>>> bridge or panel as child nodes instead of ports. (i think dt-binding
>>>>>>>>>>> has to fix it to make port@0 optional)
>>>>>>>>>>
>>>>>>>>>> The current upstream DT binding document says:
>>>>>>>>>>
>>>>>>>>>>          required:
>>>>>>>>>>            - port@0
>>>>>>>>>>            - port@1
>>>>>>>>>>
>>>>>>>>>> So port@0 is mandatory.
>>>>>>>>>
>>>>>>>>> In the binding, sure, but fundamentally the DT excerpt Jagan provided is
>>>>>>>>> correct. If the bridge supports DCS, there's no reason to use the OF
>>>>>>>>> graph in the first place: the bridge node will be a child node of the
>>>>>>>>> MIPI-DSI controller (and there's no obligation to use the OF-graph for a
>>>>>>>>> MIPI-DSI controller).
>>>>>>>>>
>>>>>>>>> I believe port@0 should be made optional (or downright removed if
>>>>>>>>> MIPI-DCS in the only control bus).
>>>>>>>>
>>>>>>>> That's out of scope of this series anyway, so Jagan can implement patches
>>>>>>>> for that mode if needed.
>>>>>>>
>>>>>>> Not really? You can't count on the port@0 being there generally
>>>>>>> speaking, so you can't count on data-lanes being there either, which
>>>>>>> exactly what you're doing in this patch.
>>>>>>
>>>>>> I can because the upstream DT bindings currently say port@0 must be present,
>>>>>> see above. If that requirement should be relaxed, sure, but that's a
>>>>>> separate series.
>>>>>
>>>>> And another upstream DT bindings say that you don't need them at all.
>>>>
>>>> Which "another upstream DT bindings" do you refer to ?
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
>>>
>>>>> Yes, there's a conflict. Yes, it's unfortunate. But the generic DSI
>>>>> binding is far more relevant than a single bridge driver.
>>>>
>>>> That seems to be the wrong way around, how can generic subsystem-wide
>>>> binding take precedence over specific driver binding ?
>>>
>>> This is the binding of the bus. You're part of that bus. You're a child
>>> node of that bus, but nothing ever mandates that your parent node uses
>>> the same convention. And some don't. And since your bridge can be
>>> connected to pretty much any DSI controller, you have to use the lowest
>>> common denominator, not make up some new constraints that not all
>>> controller will be able to comply with.
>>
>> It seems to me the ICN6211 DT bindings currently further constraint the
>> generic DSI bus bindings, and that seems OK to me.
>>
>> Let me reiterate this again -- if someone wants to relax the requirements
>> currently imposed by the ICN6211 DT bindings, fine, but that can be done in
>> a separate patchset AND that needs DT bindings update. Furthermore, there
>> are no users of this ICN6211 bridge in upstream DTs, so there is currently
>> no bridge which would operate without OF graph using this driver.
> 
> And let me reiterate this again: something that used to work for a user
> doesn't anymore when your series is applied. This is a textbook
> regression. I suggested a way forward, that you don't like for some
> reason, fine. But pushing through a regression is just not acceptable.

How can this be a regression if this mode of operation could not have 
ever been supported with valid upstream DT bindings in the first place ?

Should we now require that kernel drivers somehow magically support all 
kinds of random broken DT bindings in addition to ones which pass YAML 
DT validation ?

>>>>> So figuring it out is very much a prerequisite to that series,
>>>>> especially since those patches effectively make the OF-graph mandatory
>>>>> in some situations, while it was purely aesthetics before.
>>>>
>>>> The OF-graph is mandatory per the DT bindings of this driver. If you
>>>> implement invalid DT which does not contain port@0, it will fail DT
>>>> validation.
>>>>
>>>> If this requirement should be relaxed, sure, it can and I don't think it
>>>> would be hard to do, but I don't see why that should be part of this series,
>>>> which follows the upstream DT binding document for this driver.
>>>>
>>>> If I cannot trust the driver DT bindings to indicate what is and is not
>>>> mandatory, what other document can I trust then ...
>>>
>>> Oh, come on. Doing that, you also require OF-Graph support for the DSI
>>> controller you attach to, and you can't require that. This is very
>>> different from just requiring a property that doesn't have any impact on
>>> any other device, and you know that very well.
>>
>> Currently the ICN6211 DT bindings DO require that kind of bridge.
> 
> And while this wasn't enforced before, you make it a hard requirement
> with this series. This is what changed, and what caused this whole
> discussion.

The current DT bindings already make it a hard requirement, so no, 
nothing changed here.

Unless what you are trying to ask for is support for broken DT bindings 
which do not pass YAML DT validation by this driver, but that is very 
dangerous, because then the question is, how far should we support such 
broken bindings. What kind of broken is still OK and what kind of broken 
is no longer OK ?

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 10808 bytes --]

On Fri, Mar 11, 2022 at 11:36:58AM +0100, Marek Vasut wrote:
> On 3/10/22 15:18, Maxime Ripard wrote:
> > On Thu, Mar 10, 2022 at 01:47:13PM +0100, Marek Vasut wrote:
> > > On 3/10/22 11:53, Maxime Ripard wrote:
> > > > On Tue, Mar 08, 2022 at 10:41:05PM +0100, Marek Vasut wrote:
> > > > > On 3/8/22 17:21, Maxime Ripard wrote:
> > > > > > On Tue, Mar 08, 2022 at 03:47:22PM +0100, Marek Vasut wrote:
> > > > > > > On 3/8/22 14:49, Maxime Ripard wrote:
> > > > > > > > On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote:
> > > > > > > > > On 3/8/22 13:51, Maxime Ripard wrote:
> > > > > > > > > > On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
> > > > > > > > > > > On 3/8/22 11:07, Jagan Teki wrote:
> > > > > > > > > > > > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
> > > > > > > > > > > > > 
> > > > > > > > > > > > > On 3/8/22 09:03, Jagan Teki wrote:
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > 
> > > > > > > > > > > > > [...]
> > > > > > > > > > > > > 
> > > > > > > > > > > > > > > @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
> > > > > > > > > > > > > > >         static int chipone_parse_dt(struct chipone *icn)
> > > > > > > > > > > > > > >         {
> > > > > > > > > > > > > > >                struct device *dev = icn->dev;
> > > > > > > > > > > > > > > +       struct device_node *endpoint;
> > > > > > > > > > > > > > >                struct drm_panel *panel;
> > > > > > > > > > > > > > > +       int dsi_lanes;
> > > > > > > > > > > > > > >                int ret;
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > >                icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
> > > > > > > > > > > > > > > @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
> > > > > > > > > > > > > > >                        return PTR_ERR(icn->enable_gpio);
> > > > > > > > > > > > > > >                }
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> > > > > > > > > > > > > > > +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> > > > > > > > > > > > > > > +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
> > > > > > > > > > > > > > > +       of_node_put(endpoint);
> > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > +       if (!icn->host_node)
> > > > > > > > > > > > > > > +               return -ENODEV;
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > The non-ports-based OF graph returns a -19 example on the Allwinner
> > > > > > > > > > > > > > Display pipeline in R16 [1].
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > We need to have a helper to return host_node for non-ports as I have
> > > > > > > > > > > > > > done it for drm_of_find_bridge.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > [1] https://patchwork.amarulasolutions.com/patch/1805/
> > > > > > > > > > > > > 
> > > > > > > > > > > > > The link points to a patch marked "DO NOT MERGE", maybe that patch is
> > > > > > > > > > > > > missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
> > > > > > > > > > > > > required, see:
> > > > > > > > > > > > > 
> > > > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
> > > > > > > > > > > > > 
> > > > > > > > > > > > > What is "non-ports-based OF graph" ?
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I don't see drm_of_find_bridge() in linux-next , what is that ?
> > > > > > > > > > > > 
> > > > > > > > > > > > port@0 is optional as some of the DSI host OF-graph represent the
> > > > > > > > > > > > bridge or panel as child nodes instead of ports. (i think dt-binding
> > > > > > > > > > > > has to fix it to make port@0 optional)
> > > > > > > > > > > 
> > > > > > > > > > > The current upstream DT binding document says:
> > > > > > > > > > > 
> > > > > > > > > > >          required:
> > > > > > > > > > >            - port@0
> > > > > > > > > > >            - port@1
> > > > > > > > > > > 
> > > > > > > > > > > So port@0 is mandatory.
> > > > > > > > > > 
> > > > > > > > > > In the binding, sure, but fundamentally the DT excerpt Jagan provided is
> > > > > > > > > > correct. If the bridge supports DCS, there's no reason to use the OF
> > > > > > > > > > graph in the first place: the bridge node will be a child node of the
> > > > > > > > > > MIPI-DSI controller (and there's no obligation to use the OF-graph for a
> > > > > > > > > > MIPI-DSI controller).
> > > > > > > > > > 
> > > > > > > > > > I believe port@0 should be made optional (or downright removed if
> > > > > > > > > > MIPI-DCS in the only control bus).
> > > > > > > > > 
> > > > > > > > > That's out of scope of this series anyway, so Jagan can implement patches
> > > > > > > > > for that mode if needed.
> > > > > > > > 
> > > > > > > > Not really? You can't count on the port@0 being there generally
> > > > > > > > speaking, so you can't count on data-lanes being there either, which
> > > > > > > > exactly what you're doing in this patch.
> > > > > > > 
> > > > > > > I can because the upstream DT bindings currently say port@0 must be present,
> > > > > > > see above. If that requirement should be relaxed, sure, but that's a
> > > > > > > separate series.
> > > > > > 
> > > > > > And another upstream DT bindings say that you don't need them at all.
> > > > > 
> > > > > Which "another upstream DT bindings" do you refer to ?
> > > > 
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
> > > > 
> > > > > > Yes, there's a conflict. Yes, it's unfortunate. But the generic DSI
> > > > > > binding is far more relevant than a single bridge driver.
> > > > > 
> > > > > That seems to be the wrong way around, how can generic subsystem-wide
> > > > > binding take precedence over specific driver binding ?
> > > > 
> > > > This is the binding of the bus. You're part of that bus. You're a child
> > > > node of that bus, but nothing ever mandates that your parent node uses
> > > > the same convention. And some don't. And since your bridge can be
> > > > connected to pretty much any DSI controller, you have to use the lowest
> > > > common denominator, not make up some new constraints that not all
> > > > controller will be able to comply with.
> > > 
> > > It seems to me the ICN6211 DT bindings currently further constraint the
> > > generic DSI bus bindings, and that seems OK to me.
> > > 
> > > Let me reiterate this again -- if someone wants to relax the requirements
> > > currently imposed by the ICN6211 DT bindings, fine, but that can be done in
> > > a separate patchset AND that needs DT bindings update. Furthermore, there
> > > are no users of this ICN6211 bridge in upstream DTs, so there is currently
> > > no bridge which would operate without OF graph using this driver.
> > 
> > And let me reiterate this again: something that used to work for a user
> > doesn't anymore when your series is applied. This is a textbook
> > regression. I suggested a way forward, that you don't like for some
> > reason, fine. But pushing through a regression is just not acceptable.
> 
> How can this be a regression if this mode of operation could not have ever
> been supported with valid upstream DT bindings in the first place ?
> 
> Should we now require that kernel drivers somehow magically support all
> kinds of random broken DT bindings in addition to ones which pass YAML DT
> validation ?

The thing is, as I told you multiple times already, it was broken from
the bridge standpoint, but not from the controller's. If it had been
correct for the bridge, it wouldn't have been for the controller. So,
same story.

The only difference is that it wouldn't affect you, but I don't see how
it's relevant.

> > > > > > So figuring it out is very much a prerequisite to that series,
> > > > > > especially since those patches effectively make the OF-graph mandatory
> > > > > > in some situations, while it was purely aesthetics before.
> > > > > 
> > > > > The OF-graph is mandatory per the DT bindings of this driver. If you
> > > > > implement invalid DT which does not contain port@0, it will fail DT
> > > > > validation.
> > > > > 
> > > > > If this requirement should be relaxed, sure, it can and I don't think it
> > > > > would be hard to do, but I don't see why that should be part of this series,
> > > > > which follows the upstream DT binding document for this driver.
> > > > > 
> > > > > If I cannot trust the driver DT bindings to indicate what is and is not
> > > > > mandatory, what other document can I trust then ...
> > > > 
> > > > Oh, come on. Doing that, you also require OF-Graph support for the DSI
> > > > controller you attach to, and you can't require that. This is very
> > > > different from just requiring a property that doesn't have any impact on
> > > > any other device, and you know that very well.
> > > 
> > > Currently the ICN6211 DT bindings DO require that kind of bridge.
> > 
> > And while this wasn't enforced before, you make it a hard requirement
> > with this series. This is what changed, and what caused this whole
> > discussion.
> 
> The current DT bindings already make it a hard requirement, so no, nothing
> changed here.
> 
> Unless what you are trying to ask for is support for broken DT bindings
> which do not pass YAML DT validation by this driver, but that is very
> dangerous, because then the question is, how far should we support such
> broken bindings. What kind of broken is still OK and what kind of broken is
> no longer OK ?

If it ever worked in a mainline release, it must always work. See:
https://www.kernel.org/doc/html/latest/devicetree/bindings/ABI.html

As far as I'm concerned, it's the sole criteria. So to answer your
question, if it was broken but worked at some point, yes, we need to
keep supporting it. If it never worked, no, we don't.

Honestly, I don't get the push-back. To fix this properly would require:
  - to remove port@0 being mandatory
  - to move the data-lanes property to the bridge node itself.

That's it. It takes 5 minutes, 30 minutes with the test and the commit
log. We've spent more time arguing about it already.

So if you want to continue debating on whether it's a regression or not,
or whether DT ABI stability is a good thing or not, go ahead. But as far
as I'm concerned, this isn't really up for debate.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Marek Vasut]

On 3/11/22 17:29, Maxime Ripard wrote:
> On Fri, Mar 11, 2022 at 11:36:58AM +0100, Marek Vasut wrote:
>> On 3/10/22 15:18, Maxime Ripard wrote:
>>> On Thu, Mar 10, 2022 at 01:47:13PM +0100, Marek Vasut wrote:
>>>> On 3/10/22 11:53, Maxime Ripard wrote:
>>>>> On Tue, Mar 08, 2022 at 10:41:05PM +0100, Marek Vasut wrote:
>>>>>> On 3/8/22 17:21, Maxime Ripard wrote:
>>>>>>> On Tue, Mar 08, 2022 at 03:47:22PM +0100, Marek Vasut wrote:
>>>>>>>> On 3/8/22 14:49, Maxime Ripard wrote:
>>>>>>>>> On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote:
>>>>>>>>>> On 3/8/22 13:51, Maxime Ripard wrote:
>>>>>>>>>>> On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
>>>>>>>>>>>> On 3/8/22 11:07, Jagan Teki wrote:
>>>>>>>>>>>>> On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 3/8/22 09:03, Jagan Teki wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> [...]
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
>>>>>>>>>>>>>>>>          static int chipone_parse_dt(struct chipone *icn)
>>>>>>>>>>>>>>>>          {
>>>>>>>>>>>>>>>>                 struct device *dev = icn->dev;
>>>>>>>>>>>>>>>> +       struct device_node *endpoint;
>>>>>>>>>>>>>>>>                 struct drm_panel *panel;
>>>>>>>>>>>>>>>> +       int dsi_lanes;
>>>>>>>>>>>>>>>>                 int ret;
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>                 icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
>>>>>>>>>>>>>>>> @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
>>>>>>>>>>>>>>>>                         return PTR_ERR(icn->enable_gpio);
>>>>>>>>>>>>>>>>                 }
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
>>>>>>>>>>>>>>>> +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
>>>>>>>>>>>>>>>> +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
>>>>>>>>>>>>>>>> +       of_node_put(endpoint);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> +       if (!icn->host_node)
>>>>>>>>>>>>>>>> +               return -ENODEV;
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The non-ports-based OF graph returns a -19 example on the Allwinner
>>>>>>>>>>>>>>> Display pipeline in R16 [1].
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> We need to have a helper to return host_node for non-ports as I have
>>>>>>>>>>>>>>> done it for drm_of_find_bridge.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> [1] https://patchwork.amarulasolutions.com/patch/1805/
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The link points to a patch marked "DO NOT MERGE", maybe that patch is
>>>>>>>>>>>>>> missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
>>>>>>>>>>>>>> required, see:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> What is "non-ports-based OF graph" ?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I don't see drm_of_find_bridge() in linux-next , what is that ?
>>>>>>>>>>>>>
>>>>>>>>>>>>> port@0 is optional as some of the DSI host OF-graph represent the
>>>>>>>>>>>>> bridge or panel as child nodes instead of ports. (i think dt-binding
>>>>>>>>>>>>> has to fix it to make port@0 optional)
>>>>>>>>>>>>
>>>>>>>>>>>> The current upstream DT binding document says:
>>>>>>>>>>>>
>>>>>>>>>>>>           required:
>>>>>>>>>>>>             - port@0
>>>>>>>>>>>>             - port@1
>>>>>>>>>>>>
>>>>>>>>>>>> So port@0 is mandatory.
>>>>>>>>>>>
>>>>>>>>>>> In the binding, sure, but fundamentally the DT excerpt Jagan provided is
>>>>>>>>>>> correct. If the bridge supports DCS, there's no reason to use the OF
>>>>>>>>>>> graph in the first place: the bridge node will be a child node of the
>>>>>>>>>>> MIPI-DSI controller (and there's no obligation to use the OF-graph for a
>>>>>>>>>>> MIPI-DSI controller).
>>>>>>>>>>>
>>>>>>>>>>> I believe port@0 should be made optional (or downright removed if
>>>>>>>>>>> MIPI-DCS in the only control bus).
>>>>>>>>>>
>>>>>>>>>> That's out of scope of this series anyway, so Jagan can implement patches
>>>>>>>>>> for that mode if needed.
>>>>>>>>>
>>>>>>>>> Not really? You can't count on the port@0 being there generally
>>>>>>>>> speaking, so you can't count on data-lanes being there either, which
>>>>>>>>> exactly what you're doing in this patch.
>>>>>>>>
>>>>>>>> I can because the upstream DT bindings currently say port@0 must be present,
>>>>>>>> see above. If that requirement should be relaxed, sure, but that's a
>>>>>>>> separate series.
>>>>>>>
>>>>>>> And another upstream DT bindings say that you don't need them at all.
>>>>>>
>>>>>> Which "another upstream DT bindings" do you refer to ?
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
>>>>>
>>>>>>> Yes, there's a conflict. Yes, it's unfortunate. But the generic DSI
>>>>>>> binding is far more relevant than a single bridge driver.
>>>>>>
>>>>>> That seems to be the wrong way around, how can generic subsystem-wide
>>>>>> binding take precedence over specific driver binding ?
>>>>>
>>>>> This is the binding of the bus. You're part of that bus. You're a child
>>>>> node of that bus, but nothing ever mandates that your parent node uses
>>>>> the same convention. And some don't. And since your bridge can be
>>>>> connected to pretty much any DSI controller, you have to use the lowest
>>>>> common denominator, not make up some new constraints that not all
>>>>> controller will be able to comply with.
>>>>
>>>> It seems to me the ICN6211 DT bindings currently further constraint the
>>>> generic DSI bus bindings, and that seems OK to me.
>>>>
>>>> Let me reiterate this again -- if someone wants to relax the requirements
>>>> currently imposed by the ICN6211 DT bindings, fine, but that can be done in
>>>> a separate patchset AND that needs DT bindings update. Furthermore, there
>>>> are no users of this ICN6211 bridge in upstream DTs, so there is currently
>>>> no bridge which would operate without OF graph using this driver.
>>>
>>> And let me reiterate this again: something that used to work for a user
>>> doesn't anymore when your series is applied. This is a textbook
>>> regression. I suggested a way forward, that you don't like for some
>>> reason, fine. But pushing through a regression is just not acceptable.
>>
>> How can this be a regression if this mode of operation could not have ever
>> been supported with valid upstream DT bindings in the first place ?
>>
>> Should we now require that kernel drivers somehow magically support all
>> kinds of random broken DT bindings in addition to ones which pass YAML DT
>> validation ?
> 
> The thing is, as I told you multiple times already, it was broken from
> the bridge standpoint, but not from the controller's. If it had been
> correct for the bridge, it wouldn't have been for the controller. So,
> same story.
> 
> The only difference is that it wouldn't affect you, but I don't see how
> it's relevant.

I'm sorry, I do not understand this answer.

>>>>>>> So figuring it out is very much a prerequisite to that series,
>>>>>>> especially since those patches effectively make the OF-graph mandatory
>>>>>>> in some situations, while it was purely aesthetics before.
>>>>>>
>>>>>> The OF-graph is mandatory per the DT bindings of this driver. If you
>>>>>> implement invalid DT which does not contain port@0, it will fail DT
>>>>>> validation.
>>>>>>
>>>>>> If this requirement should be relaxed, sure, it can and I don't think it
>>>>>> would be hard to do, but I don't see why that should be part of this series,
>>>>>> which follows the upstream DT binding document for this driver.
>>>>>>
>>>>>> If I cannot trust the driver DT bindings to indicate what is and is not
>>>>>> mandatory, what other document can I trust then ...
>>>>>
>>>>> Oh, come on. Doing that, you also require OF-Graph support for the DSI
>>>>> controller you attach to, and you can't require that. This is very
>>>>> different from just requiring a property that doesn't have any impact on
>>>>> any other device, and you know that very well.
>>>>
>>>> Currently the ICN6211 DT bindings DO require that kind of bridge.
>>>
>>> And while this wasn't enforced before, you make it a hard requirement
>>> with this series. This is what changed, and what caused this whole
>>> discussion.
>>
>> The current DT bindings already make it a hard requirement, so no, nothing
>> changed here.
>>
>> Unless what you are trying to ask for is support for broken DT bindings
>> which do not pass YAML DT validation by this driver, but that is very
>> dangerous, because then the question is, how far should we support such
>> broken bindings. What kind of broken is still OK and what kind of broken is
>> no longer OK ?
> 
> If it ever worked in a mainline release, it must always work. See:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/ABI.html

> As far as I'm concerned, it's the sole criteria. So to answer your
> question, if it was broken but worked at some point, yes, we need to
> keep supporting it. If it never worked, no, we don't.

There are no users of this driver in any mainline release.

DT is ABI, and ICN6211 DT bindings says port@0 is mandatory. If this 
driver worked with some broken downstream DT without port@0, then that 
downstream depended on undefined behavior which I cannot fathom how it 
can be considered part of kernel ABI. That downstream should fix its DT 
instead.

> Honestly, I don't get the push-back.

Because what I am being asked to do here is implement some sort of 
undefined behavior backward compatibility. For behavior which has no 
users. Worse, for behavior which can only be triggered if your DT is 
broken and does not even pass the YAML DT validation.

That kind of new requirement seems bonkers to me.

> To fix this properly would require:
>    - to remove port@0 being mandatory
>    - to move the data-lanes property to the bridge node itself.

That does not work, data-lanes is port property.

> That's it. It takes 5 minutes, 30 minutes with the test and the commit
> log. We've spent more time arguing about it already.

How can I test this bug compatibility requirement ?

I don't have a broken DT, my DT does pass the YAML DT validation, so 
should I explicitly break my DT to perform the test ?

Are we going to start asking people to implement support for randomly 
broken DTs in their drivers during code review too ?

> So if you want to continue debating on whether it's a regression or not,
> or whether DT ABI stability is a good thing or not, go ahead. But as far
> as I'm concerned, this isn't really up for debate.

As far as I can tell, undefined behavior can hardly be part of stable ABI.

[...]

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Jagan Teki]

Hi Marek,

On Sat, Mar 12, 2022 at 1:32 AM Marek Vasut <marex@denx.de> wrote:
>
> On 3/11/22 17:29, Maxime Ripard wrote:
> > On Fri, Mar 11, 2022 at 11:36:58AM +0100, Marek Vasut wrote:
> >> On 3/10/22 15:18, Maxime Ripard wrote:
> >>> On Thu, Mar 10, 2022 at 01:47:13PM +0100, Marek Vasut wrote:
> >>>> On 3/10/22 11:53, Maxime Ripard wrote:
> >>>>> On Tue, Mar 08, 2022 at 10:41:05PM +0100, Marek Vasut wrote:
> >>>>>> On 3/8/22 17:21, Maxime Ripard wrote:
> >>>>>>> On Tue, Mar 08, 2022 at 03:47:22PM +0100, Marek Vasut wrote:
> >>>>>>>> On 3/8/22 14:49, Maxime Ripard wrote:
> >>>>>>>>> On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote:
> >>>>>>>>>> On 3/8/22 13:51, Maxime Ripard wrote:
> >>>>>>>>>>> On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
> >>>>>>>>>>>> On 3/8/22 11:07, Jagan Teki wrote:
> >>>>>>>>>>>>> On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> On 3/8/22 09:03, Jagan Teki wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Hi,
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> [...]
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
> >>>>>>>>>>>>>>>>          static int chipone_parse_dt(struct chipone *icn)
> >>>>>>>>>>>>>>>>          {
> >>>>>>>>>>>>>>>>                 struct device *dev = icn->dev;
> >>>>>>>>>>>>>>>> +       struct device_node *endpoint;
> >>>>>>>>>>>>>>>>                 struct drm_panel *panel;
> >>>>>>>>>>>>>>>> +       int dsi_lanes;
> >>>>>>>>>>>>>>>>                 int ret;
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>                 icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
> >>>>>>>>>>>>>>>> @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
> >>>>>>>>>>>>>>>>                         return PTR_ERR(icn->enable_gpio);
> >>>>>>>>>>>>>>>>                 }
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> >>>>>>>>>>>>>>>> +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> >>>>>>>>>>>>>>>> +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
> >>>>>>>>>>>>>>>> +       of_node_put(endpoint);
> >>>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>>> +       if (!icn->host_node)
> >>>>>>>>>>>>>>>> +               return -ENODEV;
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> The non-ports-based OF graph returns a -19 example on the Allwinner
> >>>>>>>>>>>>>>> Display pipeline in R16 [1].
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> We need to have a helper to return host_node for non-ports as I have
> >>>>>>>>>>>>>>> done it for drm_of_find_bridge.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> [1] https://patchwork.amarulasolutions.com/patch/1805/
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> The link points to a patch marked "DO NOT MERGE", maybe that patch is
> >>>>>>>>>>>>>> missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
> >>>>>>>>>>>>>> required, see:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> What is "non-ports-based OF graph" ?
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> I don't see drm_of_find_bridge() in linux-next , what is that ?
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> port@0 is optional as some of the DSI host OF-graph represent the
> >>>>>>>>>>>>> bridge or panel as child nodes instead of ports. (i think dt-binding
> >>>>>>>>>>>>> has to fix it to make port@0 optional)
> >>>>>>>>>>>>
> >>>>>>>>>>>> The current upstream DT binding document says:
> >>>>>>>>>>>>
> >>>>>>>>>>>>           required:
> >>>>>>>>>>>>             - port@0
> >>>>>>>>>>>>             - port@1
> >>>>>>>>>>>>
> >>>>>>>>>>>> So port@0 is mandatory.
> >>>>>>>>>>>
> >>>>>>>>>>> In the binding, sure, but fundamentally the DT excerpt Jagan provided is
> >>>>>>>>>>> correct. If the bridge supports DCS, there's no reason to use the OF
> >>>>>>>>>>> graph in the first place: the bridge node will be a child node of the
> >>>>>>>>>>> MIPI-DSI controller (and there's no obligation to use the OF-graph for a
> >>>>>>>>>>> MIPI-DSI controller).
> >>>>>>>>>>>
> >>>>>>>>>>> I believe port@0 should be made optional (or downright removed if
> >>>>>>>>>>> MIPI-DCS in the only control bus).
> >>>>>>>>>>
> >>>>>>>>>> That's out of scope of this series anyway, so Jagan can implement patches
> >>>>>>>>>> for that mode if needed.
> >>>>>>>>>
> >>>>>>>>> Not really? You can't count on the port@0 being there generally
> >>>>>>>>> speaking, so you can't count on data-lanes being there either, which
> >>>>>>>>> exactly what you're doing in this patch.
> >>>>>>>>
> >>>>>>>> I can because the upstream DT bindings currently say port@0 must be present,
> >>>>>>>> see above. If that requirement should be relaxed, sure, but that's a
> >>>>>>>> separate series.
> >>>>>>>
> >>>>>>> And another upstream DT bindings say that you don't need them at all.
> >>>>>>
> >>>>>> Which "another upstream DT bindings" do you refer to ?
> >>>>>
> >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
> >>>>>
> >>>>>>> Yes, there's a conflict. Yes, it's unfortunate. But the generic DSI
> >>>>>>> binding is far more relevant than a single bridge driver.
> >>>>>>
> >>>>>> That seems to be the wrong way around, how can generic subsystem-wide
> >>>>>> binding take precedence over specific driver binding ?
> >>>>>
> >>>>> This is the binding of the bus. You're part of that bus. You're a child
> >>>>> node of that bus, but nothing ever mandates that your parent node uses
> >>>>> the same convention. And some don't. And since your bridge can be
> >>>>> connected to pretty much any DSI controller, you have to use the lowest
> >>>>> common denominator, not make up some new constraints that not all
> >>>>> controller will be able to comply with.
> >>>>
> >>>> It seems to me the ICN6211 DT bindings currently further constraint the
> >>>> generic DSI bus bindings, and that seems OK to me.
> >>>>
> >>>> Let me reiterate this again -- if someone wants to relax the requirements
> >>>> currently imposed by the ICN6211 DT bindings, fine, but that can be done in
> >>>> a separate patchset AND that needs DT bindings update. Furthermore, there
> >>>> are no users of this ICN6211 bridge in upstream DTs, so there is currently
> >>>> no bridge which would operate without OF graph using this driver.
> >>>
> >>> And let me reiterate this again: something that used to work for a user
> >>> doesn't anymore when your series is applied. This is a textbook
> >>> regression. I suggested a way forward, that you don't like for some
> >>> reason, fine. But pushing through a regression is just not acceptable.
> >>
> >> How can this be a regression if this mode of operation could not have ever
> >> been supported with valid upstream DT bindings in the first place ?
> >>
> >> Should we now require that kernel drivers somehow magically support all
> >> kinds of random broken DT bindings in addition to ones which pass YAML DT
> >> validation ?
> >
> > The thing is, as I told you multiple times already, it was broken from
> > the bridge standpoint, but not from the controller's. If it had been
> > correct for the bridge, it wouldn't have been for the controller. So,
> > same story.
> >
> > The only difference is that it wouldn't affect you, but I don't see how
> > it's relevant.
>
> I'm sorry, I do not understand this answer.
>
> >>>>>>> So figuring it out is very much a prerequisite to that series,
> >>>>>>> especially since those patches effectively make the OF-graph mandatory
> >>>>>>> in some situations, while it was purely aesthetics before.
> >>>>>>
> >>>>>> The OF-graph is mandatory per the DT bindings of this driver. If you
> >>>>>> implement invalid DT which does not contain port@0, it will fail DT
> >>>>>> validation.
> >>>>>>
> >>>>>> If this requirement should be relaxed, sure, it can and I don't think it
> >>>>>> would be hard to do, but I don't see why that should be part of this series,
> >>>>>> which follows the upstream DT binding document for this driver.
> >>>>>>
> >>>>>> If I cannot trust the driver DT bindings to indicate what is and is not
> >>>>>> mandatory, what other document can I trust then ...
> >>>>>
> >>>>> Oh, come on. Doing that, you also require OF-Graph support for the DSI
> >>>>> controller you attach to, and you can't require that. This is very
> >>>>> different from just requiring a property that doesn't have any impact on
> >>>>> any other device, and you know that very well.
> >>>>
> >>>> Currently the ICN6211 DT bindings DO require that kind of bridge.
> >>>
> >>> And while this wasn't enforced before, you make it a hard requirement
> >>> with this series. This is what changed, and what caused this whole
> >>> discussion.
> >>
> >> The current DT bindings already make it a hard requirement, so no, nothing
> >> changed here.
> >>
> >> Unless what you are trying to ask for is support for broken DT bindings
> >> which do not pass YAML DT validation by this driver, but that is very
> >> dangerous, because then the question is, how far should we support such
> >> broken bindings. What kind of broken is still OK and what kind of broken is
> >> no longer OK ?
> >
> > If it ever worked in a mainline release, it must always work. See:
> > https://www.kernel.org/doc/html/latest/devicetree/bindings/ABI.html
>
> > As far as I'm concerned, it's the sole criteria. So to answer your
> > question, if it was broken but worked at some point, yes, we need to
> > keep supporting it. If it never worked, no, we don't.
>
> There are no users of this driver in any mainline release.
>
> DT is ABI, and ICN6211 DT bindings says port@0 is mandatory. If this
> driver worked with some broken downstream DT without port@0, then that
> downstream depended on undefined behavior which I cannot fathom how it
> can be considered part of kernel ABI. That downstream should fix its DT
> instead.

Yes, agreed that ICN6211 DT bindings say port@0 is mandatory. However,
marking port@0 (after fixing DT binding) with non-I2C-ICN6211 is still
valid and we can see many examples of adding a bridge as a child to
the DSI host (without port@0) and it is not broken DT binding at all
(at least when it comes to DSI).  You are correct and port@0 is
mandatory for I2C-ICN6211.

Since the driver and this series trying to support the non-I2C and I2C
based ICN6211 bridge. I think it is valid to make changes according to
this series - IMHO.

Thanks,
Jagan.

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Jagan Teki]

Hi Marek,

Small correction in the previous comment.

On Sat, Mar 12, 2022 at 2:05 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> Hi Marek,
>
> On Sat, Mar 12, 2022 at 1:32 AM Marek Vasut <marex@denx.de> wrote:
> >
> > On 3/11/22 17:29, Maxime Ripard wrote:
> > > On Fri, Mar 11, 2022 at 11:36:58AM +0100, Marek Vasut wrote:
> > >> On 3/10/22 15:18, Maxime Ripard wrote:
> > >>> On Thu, Mar 10, 2022 at 01:47:13PM +0100, Marek Vasut wrote:
> > >>>> On 3/10/22 11:53, Maxime Ripard wrote:
> > >>>>> On Tue, Mar 08, 2022 at 10:41:05PM +0100, Marek Vasut wrote:
> > >>>>>> On 3/8/22 17:21, Maxime Ripard wrote:
> > >>>>>>> On Tue, Mar 08, 2022 at 03:47:22PM +0100, Marek Vasut wrote:
> > >>>>>>>> On 3/8/22 14:49, Maxime Ripard wrote:
> > >>>>>>>>> On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote:
> > >>>>>>>>>> On 3/8/22 13:51, Maxime Ripard wrote:
> > >>>>>>>>>>> On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
> > >>>>>>>>>>>> On 3/8/22 11:07, Jagan Teki wrote:
> > >>>>>>>>>>>>> On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> On 3/8/22 09:03, Jagan Teki wrote:
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> Hi,
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> [...]
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>> @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
> > >>>>>>>>>>>>>>>>          static int chipone_parse_dt(struct chipone *icn)
> > >>>>>>>>>>>>>>>>          {
> > >>>>>>>>>>>>>>>>                 struct device *dev = icn->dev;
> > >>>>>>>>>>>>>>>> +       struct device_node *endpoint;
> > >>>>>>>>>>>>>>>>                 struct drm_panel *panel;
> > >>>>>>>>>>>>>>>> +       int dsi_lanes;
> > >>>>>>>>>>>>>>>>                 int ret;
> > >>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>                 icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
> > >>>>>>>>>>>>>>>> @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
> > >>>>>>>>>>>>>>>>                         return PTR_ERR(icn->enable_gpio);
> > >>>>>>>>>>>>>>>>                 }
> > >>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>> +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> > >>>>>>>>>>>>>>>> +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> > >>>>>>>>>>>>>>>> +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
> > >>>>>>>>>>>>>>>> +       of_node_put(endpoint);
> > >>>>>>>>>>>>>>>> +
> > >>>>>>>>>>>>>>>> +       if (!icn->host_node)
> > >>>>>>>>>>>>>>>> +               return -ENODEV;
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> The non-ports-based OF graph returns a -19 example on the Allwinner
> > >>>>>>>>>>>>>>> Display pipeline in R16 [1].
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> We need to have a helper to return host_node for non-ports as I have
> > >>>>>>>>>>>>>>> done it for drm_of_find_bridge.
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> [1] https://patchwork.amarulasolutions.com/patch/1805/
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> The link points to a patch marked "DO NOT MERGE", maybe that patch is
> > >>>>>>>>>>>>>> missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
> > >>>>>>>>>>>>>> required, see:
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> What is "non-ports-based OF graph" ?
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> I don't see drm_of_find_bridge() in linux-next , what is that ?
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> port@0 is optional as some of the DSI host OF-graph represent the
> > >>>>>>>>>>>>> bridge or panel as child nodes instead of ports. (i think dt-binding
> > >>>>>>>>>>>>> has to fix it to make port@0 optional)
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> The current upstream DT binding document says:
> > >>>>>>>>>>>>
> > >>>>>>>>>>>>           required:
> > >>>>>>>>>>>>             - port@0
> > >>>>>>>>>>>>             - port@1
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> So port@0 is mandatory.
> > >>>>>>>>>>>
> > >>>>>>>>>>> In the binding, sure, but fundamentally the DT excerpt Jagan provided is
> > >>>>>>>>>>> correct. If the bridge supports DCS, there's no reason to use the OF
> > >>>>>>>>>>> graph in the first place: the bridge node will be a child node of the
> > >>>>>>>>>>> MIPI-DSI controller (and there's no obligation to use the OF-graph for a
> > >>>>>>>>>>> MIPI-DSI controller).
> > >>>>>>>>>>>
> > >>>>>>>>>>> I believe port@0 should be made optional (or downright removed if
> > >>>>>>>>>>> MIPI-DCS in the only control bus).
> > >>>>>>>>>>
> > >>>>>>>>>> That's out of scope of this series anyway, so Jagan can implement patches
> > >>>>>>>>>> for that mode if needed.
> > >>>>>>>>>
> > >>>>>>>>> Not really? You can't count on the port@0 being there generally
> > >>>>>>>>> speaking, so you can't count on data-lanes being there either, which
> > >>>>>>>>> exactly what you're doing in this patch.
> > >>>>>>>>
> > >>>>>>>> I can because the upstream DT bindings currently say port@0 must be present,
> > >>>>>>>> see above. If that requirement should be relaxed, sure, but that's a
> > >>>>>>>> separate series.
> > >>>>>>>
> > >>>>>>> And another upstream DT bindings say that you don't need them at all.
> > >>>>>>
> > >>>>>> Which "another upstream DT bindings" do you refer to ?
> > >>>>>
> > >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
> > >>>>>
> > >>>>>>> Yes, there's a conflict. Yes, it's unfortunate. But the generic DSI
> > >>>>>>> binding is far more relevant than a single bridge driver.
> > >>>>>>
> > >>>>>> That seems to be the wrong way around, how can generic subsystem-wide
> > >>>>>> binding take precedence over specific driver binding ?
> > >>>>>
> > >>>>> This is the binding of the bus. You're part of that bus. You're a child
> > >>>>> node of that bus, but nothing ever mandates that your parent node uses
> > >>>>> the same convention. And some don't. And since your bridge can be
> > >>>>> connected to pretty much any DSI controller, you have to use the lowest
> > >>>>> common denominator, not make up some new constraints that not all
> > >>>>> controller will be able to comply with.
> > >>>>
> > >>>> It seems to me the ICN6211 DT bindings currently further constraint the
> > >>>> generic DSI bus bindings, and that seems OK to me.
> > >>>>
> > >>>> Let me reiterate this again -- if someone wants to relax the requirements
> > >>>> currently imposed by the ICN6211 DT bindings, fine, but that can be done in
> > >>>> a separate patchset AND that needs DT bindings update. Furthermore, there
> > >>>> are no users of this ICN6211 bridge in upstream DTs, so there is currently
> > >>>> no bridge which would operate without OF graph using this driver.
> > >>>
> > >>> And let me reiterate this again: something that used to work for a user
> > >>> doesn't anymore when your series is applied. This is a textbook
> > >>> regression. I suggested a way forward, that you don't like for some
> > >>> reason, fine. But pushing through a regression is just not acceptable.
> > >>
> > >> How can this be a regression if this mode of operation could not have ever
> > >> been supported with valid upstream DT bindings in the first place ?
> > >>
> > >> Should we now require that kernel drivers somehow magically support all
> > >> kinds of random broken DT bindings in addition to ones which pass YAML DT
> > >> validation ?
> > >
> > > The thing is, as I told you multiple times already, it was broken from
> > > the bridge standpoint, but not from the controller's. If it had been
> > > correct for the bridge, it wouldn't have been for the controller. So,
> > > same story.
> > >
> > > The only difference is that it wouldn't affect you, but I don't see how
> > > it's relevant.
> >
> > I'm sorry, I do not understand this answer.
> >
> > >>>>>>> So figuring it out is very much a prerequisite to that series,
> > >>>>>>> especially since those patches effectively make the OF-graph mandatory
> > >>>>>>> in some situations, while it was purely aesthetics before.
> > >>>>>>
> > >>>>>> The OF-graph is mandatory per the DT bindings of this driver. If you
> > >>>>>> implement invalid DT which does not contain port@0, it will fail DT
> > >>>>>> validation.
> > >>>>>>
> > >>>>>> If this requirement should be relaxed, sure, it can and I don't think it
> > >>>>>> would be hard to do, but I don't see why that should be part of this series,
> > >>>>>> which follows the upstream DT binding document for this driver.
> > >>>>>>
> > >>>>>> If I cannot trust the driver DT bindings to indicate what is and is not
> > >>>>>> mandatory, what other document can I trust then ...
> > >>>>>
> > >>>>> Oh, come on. Doing that, you also require OF-Graph support for the DSI
> > >>>>> controller you attach to, and you can't require that. This is very
> > >>>>> different from just requiring a property that doesn't have any impact on
> > >>>>> any other device, and you know that very well.
> > >>>>
> > >>>> Currently the ICN6211 DT bindings DO require that kind of bridge.
> > >>>
> > >>> And while this wasn't enforced before, you make it a hard requirement
> > >>> with this series. This is what changed, and what caused this whole
> > >>> discussion.
> > >>
> > >> The current DT bindings already make it a hard requirement, so no, nothing
> > >> changed here.
> > >>
> > >> Unless what you are trying to ask for is support for broken DT bindings
> > >> which do not pass YAML DT validation by this driver, but that is very
> > >> dangerous, because then the question is, how far should we support such
> > >> broken bindings. What kind of broken is still OK and what kind of broken is
> > >> no longer OK ?
> > >
> > > If it ever worked in a mainline release, it must always work. See:
> > > https://www.kernel.org/doc/html/latest/devicetree/bindings/ABI.html
> >
> > > As far as I'm concerned, it's the sole criteria. So to answer your
> > > question, if it was broken but worked at some point, yes, we need to
> > > keep supporting it. If it never worked, no, we don't.
> >
> > There are no users of this driver in any mainline release.
> >
> > DT is ABI, and ICN6211 DT bindings says port@0 is mandatory. If this
> > driver worked with some broken downstream DT without port@0, then that
> > downstream depended on undefined behavior which I cannot fathom how it
> > can be considered part of kernel ABI. That downstream should fix its DT
> > instead.
>
> Yes, agreed that ICN6211 DT bindings say port@0 is mandatory. However,
> marking port@0 (after fixing DT binding) with non-I2C-ICN6211 is still

s/port@0/port@0 as optional/

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Marek Vasut]

On 3/11/22 21:38, Jagan Teki wrote:
> Hi Marek,
> 
> Small correction in the previous comment.
> 
> On Sat, Mar 12, 2022 at 2:05 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>>
>> Hi Marek,
>>
>> On Sat, Mar 12, 2022 at 1:32 AM Marek Vasut <marex@denx.de> wrote:
>>>
>>> On 3/11/22 17:29, Maxime Ripard wrote:
>>>> On Fri, Mar 11, 2022 at 11:36:58AM +0100, Marek Vasut wrote:
>>>>> On 3/10/22 15:18, Maxime Ripard wrote:
>>>>>> On Thu, Mar 10, 2022 at 01:47:13PM +0100, Marek Vasut wrote:
>>>>>>> On 3/10/22 11:53, Maxime Ripard wrote:
>>>>>>>> On Tue, Mar 08, 2022 at 10:41:05PM +0100, Marek Vasut wrote:
>>>>>>>>> On 3/8/22 17:21, Maxime Ripard wrote:
>>>>>>>>>> On Tue, Mar 08, 2022 at 03:47:22PM +0100, Marek Vasut wrote:
>>>>>>>>>>> On 3/8/22 14:49, Maxime Ripard wrote:
>>>>>>>>>>>> On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote:
>>>>>>>>>>>>> On 3/8/22 13:51, Maxime Ripard wrote:
>>>>>>>>>>>>>> On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
>>>>>>>>>>>>>>> On 3/8/22 11:07, Jagan Teki wrote:
>>>>>>>>>>>>>>>> On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 3/8/22 09:03, Jagan Teki wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> [...]
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
>>>>>>>>>>>>>>>>>>>           static int chipone_parse_dt(struct chipone *icn)
>>>>>>>>>>>>>>>>>>>           {
>>>>>>>>>>>>>>>>>>>                  struct device *dev = icn->dev;
>>>>>>>>>>>>>>>>>>> +       struct device_node *endpoint;
>>>>>>>>>>>>>>>>>>>                  struct drm_panel *panel;
>>>>>>>>>>>>>>>>>>> +       int dsi_lanes;
>>>>>>>>>>>>>>>>>>>                  int ret;
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>                  icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
>>>>>>>>>>>>>>>>>>> @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
>>>>>>>>>>>>>>>>>>>                          return PTR_ERR(icn->enable_gpio);
>>>>>>>>>>>>>>>>>>>                  }
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
>>>>>>>>>>>>>>>>>>> +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
>>>>>>>>>>>>>>>>>>> +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
>>>>>>>>>>>>>>>>>>> +       of_node_put(endpoint);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> +       if (!icn->host_node)
>>>>>>>>>>>>>>>>>>> +               return -ENODEV;
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> The non-ports-based OF graph returns a -19 example on the Allwinner
>>>>>>>>>>>>>>>>>> Display pipeline in R16 [1].
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> We need to have a helper to return host_node for non-ports as I have
>>>>>>>>>>>>>>>>>> done it for drm_of_find_bridge.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> [1] https://patchwork.amarulasolutions.com/patch/1805/
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> The link points to a patch marked "DO NOT MERGE", maybe that patch is
>>>>>>>>>>>>>>>>> missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
>>>>>>>>>>>>>>>>> required, see:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> What is "non-ports-based OF graph" ?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I don't see drm_of_find_bridge() in linux-next , what is that ?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> port@0 is optional as some of the DSI host OF-graph represent the
>>>>>>>>>>>>>>>> bridge or panel as child nodes instead of ports. (i think dt-binding
>>>>>>>>>>>>>>>> has to fix it to make port@0 optional)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The current upstream DT binding document says:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>            required:
>>>>>>>>>>>>>>>              - port@0
>>>>>>>>>>>>>>>              - port@1
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> So port@0 is mandatory.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> In the binding, sure, but fundamentally the DT excerpt Jagan provided is
>>>>>>>>>>>>>> correct. If the bridge supports DCS, there's no reason to use the OF
>>>>>>>>>>>>>> graph in the first place: the bridge node will be a child node of the
>>>>>>>>>>>>>> MIPI-DSI controller (and there's no obligation to use the OF-graph for a
>>>>>>>>>>>>>> MIPI-DSI controller).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I believe port@0 should be made optional (or downright removed if
>>>>>>>>>>>>>> MIPI-DCS in the only control bus).
>>>>>>>>>>>>>
>>>>>>>>>>>>> That's out of scope of this series anyway, so Jagan can implement patches
>>>>>>>>>>>>> for that mode if needed.
>>>>>>>>>>>>
>>>>>>>>>>>> Not really? You can't count on the port@0 being there generally
>>>>>>>>>>>> speaking, so you can't count on data-lanes being there either, which
>>>>>>>>>>>> exactly what you're doing in this patch.
>>>>>>>>>>>
>>>>>>>>>>> I can because the upstream DT bindings currently say port@0 must be present,
>>>>>>>>>>> see above. If that requirement should be relaxed, sure, but that's a
>>>>>>>>>>> separate series.
>>>>>>>>>>
>>>>>>>>>> And another upstream DT bindings say that you don't need them at all.
>>>>>>>>>
>>>>>>>>> Which "another upstream DT bindings" do you refer to ?
>>>>>>>>
>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
>>>>>>>>
>>>>>>>>>> Yes, there's a conflict. Yes, it's unfortunate. But the generic DSI
>>>>>>>>>> binding is far more relevant than a single bridge driver.
>>>>>>>>>
>>>>>>>>> That seems to be the wrong way around, how can generic subsystem-wide
>>>>>>>>> binding take precedence over specific driver binding ?
>>>>>>>>
>>>>>>>> This is the binding of the bus. You're part of that bus. You're a child
>>>>>>>> node of that bus, but nothing ever mandates that your parent node uses
>>>>>>>> the same convention. And some don't. And since your bridge can be
>>>>>>>> connected to pretty much any DSI controller, you have to use the lowest
>>>>>>>> common denominator, not make up some new constraints that not all
>>>>>>>> controller will be able to comply with.
>>>>>>>
>>>>>>> It seems to me the ICN6211 DT bindings currently further constraint the
>>>>>>> generic DSI bus bindings, and that seems OK to me.
>>>>>>>
>>>>>>> Let me reiterate this again -- if someone wants to relax the requirements
>>>>>>> currently imposed by the ICN6211 DT bindings, fine, but that can be done in
>>>>>>> a separate patchset AND that needs DT bindings update. Furthermore, there
>>>>>>> are no users of this ICN6211 bridge in upstream DTs, so there is currently
>>>>>>> no bridge which would operate without OF graph using this driver.
>>>>>>
>>>>>> And let me reiterate this again: something that used to work for a user
>>>>>> doesn't anymore when your series is applied. This is a textbook
>>>>>> regression. I suggested a way forward, that you don't like for some
>>>>>> reason, fine. But pushing through a regression is just not acceptable.
>>>>>
>>>>> How can this be a regression if this mode of operation could not have ever
>>>>> been supported with valid upstream DT bindings in the first place ?
>>>>>
>>>>> Should we now require that kernel drivers somehow magically support all
>>>>> kinds of random broken DT bindings in addition to ones which pass YAML DT
>>>>> validation ?
>>>>
>>>> The thing is, as I told you multiple times already, it was broken from
>>>> the bridge standpoint, but not from the controller's. If it had been
>>>> correct for the bridge, it wouldn't have been for the controller. So,
>>>> same story.
>>>>
>>>> The only difference is that it wouldn't affect you, but I don't see how
>>>> it's relevant.
>>>
>>> I'm sorry, I do not understand this answer.
>>>
>>>>>>>>>> So figuring it out is very much a prerequisite to that series,
>>>>>>>>>> especially since those patches effectively make the OF-graph mandatory
>>>>>>>>>> in some situations, while it was purely aesthetics before.
>>>>>>>>>
>>>>>>>>> The OF-graph is mandatory per the DT bindings of this driver. If you
>>>>>>>>> implement invalid DT which does not contain port@0, it will fail DT
>>>>>>>>> validation.
>>>>>>>>>
>>>>>>>>> If this requirement should be relaxed, sure, it can and I don't think it
>>>>>>>>> would be hard to do, but I don't see why that should be part of this series,
>>>>>>>>> which follows the upstream DT binding document for this driver.
>>>>>>>>>
>>>>>>>>> If I cannot trust the driver DT bindings to indicate what is and is not
>>>>>>>>> mandatory, what other document can I trust then ...
>>>>>>>>
>>>>>>>> Oh, come on. Doing that, you also require OF-Graph support for the DSI
>>>>>>>> controller you attach to, and you can't require that. This is very
>>>>>>>> different from just requiring a property that doesn't have any impact on
>>>>>>>> any other device, and you know that very well.
>>>>>>>
>>>>>>> Currently the ICN6211 DT bindings DO require that kind of bridge.
>>>>>>
>>>>>> And while this wasn't enforced before, you make it a hard requirement
>>>>>> with this series. This is what changed, and what caused this whole
>>>>>> discussion.
>>>>>
>>>>> The current DT bindings already make it a hard requirement, so no, nothing
>>>>> changed here.
>>>>>
>>>>> Unless what you are trying to ask for is support for broken DT bindings
>>>>> which do not pass YAML DT validation by this driver, but that is very
>>>>> dangerous, because then the question is, how far should we support such
>>>>> broken bindings. What kind of broken is still OK and what kind of broken is
>>>>> no longer OK ?
>>>>
>>>> If it ever worked in a mainline release, it must always work. See:
>>>> https://www.kernel.org/doc/html/latest/devicetree/bindings/ABI.html
>>>
>>>> As far as I'm concerned, it's the sole criteria. So to answer your
>>>> question, if it was broken but worked at some point, yes, we need to
>>>> keep supporting it. If it never worked, no, we don't.
>>>
>>> There are no users of this driver in any mainline release.
>>>
>>> DT is ABI, and ICN6211 DT bindings says port@0 is mandatory. If this
>>> driver worked with some broken downstream DT without port@0, then that
>>> downstream depended on undefined behavior which I cannot fathom how it
>>> can be considered part of kernel ABI. That downstream should fix its DT
>>> instead.
>>
>> Yes, agreed that ICN6211 DT bindings say port@0 is mandatory. However,
>> marking port@0 (after fixing DT binding) with non-I2C-ICN6211 is still
> 
> s/port@0/port@0 as optional/

The V4 requires the port@0 only for I2C bus option now, that should at 
least retain the "compatibility".

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Laurent Pinchart]

Hi Maxime,

On Tue, Mar 08, 2022 at 01:51:40PM +0100, Maxime Ripard wrote:
> On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
> > On 3/8/22 11:07, Jagan Teki wrote:
> > > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
> > > > 
> > > > On 3/8/22 09:03, Jagan Teki wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > [...]
> > > > 
> > > > > > @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
> > > > > >    static int chipone_parse_dt(struct chipone *icn)
> > > > > >    {
> > > > > >           struct device *dev = icn->dev;
> > > > > > +       struct device_node *endpoint;
> > > > > >           struct drm_panel *panel;
> > > > > > +       int dsi_lanes;
> > > > > >           int ret;
> > > > > > 
> > > > > >           icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
> > > > > > @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
> > > > > >                   return PTR_ERR(icn->enable_gpio);
> > > > > >           }
> > > > > > 
> > > > > > +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> > > > > > +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> > > > > > +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
> > > > > > +       of_node_put(endpoint);
> > > > > > +
> > > > > > +       if (!icn->host_node)
> > > > > > +               return -ENODEV;
> > > > > 
> > > > > The non-ports-based OF graph returns a -19 example on the Allwinner
> > > > > Display pipeline in R16 [1].
> > > > > 
> > > > > We need to have a helper to return host_node for non-ports as I have
> > > > > done it for drm_of_find_bridge.
> > > > > 
> > > > > [1] https://patchwork.amarulasolutions.com/patch/1805/
> > > > 
> > > > The link points to a patch marked "DO NOT MERGE", maybe that patch is
> > > > missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
> > > > required, see:
> > > > 
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
> > > > 
> > > > What is "non-ports-based OF graph" ?
> > > > 
> > > > I don't see drm_of_find_bridge() in linux-next , what is that ?
> > > 
> > > port@0 is optional as some of the DSI host OF-graph represent the
> > > bridge or panel as child nodes instead of ports. (i think dt-binding
> > > has to fix it to make port@0 optional)
> > 
> > The current upstream DT binding document says:
> > 
> >     required:
> >       - port@0
> >       - port@1
> > 
> > So port@0 is mandatory.
> 
> In the binding, sure, but fundamentally the DT excerpt Jagan provided is
> correct. If the bridge supports DCS, there's no reason to use the OF
> graph in the first place: the bridge node will be a child node of the
> MIPI-DSI controller (and there's no obligation to use the OF-graph for a
> MIPI-DSI controller).
> 
> I believe port@0 should be made optional (or downright removed if
> MIPI-DCS in the only control bus).

I think we should make ports mandatory in all cases actually.

The DT parent-child hierarchy is meant to model control relations
between devices, so a DSI device controlled through DCS should be a
child of the DSI controller. No disagreement there.

The OF graph is meant to model data connections. While a DSI device
controlled through DCS will use the same DSI link for data transfer, the
two concepts are different. We have taken shortcuts and decided to not
use OF graph for some DSI devices (not necessarily as a well thought
decision, it was sometimes just not considered). This has led to
different issues that we're having to deal with today, making it more
difficult to develop generic code. Going forward, I think new bindings
should always use OF graph to model the data connection.


-- 
Regards,

Laurent Pinchart

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4715 bytes --]

On Thu, Mar 10, 2022 at 12:42:12PM +0200, Laurent Pinchart wrote:
> Hi Maxime,
> 
> On Tue, Mar 08, 2022 at 01:51:40PM +0100, Maxime Ripard wrote:
> > On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
> > > On 3/8/22 11:07, Jagan Teki wrote:
> > > > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
> > > > > 
> > > > > On 3/8/22 09:03, Jagan Teki wrote:
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > > @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
> > > > > > >    static int chipone_parse_dt(struct chipone *icn)
> > > > > > >    {
> > > > > > >           struct device *dev = icn->dev;
> > > > > > > +       struct device_node *endpoint;
> > > > > > >           struct drm_panel *panel;
> > > > > > > +       int dsi_lanes;
> > > > > > >           int ret;
> > > > > > > 
> > > > > > >           icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
> > > > > > > @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
> > > > > > >                   return PTR_ERR(icn->enable_gpio);
> > > > > > >           }
> > > > > > > 
> > > > > > > +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> > > > > > > +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> > > > > > > +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
> > > > > > > +       of_node_put(endpoint);
> > > > > > > +
> > > > > > > +       if (!icn->host_node)
> > > > > > > +               return -ENODEV;
> > > > > > 
> > > > > > The non-ports-based OF graph returns a -19 example on the Allwinner
> > > > > > Display pipeline in R16 [1].
> > > > > > 
> > > > > > We need to have a helper to return host_node for non-ports as I have
> > > > > > done it for drm_of_find_bridge.
> > > > > > 
> > > > > > [1] https://patchwork.amarulasolutions.com/patch/1805/
> > > > > 
> > > > > The link points to a patch marked "DO NOT MERGE", maybe that patch is
> > > > > missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
> > > > > required, see:
> > > > > 
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
> > > > > 
> > > > > What is "non-ports-based OF graph" ?
> > > > > 
> > > > > I don't see drm_of_find_bridge() in linux-next , what is that ?
> > > > 
> > > > port@0 is optional as some of the DSI host OF-graph represent the
> > > > bridge or panel as child nodes instead of ports. (i think dt-binding
> > > > has to fix it to make port@0 optional)
> > > 
> > > The current upstream DT binding document says:
> > > 
> > >     required:
> > >       - port@0
> > >       - port@1
> > > 
> > > So port@0 is mandatory.
> > 
> > In the binding, sure, but fundamentally the DT excerpt Jagan provided is
> > correct. If the bridge supports DCS, there's no reason to use the OF
> > graph in the first place: the bridge node will be a child node of the
> > MIPI-DSI controller (and there's no obligation to use the OF-graph for a
> > MIPI-DSI controller).
> > 
> > I believe port@0 should be made optional (or downright removed if
> > MIPI-DCS in the only control bus).
> 
> I think we should make ports mandatory in all cases actually.
> 
> The DT parent-child hierarchy is meant to model control relations
> between devices, so a DSI device controlled through DCS should be a
> child of the DSI controller. No disagreement there.
> 
> The OF graph is meant to model data connections. While a DSI device
> controlled through DCS will use the same DSI link for data transfer, the
> two concepts are different. We have taken shortcuts and decided to not
> use OF graph for some DSI devices (not necessarily as a well thought
> decision, it was sometimes just not considered).

I disagree. Unless the data path is explicitly stated using the OF-graph
or some other binding, it's inferred. We never asked ourselves where the
data from an i2c chip, an ethernet controller or an v4l2 output device
was coming from. It comes from the parent bus, because it's what makes
sense. Making a requirement on the OF-Graph to model this would create a
big inconsistency.

> This has led to different issues that we're having to deal with today,
> making it more difficult to develop generic code. Going forward, I
> think new bindings should always use OF graph to model the data
> connection.

Either way, that discussion is irrelevant. Not all DSI controllers use
OF-Graph, a bridge can be attached to any of them, so we can't require
OF-Graph support in any bridge.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Laurent Pinchart]

Hi Maxime,

On Thu, Mar 10, 2022 at 11:57:38AM +0100, Maxime Ripard wrote:
> On Thu, Mar 10, 2022 at 12:42:12PM +0200, Laurent Pinchart wrote:
> > On Tue, Mar 08, 2022 at 01:51:40PM +0100, Maxime Ripard wrote:
> > > On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
> > > > On 3/8/22 11:07, Jagan Teki wrote:
> > > > > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
> > > > > > 
> > > > > > On 3/8/22 09:03, Jagan Teki wrote:
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > [...]
> > > > > > 
> > > > > > > > @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
> > > > > > > >    static int chipone_parse_dt(struct chipone *icn)
> > > > > > > >    {
> > > > > > > >           struct device *dev = icn->dev;
> > > > > > > > +       struct device_node *endpoint;
> > > > > > > >           struct drm_panel *panel;
> > > > > > > > +       int dsi_lanes;
> > > > > > > >           int ret;
> > > > > > > > 
> > > > > > > >           icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
> > > > > > > > @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
> > > > > > > >                   return PTR_ERR(icn->enable_gpio);
> > > > > > > >           }
> > > > > > > > 
> > > > > > > > +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> > > > > > > > +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> > > > > > > > +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
> > > > > > > > +       of_node_put(endpoint);
> > > > > > > > +
> > > > > > > > +       if (!icn->host_node)
> > > > > > > > +               return -ENODEV;
> > > > > > > 
> > > > > > > The non-ports-based OF graph returns a -19 example on the Allwinner
> > > > > > > Display pipeline in R16 [1].
> > > > > > > 
> > > > > > > We need to have a helper to return host_node for non-ports as I have
> > > > > > > done it for drm_of_find_bridge.
> > > > > > > 
> > > > > > > [1] https://patchwork.amarulasolutions.com/patch/1805/
> > > > > > 
> > > > > > The link points to a patch marked "DO NOT MERGE", maybe that patch is
> > > > > > missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
> > > > > > required, see:
> > > > > > 
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
> > > > > > 
> > > > > > What is "non-ports-based OF graph" ?
> > > > > > 
> > > > > > I don't see drm_of_find_bridge() in linux-next , what is that ?
> > > > > 
> > > > > port@0 is optional as some of the DSI host OF-graph represent the
> > > > > bridge or panel as child nodes instead of ports. (i think dt-binding
> > > > > has to fix it to make port@0 optional)
> > > > 
> > > > The current upstream DT binding document says:
> > > > 
> > > >     required:
> > > >       - port@0
> > > >       - port@1
> > > > 
> > > > So port@0 is mandatory.
> > > 
> > > In the binding, sure, but fundamentally the DT excerpt Jagan provided is
> > > correct. If the bridge supports DCS, there's no reason to use the OF
> > > graph in the first place: the bridge node will be a child node of the
> > > MIPI-DSI controller (and there's no obligation to use the OF-graph for a
> > > MIPI-DSI controller).
> > > 
> > > I believe port@0 should be made optional (or downright removed if
> > > MIPI-DCS in the only control bus).
> > 
> > I think we should make ports mandatory in all cases actually.
> > 
> > The DT parent-child hierarchy is meant to model control relations
> > between devices, so a DSI device controlled through DCS should be a
> > child of the DSI controller. No disagreement there.
> > 
> > The OF graph is meant to model data connections. While a DSI device
> > controlled through DCS will use the same DSI link for data transfer, the
> > two concepts are different. We have taken shortcuts and decided to not
> > use OF graph for some DSI devices (not necessarily as a well thought
> > decision, it was sometimes just not considered).
> 
> I disagree. Unless the data path is explicitly stated using the OF-graph
> or some other binding, it's inferred.

It is today, and for video data, I think it's showing to be a problem
:-)

> We never asked ourselves where the
> data from an i2c chip, an ethernet controller or an v4l2 output device
> was coming from. It comes from the parent bus, because it's what makes
> sense. Making a requirement on the OF-Graph to model this would create a
> big inconsistency.

I'm afraid I disagree, especially when it comes to data transfers from
device to device. The device tree has never tried to model those until
OF graph.

> > This has led to different issues that we're having to deal with today,
> > making it more difficult to develop generic code. Going forward, I
> > think new bindings should always use OF graph to model the data
> > connection.
> 
> Either way, that discussion is irrelevant. Not all DSI controllers use
> OF-Graph, a bridge can be attached to any of them, so we can't require
> OF-Graph support in any bridge.

Not in any bridge, but we could in new ones, and we could also require
it in new DT for existing bridge to support new features.

-- 
Regards,

Laurent Pinchart

Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6145 bytes --]

On Thu, Mar 10, 2022 at 01:16:57PM +0200, Laurent Pinchart wrote:
> On Thu, Mar 10, 2022 at 11:57:38AM +0100, Maxime Ripard wrote:
> > On Thu, Mar 10, 2022 at 12:42:12PM +0200, Laurent Pinchart wrote:
> > > On Tue, Mar 08, 2022 at 01:51:40PM +0100, Maxime Ripard wrote:
> > > > On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote:
> > > > > On 3/8/22 11:07, Jagan Teki wrote:
> > > > > > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut <marex@denx.de> wrote:
> > > > > > > 
> > > > > > > On 3/8/22 09:03, Jagan Teki wrote:
> > > > > > > 
> > > > > > > Hi,
> > > > > > > 
> > > > > > > [...]
> > > > > > > 
> > > > > > > > > @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
> > > > > > > > >    static int chipone_parse_dt(struct chipone *icn)
> > > > > > > > >    {
> > > > > > > > >           struct device *dev = icn->dev;
> > > > > > > > > +       struct device_node *endpoint;
> > > > > > > > >           struct drm_panel *panel;
> > > > > > > > > +       int dsi_lanes;
> > > > > > > > >           int ret;
> > > > > > > > > 
> > > > > > > > >           icn->vdd1 = devm_regulator_get_optional(dev, "vdd1");
> > > > > > > > > @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn)
> > > > > > > > >                   return PTR_ERR(icn->enable_gpio);
> > > > > > > > >           }
> > > > > > > > > 
> > > > > > > > > +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> > > > > > > > > +       dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> > > > > > > > > +       icn->host_node = of_graph_get_remote_port_parent(endpoint);
> > > > > > > > > +       of_node_put(endpoint);
> > > > > > > > > +
> > > > > > > > > +       if (!icn->host_node)
> > > > > > > > > +               return -ENODEV;
> > > > > > > > 
> > > > > > > > The non-ports-based OF graph returns a -19 example on the Allwinner
> > > > > > > > Display pipeline in R16 [1].
> > > > > > > > 
> > > > > > > > We need to have a helper to return host_node for non-ports as I have
> > > > > > > > done it for drm_of_find_bridge.
> > > > > > > > 
> > > > > > > > [1] https://patchwork.amarulasolutions.com/patch/1805/
> > > > > > > 
> > > > > > > The link points to a patch marked "DO NOT MERGE", maybe that patch is
> > > > > > > missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are
> > > > > > > required, see:
> > > > > > > 
> > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53
> > > > > > > 
> > > > > > > What is "non-ports-based OF graph" ?
> > > > > > > 
> > > > > > > I don't see drm_of_find_bridge() in linux-next , what is that ?
> > > > > > 
> > > > > > port@0 is optional as some of the DSI host OF-graph represent the
> > > > > > bridge or panel as child nodes instead of ports. (i think dt-binding
> > > > > > has to fix it to make port@0 optional)
> > > > > 
> > > > > The current upstream DT binding document says:
> > > > > 
> > > > >     required:
> > > > >       - port@0
> > > > >       - port@1
> > > > > 
> > > > > So port@0 is mandatory.
> > > > 
> > > > In the binding, sure, but fundamentally the DT excerpt Jagan provided is
> > > > correct. If the bridge supports DCS, there's no reason to use the OF
> > > > graph in the first place: the bridge node will be a child node of the
> > > > MIPI-DSI controller (and there's no obligation to use the OF-graph for a
> > > > MIPI-DSI controller).
> > > > 
> > > > I believe port@0 should be made optional (or downright removed if
> > > > MIPI-DCS in the only control bus).
> > > 
> > > I think we should make ports mandatory in all cases actually.
> > > 
> > > The DT parent-child hierarchy is meant to model control relations
> > > between devices, so a DSI device controlled through DCS should be a
> > > child of the DSI controller. No disagreement there.
> > > 
> > > The OF graph is meant to model data connections. While a DSI device
> > > controlled through DCS will use the same DSI link for data transfer, the
> > > two concepts are different. We have taken shortcuts and decided to not
> > > use OF graph for some DSI devices (not necessarily as a well thought
> > > decision, it was sometimes just not considered).
> > 
> > I disagree. Unless the data path is explicitly stated using the OF-graph
> > or some other binding, it's inferred.
> 
> It is today, and for video data, I think it's showing to be a problem
> :-)
> 
> > We never asked ourselves where the
> > data from an i2c chip, an ethernet controller or an v4l2 output device
> > was coming from. It comes from the parent bus, because it's what makes
> > sense. Making a requirement on the OF-Graph to model this would create a
> > big inconsistency.
> 
> I'm afraid I disagree, especially when it comes to data transfers from
> device to device. The device tree has never tried to model those until
> OF graph.

Except the data transfer isn't happening between "siblings" devices like
a CRTC and an HDMI controller that the OF-Graph describes perfectly
fine. Here the transfer happens between a parent device and its child.
So it's a bus to device transfer, just like any DMA transfer, or MDIO
transfer, or SPI transfer, etc.

> > > This has led to different issues that we're having to deal with today,
> > > making it more difficult to develop generic code. Going forward, I
> > > think new bindings should always use OF graph to model the data
> > > connection.
> > 
> > Either way, that discussion is irrelevant. Not all DSI controllers use
> > OF-Graph, a bridge can be attached to any of them, so we can't require
> > OF-Graph support in any bridge.
> 
> Not in any bridge, but we could in new ones, and we could also require
> it in new DT for existing bridge to support new features.

Above disagreement aside, since not all controllers are using it, it
would effectively prevent those new bridges from being used with
!OF-Graph controllers. I don't see how that can be an option.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH V3 06/13] drm: bridge: icn6211: Add generic DSI-to-DPI PLL configuration

[Marek Vasut]

The chip contains fractional PLL, however the driver currently hard-codes
one specific PLL setting. Implement generic PLL parameter calculation code,
so any DPI panel with arbitrary pixel clock can be attached to this bridge.

The datasheet for this bridge is not available, the PLL behavior has been
inferred from [1] and [2] and by analyzing the DPI pixel clock with scope.
The PLL limits might be wrong, but at least the calculated values match all
the example code available. This is better than one hard-coded pixel clock
value anyway.

[1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/gpu/drm/bridge/icn6211.c
[2] https://github.com/tdjastrzebski/ICN6211-Configurator

Acked-by: Maxime Ripard <maxime@cerno.tech>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
V2: Rebase on next-20220214
V3: Add AB from Maxime
---
 drivers/gpu/drm/bridge/chipone-icn6211.c | 87 +++++++++++++++++++++++-
 1 file changed, 84 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index df8e75a068ad0..71c83a18984fa 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -163,6 +163,87 @@ static inline int chipone_dsi_write(struct chipone *icn,  const void *seq,
 		chipone_dsi_write(icn, d, ARRAY_SIZE(d));	\
 	}
 
+static void chipone_configure_pll(struct chipone *icn,
+				  const struct drm_display_mode *mode)
+{
+	unsigned int best_p = 0, best_m = 0, best_s = 0;
+	unsigned int delta, min_delta = 0xffffffff;
+	unsigned int freq_p, freq_s, freq_out;
+	unsigned int p_min, p_max;
+	unsigned int p, m, s;
+	unsigned int fin;
+
+	/*
+	 * DSI clock lane frequency (input into PLL) is calculated as:
+	 *  DSI_CLK = mode clock * bpp / dsi_data_lanes / 2
+	 * the 2 is there because the bus is DDR.
+	 *
+	 * DPI pixel clock frequency (output from PLL) is mode clock.
+	 *
+	 * The chip contains fractional PLL which works as follows:
+	 *  DPI_CLK = ((DSI_CLK / P) * M) / S
+	 * P is pre-divider, register PLL_REF_DIV[3:0] is 2^(n+1) divider
+	 *                   register PLL_REF_DIV[4] is extra 1:2 divider
+	 * M is integer multiplier, register PLL_INT(0) is multiplier
+	 * S is post-divider, register PLL_REF_DIV[7:5] is 2^(n+1) divider
+	 *
+	 * It seems the PLL input clock after applying P pre-divider have
+	 * to be lower than 20 MHz.
+	 */
+	fin = mode->clock * mipi_dsi_pixel_format_to_bpp(icn->dsi->format) /
+	      icn->dsi_lanes / 2; /* in kHz */
+
+	/* Minimum value of P predivider for PLL input in 5..20 MHz */
+	p_min = ffs(fin / 20000);
+	p_max = (fls(fin / 5000) - 1) & 0x1f;
+
+	for (p = p_min; p < p_max; p++) {	/* PLL_REF_DIV[4,3:0] */
+		freq_p = fin / BIT(p + 1);
+		if (freq_p == 0)		/* Divider too high */
+			break;
+
+		for (s = 0; s < 0x7; s++) {	/* PLL_REF_DIV[7:5] */
+			freq_s = freq_p / BIT(s + 1);
+			if (freq_s == 0)	/* Divider too high */
+				break;
+
+			m = mode->clock / freq_s;
+
+			/* Multiplier is 8 bit */
+			if (m > 0xff)
+				continue;
+
+			/* Limit PLL VCO frequency to 1 GHz */
+			freq_out = (fin * m) / BIT(p + 1);
+			if (freq_out > 1000000)
+				continue;
+
+			/* Apply post-divider */
+			freq_out /= BIT(s + 1);
+
+			delta = abs(mode->clock - freq_out);
+			if (delta < min_delta) {
+				best_p = p;
+				best_m = m;
+				best_s = s;
+				min_delta = delta;
+			}
+		}
+	}
+
+	dev_dbg(icn->dev,
+		"PLL: P[3:0]=2^%d P[4]=2*%d M=%d S[7:5]=2^%d delta=%d => DSI f_in=%d kHz ; DPI f_out=%ld kHz\n",
+		best_p, !!best_p, best_m, best_s + 1, min_delta, fin,
+		(fin * best_m) / BIT(best_p + best_s + 2));
+
+	/* Clock source selection fixed to MIPI DSI clock lane */
+	ICN6211_DSI(icn, PLL_CTRL(6), PLL_CTRL_6_MIPI_CLK);
+	ICN6211_DSI(icn, PLL_REF_DIV,
+		    (best_p ? PLL_REF_DIV_Pe : 0) | /* Prefer /2 pre-divider */
+		    PLL_REF_DIV_P(best_p) | PLL_REF_DIV_S(best_s));
+	ICN6211_DSI(icn, PLL_INT(0), best_m);
+}
+
 static void chipone_atomic_enable(struct drm_bridge *bridge,
 				  struct drm_bridge_state *old_bridge_state)
 {
@@ -228,9 +309,9 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 	      ((bus_flags & DRM_BUS_FLAG_DE_HIGH) ? BIST_POL_DE_POL : 0);
 	ICN6211_DSI(icn, BIST_POL, pol);
 
-	ICN6211_DSI(icn, PLL_CTRL(6), PLL_CTRL_6_MIPI_CLK);
-	ICN6211_DSI(icn, PLL_REF_DIV, 0x71);
-	ICN6211_DSI(icn, PLL_INT(0), 0x2b);
+	/* Configure PLL settings */
+	chipone_configure_pll(icn, mode);
+
 	ICN6211_DSI(icn, SYS_CTRL(0), 0x40);
 	ICN6211_DSI(icn, SYS_CTRL(1), 0x98);
 
-- 
2.34.1

Re: [PATCH V3 06/13] drm: bridge: icn6211: Add generic DSI-to-DPI PLL configuration

[Jagan Teki]

On Fri, Mar 4, 2022 at 5:55 AM Marek Vasut <marex@denx.de> wrote:
>
> The chip contains fractional PLL, however the driver currently hard-codes
> one specific PLL setting. Implement generic PLL parameter calculation code,
> so any DPI panel with arbitrary pixel clock can be attached to this bridge.
>
> The datasheet for this bridge is not available, the PLL behavior has been
> inferred from [1] and [2] and by analyzing the DPI pixel clock with scope.
> The PLL limits might be wrong, but at least the calculated values match all
> the example code available. This is better than one hard-coded pixel clock
> value anyway.
>
> [1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/gpu/drm/bridge/icn6211.c
> [2] https://github.com/tdjastrzebski/ICN6211-Configurator
>
> Acked-by: Maxime Ripard <maxime@cerno.tech>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> To: dri-devel@lists.freedesktop.org
> ---
> V2: Rebase on next-20220214
> V3: Add AB from Maxime
> ---
>  drivers/gpu/drm/bridge/chipone-icn6211.c | 87 +++++++++++++++++++++++-
>  1 file changed, 84 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
> index df8e75a068ad0..71c83a18984fa 100644
> --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> @@ -163,6 +163,87 @@ static inline int chipone_dsi_write(struct chipone *icn,  const void *seq,
>                 chipone_dsi_write(icn, d, ARRAY_SIZE(d));       \
>         }
>
> +static void chipone_configure_pll(struct chipone *icn,
> +                                 const struct drm_display_mode *mode)
> +{
> +       unsigned int best_p = 0, best_m = 0, best_s = 0;
> +       unsigned int delta, min_delta = 0xffffffff;
> +       unsigned int freq_p, freq_s, freq_out;
> +       unsigned int p_min, p_max;
> +       unsigned int p, m, s;
> +       unsigned int fin;
> +
> +       /*
> +        * DSI clock lane frequency (input into PLL) is calculated as:
> +        *  DSI_CLK = mode clock * bpp / dsi_data_lanes / 2
> +        * the 2 is there because the bus is DDR.
> +        *
> +        * DPI pixel clock frequency (output from PLL) is mode clock.
> +        *
> +        * The chip contains fractional PLL which works as follows:
> +        *  DPI_CLK = ((DSI_CLK / P) * M) / S
> +        * P is pre-divider, register PLL_REF_DIV[3:0] is 2^(n+1) divider
> +        *                   register PLL_REF_DIV[4] is extra 1:2 divider
> +        * M is integer multiplier, register PLL_INT(0) is multiplier
> +        * S is post-divider, register PLL_REF_DIV[7:5] is 2^(n+1) divider
> +        *
> +        * It seems the PLL input clock after applying P pre-divider have
> +        * to be lower than 20 MHz.
> +        */
> +       fin = mode->clock * mipi_dsi_pixel_format_to_bpp(icn->dsi->format) /

no dsi in chipone structure. please preserve format during the probe.

drivers/gpu/drm/bridge/chipone-icn6211.c:193:61: error: ‘struct
chipone’ has no member named ‘dsi’
  193 |         fin = mode->clock *
mipi_dsi_pixel_format_to_bpp(icn->dsi->format) /

Thanks,
Jagan.

Re: [PATCH V3 06/13] drm: bridge: icn6211: Add generic DSI-to-DPI PLL configuration

[Marek Vasut]

On 3/8/22 09:07, Jagan Teki wrote:
> On Fri, Mar 4, 2022 at 5:55 AM Marek Vasut <marex@denx.de> wrote:
>>
>> The chip contains fractional PLL, however the driver currently hard-codes
>> one specific PLL setting. Implement generic PLL parameter calculation code,
>> so any DPI panel with arbitrary pixel clock can be attached to this bridge.
>>
>> The datasheet for this bridge is not available, the PLL behavior has been
>> inferred from [1] and [2] and by analyzing the DPI pixel clock with scope.
>> The PLL limits might be wrong, but at least the calculated values match all
>> the example code available. This is better than one hard-coded pixel clock
>> value anyway.
>>
>> [1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/gpu/drm/bridge/icn6211.c
>> [2] https://github.com/tdjastrzebski/ICN6211-Configurator
>>
>> Acked-by: Maxime Ripard <maxime@cerno.tech>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Jagan Teki <jagan@amarulasolutions.com>
>> Cc: Maxime Ripard <maxime@cerno.tech>
>> Cc: Robert Foss <robert.foss@linaro.org>
>> Cc: Sam Ravnborg <sam@ravnborg.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> To: dri-devel@lists.freedesktop.org
>> ---
>> V2: Rebase on next-20220214
>> V3: Add AB from Maxime
>> ---
>>   drivers/gpu/drm/bridge/chipone-icn6211.c | 87 +++++++++++++++++++++++-
>>   1 file changed, 84 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
>> index df8e75a068ad0..71c83a18984fa 100644
>> --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
>> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
>> @@ -163,6 +163,87 @@ static inline int chipone_dsi_write(struct chipone *icn,  const void *seq,
>>                  chipone_dsi_write(icn, d, ARRAY_SIZE(d));       \
>>          }
>>
>> +static void chipone_configure_pll(struct chipone *icn,
>> +                                 const struct drm_display_mode *mode)
>> +{
>> +       unsigned int best_p = 0, best_m = 0, best_s = 0;
>> +       unsigned int delta, min_delta = 0xffffffff;
>> +       unsigned int freq_p, freq_s, freq_out;
>> +       unsigned int p_min, p_max;
>> +       unsigned int p, m, s;
>> +       unsigned int fin;
>> +
>> +       /*
>> +        * DSI clock lane frequency (input into PLL) is calculated as:
>> +        *  DSI_CLK = mode clock * bpp / dsi_data_lanes / 2
>> +        * the 2 is there because the bus is DDR.
>> +        *
>> +        * DPI pixel clock frequency (output from PLL) is mode clock.
>> +        *
>> +        * The chip contains fractional PLL which works as follows:
>> +        *  DPI_CLK = ((DSI_CLK / P) * M) / S
>> +        * P is pre-divider, register PLL_REF_DIV[3:0] is 2^(n+1) divider
>> +        *                   register PLL_REF_DIV[4] is extra 1:2 divider
>> +        * M is integer multiplier, register PLL_INT(0) is multiplier
>> +        * S is post-divider, register PLL_REF_DIV[7:5] is 2^(n+1) divider
>> +        *
>> +        * It seems the PLL input clock after applying P pre-divider have
>> +        * to be lower than 20 MHz.
>> +        */
>> +       fin = mode->clock * mipi_dsi_pixel_format_to_bpp(icn->dsi->format) /
> 
> no dsi in chipone structure. please preserve format during the probe.
> 
> drivers/gpu/drm/bridge/chipone-icn6211.c:193:61: error: ‘struct
> chipone’ has no member named ‘dsi’
>    193 |         fin = mode->clock *
> mipi_dsi_pixel_format_to_bpp(icn->dsi->format) /

Ah , yes, this was added in
[PATCH V3 11/13] drm: bridge: icn6211: Add I2C configuration support
and needs to be moved here.

[PATCH V3 07/13] drm: bridge: icn6211: Use DSI burst mode without EoT and with LP command mode

[Marek Vasut]

The DSI burst mode is more energy efficient than the DSI sync pulse mode,
make use of the burst mode since the chip supports it as well. Disable the
generation of EoT packet, the chip ignores it, so no point in emitting it.
Enable transmission of data in LP mode, otherwise register read via DSI
does not work with this chip.

Acked-by: Maxime Ripard <maxime@cerno.tech>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
V2: Rebase on next-20220214
V3: Add AB from Maxime
---
 drivers/gpu/drm/bridge/chipone-icn6211.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 71c83a18984fa..b4e886c2b92a5 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -503,7 +503,8 @@ static int chipone_probe(struct mipi_dsi_device *dsi)
 
 	dsi->lanes = icn->dsi_lanes;
 	dsi->format = MIPI_DSI_FMT_RGB888;
-	dsi->mode_flags = MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
+			  MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET;
 
 	ret = mipi_dsi_attach(dsi);
 	if (ret < 0) {
-- 
2.34.1

[PATCH V3 08/13] drm: bridge: icn6211: Disable DPI color swap

[Marek Vasut]

The chip is capable of swapping DPI RGB channels. The driver currently
does not implement support for this functionality. Write the MIPI_PN_SWAP
register to 0 to assure the color swap is disabled.

Acked-by: Maxime Ripard <maxime@cerno.tech>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
V2: Rebase on next-20220214
V3: Add AB from Maxime
---
 drivers/gpu/drm/bridge/chipone-icn6211.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index b4e886c2b92a5..1a3afefcc9e80 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -302,6 +302,7 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 
 	ICN6211_DSI(icn, MIPI_PD_CK_LANE, 0xa0);
 	ICN6211_DSI(icn, PLL_CTRL(12), 0xff);
+	ICN6211_DSI(icn, MIPI_PN_SWAP, 0x00);
 
 	/* DPI HS/VS/DE polarity */
 	pol = ((mode->flags & DRM_MODE_FLAG_PHSYNC) ? BIST_POL_HSYNC_POL : 0) |
-- 
2.34.1

[PATCH V3 09/13] drm: bridge: icn6211: Set SYS_CTRL_1 to value used in examples

[Marek Vasut]

Both example code [1], [2] as well as one provided by custom panel vendor
set register SYS_CTRL_1 to 0x88. What exactly does the value mean is unknown
due to unavailable datasheet. Align this register value with example code.

[1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/gpu/drm/bridge/icn6211.c
[2] https://github.com/tdjastrzebski/ICN6211-Configurator

Acked-by: Maxime Ripard <maxime@cerno.tech>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
V2: Rebase on next-20220214
V3: Add AB from Maxime
---
 drivers/gpu/drm/bridge/chipone-icn6211.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 1a3afefcc9e80..095002a40d0e8 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -314,7 +314,7 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 	chipone_configure_pll(icn, mode);
 
 	ICN6211_DSI(icn, SYS_CTRL(0), 0x40);
-	ICN6211_DSI(icn, SYS_CTRL(1), 0x98);
+	ICN6211_DSI(icn, SYS_CTRL(1), 0x88);
 
 	/* icn6211 specific sequence */
 	ICN6211_DSI(icn, MIPI_FORCE_0, 0x20);
-- 
2.34.1

[PATCH V3 10/13] drm: bridge: icn6211: Implement atomic_get_input_bus_fmts

[Marek Vasut]

Implement .atomic_get_input_bus_fmts callback, which sets up the
input (DSI-end) format, and that format can then be used in pipeline
format negotiation between the DSI-end of this bridge and the other
component closer to the scanout engine.

Acked-by: Maxime Ripard <maxime@cerno.tech>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
V2: Rebase on next-20220214
V3: Add AB from Maxime
---
 drivers/gpu/drm/bridge/chipone-icn6211.c | 27 ++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 095002a40d0e8..afc619e215c3b 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -389,6 +389,32 @@ static int chipone_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flag
 	return drm_bridge_attach(bridge->encoder, icn->panel_bridge, bridge, flags);
 }
 
+#define MAX_INPUT_SEL_FORMATS	1
+
+static u32 *
+chipone_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
+				  struct drm_bridge_state *bridge_state,
+				  struct drm_crtc_state *crtc_state,
+				  struct drm_connector_state *conn_state,
+				  u32 output_fmt,
+				  unsigned int *num_input_fmts)
+{
+	u32 *input_fmts;
+
+	*num_input_fmts = 0;
+
+	input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),
+			     GFP_KERNEL);
+	if (!input_fmts)
+		return NULL;
+
+	/* This is the DSI-end bus format */
+	input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
+	*num_input_fmts = 1;
+
+	return input_fmts;
+}
+
 static const struct drm_bridge_funcs chipone_bridge_funcs = {
 	.atomic_duplicate_state	= drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state	= drm_atomic_helper_bridge_destroy_state,
@@ -398,6 +424,7 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
 	.atomic_post_disable	= chipone_atomic_post_disable,
 	.mode_set		= chipone_mode_set,
 	.attach			= chipone_attach,
+	.atomic_get_input_bus_fmts = chipone_atomic_get_input_bus_fmts,
 };
 
 static int chipone_parse_dt(struct chipone *icn)
-- 
2.34.1

[PATCH V3 11/13] drm: bridge: icn6211: Add I2C configuration support

[Marek Vasut]

The ICN6211 chip starts in I2C configuration mode after cold boot.
Implement support for configuring the chip via I2C in addition to
the current DSI LP command mode configuration support. The later
seems to be available only on chips which have additional MCU on
the panel/bridge board which preconfigures the ICN6211, while the
I2C configuration mode added by this patch does not require any
such MCU.

Acked-by: Maxime Ripard <maxime@cerno.tech>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
V2: - Drop the abridge variable
    - Rename chipone_dsi_setup to chipone_dsi_host_attach and call
      it from chipone_i2c_probe()
V3: Add AB from Maxime
---
 drivers/gpu/drm/bridge/chipone-icn6211.c | 183 ++++++++++++++++++++---
 1 file changed, 161 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index afc619e215c3b..4ad149c13f599 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -11,6 +11,7 @@
 
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/regulator/consumer.h>
@@ -133,15 +134,18 @@
 
 struct chipone {
 	struct device *dev;
+	struct i2c_client *client;
 	struct drm_bridge bridge;
 	struct drm_display_mode mode;
 	struct drm_bridge *panel_bridge;
 	struct device_node *host_node;
+	struct mipi_dsi_device *dsi;
 	struct gpio_desc *enable_gpio;
 	struct regulator *vdd1;
 	struct regulator *vdd2;
 	struct regulator *vdd3;
 	int dsi_lanes;
+	bool interface_i2c;
 };
 
 static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
@@ -152,9 +156,10 @@ static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
 static inline int chipone_dsi_write(struct chipone *icn,  const void *seq,
 				    size_t len)
 {
-	struct mipi_dsi_device *dsi = to_mipi_dsi_device(icn->dev);
-
-	return mipi_dsi_generic_write(dsi, seq, len);
+	if (icn->interface_i2c)
+		i2c_smbus_write_byte_data(icn->client, reg, val);
+	else
+		mipi_dsi_generic_write(icn->dsi, (u8[]){reg, val}, 2);
 }
 
 #define ICN6211_DSI(icn, seq...)				\
@@ -259,7 +264,10 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 	bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
 	bus_flags = bridge_state->output_bus_cfg.flags;
 
-	ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_DSI);
+	if (icn->interface_i2c)
+		ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_I2C);
+	else
+		ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_DSI);
 
 	ICN6211_DSI(icn, HACTIVE_LI, mode->hdisplay & 0xff);
 
@@ -380,6 +388,57 @@ static void chipone_mode_set(struct drm_bridge *bridge,
 	struct chipone *icn = bridge_to_chipone(bridge);
 
 	drm_mode_copy(&icn->mode, adjusted_mode);
+};
+
+static int chipone_dsi_attach(struct chipone *icn)
+{
+	struct mipi_dsi_device *dsi = icn->dsi;
+	int ret;
+
+	dsi->lanes = icn->dsi_lanes;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
+			  MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET;
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0)
+		dev_err(icn->dev, "failed to attach dsi\n");
+
+	return ret;
+}
+
+static int chipone_dsi_host_attach(struct chipone *icn)
+{
+	struct device *dev = icn->dev;
+	struct mipi_dsi_device *dsi;
+	struct mipi_dsi_host *host;
+	int ret = 0;
+
+	const struct mipi_dsi_device_info info = {
+		.type = "chipone",
+		.channel = 0,
+		.node = NULL,
+	};
+
+	host = of_find_mipi_dsi_host_by_node(icn->host_node);
+	if (!host) {
+		dev_err(dev, "failed to find dsi host\n");
+		return -EPROBE_DEFER;
+	}
+
+	dsi = mipi_dsi_device_register_full(host, &info);
+	if (IS_ERR(dsi)) {
+		return dev_err_probe(dev, PTR_ERR(dsi),
+				     "failed to create dsi device\n");
+	}
+
+	icn->dsi = dsi;
+
+	ret = chipone_dsi_attach(icn);
+	if (ret < 0)
+		mipi_dsi_device_unregister(dsi);
+
+	return ret;
 }
 
 static int chipone_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags)
@@ -506,9 +565,8 @@ static int chipone_parse_dt(struct chipone *icn)
 	return ret;
 }
 
-static int chipone_probe(struct mipi_dsi_device *dsi)
+static int chipone_common_probe(struct device *dev, struct chipone **icnr)
 {
-	struct device *dev = &dsi->dev;
 	struct chipone *icn;
 	int ret;
 
@@ -516,7 +574,6 @@ static int chipone_probe(struct mipi_dsi_device *dsi)
 	if (!icn)
 		return -ENOMEM;
 
-	mipi_dsi_set_drvdata(dsi, icn);
 	icn->dev = dev;
 
 	ret = chipone_parse_dt(icn);
@@ -527,29 +584,77 @@ static int chipone_probe(struct mipi_dsi_device *dsi)
 	icn->bridge.type = DRM_MODE_CONNECTOR_DPI;
 	icn->bridge.of_node = dev->of_node;
 
-	drm_bridge_add(&icn->bridge);
+	*icnr = icn;
 
-	dsi->lanes = icn->dsi_lanes;
-	dsi->format = MIPI_DSI_FMT_RGB888;
-	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
-			  MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET;
+	return ret;
+}
 
-	ret = mipi_dsi_attach(dsi);
-	if (ret < 0) {
+static int chipone_dsi_probe(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	struct chipone *icn;
+	int ret;
+
+	ret = chipone_common_probe(dev, &icn);
+	if (ret)
+		return ret;
+
+	icn->interface_i2c = false;
+	icn->dsi = dsi;
+
+	mipi_dsi_set_drvdata(dsi, icn);
+
+	drm_bridge_add(&icn->bridge);
+
+	ret = chipone_dsi_attach(icn);
+	if (ret)
 		drm_bridge_remove(&icn->bridge);
-		dev_err(dev, "failed to attach dsi\n");
-	}
 
 	return ret;
 }
 
-static int chipone_remove(struct mipi_dsi_device *dsi)
+static int chipone_i2c_probe(struct i2c_client *client,
+			     const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct chipone *icn;
+	int ret;
+
+	ret = chipone_common_probe(dev, &icn);
+	if (ret)
+		return ret;
+
+	icn->interface_i2c = true;
+	icn->client = client;
+	dev_set_drvdata(dev, icn);
+	i2c_set_clientdata(client, icn);
+
+	drm_bridge_add(&icn->bridge);
+
+	return chipone_dsi_host_attach(icn);
+}
+
+static void chipone_common_remove(struct chipone *icn)
+{
+	of_node_put(icn->host_node);
+}
+
+static int chipone_dsi_remove(struct mipi_dsi_device *dsi)
 {
 	struct chipone *icn = mipi_dsi_get_drvdata(dsi);
 
 	mipi_dsi_detach(dsi);
 	drm_bridge_remove(&icn->bridge);
-	of_node_put(icn->host_node);
+	chipone_common_remove(icn);
+
+	return 0;
+}
+
+static int chipone_i2c_remove(struct i2c_client *client)
+{
+	struct chipone *icn = i2c_get_clientdata(client);
+
+	chipone_common_remove(icn);
 
 	return 0;
 }
@@ -560,16 +665,50 @@ static const struct of_device_id chipone_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, chipone_of_match);
 
-static struct mipi_dsi_driver chipone_driver = {
-	.probe = chipone_probe,
-	.remove = chipone_remove,
+static struct mipi_dsi_driver chipone_dsi_driver = {
+	.probe = chipone_dsi_probe,
+	.remove = chipone_dsi_remove,
 	.driver = {
 		.name = "chipone-icn6211",
 		.owner = THIS_MODULE,
 		.of_match_table = chipone_of_match,
 	},
 };
-module_mipi_dsi_driver(chipone_driver);
+
+static struct i2c_device_id chipone_i2c_id[] = {
+	{ "chipone,icn6211" },
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, chipone_i2c_id);
+
+static struct i2c_driver chipone_i2c_driver = {
+	.probe = chipone_i2c_probe,
+	.remove = chipone_i2c_remove,
+	.id_table = chipone_i2c_id,
+	.driver = {
+		.name = "chipone-icn6211-i2c",
+		.owner = THIS_MODULE,
+		.of_match_table = chipone_of_match,
+	},
+};
+
+static int __init chipone_init(void)
+{
+	if (IS_ENABLED(CONFIG_DRM_MIPI_DSI))
+		mipi_dsi_driver_register(&chipone_dsi_driver);
+
+	return i2c_add_driver(&chipone_i2c_driver);
+}
+module_init(chipone_init);
+
+static void __init chipone_exit(void)
+{
+	i2c_del_driver(&chipone_i2c_driver);
+
+	if (IS_ENABLED(CONFIG_DRM_MIPI_DSI))
+		mipi_dsi_driver_unregister(&chipone_dsi_driver);
+}
+module_exit(chipone_exit);
 
 MODULE_AUTHOR("Jagan Teki <jagan@amarulasolutions.com>");
 MODULE_DESCRIPTION("Chipone ICN6211 MIPI-DSI to RGB Converter Bridge");
-- 
2.34.1

[PATCH V3 12/13] drm: bridge: icn6211: Rework ICN6211_DSI to chipone_writeb()

[Marek Vasut]

Rename and inline macro ICN6211_DSI() into function chipone_writeb()
to keep all function names lower-case. No functional change.

Acked-by: Maxime Ripard <maxime@cerno.tech>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
V2: Rebase on next-20220214
V3: Add AB from Maxime
---
 drivers/gpu/drm/bridge/chipone-icn6211.c | 63 +++++++++++-------------
 1 file changed, 28 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 4ad149c13f599..c66eacc6b1e2a 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -153,8 +153,7 @@ static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
 	return container_of(bridge, struct chipone, bridge);
 }
 
-static inline int chipone_dsi_write(struct chipone *icn,  const void *seq,
-				    size_t len)
+static void chipone_writeb(struct chipone *icn, u8 reg, u8 val)
 {
 	if (icn->interface_i2c)
 		i2c_smbus_write_byte_data(icn->client, reg, val);
@@ -162,12 +161,6 @@ static inline int chipone_dsi_write(struct chipone *icn,  const void *seq,
 		mipi_dsi_generic_write(icn->dsi, (u8[]){reg, val}, 2);
 }
 
-#define ICN6211_DSI(icn, seq...)				\
-	{							\
-		const u8 d[] = { seq };				\
-		chipone_dsi_write(icn, d, ARRAY_SIZE(d));	\
-	}
-
 static void chipone_configure_pll(struct chipone *icn,
 				  const struct drm_display_mode *mode)
 {
@@ -242,11 +235,11 @@ static void chipone_configure_pll(struct chipone *icn,
 		(fin * best_m) / BIT(best_p + best_s + 2));
 
 	/* Clock source selection fixed to MIPI DSI clock lane */
-	ICN6211_DSI(icn, PLL_CTRL(6), PLL_CTRL_6_MIPI_CLK);
-	ICN6211_DSI(icn, PLL_REF_DIV,
+	chipone_writeb(icn, PLL_CTRL(6), PLL_CTRL_6_MIPI_CLK);
+	chipone_writeb(icn, PLL_REF_DIV,
 		    (best_p ? PLL_REF_DIV_Pe : 0) | /* Prefer /2 pre-divider */
 		    PLL_REF_DIV_P(best_p) | PLL_REF_DIV_S(best_s));
-	ICN6211_DSI(icn, PLL_INT(0), best_m);
+	chipone_writeb(icn, PLL_INT(0), best_m);
 }
 
 static void chipone_atomic_enable(struct drm_bridge *bridge,
@@ -265,19 +258,19 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 	bus_flags = bridge_state->output_bus_cfg.flags;
 
 	if (icn->interface_i2c)
-		ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_I2C);
+		chipone_writeb(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_I2C);
 	else
-		ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_DSI);
+		chipone_writeb(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_DSI);
 
-	ICN6211_DSI(icn, HACTIVE_LI, mode->hdisplay & 0xff);
+	chipone_writeb(icn, HACTIVE_LI, mode->hdisplay & 0xff);
 
-	ICN6211_DSI(icn, VACTIVE_LI, mode->vdisplay & 0xff);
+	chipone_writeb(icn, VACTIVE_LI, mode->vdisplay & 0xff);
 
 	/*
 	 * lsb nibble: 2nd nibble of hdisplay
 	 * msb nibble: 2nd nibble of vdisplay
 	 */
-	ICN6211_DSI(icn, VACTIVE_HACTIVE_HI,
+	chipone_writeb(icn, VACTIVE_HACTIVE_HI,
 		    ((mode->hdisplay >> 8) & 0xf) |
 		    (((mode->vdisplay >> 8) & 0xf) << 4));
 
@@ -285,49 +278,49 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 	hsync = mode->hsync_end - mode->hsync_start;
 	hbp = mode->htotal - mode->hsync_end;
 
-	ICN6211_DSI(icn, HFP_LI, hfp & 0xff);
-	ICN6211_DSI(icn, HSYNC_LI, hsync & 0xff);
-	ICN6211_DSI(icn, HBP_LI, hbp & 0xff);
+	chipone_writeb(icn, HFP_LI, hfp & 0xff);
+	chipone_writeb(icn, HSYNC_LI, hsync & 0xff);
+	chipone_writeb(icn, HBP_LI, hbp & 0xff);
 	/* Top two bits of Horizontal Front porch/Sync/Back porch */
-	ICN6211_DSI(icn, HFP_HSW_HBP_HI,
+	chipone_writeb(icn, HFP_HSW_HBP_HI,
 		    HFP_HSW_HBP_HI_HFP(hfp) |
 		    HFP_HSW_HBP_HI_HS(hsync) |
 		    HFP_HSW_HBP_HI_HBP(hbp));
 
-	ICN6211_DSI(icn, VFP, mode->vsync_start - mode->vdisplay);
+	chipone_writeb(icn, VFP, mode->vsync_start - mode->vdisplay);
 
-	ICN6211_DSI(icn, VSYNC, mode->vsync_end - mode->vsync_start);
+	chipone_writeb(icn, VSYNC, mode->vsync_end - mode->vsync_start);
 
-	ICN6211_DSI(icn, VBP, mode->vtotal - mode->vsync_end);
+	chipone_writeb(icn, VBP, mode->vtotal - mode->vsync_end);
 
 	/* dsi specific sequence */
-	ICN6211_DSI(icn, SYNC_EVENT_DLY, 0x80);
-	ICN6211_DSI(icn, HFP_MIN, hfp & 0xff);
+	chipone_writeb(icn, SYNC_EVENT_DLY, 0x80);
+	chipone_writeb(icn, HFP_MIN, hfp & 0xff);
 
 	/* DSI data lane count */
-	ICN6211_DSI(icn, DSI_CTRL,
+	chipone_writeb(icn, DSI_CTRL,
 		    DSI_CTRL_UNKNOWN | DSI_CTRL_DSI_LANES(icn->dsi_lanes - 1));
 
-	ICN6211_DSI(icn, MIPI_PD_CK_LANE, 0xa0);
-	ICN6211_DSI(icn, PLL_CTRL(12), 0xff);
-	ICN6211_DSI(icn, MIPI_PN_SWAP, 0x00);
+	chipone_writeb(icn, MIPI_PD_CK_LANE, 0xa0);
+	chipone_writeb(icn, PLL_CTRL(12), 0xff);
+	chipone_writeb(icn, MIPI_PN_SWAP, 0x00);
 
 	/* DPI HS/VS/DE polarity */
 	pol = ((mode->flags & DRM_MODE_FLAG_PHSYNC) ? BIST_POL_HSYNC_POL : 0) |
 	      ((mode->flags & DRM_MODE_FLAG_PVSYNC) ? BIST_POL_VSYNC_POL : 0) |
 	      ((bus_flags & DRM_BUS_FLAG_DE_HIGH) ? BIST_POL_DE_POL : 0);
-	ICN6211_DSI(icn, BIST_POL, pol);
+	chipone_writeb(icn, BIST_POL, pol);
 
 	/* Configure PLL settings */
 	chipone_configure_pll(icn, mode);
 
-	ICN6211_DSI(icn, SYS_CTRL(0), 0x40);
-	ICN6211_DSI(icn, SYS_CTRL(1), 0x88);
+	chipone_writeb(icn, SYS_CTRL(0), 0x40);
+	chipone_writeb(icn, SYS_CTRL(1), 0x88);
 
 	/* icn6211 specific sequence */
-	ICN6211_DSI(icn, MIPI_FORCE_0, 0x20);
-	ICN6211_DSI(icn, PLL_CTRL(1), 0x20);
-	ICN6211_DSI(icn, CONFIG_FINISH, 0x10);
+	chipone_writeb(icn, MIPI_FORCE_0, 0x20);
+	chipone_writeb(icn, PLL_CTRL(1), 0x20);
+	chipone_writeb(icn, CONFIG_FINISH, 0x10);
 
 	usleep_range(10000, 11000);
 }
-- 
2.34.1

[PATCH V3 13/13] drm: bridge: icn6211: Read and validate chip IDs before configuration

[Marek Vasut]

Read out the Vendor/Chip/Version ID registers from the chip before
performing any configuration, and validate that the registers have
correct values. This is mostly a simple test whether DSI register
access does work, since that tends to be broken on various bridges.

Acked-by: Maxime Ripard <maxime@cerno.tech>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
V2: Rebase on next-20220214
V3: Add AB from Maxime
---
 drivers/gpu/drm/bridge/chipone-icn6211.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index c66eacc6b1e2a..0a07023d0aeec 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -153,6 +153,14 @@ static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
 	return container_of(bridge, struct chipone, bridge);
 }
 
+static void chipone_readb(struct chipone *icn, u8 reg, u8 *val)
+{
+	if (icn->interface_i2c)
+		*val = i2c_smbus_read_byte_data(icn->client, reg);
+	else
+		mipi_dsi_generic_read(icn->dsi, (u8[]){reg, 1}, 2, val, 1);
+}
+
 static void chipone_writeb(struct chipone *icn, u8 reg, u8 val)
 {
 	if (icn->interface_i2c)
@@ -251,7 +259,21 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 	const struct drm_bridge_state *bridge_state;
 	u16 hfp, hbp, hsync;
 	u32 bus_flags;
-	u8 pol;
+	u8 pol, id[4];
+
+	chipone_readb(icn, VENDOR_ID, id);
+	chipone_readb(icn, DEVICE_ID_H, id + 1);
+	chipone_readb(icn, DEVICE_ID_L, id + 2);
+	chipone_readb(icn, VERSION_ID, id + 3);
+
+	dev_dbg(icn->dev,
+		"Chip IDs: Vendor=0x%02x Device=0x%02x:0x%02x Version=0x%02x\n",
+		id[0], id[1], id[2], id[3]);
+
+	if (id[0] != 0xc1 || id[1] != 0x62 || id[2] != 0x11) {
+		dev_dbg(icn->dev, "Invalid Chip IDs, aborting configuration\n");
+		return;
+	}
 
 	/* Get the DPI flags from the bridge state. */
 	bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
-- 
2.34.1