diff mbox series

[14/18] acpi: utils: Add function to fetch dependent acpi_devices

Message ID 20201130133129.1024662-15-djrscally@gmail.com
State New
Headers show
Series Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows | expand

Commit Message

Daniel Scally Nov. 30, 2020, 1:31 p.m. UTC
ACPI devices declare themselves dependent on other devices via the _DEP
buffer. Fetching the dependee from dependent is a matter of parsing
_DEP, but currently there's no method to fetch dependent from dependee.
Add one, so we can parse sensors dependent on a PMIC from the PMIC's
acpi_driver.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes since RFC v3:

	- Patch introduced

 drivers/acpi/utils.c    | 68 +++++++++++++++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h |  2 ++
 2 files changed, 70 insertions(+)

Comments

Andy Shevchenko Nov. 30, 2020, 6:23 p.m. UTC | #1
On Mon, Nov 30, 2020 at 01:31:25PM +0000, Daniel Scally wrote:
> ACPI devices declare themselves dependent on other devices via the _DEP
> buffer. Fetching the dependee from dependent is a matter of parsing
> _DEP, but currently there's no method to fetch dependent from dependee.
> Add one, so we can parse sensors dependent on a PMIC from the PMIC's
> acpi_driver.

Do I understand correctly that it's an existing table provided by firmware that
(ab)uses _DEP in such way? Note, the specification doesn't tell we may use it
in this way, OTOH I don't remember if it strictly forbids such use.

So, please elaborate in the commit message why you need this and pint out to
the 6.5.8 "_DEP (Operation Region Dependencies)" which clearly says about
OpRegions and that part already supported by ACPI in the Linux, if I'm not
mistaken, need to refresh my memory.

...

> +	handle = adev->handle;
> +
> +	if (!acpi_has_method(handle, "_DEP"))
> +		return 0;
> +
> +	status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_handles);
> +	if (ACPI_FAILURE(status))
> +		return 0;
> +
> +	for (i = 0; i < dep_handles.count; i++) {
> +		struct acpi_device_info *info;
> +
> +		status = acpi_get_object_info(dep_handles.handles[i], &info);
> +		if (ACPI_FAILURE(status))
> +			continue;
> +
> +		if (info->valid & ACPI_VALID_HID) {
> +			ret = acpi_bus_get_device(dep_handles.handles[i], &candidate);
> +			if (ret || !candidate) {
> +				kfree(info);
> +				continue;
> +			}
> +
> +			if (candidate == dependee) {
> +				acpi_dev_put(candidate);
> +				kfree(info);
> +				return 1;
> +			}
> +
> +			kfree(info);
> +		}
> +	}

Can you utilize (by moving to here and export for ACPI layer the
acpi_lpss_dep()?
Andy Shevchenko Dec. 1, 2020, 3:10 p.m. UTC | #2
On Mon, Nov 30, 2020 at 11:54:44PM +0000, Dan Scally wrote:
> Hi Andy
> 
> On 30/11/2020 18:23, Andy Shevchenko wrote:
> > On Mon, Nov 30, 2020 at 01:31:25PM +0000, Daniel Scally wrote:
> >> ACPI devices declare themselves dependent on other devices via the _DEP
> >> buffer. Fetching the dependee from dependent is a matter of parsing
> >> _DEP, but currently there's no method to fetch dependent from dependee.
> >> Add one, so we can parse sensors dependent on a PMIC from the PMIC's
> >> acpi_driver.
> > Do I understand correctly that it's an existing table provided by firmware that
> > (ab)uses _DEP in such way? Note, the specification doesn't tell we may use it
> > in this way, OTOH I don't remember if it strictly forbids such use.
> >
> > So, please elaborate in the commit message why you need this and pint out to
> > the 6.5.8 "_DEP (Operation Region Dependencies)" which clearly says about
> > OpRegions and that part already supported by ACPI in the Linux, if I'm not
> > mistaken, need to refresh my memory.
> 
> 
> Laurent's reply is good explanation, but for example see my Lenovo Miix
> 510's DSDT:
> 
> 
> https://gist.githubusercontent.com/djrscally/e64d112180517352fa3392878b0f4a7d/raw/88b90b3ea4204fd7845257b6666fdade47cc2981/dsdt.dsl
> 
> 
> Search OVTI2680 and OVTI5648 for the cameras. Both are dependent on
> IN3472 devices (PMI0 and PMI1) which are the discrete type that we're
> attempting to handle here.

Yes, it seems since PMIC is kinda "power resource" (don't mix with real power
resource as by ACPI specifications) and that's why they decided to include it
into _DEP.  So, it seems a de facto common practice. Thus, it would be nice to
have the above in the commit message in some form. Can you do it?
Daniel Scally Dec. 1, 2020, 3:23 p.m. UTC | #3
On 01/12/2020 15:10, Andy Shevchenko wrote:
> On Mon, Nov 30, 2020 at 11:54:44PM +0000, Dan Scally wrote:

>> Hi Andy

>>

>> On 30/11/2020 18:23, Andy Shevchenko wrote:

>>> On Mon, Nov 30, 2020 at 01:31:25PM +0000, Daniel Scally wrote:

>>>> ACPI devices declare themselves dependent on other devices via the _DEP

>>>> buffer. Fetching the dependee from dependent is a matter of parsing

>>>> _DEP, but currently there's no method to fetch dependent from dependee.

>>>> Add one, so we can parse sensors dependent on a PMIC from the PMIC's

>>>> acpi_driver.

>>> Do I understand correctly that it's an existing table provided by firmware that

>>> (ab)uses _DEP in such way? Note, the specification doesn't tell we may use it

>>> in this way, OTOH I don't remember if it strictly forbids such use.

>>>

>>> So, please elaborate in the commit message why you need this and pint out to

>>> the 6.5.8 "_DEP (Operation Region Dependencies)" which clearly says about

>>> OpRegions and that part already supported by ACPI in the Linux, if I'm not

>>> mistaken, need to refresh my memory.

>>

>> Laurent's reply is good explanation, but for example see my Lenovo Miix

>> 510's DSDT:

>>

>>

>> https://gist.githubusercontent.com/djrscally/e64d112180517352fa3392878b0f4a7d/raw/88b90b3ea4204fd7845257b6666fdade47cc2981/dsdt.dsl

>>

>>

>> Search OVTI2680 and OVTI5648 for the cameras. Both are dependent on

>> IN3472 devices (PMI0 and PMI1) which are the discrete type that we're

>> attempting to handle here.

> Yes, it seems since PMIC is kinda "power resource" (don't mix with real power

> resource as by ACPI specifications) and that's why they decided to include it

> into _DEP.  So, it seems a de facto common practice. Thus, it would be nice to

> have the above in the commit message in some form. Can you do it?

>

Sure, no problem. I'll include that in the next version
diff mbox series

Patch

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index c177165c8db2..7099529121db 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -807,6 +807,52 @@  static int acpi_dev_match_cb(struct device *dev, const void *data)
 	return hrv == match->hrv;
 }
 
