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

[Marek Vasut]

On 10/4/24 5:02 AM, Abbarapu, Venkatesh wrote:
> Hi Marek,
> 
>> -----Original Message-----
>> From: Marek Vasut <marex at denx.de>
>> Sent: Thursday, October 3, 2024 10:35 PM
>> To: Abbarapu, Venkatesh <venkatesh.abbarapu at amd.com>; u-boot at lists.denx.de
>> Cc: Simek, Michal <michal.simek at amd.com>; fabrice.gasnier at foss.st.com; git
>> (AMD-Xilinx) <git at 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 at denx.de>
>>>> Sent: Thursday, October 3, 2024 5:38 PM
>>>> To: Abbarapu, Venkatesh <venkatesh.abbarapu at amd.com>;
>>>> u-boot at lists.denx.de
>>>> Cc: Simek, Michal <michal.simek at amd.com>;
>>>> fabrice.gasnier at foss.st.com; git
>>>> (AMD-Xilinx) <git at 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;