> 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
