[PATCH v4 0/3] SN65DSI83/4 lvds_vod_swing properties

[PATCH v4 0/3] SN65DSI83/4 lvds_vod_swing properties

[Andrej Picej]

Hi all,

The LVDS differential voltage swing can be specified as arrays of min, max
in microvolts. Two arrays, one for data-lanes and one for clock-lane can
be specified. Additionally, because LVDS voltage swing depends on near-end
termination this can now also be specified with separate property.

Driver goes through the tables, taken from datasheet [1] and selects the
appropriate configuration. If appropriate configuration can not be found
the probe fails. If these properties are not defined default values are
used as before.

This patch series depends on the patch
"[PATCH v2 11/15] arm64: dts: imx8mm-phyboard-polis: Add support for PEB-AV-10"
(https://lore.kernel.org/all/20241202072052.2195283-12-andrej.picej@norik.com/)
which is currently under review. Please apply the dependent series first before
applying this one.

v1 is at: https://lore.kernel.org/all/20241127103031.1007893-1-andrej.picej@norik.com/
v2 is at: https://lore.kernel.org/all/20241203085822.2475138-1-andrej.picej@norik.com/
v3 is at: https://lore.kernel.org/all/20241203110054.2506123-1-andrej.picej@norik.com/

[1] https://www.ti.com/lit/ds/symlink/sn65dsi83.pdf?ts=1732738773429&ref_url=https%253A%252F%252Fwww.mouser.co.uk%252F

Best regards,
Andrej

Andrej Picej (3):
  dt-bindings: drm/bridge: ti-sn65dsi83: Add properties for
    ti,lvds-vod-swing
  drm/bridge: ti-sn65dsi83: Add ti,lvds-vod-swing optional properties
  arm64: dts: imx8mm-phyboard-polis-peb-av-10: Set lvds-vod-swing

 .../bindings/display/bridge/ti,sn65dsi83.yaml |  34 +++-
 .../imx8mm-phyboard-polis-peb-av-10.dtso      |   2 +
 drivers/gpu/drm/bridge/ti-sn65dsi83.c         | 147 +++++++++++++++++-
 3 files changed, 178 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH v4 1/3] dt-bindings: drm/bridge: ti-sn65dsi83: Add properties for ti,lvds-vod-swing

[Andrej Picej]

Add properties which can be used to specify LVDS differential output
voltage. Since this also depends on near-end signal termination also
include property which sets this. LVDS differential output voltage is
specified with an array (min, max), which should match the one from
connected device.

Signed-off-by: Andrej Picej <andrej.picej@norik.com>
---
Changes in v4:
- removed "additionalProperties: true" from the patch as it is not needed
Changes in v3:
- no change
Changes in v2:
- move LVDS port schema to a $defs and reference it from there
- properties are now defined in microvolts/ohms
- use 1 property for data-lane and 1 for clock-lane LVDS voltage swing
- add 1 property which sets LVDS near-end termination
- since major change was done change the authorship to myself
---
 .../bindings/display/bridge/ti,sn65dsi83.yaml | 34 +++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
index 48a97bb3e2e0..bad6f5c81b06 100644
--- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
@@ -80,12 +80,12 @@ properties:
                   - const: 4
 
       port@2:
-        $ref: /schemas/graph.yaml#/properties/port
         description: Video port for LVDS Channel-A output (panel or bridge).
+        $ref: '#/$defs/lvds-port'
 
       port@3:
-        $ref: /schemas/graph.yaml#/properties/port
         description: Video port for LVDS Channel-B output (panel or bridge).
+        $ref: '#/$defs/lvds-port'
 
     required:
       - port@0
@@ -96,6 +96,36 @@ required:
   - reg
   - ports
 
+$defs:
+  lvds-port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    unevaluatedProperties: false
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          ti,lvds-termination-ohms:
+            description: The value of near end differential termination in ohms.
+            enum: [100, 200]
+            default: 200
+
+          ti,lvds-vod-swing-clock-microvolt:
+            description: LVDS diferential output voltage <min max> for clock
+              lanes in microvolts.
+            $ref: /schemas/types.yaml#/definitions/uint32-array
+            minItems: 2
+            maxItems: 2
+
+          ti,lvds-vod-swing-data-microvolt:
+            description: LVDS diferential output voltage <min max> for data
+              lanes in microvolts.
+            $ref: /schemas/types.yaml#/definitions/uint32-array
+            minItems: 2
+            maxItems: 2
+
 allOf:
   - if:
       properties:
-- 
2.34.1

[PATCH v4 2/3] drm/bridge: ti-sn65dsi83: Add ti,lvds-vod-swing optional properties

[Andrej Picej]

Add a optional properties to change LVDS output voltage. This should not
be static as this depends mainly on the connected display voltage
requirement. We have three properties:
- "ti,lvds-termination-ohms", which sets near end termination,
- "ti,lvds-vod-swing-data-microvolt" and
- "ti,lvds-vod-swing-clock-microvolt" which both set LVDS differential
output voltage for data and clock lanes. They are defined as an array
with min and max values. The appropriate bitfield will be set if
selected constraints can be met.

If "ti,lvds-termination-ohms" is not defined the default of 200 Ohm near
end termination will be used. Selecting only one:
"ti,lvds-vod-swing-data-microvolt" or
"ti,lvds-vod-swing-clock-microvolt" can be done, but the output voltage
constraint for only data/clock lanes will be met. Setting both is
recommended.

Signed-off-by: Andrej Picej <andrej.picej@norik.com>
---
Changes in v4:
- fix typo in commit message bitfiled -> bitfield
- use arrays (lvds_vod_swing_conf and lvds_term_conf) in private data, instead
of separate variables for channel A/B
- add more checks on return value of "of_property_read_u32_array"
Changes in v3:
- use microvolts for default array values 1000 mV -> 1000000 uV.
Changes in v2:
- use datasheet tables to get the proper configuration
- since major change was done change the authorship to myself
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 147 +++++++++++++++++++++++++-
 1 file changed, 144 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 57a7ed13f996..f724d2a6777b 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -132,6 +132,16 @@
 #define  REG_IRQ_STAT_CHA_SOT_BIT_ERR		BIT(2)
 #define  REG_IRQ_STAT_CHA_PLL_UNLOCK		BIT(0)
 
