> -----Original Message----- > From: Matthias Kaehlcke <matthias@xxxxxxxxxxxx> > Sent: Wednesday, September 18, 2024 10:37 PM > To: Pandey, Radhey Shyam <radhey.shyam.pandey@xxxxxxx> > Cc: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>; mka@xxxxxxxxxxxx; > sakari.ailus@xxxxxxxxxxxxxxx; wentong.wu@xxxxxxxxx; > javier.carrasco@xxxxxxxxxxxxxx; stefan.eichenberger@xxxxxxxxxxx; > francesco.dolcini@xxxxxxxxxxx; jbrunet@xxxxxxxxxxxx; macpaul.lin@xxxxxxxxxxxx; > frieder.schrempf@xxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx> > Subject: Re: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add Microchip usb5744 > SMBus programming support > > El Tue, Sep 03, 2024 at 07:19:09AM GMT Pandey, Radhey Shyam ha dit: > > > > -----Original Message----- > > > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> > > > Sent: Tuesday, September 3, 2024 12:10 PM > > > To: Pandey, Radhey Shyam <radhey.shyam.pandey@xxxxxxx> > > > Cc: mka@xxxxxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxx; > > > wentong.wu@xxxxxxxxx; javier.carrasco@xxxxxxxxxxxxxx; > > > stefan.eichenberger@xxxxxxxxxxx; francesco.dolcini@xxxxxxxxxxx; > > > jbrunet@xxxxxxxxxxxx; macpaul.lin@xxxxxxxxxxxx; > > > frieder.schrempf@xxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; linux- > > > kernel@xxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx> > > > 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@xxxxxxx> > > > > --- > > > > 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?) > > > > 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. > > I expect using IS_ENABLED(CONFIG_I2C) would cause issues when > USB_ONBOARD_DEV=y and CONFIG_I2C=m. > > Another option could be adding an option USB_ONBOARD_DEV_USB5744 (or > similar) and make that dependent on CONFIG_I2C. Yes, theoretically this can also be valid build configuration. I assume your suggestion is something like below: +config USB_ONBOARD_DEV_USB5744 + tristate "Onboard USB Microchip usb5744 hub with SMBus support" + depends on USB_ONBOARD_DEV && I2C + help + Say Y here if you want to support onboard USB Microchip usb5744 + hub that requires SMBus initialization. diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c index 560591e02d6a..6169780a18d9 100644 --- a/drivers/usb/misc/onboard_usb_dev.c +++ b/drivers/usb/misc/onboard_usb_dev.c @@ -311,7 +311,7 @@ static void onboard_dev_attach_usb_driver(struct work_struct *work) static int onboard_dev_5744_i2c_init(struct i2c_client *client) { -#if IS_ENABLED(CONFIG_I2C) +#if IS_ENABLED(CONFIG_USB_ONBOARD_DEV_USB5744) <*> Onboard USB device support <M> Onboard USB Microchip usb5744 hub with SMBus support Here IS_ENABLED will report true either onboard usb5744 is module/in-built. So, we hit same issue as USB_ONBOARD_DEV=y and I2C=m. FYI, as this series merged to -next based on our discussion I will send another series fixing this particular build configuration and other coding style changes based on v4 review comments. Thanks, Radhey
