Message-ID: <Zta6cBq881Ju7r7H@hovoldconsulting.com>
Date: Tue, 3 Sep 2024 09:27:44 +0200
From: Johan Hovold <johan@...nel.org>
To: Abel Vesa <abel.vesa@...aro.org>
Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Bjorn Andersson <andersson@...nel.org>,
	Konrad Dybcio <konradybcio@...nel.org>,
	Rajendra Nayak <quic_rjendra@...cinc.com>,
	Sibi Sankar <quic_sibis@...cinc.com>,
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
	Trilok Soni <quic_tsoni@...cinc.com>, linux-kernel@...r.kernel.org,
	linux-usb@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH RFC 2/2] usb: typec: Add support for Parade PS8830 Type-C
 Retimer

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@...aro.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

Powered by blists - more mailing lists