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

[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? 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

-- 
With Best Regards,
Andy Shevchenko