[PATCH v6 0/2] usb: typec: Add new driver for Parade PS8830 Type-C Retimer

[PATCH v6 0/2] usb: typec: Add new driver for Parade PS8830 Type-C Retimer

[Abel Vesa]

The Parade PS8830 is a Type-C multi-protocol retimer that is controlled
via I2C. It provides altmode and orientation handling and usually sits
between the Type-C port and the PHY.

It is currently used alongside Qualcomm Snapdragon X Elite SoCs on quite
a few laptops already.

This new driver adds support for the following 3 modes:
 - DP 4lanes (pin assignments C and E)
 - DP 2lanes + USB3 (pin assignment D)
 - USB3

This retimer is a LTTPR (Link-Training Tunable PHY Repeater) which means
it can support link training from source to itself. This means that the
DP driver needs to be aware of the repeater presence and to handle
the link training accordingly. This is currently missing from msm dp
driver, but there is already a patchset [1] on the list that adds it.

The support for LTTPR in drm/msm/dp is here (v5):
https://lore.kernel.org/all/20250203-drm-dp-msm-add-lttpr-transparent-mode-set-v5-4-c865d0e56d6e@linaro.org/

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
Changes in v6:
- Dropped the DT patches as they should not be merged until the LTTPR
  and the fixes in ucsi and pmic glink altmode are also merged,
  otherwise it would render the external DP and the resume from
  system suspend broken.
- Replaced dev_err_probe with dev_err for typec switch and retimer
  register failure handling, like Johan suggested.
- Fixed the gpio orientation for when the retimer is already
  initialized.
- Dropped the useless "No SVID" comment in ps883x_retimer_set()
- Added definition of all ports (including port@2) in dt-bindings
  schema.
- Link to v5: https://lore.kernel.org/r/20241112-x1e80100-ps8830-v5-0-4ad83af4d162@linaro.org

Changes in v5:
- Renamed binding schema to be the same as the compatible.
- Dropped the ps8830,boot-on from the schema.
- Added register offsets and bits names to the driver, like Konrad
  suggested.
- Reordered the vregs and clocks enabling, as per Johan's request.
- Used the dev_err_probe for regmap init failure and dropped the
  multiple regulator disable calls, replacing it with single call to
  helper, as Christophe suggested. Also replaced dev_err with
  dev_err_probe on typec_switch_register and typec_mux_register failure.
- Added some new pinctrl specific properties to all pmic provided
  gpios that control retimer vregs.
- Re-ordered alphabetically the retimers default state pinconfs.
- Added the T14s patches with same exact support, as per Johan's
  request.
- Link to v4: https://lore.kernel.org/r/20241101-x1e80100-ps8830-v4-0-f0f7518b263e@linaro.org

Changes in v4:
- Renamed the driver and bindings schema to ps883x to allow future
  support for the PS8833.
- Dropped the dedicated DT property for keeping the retimers from
  resetting on probe, and replaced that with a read to figure out
  if it has been already configured or not. This involves leaving the
  reset gpio as-is on probe if the retimer has been already configured.
- Replaced the fwnode_typec_switch_get() call with typec_switch_get()
- Replaced the fwnode_typec_mux_get() call with typec_mux_get()
- Dropped the clock name, as there is only one clock. As per Bjorn's
  suggestion.
- Dropped regcache as it seems it is not needed.
- Re-worded all commit messages to explain better the problem and the
  proposed changes.
- Link to v3: https://lore.kernel.org/r/20241022-x1e80100-ps8830-v3-0-68a95f351e99@linaro.org

Changes in v3:
- Reworked the schema binding by using the usb/usb-switch.yaml defined
  port graph and properties. Addressed all comments from Johan and
  Dmitry.
- Dropped the manual caching of the config values on regmap write in the
  driver.
- Reordered the DP pin assignment states within the switch clause, as
  Dmitry suggested.
- Added SVID check to not allow any altmode other than DP.
- Added DT patches (retimer for USB orientation handling and DP
  enablement). Did this in order to offer a full picture of how it all
  fits together.
- Split the DP enablement in DT in a separate patchset so the USB
  handling can be merged separately. 
- Added ps8830,boot-on to let the driver know it is supposed to skip
  resetting the retimer on driver probe, as the bootloader might already
  let it in a pre-configured state.
- Marked all retimer voltage regulators as boot-on since we want to
  maintain the state for coldplug orientation.
- Added pinconf for all retimer0 gpios.
- Didn't pick up Konrad's T-b tags and Krzysztof's R-b tag as the rework
  is quite extensive. Especially because of the ps8830,boot-on and what
  it does.
- Link to v2: https://lore.kernel.org/r/20241004-x1e80100-ps8830-v2-0-5cd8008c8c40@linaro.org

Changes in v2:
- Addressed all comments from Johan and Konrad.
- Reworked the handling of the vregs so it would be more cleaner.
  Dropped the usage of bulk regulators API and handled them separately.
  Also discribed all regulators according to data sheet.
- Added all delays according to data sheet.
- Fixed coldplug (on boot) orientation detection.
- Didn't pick Krzysztof's R-b tag because the bindings changed w.r.t
  supplies.
- Link to v1: https://lore.kernel.org/r/20240829-x1e80100-ps8830-v1-0-bcc4790b1d45@linaro.org

---
Abel Vesa (2):
      dt-bindings: usb: Add Parade PS8830 Type-C retimer bindings
      usb: typec: Add support for Parade PS8830 Type-C Retimer

 .../devicetree/bindings/usb/parade,ps8830.yaml     | 140 +++++++
 drivers/usb/typec/mux/Kconfig                      |  10 +
 drivers/usb/typec/mux/Makefile                     |   1 +
 drivers/usb/typec/mux/ps883x.c                     | 437 +++++++++++++++++++++
 4 files changed, 588 insertions(+)
---
base-commit: 00f3246adeeacbda0bd0b303604e46eb59c32e6e
change-id: 20240521-x1e80100-ps8830-d5ccca95b557

Best regards,
-- 
Abel Vesa <abel.vesa@linaro.org>

[PATCH v6 1/2] dt-bindings: usb: Add Parade PS8830 Type-C retimer bindings

[Abel Vesa]

The Parade PS8830 is a USB4, DisplayPort and Thunderbolt 4 retimer,
controlled over I2C. It usually sits between a USB/DisplayPort PHY and the
Type-C connector, and provides orientation and altmode handling.

Currently, it is found on all boards featuring the Qualcomm Snapdragon
X Elite SoCs.

Document bindings for its new driver. Future-proof the schema for the
PS8833 variant, which seems to be similar to PS8830.

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 .../devicetree/bindings/usb/parade,ps8830.yaml     | 140 +++++++++++++++++++++
 1 file changed, 140 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/parade,ps8830.yaml b/Documentation/devicetree/bindings/usb/parade,ps8830.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..935d57f5d26fe597308f1200ace9559255bad1e4
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/parade,ps8830.yaml
@@ -0,0 +1,140 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/parade,ps8830.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Parade PS883x USB and DisplayPort Retimer
+
+maintainers:
+  - Abel Vesa <abel.vesa@linaro.org>
+
+properties:
+  compatible:
+    enum:
+      - parade,ps8830
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: XO Clock
+
+  reset-gpios:
+    maxItems: 1
+
+  vdd-supply:
+    description: power supply (1.07V)
+
+  vdd33-supply:
+    description: power supply (3.3V)
+
+  vdd33-cap-supply:
+    description: power supply (3.3V)
+
+  vddar-supply:
+    description: power supply (1.07V)
+
+  vddat-supply:
+    description: power supply (1.07V)
+
+  vddio-supply:
+    description: power supply (1.2V or 1.8V)
+
+  orientation-switch: true
+  retimer-switch: true
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: Super Speed (SS) Output endpoint to the Type-C connector
+
+      port@1:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        description: Super Speed (SS) Input endpoint from the Super-Speed PHY
+        unevaluatedProperties: false
+
+      port@2:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          Sideband Use (SBU) AUX lines endpoint to the Type-C connector for the purpose of
+          handling altmode muxing and orientation switching.
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - reset-gpios
+  - vdd-supply
+  - vdd33-supply
+  - vdd33-cap-supply
+  - vddat-supply
+  - vddio-supply
+  - orientation-switch
+  - retimer-switch
+
+allOf:
+  - $ref: usb-switch.yaml#
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        typec-mux@8 {
+            compatible = "parade,ps8830";
+            reg = <0x8>;
+
+            clocks = <&clk_rtmr_xo>;
+
+            vdd-supply = <&vreg_rtmr_1p15>;
+            vdd33-supply = <&vreg_rtmr_3p3>;
+            vdd33-cap-supply = <&vreg_rtmr_3p3>;
+            vddar-supply = <&vreg_rtmr_1p15>;
+            vddat-supply = <&vreg_rtmr_1p15>;
+            vddio-supply = <&vreg_rtmr_1p8>;
+
+            reset-gpios = <&tlmm 10 GPIO_ACTIVE_LOW>;
+
+            retimer-switch;
+            orientation-switch;
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+
+                    endpoint {
+                        remote-endpoint = <&typec_con_ss>;
+                    };
+                };
+
+                port@1 {
+                    reg = <1>;
+
+                    endpoint {
+                        remote-endpoint = <&usb_phy_ss>;
+                    };
+                };
+
+                port@2 {
+                    reg = <2>;
+
+                    endpoint {
+                        remote-endpoint = <&typec_dp_aux>;
+                    };
+                };
+            };
+        };
+    };
+...

