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

[PATCH RFC 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 - with pin assignments C and E
 - USB3
 - DP 2lanes + USB3

Only DP 4lanes and USB3 modes have been succesfully tested on
Qualcomm (X Elite) CRD and Lenovo Thinkpad T14s so fat.
Devicetree patches for these 2 boards will follow.

The DP 2lanes + USB3 is still work-in-progress as it might involve changes
outside of this retimer driver.

Signed-off-by: Abel Vesa <abel.vesa@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     | 117 +++++++
 drivers/usb/typec/mux/Kconfig                      |  10 +
 drivers/usb/typec/mux/Makefile                     |   1 +
 drivers/usb/typec/mux/ps8830.c                     | 347 +++++++++++++++++++++
 4 files changed, 475 insertions(+)
---
base-commit: b18bbfc14a38b5234e09c2adcf713e38063a7e6e
change-id: 20240521-x1e80100-ps8830-d5ccca95b557

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

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

[Abel Vesa]

Document bindings for the Parade PS8830 Type-C retimer. This retimer is
currently found on all boards featuring Qualcomm Snapdragon X Elite SoCs
and it is needed to provide altmode muxing between DP and USB.

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

diff --git a/Documentation/devicetree/bindings/usb/parade,ps8830.yaml b/Documentation/devicetree/bindings/usb/parade,ps8830.yaml
new file mode 100644
index 000000000000..1223abf5c2f8
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/parade,ps8830.yaml
@@ -0,0 +1,117 @@
+# 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 PS8830 USB and DisplayPort Retimer
+
+maintainers:
+  - Abel Vesa <abel.vesa@linaro.org>
+
+properties:
+  compatible:
+    enum:
+      - parade,ps8830
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: XO Clock
+
+  clock-names:
+    items:
+      - const: xo
+
+  reset-gpios:
+    maxItems: 1
+
+  vdd15-supply:
+    description: power supply (1.5V)
+
+  vdd18-supply:
+    description: power supply (1.8V)
+
+  vdd33-supply:
+    description: power supply (3.3V)
+
+  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
+
+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>;
+
+            vdd15-supply = <&vreg_rtmr_1p15>;
+            vdd18-supply = <&vreg_rtmr_1p8>;
+            vdd33-supply = <&vreg_rtmr_3p3>;
+
+            reset-gpios = <&pm8550_gpios 10 GPIO_ACTIVE_HIGH>;
+
+            retimer-switch;
+            orientation-switch;
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+                    usb_con_ss: endpoint {
+                        remote-endpoint = <&typec_con_ss>;
+                    };
+                };
+                port@1 {
+                    reg = <1>;
+                    phy_con_ss: endpoint {
+                        remote-endpoint = <&usb_phy_ss>;
+                        data-lanes = <3 2 1 0>;
+                    };
+                };
+                port@2 {
+                    reg = <2>;
+                    usb_con_sbu: endpoint {
+                        remote-endpoint = <&typec_dp_aux>;
+                    };
+                };
+            };
+        };
+    };
+...

-- 
2.34.1

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

[Krzysztof Kozlowski]

On Thu, Aug 29, 2024 at 09:44:25PM +0300, Abel Vesa wrote:
> Document bindings for the Parade PS8830 Type-C retimer. This retimer is
> currently found on all boards featuring Qualcomm Snapdragon X Elite SoCs
> and it is needed to provide altmode muxing between DP and USB.
> 
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  .../devicetree/bindings/usb/parade,ps8830.yaml     | 117 +++++++++++++++++++++
>  1 file changed, 117 insertions(+)
> 

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

Best regards,
Krzysztof

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

[Johan Hovold]

On Thu, Aug 29, 2024 at 09:44:25PM +0300, Abel Vesa wrote:
> Document bindings for the Parade PS8830 Type-C retimer. This retimer is
> currently found on all boards featuring Qualcomm Snapdragon X Elite SoCs
> and it is needed to provide altmode muxing between DP and USB.

> +  vdd15-supply:
> +    description: power supply (1.5V)

