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

andriy.shevchenko@linux.intel.com 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()?
Laurent Pinchart Nov. 30, 2020, 6:40 p.m. UTC | #2
Hi Andy,

On Mon, Nov 30, 2020 at 08:23:54PM +0200, 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.

The ACPI "bindings" (I come from the DT world, is there a standard term
to describe this in ACPI ?) for the camera in Windows-based Kaby Lake
machines could be used as textbook examples of how to abuse ACPI, in
many different ways :-) I'm sure that applies to ACPI in general
though...

Depending on the device, camera sensors are controlled by a PMIC that
provides regulators, clocks and GPIOs (for the reset and power down
signals), or directly by GPIOs that control discrete regulators and
sensor signals. In the first case an INT3472 device models the
regulator, which can be a TI TPS68470 or a uPI Semi uP6641Q (two
completely different devices with a single HID...). The device model is
specified in the CLDB, a custom data table for INT3472.

In the latter case, Intel has created ACPI bindings for a "discrete
PMIC". It uses an ACPI device object with HID set to INT3472 as well,
also with a CLDB whose type field indicate the PMIC is "discrete". The
ACPI device is only used to reference up to 4 GPIOs (provided by the
Kaby Lake GPIO controller, the LPSS) in the _CRS. There's also a _DSM
that reports, for each GPIO, its function. All this information should
have been stored in the camera sensor ACPI device object, but that would
have been too simple...

In both cases, the PMIC device object is referenced by the _DEP data. We
need to access it to dig up the GPIOs, look up their type, and register
fixed regulators, supply mappings and GPIO mappings for the sensor.

> 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()?
Daniel Scally Nov. 30, 2020, 11:54 p.m. UTC | #3
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.

>
> ...
>
>> +	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()?
oooh, yes, I think I can. Thank you!
andriy.shevchenko@linux.intel.com Dec. 1, 2020, 3:10 p.m. UTC | #4
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 | #5
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 *