-- 
2.34.1

[PATCH v6 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Abel Vesa]

The Parade PS8830 is a USB4, DisplayPort and Thunderbolt 4 retimer,
controlled over I2C. It usually sits between a USB/DisplayPort PHY
and the Type-C connector, and provides orientation and altmode handling.

The boards that use this retimer are the ones featuring the Qualcomm
Snapdragon X Elite SoCs.

Add a driver with support for the following modes:
 - DisplayPort 4-lanes
 - DisplayPort 2-lanes + USB3
 - USB3

There is another variant of this retimer which is called PS8833. It seems
to be really similar to the PS8830, so future-proof this driver by
naming it ps883x.

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 drivers/usb/typec/mux/Kconfig  |  10 +
 drivers/usb/typec/mux/Makefile |   1 +
 drivers/usb/typec/mux/ps883x.c | 437 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 448 insertions(+)

diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
index 67381b4ef4f68f4a6e73f157365ee24d0ab7109a..6dd8f961b593261fde1d39b238b981966e463599 100644
--- a/drivers/usb/typec/mux/Kconfig
+++ b/drivers/usb/typec/mux/Kconfig
@@ -56,6 +56,16 @@ config TYPEC_MUX_NB7VPQ904M
 	  Say Y or M if your system has a On Semiconductor NB7VPQ904M Type-C
 	  redriver chip found on some devices with a Type-C port.
 
+config TYPEC_MUX_PS883X
+	tristate "Parade PS883x Type-C retimer driver"
+	depends on I2C
+	depends on DRM || DRM=n
+	select DRM_AUX_BRIDGE if DRM_BRIDGE && OF
+	select REGMAP_I2C
+	help
+	  Say Y or M if your system has a Parade PS883x Type-C retimer chip
+	  found on some devices with a Type-C port.
+
 config TYPEC_MUX_PTN36502
 	tristate "NXP PTN36502 Type-C redriver driver"
 	depends on I2C
diff --git a/drivers/usb/typec/mux/Makefile b/drivers/usb/typec/mux/Makefile
index 60879446da9365183567d3374a2fb7b5171fb3d7..b4f599eb5053b8f20e9a41409b0a2d9a03d850b6 100644
--- a/drivers/usb/typec/mux/Makefile
+++ b/drivers/usb/typec/mux/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_TYPEC_MUX_PI3USB30532)	+= pi3usb30532.o
 obj-$(CONFIG_TYPEC_MUX_INTEL_PMC)	+= intel_pmc_mux.o
 obj-$(CONFIG_TYPEC_MUX_IT5205)		+= it5205.o
 obj-$(CONFIG_TYPEC_MUX_NB7VPQ904M)	+= nb7vpq904m.o
+obj-$(CONFIG_TYPEC_MUX_PS883X)		+= ps883x.o
 obj-$(CONFIG_TYPEC_MUX_PTN36502)	+= ptn36502.o
 obj-$(CONFIG_TYPEC_MUX_TUSB1046)	+= tusb1046.o
 obj-$(CONFIG_TYPEC_MUX_WCD939X_USBSS)	+= wcd939x-usbss.o