As Konrad already pointed out, this appears to be a 1.15 V supply, in
which case the name and description needs an update.

> +
> +  vdd18-supply:
> +    description: power supply (1.8V)
> +
> +  vdd33-supply:
> +    description: power supply (3.3V)

Johan

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

[Abel Vesa]

The Parade PS8830 is a Type-C muti-protocol retimer controlled over I2C.
It provides both altmode and orientation handling.

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

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/ps8830.c | 347 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 358 insertions(+)

diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
index ce7db6ad3057..48613b67f1c5 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_PS8830
+	tristate "Parade PS8830 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 PS8830 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 bb96f30267af..4b23b12cfe45 100644
--- a/drivers/usb/typec/mux/Makefile
+++ b/drivers/usb/typec/mux/Makefile
@@ -6,5 +6,6 @@ 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_PS8830)		+= ps8830.o
 obj-$(CONFIG_TYPEC_MUX_PTN36502)	+= ptn36502.o
 obj-$(CONFIG_TYPEC_MUX_WCD939X_USBSS)	+= wcd939x-usbss.o
diff --git a/drivers/usb/typec/mux/ps8830.c b/drivers/usb/typec/mux/ps8830.c
new file mode 100644
index 000000000000..517ccac5932f
--- /dev/null
+++ b/drivers/usb/typec/mux/ps8830.c
@@ -0,0 +1,347 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Parade PS8830 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>
+
+struct ps8830_retimer {
+	struct i2c_client *client;
+	struct regulator_bulk_data supplies[4];
+	struct gpio_desc *reset_gpio;
+	struct regmap *regmap;
+	struct typec_switch_dev *sw;
+	struct typec_retimer *retimer;
+	struct clk *xo_clk;
+
+	bool needs_update;
+	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;
+	int cfg[3];
+
+};
+
+static int ps8830_configure(struct ps8830_retimer *retimer, int cfg0, int cfg1, int cfg2)
+{
+	if (cfg0 == retimer->cfg[0] &&
+	    cfg1 == retimer->cfg[1] &&
+	    cfg2 == retimer->cfg[2])
+		return 0;
+
+	retimer->cfg[0] = cfg0;
+	retimer->cfg[1] = cfg1;
+	retimer->cfg[2] = cfg2;
+
+	regmap_write(retimer->regmap, 0x0, cfg0);
+	regmap_write(retimer->regmap, 0x1, cfg1);
+	regmap_write(retimer->regmap, 0x2, cfg2);
+
+	return 0;
+}
+
+static int ps8380_set(struct ps8830_retimer *retimer)
+{
+	int cfg0 = 0x00, cfg1 = 0x00, cfg2 = 0x00;
+	int ret;
+
+	retimer->needs_update = false;
+
+	switch (retimer->orientation) {
+	/* Safe mode */
+	case TYPEC_ORIENTATION_NONE:
+		cfg0 = 0x01;
+		cfg1 = 0x00;
+		cfg2 = 0x00;
+		break;
+	case TYPEC_ORIENTATION_NORMAL:
+		cfg0 = 0x01;
+		break;
+	case TYPEC_ORIENTATION_REVERSE:
+		cfg0 = 0x03;
+		break;
+	}
+
+	switch (retimer->mode) {
+	/* Safe mode */
+	case TYPEC_STATE_SAFE:
+		cfg0 = 0x01;
+		cfg1 = 0x00;
+		cfg2 = 0x00;
+		break;
+
+	/* USB3 Only */
+	case TYPEC_STATE_USB:
+		cfg0 |= 0x20;
+		break;
+
+	/* DP Only */
+	case TYPEC_DP_STATE_C:
+	case TYPEC_DP_STATE_E:
+		cfg0 &= 0x0f;
+		cfg1 = 0x85;
+		break;
+
+	/* DP + USB */
+	case TYPEC_DP_STATE_D:
+	case TYPEC_DP_STATE_F:
+		cfg0 |= 0x20;
+		cfg1 = 0x85;
+		break;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	gpiod_set_value(retimer->reset_gpio, 0);
+	msleep(20);
+	gpiod_set_value(retimer->reset_gpio, 1);
+
+	msleep(60);
+
+	ret = ps8830_configure(retimer, 0x01, 0x00, 0x00);
+
+	msleep(30);
+
+	return ps8830_configure(retimer, cfg0, cfg1, cfg2);
+}
+
+static int ps8830_sw_set(struct typec_switch_dev *sw,
+			 enum typec_orientation orientation)
+{
+	struct ps8830_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;
+		retimer->needs_update = true;
+	}
+
+	if (retimer->needs_update)
+		ret = ps8380_set(retimer);
+
+	mutex_unlock(&retimer->lock);
+
+	return ret;
+}
+
+static int ps8830_retimer_set(struct typec_retimer *rtmr,
+			      struct typec_retimer_state *state)
+{
+	struct ps8830_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;
+		retimer->needs_update = true;
+	}
+
+	if (retimer->needs_update)
+		ret = ps8380_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 const struct regmap_config ps8830_retimer_regmap = {
+	.max_register = 0x1f,
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int ps8830_retimer_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct typec_switch_desc sw_desc = { };
+	struct typec_retimer_desc rtmr_desc = { };
+	struct ps8830_retimer *retimer;
+	int ret;
+
+	retimer = devm_kzalloc(dev, sizeof(*retimer), GFP_KERNEL);
+	if (!retimer)
+		return -ENOMEM;
+
+	retimer->client = client;
+
+	retimer->regmap = devm_regmap_init_i2c(client, &ps8830_retimer_regmap);
+	if (IS_ERR(retimer->regmap)) {
+		dev_err(dev, "Failed to allocate register map\n");
+		return PTR_ERR(retimer->regmap);
+	}
+
+	retimer->supplies[0].supply = "vdd33";
+	retimer->supplies[1].supply = "vdd18";
+	retimer->supplies[2].supply = "vdd15";
+	retimer->supplies[3].supply = "vcc";
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(retimer->supplies),
+				      retimer->supplies);
+	if (ret)
+		return ret;
+
+	retimer->xo_clk = devm_clk_get(dev, "xo");
+	if (IS_ERR(retimer->xo_clk))
+		return PTR_ERR(retimer->xo_clk);
+
+	retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(retimer->reset_gpio))
+		return PTR_ERR(retimer->reset_gpio);
+
+	retimer->typec_switch = fwnode_typec_switch_get(dev->fwnode);
+	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 = fwnode_typec_mux_get(dev->fwnode);
+	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 = regulator_bulk_enable(ARRAY_SIZE(retimer->supplies),
+				    retimer->supplies);
+	if (ret < 0) {
+		dev_err(dev, "cannot enable regulators %d\n", 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_disable_vreg;
+	}
+
+	gpiod_set_value(retimer->reset_gpio, 0);
+	msleep(20);
+	gpiod_set_value(retimer->reset_gpio, 1);
+
+	msleep(60);
+	mutex_init(&retimer->lock);
+
+	sw_desc.drvdata = retimer;
+	sw_desc.fwnode = dev_fwnode(dev);
+	sw_desc.set = ps8830_sw_set;
+
+	ret = drm_aux_bridge_register(dev);
+	if (ret)
+		goto err_disable_gpio;
+
+	retimer->sw = typec_switch_register(dev, &sw_desc);
+	if (IS_ERR(retimer->sw)) {
+		ret = dev_err_probe(dev, PTR_ERR(retimer->sw),
+				    "Error registering typec switch\n");
+		goto err_disable_gpio;
+	}
+
+	rtmr_desc.drvdata = retimer;
+	rtmr_desc.fwnode = dev_fwnode(dev);
+	rtmr_desc.set = ps8830_retimer_set;
+
+	retimer->retimer = typec_retimer_register(dev, &rtmr_desc);
+	if (IS_ERR(retimer->retimer)) {
+		ret = dev_err_probe(dev, PTR_ERR(retimer->retimer),
+				    "Error registering typec retimer\n");
+		goto err_switch_unregister;
+	}
+
+	dev_info(dev, "Registered Parade PS8830 retimer\n");
+	return 0;
+
+err_switch_unregister:
+	typec_switch_unregister(retimer->sw);
+
+err_disable_gpio:
+	gpiod_set_value(retimer->reset_gpio, 0);
+	clk_disable_unprepare(retimer->xo_clk);
+
+err_disable_vreg:
+	regulator_bulk_disable(ARRAY_SIZE(retimer->supplies),
+			       retimer->supplies);
+err_mux_put:
+	typec_mux_put(retimer->typec_mux);
+
+err_switch_put:
+	typec_switch_put(retimer->typec_switch);
+
+	return ret;
+}
+
+static void ps8830_retimer_remove(struct i2c_client *client)
+{
+	struct ps8830_retimer *retimer = i2c_get_clientdata(client);
+
+	typec_retimer_unregister(retimer->retimer);
+	typec_switch_unregister(retimer->sw);
+
+	gpiod_set_value(retimer->reset_gpio, 0);
+
+	clk_disable_unprepare(retimer->xo_clk);
+
+	regulator_bulk_disable(ARRAY_SIZE(retimer->supplies),
+			       retimer->supplies);
+
+	typec_mux_put(retimer->typec_mux);
+	typec_switch_put(retimer->typec_switch);
+}
+
+static const struct i2c_device_id ps8830_retimer_table[] = {
+	{ "parade,ps8830" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ps8830_retimer_table);
+
+static const struct of_device_id ps8830_retimer_of_table[] = {
+	{ .compatible = "parade,ps8830" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ps8830_retimer_of_table);
+
+static struct i2c_driver ps8830_retimer_driver = {
+	.driver = {
+		.name = "ps8830_retimer",
+		.of_match_table = ps8830_retimer_of_table,
+	},
+	.probe		= ps8830_retimer_probe,
+	.remove		= ps8830_retimer_remove,
+	.id_table	= ps8830_retimer_table,
+};
+
+module_i2c_driver(ps8830_retimer_driver);
+
+MODULE_DESCRIPTION("Parade PS8830 Type-C Retimer driver");
+MODULE_LICENSE("GPL");

-- 
2.34.1

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

[Konrad Dybcio]

On 29.08.2024 8:44 PM, Abel Vesa wrote:
> The Parade PS8830 is a Type-C muti-protocol retimer controlled over I2C.
> It provides both altmode and orientation handling.
> 
> Add a driver with support for the following modes:
>  - DP 4lanes
>  - USB3
>  - DP 2lanes + USB3
> 
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---

[...]

> +	retimer->supplies[0].supply = "vdd33";
> +	retimer->supplies[1].supply = "vdd18";
> +	retimer->supplies[2].supply = "vdd15";

1.15?

Konrad

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

[Johan Hovold]

On Thu, Aug 29, 2024 at 09:44:26PM +0300, Abel Vesa wrote:
> The Parade PS8830 is a Type-C muti-protocol retimer controlled over I2C.
> It provides both altmode and orientation handling.
> 
> Add a driver with support for the following modes:
>  - DP 4lanes
>  - USB3
>  - DP 2lanes + USB3
> 
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>

> +struct ps8830_retimer {
> +	struct i2c_client *client;
> +	struct regulator_bulk_data supplies[4];
> +	struct gpio_desc *reset_gpio;
> +	struct regmap *regmap;
> +	struct typec_switch_dev *sw;
> +	struct typec_retimer *retimer;
> +	struct clk *xo_clk;
> +
> +	bool needs_update;
> +	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;
> +	int cfg[3];
> +

Stray newline.

> +};
> +
> +static int ps8830_configure(struct ps8830_retimer *retimer, int cfg0, int cfg1, int cfg2)
> +{
> +	if (cfg0 == retimer->cfg[0] &&
> +	    cfg1 == retimer->cfg[1] &&
> +	    cfg2 == retimer->cfg[2])
> +		return 0;
> +
> +	retimer->cfg[0] = cfg0;
> +	retimer->cfg[1] = cfg1;
> +	retimer->cfg[2] = cfg2;
> +
> +	regmap_write(retimer->regmap, 0x0, cfg0);
> +	regmap_write(retimer->regmap, 0x1, cfg1);
> +	regmap_write(retimer->regmap, 0x2, cfg2);
> +
> +	return 0;
> +}

You always return 0 here so should this be a void function?

> +
> +static int ps8380_set(struct ps8830_retimer *retimer)
> +{
> +	int cfg0 = 0x00, cfg1 = 0x00, cfg2 = 0x00;

Please avoid doing multiple initialisations like this (one per line is
more readable).

> +	int ret;
> +
> +	retimer->needs_update = false;
> +
> +	switch (retimer->orientation) {
> +	/* Safe mode */
> +	case TYPEC_ORIENTATION_NONE:
> +		cfg0 = 0x01;
> +		cfg1 = 0x00;
> +		cfg2 = 0x00;
> +		break;
> +	case TYPEC_ORIENTATION_NORMAL:
> +		cfg0 = 0x01;
> +		break;
> +	case TYPEC_ORIENTATION_REVERSE:
> +		cfg0 = 0x03;
> +		break;
> +	}
> +
> +	switch (retimer->mode) {
> +	/* Safe mode */
> +	case TYPEC_STATE_SAFE:
> +		cfg0 = 0x01;
> +		cfg1 = 0x00;
> +		cfg2 = 0x00;
> +		break;
> +
> +	/* USB3 Only */
> +	case TYPEC_STATE_USB:
> +		cfg0 |= 0x20;
> +		break;
> +
> +	/* DP Only */
> +	case TYPEC_DP_STATE_C:
> +	case TYPEC_DP_STATE_E:
> +		cfg0 &= 0x0f;
> +		cfg1 = 0x85;
> +		break;
> +
> +	/* DP + USB */
> +	case TYPEC_DP_STATE_D:
> +	case TYPEC_DP_STATE_F:
> +		cfg0 |= 0x20;
> +		cfg1 = 0x85;
> +		break;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	gpiod_set_value(retimer->reset_gpio, 0);
> +	msleep(20);
> +	gpiod_set_value(retimer->reset_gpio, 1);
> +
> +	msleep(60);
> +
> +	ret = ps8830_configure(retimer, 0x01, 0x00, 0x00);
> +
> +	msleep(30);
> +
> +	return ps8830_configure(retimer, cfg0, cfg1, cfg2);

As the build bots pointed out, ret is never used, and the configure
function always returns 0. Make the function type void and return 0
explicitly here instead?

> +}

> +static int ps8830_retimer_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct typec_switch_desc sw_desc = { };
> +	struct typec_retimer_desc rtmr_desc = { };
> +	struct ps8830_retimer *retimer;
> +	int ret;
> +
> +	retimer = devm_kzalloc(dev, sizeof(*retimer), GFP_KERNEL);
> +	if (!retimer)
> +		return -ENOMEM;
> +
> +	retimer->client = client;
> +
> +	retimer->regmap = devm_regmap_init_i2c(client, &ps8830_retimer_regmap);
> +	if (IS_ERR(retimer->regmap)) {
> +		dev_err(dev, "Failed to allocate register map\n");
> +		return PTR_ERR(retimer->regmap);
> +	}
> +
> +	retimer->supplies[0].supply = "vdd33";
> +	retimer->supplies[1].supply = "vdd18";
> +	retimer->supplies[2].supply = "vdd15";

vdd115?

> +	retimer->supplies[3].supply = "vcc";
> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(retimer->supplies),
> +				      retimer->supplies);
> +	if (ret)
> +		return ret;
> +
> +	retimer->xo_clk = devm_clk_get(dev, "xo");
> +	if (IS_ERR(retimer->xo_clk))
> +		return PTR_ERR(retimer->xo_clk);
> +
> +	retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(retimer->reset_gpio))
> +		return PTR_ERR(retimer->reset_gpio);
> +
> +	retimer->typec_switch = fwnode_typec_switch_get(dev->fwnode);
> +	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 = fwnode_typec_mux_get(dev->fwnode);
> +	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 = regulator_bulk_enable(ARRAY_SIZE(retimer->supplies),
> +				    retimer->supplies);
> +	if (ret < 0) {
> +		dev_err(dev, "cannot enable regulators %d\n", ret);

Please add a colon after "regulators" to maintain a consistent style of
error messages. 

> +		goto err_mux_put;
> +	}
> +
> +	ret = clk_prepare_enable(retimer->xo_clk);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable XO: %d\n", ret);

Lower case "failed" for consistency.

> +		goto err_disable_vreg;
> +	}
> +
> +	gpiod_set_value(retimer->reset_gpio, 0);
> +	msleep(20);
> +	gpiod_set_value(retimer->reset_gpio, 1);
> +
> +	msleep(60);
> +	mutex_init(&retimer->lock);

