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

["Wu, Wentong" <wentong.wu@xxxxxxxxx>]

> 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