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

[Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>]

On 24/10/2025 21:18, Krishna Kurapati wrote:
> There is a ID pin present on HD3SS3220 controller that can be routed
> to SoC. As per the datasheet:
> 
> "Upon detecting a UFP device, HD3SS3220 will keep ID pin high if VBUS is
> not at VSafe0V. Once VBUS is at VSafe0V, the HD3SS3220 will assert ID pin
> low. This is done to enforce Type-C requirement that VBUS must be at
> VSafe0V before re-enabling VBUS"
> 
> Add support to read the ID pin state and enable VBUS accordingly.
> 
> Signed-off-by: Krishna Kurapati <krishna.kurapati@oss.qualcomm.com>
> ---
>   drivers/usb/typec/hd3ss3220.c | 101 ++++++++++++++++++++++++++++++++++
>   1 file changed, 101 insertions(+)
> 
> diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
> index 3ecc688dda82..089c4168c7b5 100644
> --- a/drivers/usb/typec/hd3ss3220.c
> +++ b/drivers/usb/typec/hd3ss3220.c
> @@ -15,6 +15,9 @@
>   #include <linux/usb/typec.h>
>   #include <linux/delay.h>
>   #include <linux/workqueue.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/of_graph.h>
>   
>   #define HD3SS3220_REG_CN_STAT		0x08
>   #define HD3SS3220_REG_CN_STAT_CTRL	0x09
> @@ -54,6 +57,11 @@ struct hd3ss3220 {
>   	struct delayed_work output_poll_work;
>   	enum usb_role role_state;
>   	bool poll;
> +
> +	struct gpio_desc *id_gpiod;
> +	int id_irq;
> +
> +	struct regulator *vbus;
>   };
>   
>   static int hd3ss3220_set_power_opmode(struct hd3ss3220 *hd3ss3220, int power_opmode)
> @@ -319,6 +327,71 @@ static const struct regmap_config config = {
>   	.max_register = 0x0A,
>   };
>   
> +static irqreturn_t hd3ss3220_id_isr(int irq, void *dev_id)
> +{
> +	struct hd3ss3220 *hd3ss3220 = dev_id;
> +	int ret;
> +	int id;
> +
> +	if (IS_ERR_OR_NULL(hd3ss3220->vbus))
> +		return IRQ_HANDLED;
> +
> +	id = hd3ss3220->id_gpiod ? gpiod_get_value_cansleep(hd3ss3220->id_gpiod) : 1;
> +
> +	if (!id) {
> +		ret = regulator_enable(hd3ss3220->vbus);
> +		if (ret)
> +			dev_err(hd3ss3220->dev, "enable vbus regulator failed\n");
> +	} else {
> +		regulator_disable(hd3ss3220->vbus);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int hd3ss3220_get_vbus_supply(struct hd3ss3220 *hd3ss3220)
> +{
> +	struct device_node *hd3ss3220_node = hd3ss3220->dev->of_node;
> +	const char *compat_string;
> +	struct device_node *np;
> +	int num_ports = 0;
> +	int ret = 0;
> +	int i = 0;
> +
> +	num_ports = of_graph_get_port_count(hd3ss3220_node);
> +
> +	for (i = 0; i < num_ports; i++) {
> +		np = of_graph_get_remote_node(hd3ss3220_node, i, 0);
> +		if (!np) {
> +			dev_err(hd3ss3220->dev, "failed to get device node");
> +			ret = -ENODEV;
> +			goto done;
> +		}
> +
> +		ret = of_property_read_string(np, "compatible", &compat_string);
> +		if (ret) {
> +			of_node_put(np);
> +			dev_err(hd3ss3220->dev, "failed to get compatible string");
> +			ret = -ENODEV;
> +			goto done;
> +		}
> +
> +		if (strcmp(compat_string, "usb-c-connector") == 0) {
> +			hd3ss3220->vbus = of_regulator_get(hd3ss3220->dev, np, "vbus");
> +			if (PTR_ERR(hd3ss3220->vbus) == -ENODEV)
> +				hd3ss3220->vbus = NULL;
> +
> +			if (IS_ERR(hd3ss3220->vbus))
> +				ret = -ENODEV;
> +		}

This whole loop is an overkill. You know that only port@0 can be 
connected to the usb-c-connector. There is no need to check other ports. 
there is no need to check for the compatible, etc.

> +
> +		of_node_put(np);
> +	}
> +
> +done:
> +	return ret;
> +}
> +
>   static int hd3ss3220_probe(struct i2c_client *client)
>   {
>   	struct typec_capability typec_cap = { };
> @@ -354,6 +427,34 @@ static int hd3ss3220_probe(struct i2c_client *client)
>   		hd3ss3220->role_sw = usb_role_switch_get(hd3ss3220->dev);
>   	}
>   
> +	hd3ss3220->id_gpiod = devm_gpiod_get_optional(hd3ss3220->dev, "id", GPIOD_IN);
> +	if (IS_ERR(hd3ss3220->id_gpiod))
> +		return PTR_ERR(hd3ss3220->id_gpiod);
> +
> +	if (hd3ss3220->id_gpiod) {
> +		hd3ss3220->id_irq = gpiod_to_irq(hd3ss3220->id_gpiod);
> +		if (hd3ss3220->id_irq < 0) {
> +			dev_err(hd3ss3220->dev, "failed to get ID IRQ\n");
> +			return hd3ss3220->id_irq;
> +		}
> +
> +		ret = devm_request_threaded_irq(hd3ss3220->dev,
> +						hd3ss3220->id_irq, NULL,
> +						hd3ss3220_id_isr,
> +						IRQF_TRIGGER_RISING |
> +						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +						dev_name(hd3ss3220->dev), hd3ss3220);
> +		if (ret < 0) {
> +			dev_err(hd3ss3220->dev, "failed to get id irq\n");
> +			return ret;
> +		}
> +	}
> +
> +	ret = hd3ss3220_get_vbus_supply(hd3ss3220);
> +	if (ret)
> +		return dev_err_probe(hd3ss3220->dev,
> +				     PTR_ERR(hd3ss3220->vbus), "failed to get vbus\n");
> +
>   	if (IS_ERR(hd3ss3220->role_sw)) {
>   		ret = PTR_ERR(hd3ss3220->role_sw);
>   		goto err_put_fwnode;


-- 
With best wishes
Dmitry