| Message ID | 1696833205-16716-1-git-send-email-wentong.wu@intel.com |
|---|---|
| Headers | show |
| Series | Add Intel LJCA device driver | expand |
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.
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.
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.
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
> 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. > 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. > > 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. > > 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. Yes, total agree. Thanks Thanks Wentong > > Regards, > > Hans >
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
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
> 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 >
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
> 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
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.
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
> 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