Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <6a87b43a-0648-28d4-6c69-e0f684e44eb6@redhat.com>
Date:   Wed, 11 Oct 2023 12:37:51 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v20 1/4] usb: Add support for Intel LJCA device
Content-Language: en-US, nl
To:     Andy Shevchenko <andriy.shevchenko@intel.com>,
        Wentong Wu <wentong.wu@intel.com>
Cc:     gregkh@linuxfoundation.org, oneukum@suse.com, wsa@kernel.org,
        andi.shyti@linux.intel.com, broonie@kernel.org,
        bartosz.golaszewski@linaro.org, linus.walleij@linaro.org,
        linux-usb@vger.kernel.org, linux-i2c@vger.kernel.org,
        linux-spi@vger.kernel.org, sakari.ailus@linux.intel.com,
        zhifeng.wang@intel.com, linux-gpio@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <1696833205-16716-1-git-send-email-wentong.wu@intel.com>
 <1696833205-16716-2-git-send-email-wentong.wu@intel.com>
 <ZSZ3IPgLk7uC5UGI@smile.fi.intel.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <ZSZ3IPgLk7uC5UGI@smile.fi.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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