| Message ID | 20220327161344.50477-2-djrscally@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add multiple-consumer support to int3472-tps68470 driver | expand |
On Sun, Mar 27, 2022 at 6:13 PM Daniel Scally <djrscally@gmail.com> 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. > > Replace that function with a new function that fetches the next consumer > of a supplier device. Where no "previous" consumer is passed in, it > behaves identically to the original function. > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v2: > > - Removed acpi_dev_get_first_consumer_dev() entirely > > drivers/acpi/scan.c | 37 +++++++++++++++------ > drivers/platform/x86/intel/int3472/common.c | 2 +- > include/acpi/acpi_bus.h | 4 ++- > 3 files changed, 30 insertions(+), 13 deletions(-) > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index 010ef0b28374..8797e4a33674 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -2215,9 +2215,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; I would prefer struct acpi_device **adev_p = data; struct acpi_device *adev = *adev_p; > + > + /* > + * 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; *adev_p = NULL; > + > + return 0; > + } > > adev = acpi_bus_get_acpi_device(dep->consumer); > if (adev) { > @@ -2348,25 +2360,28 @@ 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. > + * If the returned adev is not passed as @start to this function, 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; And here if (adev == start) return NULL; return adev; > } > -EXPORT_SYMBOL_GPL(acpi_dev_get_first_consumer_dev); > +EXPORT_SYMBOL_GPL(acpi_dev_get_next_consumer_dev); > > /** > * acpi_bus_scan - Add ACPI device node objects in a given namespace scope. > diff --git a/drivers/platform/x86/intel/int3472/common.c b/drivers/platform/x86/intel/int3472/common.c > index 77cf058e4168..9db2bb0bbba4 100644 > --- a/drivers/platform/x86/intel/int3472/common.c > +++ b/drivers/platform/x86/intel/int3472/common.c > @@ -62,7 +62,7 @@ int skl_int3472_get_sensor_adev_and_name(struct device *dev, > struct acpi_device *sensor; > int ret = 0; > > - sensor = acpi_dev_get_first_consumer_dev(adev); > + sensor = acpi_dev_get_next_consumer_dev(adev, NULL); > if (!sensor) { > dev_err(dev, "INT3472 seems to have no dependents.\n"); > return -ENODEV; > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index 2f93ecf05dac..cdc726d251b6 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -696,7 +696,9 @@ 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_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 * > acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv); > struct acpi_device * > -- > 2.25.1 >
Hello On 05/04/2022 13:57, Rafael J. Wysocki wrote: > On Sun, Mar 27, 2022 at 6:13 PM Daniel Scally <djrscally@gmail.com> 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. >> >> Replace that function with a new function that fetches the next consumer >> of a supplier device. Where no "previous" consumer is passed in, it >> behaves identically to the original function. >> >> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >> Signed-off-by: Daniel Scally <djrscally@gmail.com> >> --- >> Changes in v2: >> >> - Removed acpi_dev_get_first_consumer_dev() entirely >> >> drivers/acpi/scan.c | 37 +++++++++++++++------ >> drivers/platform/x86/intel/int3472/common.c | 2 +- >> include/acpi/acpi_bus.h | 4 ++- >> 3 files changed, 30 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c >> index 010ef0b28374..8797e4a33674 100644 >> --- a/drivers/acpi/scan.c >> +++ b/drivers/acpi/scan.c >> @@ -2215,9 +2215,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; > I would prefer > > struct acpi_device **adev_p = data; > struct acpi_device *adev = *adev_p; This and the below are fine for me; I'll send a v3 > >> + >> + /* >> + * 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; > *adev_p = NULL; > >> + >> + return 0; >> + } >> >> adev = acpi_bus_get_acpi_device(dep->consumer); >> if (adev) { >> @@ -2348,25 +2360,28 @@ 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. >> + * If the returned adev is not passed as @start to this function, 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; > And here > > if (adev == start) > return NULL; > > return adev; > >> } >> -EXPORT_SYMBOL_GPL(acpi_dev_get_first_consumer_dev); >> +EXPORT_SYMBOL_GPL(acpi_dev_get_next_consumer_dev); >> >> /** >> * acpi_bus_scan - Add ACPI device node objects in a given namespace scope. >> diff --git a/drivers/platform/x86/intel/int3472/common.c b/drivers/platform/x86/intel/int3472/common.c >> index 77cf058e4168..9db2bb0bbba4 100644 >> --- a/drivers/platform/x86/intel/int3472/common.c >> +++ b/drivers/platform/x86/intel/int3472/common.c >> @@ -62,7 +62,7 @@ int skl_int3472_get_sensor_adev_and_name(struct device *dev, >> struct acpi_device *sensor; >> int ret = 0; >> >> - sensor = acpi_dev_get_first_consumer_dev(adev); >> + sensor = acpi_dev_get_next_consumer_dev(adev, NULL); >> if (!sensor) { >> dev_err(dev, "INT3472 seems to have no dependents.\n"); >> return -ENODEV; >> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h >> index 2f93ecf05dac..cdc726d251b6 100644 >> --- a/include/acpi/acpi_bus.h >> +++ b/include/acpi/acpi_bus.h >> @@ -696,7 +696,9 @@ 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_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 * >> acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv); >> struct acpi_device * >> -- >> 2.25.1 >>
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 010ef0b28374..8797e4a33674 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -2215,9 +2215,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) { @@ -2348,25 +2360,28 @@ 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. + * If the returned adev is not passed as @start to this function, 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_first_consumer_dev); +EXPORT_SYMBOL_GPL(acpi_dev_get_next_consumer_dev); /** * acpi_bus_scan - Add ACPI device node objects in a given namespace scope. diff --git a/drivers/platform/x86/intel/int3472/common.c b/drivers/platform/x86/intel/int3472/common.c index 77cf058e4168..9db2bb0bbba4 100644 --- a/drivers/platform/x86/intel/int3472/common.c +++ b/drivers/platform/x86/intel/int3472/common.c @@ -62,7 +62,7 @@ int skl_int3472_get_sensor_adev_and_name(struct device *dev, struct acpi_device *sensor; int ret = 0; - sensor = acpi_dev_get_first_consumer_dev(adev); + sensor = acpi_dev_get_next_consumer_dev(adev, NULL); if (!sensor) { dev_err(dev, "INT3472 seems to have no dependents.\n"); return -ENODEV; diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index 2f93ecf05dac..cdc726d251b6 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -696,7 +696,9 @@ 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_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 * acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv); struct acpi_device *
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. Replace that function with a new function that fetches the next consumer of a supplier device. Where no "previous" consumer is passed in, it behaves identically to the original function. Reviewed-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Daniel Scally <djrscally@gmail.com> --- Changes in v2: - Removed acpi_dev_get_first_consumer_dev() entirely drivers/acpi/scan.c | 37 +++++++++++++++------ drivers/platform/x86/intel/int3472/common.c | 2 +- include/acpi/acpi_bus.h | 4 ++- 3 files changed, 30 insertions(+), 13 deletions(-)