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

[Laurent Pinchart]

Hi Andy,

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

-- 
Regards,

Laurent Pinchart