I'd initialise resources like this before resetting the device (e.g.
move above regmap init).

> +
> +	sw_desc.drvdata = retimer;
> +	sw_desc.fwnode = dev_fwnode(dev);
> +	sw_desc.set = ps8830_sw_set;
> +
> +	ret = drm_aux_bridge_register(dev);
> +	if (ret)
> +		goto err_disable_gpio;
> +
> +	retimer->sw = typec_switch_register(dev, &sw_desc);
> +	if (IS_ERR(retimer->sw)) {
> +		ret = dev_err_probe(dev, PTR_ERR(retimer->sw),
> +				    "Error registering typec switch\n");

Switch registration cannot return EPROBE_DEFER so I suggest using
dev_err() for clarity (e.g. as you must not call functions that can
defer probe after registering child devices like the aux bridge).

> +		goto err_disable_gpio;
> +	}
> +
> +	rtmr_desc.drvdata = retimer;
> +	rtmr_desc.fwnode = dev_fwnode(dev);
> +	rtmr_desc.set = ps8830_retimer_set;
> +
> +	retimer->retimer = typec_retimer_register(dev, &rtmr_desc);
> +	if (IS_ERR(retimer->retimer)) {
> +		ret = dev_err_probe(dev, PTR_ERR(retimer->retimer),
> +				    "Error registering typec retimer\n");

Same here.

> +		goto err_switch_unregister;
> +	}
> +
> +	dev_info(dev, "Registered Parade PS8830 retimer\n");