+enum sn65dsi83_channel {
+	CHANNEL_A,
+	CHANNEL_B
+};
+
+enum sn65dsi83_lvds_term {
+	OHM_100,
+	OHM_200
+};
+
 enum sn65dsi83_model {
 	MODEL_SN65DSI83,
 	MODEL_SN65DSI84,
@@ -147,6 +157,8 @@ struct sn65dsi83 {
 	struct regulator		*vcc;
 	bool				lvds_dual_link;
 	bool				lvds_dual_link_even_odd_swap;
+	int				lvds_vod_swing_conf[2];
+	int				lvds_term_conf[2];
 };
 
 static const struct regmap_range sn65dsi83_readable_ranges[] = {
@@ -237,6 +249,36 @@ static const struct regmap_config sn65dsi83_regmap_config = {
 	.max_register = REG_IRQ_STAT,
 };
 
+static const int lvds_vod_swing_data_table[2][4][2] = {
+	{	/* 100 Ohm */
+		{ 180000, 313000 },
+		{ 215000, 372000 },
+		{ 250000, 430000 },
+		{ 290000, 488000 },
+	},
+	{	/* 200 Ohm */
+		{ 150000, 261000 },
+		{ 200000, 346000 },
+		{ 250000, 428000 },
+		{ 300000, 511000 },
+	},
+};
+
+static const int lvds_vod_swing_clock_table[2][4][2] = {
+	{	/* 100 Ohm */
+		{ 140000, 244000 },
+		{ 168000, 290000 },
+		{ 195000, 335000 },
+		{ 226000, 381000 },
+	},
+	{	/* 200 Ohm */
+		{ 117000, 204000 },
+		{ 156000, 270000 },
+		{ 195000, 334000 },
+		{ 234000, 399000 },
+	},
+};
+
 static struct sn65dsi83 *bridge_to_sn65dsi83(struct drm_bridge *bridge)
 {
 	return container_of(bridge, struct sn65dsi83, bridge);
@@ -435,12 +477,16 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 		val |= REG_LVDS_FMT_LVDS_LINK_CFG;
 
 	regmap_write(ctx->regmap, REG_LVDS_FMT, val);
-	regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x05);
+	regmap_write(ctx->regmap, REG_LVDS_VCOM,
+			REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_A]) |
+			REG_LVDS_VCOM_CHB_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_B]));
 	regmap_write(ctx->regmap, REG_LVDS_LANE,
 		     (ctx->lvds_dual_link_even_odd_swap ?
 		      REG_LVDS_LANE_EVEN_ODD_SWAP : 0) |
-		     REG_LVDS_LANE_CHA_LVDS_TERM |
-		     REG_LVDS_LANE_CHB_LVDS_TERM);
+		     (ctx->lvds_term_conf[CHANNEL_A] ?
+			  REG_LVDS_LANE_CHA_LVDS_TERM : 0) |
+		     (ctx->lvds_term_conf[CHANNEL_B] ?
+			  REG_LVDS_LANE_CHB_LVDS_TERM : 0));
 	regmap_write(ctx->regmap, REG_LVDS_CM, 0x00);
 
 	le16val = cpu_to_le16(mode->hdisplay);
@@ -576,10 +622,101 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = {
 	.atomic_get_input_bus_fmts = sn65dsi83_atomic_get_input_bus_fmts,
 };
 
+static int sn65dsi83_select_lvds_vod_swing(struct device *dev,
+	u32 lvds_vod_swing_data[2], u32 lvds_vod_swing_clk[2], u8 lvds_term)
+{
+	int i;
+
+	for (i = 0; i <= 3; i++) {
+		if (lvds_vod_swing_data_table[lvds_term][i][0] >= lvds_vod_swing_data[0] &&
+		lvds_vod_swing_data_table[lvds_term][i][1] <= lvds_vod_swing_data[1] &&
+		lvds_vod_swing_clock_table[lvds_term][i][0] >= lvds_vod_swing_clk[0] &&
+		lvds_vod_swing_clock_table[lvds_term][i][1] <= lvds_vod_swing_clk[1])
+			return i;
+	}
+
+	dev_err(dev, "failed to find appropriate LVDS_VOD_SWING configuration\n");
+	return -EINVAL;
+}
+
+static int sn65dsi83_parse_lvds_endpoint(struct sn65dsi83 *ctx, int channel)
+{
+	struct device *dev = ctx->dev;
+	struct device_node *endpoint;
+	/* Set so the property can be freely selected if not defined */
+	u32 lvds_vod_swing_data[2] = { 0, 1000000 };
+	u32 lvds_vod_swing_clk[2] = { 0, 1000000 };
+	u32 lvds_term = 200;
+	u8 lvds_term_conf;
+	int endpoint_reg;
+	int lvds_vod_swing_conf;
+	int ret = 0;
+	int ret_data;
+	int ret_clock;
+
+	if (channel == CHANNEL_A)
+		endpoint_reg = 2;
+	else
+		endpoint_reg = 3;
+
+	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, endpoint_reg, -1);
+	of_property_read_u32(endpoint, "ti,lvds-termination-ohms", &lvds_term);
+
+	if (lvds_term == 200)
+		lvds_term_conf = OHM_200;
+	else
+		lvds_term_conf = OHM_100;
+
+	ctx->lvds_term_conf[channel] = lvds_term_conf;
+
+	ret_data = of_property_read_u32_array(endpoint,
+			"ti,lvds-vod-swing-data-microvolt", lvds_vod_swing_data,
+			ARRAY_SIZE(lvds_vod_swing_data));
+	if (ret_data != 0 && ret_data != -EINVAL) {
+		ret = ret_data;
+		goto exit;
+	}
+
+	ret_clock = of_property_read_u32_array(endpoint,
+			"ti,lvds-vod-swing-clock-microvolt", lvds_vod_swing_clk,
+			ARRAY_SIZE(lvds_vod_swing_clk));
+	if (ret_clock != 0 && ret_clock != -EINVAL) {
+		ret = ret_clock;
+		goto exit;
+	}
+
+	/* If any of the two properties is defined. */
+	if (!ret_data || !ret_clock) {
+		lvds_vod_swing_conf = sn65dsi83_select_lvds_vod_swing(dev,
+			lvds_vod_swing_data, lvds_vod_swing_clk,
+			lvds_term_conf);
+		if (lvds_vod_swing_conf < 0) {
+			ret = lvds_vod_swing_conf;
+			goto exit;
+		}
+
+		ctx->lvds_vod_swing_conf[channel] = lvds_vod_swing_conf;
+	}
+	ret = 0;
+exit:
+	of_node_put(endpoint);
+	return ret;
+}
+
 static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
 {
 	struct drm_bridge *panel_bridge;
 	struct device *dev = ctx->dev;
+	int ret;
+
+	ctx->lvds_vod_swing_conf[CHANNEL_A] = 0x1;
+	ctx->lvds_vod_swing_conf[CHANNEL_B] = 0x1;
+	ctx->lvds_term_conf[CHANNEL_A] = 0x1;
+	ctx->lvds_term_conf[CHANNEL_B] = 0x1;
+
+	ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_A);
+	if (ret < 0)
+		return ret;
 
 	ctx->lvds_dual_link = false;
 	ctx->lvds_dual_link_even_odd_swap = false;
@@ -587,6 +724,10 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
 		struct device_node *port2, *port3;
 		int dual_link;
 
+		ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_B);
+		if (ret < 0)
+			return ret;
+
 		port2 = of_graph_get_port_by_id(dev->of_node, 2);
 		port3 = of_graph_get_port_by_id(dev->of_node, 3);
 		dual_link = drm_of_lvds_get_dual_link_pixel_order(port2, port3);
-- 
2.34.1

[PATCH v4 3/3] arm64: dts: imx8mm-phyboard-polis-peb-av-10: Set lvds-vod-swing

[Andrej Picej]

Set custom differential output voltage for LVDS, to fulfill requirements
of the connected display. LVDS differential voltage for data-lanes and
clock output has to be between 200 mV and 600 mV.
Driver sets 200 Ohm near-end termination by default.

Signed-off-by: Andrej Picej <andrej.picej@norik.com>
---
Changes in v4:
- no change
Changes in v3:
- no change
Changes in v2:
- use new properties from previous patches
---
 .../boot/dts/freescale/imx8mm-phyboard-polis-peb-av-10.dtso     | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm-phyboard-polis-peb-av-10.dtso b/arch/arm64/boot/dts/freescale/imx8mm-phyboard-polis-peb-av-10.dtso
index a9de42cf14be..8bf9cc553bea 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-phyboard-polis-peb-av-10.dtso
+++ b/arch/arm64/boot/dts/freescale/imx8mm-phyboard-polis-peb-av-10.dtso
@@ -186,6 +186,8 @@ port@2 {
 			reg = <2>;
 			bridge_out: endpoint {
 				remote-endpoint = <&panel_in>;
+				ti,lvds-vod-swing-clock-microvolt = <200000 600000>;
+				ti,lvds-vod-swing-data-microvolt = <200000 600000>;
 			};
 		};
 	};
-- 
2.34.1

Re: [PATCH v4 2/3] drm/bridge: ti-sn65dsi83: Add ti,lvds-vod-swing optional properties

[Dmitry Baryshkov]

