Re: [PATCH v5 1/5] usb: Add support for Intel LJCA device

[Ye, Xiang]

Hi Lee,

Thanks for the review.
On Mon, Mar 13, 2023 at 05:03:41PM +0000, Lee Jones wrote:
> On Mon, 13 Mar 2023, Ye Xiang wrote:
> 
> > This patch implements the USB part of Intel USB-I2C/GPIO/SPI adapter
> > device named "La Jolla Cove Adapter" (LJCA).
> >
> > The communication between the various LJCA module drivers and the
> > hardware will be muxed/demuxed by this driver. The sub-module of
> > LJCA can use ljca_transfer() to issue a transfer between host
> > and hardware.
> >
> > Each sub-module of LJCA device is identified by type field within
> > the LJCA message header.
> >
> > The minimum code in ASL that covers this board is
> > Scope (\_SB.PCI0.DWC3.RHUB.HS01)
> >     {
> >         Device (GPIO)
> >         {
> >             Name (_ADR, Zero)
> >             Name (_STA, 0x0F)
> >         }
> >
> >         Device (I2C)
> >         {
> >             Name (_ADR, One)
> >             Name (_STA, 0x0F)
> >         }
> >
> >         Device (SPI)
> >         {
> >             Name (_ADR, 0x02)
> >             Name (_STA, 0x0F)
> >         }
> >     }
> >
> > Signed-off-by: Ye Xiang <xiang.ye@xxxxxxxxx>
> > Reviewed-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/usb/misc/Kconfig  |  13 +
> >  drivers/usb/misc/Makefile |   1 +
> >  drivers/usb/misc/ljca.c   | 998 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/usb/ljca.h  |  95 ++++
> >  4 files changed, 1107 insertions(+)
> >  create mode 100644 drivers/usb/misc/ljca.c
> >  create mode 100644 include/linux/usb/ljca.h
> >
> > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> > index a5f7652db7da..59ec120c26d4 100644
> > --- a/drivers/usb/misc/Kconfig
> > +++ b/drivers/usb/misc/Kconfig
> > @@ -273,6 +273,19 @@ config USB_LINK_LAYER_TEST
> >  	  Layer Test Device. Say Y only when you want to conduct USB Super Speed
> >  	  Link Layer Test for host controllers.
> >
> > +config USB_LJCA
> > +	tristate "Intel La Jolla Cove Adapter support"
> > +	select MFD_CORE
> > +	depends on USB
> > +	help
> > +	  This adds support for Intel La Jolla Cove USB-I2C/SPI/GPIO
> > +	  Master Adapter (LJCA). Additional drivers such as I2C_LJCA,
> > +	  GPIO_LJCA and SPI_LJCA must be enabled in order to use the
> > +	  functionality of the device.
> > +
> > +	  This driver can also be built as a module. If so, the module
> > +	  will be called ljca.
> > +
> >  config USB_CHAOSKEY
> >  	tristate "ChaosKey random number generator driver support"
> >  	depends on HW_RANDOM
> > diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> > index 93581baec3a8..6f6adfbe17e0 100644
> > --- a/drivers/usb/misc/Makefile
> > +++ b/drivers/usb/misc/Makefile
> > @@ -29,6 +29,7 @@ obj-$(CONFIG_USB_HUB_USB251XB)		+= usb251xb.o
> >  obj-$(CONFIG_USB_HSIC_USB3503)		+= usb3503.o
> >  obj-$(CONFIG_USB_HSIC_USB4604)		+= usb4604.o
> >  obj-$(CONFIG_USB_CHAOSKEY)		+= chaoskey.o
> > +obj-$(CONFIG_USB_LJCA)			+= ljca.o
> >
> >  obj-$(CONFIG_USB_SISUSBVGA)		+= sisusbvga/
> >  obj-$(CONFIG_USB_LINK_LAYER_TEST)	+= lvstest.o
> > diff --git a/drivers/usb/misc/ljca.c b/drivers/usb/misc/ljca.c
> > new file mode 100644
> > index 000000000000..ab98deaf0074
> > --- /dev/null
> > +++ b/drivers/usb/misc/ljca.c
> > @@ -0,0 +1,998 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Intel La Jolla Cove Adapter USB driver
> > + *
> > + * Copyright (c) 2023, Intel Corporation.
> > + */
> > +
> > +#include <linux/dev_printk.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/core.h>
> 
> Please don't use the MFD API outside of drivers/mfd.
> 
> If you wish to use the API, please do.
> 
> Strip out (only) the MFD parts and move them into drivers/mfd.
I have no idea about how to split MFD parts out from this driver
currently. The MFD part just have mfd cells filling and the call
mfd_add_hotplug_devices to register mfd devices. How to module them
as an independent driver?
Would you give some hints or recommendations?

And I am a little comfused about where this USB device driver should
be put to (drivers/mfd or drivers/usb).

As far as I know, where a driver should be put is based on what
it provides. This driver just do some urb package transfer to provides
multi-functions, such as GPIO function, I2C function, SPI function.
so it should be under drivers/mfd folder. Please correct me, if
something is wrong. Thanks

> 
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/mutex.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +#include <linux/usb.h>
> > +#include <linux/usb/ljca.h>
>
--
Thanks
Ye Xiang