Drop this, drivers shouldn't spam the logs on success.

> +	return 0;
> +
> +err_switch_unregister:
> +	typec_switch_unregister(retimer->sw);
> +
> +err_disable_gpio:
> +	gpiod_set_value(retimer->reset_gpio, 0);
> +	clk_disable_unprepare(retimer->xo_clk);
> +
> +err_disable_vreg:
> +	regulator_bulk_disable(ARRAY_SIZE(retimer->supplies),
> +			       retimer->supplies);
> +err_mux_put:
> +	typec_mux_put(retimer->typec_mux);
> +
> +err_switch_put:
> +	typec_switch_put(retimer->typec_switch);
> +
> +	return ret;
> +}

> +static const struct i2c_device_id ps8830_retimer_table[] = {
> +	{ "parade,ps8830" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ps8830_retimer_table);

This one should not be needed, right?

> +static const struct of_device_id ps8830_retimer_of_table[] = {
> +	{ .compatible = "parade,ps8830" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ps8830_retimer_of_table);

Johan

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

[Johan Hovold]

On Tue, Sep 03, 2024 at 09:27:45AM +0200, Johan Hovold wrote:
> On Thu, Aug 29, 2024 at 09:44:26PM +0300, Abel Vesa wrote:
> > The Parade PS8830 is a Type-C muti-protocol retimer controlled over I2C.
> > It provides both altmode and orientation handling.
> > 
> > Add a driver with support for the following modes:
> >  - DP 4lanes
> >  - USB3
> >  - DP 2lanes + USB3
> > 
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>

> > +	retimer->supplies[0].supply = "vdd33";
> > +	retimer->supplies[1].supply = "vdd18";
> > +	retimer->supplies[2].supply = "vdd15";
> 
> vdd115?
> 
> > +	retimer->supplies[3].supply = "vcc";

I took a look at the schematics and it seems like all but one of the
above supply names are wrong and that some are missing. "vcc" also does
not exist in either the binding or dt patches you sent separately.

From what I can tell the supplies are:

	vdd		1.15 V
	vdd33		3.3 V
	vdd33_cap	3.3 V
	vddat		1.15 V
	vddar		1.15 V
	vddio		1.8 V

Also, have you checked that there are no ordering constraints between
the supplies?

Johan

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

[Abel Vesa]

On 24-09-04 15:20:55, Johan Hovold wrote:
> On Tue, Sep 03, 2024 at 09:27:45AM +0200, Johan Hovold wrote:
> > On Thu, Aug 29, 2024 at 09:44:26PM +0300, Abel Vesa wrote:
> > > The Parade PS8830 is a Type-C muti-protocol retimer controlled over I2C.
> > > It provides both altmode and orientation handling.
> > > 
> > > Add a driver with support for the following modes:
> > >  - DP 4lanes
> > >  - USB3
> > >  - DP 2lanes + USB3
> > > 
> > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> 
> > > +	retimer->supplies[0].supply = "vdd33";
> > > +	retimer->supplies[1].supply = "vdd18";
> > > +	retimer->supplies[2].supply = "vdd15";
> > 
> > vdd115?
> > 
> > > +	retimer->supplies[3].supply = "vcc";
> 
> I took a look at the schematics and it seems like all but one of the
> above supply names are wrong and that some are missing. "vcc" also does
> not exist in either the binding or dt patches you sent separately.
> 
> From what I can tell the supplies are:
> 
> 	vdd		1.15 V
> 	vdd33		3.3 V
> 	vdd33_cap	3.3 V
> 	vddat		1.15 V
> 	vddar		1.15 V
> 	vddio		1.8 V

The schematics seem to suggest that vdd, vddat and vddar are all
supplied by the 1.15V supply. As for the vdd33 and vdd33_cap, their
seem to be supplied by the 3.3V supply.

> 
> Also, have you checked that there are no ordering constraints between
> the supplies?

The documentation seems to suggest that there are some timing as well as
ordering contrains, yes. I can't tell for sure if that is really needed
or not.

Thanks for reviewing.

> 
> Johan
> 

Abel

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

[Dmitry Baryshkov]

On Mon, 23 Sept 2024 at 17:41, Abel Vesa <abel.vesa@linaro.org> wrote:
>
> On 24-09-04 15:20:55, Johan Hovold wrote:
> > On Tue, Sep 03, 2024 at 09:27:45AM +0200, Johan Hovold wrote:
> > > On Thu, Aug 29, 2024 at 09:44:26PM +0300, Abel Vesa wrote:
> > > > The Parade PS8830 is a Type-C muti-protocol retimer controlled over I2C.
> > > > It provides both altmode and orientation handling.
> > > >
> > > > Add a driver with support for the following modes:
> > > >  - DP 4lanes
> > > >  - USB3
> > > >  - DP 2lanes + USB3
> > > >
> > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> >
> > > > + retimer->supplies[0].supply = "vdd33";
> > > > + retimer->supplies[1].supply = "vdd18";
> > > > + retimer->supplies[2].supply = "vdd15";
> > >
> > > vdd115?
> > >
> > > > + retimer->supplies[3].supply = "vcc";
> >
> > I took a look at the schematics and it seems like all but one of the
> > above supply names are wrong and that some are missing. "vcc" also does
> > not exist in either the binding or dt patches you sent separately.
> >
> > From what I can tell the supplies are:
> >
> >       vdd             1.15 V
> >       vdd33           3.3 V
> >       vdd33_cap       3.3 V
> >       vddat           1.15 V
> >       vddar           1.15 V
> >       vddio           1.8 V
>
> The schematics seem to suggest that vdd, vddat and vddar are all
> supplied by the 1.15V supply. As for the vdd33 and vdd33_cap, their
> seem to be supplied by the 3.3V supply.

Please follow the datasheet when naming the supplies. Some of them
might be supplied by a single rail, but that might be a property of
your platform.

>
> >
> > Also, have you checked that there are no ordering constraints between
> > the supplies?
>
> The documentation seems to suggest that there are some timing as well as
> ordering contrains, yes. I can't tell for sure if that is really needed
> or not.

Again, please follow the datasheet.

>
> Thanks for reviewing.
>
> >
> > Johan
> >
>
> Abel



-- 
With best wishes
Dmitry

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

[Abel Vesa]

On 24-09-23 18:03:09, Dmitry Baryshkov wrote:
> On Mon, 23 Sept 2024 at 17:41, Abel Vesa <abel.vesa@linaro.org> wrote:
> >
> > On 24-09-04 15:20:55, Johan Hovold wrote:
> > > On Tue, Sep 03, 2024 at 09:27:45AM +0200, Johan Hovold wrote:
> > > > On Thu, Aug 29, 2024 at 09:44:26PM +0300, Abel Vesa wrote:
> > > > > The Parade PS8830 is a Type-C muti-protocol retimer controlled over I2C.
> > > > > It provides both altmode and orientation handling.
> > > > >
> > > > > Add a driver with support for the following modes:
> > > > >  - DP 4lanes
> > > > >  - USB3
> > > > >  - DP 2lanes + USB3
> > > > >
> > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > >
> > > > > + retimer->supplies[0].supply = "vdd33";
> > > > > + retimer->supplies[1].supply = "vdd18";
> > > > > + retimer->supplies[2].supply = "vdd15";
> > > >
> > > > vdd115?
> > > >
> > > > > + retimer->supplies[3].supply = "vcc";
> > >
> > > I took a look at the schematics and it seems like all but one of the
> > > above supply names are wrong and that some are missing. "vcc" also does
> > > not exist in either the binding or dt patches you sent separately.
> > >
> > > From what I can tell the supplies are:
> > >
> > >       vdd             1.15 V
> > >       vdd33           3.3 V
> > >       vdd33_cap       3.3 V
> > >       vddat           1.15 V
> > >       vddar           1.15 V
> > >       vddio           1.8 V
> >
> > The schematics seem to suggest that vdd, vddat and vddar are all
> > supplied by the 1.15V supply. As for the vdd33 and vdd33_cap, their
> > seem to be supplied by the 3.3V supply.
> 
> Please follow the datasheet when naming the supplies. Some of them
> might be supplied by a single rail, but that might be a property of
> your platform.

Fair enough, then will use the ones Johan listed here as those are the
ones the datasheet lists as well.

> 
> >
> > >
> > > Also, have you checked that there are no ordering constraints between
> > > the supplies?
> >
> > The documentation seems to suggest that there are some timing as well as
> > ordering contrains, yes. I can't tell for sure if that is really needed
> > or not.
> 
> Again, please follow the datasheet.

OK, will drop the bulk, will use simple get instead and add proper
timings according to datasheet.

> 
> >
> > Thanks for reviewing.
> >
> > >
> > > Johan
> > >
> >
> > Abel
> 
> 
> 
> -- 
> With best wishes
> Dmitry
> 

Thanks for feedback.

Abel

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

[Konrad Dybcio]

On 29-Aug-24 20:44, Abel Vesa wrote:
> 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 - with pin assignments C and E
>  - USB3
>  - DP 2lanes + USB3
> 
> Only DP 4lanes and USB3 modes have been succesfully tested on
> Qualcomm (X Elite) CRD and Lenovo Thinkpad T14s so fat.
> Devicetree patches for these 2 boards will follow.
> 
> The DP 2lanes + USB3 is still work-in-progress as it might involve changes
> outside of this retimer driver.

Yep, it's the QC combo PHY being grumpy, this one's fine

Tested-by: Konrad Dybcio <quic_kdybcio@quicinc.com> # x1e80100-romulus

Konrad