From: Hans de Goede <hdegoede@redhat.com>
To: "Rafael J . Wysocki" <rjw@rjwysocki.net>,
Mark Gross <markgross@kernel.org>,
Andy Shevchenko <andy@infradead.org>,
Daniel Scally <djrscally@gmail.com>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Liam Girdwood <lgirdwood@gmail.com>,
Mark Brown <broonie@kernel.org>,
Michael Turquette <mturquette@baylibre.com>,
Stephen Boyd <sboyd@kernel.org>
Cc: Len Brown <lenb@kernel.org>,
linux-acpi@vger.kernel.org, platform-driver-x86@vger.kernel.org,
linux-kernel@vger.kernel.org,
Sakari Ailus <sakari.ailus@linux.intel.com>,
Kate Hsuan <hpa@redhat.com>,
linux-media@vger.kernel.org, linux-clk@vger.kernel.org
Subject: Re: [PATCH v2 02/13] ACPI: honor dependencies for devices with a _DEP pointing to an INT3472 device
Date: Sun, 10 Oct 2021 17:10:47 +0200 [thread overview]
Message-ID: <d90f8514-a779-06ad-5e8d-5c42a5771b15@redhat.com> (raw)
In-Reply-To: <20211009160548.306550-3-hdegoede@redhat.com>
Hi,
On 10/9/21 6:05 PM, Hans de Goede wrote:
> The clk and regulator frameworks expect clk/regulator consumer-devices
> to have info about the consumed clks/regulators described in the device's
> fw_node.
>
> To work around cases where this info is not present in the firmware tables,
> which is often the case on x86/ACPI devices, both frameworks allow the
> provider-driver to attach info about consumers to the clks/regulators
> when registering these.
>
> This causes problems with the probe ordering wrt drivers for consumers
> of these clks/regulators. Since the lookups are only registered when the
> provider-driver binds, trying to get these clks/regulators before then
> results in a -ENOENT error for clks and a dummy regulator for regulators.
>
> One case where we hit this issue is camera sensors such as e.g. the OV8865
> sensor found on the Microsoft Surface Go. The sensor uses clks, regulators
> and GPIOs provided by a TPS68470 PMIC which is described in an INT3472
> ACPI device. There is special platform code handling this and setting
> platform_data with the necessary consumer info on the MFD cells
> instantiated for the PMIC under: drivers/platform/x86/intel/int3472.
>
> For this to work properly the ov8865 driver must not bind to the i2c-client
> for the OV8865 sensor until after the TPS68470 PMIC gpio, regulator and
> clk MFD cells have all been fully setup.
>
> The OV8865 on the Microsoft Surface Go is just one example, all X86
> devices using the Intel IPU3 camera block found on recent Intel SoCs
> have similar issues where there is an INT3472 HID ACPI-device which
> describes the clks and regulators and the driver for this INT3472 device
> must be fully initialized before the sensor driver (any sensor driver)
> binds for things to work properly.
>
> On these devices the ACPI nodes describing the sensors all have a _DEP
> dependency on the matching INT3472 ACPI device (there is one per sensor).
>
> This allows solving the probe-ordering problem by making ACPI-devices
> honor the _DEP dependencies (like we already do for batteries) when they
> have a dependency on an INT3472 device.
>
> Note the matching is done on there being a _DEP on an INT3472 device,
> rather then matching on the HID of the sensor device itself to avoid
> having to maintain an ever growing list of HIDs of sensors which need
> the honor_dep behavior.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
In the end I was not entirely happy with the approach in patch 1 + 2
(this patch) myself, reporting 0 for the status of some devices until
the _DEP-s are met has some troublesome implications for code which
behavior depends on acpi_dev_is_present() calls (or equivalent code).
Which led to the: "[PATCH v2 13/13] media: ipu3-cio2: Add module
soft-deps for the INT3472 drivers" adding unnecessary soft
module-dependencies to the ipu3-cio2 code to make sure that the
status was not reported as 0 because of unmet deps when that code
runs.
Besides these soft-deps being non-sense / undesirable. This also
is a bit racy since the status only gets updated to reflect the
unmet_deps==0 once the acpi_scan_clear_dep() workqueue work
has run and there is no guarantee the work has fully run
once the modprobe-s of the INT3472 are done (it should run
soon afterwards, but there is a race there).
So I'm working on a rewrite of patch 1 + 2 now which replaces
the approach where we "lie" about the status with deferring the
enumeration step (1) until all the _DEP-s are met (for select
devices).
Expect a v3 with this new approach soon...
Regards,
Hans
1) The step where a platform_device or e.g. i2c-client is
instantiated as physical_device_node for the ACPI-device
> ---
> drivers/acpi/scan.c | 16 ++++++++++++++--
> include/acpi/acpi_bus.h | 1 +
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 4e0a946b35ed..976724540197 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -796,6 +796,12 @@ static const char * const acpi_ignore_dep_ids[] = {
> NULL
> };
>
> +/* List of HIDs for which we honor deps of matching ACPI devs, when checking _DEP lists. */
> +static const char * const acpi_honor_dep_ids[] = {
> + "INT3472", /* Camera sensor PMIC / clk and regulator info */
> + NULL
> +};
> +
> static struct acpi_device *acpi_bus_get_parent(acpi_handle handle)
> {
> struct acpi_device *device = NULL;
> @@ -1761,8 +1767,12 @@ static void acpi_scan_dep_init(struct acpi_device *adev)
> adev->honor_deps = true;
>
> list_for_each_entry(dep, &acpi_dep_list, node) {
> - if (dep->consumer == adev->handle)
> + if (dep->consumer == adev->handle) {
> + if (dep->honor_dep)
> + adev->honor_deps = true;
> +
> adev->dep_unmet++;
> + }
> }
> }
>
> @@ -1966,7 +1976,7 @@ static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep)
> for (count = 0, i = 0; i < dep_devices.count; i++) {
> struct acpi_device_info *info;
> struct acpi_dep_data *dep;
> - bool skip;
> + bool skip, honor_dep;
>
> status = acpi_get_object_info(dep_devices.handles[i], &info);
> if (ACPI_FAILURE(status)) {
> @@ -1975,6 +1985,7 @@ static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep)
> }
>
> skip = acpi_info_matches_ids(info, acpi_ignore_dep_ids);
> + honor_dep = acpi_info_matches_ids(info, acpi_honor_dep_ids);
> kfree(info);
>
> if (skip)
> @@ -1988,6 +1999,7 @@ static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep)
>
> dep->supplier = dep_devices.handles[i];
> dep->consumer = handle;
> + dep->honor_dep = honor_dep;
>
> mutex_lock(&acpi_dep_list_lock);
> list_add_tail(&dep->node , &acpi_dep_list);
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 0ba344a5f4f8..b6fb050e77bb 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -284,6 +284,7 @@ struct acpi_dep_data {
> struct list_head node;
> acpi_handle supplier;
> acpi_handle consumer;
> + bool honor_dep;
> };
>
> /* Performance Management */
>
next prev parent reply other threads:[~2021-10-10 15:10 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-09 16:05 [PATCH v2 00/13] Add support for X86/ACPI camera sensor/PMIC setup with clk and regulator platform data Hans de Goede
2021-10-09 16:05 ` [PATCH v2 01/13] ACPI: Add a honor_deps flag to struct acpi_device Hans de Goede
2021-10-09 16:05 ` [PATCH v2 02/13] ACPI: honor dependencies for devices with a _DEP pointing to an INT3472 device Hans de Goede
2021-10-10 15:10 ` Hans de Goede [this message]
2021-10-09 16:05 ` [PATCH v2 03/13] platform_data: Add linux/platform_data/tps68470.h file Hans de Goede
2021-10-09 16:05 ` [PATCH v2 04/13] regulator: Introduce tps68470-regulator driver Hans de Goede
2021-10-11 11:02 ` Mark Brown
2021-10-09 16:05 ` [PATCH v2 05/13] clk: Introduce clk-tps68470 driver Hans de Goede
2021-10-09 16:05 ` [PATCH v2 06/13] platform/x86: int3472: Enable I2c daisy chain Hans de Goede
2021-10-09 16:05 ` [PATCH v2 07/13] platform/x86: int3472: Split into 2 drivers Hans de Goede
2021-10-09 16:05 ` [PATCH v2 08/13] platform/x86: int3472: Add get_sensor_adev_and_name() helper Hans de Goede
2021-10-09 16:05 ` [PATCH v2 09/13] platform/x86: int3472: Pass tps68470_clk_platform_data to the tps68470-regulator MFD-cell Hans de Goede
2021-10-09 16:05 ` [PATCH v2 10/13] platform/x86: int3472: Pass tps68470_regulator_platform_data " Hans de Goede
2021-10-09 16:05 ` [PATCH v2 11/13] platform/x86: int3472: Deal with probe ordering issues Hans de Goede
2021-10-09 16:05 ` [PATCH v2 12/13] media: ipu3-cio2: Add INT347A to cio2-bridge Hans de Goede
2021-10-09 16:05 ` [PATCH v2 13/13] media: ipu3-cio2: Add module soft-deps for the INT3472 drivers Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d90f8514-a779-06ad-5e8d-5c42a5771b15@redhat.com \
--to=hdegoede@redhat.com \
--cc=andy@infradead.org \
--cc=broonie@kernel.org \
--cc=djrscally@gmail.com \
--cc=hpa@redhat.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=lenb@kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=markgross@kernel.org \
--cc=mchehab@kernel.org \
--cc=mturquette@baylibre.com \
--cc=platform-driver-x86@vger.kernel.org \
--cc=rjw@rjwysocki.net \
--cc=sakari.ailus@linux.intel.com \
--cc=sboyd@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).