Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
From:   "Wu, Wentong" <wentong.wu@intel.com>
To:     "Shevchenko, Andriy" <andriy.shevchenko@intel.com>
CC:     Hans de Goede <hdegoede@redhat.com>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "oneukum@suse.com" <oneukum@suse.com>,
        "wsa@kernel.org" <wsa@kernel.org>,
        "andi.shyti@linux.intel.com" <andi.shyti@linux.intel.com>,
        "broonie@kernel.org" <broonie@kernel.org>,
        "bartosz.golaszewski@linaro.org" <bartosz.golaszewski@linaro.org>,
        "linus.walleij@linaro.org" <linus.walleij@linaro.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
        "linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>,
        "sakari.ailus@linux.intel.com" <sakari.ailus@linux.intel.com>,
        "Wang, Zhifeng" <zhifeng.wang@intel.com>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v20 1/4] usb: Add support for Intel LJCA device
Thread-Topic: [PATCH v20 1/4] usb: Add support for Intel LJCA device
Thread-Index: AQHZ+nqZYOWB2QQzm0agSGlxpBPNmbBEZGUAgAAEnYCAAByScIABf/iAgAImwQCAAPlcAIACx//wgAAk8gCAAHc9cA==
Date:   Mon, 16 Oct 2023 15:05:09 +0000
Message-ID: <DM6PR11MB4316382324D15985A70E531C8DD7A@DM6PR11MB4316.namprd11.prod.outlook.com>
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>
 <6a87b43a-0648-28d4-6c69-e0f684e44eb6@redhat.com>
 <DM6PR11MB4316BE44F53E276384FF06C88DCCA@DM6PR11MB4316.namprd11.prod.outlook.com>
 <5d2e9eba-a941-ea9a-161a-5b97d09d5d35@redhat.com>
 <ZSmjEKfYzFuAHXW+@smile.fi.intel.com>
 <9a080d06-586d-686f-997e-674cb8d16099@redhat.com>
 <DM6PR11MB43169A9ADDA7681DB7D9347C8DD7A@DM6PR11MB4316.namprd11.prod.outlook.com>
 <ZSzogNhlX9njvOIU@smile.fi.intel.com>
