[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 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".

Thanks
Venkatesh