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

[Johan Hovold <johan@kernel.org>]

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