diff mbox series

[v20,1/4] usb: Add support for Intel LJCA device

Message ID 1696833205-16716-2-git-send-email-wentong.wu@intel.com
State Accepted
Commit acd6199f195d6de814ac4090ce0864a613b1580e
Headers show
Series [v20,1/4] usb: Add support for Intel LJCA device | expand

Commit Message

Wu, Wentong Oct. 9, 2023, 6:33 a.m. UTC
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. Three modules (
I2C, GPIO, and SPI) are supported currently.

Each sub-module of LJCA device is identified by type field within
the LJCA message header.

The sub-modules of LJCA can use ljca_transfer() to issue a transfer
between host and hardware. And ljca_register_event_cb is exported
to LJCA sub-module drivers for hardware event subscription.

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: Wentong Wu <wentong.wu@intel.com>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/misc/Kconfig    |  13 +
 drivers/usb/misc/Makefile   |   1 +
 drivers/usb/misc/usb-ljca.c | 902 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/ljca.h    | 145 +++++++
 4 files changed, 1061 insertions(+)
 create mode 100644 drivers/usb/misc/usb-ljca.c
 create mode 100644 include/linux/usb/ljca.h

Comments

Andy Shevchenko Oct. 11, 2023, 10:21 a.m. UTC | #1
On Mon, Oct 09, 2023 at 02:33:22PM +0800, Wentong Wu wrote:
> 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. Three modules (
> I2C, GPIO, and SPI) are supported currently.
> 
> Each sub-module of LJCA device is identified by type field within
> the LJCA message header.
> 
> The sub-modules of LJCA can use ljca_transfer() to issue a transfer
> between host and hardware. And ljca_register_event_cb is exported
> to LJCA sub-module drivers for hardware event subscription.
> 
> 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)
>         }
>     }

This commit message is not true anymore, or misleading at bare minimum.
The ACPI specification is crystal clear about usage _ADR and _HID, i.e.
they must NOT be used together for the same device node. So, can you
clarify how the DSDT is organized and update the commit message and
it may require (quite likely) to redesign the architecture of this
driver. Sorry I missed this from previous rounds as I was busy by
something else.

Greg, please do not promote this to the next before above will be clarified.

P.S> Using _ADR and _HID together is an immediate NAK from me.
Hans de Goede Oct. 11, 2023, 10:37 a.m. UTC | #2
Hi,

On 10/11/23 12:21, Andy Shevchenko wrote:
> On Mon, Oct 09, 2023 at 02:33:22PM +0800, Wentong Wu wrote:
>> 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. Three modules (
>> I2C, GPIO, and SPI) are supported currently.
>>
>> Each sub-module of LJCA device is identified by type field within
>> the LJCA message header.
>>
>> The sub-modules of LJCA can use ljca_transfer() to issue a transfer
>> between host and hardware. And ljca_register_event_cb is exported
>> to LJCA sub-module drivers for hardware event subscription.
>>
>> 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)
>>         }
>>     }
> 
> This commit message is not true anymore, or misleading at bare minimum.
> The ACPI specification is crystal clear about usage _ADR and _HID, i.e.
> they must NOT be used together for the same device node. So, can you
> clarify how the DSDT is organized and update the commit message and
> it may require (quite likely) to redesign the architecture of this
> driver. Sorry I missed this from previous rounds as I was busy by
> something else.

This part of the commit message unfortunately is not accurate.
_ADR is not used in either DSDTs of shipping hw; nor in the code.

The code in question parsing the relevant part of the DSDT looks
like this:

static int ljca_match_device_ids(struct acpi_device *adev, void *data)
{
        struct ljca_match_ids_walk_data *wd = data;
        const char *uid = acpi_device_uid(adev);

        if (acpi_match_device_ids(adev, wd->ids))
                return 0;

        if (!wd->uid)
                goto match;

        if (!uid)
                /*
                 * Some DSDTs have only one ACPI companion for the two I2C
                 * controllers and they don't set a UID at all (e.g. Dell
                 * Latitude 9420). On these platforms only the first I2C
                 * controller is used, so if a HID match has no UID we use
                 * "0" as the UID and assign ACPI companion to the first
                 * I2C controller.
                 */
                uid = "0";
        else
                uid = strchr(uid, wd->uid[0]);

        if (!uid || strcmp(uid, wd->uid))
                return 0;

match:
        wd->adev = adev;

        return 1;
}

Notice that it check _UID (for some child devices, only those of
which there may be more then 1 have a UID set in the DSDT) and
that in case of requested but missing UID it assumes UID = "0"
for compatibility with older DSDTs which lack the UID.

And relevant DSDT bits from early hw (TigerLake Dell Latitude 9420)
Note no UID for the I2C node even though the LJCA USB IO expander
has 2 I2C controllers :

    Scope (_SB.PC00.XHCI.RHUB.HS06)
    {
            Device (VGPO)
            {
                Name (_HID, "INTC1074")  // _HID: Hardware ID
                Name (_DDN, "Intel UsbGpio Device")  // _DDN: DOS Device Name
            }

            Device (VI2C)
            {
                Name (_HID, "INTC1075")  // _HID: Hardware ID
                Name (_DDN, "Intel UsbI2C Device")  // _DDN: DOS Device Name
            }
    }

And for newer hw (Lenovo Thinkpad X1 yoga gen7, alder lake):

        Scope (_SB.PC00.XHCI.RHUB.HS08)
        {
            Device (VGPO)
            {
                Name (_HID, "INTC1096")  // _HID: Hardware ID
                Name (_DDN, "Intel UsbGpio Device")  // _DDN: DOS Device Name
            }

            Device (VIC0)
            {
                Name (_HID, "INTC1097")  // _HID: Hardware ID
                Name (_DDN, "Intel UsbI2C Device")  // _DDN: DOS Device Name
                Name (_UID, Zero)  // _UID: Unique ID
            }

            Device (VIC1)
            {
                Name (_HID, "INTC1097")  // _HID: Hardware ID
                Name (_DDN, "Intel UsbI2C Device")  // _DDN: DOS Device Name
                Name (_UID, One)  // _UID: Unique ID
            }

            Device (VSPI)
            {
                Name (_HID, "INTC1098")  // _HID: Hardware ID
                Name (_DDN, "Intel UsbSPI Device")  // _DDN: DOS Device Name
            }
        }

Note UIDs are used for the I2C controllers but not for the singleton
SPI and GPIO controllers.

TL;DR: there is nothing to worry about here, but the commit message
should be updated to reflect reality.

Regards,

Hans
Andy Shevchenko Oct. 11, 2023, 10:43 a.m. UTC | #3
On Wed, Oct 11, 2023 at 12:37:51PM +0200, Hans de Goede wrote:
> On 10/11/23 12:21, Andy Shevchenko wrote:
> > On Mon, Oct 09, 2023 at 02:33:22PM +0800, Wentong Wu wrote:

...

> TL;DR: there is nothing to worry about here, but the commit message
> should be updated to reflect reality.

I have just sent the similar worry, but thanks that you have checked
the code and we don't need to worry too much.
Wu, Wentong Oct. 11, 2023, 12:50 p.m. UTC | #4
> From: Hans de Goede <hdegoede>
> 
> Hi,
> 
> On 10/11/23 12:21, Andy Shevchenko wrote:
> > On Mon, Oct 09, 2023 at 02:33:22PM +0800, Wentong Wu wrote:
> >> 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. Three modules ( I2C,
> >> GPIO, and SPI) are supported currently.
> >>
> >> Each sub-module of LJCA device is identified by type field within the
> >> LJCA message header.
> >>
> >> The sub-modules of LJCA can use ljca_transfer() to issue a transfer
> >> between host and hardware. And ljca_register_event_cb is exported to
> >> LJCA sub-module drivers for hardware event subscription.
> >>
> >> 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)
> >>         }
> >>     }
> >
> > This commit message is not true anymore, or misleading at bare minimum.
> > The ACPI specification is crystal clear about usage _ADR and _HID, i.e.
> > they must NOT be used together for the same device node. So, can you
> > clarify how the DSDT is organized and update the commit message and it
> > may require (quite likely) to redesign the architecture of this
> > driver. Sorry I missed this from previous rounds as I was busy by
> > something else.
> 
> This part of the commit message unfortunately is not accurate.
> _ADR is not used in either DSDTs of shipping hw; nor in the code.

We have covered the _ADR in the code like below, it first try to find the
child device based on _ADR, if not found, it will check the _HID, and there
is clear comment in the function.

/* bind auxiliary device to acpi device */
static void ljca_auxdev_acpi_bind(struct ljca_adapter *adap,
				   struct auxiliary_device *auxdev,
				   u64 adr, u8 id)
{
	struct ljca_match_ids_walk_data wd = { 0 };
	struct acpi_device *parent, *adev;
	struct device *dev = adap->dev;
	char uid[4];

	parent = ACPI_COMPANION(dev);
	if (!parent)
		return;

	/*
	 * get auxdev ACPI handle from the ACPI device directly
	 * under the parent that matches _ADR.
	 */
	adev = acpi_find_child_device(parent, adr, false);
	if (adev) {
		ACPI_COMPANION_SET(&auxdev->dev, adev);
		return;
	}

	/*
	 * _ADR is a grey area in the ACPI specification, some
	 * platforms use _HID to distinguish children devices.
	 */
	switch (adr) {
	case LJCA_GPIO_ACPI_ADR:
		wd.ids = ljca_gpio_hids;
		break;
	case LJCA_I2C1_ACPI_ADR:
	case LJCA_I2C2_ACPI_ADR:
		snprintf(uid, sizeof(uid), "%d", id);
		wd.uid = uid;
		wd.ids = ljca_i2c_hids;
		break;
	case LJCA_SPI1_ACPI_ADR:
	case LJCA_SPI2_ACPI_ADR:
		wd.ids = ljca_spi_hids;
		break;
	default:
		dev_warn(dev, "unsupported _ADR\n");
		return;
	}

	acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd);

