Re: [PATCH] platform/x86: amd: Add ISP platform info
From: Armin Wolf
Date: Tue Mar 04 2025 - 20:11:57 EST
Am 04.03.25 um 18:44 schrieb Nirujogi, Pratap:
Hi Armin,
Thanks for your comment.
Thanks,
Pratap
On 3/3/2025 6:56 PM, Armin Wolf wrote:
Caution: This message originated from an External Source. Use proper
caution when opening attachments, clicking links, or responding.
Am 04.03.25 um 00:14 schrieb Nirujogi, Pratap:
Hi Hans,
Thanks for your review. Please see the inline comments and let us know
your insights.
Thanks,
Pratap
On 3/3/2025 8:41 AM, Hans de Goede wrote:
Caution: This message originated from an External Source. Use proper
caution when opening attachments, clicking links, or responding.
Hi Pratap,
Thank you for your patch.
On 28-Feb-25 18:02, Pratap Nirujogi wrote:
Add ov05c i2c boardinfo and GPIO pin info for AMD ISP platform.
Details of the resources added:
- Added i2c bus number for AMD ISP platform is 99.
- Added GPIO 85 to allow ISP driver to enable and disable ISP access.
- Added GPIO 0 to allow sensor driver to enable and disable sensor
module.
Signed-off-by: Pratap Nirujogi <pratap.nirujogi@xxxxxxx>
---
drivers/platform/x86/amd/Kconfig | 11 +++++
drivers/platform/x86/amd/Makefile | 1 +
drivers/platform/x86/amd/amd_isp.c | 72
++++++++++++++++++++++++++++++
3 files changed, 84 insertions(+)
create mode 100644 drivers/platform/x86/amd/amd_isp.c
diff --git a/drivers/platform/x86/amd/Kconfig
b/drivers/platform/x86/amd/Kconfig
index c3e086ea64fc..4b373edd750d 100644
--- a/drivers/platform/x86/amd/Kconfig
+++ b/drivers/platform/x86/amd/Kconfig
@@ -32,3 +32,14 @@ config AMD_WBRF
This mechanism will only be activated on platforms that
advertise a
need for it.
+
+config AMD_ISP_PLATFORM
+ bool "AMD platform with ISP4 that supports Camera sensor
device"
+ depends on I2C && X86_64 && AMD_ISP4
+ help
+ For AMD platform that support Image signal processor
generation 4, it
+ is necessary to add platform specific camera sensor module
board info
+ which includes the sensor driver device id and the i2c
address.
+
+ If you have a AMD platform that support ISP4 and with a
sensor
+ connected to it, say Y here
diff --git a/drivers/platform/x86/amd/Makefile
b/drivers/platform/x86/amd/Makefile
index 56f62fc9c97b..0d89e2d4f7e6 100644
--- a/drivers/platform/x86/amd/Makefile
+++ b/drivers/platform/x86/amd/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_AMD_PMC) += pmc/
obj-$(CONFIG_AMD_HSMP) += hsmp/
obj-$(CONFIG_AMD_PMF) += pmf/
obj-$(CONFIG_AMD_WBRF) += wbrf.o
+obj-$(CONFIG_AMD_ISP_PLATFORM) += amd_isp.o
diff --git a/drivers/platform/x86/amd/amd_isp.c
b/drivers/platform/x86/amd/amd_isp.c
new file mode 100644
index 000000000000..751f209e9509
--- /dev/null
+++ b/drivers/platform/x86/amd/amd_isp.c
@@ -0,0 +1,72 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright 2025 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person
obtaining a
+ * copy of this software and associated documentation files (the
"Software"),
+ * to deal in the Software without restriction, including without
limitation
+ * the rights to use, copy, modify, merge, publish, distribute,
sublicense,
+ * and/or sell copies of the Software, and to permit persons to
whom the
+ * Software is furnished to do so, subject to the following
conditions:
+ *
+ * The above copyright notice and this permission notice shall be
included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,
DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/gpio/machine.h>
+
+#define AMDISP_I2C_BUS 99
I'm not a fan of using static i2c-bus numbers for this. static bus
numbers are
something of the past and we typically do not use these on x86
anymore.
Using this static number + i2c_register_board_info() also requires
this code
to be builtin rather then modular which is also undesirable.
For a more dynamic way of manually adding i2c-devices see:
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/
linux.git/tree/drivers/platform/x86/dell/dell-lis3lv02d.c
But a better question here is why instantiate the sensor i2c device
manually at all.
ACPI has a standardized way to describe I2C-clients which tyically
is used for all I2C devices on ACPI platforms like I2C touchscreens /
touchpads / audio-codecs / accelerometers / etc.
I don't see why the camera sensor on AMD platforms is so special that
it could not be described in ACPI using an ACPI child-device of the
i2c-controller with a ACPI resource (_CRS entry) of the
I2cSerialBusV2()
type.
Likewise the sensor enable GPIO should also be described in the ACPI
table as a Gpio type resource in the same _CRS table.
We have to take this approach because ISP is a child to GFX PCI device
in AMD HW architectures, and since it is not an independent device,
its device specific configuration (gpio pin ids, i2c-bus number etc.)
is not registered in ACPI.
Can you run acpidump -o acpidump.txt on a laptop with this camera
sensor and send me the acpidupm.txt offlist ? Please run this
on a production hardware laptop model using production firmware.
Please refer the attached acpidump.txt
I suspect that Windows will also be using the ACPI description
for the sensor so we really should figure out what Windows is doing
here.
Yes, same ACPI configuration for both Windows and Linux. Similar
approach followed even on windows to control the isp gpio pins.
The OMNI5C10 ACPI device has a _DSM method that supports the GUID
f8fd3bff-21b7-4a99-bdc8-c414a3e9453c. Do you know
more about the purpose of this method?
Thanks,
Armin Wolf
This GUID is windows specific. Sorry, I'm not completely sure on what
exactly it is used for. I'm thinking it is used to distinguish the
different sensor modules that OEMs supports on different SKUs. I confirm
its not applicable for Linux.
Ok, thanks for checking :)
As Mario mentioned we cannot just assume that the GPIOs +
sensor address and model are valid for all laptops. Ideally we should
be getting this information from ACPI rather then hardcoding it
in the kernel.
Yes, we initially assumed CONFIG_AMD_ISP_PLATFORM=y will be set only
on the intended platforms, but as that assumption is not valid, the
below
check is added in v2 patch checking the specific ov05c acpi hw id to
present before running the driver.
/* check for valid platform before configuring isp4 board resources */
if (!acpi_dev_found(AMDISP_ACPI_CAM_HID))
return -ENODEV;
+
+static struct gpiod_lookup_table isp_gpio_table = {
+ .dev_id = "amd_isp_capture",
+ .table = {
+ GPIO_LOOKUP("AMDI0030:00", 85, "enable_isp",
GPIO_ACTIVE_HIGH),
+ { }
+ },
+};
This too really should be an Gpio() type ACPI resource on the ACPI
device
node for the ISP.
How/where is this "amd_isp_capture" device created ?
"amd_isp_capture" is the V4L2 ISP driver in this case. The patches for
ISP driver are yet to be submitted. It will be loaded during AMDGPU
device probe on the AMD platforms supporting isp4.2 HW. AMDGPU
reference to trigger the isp device probe:
https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/amd/
amdgpu/isp_v4_1_1.c#L108
Regards,
Hans
+
+static struct gpiod_lookup_table isp_sensor_gpio_table = {
+ .dev_id = "ov05c",
+ .table = {
+ GPIO_LOOKUP("amdisp-pinctrl", 0, "sensor0_enable",
GPIO_ACTIVE_HIGH),
+ { }
+ },
+};
+
+static struct i2c_board_info sensor_info = {
+ .dev_name = "ov05c",
+ I2C_BOARD_INFO("ov05c", 0x10),
+};
+
+static int __init amd_isp_init(void)
+{
+ int ret;
+
+ gpiod_add_lookup_table(&isp_gpio_table);
+ gpiod_add_lookup_table(&isp_sensor_gpio_table);
+
+ ret = i2c_register_board_info(AMDISP_I2C_BUS, &sensor_info, 1);
+ if (ret)
+ pr_err("%s: cannot register i2c board devices:%s",
+ __func__, sensor_info.dev_name);
+
+ return ret;
+}
+
+arch_initcall(amd_isp_init);
+
+MODULE_AUTHOR("Benjamin Chan <benjamin.chan@xxxxxxx>");
+MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@xxxxxxx>");
+MODULE_DESCRIPTION("AMD ISP Platform parameters");
+MODULE_LICENSE("GPL and additional rights");