diff --git a/drivers/usb/typec/mux/ps883x.c b/drivers/usb/typec/mux/ps883x.c
new file mode 100644
index 0000000000000000000000000000000000000000..10e407ab6b7f6ce3c6d4a61a2d6219eb1b9e85e8
--- /dev/null
+++ b/drivers/usb/typec/mux/ps883x.c
@@ -0,0 +1,437 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Parade ps883x usb retimer driver
+ *
+ * Copyright (C) 2024 Linaro Ltd.
+ */
+
+#include <drm/bridge/aux-bridge.h>
+#include <linux/clk.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/usb/typec_altmode.h>
+#include <linux/usb/typec_dp.h>
+#include <linux/usb/typec_mux.h>
+#include <linux/usb/typec_retimer.h>
+
+#define REG_USB_PORT_CONN_STATUS_0		0x00
+
+#define CONN_STATUS_0_CONNECTION_PRESENT	BIT(0)
+#define CONN_STATUS_0_ORIENTATION_REVERSED	BIT(1)
+#define CONN_STATUS_0_USB_3_1_CONNECTED		BIT(5)
+
+#define REG_USB_PORT_CONN_STATUS_1		0x01
+
+#define CONN_STATUS_1_DP_CONNECTED		BIT(0)
+#define CONN_STATUS_1_DP_SINK_REQUESTED		BIT(1)
+#define CONN_STATUS_1_DP_PIN_ASSIGNMENT_C_D	BIT(2)
+#define CONN_STATUS_1_DP_HPD_LEVEL		BIT(7)
+
+#define REG_USB_PORT_CONN_STATUS_2		0x02
+
+struct ps883x_retimer {
+	struct i2c_client *client;
+	struct gpio_desc *reset_gpio;
+	struct regmap *regmap;
+	struct typec_switch_dev *sw;
+	struct typec_retimer *retimer;
+	struct clk *xo_clk;
+	struct regulator *vdd_supply;
+	struct regulator *vdd33_supply;
+	struct regulator *vdd33_cap_supply;
+	struct regulator *vddat_supply;
+	struct regulator *vddar_supply;
+	struct regulator *vddio_supply;
+
+	struct typec_switch *typec_switch;
+	struct typec_mux *typec_mux;
+
+	struct mutex lock; /* protect non-concurrent retimer & switch */
+
+	enum typec_orientation orientation;
+	unsigned long mode;
+	unsigned int svid;
+};
+
+static void ps883x_configure(struct ps883x_retimer *retimer, int cfg0,
+			     int cfg1, int cfg2)
+{
+	regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_0, cfg0);
+	regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_1, cfg1);
+	regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_2, cfg2);
+}
+
+static int ps883x_set(struct ps883x_retimer *retimer)
+{
+	int cfg0 = CONN_STATUS_0_CONNECTION_PRESENT;
+	int cfg1 = 0x00;
+	int cfg2 = 0x00;
+
+	if (retimer->orientation == TYPEC_ORIENTATION_NONE ||
+	    retimer->mode == TYPEC_STATE_SAFE) {
+		ps883x_configure(retimer, cfg0, cfg1, cfg2);
+		return 0;
+	}
+
+	if (retimer->mode != TYPEC_STATE_USB && retimer->svid != USB_TYPEC_DP_SID)
+		return -EINVAL;
+
+	if (retimer->orientation == TYPEC_ORIENTATION_REVERSE)
+		cfg0 |= CONN_STATUS_0_ORIENTATION_REVERSED;
+
+	switch (retimer->mode) {
+	case TYPEC_STATE_USB:
+		cfg0 |= CONN_STATUS_0_USB_3_1_CONNECTED;
+		break;
+
+	case TYPEC_DP_STATE_C:
+		cfg1 = CONN_STATUS_1_DP_CONNECTED |
+		       CONN_STATUS_1_DP_SINK_REQUESTED |
+		       CONN_STATUS_1_DP_PIN_ASSIGNMENT_C_D |
+		       CONN_STATUS_1_DP_HPD_LEVEL;
+		break;
+
+	case TYPEC_DP_STATE_D:
+		cfg0 |= CONN_STATUS_0_USB_3_1_CONNECTED;
+		cfg1 = CONN_STATUS_1_DP_CONNECTED |
+		       CONN_STATUS_1_DP_SINK_REQUESTED |
+		       CONN_STATUS_1_DP_PIN_ASSIGNMENT_C_D |
+		       CONN_STATUS_1_DP_HPD_LEVEL;
+		break;
+
+	case TYPEC_DP_STATE_E:
+		cfg1 = CONN_STATUS_1_DP_CONNECTED |
+		       CONN_STATUS_1_DP_HPD_LEVEL;
+		break;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	ps883x_configure(retimer, cfg0, cfg1, cfg2);
+
+	return 0;
+}
+
+static int ps883x_sw_set(struct typec_switch_dev *sw,
+			 enum typec_orientation orientation)
+{
+	struct ps883x_retimer *retimer = typec_switch_get_drvdata(sw);
+	int ret = 0;
+
+	ret = typec_switch_set(retimer->typec_switch, orientation);
+	if (ret)
+		return ret;
+
+	mutex_lock(&retimer->lock);
+
+	if (retimer->orientation != orientation) {
+		retimer->orientation = orientation;
+
+		ret = ps883x_set(retimer);
+	}
+
+	mutex_unlock(&retimer->lock);
+
+	return ret;
+}
+
+static int ps883x_retimer_set(struct typec_retimer *rtmr,
+			      struct typec_retimer_state *state)
+{
+	struct ps883x_retimer *retimer = typec_retimer_get_drvdata(rtmr);
+	struct typec_mux_state mux_state;
+	int ret = 0;
+
+	mutex_lock(&retimer->lock);
+
+	if (state->mode != retimer->mode) {
+		retimer->mode = state->mode;
+
+		if (state->alt)
+			retimer->svid = state->alt->svid;
+		else
+			retimer->svid = 0;
+
+		ret = ps883x_set(retimer);
+	}
+
+	mutex_unlock(&retimer->lock);
+
+	if (ret)
+		return ret;
+
+	mux_state.alt = state->alt;
+	mux_state.data = state->data;
+	mux_state.mode = state->mode;
+
+	return typec_mux_set(retimer->typec_mux, &mux_state);
+}
+
+static int ps883x_enable_vregs(struct ps883x_retimer *retimer)
+{
+	struct device *dev = &retimer->client->dev;
+	int ret;
+
+	ret = regulator_enable(retimer->vdd33_supply);
+	if (ret) {
+		dev_err(dev, "cannot enable VDD 3.3V regulator: %d\n", ret);
+		return ret;
+	}
+
+	ret = regulator_enable(retimer->vdd33_cap_supply);
+	if (ret) {
+		dev_err(dev, "cannot enable VDD 3.3V CAP regulator: %d\n", ret);
+		goto err_vdd33_disable;
+	}
+
+	usleep_range(4000, 10000);
+
+	ret = regulator_enable(retimer->vdd_supply);
+	if (ret) {
+		dev_err(dev, "cannot enable VDD regulator: %d\n", ret);
+		goto err_vdd33_cap_disable;
+	}
+
+	ret = regulator_enable(retimer->vddar_supply);
+	if (ret) {
+		dev_err(dev, "cannot enable VDD AR regulator: %d\n", ret);
+		goto err_vdd_disable;
+	}
+
+	ret = regulator_enable(retimer->vddat_supply);
+	if (ret) {
+		dev_err(dev, "cannot enable VDD AT regulator: %d\n", ret);
+		goto err_vddar_disable;
+	}
+
+	ret = regulator_enable(retimer->vddio_supply);
+	if (ret) {
+		dev_err(dev, "cannot enable VDD IO regulator: %d\n", ret);
+		goto err_vddat_disable;
+	}
+
+	return 0;
+
+err_vddat_disable:
+	regulator_disable(retimer->vddat_supply);
+err_vddar_disable:
+	regulator_disable(retimer->vddar_supply);
+err_vdd_disable:
+	regulator_disable(retimer->vdd_supply);
+err_vdd33_cap_disable:
+	regulator_disable(retimer->vdd33_cap_supply);
+err_vdd33_disable:
+	regulator_disable(retimer->vdd33_supply);
+
+	return ret;
+}
+
+static void ps883x_disable_vregs(struct ps883x_retimer *retimer)
+{
+	regulator_disable(retimer->vddio_supply);
+	regulator_disable(retimer->vddat_supply);
+	regulator_disable(retimer->vddar_supply);
+	regulator_disable(retimer->vdd_supply);
+	regulator_disable(retimer->vdd33_cap_supply);
+	regulator_disable(retimer->vdd33_supply);
+}
+
+static int ps883x_get_vregs(struct ps883x_retimer *retimer)
+{
+	struct device *dev = &retimer->client->dev;
+
+	retimer->vdd_supply = devm_regulator_get(dev, "vdd");
+	if (IS_ERR(retimer->vdd_supply))
+		return dev_err_probe(dev, PTR_ERR(retimer->vdd_supply),
+				     "failed to get VDD\n");
+
+	retimer->vdd33_supply = devm_regulator_get(dev, "vdd33");
+	if (IS_ERR(retimer->vdd33_supply))
+		return dev_err_probe(dev, PTR_ERR(retimer->vdd33_supply),
+				     "failed to get VDD 3.3V\n");
+
+	retimer->vdd33_cap_supply = devm_regulator_get(dev, "vdd33-cap");
+	if (IS_ERR(retimer->vdd33_cap_supply))
+		return dev_err_probe(dev, PTR_ERR(retimer->vdd33_cap_supply),
+				     "failed to get VDD CAP 3.3V\n");
+
+	retimer->vddat_supply = devm_regulator_get(dev, "vddat");
+	if (IS_ERR(retimer->vddat_supply))
+		return dev_err_probe(dev, PTR_ERR(retimer->vddat_supply),
+				     "failed to get VDD AT\n");
+
+	retimer->vddar_supply = devm_regulator_get(dev, "vddar");
+	if (IS_ERR(retimer->vddar_supply))
+		return dev_err_probe(dev, PTR_ERR(retimer->vddar_supply),
+				     "failed to get VDD AR\n");
+
+	retimer->vddio_supply = devm_regulator_get(dev, "vddio");
+	if (IS_ERR(retimer->vddio_supply))
+		return dev_err_probe(dev, PTR_ERR(retimer->vddio_supply),
+				     "failed to get VDD IO\n");
+
+	return 0;
+}
+
+static const struct regmap_config ps883x_retimer_regmap = {
+	.max_register = 0x1f,
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int ps883x_retimer_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct typec_switch_desc sw_desc = { };
+	struct typec_retimer_desc rtmr_desc = { };
+	struct ps883x_retimer *retimer;
+	int ret;
+
+	retimer = devm_kzalloc(dev, sizeof(*retimer), GFP_KERNEL);
+	if (!retimer)
+		return -ENOMEM;
+
+	retimer->client = client;
+
+	mutex_init(&retimer->lock);
+
+	retimer->regmap = devm_regmap_init_i2c(client, &ps883x_retimer_regmap);
+	if (IS_ERR(retimer->regmap))
+		return dev_err_probe(dev, PTR_ERR(retimer->regmap),
+				     "failed to allocate register map\n");
+
+	ret = ps883x_get_vregs(retimer);
+	if (ret)
+		return ret;
+
+	retimer->xo_clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(retimer->xo_clk))
+		return dev_err_probe(dev, PTR_ERR(retimer->xo_clk),
+				     "failed to get xo clock\n");
+
+	retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS);
+	if (IS_ERR(retimer->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(retimer->reset_gpio),
+				     "failed to get reset gpio\n");
+
+	retimer->typec_switch = typec_switch_get(dev);
+	if (IS_ERR(retimer->typec_switch))
+		return dev_err_probe(dev, PTR_ERR(retimer->typec_switch),
+				     "failed to acquire orientation-switch\n");
+
+	retimer->typec_mux = typec_mux_get(dev);
+	if (IS_ERR(retimer->typec_mux)) {
+		ret = dev_err_probe(dev, PTR_ERR(retimer->typec_mux),
+				    "failed to acquire mode-mux\n");
+		goto err_switch_put;
+	}
+
+	ret = drm_aux_bridge_register(dev);
+	if (ret)
+		goto err_mux_put;
+
+	ret = ps883x_enable_vregs(retimer);
+	if (ret)
+		goto err_mux_put;
+
+	ret = clk_prepare_enable(retimer->xo_clk);
+	if (ret) {
+		dev_err(dev, "failed to enable XO: %d\n", ret);
+		goto err_vregs_disable;
+	}
+
+	sw_desc.drvdata = retimer;
+	sw_desc.fwnode = dev_fwnode(dev);
+	sw_desc.set = ps883x_sw_set;
+
+	retimer->sw = typec_switch_register(dev, &sw_desc);
+	if (IS_ERR(retimer->sw)) {
+		ret = PTR_ERR(retimer->sw);
+		dev_err(dev, "failed to register typec switch: %d\n", ret);
+		goto err_clk_disable;
+	}
+
+	rtmr_desc.drvdata = retimer;
+	rtmr_desc.fwnode = dev_fwnode(dev);
+	rtmr_desc.set = ps883x_retimer_set;
+
+	retimer->retimer = typec_retimer_register(dev, &rtmr_desc);
+	if (IS_ERR(retimer->retimer)) {
+		ret = PTR_ERR(retimer->retimer);
+		dev_err(dev, "failed to register typec retimer: %d\n", ret);
+		goto err_switch_unregister;
+	}
+
+	/* skip resetting if already configured */
+	if (regmap_test_bits(retimer->regmap, REG_USB_PORT_CONN_STATUS_0,
+			     CONN_STATUS_0_CONNECTION_PRESENT) == 1)
+		return gpiod_direction_output(retimer->reset_gpio, 0);
+
+	gpiod_direction_output(retimer->reset_gpio, 1);
+
+	/* VDD IO supply enable to reset release delay */
+	usleep_range(4000, 14000);
+
+	gpiod_set_value(retimer->reset_gpio, 0);
+
+	/* firmware initialization delay */
+	msleep(60);
+
+	return 0;
+
+err_switch_unregister:
+	typec_switch_unregister(retimer->sw);
+err_vregs_disable:
+	ps883x_disable_vregs(retimer);
+err_clk_disable:
+	clk_disable_unprepare(retimer->xo_clk);
+err_mux_put:
+	typec_mux_put(retimer->typec_mux);
+err_switch_put:
+	typec_switch_put(retimer->typec_switch);
+
+	return ret;
+}
+
+static void ps883x_retimer_remove(struct i2c_client *client)
+{
+	struct ps883x_retimer *retimer = i2c_get_clientdata(client);
+
+	typec_retimer_unregister(retimer->retimer);
+	typec_switch_unregister(retimer->sw);
+
+	gpiod_set_value(retimer->reset_gpio, 1);
+
+	clk_disable_unprepare(retimer->xo_clk);
+
+	ps883x_disable_vregs(retimer);
+
+	typec_mux_put(retimer->typec_mux);
+	typec_switch_put(retimer->typec_switch);
+}
+
+static const struct of_device_id ps883x_retimer_of_table[] = {
+	{ .compatible = "parade,ps8830" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ps883x_retimer_of_table);
+
+static struct i2c_driver ps883x_retimer_driver = {
+	.driver = {
+		.name = "ps883x_retimer",
+		.of_match_table = ps883x_retimer_of_table,
+	},
+	.probe		= ps883x_retimer_probe,
+	.remove		= ps883x_retimer_remove,
+};
+
+module_i2c_driver(ps883x_retimer_driver);
+
+MODULE_DESCRIPTION("Parade ps883x Type-C Retimer driver");
+MODULE_LICENSE("GPL");

-- 
2.34.1

Re: [PATCH v6 1/2] dt-bindings: usb: Add Parade PS8830 Type-C retimer bindings

[Conor Dooley]

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

On Thu, Feb 06, 2025 at 11:28:27AM +0200, Abel Vesa wrote:
> The Parade PS8830 is a USB4, DisplayPort and Thunderbolt 4 retimer,
> controlled over I2C. It usually sits between a USB/DisplayPort PHY and the
> Type-C connector, and provides orientation and altmode handling.
> 
> Currently, it is found on all boards featuring the Qualcomm Snapdragon
> X Elite SoCs.
> 
> Document bindings for its new driver. Future-proof the schema for the
> PS8833 variant, which seems to be similar to PS8830.
> 
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>

This seems fine, but usb switches aren't something I know much about..
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

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

Re: [PATCH v6 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Heikki Krogerus]

On Thu, Feb 06, 2025 at 11:28:28AM +0200, Abel Vesa wrote:
> The Parade PS8830 is a USB4, DisplayPort and Thunderbolt 4 retimer,
> controlled over I2C. It usually sits between a USB/DisplayPort PHY
> and the Type-C connector, and provides orientation and altmode handling.
> 
> The boards that use this retimer are the ones featuring the Qualcomm
> Snapdragon X Elite SoCs.
> 
> Add a driver with support for the following modes:
>  - DisplayPort 4-lanes
>  - DisplayPort 2-lanes + USB3
>  - USB3
> 
> There is another variant of this retimer which is called PS8833. It seems
> to be really similar to the PS8830, so future-proof this driver by
> naming it ps883x.
> 
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/mux/Kconfig  |  10 +
>  drivers/usb/typec/mux/Makefile |   1 +
>  drivers/usb/typec/mux/ps883x.c | 437 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 448 insertions(+)
> 
> diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
> index 67381b4ef4f68f4a6e73f157365ee24d0ab7109a..6dd8f961b593261fde1d39b238b981966e463599 100644
> --- a/drivers/usb/typec/mux/Kconfig
> +++ b/drivers/usb/typec/mux/Kconfig
> @@ -56,6 +56,16 @@ config TYPEC_MUX_NB7VPQ904M
>  	  Say Y or M if your system has a On Semiconductor NB7VPQ904M Type-C
>  	  redriver chip found on some devices with a Type-C port.
>  
> +config TYPEC_MUX_PS883X
> +	tristate "Parade PS883x Type-C retimer driver"
> +	depends on I2C
> +	depends on DRM || DRM=n
> +	select DRM_AUX_BRIDGE if DRM_BRIDGE && OF
> +	select REGMAP_I2C
> +	help
> +	  Say Y or M if your system has a Parade PS883x Type-C retimer chip
> +	  found on some devices with a Type-C port.
> +
>  config TYPEC_MUX_PTN36502
>  	tristate "NXP PTN36502 Type-C redriver driver"
>  	depends on I2C
> diff --git a/drivers/usb/typec/mux/Makefile b/drivers/usb/typec/mux/Makefile
> index 60879446da9365183567d3374a2fb7b5171fb3d7..b4f599eb5053b8f20e9a41409b0a2d9a03d850b6 100644
> --- a/drivers/usb/typec/mux/Makefile
> +++ b/drivers/usb/typec/mux/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_TYPEC_MUX_PI3USB30532)	+= pi3usb30532.o
>  obj-$(CONFIG_TYPEC_MUX_INTEL_PMC)	+= intel_pmc_mux.o
>  obj-$(CONFIG_TYPEC_MUX_IT5205)		+= it5205.o
>  obj-$(CONFIG_TYPEC_MUX_NB7VPQ904M)	+= nb7vpq904m.o
> +obj-$(CONFIG_TYPEC_MUX_PS883X)		+= ps883x.o
>  obj-$(CONFIG_TYPEC_MUX_PTN36502)	+= ptn36502.o
>  obj-$(CONFIG_TYPEC_MUX_TUSB1046)	+= tusb1046.o
>  obj-$(CONFIG_TYPEC_MUX_WCD939X_USBSS)	+= wcd939x-usbss.o
> diff --git a/drivers/usb/typec/mux/ps883x.c b/drivers/usb/typec/mux/ps883x.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..10e407ab6b7f6ce3c6d4a61a2d6219eb1b9e85e8
> --- /dev/null
> +++ b/drivers/usb/typec/mux/ps883x.c
> @@ -0,0 +1,437 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Parade ps883x usb retimer driver
> + *
> + * Copyright (C) 2024 Linaro Ltd.
> + */
> +
> +#include <drm/bridge/aux-bridge.h>
> +#include <linux/clk.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/usb/typec_altmode.h>
> +#include <linux/usb/typec_dp.h>
> +#include <linux/usb/typec_mux.h>
> +#include <linux/usb/typec_retimer.h>
> +
> +#define REG_USB_PORT_CONN_STATUS_0		0x00
> +
> +#define CONN_STATUS_0_CONNECTION_PRESENT	BIT(0)
> +#define CONN_STATUS_0_ORIENTATION_REVERSED	BIT(1)
> +#define CONN_STATUS_0_USB_3_1_CONNECTED		BIT(5)
> +
> +#define REG_USB_PORT_CONN_STATUS_1		0x01
> +
> +#define CONN_STATUS_1_DP_CONNECTED		BIT(0)
> +#define CONN_STATUS_1_DP_SINK_REQUESTED		BIT(1)
> +#define CONN_STATUS_1_DP_PIN_ASSIGNMENT_C_D	BIT(2)
> +#define CONN_STATUS_1_DP_HPD_LEVEL		BIT(7)
> +
> +#define REG_USB_PORT_CONN_STATUS_2		0x02
> +
> +struct ps883x_retimer {
> +	struct i2c_client *client;
> +	struct gpio_desc *reset_gpio;
> +	struct regmap *regmap;
> +	struct typec_switch_dev *sw;
> +	struct typec_retimer *retimer;
> +	struct clk *xo_clk;
> +	struct regulator *vdd_supply;
> +	struct regulator *vdd33_supply;
> +	struct regulator *vdd33_cap_supply;
> +	struct regulator *vddat_supply;
> +	struct regulator *vddar_supply;
> +	struct regulator *vddio_supply;
> +
> +	struct typec_switch *typec_switch;
> +	struct typec_mux *typec_mux;
> +
> +	struct mutex lock; /* protect non-concurrent retimer & switch */
> +
> +	enum typec_orientation orientation;
> +	unsigned long mode;
> +	unsigned int svid;
> +};
> +
> +static void ps883x_configure(struct ps883x_retimer *retimer, int cfg0,
> +			     int cfg1, int cfg2)
> +{
> +	regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_0, cfg0);
> +	regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_1, cfg1);
> +	regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_2, cfg2);
> +}
> +
> +static int ps883x_set(struct ps883x_retimer *retimer)
> +{
> +	int cfg0 = CONN_STATUS_0_CONNECTION_PRESENT;
> +	int cfg1 = 0x00;
> +	int cfg2 = 0x00;
> +
> +	if (retimer->orientation == TYPEC_ORIENTATION_NONE ||
> +	    retimer->mode == TYPEC_STATE_SAFE) {
> +		ps883x_configure(retimer, cfg0, cfg1, cfg2);
> +		return 0;
> +	}
> +
> +	if (retimer->mode != TYPEC_STATE_USB && retimer->svid != USB_TYPEC_DP_SID)
> +		return -EINVAL;
> +
> +	if (retimer->orientation == TYPEC_ORIENTATION_REVERSE)
> +		cfg0 |= CONN_STATUS_0_ORIENTATION_REVERSED;
> +
> +	switch (retimer->mode) {
> +	case TYPEC_STATE_USB:
> +		cfg0 |= CONN_STATUS_0_USB_3_1_CONNECTED;
> +		break;
> +
> +	case TYPEC_DP_STATE_C:
> +		cfg1 = CONN_STATUS_1_DP_CONNECTED |
> +		       CONN_STATUS_1_DP_SINK_REQUESTED |
> +		       CONN_STATUS_1_DP_PIN_ASSIGNMENT_C_D |
> +		       CONN_STATUS_1_DP_HPD_LEVEL;
> +		break;
> +
> +	case TYPEC_DP_STATE_D:
> +		cfg0 |= CONN_STATUS_0_USB_3_1_CONNECTED;
> +		cfg1 = CONN_STATUS_1_DP_CONNECTED |
> +		       CONN_STATUS_1_DP_SINK_REQUESTED |
> +		       CONN_STATUS_1_DP_PIN_ASSIGNMENT_C_D |
> +		       CONN_STATUS_1_DP_HPD_LEVEL;
> +		break;
> +
> +	case TYPEC_DP_STATE_E:
> +		cfg1 = CONN_STATUS_1_DP_CONNECTED |
> +		       CONN_STATUS_1_DP_HPD_LEVEL;
> +		break;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ps883x_configure(retimer, cfg0, cfg1, cfg2);
> +
> +	return 0;
> +}
> +
> +static int ps883x_sw_set(struct typec_switch_dev *sw,
> +			 enum typec_orientation orientation)
> +{
> +	struct ps883x_retimer *retimer = typec_switch_get_drvdata(sw);
> +	int ret = 0;
> +
> +	ret = typec_switch_set(retimer->typec_switch, orientation);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&retimer->lock);
> +
> +	if (retimer->orientation != orientation) {
> +		retimer->orientation = orientation;
> +
> +		ret = ps883x_set(retimer);
> +	}
> +
> +	mutex_unlock(&retimer->lock);
> +
> +	return ret;
> +}
> +
> +static int ps883x_retimer_set(struct typec_retimer *rtmr,
> +			      struct typec_retimer_state *state)
> +{
> +	struct ps883x_retimer *retimer = typec_retimer_get_drvdata(rtmr);
> +	struct typec_mux_state mux_state;
> +	int ret = 0;
> +
> +	mutex_lock(&retimer->lock);
> +
> +	if (state->mode != retimer->mode) {
> +		retimer->mode = state->mode;
> +
> +		if (state->alt)
> +			retimer->svid = state->alt->svid;
> +		else
> +			retimer->svid = 0;
> +
> +		ret = ps883x_set(retimer);
> +	}
> +
> +	mutex_unlock(&retimer->lock);
> +
> +	if (ret)
> +		return ret;
> +
> +	mux_state.alt = state->alt;
> +	mux_state.data = state->data;
> +	mux_state.mode = state->mode;
> +
> +	return typec_mux_set(retimer->typec_mux, &mux_state);
> +}
> +
> +static int ps883x_enable_vregs(struct ps883x_retimer *retimer)
> +{
> +	struct device *dev = &retimer->client->dev;
> +	int ret;
> +
> +	ret = regulator_enable(retimer->vdd33_supply);
> +	if (ret) {
> +		dev_err(dev, "cannot enable VDD 3.3V regulator: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(retimer->vdd33_cap_supply);
> +	if (ret) {
> +		dev_err(dev, "cannot enable VDD 3.3V CAP regulator: %d\n", ret);
> +		goto err_vdd33_disable;
> +	}
> +
> +	usleep_range(4000, 10000);
> +
> +	ret = regulator_enable(retimer->vdd_supply);
> +	if (ret) {
> +		dev_err(dev, "cannot enable VDD regulator: %d\n", ret);
> +		goto err_vdd33_cap_disable;
> +	}
> +
> +	ret = regulator_enable(retimer->vddar_supply);
> +	if (ret) {
> +		dev_err(dev, "cannot enable VDD AR regulator: %d\n", ret);
> +		goto err_vdd_disable;
> +	}
> +
> +	ret = regulator_enable(retimer->vddat_supply);
> +	if (ret) {
> +		dev_err(dev, "cannot enable VDD AT regulator: %d\n", ret);
> +		goto err_vddar_disable;
> +	}
> +
> +	ret = regulator_enable(retimer->vddio_supply);
> +	if (ret) {
> +		dev_err(dev, "cannot enable VDD IO regulator: %d\n", ret);
> +		goto err_vddat_disable;
> +	}
> +
> +	return 0;
> +
> +err_vddat_disable:
> +	regulator_disable(retimer->vddat_supply);
> +err_vddar_disable:
> +	regulator_disable(retimer->vddar_supply);
> +err_vdd_disable:
> +	regulator_disable(retimer->vdd_supply);
> +err_vdd33_cap_disable:
> +	regulator_disable(retimer->vdd33_cap_supply);
> +err_vdd33_disable:
> +	regulator_disable(retimer->vdd33_supply);
> +
> +	return ret;
> +}
> +
> +static void ps883x_disable_vregs(struct ps883x_retimer *retimer)
> +{
> +	regulator_disable(retimer->vddio_supply);
> +	regulator_disable(retimer->vddat_supply);
> +	regulator_disable(retimer->vddar_supply);
> +	regulator_disable(retimer->vdd_supply);
> +	regulator_disable(retimer->vdd33_cap_supply);
> +	regulator_disable(retimer->vdd33_supply);
> +}
> +
> +static int ps883x_get_vregs(struct ps883x_retimer *retimer)
> +{
> +	struct device *dev = &retimer->client->dev;
> +
> +	retimer->vdd_supply = devm_regulator_get(dev, "vdd");
> +	if (IS_ERR(retimer->vdd_supply))
> +		return dev_err_probe(dev, PTR_ERR(retimer->vdd_supply),
> +				     "failed to get VDD\n");
> +
> +	retimer->vdd33_supply = devm_regulator_get(dev, "vdd33");
> +	if (IS_ERR(retimer->vdd33_supply))
> +		return dev_err_probe(dev, PTR_ERR(retimer->vdd33_supply),
> +				     "failed to get VDD 3.3V\n");
> +
> +	retimer->vdd33_cap_supply = devm_regulator_get(dev, "vdd33-cap");
> +	if (IS_ERR(retimer->vdd33_cap_supply))
> +		return dev_err_probe(dev, PTR_ERR(retimer->vdd33_cap_supply),
> +				     "failed to get VDD CAP 3.3V\n");
> +
> +	retimer->vddat_supply = devm_regulator_get(dev, "vddat");
> +	if (IS_ERR(retimer->vddat_supply))
> +		return dev_err_probe(dev, PTR_ERR(retimer->vddat_supply),
> +				     "failed to get VDD AT\n");
> +
> +	retimer->vddar_supply = devm_regulator_get(dev, "vddar");
> +	if (IS_ERR(retimer->vddar_supply))
> +		return dev_err_probe(dev, PTR_ERR(retimer->vddar_supply),
> +				     "failed to get VDD AR\n");
> +
> +	retimer->vddio_supply = devm_regulator_get(dev, "vddio");
> +	if (IS_ERR(retimer->vddio_supply))
> +		return dev_err_probe(dev, PTR_ERR(retimer->vddio_supply),
> +				     "failed to get VDD IO\n");
> +
> +	return 0;
> +}
> +
> +static const struct regmap_config ps883x_retimer_regmap = {
> +	.max_register = 0x1f,
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static int ps883x_retimer_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct typec_switch_desc sw_desc = { };
> +	struct typec_retimer_desc rtmr_desc = { };
> +	struct ps883x_retimer *retimer;
> +	int ret;
> +
> +	retimer = devm_kzalloc(dev, sizeof(*retimer), GFP_KERNEL);
> +	if (!retimer)
> +		return -ENOMEM;
> +
> +	retimer->client = client;
> +
> +	mutex_init(&retimer->lock);
> +
> +	retimer->regmap = devm_regmap_init_i2c(client, &ps883x_retimer_regmap);
> +	if (IS_ERR(retimer->regmap))
> +		return dev_err_probe(dev, PTR_ERR(retimer->regmap),
> +				     "failed to allocate register map\n");
> +
> +	ret = ps883x_get_vregs(retimer);
> +	if (ret)
> +		return ret;
> +
> +	retimer->xo_clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(retimer->xo_clk))
> +		return dev_err_probe(dev, PTR_ERR(retimer->xo_clk),
> +				     "failed to get xo clock\n");
> +
> +	retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS);
> +	if (IS_ERR(retimer->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(retimer->reset_gpio),
> +				     "failed to get reset gpio\n");
> +
> +	retimer->typec_switch = typec_switch_get(dev);
> +	if (IS_ERR(retimer->typec_switch))
> +		return dev_err_probe(dev, PTR_ERR(retimer->typec_switch),
> +				     "failed to acquire orientation-switch\n");
> +
> +	retimer->typec_mux = typec_mux_get(dev);
> +	if (IS_ERR(retimer->typec_mux)) {
> +		ret = dev_err_probe(dev, PTR_ERR(retimer->typec_mux),
> +				    "failed to acquire mode-mux\n");
> +		goto err_switch_put;
> +	}
> +
> +	ret = drm_aux_bridge_register(dev);
> +	if (ret)
> +		goto err_mux_put;
> +
> +	ret = ps883x_enable_vregs(retimer);
> +	if (ret)
> +		goto err_mux_put;
> +
> +	ret = clk_prepare_enable(retimer->xo_clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable XO: %d\n", ret);
> +		goto err_vregs_disable;
> +	}
> +
> +	sw_desc.drvdata = retimer;
> +	sw_desc.fwnode = dev_fwnode(dev);
> +	sw_desc.set = ps883x_sw_set;
> +
> +	retimer->sw = typec_switch_register(dev, &sw_desc);
> +	if (IS_ERR(retimer->sw)) {
> +		ret = PTR_ERR(retimer->sw);
> +		dev_err(dev, "failed to register typec switch: %d\n", ret);
> +		goto err_clk_disable;
> +	}
> +
> +	rtmr_desc.drvdata = retimer;
> +	rtmr_desc.fwnode = dev_fwnode(dev);
> +	rtmr_desc.set = ps883x_retimer_set;
> +
> +	retimer->retimer = typec_retimer_register(dev, &rtmr_desc);
> +	if (IS_ERR(retimer->retimer)) {
> +		ret = PTR_ERR(retimer->retimer);
> +		dev_err(dev, "failed to register typec retimer: %d\n", ret);
> +		goto err_switch_unregister;
> +	}
> +
> +	/* skip resetting if already configured */
> +	if (regmap_test_bits(retimer->regmap, REG_USB_PORT_CONN_STATUS_0,
> +			     CONN_STATUS_0_CONNECTION_PRESENT) == 1)
> +		return gpiod_direction_output(retimer->reset_gpio, 0);
> +
> +	gpiod_direction_output(retimer->reset_gpio, 1);
> +
> +	/* VDD IO supply enable to reset release delay */
> +	usleep_range(4000, 14000);
> +
> +	gpiod_set_value(retimer->reset_gpio, 0);
> +
> +	/* firmware initialization delay */
> +	msleep(60);
> +
> +	return 0;
> +
> +err_switch_unregister:
> +	typec_switch_unregister(retimer->sw);
> +err_vregs_disable:
> +	ps883x_disable_vregs(retimer);
> +err_clk_disable:
> +	clk_disable_unprepare(retimer->xo_clk);
> +err_mux_put:
> +	typec_mux_put(retimer->typec_mux);
> +err_switch_put:
> +	typec_switch_put(retimer->typec_switch);
> +
> +	return ret;
> +}
> +
> +static void ps883x_retimer_remove(struct i2c_client *client)
> +{
> +	struct ps883x_retimer *retimer = i2c_get_clientdata(client);
> +
> +	typec_retimer_unregister(retimer->retimer);
> +	typec_switch_unregister(retimer->sw);
> +
> +	gpiod_set_value(retimer->reset_gpio, 1);
> +
> +	clk_disable_unprepare(retimer->xo_clk);
> +
> +	ps883x_disable_vregs(retimer);
> +
> +	typec_mux_put(retimer->typec_mux);
> +	typec_switch_put(retimer->typec_switch);
> +}
> +
> +static const struct of_device_id ps883x_retimer_of_table[] = {
> +	{ .compatible = "parade,ps8830" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ps883x_retimer_of_table);
> +
> +static struct i2c_driver ps883x_retimer_driver = {
> +	.driver = {
> +		.name = "ps883x_retimer",
> +		.of_match_table = ps883x_retimer_of_table,
> +	},
> +	.probe		= ps883x_retimer_probe,
> +	.remove		= ps883x_retimer_remove,
> +};
> +
> +module_i2c_driver(ps883x_retimer_driver);
> +
> +MODULE_DESCRIPTION("Parade ps883x Type-C Retimer driver");
> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.34.1

-- 
heikki

Re: [PATCH v6 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Johan Hovold]

On Thu, Feb 06, 2025 at 11:28:28AM +0200, Abel Vesa wrote:
> The Parade PS8830 is a USB4, DisplayPort and Thunderbolt 4 retimer,
> controlled over I2C. It usually sits between a USB/DisplayPort PHY
> and the Type-C connector, and provides orientation and altmode handling.

> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Parade ps883x usb retimer driver

Nit: USB

> + *
> + * Copyright (C) 2024 Linaro Ltd.
> + */

> +static int ps883x_set(struct ps883x_retimer *retimer)
> +{
> +	int cfg0 = CONN_STATUS_0_CONNECTION_PRESENT;
> +	int cfg1 = 0x00;
> +	int cfg2 = 0x00;
> +
> +	if (retimer->orientation == TYPEC_ORIENTATION_NONE ||
> +	    retimer->mode == TYPEC_STATE_SAFE) {
> +		ps883x_configure(retimer, cfg0, cfg1, cfg2);
> +		return 0;
> +	}
> +
> +	if (retimer->mode != TYPEC_STATE_USB && retimer->svid != USB_TYPEC_DP_SID)
> +		return -EINVAL;
> +
> +	if (retimer->orientation == TYPEC_ORIENTATION_REVERSE)
> +		cfg0 |= CONN_STATUS_0_ORIENTATION_REVERSED;
> +
> +	switch (retimer->mode) {
> +	case TYPEC_STATE_USB:
> +		cfg0 |= CONN_STATUS_0_USB_3_1_CONNECTED;
> +		break;
> +

I'd drop these newlines before case statements, but your choice.

> +	case TYPEC_DP_STATE_C:
> +		cfg1 = CONN_STATUS_1_DP_CONNECTED |
> +		       CONN_STATUS_1_DP_SINK_REQUESTED |
> +		       CONN_STATUS_1_DP_PIN_ASSIGNMENT_C_D |
> +		       CONN_STATUS_1_DP_HPD_LEVEL;
> +		break;
> +
> +	case TYPEC_DP_STATE_D:
> +		cfg0 |= CONN_STATUS_0_USB_3_1_CONNECTED;
> +		cfg1 = CONN_STATUS_1_DP_CONNECTED |
> +		       CONN_STATUS_1_DP_SINK_REQUESTED |
> +		       CONN_STATUS_1_DP_PIN_ASSIGNMENT_C_D |
> +		       CONN_STATUS_1_DP_HPD_LEVEL;
> +		break;
> +
> +	case TYPEC_DP_STATE_E:
> +		cfg1 = CONN_STATUS_1_DP_CONNECTED |
> +		       CONN_STATUS_1_DP_HPD_LEVEL;
> +		break;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}

> +static int ps883x_retimer_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct typec_switch_desc sw_desc = { };
> +	struct typec_retimer_desc rtmr_desc = { };
> +	struct ps883x_retimer *retimer;
> +	int ret;
> +
> +	retimer = devm_kzalloc(dev, sizeof(*retimer), GFP_KERNEL);
> +	if (!retimer)
> +		return -ENOMEM;
> +
> +	retimer->client = client;
> +
> +	mutex_init(&retimer->lock);
> +
> +	retimer->regmap = devm_regmap_init_i2c(client, &ps883x_retimer_regmap);
> +	if (IS_ERR(retimer->regmap))
> +		return dev_err_probe(dev, PTR_ERR(retimer->regmap),
> +				     "failed to allocate register map\n");
> +
> +	ret = ps883x_get_vregs(retimer);
> +	if (ret)
> +		return ret;
> +
> +	retimer->xo_clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(retimer->xo_clk))
> +		return dev_err_probe(dev, PTR_ERR(retimer->xo_clk),
> +				     "failed to get xo clock\n");
> +
> +	retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS);

What if the reset pin has not been configured by the boot firmware? Then
this input the to device will be floating when you power it on,
something which you'd typically try to avoid by asserting reset before
enabling power.

> +	if (IS_ERR(retimer->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(retimer->reset_gpio),
> +				     "failed to get reset gpio\n");
> +
> +	retimer->typec_switch = typec_switch_get(dev);
> +	if (IS_ERR(retimer->typec_switch))
> +		return dev_err_probe(dev, PTR_ERR(retimer->typec_switch),
> +				     "failed to acquire orientation-switch\n");
> +
> +	retimer->typec_mux = typec_mux_get(dev);
> +	if (IS_ERR(retimer->typec_mux)) {
> +		ret = dev_err_probe(dev, PTR_ERR(retimer->typec_mux),
> +				    "failed to acquire mode-mux\n");
> +		goto err_switch_put;
> +	}
> +
> +	ret = drm_aux_bridge_register(dev);
> +	if (ret)
> +		goto err_mux_put;
> +
> +	ret = ps883x_enable_vregs(retimer);
> +	if (ret)
> +		goto err_mux_put;
> +
> +	ret = clk_prepare_enable(retimer->xo_clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable XO: %d\n", ret);
> +		goto err_vregs_disable;
> +	}
> +
> +	sw_desc.drvdata = retimer;
> +	sw_desc.fwnode = dev_fwnode(dev);
> +	sw_desc.set = ps883x_sw_set;
> +
> +	retimer->sw = typec_switch_register(dev, &sw_desc);
> +	if (IS_ERR(retimer->sw)) {
> +		ret = PTR_ERR(retimer->sw);
> +		dev_err(dev, "failed to register typec switch: %d\n", ret);
> +		goto err_clk_disable;
> +	}
> +
> +	rtmr_desc.drvdata = retimer;
> +	rtmr_desc.fwnode = dev_fwnode(dev);
> +	rtmr_desc.set = ps883x_retimer_set;
> +
> +	retimer->retimer = typec_retimer_register(dev, &rtmr_desc);
> +	if (IS_ERR(retimer->retimer)) {
> +		ret = PTR_ERR(retimer->retimer);
> +		dev_err(dev, "failed to register typec retimer: %d\n", ret);
> +		goto err_switch_unregister;
> +	}
> +
> +	/* skip resetting if already configured */
> +	if (regmap_test_bits(retimer->regmap, REG_USB_PORT_CONN_STATUS_0,
> +			     CONN_STATUS_0_CONNECTION_PRESENT) == 1)
> +		return gpiod_direction_output(retimer->reset_gpio, 0);

I'm still a little concerned about this. Won't you end up with i2c
timeout errors in the logs if the device is held in reset before probe?

Have you tried unbinding the device and rebinding to test this?

And what about the CONN_STATUS_0_CONNECTION_PRESENT bit; it sounds like
it just reflects the connected status. Are you sure it will not be set
for a device that has not yet been configured?

> +
> +	gpiod_direction_output(retimer->reset_gpio, 1);
> +
> +	/* VDD IO supply enable to reset release delay */
> +	usleep_range(4000, 14000);
> +
> +	gpiod_set_value(retimer->reset_gpio, 0);
> +
> +	/* firmware initialization delay */
> +	msleep(60);
> +
> +	return 0;
> +
> +err_switch_unregister:
> +	typec_switch_unregister(retimer->sw);
> +err_vregs_disable:
> +	ps883x_disable_vregs(retimer);
> +err_clk_disable:
> +	clk_disable_unprepare(retimer->xo_clk);

This one should go above err_vregs_disable or can end up with an
unbalanced clock disable or regulators left on after probe failure.

And you should assert reset before disabling clocks as well to avoid
driving the pin after disabling power.

> +err_mux_put:
> +	typec_mux_put(retimer->typec_mux);
> +err_switch_put:
> +	typec_switch_put(retimer->typec_switch);
> +
> +	return ret;
> +}
> +
> +static void ps883x_retimer_remove(struct i2c_client *client)
> +{
> +	struct ps883x_retimer *retimer = i2c_get_clientdata(client);
> +
> +	typec_retimer_unregister(retimer->retimer);
> +	typec_switch_unregister(retimer->sw);
> +
> +	gpiod_set_value(retimer->reset_gpio, 1);
> +
> +	clk_disable_unprepare(retimer->xo_clk);
> +
> +	ps883x_disable_vregs(retimer);
> +
> +	typec_mux_put(retimer->typec_mux);
> +	typec_switch_put(retimer->typec_switch);
> +}

Johan

Re: [PATCH v6 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Johan Hovold]

On Fri, Feb 14, 2025 at 09:52:33AM +0100, Johan Hovold wrote:
> On Thu, Feb 06, 2025 at 11:28:28AM +0200, Abel Vesa wrote:
> > The Parade PS8830 is a USB4, DisplayPort and Thunderbolt 4 retimer,
> > controlled over I2C. It usually sits between a USB/DisplayPort PHY
> > and the Type-C connector, and provides orientation and altmode handling.

Turns out Greg had picked this one up 20 minutes before I sent my
comments. I did see Heikki's ack the day before and realised time was
short but was not able to drop everything and review the last revision
due to meetings that afternoon.

Well, well, I guess you can say I only have myself to blame for not
reviewing within a week of the last revision being posted.

> > +static int ps883x_retimer_probe(struct i2c_client *client)
> > +{
> > +	struct device *dev = &client->dev;
> > +	struct typec_switch_desc sw_desc = { };
> > +	struct typec_retimer_desc rtmr_desc = { };
> > +	struct ps883x_retimer *retimer;
> > +	int ret;
> > +
> > +	retimer = devm_kzalloc(dev, sizeof(*retimer), GFP_KERNEL);
> > +	if (!retimer)
> > +		return -ENOMEM;
> > +
> > +	retimer->client = client;
> > +
> > +	mutex_init(&retimer->lock);
> > +
> > +	retimer->regmap = devm_regmap_init_i2c(client, &ps883x_retimer_regmap);
> > +	if (IS_ERR(retimer->regmap))
> > +		return dev_err_probe(dev, PTR_ERR(retimer->regmap),
> > +				     "failed to allocate register map\n");
> > +
> > +	ret = ps883x_get_vregs(retimer);
> > +	if (ret)
> > +		return ret;
> > +
> > +	retimer->xo_clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(retimer->xo_clk))
> > +		return dev_err_probe(dev, PTR_ERR(retimer->xo_clk),
> > +				     "failed to get xo clock\n");
> > +
> > +	retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS);
> 
> What if the reset pin has not been configured by the boot firmware? Then
> this input the to device will be floating when you power it on,
> something which you'd typically try to avoid by asserting reset before
> enabling power.
> 
> > +	if (IS_ERR(retimer->reset_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(retimer->reset_gpio),
> > +				     "failed to get reset gpio\n");
> > +
> > +	retimer->typec_switch = typec_switch_get(dev);
> > +	if (IS_ERR(retimer->typec_switch))
> > +		return dev_err_probe(dev, PTR_ERR(retimer->typec_switch),
> > +				     "failed to acquire orientation-switch\n");
> > +
> > +	retimer->typec_mux = typec_mux_get(dev);
> > +	if (IS_ERR(retimer->typec_mux)) {
> > +		ret = dev_err_probe(dev, PTR_ERR(retimer->typec_mux),
> > +				    "failed to acquire mode-mux\n");
> > +		goto err_switch_put;
> > +	}
> > +
> > +	ret = drm_aux_bridge_register(dev);
> > +	if (ret)
> > +		goto err_mux_put;
> > +
> > +	ret = ps883x_enable_vregs(retimer);
> > +	if (ret)
> > +		goto err_mux_put;
> > +
> > +	ret = clk_prepare_enable(retimer->xo_clk);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable XO: %d\n", ret);
> > +		goto err_vregs_disable;
> > +	}
> > +
> > +	sw_desc.drvdata = retimer;
> > +	sw_desc.fwnode = dev_fwnode(dev);
> > +	sw_desc.set = ps883x_sw_set;
> > +
> > +	retimer->sw = typec_switch_register(dev, &sw_desc);
> > +	if (IS_ERR(retimer->sw)) {
> > +		ret = PTR_ERR(retimer->sw);
> > +		dev_err(dev, "failed to register typec switch: %d\n", ret);
> > +		goto err_clk_disable;
> > +	}
> > +
> > +	rtmr_desc.drvdata = retimer;
> > +	rtmr_desc.fwnode = dev_fwnode(dev);
> > +	rtmr_desc.set = ps883x_retimer_set;
> > +
> > +	retimer->retimer = typec_retimer_register(dev, &rtmr_desc);
> > +	if (IS_ERR(retimer->retimer)) {
> > +		ret = PTR_ERR(retimer->retimer);
> > +		dev_err(dev, "failed to register typec retimer: %d\n", ret);
> > +		goto err_switch_unregister;
> > +	}
> > +
> > +	/* skip resetting if already configured */
> > +	if (regmap_test_bits(retimer->regmap, REG_USB_PORT_CONN_STATUS_0,
> > +			     CONN_STATUS_0_CONNECTION_PRESENT) == 1)
> > +		return gpiod_direction_output(retimer->reset_gpio, 0);
> 
> I'm still a little concerned about this. Won't you end up with i2c
> timeout errors in the logs if the device is held in reset before probe?