> 
> The code in question parsing the relevant part of the DSDT looks like this:
> 
> static int ljca_match_device_ids(struct acpi_device *adev, void *data) {
>         struct ljca_match_ids_walk_data *wd = data;
>         const char *uid = acpi_device_uid(adev);
> 
>         if (acpi_match_device_ids(adev, wd->ids))
>                 return 0;
> 
>         if (!wd->uid)
>                 goto match;
> 
>         if (!uid)
>                 /*
>                  * Some DSDTs have only one ACPI companion for the two I2C
>                  * controllers and they don't set a UID at all (e.g. Dell
>                  * Latitude 9420). On these platforms only the first I2C
>                  * controller is used, so if a HID match has no UID we use
>                  * "0" as the UID and assign ACPI companion to the first
>                  * I2C controller.
>                  */
>                 uid = "0";
>         else
>                 uid = strchr(uid, wd->uid[0]);
> 
>         if (!uid || strcmp(uid, wd->uid))
>                 return 0;
> 
> match:
>         wd->adev = adev;
> 
>         return 1;
> }
> 
> Notice that it check _UID (for some child devices, only those of which there may
> be more then 1 have a UID set in the DSDT) and that in case of requested but
> missing UID it assumes UID = "0"
> for compatibility with older DSDTs which lack the UID.
> 
> And relevant DSDT bits from early hw (TigerLake Dell Latitude 9420) Note no UID
> for the I2C node even though the LJCA USB IO expander has 2 I2C controllers :
> 
>     Scope (_SB.PC00.XHCI.RHUB.HS06)
>     {
>             Device (VGPO)
>             {
>                 Name (_HID, "INTC1074")  // _HID: Hardware ID
>                 Name (_DDN, "Intel UsbGpio Device")  // _DDN: DOS Device Name
>             }
> 
>             Device (VI2C)
>             {
>                 Name (_HID, "INTC1075")  // _HID: Hardware ID
>                 Name (_DDN, "Intel UsbI2C Device")  // _DDN: DOS Device Name
>             }
>     }
> 
> And for newer hw (Lenovo Thinkpad X1 yoga gen7, alder lake):
> 
>         Scope (_SB.PC00.XHCI.RHUB.HS08)
>         {
>             Device (VGPO)
>             {
>                 Name (_HID, "INTC1096")  // _HID: Hardware ID
>                 Name (_DDN, "Intel UsbGpio Device")  // _DDN: DOS Device Name
>             }
> 
>             Device (VIC0)
>             {
>                 Name (_HID, "INTC1097")  // _HID: Hardware ID
>                 Name (_DDN, "Intel UsbI2C Device")  // _DDN: DOS Device Name
>                 Name (_UID, Zero)  // _UID: Unique ID
>             }
> 
>             Device (VIC1)
>             {
>                 Name (_HID, "INTC1097")  // _HID: Hardware ID
>                 Name (_DDN, "Intel UsbI2C Device")  // _DDN: DOS Device Name
>                 Name (_UID, One)  // _UID: Unique ID
>             }
> 
>             Device (VSPI)
>             {
>                 Name (_HID, "INTC1098")  // _HID: Hardware ID
>                 Name (_DDN, "Intel UsbSPI Device")  // _DDN: DOS Device Name
>             }
>         }
> 
> Note UIDs are used for the I2C controllers but not for the singleton SPI and GPIO
> controllers.
> 
> TL;DR: there is nothing to worry about here, but the commit message should be
> updated to reflect reality.
> 
> Regards,
> 
> Hans
Hans de Goede Oct. 12, 2023, 11:14 a.m. UTC | #5
Hi,

On 10/11/23 14:50, Wu, Wentong wrote:
>> From: Hans de Goede <hdegoede>
>>
>> Hi,
>>
>> On 10/11/23 12:21, Andy Shevchenko wrote:
>>> On Mon, Oct 09, 2023 at 02:33:22PM +0800, Wentong Wu wrote:
>>>> 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. Three modules ( I2C,
>>>> GPIO, and SPI) are supported currently.
>>>>
>>>> Each sub-module of LJCA device is identified by type field within the
>>>> LJCA message header.
>>>>
>>>> The sub-modules of LJCA can use ljca_transfer() to issue a transfer
>>>> between host and hardware. And ljca_register_event_cb is exported to
>>>> LJCA sub-module drivers for hardware event subscription.
>>>>
>>>> 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)
>>>>         }
>>>>     }
>>>
>>> This commit message is not true anymore, or misleading at bare minimum.
>>> The ACPI specification is crystal clear about usage _ADR and _HID, i.e.
>>> they must NOT be used together for the same device node. So, can you
>>> clarify how the DSDT is organized and update the commit message and it
>>> may require (quite likely) to redesign the architecture of this
>>> driver. Sorry I missed this from previous rounds as I was busy by
>>> something else.
>>
>> This part of the commit message unfortunately is not accurate.
>> _ADR is not used in either DSDTs of shipping hw; nor in the code.
> 
> We have covered the _ADR in the code like below, it first try to find the
> child device based on _ADR, if not found, it will check the _HID, and there
> is clear comment in the function.
> 
> /* bind auxiliary device to acpi device */
> static void ljca_auxdev_acpi_bind(struct ljca_adapter *adap,
> 				   struct auxiliary_device *auxdev,
> 				   u64 adr, u8 id)
> {
> 	struct ljca_match_ids_walk_data wd = { 0 };
> 	struct acpi_device *parent, *adev;
> 	struct device *dev = adap->dev;
> 	char uid[4];
> 
> 	parent = ACPI_COMPANION(dev);
> 	if (!parent)
> 		return;
> 
> 	/*
> 	 * get auxdev ACPI handle from the ACPI device directly
> 	 * under the parent that matches _ADR.
> 	 */
> 	adev = acpi_find_child_device(parent, adr, false);
> 	if (adev) {
> 		ACPI_COMPANION_SET(&auxdev->dev, adev);
> 		return;
> 	}
> 
> 	/*
> 	 * _ADR is a grey area in the ACPI specification, some
> 	 * platforms use _HID to distinguish children devices.
> 	 */
> 	switch (adr) {
> 	case LJCA_GPIO_ACPI_ADR:
> 		wd.ids = ljca_gpio_hids;
> 		break;
> 	case LJCA_I2C1_ACPI_ADR:
> 	case LJCA_I2C2_ACPI_ADR:
> 		snprintf(uid, sizeof(uid), "%d", id);
> 		wd.uid = uid;
> 		wd.ids = ljca_i2c_hids;
> 		break;
> 	case LJCA_SPI1_ACPI_ADR:
> 	case LJCA_SPI2_ACPI_ADR:
> 		wd.ids = ljca_spi_hids;
> 		break;
> 	default:
> 		dev_warn(dev, "unsupported _ADR\n");
> 		return;
> 	}
> 
> 	acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd);

Ah ok, I see. So the code:

1. First tries to find the matching child acpi_device for the auxdev by ADR

2. If 1. fails then falls back to HID + UID matching

And there are DSDTs which use either:

1. Only use _ADR to identify which child device is which, like the example
   DSDT snippet from the commit msg.

2. Only use _HID + _UID like the 2 example DSDT snippets from me email

But there never is a case where both _ADR and _HID are used at
the same time (which would be an ACPI spec violation as Andy said).

So AFAICT there is no issue here since  _ADR and _HID are never
user at the same time and the commit message correctly describes
scenario 1. from above, so the commit message is fine too.

So I believe that we can continue with this patch series in
its current v20 form, which has already been staged for
going into -next by Greg.

Andy can you confirm that moving ahead with the current
version is ok ?

Regards,

Hans
Andy Shevchenko Oct. 13, 2023, 8:05 p.m. UTC | #6
On Thu, Oct 12, 2023 at 01:14:23PM +0200, Hans de Goede wrote:
> On 10/11/23 14:50, Wu, Wentong wrote:
> >> On 10/11/23 12:21, Andy Shevchenko wrote:
> >>> On Mon, Oct 09, 2023 at 02:33:22PM +0800, Wentong Wu wrote:
> >>>> 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. Three modules ( I2C,
> >>>> GPIO, and SPI) are supported currently.
> >>>>
> >>>> Each sub-module of LJCA device is identified by type field within the
> >>>> LJCA message header.
> >>>>
> >>>> The sub-modules of LJCA can use ljca_transfer() to issue a transfer
> >>>> between host and hardware. And ljca_register_event_cb is exported to
> >>>> LJCA sub-module drivers for hardware event subscription.
> >>>>
> >>>> 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)
> >>>>         }
> >>>>     }
> >>>
> >>> This commit message is not true anymore, or misleading at bare minimum.
> >>> The ACPI specification is crystal clear about usage _ADR and _HID, i.e.
> >>> they must NOT be used together for the same device node. So, can you
> >>> clarify how the DSDT is organized and update the commit message and it
> >>> may require (quite likely) to redesign the architecture of this
> >>> driver. Sorry I missed this from previous rounds as I was busy by
> >>> something else.
> >>
> >> This part of the commit message unfortunately is not accurate.
> >> _ADR is not used in either DSDTs of shipping hw; nor in the code.
> > 
> > We have covered the _ADR in the code like below, it first try to find the
> > child device based on _ADR, if not found, it will check the _HID, and there
> > is clear comment in the function.
> > 
> > /* bind auxiliary device to acpi device */
> > static void ljca_auxdev_acpi_bind(struct ljca_adapter *adap,
> > 				   struct auxiliary_device *auxdev,
> > 				   u64 adr, u8 id)
> > {
> > 	struct ljca_match_ids_walk_data wd = { 0 };
> > 	struct acpi_device *parent, *adev;
> > 	struct device *dev = adap->dev;
> > 	char uid[4];
> > 
> > 	parent = ACPI_COMPANION(dev);
> > 	if (!parent)
> > 		return;
> > 
> > 	/*
> > 	 * get auxdev ACPI handle from the ACPI device directly
> > 	 * under the parent that matches _ADR.
> > 	 */
> > 	adev = acpi_find_child_device(parent, adr, false);
> > 	if (adev) {
> > 		ACPI_COMPANION_SET(&auxdev->dev, adev);
> > 		return;
> > 	}
> > 
> > 	/*
> > 	 * _ADR is a grey area in the ACPI specification, some
> > 	 * platforms use _HID to distinguish children devices.
> > 	 */
> > 	switch (adr) {
> > 	case LJCA_GPIO_ACPI_ADR:
> > 		wd.ids = ljca_gpio_hids;
> > 		break;
> > 	case LJCA_I2C1_ACPI_ADR:
> > 	case LJCA_I2C2_ACPI_ADR:
> > 		snprintf(uid, sizeof(uid), "%d", id);
> > 		wd.uid = uid;
> > 		wd.ids = ljca_i2c_hids;
> > 		break;
> > 	case LJCA_SPI1_ACPI_ADR:
> > 	case LJCA_SPI2_ACPI_ADR:
> > 		wd.ids = ljca_spi_hids;
> > 		break;
> > 	default:
> > 		dev_warn(dev, "unsupported _ADR\n");
> > 		return;
> > 	}
> > 
> > 	acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd);
> 
> Ah ok, I see. So the code:
> 
> 1. First tries to find the matching child acpi_device for the auxdev by ADR
> 
> 2. If 1. fails then falls back to HID + UID matching
> 
> And there are DSDTs which use either:
> 
> 1. Only use _ADR to identify which child device is which, like the example
>    DSDT snippet from the commit msg.
> 
> 2. Only use _HID + _UID like the 2 example DSDT snippets from me email
> 
> But there never is a case where both _ADR and _HID are used at
> the same time (which would be an ACPI spec violation as Andy said).
> 
> So AFAICT there is no issue here since  _ADR and _HID are never
> user at the same time and the commit message correctly describes
> scenario 1. from above, so the commit message is fine too.
> 
> So I believe that we can continue with this patch series in
> its current v20 form, which has already been staged for
> going into -next by Greg.
> 
> Andy can you confirm that moving ahead with the current
> version is ok ?

