Re: [RFC PATCH v3 9/9] ipu3-cio2: Add functionality allowing software_node connections to sensors on platforms designed for Windows

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Dan,

On Fri, Nov 13, 2020 at 10:02:30AM +0000, Dan Scally wrote:
> On 29/10/2020 22:51, Laurent Pinchart wrote:
> > On Fri, Oct 30, 2020 at 12:22:15AM +0200, Andy Shevchenko wrote:
> >> On Thu, Oct 29, 2020 at 11:29:30PM +0200, Laurent Pinchart wrote:
> >>> On Thu, Oct 29, 2020 at 10:26:56PM +0200, Andy Shevchenko wrote:
> >>>> On Thu, Oct 29, 2020 at 10:21 PM Laurent Pinchart wrote:
> >>>>> On Mon, Oct 26, 2020 at 06:10:50PM +0200, Andy Shevchenko wrote:
> >>>>>> On Sat, Oct 24, 2020 at 12:37:02PM +0300, Laurent Pinchart wrote:
> >>>>>>> On Sat, Oct 24, 2020 at 09:50:07AM +0100, Dan Scally wrote:
> >>>>>>>> On 24/10/2020 02:24, Laurent Pinchart wrote:
> >>>>>>>>> On Mon, Oct 19, 2020 at 11:59:03PM +0100, Daniel Scally wrote:
> >>>>>>>>>> +              adev = acpi_dev_get_first_match_dev(supported_devices[i], NULL, -1);
> >>>>>>>>> What if there are multiple sensor of the same model ?
> >>>>>>>> Hmm, yeah, that would be a bit of a pickle. I guess the newer
> >>>>>>>> smartphones have multiple sensors on the back, which I presume are the
> >>>>>>>> same model. So that will probably crop up at some point. How about
> >>>>>>>> instead I use bus_for_each_dev() and in the applied function check if
> >>>>>>>> the _HID is in the supported list?
> >>>>>>> Sounds good to me.
> >>>>>>>
> >>>>>>>>>> +              if (!adev)
> >>>>>>>>>> +                      continue;
> >>>>>> Please, don't.
> >>>>>>
> >>>>>> If we have so weird ACPI tables it must be w/a differently. The all, even badly
> >>>>>> formed, ACPI tables I have seen so far are using _UID to distinguish instance
> >>>>>> of the device (see second parameter to the above function).
> >>>>>>
> >>>>>> If we meet the very broken table I would like rather to know about, then
> >>>>>> silently think ahead what could be best.
> >>>>>>
> >>>>>> I.o.w. don't change this until we will have a real example of the problematic
> >>>>>> firmware.
> >>>>> I'm not sure to follow you. Daniel's current code loops over all the
> >>>>> supported HID (as stored in the supported_devices table), and then gets
> >>>>> the first ACPI device for each of them. If multiple ACPI devices exist
> >>>>> with the same HID, we need to handle them all, so enumerating all ACPI
> >>>>> devices and checking whether their HID is one we handle seems to be the
> >>>>> right option to me.
> >>>> Devices with the same HID should be still different by another
> >>>> parameter in ACPI. The above mentioned call just uses the rough
> >>>> estimation for relaxed conditions. If you expect more than one device
> >>>> with the same HID how do you expect to distinguish them? The correct
> >>>> way is to use _UID. It may be absent, or set to a value. And this
> >>>> value should be unique (as per U letter in UID abbreviation). That
> >>>> said, the above is good enough till we find the firmware with the
> >>>> above true (several devices with the same HID). Until then the code is
> >>>> fine.
> >>> I expect those devices with the same _HID to have different _UID values,
> >>> yes. On the systems I've seen so far, that assumption is not violated,
> >>> and I don't think we need to already plan how we will support systems
> >>> where multiple devices would have the same _HID and _UID (within the
> >>> same scope). There's no disagreement there.
> >>>
> >>> My point is that supported_devices stores HID values, and doesn't care
> >>> about UID. The code loops over supported_devices, and for each entry,
> >>> calls acpi_dev_get_first_match_dev() and process the ACPI devices
> >>> returned by that call. We thus process at most one ACPI device per HID,
> >>> which isn't right.
> >>
> >> In this case we probably need something like
> >>
> >> struct acpi_device *
> >> acpi_dev_get_next_match_dev(struct acpi_device *adev,
> >> 			    const char *hid, const char *uid, s64 hrv)
> >> {
> >> 	struct device *start = adev ? &adev->dev : NULL;
> >> 	...
> >> 	dev = bus_find_device(&acpi_bus_type, start, &match, acpi_dev_match_cb);
> >> 	...
> >> }
> >>
> >> in drivers/acpi/utils.c and
> >>
> >> static inline struct acpi_device *
> >> acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
> >> {
> >> 	return acpi_dev_get_next_match_dev(NULL, hid, uid, hrv);
> >> }
> >>
> >> in include/linux/acpi.h.
> >>
> >> Then we may add
> >>
> >> #define for_each_acpi_dev_match(adev, hid, uid, hrv)			\
> >> 	for (adev = acpi_dev_get_first_match_dev(hid, uid, hrv);	\
> >> 	     adev;							\
> >> 	     adev = acpi_dev_get_next_match_dev(adev, hid, uid, hrv))
> >
> > What the cio2-bridge code needs is indeed
> >
> > 	for each hid in supported hids:
> > 		for each acpi device that is compatible with hid:
> > 			...
> >
> > which could also be expressed as
> >
> > 	for each acpi device:
> > 		if acpi device hid is in supported hids:
> > 			...
> >
> > I don't mind either option, I'll happily follow the preference of the
> > ACPI maintainers.
>
> Does this need raising elsewhere then? The original idea of just
> bus_for_each_dev(&acpi_bus_type...) I have now tested and it works fine,
> but it does mean that I need to export acpi_bus_type (currently that
> symbol's not available)...that seems much simpler to me but I'm not sure
> whether that's something to avoid, and if so whether Andy's approach is
> better.
> 
> Thoughts?

I like simple options :-) A patch to export acpi_bus_type, with enough
context in the commit message (and in the cover latter of the series),
should be enough to provide all the information the ACPI maintainers
need to decide which option is best. With a bit of luck that patch will
be considered the best option and no extra work will be needed.

-- 
Regards,

Laurent Pinchart