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

[Laurent Pinchart]

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()?

-- 
Regards,

Laurent Pinchart