On Thu, Dec 05, 2024 at 02:40:20PM +0100, Andrej Picej wrote:
> Add a optional properties to change LVDS output voltage. This should not
> be static as this depends mainly on the connected display voltage
> requirement. We have three properties:
> - "ti,lvds-termination-ohms", which sets near end termination,
> - "ti,lvds-vod-swing-data-microvolt" and
> - "ti,lvds-vod-swing-clock-microvolt" which both set LVDS differential
> output voltage for data and clock lanes. They are defined as an array
> with min and max values. The appropriate bitfield will be set if
> selected constraints can be met.
> 
> If "ti,lvds-termination-ohms" is not defined the default of 200 Ohm near
> end termination will be used. Selecting only one:
> "ti,lvds-vod-swing-data-microvolt" or
> "ti,lvds-vod-swing-clock-microvolt" can be done, but the output voltage
> constraint for only data/clock lanes will be met. Setting both is
> recommended.
> 
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> ---
> Changes in v4:
> - fix typo in commit message bitfiled -> bitfield
> - use arrays (lvds_vod_swing_conf and lvds_term_conf) in private data, instead
> of separate variables for channel A/B
> - add more checks on return value of "of_property_read_u32_array"
> Changes in v3:
> - use microvolts for default array values 1000 mV -> 1000000 uV.
> Changes in v2:
> - use datasheet tables to get the proper configuration
> - since major change was done change the authorship to myself
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi83.c | 147 +++++++++++++++++++++++++-
>  1 file changed, 144 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index 57a7ed13f996..f724d2a6777b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -132,6 +132,16 @@
>  #define  REG_IRQ_STAT_CHA_SOT_BIT_ERR		BIT(2)
>  #define  REG_IRQ_STAT_CHA_PLL_UNLOCK		BIT(0)
>  
> +enum sn65dsi83_channel {
> +	CHANNEL_A,
> +	CHANNEL_B
> +};
> +
> +enum sn65dsi83_lvds_term {
> +	OHM_100,
> +	OHM_200
> +};
> +
>  enum sn65dsi83_model {
>  	MODEL_SN65DSI83,
>  	MODEL_SN65DSI84,
> @@ -147,6 +157,8 @@ struct sn65dsi83 {
>  	struct regulator		*vcc;
>  	bool				lvds_dual_link;
>  	bool				lvds_dual_link_even_odd_swap;
> +	int				lvds_vod_swing_conf[2];
> +	int				lvds_term_conf[2];
>  };
>  
>  static const struct regmap_range sn65dsi83_readable_ranges[] = {
> @@ -237,6 +249,36 @@ static const struct regmap_config sn65dsi83_regmap_config = {
>  	.max_register = REG_IRQ_STAT,
>  };
>  
> +static const int lvds_vod_swing_data_table[2][4][2] = {
> +	{	/* 100 Ohm */
> +		{ 180000, 313000 },
> +		{ 215000, 372000 },
> +		{ 250000, 430000 },
> +		{ 290000, 488000 },
> +	},
> +	{	/* 200 Ohm */
> +		{ 150000, 261000 },
> +		{ 200000, 346000 },
> +		{ 250000, 428000 },
> +		{ 300000, 511000 },
> +	},
> +};
> +
> +static const int lvds_vod_swing_clock_table[2][4][2] = {
> +	{	/* 100 Ohm */
> +		{ 140000, 244000 },
> +		{ 168000, 290000 },
> +		{ 195000, 335000 },
> +		{ 226000, 381000 },
> +	},
> +	{	/* 200 Ohm */
> +		{ 117000, 204000 },
> +		{ 156000, 270000 },
> +		{ 195000, 334000 },
> +		{ 234000, 399000 },
> +	},
> +};
> +
>  static struct sn65dsi83 *bridge_to_sn65dsi83(struct drm_bridge *bridge)
>  {
>  	return container_of(bridge, struct sn65dsi83, bridge);
> @@ -435,12 +477,16 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
>  		val |= REG_LVDS_FMT_LVDS_LINK_CFG;
>  
>  	regmap_write(ctx->regmap, REG_LVDS_FMT, val);
> -	regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x05);
> +	regmap_write(ctx->regmap, REG_LVDS_VCOM,
> +			REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_A]) |
> +			REG_LVDS_VCOM_CHB_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_B]));
>  	regmap_write(ctx->regmap, REG_LVDS_LANE,
>  		     (ctx->lvds_dual_link_even_odd_swap ?
>  		      REG_LVDS_LANE_EVEN_ODD_SWAP : 0) |
> -		     REG_LVDS_LANE_CHA_LVDS_TERM |
> -		     REG_LVDS_LANE_CHB_LVDS_TERM);
> +		     (ctx->lvds_term_conf[CHANNEL_A] ?
> +			  REG_LVDS_LANE_CHA_LVDS_TERM : 0) |
> +		     (ctx->lvds_term_conf[CHANNEL_B] ?
> +			  REG_LVDS_LANE_CHB_LVDS_TERM : 0));
>  	regmap_write(ctx->regmap, REG_LVDS_CM, 0x00);
>  
>  	le16val = cpu_to_le16(mode->hdisplay);
> @@ -576,10 +622,101 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = {
>  	.atomic_get_input_bus_fmts = sn65dsi83_atomic_get_input_bus_fmts,
>  };
>  
> +static int sn65dsi83_select_lvds_vod_swing(struct device *dev,
> +	u32 lvds_vod_swing_data[2], u32 lvds_vod_swing_clk[2], u8 lvds_term)
> +{
> +	int i;
> +
> +	for (i = 0; i <= 3; i++) {
> +		if (lvds_vod_swing_data_table[lvds_term][i][0] >= lvds_vod_swing_data[0] &&
> +		lvds_vod_swing_data_table[lvds_term][i][1] <= lvds_vod_swing_data[1] &&
> +		lvds_vod_swing_clock_table[lvds_term][i][0] >= lvds_vod_swing_clk[0] &&
> +		lvds_vod_swing_clock_table[lvds_term][i][1] <= lvds_vod_swing_clk[1])
> +			return i;
> +	}
> +
> +	dev_err(dev, "failed to find appropriate LVDS_VOD_SWING configuration\n");
> +	return -EINVAL;
> +}
> +
> +static int sn65dsi83_parse_lvds_endpoint(struct sn65dsi83 *ctx, int channel)
> +{
> +	struct device *dev = ctx->dev;
> +	struct device_node *endpoint;
> +	/* Set so the property can be freely selected if not defined */
> +	u32 lvds_vod_swing_data[2] = { 0, 1000000 };
> +	u32 lvds_vod_swing_clk[2] = { 0, 1000000 };
> +	u32 lvds_term = 200;
> +	u8 lvds_term_conf;
> +	int endpoint_reg;
> +	int lvds_vod_swing_conf;
> +	int ret = 0;
> +	int ret_data;
> +	int ret_clock;
> +
> +	if (channel == CHANNEL_A)
> +		endpoint_reg = 2;
> +	else
> +		endpoint_reg = 3;
> +
> +	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, endpoint_reg, -1);
> +	of_property_read_u32(endpoint, "ti,lvds-termination-ohms", &lvds_term);
> +
> +	if (lvds_term == 200)
> +		lvds_term_conf = OHM_200;
> +	else
> +		lvds_term_conf = OHM_100;
> +
> +	ctx->lvds_term_conf[channel] = lvds_term_conf;
> +
> +	ret_data = of_property_read_u32_array(endpoint,
> +			"ti,lvds-vod-swing-data-microvolt", lvds_vod_swing_data,
> +			ARRAY_SIZE(lvds_vod_swing_data));
> +	if (ret_data != 0 && ret_data != -EINVAL) {
> +		ret = ret_data;
> +		goto exit;
> +	}
> +
> +	ret_clock = of_property_read_u32_array(endpoint,
> +			"ti,lvds-vod-swing-clock-microvolt", lvds_vod_swing_clk,
> +			ARRAY_SIZE(lvds_vod_swing_clk));
> +	if (ret_clock != 0 && ret_clock != -EINVAL) {
> +		ret = ret_clock;
> +		goto exit;
> +	}
> +
> +	/* If any of the two properties is defined. */
> +	if (!ret_data || !ret_clock) {
> +		lvds_vod_swing_conf = sn65dsi83_select_lvds_vod_swing(dev,
> +			lvds_vod_swing_data, lvds_vod_swing_clk,
> +			lvds_term_conf);
> +		if (lvds_vod_swing_conf < 0) {
> +			ret = lvds_vod_swing_conf;
> +			goto exit;
> +		}
> +
> +		ctx->lvds_vod_swing_conf[channel] = lvds_vod_swing_conf;
> +	}
> +	ret = 0;
> +exit:
> +	of_node_put(endpoint);
> +	return ret;
> +}
> +
>  static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
>  {
>  	struct drm_bridge *panel_bridge;
>  	struct device *dev = ctx->dev;
> +	int ret;
> +
> +	ctx->lvds_vod_swing_conf[CHANNEL_A] = 0x1;
> +	ctx->lvds_vod_swing_conf[CHANNEL_B] = 0x1;
> +	ctx->lvds_term_conf[CHANNEL_A] = 0x1;
> +	ctx->lvds_term_conf[CHANNEL_B] = 0x1;

These match the defaults in sn65dsi83_parse_lvds_endpoint(). Do we
really need those?

> +
> +	ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_A);
> +	if (ret < 0)
> +		return ret;
>  
>  	ctx->lvds_dual_link = false;
>  	ctx->lvds_dual_link_even_odd_swap = false;
> @@ -587,6 +724,10 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
>  		struct device_node *port2, *port3;
>  		int dual_link;
>  
> +		ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_B);
> +		if (ret < 0)
> +			return ret;
> +
>  		port2 = of_graph_get_port_by_id(dev->of_node, 2);
>  		port3 = of_graph_get_port_by_id(dev->of_node, 3);
>  		dual_link = drm_of_lvds_get_dual_link_pixel_order(port2, port3);
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v4 1/3] dt-bindings: drm/bridge: ti-sn65dsi83: Add properties for ti,lvds-vod-swing

