RE: [PATCH v3 1/7] usb: onboard-hub: Add reset-gpio support

4 Oct 2024


      Hi,
...
-----Original Message-----
From: Marek Vasut marex@denx.de
Sent: Friday, October 4, 2024 4:52 PM
To: Abbarapu, Venkatesh venkatesh.abbarapu@amd.com; u-boot@lists.denx.de
Cc: Simek, Michal michal.simek@amd.com; fabrice.gasnier@foss.st.com; git
(AMD-Xilinx) git@amd.com
Subject: Re: [PATCH v3 1/7] usb: onboard-hub: Add reset-gpio support
On 10/4/24 5:02 AM, Abbarapu, Venkatesh wrote:
...
Hi Marek,
...
-----Original Message-----
From: Marek Vasut marex@denx.de
Sent: Thursday, October 3, 2024 10:35 PM
To: Abbarapu, Venkatesh venkatesh.abbarapu@amd.com;
u-boot@lists.denx.de
Cc: Simek, Michal michal.simek@amd.com;
fabrice.gasnier@foss.st.com; git
(AMD-Xilinx) git@amd.com
Subject: Re: [PATCH v3 1/7] usb: onboard-hub: Add reset-gpio support
On 10/3/24 4:54 PM, Abbarapu, Venkatesh wrote:
...
Hi Marek,
...
-----Original Message-----
From: Marek Vasut marex@denx.de
Sent: Thursday, October 3, 2024 5:38 PM
To: Abbarapu, Venkatesh venkatesh.abbarapu@amd.com;
u-boot@lists.denx.de
Cc: Simek, Michal michal.simek@amd.com;
fabrice.gasnier@foss.st.com; git
(AMD-Xilinx) git@amd.com
Subject: Re: [PATCH v3 1/7] usb: onboard-hub: Add reset-gpio
support
On 10/3/24 6:40 AM, Abbarapu, Venkatesh wrote:
...
Hi Marek,
Hi,
...
>> @@ -30,7 +40,24 @@ static int usb_onboard_hub_probe(struct
>> udevice
*dev)
...
...
...
>>      	if (ret)
>>      		dev_err(dev, "can't enable vdd-supply: %d\n", ret);
>>
>> -	return ret;
>> +	hub->reset_gpio = devm_gpiod_get_optional(dev, "reset",
>> +						  GPIOD_IS_OUT |
> GPIOD_ACTIVE_LOW);
>> +	/* property is optional, don't return error! */
>> +	if (hub->reset_gpio) {
>
> if (!hub->reset_gpio)
>       return 0;
<Venkatesh> As reset_gpio is optional property,  by returning 0
the i2c sequence
wont be executed.
The code in if (hub->reset_gpio) { ... } only toggles the GPIO reset ?
<Venkatesh> Yes...if(hub->reset_gpio) only toggles the GPIO reset.
So you can return 0 if reset_gpio is NULL and exit early . What's the problem ?
<Venkatesh> if reset_gpio is NULL by returning 0 we miss the below code which
does i2c initialization sequence.
...
Need to toggle the gpio and then does the i2c sequence if the reset_gpio is
present, else we do the i2c initialization sequence.
Please show me the i2c sequence that is missed in this patch hunk:

if (hub->reset_gpio) {
ret = dm_gpio_set_value(hub->reset_gpio, 1);


if (ret)


	return ret;


udelay(data->reset_us);


ret = dm_gpio_set_value(hub->reset_gpio, 0);


if (ret)


	return ret;


udelay(data->power_on_delay_us);


}

return 0;

The i2c sequence change is part of   [PATCH v4 4/7] usb: onboard-hub: Add i2c initialization for usb5744 hub of the series
+	if (data->init) {
+		ret = data->init(dev);
+		if (ret) {
+			dev_err(dev, "onboard i2c init failed: %d\n", ret);
+			goto err;
+		}
+	}
+
    return 0;
+err:
+	dm_gpio_set_value(hub->reset_gpio, 0);
+	return ret;
 }
Thanks
Venkatesh