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

[Krishna Kurapati PSSNV]

On 11/6/2025 3:35 PM, Heikki Krogerus wrote:
> Hi Krishna,
>
> Sun, Nov 02, 2025 at 10:18:19PM +0530, Krishna Kurapati kirjoitti:
> > 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@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/usb/typec/hd3ss3220.c | 72 +++++++++++++++++++++++++++++++++++
> >   1 file changed, 72 insertions(+)
> >
> > diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
> > index 3ecc688dda82..75fbda42eaf4 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,44 @@ 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 (!hd3ss3220->vbus)
> > +		return IRQ_HANDLED;
>
> If you don't need this routine unless there is a vbus regulator, then
> don't register it at all if there is no vbus regulator.

Will move vbus check before id retrieval in probe and ignore retrieval 
of ID if vbus is absent.

> > +	id = hd3ss3220->id_gpiod ? gpiod_get_value_cansleep(hd3ss3220->id_gpiod) : 1;
>
> You still don't need to check for hd3ss3220->id_gpiod - this function
> will not get called unless it's there.
>
>          if (gpiod_get_value_cansleep(hd3ss3220->id_gpiod))
>                  ret = regulator_disable(hd3ss3220->vbus);
>          else
>                  ret = regulator_enable(hd3ss3220->vbus);

ACK.

> Note:
>
> If you are concerned that the reference to the id_gpiod may be
> released before this routine is unregistered, then that condition will
> not help. The hd3ss3220->id_gpiod member is _not_ NULL after the
> reference is released.
>
> If you need a specific order in which the references are released,
> then you can't use the resource management (devm_*) to automate things
> for you.

There is no specific order. So the id part I can keep it intact except 
for checking presence of ID pin in interrupt handler.

> > +	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 fwnode_handle *connector)
> > +{
> > +	int ret  = 0;
> > +
> > +	hd3ss3220->vbus = devm_of_regulator_get_optional(hd3ss3220->dev,
> > +							 to_of_node(connector),
> > +							 "vbus");
> > +	if (PTR_ERR(hd3ss3220->vbus) == -ENODEV)
> > +		hd3ss3220->vbus = NULL;
> > +	else if (IS_ERR(hd3ss3220->vbus))
> > +		ret = PTR_ERR(hd3ss3220->vbus);
>
> So the regulator API's optional functions return -ENODEV instead of NULL :(
> In any case, don't double assign the member. Use local variable.
>
>          struct regulator *vbus;
>
>          vbus = devm_of_regulator_get_optional(...
>          if (IS_ERR(vbus) && vbus != ERR_PTR(-ENODEV))
>                  return PTR_ERR(vbus);
>
>          hd3ss3220->vbus = vbus;
>          return 0;
>
> I don't think you need this function - just do that in the probe function.

ACK.

Regards,
Krishna,