Message-ID:
 <MN0PR12MB5953AD101E185462A6CC61BBB7932@MN0PR12MB5953.namprd12.prod.outlook.com>
Date: Tue, 3 Sep 2024 07:19:09 +0000
From: "Pandey, Radhey Shyam" <radhey.shyam.pandey@....com>
To: Greg KH <gregkh@...uxfoundation.org>
CC: "mka@...omium.org" <mka@...omium.org>, "sakari.ailus@...ux.intel.com"
	<sakari.ailus@...ux.intel.com>, "wentong.wu@...el.com"
	<wentong.wu@...el.com>, "javier.carrasco@...fvision.net"
	<javier.carrasco@...fvision.net>, "stefan.eichenberger@...adex.com"
	<stefan.eichenberger@...adex.com>, "francesco.dolcini@...adex.com"
	<francesco.dolcini@...adex.com>, "jbrunet@...libre.com"
	<jbrunet@...libre.com>, "macpaul.lin@...iatek.com"
	<macpaul.lin@...iatek.com>, "frieder.schrempf@...tron.de"
	<frieder.schrempf@...tron.de>, "linux-usb@...r.kernel.org"
	<linux-usb@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "git (AMD-Xilinx)" <git@....com>
Subject: RE: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add Microchip usb5744
 SMBus programming support

> -----Original Message-----
> From: Greg KH <gregkh@...uxfoundation.org>
> Sent: Tuesday, September 3, 2024 12:10 PM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@....com>
> Cc: mka@...omium.org; sakari.ailus@...ux.intel.com;
> wentong.wu@...el.com; javier.carrasco@...fvision.net;
> stefan.eichenberger@...adex.com; francesco.dolcini@...adex.com;
> jbrunet@...libre.com; macpaul.lin@...iatek.com;
> frieder.schrempf@...tron.de; linux-usb@...r.kernel.org; linux-
> kernel@...r.kernel.org; git (AMD-Xilinx) <git@....com>
> Subject: Re: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add Microchip
> usb5744 SMBus programming support
> 
> 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@....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@...gkh
> >
> > 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?)

In onboard driver I am calling onboard_dev_5744_i2c_init() by
detecting at runtime if "i2c-bus" phandle is present. But the 
problem is i2c_smbus_* APIs are declared and defined only for 
#if IS_ENABLED(CONFIG_I2C).

Do you think we should implement no-ops for I2C smbus APIs
(in linux/i2c.h)? OR a simpler alternative would be to
add #if IS_ENABLED(CONFIG_I2C) check in the onboard_*_i2c_init()
and return error code if CONFIG_I2C is not defined ? 
Did a grep on #if IS_ENABLED(CONFIG_I2C) and find couple of 
drivers using this approach.

Thanks,
Radhey

Powered by blists - more mailing lists