You should be able to use i2c_smbus_read_byte() to avoid logging errors
when the boot firmware has *not* enabled the device.

My worry is that this perfectly valid case has not even been tested,
and worst case you may need a device-tree property to fully determine
whether the device has been initialised by the boot firmware or not (cf.
'regulator-boot-on').

And if we need binding changes those need to be there from the start.

I guess we have a few weeks to work this out, and if needed we can
always disable the driver temporarily.

> Have you tried unbinding the device and rebinding to test this?
> 
> And what about the CONN_STATUS_0_CONNECTION_PRESENT bit; it sounds like
> it just reflects the connected status. Are you sure it will not be set
> for a device that has not yet been configured?

> > +err_switch_unregister:
> > +	typec_switch_unregister(retimer->sw);
> > +err_vregs_disable:
> > +	ps883x_disable_vregs(retimer);
> > +err_clk_disable:
> > +	clk_disable_unprepare(retimer->xo_clk);
> 
> This one should go above err_vregs_disable or can end up with an
> unbalanced clock disable or regulators left on after probe failure.
> 
> And you should assert reset before disabling clocks as well to avoid
> driving the pin after disabling power.

I'll send an incremental fix for this.

Johan

Re: [PATCH v6 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Greg Kroah-Hartman]

On Tue, Feb 18, 2025 at 08:54:47AM +0100, Johan Hovold wrote:
> On Fri, Feb 14, 2025 at 09:52:33AM +0100, Johan Hovold wrote:
> > On Thu, Feb 06, 2025 at 11:28:28AM +0200, Abel Vesa wrote:
> > > The Parade PS8830 is a USB4, DisplayPort and Thunderbolt 4 retimer,
> > > controlled over I2C. It usually sits between a USB/DisplayPort PHY
> > > and the Type-C connector, and provides orientation and altmode handling.
> 
> Turns out Greg had picked this one up 20 minutes before I sent my
> comments. I did see Heikki's ack the day before and realised time was
> short but was not able to drop everything and review the last revision
> due to meetings that afternoon.
> 
> Well, well, I guess you can say I only have myself to blame for not
> reviewing within a week of the last revision being posted.

