Re: [PATCH v5 2/2] usb: typec: hd3ss3220: Enable VBUS based on ID pin state

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Hi Krishna,

> +static int hd3ss3220_get_vbus_supply(struct hd3ss3220 *hd3ss3220)
> +{
> +	struct device_node *hd3ss3220_node = hd3ss3220->dev->of_node;
> +	struct device_node *np;
> +
> +	np = of_graph_get_remote_node(hd3ss3220_node, 0, 0);
> +	if (!np) {
> +		dev_err(hd3ss3220->dev, "failed to get device node");
> +		return -ENODEV;
> +	}

So I guess that's the connector node. Why can't you just place the
regulator reference to the hd3ss3220 controller node instead of the
connector like the port controllers do?

That would allow us to do a simple devm_regulator_get_optional() call
that's not tied to DT only.

> +	hd3ss3220->vbus = of_regulator_get_optional(hd3ss3220->dev, np, "vbus");
> +	if (IS_ERR(hd3ss3220->vbus))
> +		hd3ss3220->vbus = NULL;
> +
> +	of_node_put(np);
> +
> +	return 0;
> +}
> +
>  static int hd3ss3220_probe(struct i2c_client *client)
>  {
>  	struct typec_capability typec_cap = { };

<snip>

> +	ret = hd3ss3220_get_vbus_supply(hd3ss3220);
> +	if (ret)
> +		return dev_err_probe(hd3ss3220->dev, ret, "failed to get vbus\n");

I think you have resource leaks here. I'm pretty sure you need to do
regulator_put() somewhere. You are also leaking the connector fwnode
that was acquired just before this point..

>  	if (IS_ERR(hd3ss3220->role_sw)) {
>  		ret = PTR_ERR(hd3ss3220->role_sw);
>  		goto err_put_fwnode;

Get the regulator here after the above condition. Then add a label for
the regulator_put(). And you already have the handle to the connector
fwnode so use that one instead of getting it again:

        hd3ss3220->vbus = of_regulator_get_optional(hd3ss3220->dev, to_of_node(connector), "vbus");

But do it like that only if you really can't place the vbus regulator
reference to the controller node. I would really prefer that we could
do a simple:

        hd3ss3220->vbus = devm_regulator_get_optional(hd3ss3220->dev, "vbus");

thanks,

-- 
heikki