[Krzysztof Kozlowski]

On Thu, Dec 05, 2024 at 02:40:19PM +0100, Andrej Picej wrote:
> Add properties which can be used to specify LVDS differential output
> voltage. Since this also depends on near-end signal termination also
> include property which sets this. LVDS differential output voltage is
> specified with an array (min, max), which should match the one from
> connected device.
> 
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v4 2/3] drm/bridge: ti-sn65dsi83: Add ti,lvds-vod-swing optional properties

[Andrej Picej]

Hi Dmitry,

On 5. 12. 24 23:48, Dmitry Baryshkov wrote:
> On Thu, Dec 05, 2024 at 02:40:20PM +0100, Andrej Picej wrote:
>> Add a optional properties to change LVDS output voltage. This should not
>> be static as this depends mainly on the connected display voltage
>> requirement. We have three properties:
>> - "ti,lvds-termination-ohms", which sets near end termination,
>> - "ti,lvds-vod-swing-data-microvolt" and
>> - "ti,lvds-vod-swing-clock-microvolt" which both set LVDS differential
>> output voltage for data and clock lanes. They are defined as an array
>> with min and max values. The appropriate bitfield will be set if
>> selected constraints can be met.
>>
>> If "ti,lvds-termination-ohms" is not defined the default of 200 Ohm near
>> end termination will be used. Selecting only one:
>> "ti,lvds-vod-swing-data-microvolt" or
>> "ti,lvds-vod-swing-clock-microvolt" can be done, but the output voltage
>> constraint for only data/clock lanes will be met. Setting both is
>> recommended.
>>
>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>> ---
>> Changes in v4:
>> - fix typo in commit message bitfiled -> bitfield
>> - use arrays (lvds_vod_swing_conf and lvds_term_conf) in private data, instead
>> of separate variables for channel A/B
>> - add more checks on return value of "of_property_read_u32_array"
>> Changes in v3:
>> - use microvolts for default array values 1000 mV -> 1000000 uV.
>> Changes in v2:
>> - use datasheet tables to get the proper configuration
>> - since major change was done change the authorship to myself
>> ---
>>   drivers/gpu/drm/bridge/ti-sn65dsi83.c | 147 +++++++++++++++++++++++++-
>>   1 file changed, 144 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> index 57a7ed13f996..f724d2a6777b 100644
>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> @@ -132,6 +132,16 @@
>>   #define  REG_IRQ_STAT_CHA_SOT_BIT_ERR		BIT(2)
>>   #define  REG_IRQ_STAT_CHA_PLL_UNLOCK		BIT(0)
>>   
>> +enum sn65dsi83_channel {
>> +	CHANNEL_A,
>> +	CHANNEL_B
>> +};
>> +
>> +enum sn65dsi83_lvds_term {
>> +	OHM_100,
>> +	OHM_200
>> +};
>> +
>>   enum sn65dsi83_model {
>>   	MODEL_SN65DSI83,
>>   	MODEL_SN65DSI84,
>> @@ -147,6 +157,8 @@ struct sn65dsi83 {
>>   	struct regulator		*vcc;
>>   	bool				lvds_dual_link;
>>   	bool				lvds_dual_link_even_odd_swap;
>> +	int				lvds_vod_swing_conf[2];
>> +	int				lvds_term_conf[2];
>>   };
>>   
>>   static const struct regmap_range sn65dsi83_readable_ranges[] = {
>> @@ -237,6 +249,36 @@ static const struct regmap_config sn65dsi83_regmap_config = {
>>   	.max_register = REG_IRQ_STAT,
>>   };
>>   
>> +static const int lvds_vod_swing_data_table[2][4][2] = {
>> +	{	/* 100 Ohm */
>> +		{ 180000, 313000 },
>> +		{ 215000, 372000 },
>> +		{ 250000, 430000 },
>> +		{ 290000, 488000 },
>> +	},
>> +	{	/* 200 Ohm */
>> +		{ 150000, 261000 },
>> +		{ 200000, 346000 },
>> +		{ 250000, 428000 },
>> +		{ 300000, 511000 },
>> +	},
>> +};
>> +
>> +static const int lvds_vod_swing_clock_table[2][4][2] = {
>> +	{	/* 100 Ohm */
>> +		{ 140000, 244000 },
>> +		{ 168000, 290000 },
>> +		{ 195000, 335000 },
>> +		{ 226000, 381000 },
>> +	},
>> +	{	/* 200 Ohm */
>> +		{ 117000, 204000 },
>> +		{ 156000, 270000 },
>> +		{ 195000, 334000 },
>> +		{ 234000, 399000 },
>> +	},
>> +};
>> +
>>   static struct sn65dsi83 *bridge_to_sn65dsi83(struct drm_bridge *bridge)
>>   {
>>   	return container_of(bridge, struct sn65dsi83, bridge);
>> @@ -435,12 +477,16 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
>>   		val |= REG_LVDS_FMT_LVDS_LINK_CFG;
>>   
>>   	regmap_write(ctx->regmap, REG_LVDS_FMT, val);
>> -	regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x05);
>> +	regmap_write(ctx->regmap, REG_LVDS_VCOM,
>> +			REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_A]) |
>> +			REG_LVDS_VCOM_CHB_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_B]));
>>   	regmap_write(ctx->regmap, REG_LVDS_LANE,
>>   		     (ctx->lvds_dual_link_even_odd_swap ?
>>   		      REG_LVDS_LANE_EVEN_ODD_SWAP : 0) |
>> -		     REG_LVDS_LANE_CHA_LVDS_TERM |
>> -		     REG_LVDS_LANE_CHB_LVDS_TERM);
>> +		     (ctx->lvds_term_conf[CHANNEL_A] ?
>> +			  REG_LVDS_LANE_CHA_LVDS_TERM : 0) |
>> +		     (ctx->lvds_term_conf[CHANNEL_B] ?
>> +			  REG_LVDS_LANE_CHB_LVDS_TERM : 0));
>>   	regmap_write(ctx->regmap, REG_LVDS_CM, 0x00);
>>   
>>   	le16val = cpu_to_le16(mode->hdisplay);
>> @@ -576,10 +622,101 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = {
>>   	.atomic_get_input_bus_fmts = sn65dsi83_atomic_get_input_bus_fmts,
>>   };
>>   
>> +static int sn65dsi83_select_lvds_vod_swing(struct device *dev,
>> +	u32 lvds_vod_swing_data[2], u32 lvds_vod_swing_clk[2], u8 lvds_term)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i <= 3; i++) {
>> +		if (lvds_vod_swing_data_table[lvds_term][i][0] >= lvds_vod_swing_data[0] &&
>> +		lvds_vod_swing_data_table[lvds_term][i][1] <= lvds_vod_swing_data[1] &&
>> +		lvds_vod_swing_clock_table[lvds_term][i][0] >= lvds_vod_swing_clk[0] &&
>> +		lvds_vod_swing_clock_table[lvds_term][i][1] <= lvds_vod_swing_clk[1])
>> +			return i;
>> +	}
>> +
>> +	dev_err(dev, "failed to find appropriate LVDS_VOD_SWING configuration\n");
>> +	return -EINVAL;
>> +}
>> +
>> +static int sn65dsi83_parse_lvds_endpoint(struct sn65dsi83 *ctx, int channel)
>> +{
>> +	struct device *dev = ctx->dev;
>> +	struct device_node *endpoint;
>> +	/* Set so the property can be freely selected if not defined */
>> +	u32 lvds_vod_swing_data[2] = { 0, 1000000 };
>> +	u32 lvds_vod_swing_clk[2] = { 0, 1000000 };
>> +	u32 lvds_term = 200;
>> +	u8 lvds_term_conf;
>> +	int endpoint_reg;
>> +	int lvds_vod_swing_conf;
>> +	int ret = 0;
>> +	int ret_data;
>> +	int ret_clock;
>> +
>> +	if (channel == CHANNEL_A)
>> +		endpoint_reg = 2;
>> +	else
>> +		endpoint_reg = 3;
>> +
>> +	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, endpoint_reg, -1);
>> +	of_property_read_u32(endpoint, "ti,lvds-termination-ohms", &lvds_term);
>> +
>> +	if (lvds_term == 200)
>> +		lvds_term_conf = OHM_200;
>> +	else
>> +		lvds_term_conf = OHM_100;
>> +
>> +	ctx->lvds_term_conf[channel] = lvds_term_conf;
>> +
>> +	ret_data = of_property_read_u32_array(endpoint,
>> +			"ti,lvds-vod-swing-data-microvolt", lvds_vod_swing_data,
>> +			ARRAY_SIZE(lvds_vod_swing_data));
>> +	if (ret_data != 0 && ret_data != -EINVAL) {
>> +		ret = ret_data;
>> +		goto exit;
>> +	}
>> +
>> +	ret_clock = of_property_read_u32_array(endpoint,
>> +			"ti,lvds-vod-swing-clock-microvolt", lvds_vod_swing_clk,
>> +			ARRAY_SIZE(lvds_vod_swing_clk));
>> +	if (ret_clock != 0 && ret_clock != -EINVAL) {
>> +		ret = ret_clock;
>> +		goto exit;
>> +	}
>> +
>> +	/* If any of the two properties is defined. */
>> +	if (!ret_data || !ret_clock) {
>> +		lvds_vod_swing_conf = sn65dsi83_select_lvds_vod_swing(dev,
>> +			lvds_vod_swing_data, lvds_vod_swing_clk,
>> +			lvds_term_conf);
>> +		if (lvds_vod_swing_conf < 0) {
>> +			ret = lvds_vod_swing_conf;
>> +			goto exit;
>> +		}
>> +
>> +		ctx->lvds_vod_swing_conf[channel] = lvds_vod_swing_conf;
>> +	}
>> +	ret = 0;
>> +exit:
>> +	of_node_put(endpoint);
>> +	return ret;
>> +}
>> +
>>   static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
>>   {
>>   	struct drm_bridge *panel_bridge;
>>   	struct device *dev = ctx->dev;
>> +	int ret;
>> +
>> +	ctx->lvds_vod_swing_conf[CHANNEL_A] = 0x1;
>> +	ctx->lvds_vod_swing_conf[CHANNEL_B] = 0x1;
>> +	ctx->lvds_term_conf[CHANNEL_A] = 0x1;
>> +	ctx->lvds_term_conf[CHANNEL_B] = 0x1;
> 
> These match the defaults in sn65dsi83_parse_lvds_endpoint(). Do we
> really need those?

Yes, I think we do. This ensures that defaults are used even when 
property is not defined/LVDS channel is not used. So also LVDS channel B 
defaults are set even for sn65dsi83 (single LVDS output). Keeping the 
same reg values as before these changes.

Best regards,
Andrej


> 
>> +
>> +	ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_A);
>> +	if (ret < 0)
>> +		return ret;
>>   
>>   	ctx->lvds_dual_link = false;
>>   	ctx->lvds_dual_link_even_odd_swap = false;
>> @@ -587,6 +724,10 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
>>   		struct device_node *port2, *port3;
>>   		int dual_link;
>>   
>> +		ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_B);
>> +		if (ret < 0)
>> +			return ret;
>> +
>>   		port2 = of_graph_get_port_by_id(dev->of_node, 2);
>>   		port3 = of_graph_get_port_by_id(dev->of_node, 3);
>>   		dual_link = drm_of_lvds_get_dual_link_pixel_order(port2, port3);
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH v4 2/3] drm/bridge: ti-sn65dsi83: Add ti,lvds-vod-swing optional properties