Yes as we have a few weeks to fix corner cases.

What I'm worrying is that opening door for _ADR that seems never used is kinda
an overkill here (resolving non-existing problem). Looking at the design of the
driver I'm not sure why ACPI HIDs are collected somewhere else than in the
respective drivers. And looking at the ID lists themselves I am not sure why
the firmware of the respective hardware platforms are not using _CID.
Hans de Goede Oct. 14, 2023, 10:58 a.m. UTC | #7
Hi Andy,

On 10/13/23 22:05, Shevchenko, Andriy wrote:
> On Thu, Oct 12, 2023 at 01:14:23PM +0200, Hans de Goede wrote:

<snip>

>> Ah ok, I see. So the code:
>>
>> 1. First tries to find the matching child acpi_device for the auxdev by ADR
>>
>> 2. If 1. fails then falls back to HID + UID matching
>>
>> And there are DSDTs which use either:
>>
>> 1. Only use _ADR to identify which child device is which, like the example
>>    DSDT snippet from the commit msg.
>>
>> 2. Only use _HID + _UID like the 2 example DSDT snippets from me email
>>
>> But there never is a case where both _ADR and _HID are used at
>> the same time (which would be an ACPI spec violation as Andy said).
>>
>> So AFAICT there is no issue here since  _ADR and _HID are never
>> user at the same time and the commit message correctly describes
>> scenario 1. from above, so the commit message is fine too.
>>
>> So I believe that we can continue with this patch series in
>> its current v20 form, which has already been staged for
>> going into -next by Greg.
>>
>> Andy can you confirm that moving ahead with the current
>> version is ok ?
> 
> Yes as we have a few weeks to fix corner cases.
> 
> What I'm worrying is that opening door for _ADR that seems never used is kinda
> an overkill here (resolving non-existing problem).

I assume that there actually some DSDTs using the _ADR approach
and that this support is not there just for fun.

Wentong, can you confirm that the _ADR using codepaths are
actually used on some hardware / with some DSDTs out there ?

> Looking at the design of the
> driver I'm not sure why ACPI HIDs are collected somewhere else than in the
> respective drivers. And looking at the ID lists themselves I am not sure why
> the firmware of the respective hardware platforms are not using _CID.

This is a USB device which has 4 functions:

1. GPIO controller
2. I2C controller 1
3. I2C controller 2
4. SPI controller

The driver for the main USB interface uses
the new auxbus to create 4 child devices. The _ADR
or if that fails _HID + _UID matching is done to
find the correct acpi_device child of the acpi_device
which is the ACPI-companion of the main USB device.

After looking up the correct acpi_device child
this is then set as the fwnode / ACPI-companion
of the auxbus device created for that function.

Having the correct fwnode is important because other
parts of the DSDT reference this fwnode to specify
GPIO / I2C / SPI resources and if the fwnode of
the aux-device is not set correctly then the resources
for other devices referencing it (typically a camera
sensor) can not be found.

As for why the driver for the auxbus devices / children
do not use HID matching, AFAIK the auxbus has no support
for using ACPI (or DT) matching for aux-devices and these
drivers need to be auxiliary_driver's and bind to the
auxbus device and not to a platform_device instantiated for
the acpi_device since they need the auxbus device to access
the USB device.

Regards,

Hans
Hans de Goede Oct. 16, 2023, 7:35 a.m. UTC | #8
Hi,

On 10/16/23 07:52, Wu, Wentong wrote:
>> From: Hans de Goede <hdegoede>
>>
>> Hi Andy,
>>
>> On 10/13/23 22:05, Shevchenko, Andriy wrote:
>>> On Thu, Oct 12, 2023 at 01:14:23PM +0200, Hans de Goede wrote:
>>
>> <snip>
>>
>>>> Ah ok, I see. So the code:
>>>>
>>>> 1. First tries to find the matching child acpi_device for the auxdev
>>>> by ADR
>>>>
>>>> 2. If 1. fails then falls back to HID + UID matching
>>>>
>>>> And there are DSDTs which use either:
>>>>
>>>> 1. Only use _ADR to identify which child device is which, like the example
>>>>    DSDT snippet from the commit msg.
>>>>
>>>> 2. Only use _HID + _UID like the 2 example DSDT snippets from me
>>>> email
>>>>
>>>> But there never is a case where both _ADR and _HID are used at the
>>>> same time (which would be an ACPI spec violation as Andy said).
>>>>
>>>> So AFAICT there is no issue here since  _ADR and _HID are never user
>>>> at the same time and the commit message correctly describes scenario
>>>> 1. from above, so the commit message is fine too.
>>>>
>>>> So I believe that we can continue with this patch series in its
>>>> current v20 form, which has already been staged for going into -next
>>>> by Greg.
>>>>
>>>> Andy can you confirm that moving ahead with the current version is ok
>>>> ?
>>>
>>> Yes as we have a few weeks to fix corner cases.
>>>
>>> What I'm worrying is that opening door for _ADR that seems never used
>>> is kinda an overkill here (resolving non-existing problem).
>>
>> I assume that there actually some DSDTs using the _ADR approach and that this
>> support is not there just for fun.
> 
> right, it's not for fun, we use _ADR here is to reduce the maintain effort because
> currently it defines _HID for every new platform and the drivers have to be updated
> accordingly, while _ADR doesn't have that problem.

Hmm, this sounds to me like _ADR is currently not actually being used in any
shipping DSDTs ?

Adding support for it to the driver seems a bit premature then IMHO ?

Also HIDs can perfectly be re-used for compatible hardware in
a newer generation so that is really not a good argument to use _ADR
instead.

Regards,

Hans
Andy Shevchenko Oct. 16, 2023, 7:38 a.m. UTC | #9
On Mon, Oct 16, 2023 at 08:52:28AM +0300, Wu, Wentong wrote:
> > On 10/13/23 22:05, Shevchenko, Andriy wrote:
> > > On Thu, Oct 12, 2023 at 01:14:23PM +0200, Hans de Goede wrote:

<snip>

> > >> Ah ok, I see. So the code:
> > >>
> > >> 1. First tries to find the matching child acpi_device for the auxdev
> > >> by ADR
> > >>
> > >> 2. If 1. fails then falls back to HID + UID matching
> > >>
> > >> And there are DSDTs which use either:
> > >>
> > >> 1. Only use _ADR to identify which child device is which, like the example
> > >>    DSDT snippet from the commit msg.
> > >>
> > >> 2. Only use _HID + _UID like the 2 example DSDT snippets from me
> > >> email
> > >>
> > >> But there never is a case where both _ADR and _HID are used at the
> > >> same time (which would be an ACPI spec violation as Andy said).
> > >>
> > >> So AFAICT there is no issue here since  _ADR and _HID are never user
> > >> at the same time and the commit message correctly describes scenario
> > >> 1. from above, so the commit message is fine too.
> > >>
> > >> So I believe that we can continue with this patch series in its
> > >> current v20 form, which has already been staged for going into -next
> > >> by Greg.
> > >>
> > >> Andy can you confirm that moving ahead with the current version is ok
> > >> ?
> > >
> > > Yes as we have a few weeks to fix corner cases.
> > >
> > > What I'm worrying is that opening door for _ADR that seems never used
> > > is kinda an overkill here (resolving non-existing problem).
> > 
> > I assume that there actually some DSDTs using the _ADR approach and that this
> > support is not there just for fun.
> 
> right, it's not for fun, we use _ADR here is to reduce the maintain effort because
> currently it defines _HID for every new platform and the drivers have to be updated
> accordingly, while _ADR doesn't have that problem.

But this does not confirm if you have such devices. Moreover, My question about
_CID per function stays the same. Why firmware is not using it? In that case
you need only one ID per function in the driver (it might require some IDs in
the _HID, I don't remember that part of the spec by heart, i.e.  if _CID can be
only provided with existing _HID or not).

> > Wentong, can you confirm that the _ADR using codepaths are actually used on
> > some hardware / with some DSDTs out there ?
> 
> what I can share is that we will see.
> 
> > > Looking at the design of the
> > > driver I'm not sure why ACPI HIDs are collected somewhere else than in
> > > the respective drivers.
> 
> AFAIK, auxiliary bus doesn't support parsing fwnodes currently. Probably we can 
> support it for auxiliary bus in another patch. 

This is good idea!


> > > And looking at the ID lists themselves I am
> > > not sure why the firmware of the respective hardware platforms are not using
> > _CID.
> 
> I think firmware can select _CID as well, but the shipped hw doesn't use _CID,
> the driver has to make sure the shipped hw working as well. And switching to _CID
> for the shipped hw is not easy, and it has to change windows driver as well.

I understand, but at least you may stop growing list in the driver. And actually
using separate IDs for multifunctional device seems not ideal solution to me.

> > This is a USB device which has 4 functions:

Yes, I understand this part, but thank you for elaboration about auxbus, which
seems lack of needed support. And I would really like to see someone adds it there.

> > 1. GPIO controller
> > 2. I2C controller 1
> > 3. I2C controller 2
> > 4. SPI controller
> > 
> > The driver for the main USB interface uses the new auxbus to create 4 child
> > devices. The _ADR or if that fails _HID + _UID matching is done to find the
> > correct acpi_device child of the acpi_device which is the ACPI-companion of the
> > main USB device.
> > 
> > After looking up the correct acpi_device child this is then set as the fwnode /
> > ACPI-companion of the auxbus device created for that function.
> > 
> > Having the correct fwnode is important because other parts of the DSDT
> > reference this fwnode to specify GPIO / I2C / SPI resources and if the fwnode of
> > the aux-device is not set correctly then the resources for other devices
> > referencing it (typically a camera
> > sensor) can not be found.
> > 
> > As for why the driver for the auxbus devices / children do not use HID matching,
> > AFAIK the auxbus has no support for using ACPI (or DT) matching for aux-devices
> > and these drivers need to be auxiliary_driver's and bind to the auxbus device and
> > not to a platform_device instantiated for the acpi_device since they need the
> > auxbus device to access the USB device.
> 
> Yes, total agree. Thanks
Wu, Wentong Oct. 16, 2023, 3:05 p.m. UTC | #10
> From: Shevchenko, Andriy
> On Mon, Oct 16, 2023 at 08:52:28AM +0300, Wu, Wentong wrote:
> > > On 10/13/23 22:05, Shevchenko, Andriy wrote:
> > > > On Thu, Oct 12, 2023 at 01:14:23PM +0200, Hans de Goede wrote:
> 
> <snip>
> 
> > > >> Ah ok, I see. So the code:
> > > >>
> > > >> 1. First tries to find the matching child acpi_device for the
> > > >> auxdev by ADR
> > > >>
> > > >> 2. If 1. fails then falls back to HID + UID matching
> > > >>
> > > >> And there are DSDTs which use either:
> > > >>
> > > >> 1. Only use _ADR to identify which child device is which, like the example
> > > >>    DSDT snippet from the commit msg.
> > > >>
> > > >> 2. Only use _HID + _UID like the 2 example DSDT snippets from me
> > > >> email
> > > >>
> > > >> But there never is a case where both _ADR and _HID are used at
> > > >> the same time (which would be an ACPI spec violation as Andy said).
> > > >>
> > > >> So AFAICT there is no issue here since  _ADR and _HID are never
> > > >> user at the same time and the commit message correctly describes
> > > >> scenario 1. from above, so the commit message is fine too.
> > > >>
> > > >> So I believe that we can continue with this patch series in its
> > > >> current v20 form, which has already been staged for going into
> > > >> -next by Greg.
> > > >>
> > > >> Andy can you confirm that moving ahead with the current version
> > > >> is ok ?
> > > >
> > > > Yes as we have a few weeks to fix corner cases.
> > > >
> > > > What I'm worrying is that opening door for _ADR that seems never
> > > > used is kinda an overkill here (resolving non-existing problem).
> > >
> > > I assume that there actually some DSDTs using the _ADR approach and
> > > that this support is not there just for fun.
> >
> > right, it's not for fun, we use _ADR here is to reduce the maintain
> > effort because currently it defines _HID for every new platform and
> > the drivers have to be updated accordingly, while _ADR doesn't have that
> problem.
> 
> But this does not confirm if you have such devices. Moreover, My question
> about _CID per function stays the same. Why firmware is not using it?