I can revert it if you want me to, or an incremental fix, your call.

thanks,

greg k-h

Re: [PATCH v6 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Johan Hovold]

On Tue, Feb 18, 2025 at 09:00:27AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 18, 2025 at 08:54:47AM +0100, Johan Hovold wrote:
> > On Fri, Feb 14, 2025 at 09:52:33AM +0100, Johan Hovold wrote:
> > > On Thu, Feb 06, 2025 at 11:28:28AM +0200, Abel Vesa wrote:
> > > > The Parade PS8830 is a USB4, DisplayPort and Thunderbolt 4 retimer,
> > > > controlled over I2C. It usually sits between a USB/DisplayPort PHY
> > > > and the Type-C connector, and provides orientation and altmode handling.
> > 
> > Turns out Greg had picked this one up 20 minutes before I sent my
> > comments. I did see Heikki's ack the day before and realised time was
> > short but was not able to drop everything and review the last revision
> > due to meetings that afternoon.
> > 
> > Well, well, I guess you can say I only have myself to blame for not
> > reviewing within a week of the last revision being posted.
> 
> I can revert it if you want me to, or an incremental fix, your call.

Thanks, but I think Abel should be able to test the
not-enabled-by-boot-firmware case and if needed amend the binding these
next few weeks.

I'll just send an incremental fix for the error handling for now.

