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

[Marek Vasut]

On 10/4/24 4:57 PM, Abbarapu, Venkatesh wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Marek Vasut <marex at denx.de>
>> Sent: Friday, October 4, 2024 7:22 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/4/24 2:03 PM, Abbarapu, Venkatesh wrote:
>>> Hi,
>>>
>>>> -----Original Message-----
>>>> From: Marek Vasut <marex at denx.de>
>>>> Sent: Friday, October 4, 2024 4:52 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/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;
>>>
>>> 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;
>>>    }
>> This does not seem to be part of the if (hub->reset_gpio) { ... } code block ?
> Yes...this is not part of the if (hub->reset_gpio) {...} code block.
> As I explained above if reset_gpio is NULL by returning 0 we miss the code which
>   does i2c initialization sequence which is part of [PATCH v4 4/7] usb: onboard-hub: Add i2c
>   initialization for usb5744 hub of the series.
> 
> The sequence is "Toggle the gpio and then do the i2c initialization sequence if the
> reset_gpio is present, else we do only the i2c initialization sequence".
In that case, pull the GPIO handling into a dedicated function:

usb_onboard_hub_reset()
{
  reset_gpio = devm_gpiod_get_optional(...)
  if (!reset_gpio)
    return 0;
  // toggle reset here
  ...
  hub->reset_gpio = reset_gpio;
  return 0;
}

usb_onboard_hub_probe()
{
...
   ret = usb_onboard_hub_reset()
   if (ret) ...

   // do i2c stuff later
   return 0;
}