In-Reply-To: <ZSzogNhlX9njvOIU@smile.fi.intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ZcyLCdNxqYmPyJQUhIa8+K4WLFS9R5agEnkdQaWCKisbdeWn781oBClTMl2t?=
 =?us-ascii?Q?M0SOP/N4K/KdLYrLchpjVIptyjfZo+qp09O679oHUpfq0WaBxVSuzuBX9bBz?=
 =?us-ascii?Q?pGjKrexrbW/tAzc5oezlyutT/VS4pJS1RxN/PLH13Xnq0PQYXYYMWHpAg+xU?=
 =?us-ascii?Q?Pmn+vM2p+AZ5Vnfb/XQ4G4nLILdkRANeGheZA8Yyr7RgkokpHcEQ3qGFVsg0?=
 =?us-ascii?Q?c1Q2Vq4cHF8CCgK1PRlqgkV8rRHlYpjnJNAqa8NDi6sKUIxBEr+BNUSBdtGU?=
 =?us-ascii?Q?kzhK692Ot1EYPqN9uj43456ovI2JK9j3qf8+REY95rWspgrLJ3s7mgld2rVR?=
 =?us-ascii?Q?vnmQL54ApUqF538E1EI20i2R6bzJ8qPc3NypG84G8wKIhuSPAs2/P2f5uo8k?=
 =?us-ascii?Q?OqjiXWGc1o9MYbckjRmRiyWe7RvvJqqu6Blo4R8schaX+VCYfMAM0Zd0Uz7E?=
 =?us-ascii?Q?srazghTMcK4fSSLnRzOkcfe+b4V+GWdQv/pKkr18gAeWhaATmiu9CZ20/1H+?=
 =?us-ascii?Q?70LDag/FuvDr3+u9TMd3QUTaLeUrFwgPJrIqKqdvs061GD41dL0OQDyalzeb?=
 =?us-ascii?Q?uDN5V3ZFHJqaZ7NfKxJJb6d9lxqqjmKXVVY5lbSmLSnNQiDtJiYE5WBCDGi2?=
 =?us-ascii?Q?ft9zHxmhR3I7Ab54rbzQqnJfzlytNrttRWfB0zLZhWd0iisySuSg/BG9scSr?=
 =?us-ascii?Q?fgTusit8P9reLSzNBBO9fIxXM5XfKl8tea+3KjjQ5AdU3Mn7q4xlDa+Klqmn?=
 =?us-ascii?Q?sme/VKLY8bmmQcNOiEtUUz58jlrqWqWG0dTcqOI0N1+ZoV0vm+lBzEgSE3u8?=
 =?us-ascii?Q?goFuMrV3Y5D8Frr+mRb2Qo/9YlECsylGtpn2dz1kOqOoImG8t6SOUiyaCfjC?=
 =?us-ascii?Q?NzMd6/ph67pA5gWH8QFkOQKnBWXCH4XSfTsCf7F9Rz7ddr4vOz2fCKOiUWkt?=
 =?us-ascii?Q?Ay/osdgnK23ELcIOF6tlFas0ldHm7G4zWdbVrEjZFgdAkU/sUuioGG3kjzDG?=
 =?us-ascii?Q?4HfXoqDU10knHXtGzIJMaaphGK7DclEQ96SVMkhpqnsr4WH81Si4tR3KQTi8?=
 =?us-ascii?Q?pZreQ/J5oWeCrahplKdPLNpJUz3k41PsYtYDEm2kktNqaErTVXc02T3BjBCu?=
 =?us-ascii?Q?MLIMbDtNylfqgZOVEM2IHsJPu+UQlzSKLv/I+c7MZrBCsh4EIHcHeuC5gCyS?=
 =?us-ascii?Q?R4j8HTQ0XjgymnobNzuVZgEwiAoQw9v7fwlKDL8k7Ujp/McrCm8Se0tbVD/U?=
 =?us-ascii?Q?INuCYiGmEoydxiwQmTAiluSEXWda+tW9NGGzuykw12Fj9RElqDkeNX+Zd0lt?=
 =?us-ascii?Q?s7DeMs9f9j5EH3/CyFGNwm/+R3bOCLSBue4nPSfh9lByOZvcT8D7M5uX5tyI?=
 =?us-ascii?Q?e/qFONqMN62AKC/5g5OtRBrEIjeoqslCvrnFniEpeDr2oE0vDc9wX93BxIRN?=
 =?us-ascii?Q?YukhK/E8Ol2LOmSUdVJNy7vRygdLkr1rxghP4U/5bWE/IZYjqphs3Q8Oa2El?=
 =?us-ascii?Q?5r4AGS3DrYtiimLJte6mPpllAsVRqRMiCiejOouKCCJQsfVX0OWSTCWuIoAY?=
 =?us-ascii?Q?sb2yrG9aNu/VeVsKOACxjyAPfY9d9YJZG7Q731Ur?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB4316.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 87ccc3ab-c830-45c2-d32b-08dbce594a3d
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Oct 2023 15:05:09.6000
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: ojsVHtlK9qEjx8yq9E0NIOYKTO20ZD4oDqMWPNK0z7G/iJBvJtZM4ugChEkFPPj/jEkPGV4MygDwtbajL9fQGQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB8702
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Mon, 16 Oct 2023 08:05:58 -0700 (PDT)

> 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:
>=20
> <snip>
>=20
> > > >> 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 tha=
t
> problem.
>=20
> 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 forwa=
rd,
the driver adds support for _ADR here first.=20

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 re=
quire some
> IDs in the _HID, I don't remember that part of the spec by heart, i.e.  i=
f _CID can be
> only provided with existing _HID or not).
>=20
> > > 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.
>=20
> This is good idea!
>=20
>=20
> > > > 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 cha=
nge
> windows driver as well.
>=20
> I understand, but at least you may stop growing list in the driver.
Yes,=20

> And actually using separate IDs for multifunctional device seems not idea=
l
> solution to me.
Agree, I will consider _CID more, but currently to avoid this and also supp=
ort
shipped hardware, _ADR is at least a choice.

BR,
Wentong

> > > This is a USB device which has 4 functions:
>=20
> 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.
>=20
> > > 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 fun=
ction.
> > >
> > > 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
>=20
> --
> With Best Regards,
> Andy Shevchenko
>=20