Yes, both _ADR and _CID can stop growing list in the driver. And for _ADR, it also
only require one ID per function. I don't know why BIOS team doesn't select _CID,
but I have suggested use _ADR internally, and , to make things moving forward,
the driver adds support for _ADR here first. 

But you're right, _CID is another solution as well, we will discuss it with firmware
team more.

> In that case you need only one ID per function in the driver (it might require some
> IDs in the _HID, I don't remember that part of the spec by heart, i.e.  if _CID can be
> only provided with existing _HID or not).
> 
> > > Wentong, can you confirm that the _ADR using codepaths are actually
> > > used on some hardware / with some DSDTs out there ?
> >
> > what I can share is that we will see.
> >
> > > > Looking at the design of the
> > > > driver I'm not sure why ACPI HIDs are collected somewhere else
> > > > than in the respective drivers.
> >
> > AFAIK, auxiliary bus doesn't support parsing fwnodes currently.
> > Probably we can support it for auxiliary bus in another patch.
> 
> This is good idea!
> 
> 
> > > > And looking at the ID lists themselves I am not sure why the
> > > > firmware of the respective hardware platforms are not using
> > > _CID.
> >
> > I think firmware can select _CID as well, but the shipped hw doesn't
> > use _CID, the driver has to make sure the shipped hw working as well.
> > And switching to _CID for the shipped hw is not easy, and it has to change
> windows driver as well.
> 
> I understand, but at least you may stop growing list in the driver.
Yes, 

> And actually using separate IDs for multifunctional device seems not ideal
> solution to me.
Agree, I will consider _CID more, but currently to avoid this and also support
shipped hardware, _ADR is at least a choice.

BR,
Wentong

> > > This is a USB device which has 4 functions:
> 
> Yes, I understand this part, but thank you for elaboration about auxbus, which
> seems lack of needed support. And I would really like to see someone adds it
> there.
> 
> > > 1. GPIO controller
> > > 2. I2C controller 1
> > > 3. I2C controller 2
> > > 4. SPI controller
> > >
> > > The driver for the main USB interface uses the new auxbus to create
> > > 4 child devices. The _ADR or if that fails _HID + _UID matching is
> > > done to find the correct acpi_device child of the acpi_device which
> > > is the ACPI-companion of the main USB device.
> > >
> > > After looking up the correct acpi_device child this is then set as
> > > the fwnode / ACPI-companion of the auxbus device created for that function.
> > >
> > > Having the correct fwnode is important because other parts of the
> > > DSDT reference this fwnode to specify GPIO / I2C / SPI resources and
> > > if the fwnode of the aux-device is not set correctly then the
> > > resources for other devices referencing it (typically a camera
> > > sensor) can not be found.
> > >
> > > As for why the driver for the auxbus devices / children do not use
> > > HID matching, AFAIK the auxbus has no support for using ACPI (or DT)
> > > matching for aux-devices and these drivers need to be
> > > auxiliary_driver's and bind to the auxbus device and not to a
> > > platform_device instantiated for the acpi_device since they need the auxbus
> device to access the USB device.
> >
> > Yes, total agree. Thanks
> 
> --
> With Best Regards,
> Andy Shevchenko
>
Greg KH Oct. 16, 2023, 3:19 p.m. UTC | #11
On Mon, Oct 16, 2023 at 03:05:09PM +0000, Wu, Wentong wrote:
> > From: Shevchenko, Andriy
> > On Mon, Oct 16, 2023 at 08:52:28AM +0300, Wu, Wentong wrote:
> > > > On 10/13/23 22:05, Shevchenko, Andriy wrote:
> > > > > On Thu, Oct 12, 2023 at 01:14:23PM +0200, Hans de Goede wrote:
> > 
> > <snip>
> > 
> > > > >> Ah ok, I see. So the code:
> > > > >>
> > > > >> 1. First tries to find the matching child acpi_device for the
> > > > >> auxdev by ADR
> > > > >>
> > > > >> 2. If 1. fails then falls back to HID + UID matching
> > > > >>
> > > > >> And there are DSDTs which use either:
> > > > >>
> > > > >> 1. Only use _ADR to identify which child device is which, like the example
> > > > >>    DSDT snippet from the commit msg.
> > > > >>
> > > > >> 2. Only use _HID + _UID like the 2 example DSDT snippets from me
> > > > >> email
> > > > >>
> > > > >> But there never is a case where both _ADR and _HID are used at
> > > > >> the same time (which would be an ACPI spec violation as Andy said).
> > > > >>
> > > > >> So AFAICT there is no issue here since  _ADR and _HID are never
> > > > >> user at the same time and the commit message correctly describes
> > > > >> scenario 1. from above, so the commit message is fine too.
> > > > >>
> > > > >> So I believe that we can continue with this patch series in its
> > > > >> current v20 form, which has already been staged for going into
> > > > >> -next by Greg.
> > > > >>
> > > > >> Andy can you confirm that moving ahead with the current version
> > > > >> is ok ?
> > > > >
> > > > > Yes as we have a few weeks to fix corner cases.
> > > > >
> > > > > What I'm worrying is that opening door for _ADR that seems never
> > > > > used is kinda an overkill here (resolving non-existing problem).
> > > >
> > > > I assume that there actually some DSDTs using the _ADR approach and
> > > > that this support is not there just for fun.
> > >
> > > right, it's not for fun, we use _ADR here is to reduce the maintain
> > > effort because currently it defines _HID for every new platform and
> > > the drivers have to be updated accordingly, while _ADR doesn't have that
> > problem.
> > 
> > But this does not confirm if you have such devices. Moreover, My question
> > about _CID per function stays the same. Why firmware is not using it?
> 
> Yes, both _ADR and _CID can stop growing list in the driver. And for _ADR, it also
> only require one ID per function. I don't know why BIOS team doesn't select _CID,
> but I have suggested use _ADR internally, and , to make things moving forward,
> the driver adds support for _ADR here first. 
> 
> But you're right, _CID is another solution as well, we will discuss it with firmware
> team more.

Should I revert this series now until this gets sorted out?

thanks,

greg k-h
Wu, Wentong Oct. 16, 2023, 3:44 p.m. UTC | #12
> From: gregkh@linuxfoundation.org
> On Mon, Oct 16, 2023 at 03:05:09PM +0000, Wu, Wentong wrote:
> > > From: Shevchenko, Andriy
> > > On Mon, Oct 16, 2023 at 08:52:28AM +0300, Wu, Wentong wrote:
> > > > > On 10/13/23 22:05, Shevchenko, Andriy wrote:
> > > > > > On Thu, Oct 12, 2023 at 01:14:23PM +0200, Hans de Goede wrote:
> > >
> > > <snip>
> > >
> > > > > >> Ah ok, I see. So the code:
> > > > > >>
> > > > > >> 1. First tries to find the matching child acpi_device for the
> > > > > >> auxdev by ADR
> > > > > >>
> > > > > >> 2. If 1. fails then falls back to HID + UID matching
> > > > > >>
> > > > > >> And there are DSDTs which use either:
> > > > > >>
> > > > > >> 1. Only use _ADR to identify which child device is which, like the
> example
> > > > > >>    DSDT snippet from the commit msg.
> > > > > >>
> > > > > >> 2. Only use _HID + _UID like the 2 example DSDT snippets from
> > > > > >> me email
> > > > > >>
> > > > > >> But there never is a case where both _ADR and _HID are used
> > > > > >> at the same time (which would be an ACPI spec violation as Andy said).
> > > > > >>
> > > > > >> So AFAICT there is no issue here since  _ADR and _HID are
> > > > > >> never user at the same time and the commit message correctly
> > > > > >> describes scenario 1. from above, so the commit message is fine too.
> > > > > >>
> > > > > >> So I believe that we can continue with this patch series in
> > > > > >> its current v20 form, which has already been staged for going
> > > > > >> into -next by Greg.
> > > > > >>
> > > > > >> Andy can you confirm that moving ahead with the current
> > > > > >> version is ok ?
> > > > > >
> > > > > > Yes as we have a few weeks to fix corner cases.
> > > > > >
> > > > > > What I'm worrying is that opening door for _ADR that seems
> > > > > > never used is kinda an overkill here (resolving non-existing problem).
> > > > >
> > > > > I assume that there actually some DSDTs using the _ADR approach
> > > > > and that this support is not there just for fun.
> > > >
> > > > right, it's not for fun, we use _ADR here is to reduce the
> > > > maintain effort because currently it defines _HID for every new
> > > > platform and the drivers have to be updated accordingly, while
> > > > _ADR doesn't have that
> > > problem.
> > >
> > > But this does not confirm if you have such devices. Moreover, My
> > > question about _CID per function stays the same. Why firmware is not using
> it?
> >
> > Yes, both _ADR and _CID can stop growing list in the driver. And for
> > _ADR, it also only require one ID per function. I don't know why BIOS
> > team doesn't select _CID, but I have suggested use _ADR internally,
> > and , to make things moving forward, the driver adds support for _ADR here
> first.
> >
> > But you're right, _CID is another solution as well, we will discuss it
> > with firmware team more.
> 
> Should I revert this series now until this gets sorted out?

Current _ADR support is a solution, I don't think _CID is better than _ADR to both
stop growing list in driver and support the shipped hardware at the same time.

Andy, what's your idea? 

BR,
Wentong
> 
> thanks,
> 
> greg k-h
Andy Shevchenko Oct. 16, 2023, 4:05 p.m. UTC | #13
On Mon, Oct 16, 2023 at 06:44:21PM +0300, Wu, Wentong wrote:
> > From: gregkh@linuxfoundation.org
> > On Mon, Oct 16, 2023 at 03:05:09PM +0000, Wu, Wentong wrote:
> > > > From: Shevchenko, Andriy
> > > > On Mon, Oct 16, 2023 at 08:52:28AM +0300, Wu, Wentong wrote:

...

> > > > But this does not confirm if you have such devices. Moreover, My
> > > > question about _CID per function stays the same. Why firmware is not using
> > it?
> > >
> > > Yes, both _ADR and _CID can stop growing list in the driver. And for
> > > _ADR, it also only require one ID per function. I don't know why BIOS
> > > team doesn't select _CID, but I have suggested use _ADR internally,
> > > and , to make things moving forward, the driver adds support for _ADR here
> > first.
> > >
> > > But you're right, _CID is another solution as well, we will discuss it
> > > with firmware team more.
> > 
> > Should I revert this series now until this gets sorted out?
> 
> Current _ADR support is a solution, I don't think _CID is better than _ADR to both
> stop growing list in driver and support the shipped hardware at the same time.
> 
> Andy, what's your idea? 

In my opinion if _CID can be made, it's better than _ADR. As using _ADR like
you do is a bit of grey area in the ACPI specification. I.o.w. can you get
a confirmation, let's say, from Microsoft, that they will go your way for other
similar devices?

Btw, Microsoft has their own solution actually using _ADR for the so called
"wired" USB devices. Is it your case? If so, I'm not sure why _HID has been
used from day 1...

Also I suggest to wait for Hans' opinion on the topic.
Hans de Goede Oct. 16, 2023, 5:20 p.m. UTC | #14
Hi,

On 10/16/23 18:05, Shevchenko, Andriy wrote:
> On Mon, Oct 16, 2023 at 06:44:21PM +0300, Wu, Wentong wrote:
>>> From: gregkh@linuxfoundation.org
>>> On Mon, Oct 16, 2023 at 03:05:09PM +0000, Wu, Wentong wrote:
>>>>> From: Shevchenko, Andriy
>>>>> On Mon, Oct 16, 2023 at 08:52:28AM +0300, Wu, Wentong wrote:
> 
> ...
> 
>>>>> But this does not confirm if you have such devices. Moreover, My
>>>>> question about _CID per function stays the same. Why firmware is not using
>>> it?
>>>>
>>>> Yes, both _ADR and _CID can stop growing list in the driver. And for
>>>> _ADR, it also only require one ID per function. I don't know why BIOS
>>>> team doesn't select _CID, but I have suggested use _ADR internally,
>>>> and , to make things moving forward, the driver adds support for _ADR here
>>> first.
>>>>
>>>> But you're right, _CID is another solution as well, we will discuss it
>>>> with firmware team more.
>>>
>>> Should I revert this series now until this gets sorted out?
>>
>> Current _ADR support is a solution, I don't think _CID is better than _ADR to both
>> stop growing list in driver and support the shipped hardware at the same time.
>>
>> Andy, what's your idea? 
> 
> In my opinion if _CID can be made, it's better than _ADR. As using _ADR like
> you do is a bit of grey area in the ACPI specification. I.o.w. can you get
> a confirmation, let's say, from Microsoft, that they will go your way for other
> similar devices?
> 
> Btw, Microsoft has their own solution actually using _ADR for the so called
> "wired" USB devices. Is it your case? If so, I'm not sure why _HID has been
> used from day 1...
> 
> Also I suggest to wait for Hans' opinion on the topic.

I definitely don't think we should revert the entire series since this
supports actual hw which has already been shipping for years.

But if the _ADR support is only there to support future hw and
it is not even certain yet that that future hw is actually going
to be using _ADR support then I believe that a follow-up patch
to drop _ADR support for now is in order. We can then re-introduce
it (revert the follow up patch) if future hw actually starts
using _ADR support.

Specifically what I'm suggesting is something like the following:

diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
index c9decd0396d4..e1bbaf964786 100644
--- a/drivers/usb/misc/usb-ljca.c
+++ b/drivers/usb/misc/usb-ljca.c
@@ -457,8 +457,8 @@ static void ljca_auxdev_acpi_bind(struct ljca_adapter *adap,
 				  u64 adr, u8 id)
 {
 	struct ljca_match_ids_walk_data wd = { 0 };
-	struct acpi_device *parent, *adev;
 	struct device *dev = adap->dev;
+	struct acpi_device *parent;
 	char uid[4];
 
 	parent = ACPI_COMPANION(dev);
@@ -466,17 +466,7 @@ static void ljca_auxdev_acpi_bind(struct ljca_adapter *adap,
 		return;
 
 	/*
-	 * get auxdev ACPI handle from the ACPI device directly
-	 * under the parent that matches _ADR.
-	 */
-	adev = acpi_find_child_device(parent, adr, false);
-	if (adev) {
-		ACPI_COMPANION_SET(&auxdev->dev, adev);
-		return;
-	}
-
-	/*
-	 * _ADR is a grey area in the ACPI specification, some
+	 * Currently LJCA hw does not use _ADR instead current
 	 * platforms use _HID to distinguish children devices.
 	 */
 	switch (adr) {

As a follow-up patch to the existing series.

Regards,

Hans
Wu, Wentong Oct. 17, 2023, 12:46 a.m. UTC | #15
> From: Hans de Goede <hdegoede>
> On 10/16/23 18:05, Shevchenko, Andriy wrote:
> > On Mon, Oct 16, 2023 at 06:44:21PM +0300, Wu, Wentong wrote:
> >>> From: gregkh@linuxfoundation.org
> >>> On Mon, Oct 16, 2023 at 03:05:09PM +0000, Wu, Wentong wrote:
> >>>>> From: Shevchenko, Andriy
> >>>>> On Mon, Oct 16, 2023 at 08:52:28AM +0300, Wu, Wentong wrote:
> >
> > ...
> >
> >>>>> But this does not confirm if you have such devices. Moreover, My
> >>>>> question about _CID per function stays the same. Why firmware is
> >>>>> not using
> >>> it?
> >>>>
> >>>> Yes, both _ADR and _CID can stop growing list in the driver. And
> >>>> for _ADR, it also only require one ID per function. I don't know
> >>>> why BIOS team doesn't select _CID, but I have suggested use _ADR
> >>>> internally, and , to make things moving forward, the driver adds
> >>>> support for _ADR here
> >>> first.
> >>>>
> >>>> But you're right, _CID is another solution as well, we will discuss
> >>>> it with firmware team more.
> >>>
> >>> Should I revert this series now until this gets sorted out?
> >>
> >> Current _ADR support is a solution, I don't think _CID is better than
> >> _ADR to both stop growing list in driver and support the shipped hardware at
> the same time.
> >>
> >> Andy, what's your idea?
> >
> > In my opinion if _CID can be made, it's better than _ADR. As using
> > _ADR like you do is a bit of grey area in the ACPI specification.
> > I.o.w. can you get a confirmation, let's say, from Microsoft, that
> > they will go your way for other similar devices?
> >
> > Btw, Microsoft has their own solution actually using _ADR for the so
> > called "wired" USB devices. Is it your case? If so, I'm not sure why
> > _HID has been used from day 1...

Thanks for your info, we will discuss more with them, but I also think we
should keep this series and I will do the follow up as Hans' suggest.

> > Also I suggest to wait for Hans' opinion on the topic.
> 
> I definitely don't think we should revert the entire series since this supports
> actual hw which has already been shipping for years.

Totally agree, thanks

> 
> But if the _ADR support is only there to support future hw and it is not even
> certain yet that that future hw is actually going to be using _ADR support then I
> believe that a follow-up patch to drop _ADR support for now is in order. We can
> then re-introduce it (revert the follow up patch) if future hw actually starts using
> _ADR support.

Yes, thanks.

> 
> Specifically what I'm suggesting is something like the following:
> 
> diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c index
> c9decd0396d4..e1bbaf964786 100644
> --- a/drivers/usb/misc/usb-ljca.c
> +++ b/drivers/usb/misc/usb-ljca.c
> @@ -457,8 +457,8 @@ static void ljca_auxdev_acpi_bind(struct ljca_adapter
> *adap,
>  				  u64 adr, u8 id)
>  {
>  	struct ljca_match_ids_walk_data wd = { 0 };
> -	struct acpi_device *parent, *adev;
>  	struct device *dev = adap->dev;
> +	struct acpi_device *parent;
>  	char uid[4];
> 
>  	parent = ACPI_COMPANION(dev);
> @@ -466,17 +466,7 @@ static void ljca_auxdev_acpi_bind(struct ljca_adapter
> *adap,
>  		return;
> 
>  	/*
> -	 * get auxdev ACPI handle from the ACPI device directly
> -	 * under the parent that matches _ADR.
> -	 */
> -	adev = acpi_find_child_device(parent, adr, false);
> -	if (adev) {
> -		ACPI_COMPANION_SET(&auxdev->dev, adev);
> -		return;
> -	}
> -
> -	/*
> -	 * _ADR is a grey area in the ACPI specification, some
> +	 * Currently LJCA hw does not use _ADR instead current
>  	 * platforms use _HID to distinguish children devices.
>  	 */
>  	switch (adr) {
> 
> As a follow-up patch to the existing series.
> 
> Regards,
> 
> Hans
diff mbox series

Patch

diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index 99b15b7..c510af7 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -165,6 +165,19 @@  config APPLE_MFI_FASTCHARGE
 
 	  It is safe to say M here.
 
+config USB_LJCA
+	tristate "Intel La Jolla Cove Adapter support"
+	select AUXILIARY_BUS
+	depends on USB && ACPI
+	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 usb-ljca.
+
 source "drivers/usb/misc/sisusbvga/Kconfig"
 
 config USB_LD
diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
index 1992cc2..0bc732bc 100644
--- a/drivers/usb/misc/Makefile
+++ b/drivers/usb/misc/Makefile
@@ -11,6 +11,7 @@  obj-$(CONFIG_USB_EMI26)			+= emi26.o
 obj-$(CONFIG_USB_EMI62)			+= emi62.o
 obj-$(CONFIG_USB_EZUSB_FX2)		+= ezusb.o
 obj-$(CONFIG_APPLE_MFI_FASTCHARGE)	+= apple-mfi-fastcharge.o
+obj-$(CONFIG_USB_LJCA)			+= usb-ljca.o
 obj-$(CONFIG_USB_IDMOUSE)		+= idmouse.o
 obj-$(CONFIG_USB_IOWARRIOR)		+= iowarrior.o
 obj-$(CONFIG_USB_ISIGHTFW)		+= isight_firmware.o
diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
new file mode 100644
index 0000000..c9decd0
--- /dev/null
+++ b/drivers/usb/misc/usb-ljca.c
@@ -0,0 +1,902 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Intel La Jolla Cove Adapter USB driver
+ *
+ * Copyright (c) 2023, Intel Corporation.
+ */
+
+#include <linux/acpi.h>
+#include <linux/auxiliary_bus.h>
+#include <linux/dev_printk.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+#include <linux/usb.h>
+#include <linux/usb/ljca.h>
+
+#include <asm/unaligned.h>
+
+/* command flags */
+#define LJCA_ACK_FLAG			BIT(0)
+#define LJCA_RESP_FLAG			BIT(1)
+#define LJCA_CMPL_FLAG			BIT(2)
+
+#define LJCA_MAX_PACKET_SIZE		64u
+#define LJCA_MAX_PAYLOAD_SIZE		\
+		(LJCA_MAX_PACKET_SIZE - sizeof(struct ljca_msg))
+
+#define LJCA_WRITE_TIMEOUT_MS		200
+#define LJCA_WRITE_ACK_TIMEOUT_MS	500
+#define LJCA_ENUM_CLIENT_TIMEOUT_MS	20
+
+/* ljca client type */
+enum ljca_client_type {
+	LJCA_CLIENT_MNG = 1,
+	LJCA_CLIENT_GPIO = 3,
+	LJCA_CLIENT_I2C = 4,
+	LJCA_CLIENT_SPI = 5,
+};
+
+/* MNG client commands */
+enum ljca_mng_cmd {
+	LJCA_MNG_RESET = 2,
+	LJCA_MNG_ENUM_GPIO = 4,
+	LJCA_MNG_ENUM_I2C = 5,
+	LJCA_MNG_ENUM_SPI = 8,
+};
+
+/* ljca client acpi _ADR */
+enum ljca_client_acpi_adr {
+	LJCA_GPIO_ACPI_ADR,
+	LJCA_I2C1_ACPI_ADR,
+	LJCA_I2C2_ACPI_ADR,
+	LJCA_SPI1_ACPI_ADR,
+	LJCA_SPI2_ACPI_ADR,
+	LJCA_CLIENT_ACPI_ADR_MAX,
+};
+
+/* ljca cmd message structure */
+struct ljca_msg {
+	u8 type;
+	u8 cmd;
+	u8 flags;
+	u8 len;
+	u8 data[] __counted_by(len);
+} __packed;
+
+struct ljca_i2c_ctr_info {
+	u8 id;
+	u8 capacity;
+	u8 intr_pin;
+} __packed;
+
+struct ljca_i2c_descriptor {
+	u8 num;
+	struct ljca_i2c_ctr_info info[] __counted_by(num);
+} __packed;
+
+struct ljca_spi_ctr_info {
+	u8 id;
+	u8 capacity;
+	u8 intr_pin;
+} __packed;
+
+struct ljca_spi_descriptor {
+	u8 num;
+	struct ljca_spi_ctr_info info[] __counted_by(num);
+} __packed;
+
+struct ljca_bank_descriptor {
+	u8 bank_id;
+	u8 pin_num;
+
+	/* 1 bit for each gpio, 1 means valid */
+	__le32 valid_pins;
+} __packed;
+
+struct ljca_gpio_descriptor {
+	u8 pins_per_bank;
+	u8 bank_num;
+	struct ljca_bank_descriptor bank_desc[] __counted_by(bank_num);
+} __packed;
+
+/**
+ * struct ljca_adapter - represent a ljca adapter
+ *
+ * @intf: the usb interface for this ljca adapter
+ * @usb_dev: the usb device for this ljca adapter
+ * @dev: the specific device info of the usb interface
+ * @rx_pipe: bulk in pipe for receive data from firmware
+ * @tx_pipe: bulk out pipe for send data to firmware
+ * @rx_urb: urb used for the bulk in pipe
+ * @rx_buf: buffer used to receive command response and event
+ * @rx_len: length of rx buffer
+ * @ex_buf: external buffer to save command response
+ * @ex_buf_len: length of external buffer
+ * @actual_length: actual length of data copied to external buffer
+ * @tx_buf: buffer used to download command to firmware
+ * @tx_buf_len: length of tx buffer
+ * @lock: spinlock to protect tx_buf and ex_buf
+ * @cmd_completion: completion object as the command receives ack
+ * @mutex: mutex to avoid command download concurrently
+ * @client_list: client device list
+ * @disconnect: usb disconnect ongoing or not
+ * @reset_id: used to reset firmware
+ */
+struct ljca_adapter {
+	struct usb_interface *intf;
+	struct usb_device *usb_dev;
+	struct device *dev;
+
+	unsigned int rx_pipe;
+	unsigned int tx_pipe;
+
+	struct urb *rx_urb;
+	void *rx_buf;
+	unsigned int rx_len;
+
+	u8 *ex_buf;
+	u8 ex_buf_len;
+	u8 actual_length;
+
+	void *tx_buf;
+	u8 tx_buf_len;
+
+	spinlock_t lock;
+
+	struct completion cmd_completion;
+	struct mutex mutex;
+
+	struct list_head client_list;
+
+	bool disconnect;
+
+	u32 reset_id;
+};
+
+struct ljca_match_ids_walk_data {
+	const struct acpi_device_id *ids;
+	const char *uid;
+	struct acpi_device *adev;
+};
+
+static const struct acpi_device_id ljca_gpio_hids[] = {
+	{ "INTC1074" },
+	{ "INTC1096" },
+	{ "INTC100B" },
+	{ "INTC10D1" },
+	{},
+};
+
+static const struct acpi_device_id ljca_i2c_hids[] = {
+	{ "INTC1075" },
+	{ "INTC1097" },
+	{ "INTC100C" },
+	{ "INTC10D2" },
+	{},
+};
+
+static const struct acpi_device_id ljca_spi_hids[] = {
+	{ "INTC1091" },
+	{ "INTC1098" },
+	{ "INTC100D" },
+	{ "INTC10D3" },
+	{},
+};
+
+static void ljca_handle_event(struct ljca_adapter *adap,
+			      struct ljca_msg *header)
+{
+	struct ljca_client *client;
+
+	list_for_each_entry(client, &adap->client_list, link) {
+		/*
+		 * Currently only GPIO register event callback, but
+		 * firmware message structure should include id when
+		 * multiple same type clients register event callback.
+		 */
+		if (client->type == header->type) {
+			unsigned long flags;
+
+			spin_lock_irqsave(&client->event_cb_lock, flags);
+			client->event_cb(client->context, header->cmd,
+					 header->data, header->len);
+			spin_unlock_irqrestore(&client->event_cb_lock, flags);
+
+			break;
+		}
+	}
+}
+
+/* process command ack and received data if available */
+static void ljca_handle_cmd_ack(struct ljca_adapter *adap, struct ljca_msg *header)
+{
+	struct ljca_msg *tx_header = adap->tx_buf;
+	u8 ibuf_len, actual_len = 0;
+	unsigned long flags;
+	u8 *ibuf;
+
+	spin_lock_irqsave(&adap->lock, flags);
+
+	if (tx_header->type != header->type || tx_header->cmd != header->cmd) {
+		spin_unlock_irqrestore(&adap->lock, flags);
+		dev_err(adap->dev, "cmd ack mismatch error\n");
+		return;
+	}
+
+	ibuf_len = adap->ex_buf_len;
+	ibuf = adap->ex_buf;
+
+	if (ibuf && ibuf_len) {
+		actual_len = min(header->len, ibuf_len);
+
+		/* copy received data to external buffer */
+		memcpy(ibuf, header->data, actual_len);
+	}
+	/* update copied data length */
+	adap->actual_length = actual_len;
+
+	spin_unlock_irqrestore(&adap->lock, flags);
+
+	complete(&adap->cmd_completion);
+}
+
+static void ljca_recv(struct urb *urb)
+{
+	struct ljca_msg *header = urb->transfer_buffer;
+	struct ljca_adapter *adap = urb->context;
+	int ret;
+
+	switch (urb->status) {
+	case 0:
+		/* success */
+		break;
+	case -ENOENT:
+		/*
+		 * directly complete the possible ongoing transfer
+		 * during disconnect
+		 */
+		if (adap->disconnect)
+			complete(&adap->cmd_completion);
+		return;
+	case -ECONNRESET:
+	case -ESHUTDOWN:
+	case -EPIPE:
+		/* rx urb is terminated */
+		dev_dbg(adap->dev, "rx urb terminated with status: %d\n",
+			urb->status);
+		return;
+	default:
+		dev_dbg(adap->dev, "rx urb error: %d\n", urb->status);
+		goto resubmit;
+	}
+
+	if (header->len + sizeof(*header) != urb->actual_length)
+		goto resubmit;
+
+	if (header->flags & LJCA_ACK_FLAG)
+		ljca_handle_cmd_ack(adap, header);
+	else
+		ljca_handle_event(adap, header);
+
+resubmit:
+	ret = usb_submit_urb(urb, GFP_ATOMIC);
+	if (ret && ret != -EPERM)
+		dev_err(adap->dev, "resubmit rx urb error %d\n", ret);
+}
+
+static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
+		     const u8 *obuf, u8 obuf_len, u8 *ibuf, u8 ibuf_len,
+		     bool ack, unsigned long timeout)
+{
+	unsigned int msg_len = sizeof(struct ljca_msg) + obuf_len;
+	struct ljca_msg *header = adap->tx_buf;
+	unsigned int transferred;
+	unsigned long flags;
+	int ret;
+
+	if (adap->disconnect)
+		return -ENODEV;
+
+	if (msg_len > adap->tx_buf_len)
+		return -EINVAL;
+
+	mutex_lock(&adap->mutex);
+
+	spin_lock_irqsave(&adap->lock, flags);
+
+	header->type = type;
+	header->cmd = cmd;
+	header->len = obuf_len;
+	if (obuf)
+		memcpy(header->data, obuf, obuf_len);
+
+	header->flags = LJCA_CMPL_FLAG | (ack ? LJCA_ACK_FLAG : 0);
+
+	adap->ex_buf = ibuf;
+	adap->ex_buf_len = ibuf_len;
+	adap->actual_length = 0;
+
+	spin_unlock_irqrestore(&adap->lock, flags);
+
+	reinit_completion(&adap->cmd_completion);
+
+	ret = usb_autopm_get_interface(adap->intf);
+	if (ret < 0)
+		goto out;
+
+	ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header,
+			   msg_len, &transferred, LJCA_WRITE_TIMEOUT_MS);
+
+	usb_autopm_put_interface(adap->intf);
+
+	if (ret < 0)
+		goto out;
+	if (transferred != msg_len) {
+		ret = -EIO;
+		goto out;
+	}
+
+	if (ack) {
+		ret = wait_for_completion_timeout(&adap->cmd_completion,
+						  timeout);
+		if (!ret) {
+			ret = -ETIMEDOUT;
+			goto out;
+		}
+	}
+	ret = adap->actual_length;
+
+out:
+	spin_lock_irqsave(&adap->lock, flags);
+	adap->ex_buf = NULL;
+	adap->ex_buf_len = 0;
+
+	memset(header, 0, sizeof(*header));
+	spin_unlock_irqrestore(&adap->lock, flags);
+
+	mutex_unlock(&adap->mutex);
+
+	return ret;
+}
+
+int ljca_transfer(struct ljca_client *client, u8 cmd, const u8 *obuf,
+		  u8 obuf_len, u8 *ibuf, u8 ibuf_len)
+{
+	return ljca_send(client->adapter, client->type, cmd,
+			 obuf, obuf_len, ibuf, ibuf_len, true,
+			 LJCA_WRITE_ACK_TIMEOUT_MS);
+}
+EXPORT_SYMBOL_NS_GPL(ljca_transfer, LJCA);
+
+int ljca_transfer_noack(struct ljca_client *client, u8 cmd, const u8 *obuf,
+			u8 obuf_len)
+{
+	return ljca_send(client->adapter, client->type, cmd, obuf,
+			 obuf_len, NULL, 0, false, LJCA_WRITE_ACK_TIMEOUT_MS);
+}
+EXPORT_SYMBOL_NS_GPL(ljca_transfer_noack, LJCA);
+
+int ljca_register_event_cb(struct ljca_client *client, ljca_event_cb_t event_cb,
+			   void *context)
+{
+	unsigned long flags;
+
+	if (!event_cb)
+		return -EINVAL;
+
+	spin_lock_irqsave(&client->event_cb_lock, flags);
+
+	if (client->event_cb) {
+		spin_unlock_irqrestore(&client->event_cb_lock, flags);
+		return -EALREADY;
+	}
+
+	client->event_cb = event_cb;
+	client->context = context;
+
+	spin_unlock_irqrestore(&client->event_cb_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(ljca_register_event_cb, LJCA);
+
+void ljca_unregister_event_cb(struct ljca_client *client)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&client->event_cb_lock, flags);
+
+	client->event_cb = NULL;
+	client->context = NULL;
+
+	spin_unlock_irqrestore(&client->event_cb_lock, flags);
+}
+EXPORT_SYMBOL_NS_GPL(ljca_unregister_event_cb, LJCA);
+
+static int ljca_match_device_ids(struct acpi_device *adev, void *data)
+{
+	struct ljca_match_ids_walk_data *wd = data;
+	const char *uid = acpi_device_uid(adev);
+
+	if (acpi_match_device_ids(adev, wd->ids))
+		return 0;
+
+	if (!wd->uid)
+		goto match;
+
+	if (!uid)
+		/*
+		 * Some DSDTs have only one ACPI companion for the two I2C
+		 * controllers and they don't set a UID at all (e.g. Dell
+		 * Latitude 9420). On these platforms only the first I2C
+		 * controller is used, so if a HID match has no UID we use
+		 * "0" as the UID and assign ACPI companion to the first
+		 * I2C controller.
+		 */
+		uid = "0";
+	else
+		uid = strchr(uid, wd->uid[0]);
+
+	if (!uid || strcmp(uid, wd->uid))
+		return 0;
+
+match:
+	wd->adev = adev;
+
+	return 1;
+}
+
+/* bind auxiliary device to acpi device */
+static void ljca_auxdev_acpi_bind(struct ljca_adapter *adap,
+				  struct auxiliary_device *auxdev,
+				  u64 adr, u8 id)
+{
+	struct ljca_match_ids_walk_data wd = { 0 };
+	struct acpi_device *parent, *adev;
+	struct device *dev = adap->dev;
+	char uid[4];
+
+	parent = ACPI_COMPANION(dev);
+	if (!parent)
+		return;
+
+	/*
+	 * get auxdev ACPI handle from the ACPI device directly
+	 * under the parent that matches _ADR.
+	 */
+	adev = acpi_find_child_device(parent, adr, false);
+	if (adev) {
+		ACPI_COMPANION_SET(&auxdev->dev, adev);
+		return;
+	}
+
+	/*
+	 * _ADR is a grey area in the ACPI specification, some
+	 * platforms use _HID to distinguish children devices.
+	 */
+	switch (adr) {
+	case LJCA_GPIO_ACPI_ADR:
+		wd.ids = ljca_gpio_hids;
+		break;
+	case LJCA_I2C1_ACPI_ADR:
+	case LJCA_I2C2_ACPI_ADR:
+		snprintf(uid, sizeof(uid), "%d", id);
+		wd.uid = uid;
+		wd.ids = ljca_i2c_hids;
+		break;
+	case LJCA_SPI1_ACPI_ADR:
+	case LJCA_SPI2_ACPI_ADR:
+		wd.ids = ljca_spi_hids;
+		break;
+	default:
+		dev_warn(dev, "unsupported _ADR\n");
+		return;
+	}
+
+	acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd);
+	if (wd.adev) {
+		ACPI_COMPANION_SET(&auxdev->dev, wd.adev);
+		return;
+	}
+
+	parent = ACPI_COMPANION(dev->parent->parent);
+	if (!parent)
+		return;
+
+	acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd);
+	if (wd.adev)
+		ACPI_COMPANION_SET(&auxdev->dev, wd.adev);
+}
+
+static void ljca_auxdev_release(struct device *dev)
+{
+	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
+
+	kfree(auxdev->dev.platform_data);
+}
+
+static int ljca_new_client_device(struct ljca_adapter *adap, u8 type, u8 id,
+				  char *name, void *data, u64 adr)
+{
+	struct auxiliary_device *auxdev;
+	struct ljca_client *client;
+	int ret;
+
+	client = kzalloc(sizeof *client, GFP_KERNEL);
+	if (!client)
+		return -ENOMEM;
+
+	client->type = type;
+	client->id = id;
+	client->adapter = adap;
+	spin_lock_init(&client->event_cb_lock);
+
+	auxdev = &client->auxdev;
+	auxdev->name = name;
+	auxdev->id = id;
+
+	auxdev->dev.parent = adap->dev;
+	auxdev->dev.platform_data = data;
+	auxdev->dev.release = ljca_auxdev_release;
+
+	ret = auxiliary_device_init(auxdev);
+	if (ret)
+		goto err_free;
+
+	ljca_auxdev_acpi_bind(adap, auxdev, adr, id);
+
+	ret = auxiliary_device_add(auxdev);
+	if (ret)
+		goto err_uninit;
+
+	list_add_tail(&client->link, &adap->client_list);
+
+	return 0;
+
+err_uninit:
+	auxiliary_device_uninit(auxdev);
+
+err_free:
+	kfree(client);
+
+	return ret;
+}
+
+static int ljca_enumerate_gpio(struct ljca_adapter *adap)
+{
+	u32 valid_pin[LJCA_MAX_GPIO_NUM / BITS_PER_TYPE(u32)];
+	struct ljca_gpio_descriptor *desc;
+	struct ljca_gpio_info *gpio_info;
+	u8 buf[LJCA_MAX_PAYLOAD_SIZE];
+	int ret, gpio_num;
+	unsigned int i;
+
+	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_GPIO, NULL, 0, buf,
+			sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
+	if (ret < 0)
+		return ret;
+
+	/* check firmware response */
+	desc = (struct ljca_gpio_descriptor *)buf;
+	if (ret != struct_size(desc, bank_desc, desc->bank_num))
+		return -EINVAL;
+
+	gpio_num = desc->pins_per_bank * desc->bank_num;
+	if (gpio_num > LJCA_MAX_GPIO_NUM)
+		return -EINVAL;
+
+	/* construct platform data */
+	gpio_info = kzalloc(sizeof *gpio_info, GFP_KERNEL);
+	if (!gpio_info)
+		return -ENOMEM;
+	gpio_info->num = gpio_num;
+
+	for (i = 0; i < desc->bank_num; i++)
+		valid_pin[i] = get_unaligned_le32(&desc->bank_desc[i].valid_pins);
+	bitmap_from_arr32(gpio_info->valid_pin_map, valid_pin, gpio_num);
+
+	ret = ljca_new_client_device(adap, LJCA_CLIENT_GPIO, 0, "ljca-gpio",
+				     gpio_info, LJCA_GPIO_ACPI_ADR);
+	if (ret)
+		kfree(gpio_info);
+
+	return ret;
+}
+
+static int ljca_enumerate_i2c(struct ljca_adapter *adap)
+{
+	struct ljca_i2c_descriptor *desc;
+	struct ljca_i2c_info *i2c_info;
+	u8 buf[LJCA_MAX_PAYLOAD_SIZE];
+	unsigned int i;
+	int ret;
+
+	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_I2C, NULL, 0, buf,
+			sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
+	if (ret < 0)
+		return ret;
+
+	/* check firmware response */
+	desc = (struct ljca_i2c_descriptor *)buf;
+	if (ret != struct_size(desc, info, desc->num))
+		return -EINVAL;
+
+	for (i = 0; i < desc->num; i++) {
+		/* construct platform data */
+		i2c_info = kzalloc(sizeof *i2c_info, GFP_KERNEL);
+		if (!i2c_info)
+			return -ENOMEM;
+
+		i2c_info->id = desc->info[i].id;
+		i2c_info->capacity = desc->info[i].capacity;
+		i2c_info->intr_pin = desc->info[i].intr_pin;
+
+		ret = ljca_new_client_device(adap, LJCA_CLIENT_I2C, i,
+					     "ljca-i2c", i2c_info,
+					     LJCA_I2C1_ACPI_ADR + i);
+		if (ret) {
+			kfree(i2c_info);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int ljca_enumerate_spi(struct ljca_adapter *adap)
+{
+	struct ljca_spi_descriptor *desc;
+	struct ljca_spi_info *spi_info;
+	u8 buf[LJCA_MAX_PAYLOAD_SIZE];
+	unsigned int i;
+	int ret;
+
+	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_SPI, NULL, 0, buf,
+			sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
+	if (ret < 0)
+		return ret;
+
+	/* check firmware response */
+	desc = (struct ljca_spi_descriptor *)buf;
+	if (ret != struct_size(desc, info, desc->num))
+		return -EINVAL;
+
+	for (i = 0; i < desc->num; i++) {
+		/* construct platform data */
+		spi_info = kzalloc(sizeof *spi_info, GFP_KERNEL);
+		if (!spi_info)
+			return -ENOMEM;
+
+		spi_info->id = desc->info[i].id;
+		spi_info->capacity = desc->info[i].capacity;
+
+		ret = ljca_new_client_device(adap, LJCA_CLIENT_SPI, i,
+					     "ljca-spi", spi_info,
+					     LJCA_SPI1_ACPI_ADR + i);
+		if (ret) {
+			kfree(spi_info);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int ljca_reset_handshake(struct ljca_adapter *adap)
+{
+	__le32 reset_id = cpu_to_le32(adap->reset_id);
+	__le32 reset_id_ret = 0;
+	int ret;
+
+	adap->reset_id++;
+
+	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_RESET, (u8 *)&reset_id,
+			sizeof(__le32), (u8 *)&reset_id_ret, sizeof(__le32),
+			true, LJCA_WRITE_ACK_TIMEOUT_MS);
+	if (ret < 0)
+		return ret;
+
+	if (reset_id_ret != reset_id)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int ljca_enumerate_clients(struct ljca_adapter *adap)
+{
+	struct ljca_client *client, *next;
+	int ret;
+
+	ret = ljca_reset_handshake(adap);
+	if (ret)
+		goto err_kill;
+
+	ret = ljca_enumerate_gpio(adap);
+	if (ret) {
+		dev_err(adap->dev, "enumerate GPIO error\n");
+		goto err_kill;
+	}
+
+	ret = ljca_enumerate_i2c(adap);
+	if (ret) {
+		dev_err(adap->dev, "enumerate I2C error\n");
+		goto err_kill;
+	}
+
+	ret = ljca_enumerate_spi(adap);
+	if (ret) {
+		dev_err(adap->dev, "enumerate SPI error\n");
+		goto err_kill;
+	}
+
+	return 0;
+
+err_kill:
+	adap->disconnect = true;
+
+	usb_kill_urb(adap->rx_urb);
+
+	list_for_each_entry_safe_reverse(client, next, &adap->client_list, link) {
+		auxiliary_device_delete(&client->auxdev);
+		auxiliary_device_uninit(&client->auxdev);
+
+		list_del_init(&client->link);
+		kfree(client);
+	}
+
+	return ret;
+}
+
+static int ljca_probe(struct usb_interface *interface,
+		      const struct usb_device_id *id)
+{
+	struct usb_device *usb_dev = interface_to_usbdev(interface);
+	struct usb_host_interface *alt = interface->cur_altsetting;
+	struct usb_endpoint_descriptor *ep_in, *ep_out;
+	struct device *dev = &interface->dev;
+	struct ljca_adapter *adap;
+	int ret;
+
+	adap = devm_kzalloc(dev, sizeof(*adap), GFP_KERNEL);
+	if (!adap)
+		return -ENOMEM;
+
+	/* separate tx buffer allocation for alignment */
+	adap->tx_buf = devm_kzalloc(dev, LJCA_MAX_PACKET_SIZE, GFP_KERNEL);
+	if (!adap->tx_buf)
+		return -ENOMEM;
+	adap->tx_buf_len = LJCA_MAX_PACKET_SIZE;
+
+	mutex_init(&adap->mutex);
+	spin_lock_init(&adap->lock);
+	init_completion(&adap->cmd_completion);
+	INIT_LIST_HEAD(&adap->client_list);
+
+	adap->intf = usb_get_intf(interface);
+	adap->usb_dev = usb_dev;
+	adap->dev = dev;
+
+	/*
+	 * find the first bulk in and out endpoints.
+	 * ignore any others.
+	 */
+	ret = usb_find_common_endpoints(alt, &ep_in, &ep_out, NULL, NULL);
+	if (ret) {
+		dev_err(dev, "bulk endpoints not found\n");
+		goto err_put;
+	}
+	adap->rx_pipe = usb_rcvbulkpipe(usb_dev, usb_endpoint_num(ep_in));
+	adap->tx_pipe = usb_sndbulkpipe(usb_dev, usb_endpoint_num(ep_out));
+
+	/* setup rx buffer */
+	adap->rx_len = usb_endpoint_maxp(ep_in);
+	adap->rx_buf = devm_kzalloc(dev, adap->rx_len, GFP_KERNEL);
+	if (!adap->rx_buf) {
+		ret = -ENOMEM;
+		goto err_put;
+	}
+
+	/* alloc rx urb */
+	adap->rx_urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!adap->rx_urb) {
+		ret = -ENOMEM;
+		goto err_put;
+	}
+	usb_fill_bulk_urb(adap->rx_urb, usb_dev, adap->rx_pipe,
+			  adap->rx_buf, adap->rx_len, ljca_recv, adap);
+
+	usb_set_intfdata(interface, adap);
+
+	/* submit rx urb before enumerate clients */
+	ret = usb_submit_urb(adap->rx_urb, GFP_KERNEL);
+	if (ret) {
+		dev_err(dev, "submit rx urb failed: %d\n", ret);
+		goto err_free;
+	}
+
+	ret = ljca_enumerate_clients(adap);
+	if (ret)
+		goto err_free;
+
+	usb_enable_autosuspend(usb_dev);
+
+	return 0;
+
+err_free:
+	usb_free_urb(adap->rx_urb);
+
+err_put:
+	usb_put_intf(adap->intf);
+
+	mutex_destroy(&adap->mutex);
+
+	return ret;
+}
+
+static void ljca_disconnect(struct usb_interface *interface)
+{
+	struct ljca_adapter *adap = usb_get_intfdata(interface);
+	struct ljca_client *client, *next;
+
+	adap->disconnect = true;
+
+	usb_kill_urb(adap->rx_urb);
+
+	list_for_each_entry_safe_reverse(client, next, &adap->client_list, link) {
+		auxiliary_device_delete(&client->auxdev);
+		auxiliary_device_uninit(&client->auxdev);
+
+		list_del_init(&client->link);
+		kfree(client);
+	}
+
+	usb_free_urb(adap->rx_urb);
+
+	usb_put_intf(adap->intf);
+
+	mutex_destroy(&adap->mutex);
+}
+
+static int ljca_suspend(struct usb_interface *interface, pm_message_t message)
+{
+	struct ljca_adapter *adap = usb_get_intfdata(interface);
+
+	usb_kill_urb(adap->rx_urb);
+
+	return 0;
+}
+
+static int ljca_resume(struct usb_interface *interface)
+{
+	struct ljca_adapter *adap = usb_get_intfdata(interface);
+
+	return usb_submit_urb(adap->rx_urb, GFP_KERNEL);
+}
+
+static const struct usb_device_id ljca_table[] = {
+	{ USB_DEVICE(0x8086, 0x0b63) },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(usb, ljca_table);
+
+static struct usb_driver ljca_driver = {
+	.name = "ljca",
+	.id_table = ljca_table,
+	.probe = ljca_probe,
+	.disconnect = ljca_disconnect,
+	.suspend = ljca_suspend,
+	.resume = ljca_resume,
+	.supports_autosuspend = 1,
+};
+module_usb_driver(ljca_driver);
+
+MODULE_AUTHOR("Wentong Wu <wentong.wu@intel.com>");
+MODULE_AUTHOR("Zhifeng Wang <zhifeng.wang@intel.com>");
+MODULE_AUTHOR("Lixu Zhang <lixu.zhang@intel.com>");
+MODULE_DESCRIPTION("Intel La Jolla Cove Adapter USB driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/usb/ljca.h b/include/linux/usb/ljca.h
new file mode 100644
index 0000000..47661fe
--- /dev/null
+++ b/include/linux/usb/ljca.h
@@ -0,0 +1,145 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023, Intel Corporation. All rights reserved.
+ */
+#ifndef _LINUX_USB_LJCA_H_
+#define _LINUX_USB_LJCA_H_
+
+#include <linux/auxiliary_bus.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+#define LJCA_MAX_GPIO_NUM 64
+
+#define auxiliary_dev_to_ljca_client(auxiliary_dev)			\
+		container_of(auxiliary_dev, struct ljca_client, auxdev)
+
+struct ljca_adapter;
+
+/**
+ * typedef ljca_event_cb_t - event callback function signature
+ *
+ * @context: the execution context of who registered this callback
+ * @cmd: the command from device for this event
+ * @evt_data: the event data payload
+ * @len: the event data payload length
+ *
+ * The callback function is called in interrupt context and the data payload is
+ * only valid during the call. If the user needs later access of the data, it
+ * must copy it.
+ */
+typedef void (*ljca_event_cb_t)(void *context, u8 cmd, const void *evt_data, int len);
+
+/**
+ * struct ljca_client - represent a ljca client device
+ *
+ * @type: ljca client type
+ * @id: ljca client id within same client type
+ * @link: ljca client on the same ljca adapter
+ * @auxdev: auxiliary device object
+ * @adapter: ljca adapter the ljca client sit on
+ * @context: the execution context of the event callback
+ * @event_cb: ljca client driver register this callback to get
+ *	firmware asynchronous rx buffer pending notifications
+ * @event_cb_lock: spinlock to protect event callback
+ */
+struct ljca_client {
+	u8 type;
+	u8 id;
+	struct list_head link;
+	struct auxiliary_device auxdev;
+	struct ljca_adapter *adapter;
+
+	void *context;
+	ljca_event_cb_t event_cb;
+	/* lock to protect event_cb */
+	spinlock_t event_cb_lock;
+};
+
+/**
+ * struct ljca_gpio_info - ljca gpio client device info
+ *
+ * @num: ljca gpio client device pin number
+ * @valid_pin_map: ljca gpio client device valid pin mapping
+ */
+struct ljca_gpio_info {
+	unsigned int num;
+	DECLARE_BITMAP(valid_pin_map, LJCA_MAX_GPIO_NUM);
+};
+
+/**
+ * struct ljca_i2c_info - ljca i2c client device info
+ *
+ * @id: ljca i2c client device identification number
+ * @capacity: ljca i2c client device capacity
+ * @intr_pin: ljca i2c client device interrupt pin number if exists
+ */
+struct ljca_i2c_info {
+	u8 id;
+	u8 capacity;
+	u8 intr_pin;
+};
+
+/**
+ * struct ljca_spi_info - ljca spi client device info
+ *
+ * @id: ljca spi client device identification number
+ * @capacity: ljca spi client device capacity
+ */
+struct ljca_spi_info {
+	u8 id;
+	u8 capacity;
+};
+
+/**
+ * ljca_register_event_cb - register a callback function to receive events
+ *
+ * @client: ljca client device
+ * @event_cb: callback function
+ * @context: execution context of event callback
+ *
+ * Return: 0 in case of success, negative value in case of error
+ */
+int ljca_register_event_cb(struct ljca_client *client, ljca_event_cb_t event_cb, void *context);
+
+/**
+ * ljca_unregister_event_cb - unregister the callback function for an event
+ *
+ * @client: ljca client device
+ */
+void ljca_unregister_event_cb(struct ljca_client *client);
+
+/**
+ * ljca_transfer - issue a LJCA command and wait for a response
+ *
+ * @client: ljca client device
+ * @cmd: the command to be sent to the device
+ * @obuf: the buffer to be sent to the device; it can be NULL if the user
+ *	doesn't need to transmit data with this command
+ * @obuf_len: the size of the buffer to be sent to the device; it should
+ *	be 0 when obuf is NULL
+ * @ibuf: any data associated with the response will be copied here; it can be
+ *	NULL if the user doesn't need the response data
+ * @ibuf_len: must be initialized to the input buffer size
+ *
+ * Return: the actual length of response data for success, negative value for errors
+ */
+int ljca_transfer(struct ljca_client *client, u8 cmd, const u8 *obuf,
+		  u8 obuf_len, u8 *ibuf, u8 ibuf_len);
+
+/**
+ * ljca_transfer_noack - issue a LJCA command without a response
+ *
+ * @client: ljca client device
+ * @cmd: the command to be sent to the device
+ * @obuf: the buffer to be sent to the device; it can be NULL if the user
+ *	doesn't need to transmit data with this command
+ * @obuf_len: the size of the buffer to be sent to the device
+ *
+ * Return: 0 for success, negative value for errors
+ */
+int ljca_transfer_noack(struct ljca_client *client, u8 cmd, const u8 *obuf,
+			u8 obuf_len);
+
+#endif