[Dmitry Baryshkov]

On Mon, Dec 09, 2024 at 08:56:29AM +0100, Andrej Picej wrote:
> Hi Dmitry,
> 
> On 5. 12. 24 23:48, Dmitry Baryshkov wrote:
> > On Thu, Dec 05, 2024 at 02:40:20PM +0100, Andrej Picej wrote:
> > > Add a optional properties to change LVDS output voltage. This should not
> > > be static as this depends mainly on the connected display voltage
> > > requirement. We have three properties:
> > > - "ti,lvds-termination-ohms", which sets near end termination,
> > > - "ti,lvds-vod-swing-data-microvolt" and
> > > - "ti,lvds-vod-swing-clock-microvolt" which both set LVDS differential
> > > output voltage for data and clock lanes. They are defined as an array
> > > with min and max values. The appropriate bitfield will be set if
> > > selected constraints can be met.
> > > 
> > > If "ti,lvds-termination-ohms" is not defined the default of 200 Ohm near
> > > end termination will be used. Selecting only one:
> > > "ti,lvds-vod-swing-data-microvolt" or
> > > "ti,lvds-vod-swing-clock-microvolt" can be done, but the output voltage
> > > constraint for only data/clock lanes will be met. Setting both is
> > > recommended.
> > > 
> > > Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> > > ---
> > > Changes in v4:
> > > - fix typo in commit message bitfiled -> bitfield
> > > - use arrays (lvds_vod_swing_conf and lvds_term_conf) in private data, instead
> > > of separate variables for channel A/B
> > > - add more checks on return value of "of_property_read_u32_array"
> > > Changes in v3:
> > > - use microvolts for default array values 1000 mV -> 1000000 uV.
> > > Changes in v2:
> > > - use datasheet tables to get the proper configuration
> > > - since major change was done change the authorship to myself
> > > ---
> > >   drivers/gpu/drm/bridge/ti-sn65dsi83.c | 147 +++++++++++++++++++++++++-
> > >   1 file changed, 144 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > > index 57a7ed13f996..f724d2a6777b 100644
> > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > > @@ -132,6 +132,16 @@
> > >   #define  REG_IRQ_STAT_CHA_SOT_BIT_ERR		BIT(2)
> > >   #define  REG_IRQ_STAT_CHA_PLL_UNLOCK		BIT(0)
> > > +enum sn65dsi83_channel {
> > > +	CHANNEL_A,
> > > +	CHANNEL_B
> > > +};
> > > +
> > > +enum sn65dsi83_lvds_term {
> > > +	OHM_100,
> > > +	OHM_200
> > > +};
> > > +
> > >   enum sn65dsi83_model {
> > >   	MODEL_SN65DSI83,
> > >   	MODEL_SN65DSI84,
> > > @@ -147,6 +157,8 @@ struct sn65dsi83 {
> > >   	struct regulator		*vcc;
> > >   	bool				lvds_dual_link;
> > >   	bool				lvds_dual_link_even_odd_swap;
> > > +	int				lvds_vod_swing_conf[2];
> > > +	int				lvds_term_conf[2];
> > >   };
> > >   static const struct regmap_range sn65dsi83_readable_ranges[] = {
> > > @@ -237,6 +249,36 @@ static const struct regmap_config sn65dsi83_regmap_config = {
> > >   	.max_register = REG_IRQ_STAT,
> > >   };
> > > +static const int lvds_vod_swing_data_table[2][4][2] = {
> > > +	{	/* 100 Ohm */
> > > +		{ 180000, 313000 },
> > > +		{ 215000, 372000 },
> > > +		{ 250000, 430000 },
> > > +		{ 290000, 488000 },
> > > +	},
> > > +	{	/* 200 Ohm */
> > > +		{ 150000, 261000 },
> > > +		{ 200000, 346000 },
> > > +		{ 250000, 428000 },
> > > +		{ 300000, 511000 },
> > > +	},
> > > +};
> > > +
> > > +static const int lvds_vod_swing_clock_table[2][4][2] = {
> > > +	{	/* 100 Ohm */
> > > +		{ 140000, 244000 },
> > > +		{ 168000, 290000 },
> > > +		{ 195000, 335000 },
> > > +		{ 226000, 381000 },
> > > +	},
> > > +	{	/* 200 Ohm */
> > > +		{ 117000, 204000 },
> > > +		{ 156000, 270000 },
> > > +		{ 195000, 334000 },
> > > +		{ 234000, 399000 },
> > > +	},
> > > +};
> > > +
> > >   static struct sn65dsi83 *bridge_to_sn65dsi83(struct drm_bridge *bridge)
> > >   {
> > >   	return container_of(bridge, struct sn65dsi83, bridge);
> > > @@ -435,12 +477,16 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
> > >   		val |= REG_LVDS_FMT_LVDS_LINK_CFG;
> > >   	regmap_write(ctx->regmap, REG_LVDS_FMT, val);
> > > -	regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x05);
> > > +	regmap_write(ctx->regmap, REG_LVDS_VCOM,
> > > +			REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_A]) |
> > > +			REG_LVDS_VCOM_CHB_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_B]));
> > >   	regmap_write(ctx->regmap, REG_LVDS_LANE,
> > >   		     (ctx->lvds_dual_link_even_odd_swap ?
> > >   		      REG_LVDS_LANE_EVEN_ODD_SWAP : 0) |
> > > -		     REG_LVDS_LANE_CHA_LVDS_TERM |
> > > -		     REG_LVDS_LANE_CHB_LVDS_TERM);
> > > +		     (ctx->lvds_term_conf[CHANNEL_A] ?
> > > +			  REG_LVDS_LANE_CHA_LVDS_TERM : 0) |
> > > +		     (ctx->lvds_term_conf[CHANNEL_B] ?
> > > +			  REG_LVDS_LANE_CHB_LVDS_TERM : 0));
> > >   	regmap_write(ctx->regmap, REG_LVDS_CM, 0x00);
> > >   	le16val = cpu_to_le16(mode->hdisplay);
> > > @@ -576,10 +622,101 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = {
> > >   	.atomic_get_input_bus_fmts = sn65dsi83_atomic_get_input_bus_fmts,
> > >   };
> > > +static int sn65dsi83_select_lvds_vod_swing(struct device *dev,
> > > +	u32 lvds_vod_swing_data[2], u32 lvds_vod_swing_clk[2], u8 lvds_term)
> > > +{
> > > +	int i;
> > > +
> > > +	for (i = 0; i <= 3; i++) {
> > > +		if (lvds_vod_swing_data_table[lvds_term][i][0] >= lvds_vod_swing_data[0] &&
> > > +		lvds_vod_swing_data_table[lvds_term][i][1] <= lvds_vod_swing_data[1] &&
> > > +		lvds_vod_swing_clock_table[lvds_term][i][0] >= lvds_vod_swing_clk[0] &&
> > > +		lvds_vod_swing_clock_table[lvds_term][i][1] <= lvds_vod_swing_clk[1])
> > > +			return i;
> > > +	}
> > > +
> > > +	dev_err(dev, "failed to find appropriate LVDS_VOD_SWING configuration\n");
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static int sn65dsi83_parse_lvds_endpoint(struct sn65dsi83 *ctx, int channel)
> > > +{
> > > +	struct device *dev = ctx->dev;
> > > +	struct device_node *endpoint;
> > > +	/* Set so the property can be freely selected if not defined */
> > > +	u32 lvds_vod_swing_data[2] = { 0, 1000000 };
> > > +	u32 lvds_vod_swing_clk[2] = { 0, 1000000 };
> > > +	u32 lvds_term = 200;
> > > +	u8 lvds_term_conf;
> > > +	int endpoint_reg;
> > > +	int lvds_vod_swing_conf;
> > > +	int ret = 0;
> > > +	int ret_data;
> > > +	int ret_clock;
> > > +
> > > +	if (channel == CHANNEL_A)
> > > +		endpoint_reg = 2;
> > > +	else
> > > +		endpoint_reg = 3;
> > > +
> > > +	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, endpoint_reg, -1);
> > > +	of_property_read_u32(endpoint, "ti,lvds-termination-ohms", &lvds_term);
> > > +
> > > +	if (lvds_term == 200)
> > > +		lvds_term_conf = OHM_200;
> > > +	else
> > > +		lvds_term_conf = OHM_100;
> > > +
> > > +	ctx->lvds_term_conf[channel] = lvds_term_conf;
> > > +
> > > +	ret_data = of_property_read_u32_array(endpoint,
> > > +			"ti,lvds-vod-swing-data-microvolt", lvds_vod_swing_data,
> > > +			ARRAY_SIZE(lvds_vod_swing_data));
> > > +	if (ret_data != 0 && ret_data != -EINVAL) {
> > > +		ret = ret_data;
> > > +		goto exit;
> > > +	}
> > > +
> > > +	ret_clock = of_property_read_u32_array(endpoint,
> > > +			"ti,lvds-vod-swing-clock-microvolt", lvds_vod_swing_clk,
> > > +			ARRAY_SIZE(lvds_vod_swing_clk));
> > > +	if (ret_clock != 0 && ret_clock != -EINVAL) {
> > > +		ret = ret_clock;
> > > +		goto exit;
> > > +	}
> > > +
> > > +	/* If any of the two properties is defined. */
> > > +	if (!ret_data || !ret_clock) {
> > > +		lvds_vod_swing_conf = sn65dsi83_select_lvds_vod_swing(dev,
> > > +			lvds_vod_swing_data, lvds_vod_swing_clk,
> > > +			lvds_term_conf);
> > > +		if (lvds_vod_swing_conf < 0) {
> > > +			ret = lvds_vod_swing_conf;
> > > +			goto exit;
> > > +		}
> > > +
> > > +		ctx->lvds_vod_swing_conf[channel] = lvds_vod_swing_conf;
> > > +	}
> > > +	ret = 0;
> > > +exit:
> > > +	of_node_put(endpoint);
> > > +	return ret;
> > > +}
> > > +
> > >   static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
> > >   {
> > >   	struct drm_bridge *panel_bridge;
> > >   	struct device *dev = ctx->dev;
> > > +	int ret;
> > > +
> > > +	ctx->lvds_vod_swing_conf[CHANNEL_A] = 0x1;
> > > +	ctx->lvds_vod_swing_conf[CHANNEL_B] = 0x1;
> > > +	ctx->lvds_term_conf[CHANNEL_A] = 0x1;
> > > +	ctx->lvds_term_conf[CHANNEL_B] = 0x1;
> > 
> > These match the defaults in sn65dsi83_parse_lvds_endpoint(). Do we
> > really need those?
> 
> Yes, I think we do. This ensures that defaults are used even when property
> is not defined/LVDS channel is not used. So also LVDS channel B defaults are
> set even for sn65dsi83 (single LVDS output). Keeping the same reg values as
> before these changes.

You can move sn65dsi83_parse_lvds_endpoint() out of the if() and get the
same result. Duplicating data (or code) is a bad idea, because it's easy
to update one point and miss another point. And then usually one has a
nice debugging session, trying to understand why their changes didn't
work out.

> 
> Best regards,
> Andrej
> 
> 
> > 
> > > +
> > > +	ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_A);
> > > +	if (ret < 0)
> > > +		return ret;
> > >   	ctx->lvds_dual_link = false;
> > >   	ctx->lvds_dual_link_even_odd_swap = false;
> > > @@ -587,6 +724,10 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
> > >   		struct device_node *port2, *port3;
> > >   		int dual_link;
> > > +		ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_B);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > >   		port2 = of_graph_get_port_by_id(dev->of_node, 2);
> > >   		port3 = of_graph_get_port_by_id(dev->of_node, 3);
> > >   		dual_link = drm_of_lvds_get_dual_link_pixel_order(port2, port3);
> > > -- 
> > > 2.34.1
> > > 
> > 

