Re: [PATCH v2 1/2] usb: misc: onboard_dev: extend platform data to add power on delay field

[Matthias Kaehlcke]

On Thu, Jul 18, 2024 at 12:53:42AM +0530, Radhey Shyam Pandey wrote:
> Introduce dedicated field 'power_on_delay_us' in onboard platform data
> and update its delay for USB5744 configuration. Hub itself requires some
> delay after reset to get to state where configuration data is going to
> be accepted. Without delay upcoming support for configuration via SMBUS
> is reporting a failure on the first SMBus write.
> 
> i2c 2-002d: error -ENXIO: BYPASS_UDC_SUSPEND bit configuration failed
> 
> Similar delay is likely also required for default configuration but
> because there is enough time (code execution) between reset and usage
> of the hub any issue is not visible but it doesn't mean delay shouldn't
> be reflected.
> 
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xxxxxxx>
> Suggested-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
> ---
> Changes for v2:
> - New patch
> ---
>  drivers/usb/misc/onboard_usb_dev.c | 1 +
>  drivers/usb/misc/onboard_usb_dev.h | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
> index f2bcc1a8b95f..94d5424841fd 100644
> --- a/drivers/usb/misc/onboard_usb_dev.c
> +++ b/drivers/usb/misc/onboard_usb_dev.c
> @@ -98,6 +98,7 @@ static int onboard_dev_power_on(struct onboard_dev *onboard_dev)
>  
>  	fsleep(onboard_dev->pdata->reset_us);
>  	gpiod_set_value_cansleep(onboard_dev->reset_gpio, 0);
> +	fsleep(onboard_dev->pdata->power_on_delay_us);
>  
>  	onboard_dev->is_powered_on = true;
>  
> diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h
> index fbba549c0f47..82c76a0b3346 100644
> --- a/drivers/usb/misc/onboard_usb_dev.h
> +++ b/drivers/usb/misc/onboard_usb_dev.h
> @@ -10,6 +10,7 @@
>  
>  struct onboard_dev_pdata {
>  	unsigned long reset_us;		/* reset pulse width in us */
> +	unsigned long power_on_delay_us; /* power on pulse width in us */

nit: it isn't really a pulse width, just a simple delay.

>  	unsigned int num_supplies;	/* number of supplies */
>  	const char * const supply_names[MAX_SUPPLIES];
>  	bool is_hub;
> @@ -24,6 +25,7 @@ static const struct onboard_dev_pdata microchip_usb424_data = {
>  
>  static const struct onboard_dev_pdata microchip_usb5744_data = {
>  	.reset_us = 0,
> +	.power_on_delay_us = 10000,
>  	.num_supplies = 2,
>  	.supply_names = { "vdd", "vdd2" },
>  	.is_hub = true,
> -- 
> 2.34.1
>