And sorry about the rant, just needed to vent some frustration with the
never ending stream of interruptions and unscheduled work...

Johan

Re: [PATCH v6 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Stephan Gerhold]

On Tue, Feb 18, 2025 at 08:54:47AM +0100, Johan Hovold wrote:
> On Fri, Feb 14, 2025 at 09:52:33AM +0100, Johan Hovold wrote:
> > On Thu, Feb 06, 2025 at 11:28:28AM +0200, Abel Vesa wrote:
> > > The Parade PS8830 is a USB4, DisplayPort and Thunderbolt 4 retimer,
> > > controlled over I2C. It usually sits between a USB/DisplayPort PHY
> > > and the Type-C connector, and provides orientation and altmode handling.
> [...]
> > > +	/* skip resetting if already configured */
> > > +	if (regmap_test_bits(retimer->regmap, REG_USB_PORT_CONN_STATUS_0,
> > > +			     CONN_STATUS_0_CONNECTION_PRESENT) == 1)
> > > +		return gpiod_direction_output(retimer->reset_gpio, 0);
> > 
> > I'm still a little concerned about this. Won't you end up with i2c
> > timeout errors in the logs if the device is held in reset before probe?
> 
> You should be able to use i2c_smbus_read_byte() to avoid logging errors
> when the boot firmware has *not* enabled the device.
> 