-- 
With best wishes
Dmitry

Re: [PATCH v4 2/3] drm/bridge: ti-sn65dsi83: Add ti,lvds-vod-swing optional properties

[Andrej Picej]

On 10. 12. 24 02:18, Dmitry Baryshkov wrote:
> On Mon, Dec 09, 2024 at 08:56:29AM +0100, Andrej Picej wrote:
>> Hi Dmitry,
>>
>> On 5. 12. 24 23:48, Dmitry Baryshkov wrote:
>>> On Thu, Dec 05, 2024 at 02:40:20PM +0100, Andrej Picej wrote:
>>>> Add a optional properties to change LVDS output voltage. This should not
>>>> be static as this depends mainly on the connected display voltage
>>>> requirement. We have three properties:
>>>> - "ti,lvds-termination-ohms", which sets near end termination,
>>>> - "ti,lvds-vod-swing-data-microvolt" and
>>>> - "ti,lvds-vod-swing-clock-microvolt" which both set LVDS differential
>>>> output voltage for data and clock lanes. They are defined as an array
>>>> with min and max values. The appropriate bitfield will be set if
>>>> selected constraints can be met.
>>>>
>>>> If "ti,lvds-termination-ohms" is not defined the default of 200 Ohm near
>>>> end termination will be used. Selecting only one:
>>>> "ti,lvds-vod-swing-data-microvolt" or
>>>> "ti,lvds-vod-swing-clock-microvolt" can be done, but the output voltage
>>>> constraint for only data/clock lanes will be met. Setting both is
>>>> recommended.
>>>>
>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>>> ---
>>>> Changes in v4:
>>>> - fix typo in commit message bitfiled -> bitfield
>>>> - use arrays (lvds_vod_swing_conf and lvds_term_conf) in private data, instead
>>>> of separate variables for channel A/B
>>>> - add more checks on return value of "of_property_read_u32_array"
>>>> Changes in v3:
>>>> - use microvolts for default array values 1000 mV -> 1000000 uV.
>>>> Changes in v2:
>>>> - use datasheet tables to get the proper configuration
>>>> - since major change was done change the authorship to myself
>>>> ---
>>>>    drivers/gpu/drm/bridge/ti-sn65dsi83.c | 147 +++++++++++++++++++++++++-
>>>>    1 file changed, 144 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>>>> index 57a7ed13f996..f724d2a6777b 100644
>>>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>>>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>>>> @@ -132,6 +132,16 @@
>>>>    #define  REG_IRQ_STAT_CHA_SOT_BIT_ERR		BIT(2)
>>>>    #define  REG_IRQ_STAT_CHA_PLL_UNLOCK		BIT(0)
>>>> +enum sn65dsi83_channel {
>>>> +	CHANNEL_A,
>>>> +	CHANNEL_B
>>>> +};
>>>> +
>>>> +enum sn65dsi83_lvds_term {
>>>> +	OHM_100,
>>>> +	OHM_200
>>>> +};
>>>> +
>>>>    enum sn65dsi83_model {
>>>>    	MODEL_SN65DSI83,
>>>>    	MODEL_SN65DSI84,
>>>> @@ -147,6 +157,8 @@ struct sn65dsi83 {
>>>>    	struct regulator		*vcc;
>>>>    	bool				lvds_dual_link;
>>>>    	bool				lvds_dual_link_even_odd_swap;
>>>> +	int				lvds_vod_swing_conf[2];
>>>> +	int				lvds_term_conf[2];
>>>>    };
>>>>    static const struct regmap_range sn65dsi83_readable_ranges[] = {
>>>> @@ -237,6 +249,36 @@ static const struct regmap_config sn65dsi83_regmap_config = {
>>>>    	.max_register = REG_IRQ_STAT,
>>>>    };
>>>> +static const int lvds_vod_swing_data_table[2][4][2] = {
>>>> +	{	/* 100 Ohm */
>>>> +		{ 180000, 313000 },
>>>> +		{ 215000, 372000 },
>>>> +		{ 250000, 430000 },
>>>> +		{ 290000, 488000 },
>>>> +	},
>>>> +	{	/* 200 Ohm */
>>>> +		{ 150000, 261000 },
>>>> +		{ 200000, 346000 },
>>>> +		{ 250000, 428000 },
>>>> +		{ 300000, 511000 },
>>>> +	},
>>>> +};
>>>> +
>>>> +static const int lvds_vod_swing_clock_table[2][4][2] = {
>>>> +	{	/* 100 Ohm */
>>>> +		{ 140000, 244000 },
>>>> +		{ 168000, 290000 },
>>>> +		{ 195000, 335000 },
>>>> +		{ 226000, 381000 },
>>>> +	},
>>>> +	{	/* 200 Ohm */
>>>> +		{ 117000, 204000 },
>>>> +		{ 156000, 270000 },
>>>> +		{ 195000, 334000 },
>>>> +		{ 234000, 399000 },
>>>> +	},
>>>> +};
>>>> +
>>>>    static struct sn65dsi83 *bridge_to_sn65dsi83(struct drm_bridge *bridge)
>>>>    {
>>>>    	return container_of(bridge, struct sn65dsi83, bridge);
>>>> @@ -435,12 +477,16 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
>>>>    		val |= REG_LVDS_FMT_LVDS_LINK_CFG;
>>>>    	regmap_write(ctx->regmap, REG_LVDS_FMT, val);
>>>> -	regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x05);
>>>> +	regmap_write(ctx->regmap, REG_LVDS_VCOM,
>>>> +			REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_A]) |
>>>> +			REG_LVDS_VCOM_CHB_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_B]));
>>>>    	regmap_write(ctx->regmap, REG_LVDS_LANE,
>>>>    		     (ctx->lvds_dual_link_even_odd_swap ?
>>>>    		      REG_LVDS_LANE_EVEN_ODD_SWAP : 0) |
>>>> -		     REG_LVDS_LANE_CHA_LVDS_TERM |
>>>> -		     REG_LVDS_LANE_CHB_LVDS_TERM);
>>>> +		     (ctx->lvds_term_conf[CHANNEL_A] ?
>>>> +			  REG_LVDS_LANE_CHA_LVDS_TERM : 0) |
>>>> +		     (ctx->lvds_term_conf[CHANNEL_B] ?
>>>> +			  REG_LVDS_LANE_CHB_LVDS_TERM : 0));
>>>>    	regmap_write(ctx->regmap, REG_LVDS_CM, 0x00);
>>>>    	le16val = cpu_to_le16(mode->hdisplay);
>>>> @@ -576,10 +622,101 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = {
>>>>    	.atomic_get_input_bus_fmts = sn65dsi83_atomic_get_input_bus_fmts,
>>>>    };
>>>> +static int sn65dsi83_select_lvds_vod_swing(struct device *dev,
>>>> +	u32 lvds_vod_swing_data[2], u32 lvds_vod_swing_clk[2], u8 lvds_term)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i <= 3; i++) {
>>>> +		if (lvds_vod_swing_data_table[lvds_term][i][0] >= lvds_vod_swing_data[0] &&
>>>> +		lvds_vod_swing_data_table[lvds_term][i][1] <= lvds_vod_swing_data[1] &&
>>>> +		lvds_vod_swing_clock_table[lvds_term][i][0] >= lvds_vod_swing_clk[0] &&
>>>> +		lvds_vod_swing_clock_table[lvds_term][i][1] <= lvds_vod_swing_clk[1])
>>>> +			return i;
>>>> +	}
>>>> +
>>>> +	dev_err(dev, "failed to find appropriate LVDS_VOD_SWING configuration\n");
>>>> +	return -EINVAL;
>>>> +}
>>>> +
>>>> +static int sn65dsi83_parse_lvds_endpoint(struct sn65dsi83 *ctx, int channel)
>>>> +{
>>>> +	struct device *dev = ctx->dev;
>>>> +	struct device_node *endpoint;
>>>> +	/* Set so the property can be freely selected if not defined */
>>>> +	u32 lvds_vod_swing_data[2] = { 0, 1000000 };
>>>> +	u32 lvds_vod_swing_clk[2] = { 0, 1000000 };
>>>> +	u32 lvds_term = 200;
>>>> +	u8 lvds_term_conf;
>>>> +	int endpoint_reg;
>>>> +	int lvds_vod_swing_conf;
>>>> +	int ret = 0;
>>>> +	int ret_data;
>>>> +	int ret_clock;
>>>> +
>>>> +	if (channel == CHANNEL_A)
>>>> +		endpoint_reg = 2;
>>>> +	else
>>>> +		endpoint_reg = 3;
>>>> +
>>>> +	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, endpoint_reg, -1);
>>>> +	of_property_read_u32(endpoint, "ti,lvds-termination-ohms", &lvds_term);
>>>> +
>>>> +	if (lvds_term == 200)
>>>> +		lvds_term_conf = OHM_200;
>>>> +	else
>>>> +		lvds_term_conf = OHM_100;
>>>> +
>>>> +	ctx->lvds_term_conf[channel] = lvds_term_conf;
>>>> +
>>>> +	ret_data = of_property_read_u32_array(endpoint,
>>>> +			"ti,lvds-vod-swing-data-microvolt", lvds_vod_swing_data,
>>>> +			ARRAY_SIZE(lvds_vod_swing_data));
>>>> +	if (ret_data != 0 && ret_data != -EINVAL) {
>>>> +		ret = ret_data;
>>>> +		goto exit;
>>>> +	}
>>>> +
>>>> +	ret_clock = of_property_read_u32_array(endpoint,
>>>> +			"ti,lvds-vod-swing-clock-microvolt", lvds_vod_swing_clk,
>>>> +			ARRAY_SIZE(lvds_vod_swing_clk));
>>>> +	if (ret_clock != 0 && ret_clock != -EINVAL) {
>>>> +		ret = ret_clock;
>>>> +		goto exit;
>>>> +	}
>>>> +
>>>> +	/* If any of the two properties is defined. */
>>>> +	if (!ret_data || !ret_clock) {
>>>> +		lvds_vod_swing_conf = sn65dsi83_select_lvds_vod_swing(dev,
>>>> +			lvds_vod_swing_data, lvds_vod_swing_clk,
>>>> +			lvds_term_conf);
>>>> +		if (lvds_vod_swing_conf < 0) {
>>>> +			ret = lvds_vod_swing_conf;
>>>> +			goto exit;
>>>> +		}
>>>> +
>>>> +		ctx->lvds_vod_swing_conf[channel] = lvds_vod_swing_conf;
>>>> +	}
>>>> +	ret = 0;
>>>> +exit:
>>>> +	of_node_put(endpoint);
>>>> +	return ret;
>>>> +}
>>>> +
>>>>    static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
>>>>    {
>>>>    	struct drm_bridge *panel_bridge;
>>>>    	struct device *dev = ctx->dev;
>>>> +	int ret;
>>>> +
>>>> +	ctx->lvds_vod_swing_conf[CHANNEL_A] = 0x1;
>>>> +	ctx->lvds_vod_swing_conf[CHANNEL_B] = 0x1;
>>>> +	ctx->lvds_term_conf[CHANNEL_A] = 0x1;
>>>> +	ctx->lvds_term_conf[CHANNEL_B] = 0x1;
>>>
>>> These match the defaults in sn65dsi83_parse_lvds_endpoint(). Do we
>>> really need those?
>>
>> Yes, I think we do. This ensures that defaults are used even when property
>> is not defined/LVDS channel is not used. So also LVDS channel B defaults are
>> set even for sn65dsi83 (single LVDS output). Keeping the same reg values as
>> before these changes.
> 
> You can move sn65dsi83_parse_lvds_endpoint() out of the if() and get the
> same result. Duplicating data (or code) is a bad idea, because it's easy
> to update one point and miss another point. And then usually one has a
> nice debugging session, trying to understand why their changes didn't
> work out.

Ok I see what you are trying to accomplish. I'll set default values in 
sn65dsi83_parse_lvds_endpoint() in v5 like you suggested. Thanks.

Best regards,
Andrej

> 
>>
>> Best regards,
>> Andrej
>>
>>
>>>
>>>> +
>>>> +	ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_A);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>>    	ctx->lvds_dual_link = false;
>>>>    	ctx->lvds_dual_link_even_odd_swap = false;
>>>> @@ -587,6 +724,10 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
>>>>    		struct device_node *port2, *port3;
>>>>    		int dual_link;
>>>> +		ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_B);
>>>> +		if (ret < 0)
>>>> +			return ret;
>>>> +
>>>>    		port2 = of_graph_get_port_by_id(dev->of_node, 2);
>>>>    		port3 = of_graph_get_port_by_id(dev->of_node, 3);
>>>>    		dual_link = drm_of_lvds_get_dual_link_pixel_order(port2, port3);
>>>> -- 
>>>> 2.34.1
>>>>
>>>
>