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

[Shevchenko, Andriy]

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.

-- 
With Best Regards,
Andy Shevchenko