FWIW, regmap_test_bits() doesn't seem to print any errors either, so I
don't think switching to i2c_smbus_read_byte() is necessary.

Since I was curious, I tried booting the X1E80100 with
 1. One PS8830 instance left as-is
 2. One PS8830 instance changed to invalid I2C address
 3. One PS8830 instance changed to have reset pin asserted via pinctrl

There are no errors whatsoever, even for the one with invalid I2C
address. In other words, the slightly more concerning part is that the
driver doesn't check that any of the regmap reads/writes actually
succeed.

The diff I used for testing is below. (1) prints "skipping reset", (2)
and (3) print "continuing reset".

Thanks,
Stephan

diff --git a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
index fee694a364ea..1f8d61239723 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
+++ b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
@@ -1010,9 +1010,9 @@ &i2c1 {
 
 	status = "okay";
 
-	typec-mux@8 {
+	typec-mux@42 {
 		compatible = "parade,ps8830";
-		reg = <0x08>;
+		reg = <0x42>;
 
 		clocks = <&rpmhcc RPMH_RF_CLK5>;
 
@@ -1673,6 +1673,7 @@ rtmr1_default: rtmr1-reset-n-active-state {
 		function = "gpio";
 		drive-strength = <2>;
 		bias-disable;
+		output-low;
 	};
 
 	rtmr2_default: rtmr2-reset-n-active-state {
diff --git a/drivers/usb/typec/mux/ps883x.c b/drivers/usb/typec/mux/ps883x.c
index 10e407ab6b7f..04ed35d14fd6 100644
--- a/drivers/usb/typec/mux/ps883x.c
+++ b/drivers/usb/typec/mux/ps883x.c
@@ -370,8 +370,12 @@ static int ps883x_retimer_probe(struct i2c_client *client)
 
 	/* skip resetting if already configured */
 	if (regmap_test_bits(retimer->regmap, REG_USB_PORT_CONN_STATUS_0,
-			     CONN_STATUS_0_CONNECTION_PRESENT) == 1)
+			     CONN_STATUS_0_CONNECTION_PRESENT) == 1) {
+		dev_info(dev, "skipping reset\n");
 		return gpiod_direction_output(retimer->reset_gpio, 0);
+	} else {
+		dev_info(dev, "continuing reset\n");
+	}
 
 	gpiod_direction_output(retimer->reset_gpio, 1);
 

Re: [PATCH v6 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Johan Hovold]

On Tue, Feb 18, 2025 at 10:26:58AM +0100, Stephan Gerhold wrote:
> On Tue, Feb 18, 2025 at 08:54:47AM +0100, Johan Hovold wrote:
> > On Fri, Feb 14, 2025 at 09:52:33AM +0100, Johan Hovold wrote:
> > > On Thu, Feb 06, 2025 at 11:28:28AM +0200, Abel Vesa wrote:
> > > > The Parade PS8830 is a USB4, DisplayPort and Thunderbolt 4 retimer,
> > > > controlled over I2C. It usually sits between a USB/DisplayPort PHY
> > > > and the Type-C connector, and provides orientation and altmode handling.
> > [...]
> > > > +	/* skip resetting if already configured */
> > > > +	if (regmap_test_bits(retimer->regmap, REG_USB_PORT_CONN_STATUS_0,
> > > > +			     CONN_STATUS_0_CONNECTION_PRESENT) == 1)
> > > > +		return gpiod_direction_output(retimer->reset_gpio, 0);
> > > 
> > > I'm still a little concerned about this. Won't you end up with i2c
> > > timeout errors in the logs if the device is held in reset before probe?
> > 
> > You should be able to use i2c_smbus_read_byte() to avoid logging errors
> > when the boot firmware has *not* enabled the device.
> 
> FWIW, regmap_test_bits() doesn't seem to print any errors either, so I
> don't think switching to i2c_smbus_read_byte() is necessary.

Thanks for checking.

> Since I was curious, I tried booting the X1E80100 with
>  1. One PS8830 instance left as-is
>  2. One PS8830 instance changed to invalid I2C address
>  3. One PS8830 instance changed to have reset pin asserted via pinctrl
> 
> There are no errors whatsoever, even for the one with invalid I2C
> address. In other words, the slightly more concerning part is that the
> driver doesn't check that any of the regmap reads/writes actually
> succeed.

Indeed.
 
> The diff I used for testing is below. (1) prints "skipping reset", (2)
> and (3) print "continuing reset".

And thanks for confirming.

I've found a few more issues that should be addressed so I'm preparing
follow-up series.

Johan