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

[Abel Vesa <abel.vesa@linaro.org>]

On 24-10-15 15:03:15, Johan Hovold wrote:
> On Fri, Oct 04, 2024 at 04:57:38PM +0300, Abel Vesa wrote:
> 
> > +static int ps8830_enable_vregs(struct ps8830_retimer *retimer)
> > +{
> 
> > +	return 0;
> > +
> > +err_vddat_disable:
> > +	regulator_disable(retimer->vddat_supply);
> > +
> 
> Nit: I'd drop the empty lines between the errors cases here.

Will drop.

> 
> > +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 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;
> > +
> > +	mutex_init(&retimer->lock);
> > +
> > +	retimer->regmap = devm_regmap_init_i2c(client, &ps8830_retimer_regmap);
> > +	if (IS_ERR(retimer->regmap)) {
> > +		dev_err(dev, "failed to allocate register map\n");
> 
> Please make sure to log the errno as well here and below.

Will add.

> 
> > +		return PTR_ERR(retimer->regmap);
> > +	}
> > +
> > +	ret = ps8830_get_vregs(retimer);
> > +	if (ret)
> > +		return ret;
> > +
> > +	retimer->xo_clk = devm_clk_get(dev, "xo");
> > +	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_OUT_HIGH);
> 
> The reset line is active low and should be described as such in DT. So
> here you want to request it as logically low if you want to deassert
> reset.

This is being reworked in v3 as we need to support cases where the
retimer has been left enabled and initialized by bootloader and we want
to keep that state until unplug event for the cold-plug orientation
to work properly.

On top of that, we don't want to deassert the reset here. We do that
via gpiod_set_value() call below, after the clocks and regulators have
been enabled.

> 
> Is there now timing requirements on when you deassert reset after
> enabling the supplies?

So based on my comment above, this is actually asserting the reset.
No timing requirements for that.

> 
> > +	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 = fwnode_typec_switch_get(dev->fwnode);
> > +	if (IS_ERR(retimer->typec_switch)) {
> > +		dev_err(dev, "failed to acquire orientation-switch\n");
> 
> I saw the driver fail here once, but not sure what the errno was since
> it was not printed. Presumably it was a probe deferral and then this
> should be a dev_err_probe() as well:
> 
> 	ps8830_retimer 2-0008: failed to acquire orientation-switch

Will use dev_err_probe.

> 
> > +		return PTR_ERR(retimer->typec_switch);
> > +	}
> > +
> > +	retimer->typec_mux = fwnode_typec_mux_get(dev->fwnode);
> > +	if (IS_ERR(retimer->typec_mux)) {
> > +		dev_err(dev, "failed to acquire mode-mux\n");
> 
> Similar here perhaps?

Same.

> 
> > +		goto err_switch_put;
> > +	}
> > +
> > +	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_mux_put;
> > +
> > +	retimer->sw = typec_switch_register(dev, &sw_desc);
> > +	if (IS_ERR(retimer->sw)) {
> > +		dev_err(dev, "failed to register typec switch\n");
> > +		goto err_aux_bridge_unregister;
> > +	}
> > +
> > +	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)) {
> > +		dev_err(dev, "failed to register typec retimer\n");
> > +		goto err_switch_unregister;
> > +	}
> > +
> > +	ret = clk_prepare_enable(retimer->xo_clk);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable XO: %d\n", ret);
> > +		goto err_retimer_unregister;
> > +	}
> 
> Should you really enable the clock before the regulators?

So maybe in this case it might not really matter. But in principle,
the HW might be affected by clock glitches and such when IP block
is powered up but unclocked. Even more so if the clock enabling
(prepare, to be more exact) involves switching to a new PLL.

So clock first, then power up. At least that's my understanding of HW
in general.

> 
> > +
> > +	ret = ps8830_enable_vregs(retimer);
> > +	if (ret)
> > +		goto err_clk_disable;
> > +
> > +	/* delay needed as per datasheet */
> > +	usleep_range(4000, 14000);
> > +
> > +	gpiod_set_value(retimer->reset_gpio, 1);
> 
> Here you only deassert reset in case the line is incorrectly described
> as active high in DT.

Yes, this needs to be 0 instead of 1. And in v3 it will depend on
a DT property called ps8830,boot-on, meaning if we want to keep it
enabled and configured as left by bootloader, by using that property
we will skip the resetting altogether.

> 
> > +	return 0;
> > +
> > +err_clk_disable:
> > +	clk_disable_unprepare(retimer->xo_clk);
> > +
> > +err_retimer_unregister:
> > +	typec_retimer_unregister(retimer->retimer);
> > +
> > +err_switch_unregister:
> > +	typec_switch_unregister(retimer->sw);
> > +
> > +err_aux_bridge_unregister:
> > +	gpiod_set_value(retimer->reset_gpio, 0);
> > +	clk_disable_unprepare(retimer->xo_clk);
> > +
> > +err_mux_put:
> > +	typec_mux_put(retimer->typec_mux);
> > +
> > +err_switch_put:
> > +	typec_switch_put(retimer->typec_switch);
> 
> Drop newlines before labels?

Will do.

> 
> > +
> > +	return ret;
> > +}
> 
> Johan

Thanks for reviewing.

Abel