+static int acpi_dev_match_by_dep(struct device *dev, const void *data)
+{
+	struct acpi_device *adev = to_acpi_device(dev);
+	const struct acpi_device *dependee = data;
+	struct acpi_handle_list dep_handles;
+	struct acpi_device *candidate;
+	acpi_handle handle;
+	acpi_status status;
+	unsigned int i;
+	int ret;
+
+	handle = adev->handle;
+
+	if (!acpi_has_method(handle, "_DEP"))
+		return 0;
+
+	status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_handles);
+	if (ACPI_FAILURE(status))
+		return 0;
+
+	for (i = 0; i < dep_handles.count; i++) {
+		struct acpi_device_info *info;
+
+		status = acpi_get_object_info(dep_handles.handles[i], &info);
+		if (ACPI_FAILURE(status))
+			continue;
+
+		if (info->valid & ACPI_VALID_HID) {
+			ret = acpi_bus_get_device(dep_handles.handles[i], &candidate);
+			if (ret || !candidate) {
+				kfree(info);
+				continue;
+			}
+
+			if (candidate == dependee) {
+				acpi_dev_put(candidate);
+				kfree(info);
+				return 1;
+			}
+
+			kfree(info);
+		}
+	}
+	return 0;
+}
+
 /**
  * acpi_dev_present - Detect that a given ACPI device is present
  * @hid: Hardware ID of the device.
@@ -842,6 +888,28 @@  bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
 }
 EXPORT_SYMBOL(acpi_dev_present);
 
+/**
+ * acpi_dev_get_next_dep_dev - Return next ACPI device dependent on input dev
+ * @adev: Pointer to the dependee device
+ * @prev: Pointer to the previous dependent device (or NULL for first match)
+ *
+ * Return the next ACPI device which declares itself dependent on @adev in
+ * the _DEP buffer.
+ *
+ * The caller is responsible to call put_device() on the returned device.
+ */
+struct acpi_device *acpi_dev_get_next_dep_dev(struct acpi_device *adev,
+					      struct acpi_device *prev)
+{
+	struct device *start = prev ? &prev->dev : NULL;
+	struct device *dev;
+
+	dev = bus_find_device(&acpi_bus_type, start, adev, acpi_dev_match_by_dep);
+
+	return dev ? to_acpi_device(dev) : NULL;
+}
+EXPORT_SYMBOL(acpi_dev_get_next_dep_dev);
+
 /**
  * acpi_dev_get_next_match_dev - Return the next match of ACPI device
  * @adev: Pointer to the previous acpi_device matching this hid, uid and hrv
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 0a028ba967d3..f5dfeb030b9c 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -688,6 +688,8 @@  static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
 
 bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2);
 
+struct acpi_device *
+acpi_dev_get_next_dep_dev(struct acpi_device *adev, struct acpi_device *prev);
 struct acpi_device *
 acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
 struct acpi_device *