| Message ID | 20210118003428.568892-2-djrscally@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Introduce intel_skl_int3472 driver | expand |
Hi Daniel, Thank you for the patch. On Mon, Jan 18, 2021 at 12:34:22AM +0000, Daniel Scally wrote: > I need to be able to identify devices which declare themselves to be > dependent on other devices through _DEP; add this function to utils.c > and export it to the rest of the ACPI layer. > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v2: > - Introduced > > drivers/acpi/acpi_lpss.c | 24 ------------------------ > drivers/acpi/internal.h | 1 + > drivers/acpi/utils.c | 24 ++++++++++++++++++++++++ > 3 files changed, 25 insertions(+), 24 deletions(-) > > diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c > index be73974ce449..70c7d9a3f715 100644 > --- a/drivers/acpi/acpi_lpss.c > +++ b/drivers/acpi/acpi_lpss.c > @@ -543,30 +543,6 @@ static struct device *acpi_lpss_find_device(const char *hid, const char *uid) > return bus_find_device(&pci_bus_type, NULL, &data, match_hid_uid); > } > > -static bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) > -{ > - struct acpi_handle_list dep_devices; > - acpi_status status; > - int i; > - > - if (!acpi_has_method(adev->handle, "_DEP")) > - return false; > - > - status = acpi_evaluate_reference(adev->handle, "_DEP", NULL, > - &dep_devices); > - if (ACPI_FAILURE(status)) { > - dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n"); > - return false; > - } > - > - for (i = 0; i < dep_devices.count; i++) { > - if (dep_devices.handles[i] == handle) > - return true; > - } > - > - return false; > -} > - > static void acpi_lpss_link_consumer(struct device *dev1, > const struct lpss_device_links *link) > { > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h > index cb229e24c563..ee62c0973576 100644 > --- a/drivers/acpi/internal.h > +++ b/drivers/acpi/internal.h > @@ -79,6 +79,7 @@ static inline void acpi_lpss_init(void) {} > #endif > > void acpi_apd_init(void); > +bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle); "lpss" stands for low power subsystem, an Intel device within the PCH that handles I2C, SPI, UART, ... I think the function should be renamed, as it's now generic. acpi_dev_has_dep() is a potential candidate, I'm sure better ones exist. A bit of kerneldoc would also not hurt. > > acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src); > bool acpi_queue_hotplug_work(struct work_struct *work); > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > index ddca1550cce6..78b38775f18b 100644 > --- a/drivers/acpi/utils.c > +++ b/drivers/acpi/utils.c > @@ -807,6 +807,30 @@ static int acpi_dev_match_cb(struct device *dev, const void *data) > return hrv == match->hrv; > } > > +bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) > +{ > + struct acpi_handle_list dep_devices; > + acpi_status status; > + int i; > + > + if (!acpi_has_method(adev->handle, "_DEP")) > + return false; > + > + status = acpi_evaluate_reference(adev->handle, "_DEP", NULL, > + &dep_devices); > + if (ACPI_FAILURE(status)) { > + dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n"); > + return false; > + } > + > + for (i = 0; i < dep_devices.count; i++) { > + if (dep_devices.handles[i] == handle) > + return true; > + } > + > + return false; > +} > + > /** > * acpi_dev_present - Detect that a given ACPI device is present > * @hid: Hardware ID of the device.
Hi Laurent On 18/01/2021 07:24, Laurent Pinchart wrote: > Hi Daniel, > > Thank you for the patch. > > On Mon, Jan 18, 2021 at 12:34:22AM +0000, Daniel Scally wrote: >> I need to be able to identify devices which declare themselves to be >> dependent on other devices through _DEP; add this function to utils.c >> and export it to the rest of the ACPI layer. >> >> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Signed-off-by: Daniel Scally <djrscally@gmail.com> >> --- >> Changes in v2: >> - Introduced >> >> drivers/acpi/acpi_lpss.c | 24 ------------------------ >> drivers/acpi/internal.h | 1 + >> drivers/acpi/utils.c | 24 ++++++++++++++++++++++++ >> 3 files changed, 25 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c >> index be73974ce449..70c7d9a3f715 100644 >> --- a/drivers/acpi/acpi_lpss.c >> +++ b/drivers/acpi/acpi_lpss.c >> @@ -543,30 +543,6 @@ static struct device *acpi_lpss_find_device(const char *hid, const char *uid) >> return bus_find_device(&pci_bus_type, NULL, &data, match_hid_uid); >> } >> >> -static bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) >> -{ >> - struct acpi_handle_list dep_devices; >> - acpi_status status; >> - int i; >> - >> - if (!acpi_has_method(adev->handle, "_DEP")) >> - return false; >> - >> - status = acpi_evaluate_reference(adev->handle, "_DEP", NULL, >> - &dep_devices); >> - if (ACPI_FAILURE(status)) { >> - dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n"); >> - return false; >> - } >> - >> - for (i = 0; i < dep_devices.count; i++) { >> - if (dep_devices.handles[i] == handle) >> - return true; >> - } >> - >> - return false; >> -} >> - >> static void acpi_lpss_link_consumer(struct device *dev1, >> const struct lpss_device_links *link) >> { >> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h >> index cb229e24c563..ee62c0973576 100644 >> --- a/drivers/acpi/internal.h >> +++ b/drivers/acpi/internal.h >> @@ -79,6 +79,7 @@ static inline void acpi_lpss_init(void) {} >> #endif >> >> void acpi_apd_init(void); >> +bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle); > "lpss" stands for low power subsystem, an Intel device within the PCH > that handles I2C, SPI, UART, ... I think the function should be renamed, > as it's now generic. acpi_dev_has_dep() is a potential candidate, I'm > sure better ones exist. A bit of kerneldoc would also not hurt. Okedokey; I shall add kerneldoc and think of an appropriate name, plus rename all the uses of it. How about acpi_dev_is_dep()? "has_dep" to me implies anything at all in _DEP should return true. >> >> acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src); >> bool acpi_queue_hotplug_work(struct work_struct *work); >> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c >> index ddca1550cce6..78b38775f18b 100644 >> --- a/drivers/acpi/utils.c >> +++ b/drivers/acpi/utils.c >> @@ -807,6 +807,30 @@ static int acpi_dev_match_cb(struct device *dev, const void *data) >> return hrv == match->hrv; >> } >> >> +bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) >> +{ >> + struct acpi_handle_list dep_devices; >> + acpi_status status; >> + int i; >> + >> + if (!acpi_has_method(adev->handle, "_DEP")) >> + return false; >> + >> + status = acpi_evaluate_reference(adev->handle, "_DEP", NULL, >> + &dep_devices); >> + if (ACPI_FAILURE(status)) { >> + dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n"); >> + return false; >> + } >> + >> + for (i = 0; i < dep_devices.count; i++) { >> + if (dep_devices.handles[i] == handle) >> + return true; >> + } >> + >> + return false; >> +} >> + >> /** >> * acpi_dev_present - Detect that a given ACPI device is present >> * @hid: Hardware ID of the device.
On Mon, Jan 18, 2021 at 12:34:22AM +0000, Daniel Scally wrote: > I need to be able to identify devices which declare themselves to be > dependent on other devices through _DEP; add this function to utils.c > and export it to the rest of the ACPI layer. Prefix -> "ACPI / utils: " Otherwise good to me Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v2: > - Introduced > > drivers/acpi/acpi_lpss.c | 24 ------------------------ > drivers/acpi/internal.h | 1 + > drivers/acpi/utils.c | 24 ++++++++++++++++++++++++ > 3 files changed, 25 insertions(+), 24 deletions(-) > > diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c > index be73974ce449..70c7d9a3f715 100644 > --- a/drivers/acpi/acpi_lpss.c > +++ b/drivers/acpi/acpi_lpss.c > @@ -543,30 +543,6 @@ static struct device *acpi_lpss_find_device(const char *hid, const char *uid) > return bus_find_device(&pci_bus_type, NULL, &data, match_hid_uid); > } > > -static bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) > -{ > - struct acpi_handle_list dep_devices; > - acpi_status status; > - int i; > - > - if (!acpi_has_method(adev->handle, "_DEP")) > - return false; > - > - status = acpi_evaluate_reference(adev->handle, "_DEP", NULL, > - &dep_devices); > - if (ACPI_FAILURE(status)) { > - dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n"); > - return false; > - } > - > - for (i = 0; i < dep_devices.count; i++) { > - if (dep_devices.handles[i] == handle) > - return true; > - } > - > - return false; > -} > - > static void acpi_lpss_link_consumer(struct device *dev1, > const struct lpss_device_links *link) > { > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h > index cb229e24c563..ee62c0973576 100644 > --- a/drivers/acpi/internal.h > +++ b/drivers/acpi/internal.h > @@ -79,6 +79,7 @@ static inline void acpi_lpss_init(void) {} > #endif > > void acpi_apd_init(void); > +bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle); > > acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src); > bool acpi_queue_hotplug_work(struct work_struct *work); > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > index ddca1550cce6..78b38775f18b 100644 > --- a/drivers/acpi/utils.c > +++ b/drivers/acpi/utils.c > @@ -807,6 +807,30 @@ static int acpi_dev_match_cb(struct device *dev, const void *data) > return hrv == match->hrv; > } > > +bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) > +{ > + struct acpi_handle_list dep_devices; > + acpi_status status; > + int i; > + > + if (!acpi_has_method(adev->handle, "_DEP")) > + return false; > + > + status = acpi_evaluate_reference(adev->handle, "_DEP", NULL, > + &dep_devices); > + if (ACPI_FAILURE(status)) { > + dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n"); > + return false; > + } > + > + for (i = 0; i < dep_devices.count; i++) { > + if (dep_devices.handles[i] == handle) > + return true; > + } > + > + return false; > +} > + > /** > * acpi_dev_present - Detect that a given ACPI device is present > * @hid: Hardware ID of the device. > -- > 2.25.1 >
On Mon, Jan 18, 2021 at 09:24:10AM +0200, Laurent Pinchart wrote: > On Mon, Jan 18, 2021 at 12:34:22AM +0000, Daniel Scally wrote: ... > > +bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle); > > "lpss" stands for low power subsystem, an Intel device within the PCH > that handles I2C, SPI, UART, ... I think the function should be renamed, > as it's now generic. acpi_dev_has_dep() is a potential candidate, I'm > sure better ones exist. A bit of kerneldoc would also not hurt. Actually a good suggestions. Please apply my tag after addressing above.
On 18/01/2021 12:29, Andy Shevchenko wrote: > On Mon, Jan 18, 2021 at 09:24:10AM +0200, Laurent Pinchart wrote: >> On Mon, Jan 18, 2021 at 12:34:22AM +0000, Daniel Scally wrote: > ... > >>> +bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle); >> "lpss" stands for low power subsystem, an Intel device within the PCH >> that handles I2C, SPI, UART, ... I think the function should be renamed, >> as it's now generic. acpi_dev_has_dep() is a potential candidate, I'm >> sure better ones exist. A bit of kerneldoc would also not hurt. > Actually a good suggestions. Please apply my tag after addressing above. Will do, thanks
On Mon, Jan 18, 2021 at 1:30 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Jan 18, 2021 at 12:34:22AM +0000, Daniel Scally wrote: > > I need to be able to identify devices which declare themselves to be > > dependent on other devices through _DEP; add this function to utils.c > > and export it to the rest of the ACPI layer. > > Prefix -> "ACPI / utils: " Preferably "ACPI: utils: " for that matter and yes, please rename the function while moving it. > Otherwise good to me > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Signed-off-by: Daniel Scally <djrscally@gmail.com> > > --- > > Changes in v2: > > - Introduced > > > > drivers/acpi/acpi_lpss.c | 24 ------------------------ > > drivers/acpi/internal.h | 1 + > > drivers/acpi/utils.c | 24 ++++++++++++++++++++++++ > > 3 files changed, 25 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c > > index be73974ce449..70c7d9a3f715 100644 > > --- a/drivers/acpi/acpi_lpss.c > > +++ b/drivers/acpi/acpi_lpss.c > > @@ -543,30 +543,6 @@ static struct device *acpi_lpss_find_device(const char *hid, const char *uid) > > return bus_find_device(&pci_bus_type, NULL, &data, match_hid_uid); > > } > > > > -static bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) > > -{ > > - struct acpi_handle_list dep_devices; > > - acpi_status status; > > - int i; > > - > > - if (!acpi_has_method(adev->handle, "_DEP")) > > - return false; > > - > > - status = acpi_evaluate_reference(adev->handle, "_DEP", NULL, > > - &dep_devices); > > - if (ACPI_FAILURE(status)) { > > - dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n"); > > - return false; > > - } > > - > > - for (i = 0; i < dep_devices.count; i++) { > > - if (dep_devices.handles[i] == handle) > > - return true; > > - } > > - > > - return false; > > -} > > - > > static void acpi_lpss_link_consumer(struct device *dev1, > > const struct lpss_device_links *link) > > { > > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h > > index cb229e24c563..ee62c0973576 100644 > > --- a/drivers/acpi/internal.h > > +++ b/drivers/acpi/internal.h > > @@ -79,6 +79,7 @@ static inline void acpi_lpss_init(void) {} > > #endif > > > > void acpi_apd_init(void); > > +bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle); > > > > acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src); > > bool acpi_queue_hotplug_work(struct work_struct *work); > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > > index ddca1550cce6..78b38775f18b 100644 > > --- a/drivers/acpi/utils.c > > +++ b/drivers/acpi/utils.c > > @@ -807,6 +807,30 @@ static int acpi_dev_match_cb(struct device *dev, const void *data) > > return hrv == match->hrv; > > } > > > > +bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) > > +{ > > + struct acpi_handle_list dep_devices; > > + acpi_status status; > > + int i; > > + > > + if (!acpi_has_method(adev->handle, "_DEP")) > > + return false; > > + > > + status = acpi_evaluate_reference(adev->handle, "_DEP", NULL, > > + &dep_devices); > > + if (ACPI_FAILURE(status)) { > > + dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n"); > > + return false; > > + } > > + > > + for (i = 0; i < dep_devices.count; i++) { > > + if (dep_devices.handles[i] == handle) > > + return true; > > + } > > + > > + return false; > > +} > > + > > /** > > * acpi_dev_present - Detect that a given ACPI device is present > > * @hid: Hardware ID of the device. > > -- > > 2.25.1 > > > > -- > With Best Regards, > Andy Shevchenko > >
On Mon, Jan 18, 2021 at 05:06:30PM +0100, Rafael J. Wysocki wrote: > On Mon, Jan 18, 2021 at 1:30 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Jan 18, 2021 at 12:34:22AM +0000, Daniel Scally wrote: > > > I need to be able to identify devices which declare themselves to be > > > dependent on other devices through _DEP; add this function to utils.c > > > and export it to the rest of the ACPI layer. > > > > Prefix -> "ACPI / utils: " > > Preferably "ACPI: utils: " for that matter Ah, good to know! I was always bending between / and : there.
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c index be73974ce449..70c7d9a3f715 100644 --- a/drivers/acpi/acpi_lpss.c +++ b/drivers/acpi/acpi_lpss.c @@ -543,30 +543,6 @@ static struct device *acpi_lpss_find_device(const char *hid, const char *uid) return bus_find_device(&pci_bus_type, NULL, &data, match_hid_uid); } -static bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) -{ - struct acpi_handle_list dep_devices; - acpi_status status; - int i; - - if (!acpi_has_method(adev->handle, "_DEP")) - return false; - - status = acpi_evaluate_reference(adev->handle, "_DEP", NULL, - &dep_devices); - if (ACPI_FAILURE(status)) { - dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n"); - return false; - } - - for (i = 0; i < dep_devices.count; i++) { - if (dep_devices.handles[i] == handle) - return true; - } - - return false; -} - static void acpi_lpss_link_consumer(struct device *dev1, const struct lpss_device_links *link) { diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index cb229e24c563..ee62c0973576 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -79,6 +79,7 @@ static inline void acpi_lpss_init(void) {} #endif void acpi_apd_init(void); +bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle); acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src); bool acpi_queue_hotplug_work(struct work_struct *work); diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c index ddca1550cce6..78b38775f18b 100644 --- a/drivers/acpi/utils.c +++ b/drivers/acpi/utils.c @@ -807,6 +807,30 @@ static int acpi_dev_match_cb(struct device *dev, const void *data) return hrv == match->hrv; } +bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) +{ + struct acpi_handle_list dep_devices; + acpi_status status; + int i; + + if (!acpi_has_method(adev->handle, "_DEP")) + return false; + + status = acpi_evaluate_reference(adev->handle, "_DEP", NULL, + &dep_devices); + if (ACPI_FAILURE(status)) { + dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n"); + return false; + } + + for (i = 0; i < dep_devices.count; i++) { + if (dep_devices.handles[i] == handle) + return true; + } + + return false; +} + /** * acpi_dev_present - Detect that a given ACPI device is present * @hid: Hardware ID of the device.
I need to be able to identify devices which declare themselves to be dependent on other devices through _DEP; add this function to utils.c and export it to the rest of the ACPI layer. Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Daniel Scally <djrscally@gmail.com> --- Changes in v2: - Introduced drivers/acpi/acpi_lpss.c | 24 ------------------------ drivers/acpi/internal.h | 1 + drivers/acpi/utils.c | 24 ++++++++++++++++++++++++ 3 files changed, 25 insertions(+), 24 deletions(-)