On Mon, Nov 16, 2020 at 10:53 AM Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> On Fri, Nov 13, 2020 at 09:45:00PM +0200, Andy Shevchenko wrote:
> > On Fri, Nov 13, 2020 at 6:22 PM Laurent Pinchart wrote:
> > > 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:
> >
> > ...
> >
> > > > >> 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.
> >
> > The problem with ACPI bus is that it is not as simple as other buses,
> > i.e. it may have purely ACPI devices along with *companion* devices,
> > which are usually represented by platform bus. On top of that for
> > several ACPI devices there can be one physical node and it will be not
> > so clear what you are exactly looking for by traversing acpi_bus_type.
> > I believe it's hidden on purpose.
>
> Maybe there's something I don't get, as I'm not very familiar with the
> ACPI implementation in the kernel, but the code in the cio2-bridge,
> unless I'm mistaken, is really interested in ACPI devices on the ACPI
> bus, not companions or other devices related to the ACPI devices.
AFAICS cio2-bridge wants to find ACPI companion devices which are
enumerated as platform ones (or I²C or SPI).
> The
> iterators you have proposed above use bus_find_device() on
> acpi_bus_type, so I don't really see how they make a difference from a
> cio2-bridge point of view.
This seems to be true. The iterators have no means to distinguish
between companion devices and pure ACPI, for example.
For this one needs to call something like 'get first physical node'
followed by 'let's check that it has a companion and that the one we
have already got from ACPI bus iterator'.
> Is your point that acpi_bus_type shouldn't be
> exported because it could then be misused by *other* drivers ? Couldn't
> those drivers then equally misuse the iterators ?
My point is that the ACPI bus type here is not homogenous.
And thus I think it was the reason behind hiding it. I might be
mistaken and you may ask ACPI maintainers for the clarification.
In summary I think we are trying to solve a problem that has not yet
existed (devices with several same sensors). Do we have a DSDT of such
to look into?
--
With Best Regards,
Andy Shevchenko
