| Message ID | 20220216225304.53911-2-djrscally@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add multiple-consumer support to int3472-tps68470 driver | expand |
Hi, On 2/16/22 23:52, Daniel Scally wrote: > In commit b83e2b306736 ("ACPI: scan: Add function to fetch dependent of ACPI > device") we added a means of fetching the first device to declare itself > dependent on another ACPI device in the _DEP method. One assumption > in that patch was that there would only be a single consuming device, > but this has not held. > > Extend the functionality by adding a new function that fetches the > next consumer of a supplier device. We can then simplify the original > function by simply calling the new one. > > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > drivers/acpi/scan.c | 47 ++++++++++++++++++++++++++++++++++------- > include/acpi/acpi_bus.h | 2 ++ > 2 files changed, 41 insertions(+), 8 deletions(-) > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index 4463c2eda61e..b3ed664ee1cb 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -2256,9 +2256,21 @@ static void acpi_bus_attach(struct acpi_device *device, bool first_pass) > device->handler->hotplug.notify_online(device); > } > > -static int acpi_dev_get_first_consumer_dev_cb(struct acpi_dep_data *dep, void *data) > +static int acpi_dev_get_next_consumer_dev_cb(struct acpi_dep_data *dep, void *data) > { > - struct acpi_device *adev; > + struct acpi_device *adev = *(struct acpi_device **)data; > + > + /* > + * If we're passed a 'previous' consumer device then we need to skip > + * any consumers until we meet the previous one, and then NULL @data > + * so the next one can be returned. > + */ > + if (adev) { > + if (dep->consumer == adev->handle) > + *(struct acpi_device **)data = NULL; > + > + return 0; > + } > > adev = acpi_bus_get_acpi_device(dep->consumer); > if (adev) { > @@ -2389,23 +2401,42 @@ bool acpi_dev_ready_for_enumeration(const struct acpi_device *device) > EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration); > > /** > - * acpi_dev_get_first_consumer_dev - Return ACPI device dependent on @supplier > + * acpi_dev_get_next_consumer_dev - Return the next adev dependent on @supplier > * @supplier: Pointer to the dependee device > + * @start: Pointer to the current dependent device > * > - * Returns the first &struct acpi_device which declares itself dependent on > + * Returns the next &struct acpi_device which declares itself dependent on > * @supplier via the _DEP buffer, parsed from the acpi_dep_list. > * > * The caller is responsible for putting the reference to adev when it is no > * longer needed. This bit of the help text seems to not be entirely correct, since the reference to start gets consumed by this, so the caller only needs to put the device when it does NOT pass it as start to another acpi_dev_get_next_consumer_dev call. > */ > -struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier) > +struct acpi_device *acpi_dev_get_next_consumer_dev(struct acpi_device *supplier, > + struct acpi_device *start) > { > - struct acpi_device *adev = NULL; > + struct acpi_device *adev = start; > > acpi_walk_dep_device_list(supplier->handle, > - acpi_dev_get_first_consumer_dev_cb, &adev); > + acpi_dev_get_next_consumer_dev_cb, &adev); > > - return adev; > + acpi_dev_put(start); > + return adev == start ? NULL : adev; > +} > +EXPORT_SYMBOL_GPL(acpi_dev_get_next_consumer_dev); > + > +/** > + * acpi_dev_get_first_consumer_dev - Return ACPI device dependent on @supplier > + * @supplier: Pointer to the dependee device > + * > + * Returns the first &struct acpi_device which declares itself dependent on > + * @supplier via the _DEP buffer, parsed from the acpi_dep_list. > + * > + * The caller is responsible for putting the reference to adev when it is no > + * longer needed. > + */ > +struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier) > +{ > + return acpi_dev_get_next_consumer_dev(supplier, NULL); > } > EXPORT_SYMBOL_GPL(acpi_dev_get_first_consumer_dev); The only caller of this is skl_int3472_get_sensor_adev_and_name() IMHO it would be better to just move that over to acpi_dev_get_next_consumer_dev(..., NULL); in this same patch and just drop acpi_dev_get_first_consumer_dev() all together. I expect this entire series to get merged through the pdx86 tree, so from that pov doing this should be fine.. Regards, Hans > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index ca88c4706f2b..8b06fef04722 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -720,6 +720,8 @@ bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const ch > > void acpi_dev_clear_dependencies(struct acpi_device *supplier); > bool acpi_dev_ready_for_enumeration(const struct acpi_device *device); > +struct acpi_device *acpi_dev_get_next_consumer_dev(struct acpi_device *supplier, > + struct acpi_device *start); > struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier); > struct acpi_device * > acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
Hi, On 2/21/22 10:50, Hans de Goede wrote: > Hi, > > On 2/16/22 23:52, Daniel Scally wrote: >> In commit b83e2b306736 ("ACPI: scan: Add function to fetch dependent of ACPI >> device") we added a means of fetching the first device to declare itself >> dependent on another ACPI device in the _DEP method. One assumption >> in that patch was that there would only be a single consuming device, >> but this has not held. >> >> Extend the functionality by adding a new function that fetches the >> next consumer of a supplier device. We can then simplify the original >> function by simply calling the new one. >> >> Signed-off-by: Daniel Scally <djrscally@gmail.com> >> --- >> drivers/acpi/scan.c | 47 ++++++++++++++++++++++++++++++++++------- >> include/acpi/acpi_bus.h | 2 ++ >> 2 files changed, 41 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c >> index 4463c2eda61e..b3ed664ee1cb 100644 >> --- a/drivers/acpi/scan.c >> +++ b/drivers/acpi/scan.c >> @@ -2256,9 +2256,21 @@ static void acpi_bus_attach(struct acpi_device *device, bool first_pass) >> device->handler->hotplug.notify_online(device); >> } >> >> -static int acpi_dev_get_first_consumer_dev_cb(struct acpi_dep_data *dep, void *data) >> +static int acpi_dev_get_next_consumer_dev_cb(struct acpi_dep_data *dep, void *data) >> { >> - struct acpi_device *adev; >> + struct acpi_device *adev = *(struct acpi_device **)data; >> + >> + /* >> + * If we're passed a 'previous' consumer device then we need to skip >> + * any consumers until we meet the previous one, and then NULL @data >> + * so the next one can be returned. >> + */ >> + if (adev) { >> + if (dep->consumer == adev->handle) >> + *(struct acpi_device **)data = NULL; >> + >> + return 0; >> + } >> >> adev = acpi_bus_get_acpi_device(dep->consumer); >> if (adev) { >> @@ -2389,23 +2401,42 @@ bool acpi_dev_ready_for_enumeration(const struct acpi_device *device) >> EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration); >> >> /** >> - * acpi_dev_get_first_consumer_dev - Return ACPI device dependent on @supplier >> + * acpi_dev_get_next_consumer_dev - Return the next adev dependent on @supplier >> * @supplier: Pointer to the dependee device >> + * @start: Pointer to the current dependent device >> * >> - * Returns the first &struct acpi_device which declares itself dependent on >> + * Returns the next &struct acpi_device which declares itself dependent on >> * @supplier via the _DEP buffer, parsed from the acpi_dep_list. >> * >> * The caller is responsible for putting the reference to adev when it is no >> * longer needed. > > This bit of the help text seems to not be entirely correct, since the reference to > start gets consumed by this, so the caller only needs to put the device when it > does NOT pass it as start to another acpi_dev_get_next_consumer_dev call. > > > >> */ >> -struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier) >> +struct acpi_device *acpi_dev_get_next_consumer_dev(struct acpi_device *supplier, >> + struct acpi_device *start) >> { >> - struct acpi_device *adev = NULL; >> + struct acpi_device *adev = start; >> >> acpi_walk_dep_device_list(supplier->handle, >> - acpi_dev_get_first_consumer_dev_cb, &adev); >> + acpi_dev_get_next_consumer_dev_cb, &adev); >> >> - return adev; >> + acpi_dev_put(start); >> + return adev == start ? NULL : adev; >> +} >> +EXPORT_SYMBOL_GPL(acpi_dev_get_next_consumer_dev); >> + >> +/** >> + * acpi_dev_get_first_consumer_dev - Return ACPI device dependent on @supplier >> + * @supplier: Pointer to the dependee device >> + * >> + * Returns the first &struct acpi_device which declares itself dependent on >> + * @supplier via the _DEP buffer, parsed from the acpi_dep_list. >> + * >> + * The caller is responsible for putting the reference to adev when it is no >> + * longer needed. >> + */ >> +struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier) >> +{ >> + return acpi_dev_get_next_consumer_dev(supplier, NULL); >> } >> EXPORT_SYMBOL_GPL(acpi_dev_get_first_consumer_dev); > > The only caller of this is skl_int3472_get_sensor_adev_and_name() IMHO it would > be better to just move that over to acpi_dev_get_next_consumer_dev(..., NULL); > in this same patch and just drop acpi_dev_get_first_consumer_dev() all together. > > I expect this entire series to get merged through the pdx86 tree, so from > that pov doing this should be fine.. I forgot to add: that otherwise this looks good to me, so with the above addressed you may add my: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans >> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h >> index ca88c4706f2b..8b06fef04722 100644 >> --- a/include/acpi/acpi_bus.h >> +++ b/include/acpi/acpi_bus.h >> @@ -720,6 +720,8 @@ bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const ch >> >> void acpi_dev_clear_dependencies(struct acpi_device *supplier); >> bool acpi_dev_ready_for_enumeration(const struct acpi_device *device); >> +struct acpi_device *acpi_dev_get_next_consumer_dev(struct acpi_device *supplier, >> + struct acpi_device *start); >> struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier); >> struct acpi_device * >> acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 4463c2eda61e..b3ed664ee1cb 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -2256,9 +2256,21 @@ static void acpi_bus_attach(struct acpi_device *device, bool first_pass) device->handler->hotplug.notify_online(device); } -static int acpi_dev_get_first_consumer_dev_cb(struct acpi_dep_data *dep, void *data) +static int acpi_dev_get_next_consumer_dev_cb(struct acpi_dep_data *dep, void *data) { - struct acpi_device *adev; + struct acpi_device *adev = *(struct acpi_device **)data; + + /* + * If we're passed a 'previous' consumer device then we need to skip + * any consumers until we meet the previous one, and then NULL @data + * so the next one can be returned. + */ + if (adev) { + if (dep->consumer == adev->handle) + *(struct acpi_device **)data = NULL; + + return 0; + } adev = acpi_bus_get_acpi_device(dep->consumer); if (adev) { @@ -2389,23 +2401,42 @@ bool acpi_dev_ready_for_enumeration(const struct acpi_device *device) EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration); /** - * acpi_dev_get_first_consumer_dev - Return ACPI device dependent on @supplier + * acpi_dev_get_next_consumer_dev - Return the next adev dependent on @supplier * @supplier: Pointer to the dependee device + * @start: Pointer to the current dependent device * - * Returns the first &struct acpi_device which declares itself dependent on + * Returns the next &struct acpi_device which declares itself dependent on * @supplier via the _DEP buffer, parsed from the acpi_dep_list. * * The caller is responsible for putting the reference to adev when it is no * longer needed. */ -struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier) +struct acpi_device *acpi_dev_get_next_consumer_dev(struct acpi_device *supplier, + struct acpi_device *start) { - struct acpi_device *adev = NULL; + struct acpi_device *adev = start; acpi_walk_dep_device_list(supplier->handle, - acpi_dev_get_first_consumer_dev_cb, &adev); + acpi_dev_get_next_consumer_dev_cb, &adev); - return adev; + acpi_dev_put(start); + return adev == start ? NULL : adev; +} +EXPORT_SYMBOL_GPL(acpi_dev_get_next_consumer_dev); + +/** + * acpi_dev_get_first_consumer_dev - Return ACPI device dependent on @supplier + * @supplier: Pointer to the dependee device + * + * Returns the first &struct acpi_device which declares itself dependent on + * @supplier via the _DEP buffer, parsed from the acpi_dep_list. + * + * The caller is responsible for putting the reference to adev when it is no + * longer needed. + */ +struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier) +{ + return acpi_dev_get_next_consumer_dev(supplier, NULL); } EXPORT_SYMBOL_GPL(acpi_dev_get_first_consumer_dev); diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index ca88c4706f2b..8b06fef04722 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -720,6 +720,8 @@ bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const ch void acpi_dev_clear_dependencies(struct acpi_device *supplier); bool acpi_dev_ready_for_enumeration(const struct acpi_device *device); +struct acpi_device *acpi_dev_get_next_consumer_dev(struct acpi_device *supplier, + struct acpi_device *start); struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier); struct acpi_device * acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
In commit b83e2b306736 ("ACPI: scan: Add function to fetch dependent of ACPI device") we added a means of fetching the first device to declare itself dependent on another ACPI device in the _DEP method. One assumption in that patch was that there would only be a single consuming device, but this has not held. Extend the functionality by adding a new function that fetches the next consumer of a supplier device. We can then simplify the original function by simply calling the new one. Signed-off-by: Daniel Scally <djrscally@gmail.com> --- drivers/acpi/scan.c | 47 ++++++++++++++++++++++++++++++++++------- include/acpi/acpi_bus.h | 2 ++ 2 files changed, 41 insertions(+), 8 deletions(-)