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

[Abbarapu, Venkatesh]

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;
 }

Thanks
Venkatesh