Re: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add Microchip usb5744 SMBus programming support

[Greg KH <gregkh@linuxfoundation.org>]

On Sun, Sep 01, 2024 at 05:38:39PM +0530, Radhey Shyam Pandey wrote:
> usb5744 supports SMBus Configuration and it may be configured via the
> SMBus slave interface during the hub start-up configuration stage.
> 
> To program it driver uses i2c-bus phandle (added in commit '02be19e914b8
> dt-bindings: usb: Add support for Microchip usb5744 hub controller') to
> get i2c client device and then based on usb5744 compatible check calls
> usb5744 i2c default initialization sequence.
> 
> Apart from the USB command attach, prevent the hub from suspend.
> when the USB Attach with SMBus (0xAA56) command is issued to the hub,
> the hub is getting enumerated and then it puts in a suspend mode.
> This causes the hub to NAK any SMBus access made by the SMBus Master
> during this period and not able to see the hub's slave address while
> running the "i2c probe" command.
> 
> Prevent the MCU from putting the HUB in suspend mode through register
> write. The BYPASS_UDC_SUSPEND bit (Bit 3) of the RuntimeFlags2
> register at address 0x411D controls this aspect of the hub. The
> BYPASS_UDC_SUSPEND bit in register 0x411Dh must be set to ensure that the
> MCU is always enabled and ready to respond to SMBus runtime commands.
> This register needs to be written before the USB attach command is issued.
> 
> The byte sequence is as follows:
> Slave addr: 0x2d           00 00 05 00 01 41 1D 08
> Slave addr: 0x2d           99 37 00
> Slave addr: 0x2d           AA 56 00
> 
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
> Changes for v4:
> - Fix error: implicit declaration of function 'i2c_smbus_*' APIs by
>   introducing a dependency on I2C_CONFIG. This error is reported
>   by kernel test on v3 series and usb:usb-testing 20/25 branch.
>   https://lore.kernel.org/all/2024082503-uncoated-chaperone-7f70@gregkh
> 
> Changes for v3:
> - Add comment for UDC suspend sequence.
> - Drop USB5744_CREG_MEM_NBYTES and USB5744_CREG_NBYTES and replace
>   it with literal + comment.
> - Move microchip defines to source file.
> 
> Changes for v2:
> - Move power on reset delay to separate patch.
> - Switch to compatible based check for calling usb5755
>   onboard_dev_5744_i2c_init(). This is done to make
>   onboard_dev_5744_i2c_init() as static.
> - Fix subsystem "usb: misc: onboard_usb_dev:..."
> - Use #define for different register bits instead of magic values.
> - Use err_power_off label name.
> - Modified commit description to be in sync with v2 changes.
> ---
>  drivers/usb/misc/Kconfig           |  2 +-
>  drivers/usb/misc/onboard_usb_dev.c | 73 ++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index 50b86d531701..cb5e47d439ab 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -318,7 +318,7 @@ config BRCM_USB_PINMAP
>  
>  config USB_ONBOARD_DEV
>  	tristate "Onboard USB device support"
> -	depends on OF
> +	depends on OF && I2C

This feels wrong.

While a single device that this driver supports might need i2c, not all
of the devices do, so you have the potential to drag in a bunch of code
here for devices that do not have/need i2c at all, right?

Any way to "split this out" into a smaller chunk?  Or better yet, just
detect at runtime if you need/want to call those i2c functions (and they
should have no-ops for when i2c is not enabled, right?)

thanks,

greg k-h