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

Ye, Xiang
2023-03-14T08:03:52Z

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@intel.com>
> > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > 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