| 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 |
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()?
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()?
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!
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?
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 --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 *
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(+)