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

[Krishna Kurapati PSSNV <krishna.kurapati@oss.qualcomm.com>]

On 10/25/2025 7:44 PM, Christophe JAILLET wrote:
> Le 25/10/2025 à 14:28, Krishna Kurapati a écrit :
>> 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.
>>

[...]

>> +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))
> 
> I don't think it can be ERR. hd3ss3220_get_vbus_supply() forces it to 
> NULL in such a case.
>

ACK. Will make it (!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;
>> +    struct device_node *np;
>> +    int ret = 0;
>> +
>> +    np = of_graph_get_remote_node(hd3ss3220_node, 0, 0);
>> +    if (!np) {
>> +        dev_err(hd3ss3220->dev, "failed to get device node");
>> +        return -ENODEV;
>> +    }
>> +
>> +    hd3ss3220->vbus = of_regulator_get_optional(hd3ss3220->dev, np, 
>> "vbus");
>> +    if (IS_ERR(hd3ss3220->vbus))
>> +        hd3ss3220->vbus = NULL;
>> +
>> +    of_node_put(np);
>> +
>> +    return ret;
> 
> return 0 and avoid 'ret'?

ACK.

> 
>> +}
>> +
>>   static int hd3ss3220_probe(struct i2c_client *client)
>>   {
>>       struct typec_capability typec_cap = { };
>> @@ -354,6 +405,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");
> 
> Maybe return dev_err_probe() to log the error and simplify code?
> 

ACK.

>> +            return hd3
ss3220->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");
> 
> Maybe return dev_err_probe() to log the error and simplify code?
> 
> Above, you use "ID IRQ" and here "id irq". Maybe keep the case case? Or 
> change the 2nd message that looks a copy'n'paste error to me.
> 

ACK. Will correct it.

>> +            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");
> 
> Why PTR_ERR(hd3ss3220->vbus)? Should this be 'ret'?
> 
> If hd3ss3220_get_vbus_supply() fails, vbus will be NULL in all cases.
> 
> In hd3ss3220_get_vbus_supply(), if -ENODEV is returned, it is not 
> initialized yet, and if of_regulator_get_optional() fails, it is set to 
> NULL.
>

Yes, ret is better. Will change it accordingly.

Regards,
Krishna,