drivers/platform/x86/amd/Kconfig | 11 ++ drivers/platform/x86/amd/Makefile | 1 + drivers/platform/x86/amd/amd_isp4.c | 269 ++++++++++++++++++++++++++++ 3 files changed, 281 insertions(+) create mode 100644 drivers/platform/x86/amd/amd_isp4.c
ISP device specific configuration is not available in ACPI. Add
swnode graph to configure the missing device properties for the
OV05C10 camera device supported on amdisp platform.
Add support to create i2c-client dynamically when amdisp i2c
adapter is available.
Co-developed-by: Benjamin Chan <benjamin.chan@amd.com>
Signed-off-by: Benjamin Chan <benjamin.chan@amd.com>
Signed-off-by: Pratap Nirujogi <pratap.nirujogi@amd.com>
---
Changes v6 -> v7:
* Use devm_kzalloc() inplace of kmemdup()
* Use IS_ERR() inplace of i2c_client_has_driver()
* Remove the extra cast
drivers/platform/x86/amd/Kconfig | 11 ++
drivers/platform/x86/amd/Makefile | 1 +
drivers/platform/x86/amd/amd_isp4.c | 269 ++++++++++++++++++++++++++++
3 files changed, 281 insertions(+)
create mode 100644 drivers/platform/x86/amd/amd_isp4.c
diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/amd/Kconfig
index c3e086ea64fc..ec755b5fc93c 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
+ tristate "AMD ISP4 platform driver"
+ depends on I2C && X86_64 && ACPI && AMD_ISP4
+ help
+ Platform driver for AMD platforms containing image signal processor
+ gen 4. Provides camera sensor module board information to allow
+ sensor and V4L drivers to work properly.
+
+ This driver can also be built as a module. If so, the module
+ will be called amd_isp4.
diff --git a/drivers/platform/x86/amd/Makefile b/drivers/platform/x86/amd/Makefile
index c6c40bdcbded..b0e284b5d497 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_isp4.o
diff --git a/drivers/platform/x86/amd/amd_isp4.c b/drivers/platform/x86/amd/amd_isp4.c
new file mode 100644
index 000000000000..461a10be5ccd
--- /dev/null
+++ b/drivers/platform/x86/amd/amd_isp4.c
@@ -0,0 +1,269 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * AMD ISP platform driver for sensor i2-client instantiation
+ *
+ * Copyright 2025 Advanced Micro Devices, Inc.
+ */
+
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/units.h>
+
+#define AMDISP_OV05C10_I2C_ADDR 0x10
+#define AMDISP_OV05C10_PLAT_NAME "amdisp_ov05c10_platform"
+#define AMDISP_OV05C10_HID "OMNI5C10"
+#define AMDISP_OV05C10_REMOTE_EP_NAME "ov05c10_isp_4_1_1"
+#define AMD_ISP_PLAT_DRV_NAME "amd-isp4"
+
+/*
+ * AMD ISP platform definition to configure the device properties
+ * missing in the ACPI table.
+ */
+struct amdisp_platform {
+ const char *name;
+ u8 i2c_addr;
+ u8 max_num_swnodes;
+ struct i2c_board_info board_info;
+ struct notifier_block i2c_nb;
+ struct i2c_client *i2c_dev;
+ const struct software_node **swnodes;
+};
+
+/* Top-level OV05C10 camera node property table */
+static const struct property_entry ov05c10_camera_props[] = {
+ PROPERTY_ENTRY_U32("clock-frequency", 24 * HZ_PER_MHZ),
+ { }
+};
+
+/* Root AMD ISP OV05C10 camera node definition */
+static const struct software_node camera_node = {
+ .name = AMDISP_OV05C10_HID,
+ .properties = ov05c10_camera_props,
+};
+
+/*
+ * AMD ISP OV05C10 Ports node definition. No properties defined for
+ * ports node for OV05C10.
+ */
+static const struct software_node ports = {
+ .name = "ports",
+ .parent = &camera_node,
+};
+
+/*
+ * AMD ISP OV05C10 Port node definition. No properties defined for
+ * port node for OV05C10.
+ */
+static const struct software_node port_node = {
+ .name = "port@",
+ .parent = &ports,
+};
+
+/*
+ * Remote endpoint AMD ISP node definition. No properties defined for
+ * remote endpoint node for OV05C10.
+ */
+static const struct software_node remote_ep_isp_node = {
+ .name = AMDISP_OV05C10_REMOTE_EP_NAME,
+};
+
+/*
+ * Remote endpoint reference for isp node included in the
+ * OV05C10 endpoint.
+ */
+static const struct software_node_ref_args ov05c10_refs[] = {
+ SOFTWARE_NODE_REFERENCE(&remote_ep_isp_node),
+};
+
+/* OV05C supports one single link frequency */
+static const u64 ov05c10_link_freqs[] = {
+ 925 * HZ_PER_MHZ,
+};
+
+/* OV05C supports only 2-lane configuration */
+static const u32 ov05c10_data_lanes[] = {
+ 1,
+ 2,
+};
+
+/* OV05C10 endpoint node properties table */
+static const struct property_entry ov05c10_endpoint_props[] = {
+ PROPERTY_ENTRY_U32("bus-type", 4),
+ PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes", ov05c10_data_lanes,
+ ARRAY_SIZE(ov05c10_data_lanes)),
+ PROPERTY_ENTRY_U64_ARRAY_LEN("link-frequencies", ov05c10_link_freqs,
+ ARRAY_SIZE(ov05c10_link_freqs)),
+ PROPERTY_ENTRY_REF_ARRAY("remote-endpoint", ov05c10_refs),
+ { }
+};
+
+/* AMD ISP endpoint node definition */
+static const struct software_node endpoint_node = {
+ .name = "endpoint",
+ .parent = &port_node,
+ .properties = ov05c10_endpoint_props,
+};
+
+/*
+ * AMD ISP swnode graph uses 5 nodes and also its relationship is
+ * fixed to align with the structure that v4l2 expects for successful
+ * endpoint fwnode parsing.
+ *
+ * It is only the node property_entries that will vary for each platform
+ * supporting different sensor modules.
+ */
+#define NUM_SW_NODES 5
+
+static const struct software_node *ov05c10_nodes[NUM_SW_NODES + 1] = {
+ &camera_node,
+ &ports,
+ &port_node,
+ &endpoint_node,
+ &remote_ep_isp_node,
+ NULL
+};
+
+/* OV05C10 specific AMD ISP platform configuration */
+static const struct amdisp_platform amdisp_ov05c10_platform_config = {
+ .name = AMDISP_OV05C10_PLAT_NAME,
+ .board_info = {
+ .dev_name = "ov05c10",
+ I2C_BOARD_INFO("ov05c10", AMDISP_OV05C10_I2C_ADDR),
+ },
+ .i2c_addr = AMDISP_OV05C10_I2C_ADDR,
+ .max_num_swnodes = NUM_SW_NODES,
+ .swnodes = ov05c10_nodes,
+};
+
+static const struct acpi_device_id amdisp_sensor_ids[] = {
+ { AMDISP_OV05C10_HID },
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, amdisp_sensor_ids);
+
+static inline bool is_isp_i2c_adapter(struct i2c_adapter *adap)
+{
+ return !strcmp(adap->owner->name, "i2c_designware_amdisp");
+}
+
+static void instantiate_isp_i2c_client(struct amdisp_platform *ov05c10, struct i2c_adapter *adap)
+{
+ struct i2c_board_info *info = &ov05c10->board_info;
+ struct i2c_client *i2c_dev;
+
+ if (ov05c10->i2c_dev)
+ return;
+
+ if (!info->addr) {
+ dev_err(&adap->dev, "invalid i2c_addr 0x%x detected\n", ov05c10->i2c_addr);
+ return;
+ }
+
+ i2c_dev = i2c_new_client_device(adap, info);
+ if (IS_ERR(i2c_dev)) {
+ dev_err(&adap->dev, "error %pe registering isp i2c_client\n", i2c_dev);
+ return;
+ }
+ ov05c10->i2c_dev = i2c_dev;
+}
+
+static int isp_i2c_bus_notify(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct amdisp_platform *ov05c10 = container_of(nb, struct amdisp_platform, i2c_nb);
+ struct device *dev = data;
+ struct i2c_client *client;
+ struct i2c_adapter *adap;
+
+ switch (action) {
+ case BUS_NOTIFY_ADD_DEVICE:
+ adap = i2c_verify_adapter(dev);
+ if (!adap)
+ break;
+ if (is_isp_i2c_adapter(adap))
+ instantiate_isp_i2c_client(ov05c10, adap);
+ break;
+ case BUS_NOTIFY_REMOVED_DEVICE:
+ client = i2c_verify_client(dev);
+ if (!client)
+ break;
+ if (ov05c10->i2c_dev == client) {
+ dev_dbg(&client->adapter->dev, "amdisp i2c_client removed\n");
+ ov05c10->i2c_dev = NULL;
+ }
+ break;
+ default:
+ break;
+ }
+
+ return NOTIFY_DONE;
+}
+
+static struct amdisp_platform *prepare_amdisp_platform(struct device *dev,
+ const struct amdisp_platform *src)
+{
+ struct amdisp_platform *isp_ov05c10;
+ struct i2c_board_info *info;
+ int ret;
+
+ isp_ov05c10 = devm_kzalloc(dev, sizeof(*isp_ov05c10), GFP_KERNEL);
+ if (!isp_ov05c10)
+ return ERR_PTR(-ENOMEM);
+ memcpy(isp_ov05c10, src, sizeof(*isp_ov05c10));
+
+ info = &isp_ov05c10->board_info;
+
+ ret = software_node_register_node_group(src->swnodes);
+ if (ret)
+ return ERR_PTR(ret);
+
+ info->swnode = src->swnodes[0];
+
+ return isp_ov05c10;
+}
+
+static int amd_isp_probe(struct platform_device *pdev)
+{
+ struct amdisp_platform *ov05c10;
+ int ret;
+
+ ov05c10 = prepare_amdisp_platform(&pdev->dev, &amdisp_ov05c10_platform_config);
+ if (IS_ERR(ov05c10))
+ return dev_err_probe(&pdev->dev, PTR_ERR(ov05c10),
+ "failed to prepare AMD ISP platform fwnode\n");
+
+ ov05c10->i2c_nb.notifier_call = isp_i2c_bus_notify;
+ ret = bus_register_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
+ if (ret)
+ return ret;
+
+ platform_set_drvdata(pdev, ov05c10);
+ return 0;
+}
+
+static void amd_isp_remove(struct platform_device *pdev)
+{
+ struct amdisp_platform *ov05c10 = platform_get_drvdata(pdev);
+
+ bus_unregister_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
+ i2c_unregister_device(ov05c10->i2c_dev);
+ software_node_unregister_node_group(ov05c10->swnodes);
+}
+
+static struct platform_driver amd_isp_platform_driver = {
+ .driver = {
+ .name = AMD_ISP_PLAT_DRV_NAME,
+ .acpi_match_table = amdisp_sensor_ids,
+ },
+ .probe = amd_isp_probe,
+ .remove = amd_isp_remove,
+};
+
+module_platform_driver(amd_isp_platform_driver);
+
+MODULE_AUTHOR("Benjamin Chan <benjamin.chan@amd.com>");
+MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@amd.com>");
+MODULE_DESCRIPTION("AMD ISP4 Platform Driver");
+MODULE_LICENSE("GPL");
--
2.43.0Am 17.04.25 um 20:28 schrieb Pratap Nirujogi:
> ISP device specific configuration is not available in ACPI. Add
> swnode graph to configure the missing device properties for the
> OV05C10 camera device supported on amdisp platform.
>
> Add support to create i2c-client dynamically when amdisp i2c
> adapter is available.
>
> Co-developed-by: Benjamin Chan <benjamin.chan@amd.com>
> Signed-off-by: Benjamin Chan <benjamin.chan@amd.com>
> Signed-off-by: Pratap Nirujogi <pratap.nirujogi@amd.com>
> ---
> Changes v6 -> v7:
>
> * Use devm_kzalloc() inplace of kmemdup()
> * Use IS_ERR() inplace of i2c_client_has_driver()
> * Remove the extra cast
>
> drivers/platform/x86/amd/Kconfig | 11 ++
> drivers/platform/x86/amd/Makefile | 1 +
> drivers/platform/x86/amd/amd_isp4.c | 269 ++++++++++++++++++++++++++++
> 3 files changed, 281 insertions(+)
> create mode 100644 drivers/platform/x86/amd/amd_isp4.c
>
> diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/amd/Kconfig
> index c3e086ea64fc..ec755b5fc93c 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
> + tristate "AMD ISP4 platform driver"
> + depends on I2C && X86_64 && ACPI && AMD_ISP4
Hi,
just a question: when will the CONFIG_AMD_ISP4 symbol be introduced?
> + help
> + Platform driver for AMD platforms containing image signal processor
> + gen 4. Provides camera sensor module board information to allow
> + sensor and V4L drivers to work properly.
> +
> + This driver can also be built as a module. If so, the module
> + will be called amd_isp4.
> diff --git a/drivers/platform/x86/amd/Makefile b/drivers/platform/x86/amd/Makefile
> index c6c40bdcbded..b0e284b5d497 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_isp4.o
> diff --git a/drivers/platform/x86/amd/amd_isp4.c b/drivers/platform/x86/amd/amd_isp4.c
> new file mode 100644
> index 000000000000..461a10be5ccd
> --- /dev/null
> +++ b/drivers/platform/x86/amd/amd_isp4.c
> @@ -0,0 +1,269 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AMD ISP platform driver for sensor i2-client instantiation
> + *
> + * Copyright 2025 Advanced Micro Devices, Inc.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/units.h>
> +
> +#define AMDISP_OV05C10_I2C_ADDR 0x10
> +#define AMDISP_OV05C10_PLAT_NAME "amdisp_ov05c10_platform"
> +#define AMDISP_OV05C10_HID "OMNI5C10"
> +#define AMDISP_OV05C10_REMOTE_EP_NAME "ov05c10_isp_4_1_1"
> +#define AMD_ISP_PLAT_DRV_NAME "amd-isp4"
> +
> +/*
> + * AMD ISP platform definition to configure the device properties
> + * missing in the ACPI table.
> + */
> +struct amdisp_platform {
> + const char *name;
> + u8 i2c_addr;
> + u8 max_num_swnodes;
> + struct i2c_board_info board_info;
> + struct notifier_block i2c_nb;
> + struct i2c_client *i2c_dev;
> + const struct software_node **swnodes;
> +};
> +
> +/* Top-level OV05C10 camera node property table */
> +static const struct property_entry ov05c10_camera_props[] = {
> + PROPERTY_ENTRY_U32("clock-frequency", 24 * HZ_PER_MHZ),
> + { }
> +};
> +
> +/* Root AMD ISP OV05C10 camera node definition */
> +static const struct software_node camera_node = {
> + .name = AMDISP_OV05C10_HID,
> + .properties = ov05c10_camera_props,
> +};
> +
> +/*
> + * AMD ISP OV05C10 Ports node definition. No properties defined for
> + * ports node for OV05C10.
> + */
> +static const struct software_node ports = {
> + .name = "ports",
> + .parent = &camera_node,
> +};
> +
> +/*
> + * AMD ISP OV05C10 Port node definition. No properties defined for
> + * port node for OV05C10.
> + */
> +static const struct software_node port_node = {
> + .name = "port@",
> + .parent = &ports,
> +};
> +
> +/*
> + * Remote endpoint AMD ISP node definition. No properties defined for
> + * remote endpoint node for OV05C10.
> + */
> +static const struct software_node remote_ep_isp_node = {
> + .name = AMDISP_OV05C10_REMOTE_EP_NAME,
> +};
> +
> +/*
> + * Remote endpoint reference for isp node included in the
> + * OV05C10 endpoint.
> + */
> +static const struct software_node_ref_args ov05c10_refs[] = {
> + SOFTWARE_NODE_REFERENCE(&remote_ep_isp_node),
> +};
> +
> +/* OV05C supports one single link frequency */
> +static const u64 ov05c10_link_freqs[] = {
> + 925 * HZ_PER_MHZ,
> +};
> +
> +/* OV05C supports only 2-lane configuration */
> +static const u32 ov05c10_data_lanes[] = {
> + 1,
> + 2,
> +};
> +
> +/* OV05C10 endpoint node properties table */
> +static const struct property_entry ov05c10_endpoint_props[] = {
> + PROPERTY_ENTRY_U32("bus-type", 4),
> + PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes", ov05c10_data_lanes,
> + ARRAY_SIZE(ov05c10_data_lanes)),
> + PROPERTY_ENTRY_U64_ARRAY_LEN("link-frequencies", ov05c10_link_freqs,
> + ARRAY_SIZE(ov05c10_link_freqs)),
> + PROPERTY_ENTRY_REF_ARRAY("remote-endpoint", ov05c10_refs),
> + { }
> +};
> +
> +/* AMD ISP endpoint node definition */
> +static const struct software_node endpoint_node = {
> + .name = "endpoint",
> + .parent = &port_node,
> + .properties = ov05c10_endpoint_props,
> +};
> +
> +/*
> + * AMD ISP swnode graph uses 5 nodes and also its relationship is
> + * fixed to align with the structure that v4l2 expects for successful
> + * endpoint fwnode parsing.
> + *
> + * It is only the node property_entries that will vary for each platform
> + * supporting different sensor modules.
> + */
> +#define NUM_SW_NODES 5
> +
> +static const struct software_node *ov05c10_nodes[NUM_SW_NODES + 1] = {
> + &camera_node,
> + &ports,
> + &port_node,
> + &endpoint_node,
> + &remote_ep_isp_node,
> + NULL
> +};
> +
> +/* OV05C10 specific AMD ISP platform configuration */
> +static const struct amdisp_platform amdisp_ov05c10_platform_config = {
> + .name = AMDISP_OV05C10_PLAT_NAME,
Where is this field being used?
> + .board_info = {
> + .dev_name = "ov05c10",
> + I2C_BOARD_INFO("ov05c10", AMDISP_OV05C10_I2C_ADDR),
> + },
> + .i2c_addr = AMDISP_OV05C10_I2C_ADDR,
Please reuse board_info->addr.
> + .max_num_swnodes = NUM_SW_NODES,
Where is max_num_swnodes being used?
> + .swnodes = ov05c10_nodes,
Why not drop .swnodes and referencing ov05c10_nodes directly?
> +};
> +
> +static const struct acpi_device_id amdisp_sensor_ids[] = {
> + { AMDISP_OV05C10_HID },
> + { }
> +};
> +MODULE_DEVICE_TABLE(acpi, amdisp_sensor_ids);
> +
> +static inline bool is_isp_i2c_adapter(struct i2c_adapter *adap)
> +{
> + return !strcmp(adap->owner->name, "i2c_designware_amdisp");
> +}
> +
> +static void instantiate_isp_i2c_client(struct amdisp_platform *ov05c10, struct i2c_adapter *adap)
> +{
> + struct i2c_board_info *info = &ov05c10->board_info;
> + struct i2c_client *i2c_dev;
> +
> + if (ov05c10->i2c_dev)
> + return;
> +
> + if (!info->addr) {
> + dev_err(&adap->dev, "invalid i2c_addr 0x%x detected\n", ov05c10->i2c_addr);
> + return;
> + }
> +
> + i2c_dev = i2c_new_client_device(adap, info);
> + if (IS_ERR(i2c_dev)) {
> + dev_err(&adap->dev, "error %pe registering isp i2c_client\n", i2c_dev);
> + return;
> + }
> + ov05c10->i2c_dev = i2c_dev;
> +}
> +
> +static int isp_i2c_bus_notify(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct amdisp_platform *ov05c10 = container_of(nb, struct amdisp_platform, i2c_nb);
> + struct device *dev = data;
> + struct i2c_client *client;
> + struct i2c_adapter *adap;
> +
> + switch (action) {
> + case BUS_NOTIFY_ADD_DEVICE:
> + adap = i2c_verify_adapter(dev);
> + if (!adap)
> + break;
> + if (is_isp_i2c_adapter(adap))
> + instantiate_isp_i2c_client(ov05c10, adap);
> + break;
> + case BUS_NOTIFY_REMOVED_DEVICE:
> + client = i2c_verify_client(dev);
> + if (!client)
> + break;
> + if (ov05c10->i2c_dev == client) {
> + dev_dbg(&client->adapter->dev, "amdisp i2c_client removed\n");
> + ov05c10->i2c_dev = NULL;
> + }
> + break;
> + default:
> + break;
> + }
> +
> + return NOTIFY_DONE;
You still need to handle the situation where the AMD I2C adapter is already registered when
registering the bus notifier. In this case you will miss the BUS_NOTIFY_ADD_DEVICE event.
> +}
> +
> +static struct amdisp_platform *prepare_amdisp_platform(struct device *dev,
> + const struct amdisp_platform *src)
> +{
> + struct amdisp_platform *isp_ov05c10;
> + struct i2c_board_info *info;
> + int ret;
> +
> + isp_ov05c10 = devm_kzalloc(dev, sizeof(*isp_ov05c10), GFP_KERNEL);
> + if (!isp_ov05c10)
> + return ERR_PTR(-ENOMEM);
> + memcpy(isp_ov05c10, src, sizeof(*isp_ov05c10));
This is not what i meant. I was complaining that amdisp_ov05c10_platform_config contains both
static data (swnodes) and data assigned during runtime (board_info->swnode, i2c_dev, ...).
Please do not use a global instance of struct amdisp_platform for initialization. Instead initialize a
fresh instance of this struct inside prepare_amdisp_platform().
> +
> + info = &isp_ov05c10->board_info;
> +
> + ret = software_node_register_node_group(src->swnodes);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + info->swnode = src->swnodes[0];
> +
> + return isp_ov05c10;
> +}
> +
> +static int amd_isp_probe(struct platform_device *pdev)
> +{
> + struct amdisp_platform *ov05c10;
> + int ret;
> +
> + ov05c10 = prepare_amdisp_platform(&pdev->dev, &amdisp_ov05c10_platform_config);
> + if (IS_ERR(ov05c10))
> + return dev_err_probe(&pdev->dev, PTR_ERR(ov05c10),
> + "failed to prepare AMD ISP platform fwnode\n");
> +
> + ov05c10->i2c_nb.notifier_call = isp_i2c_bus_notify;
> + ret = bus_register_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
> + if (ret)
> + return ret;
You need to call software_node_unregister_node_group() here when bus_register_notifier() fails.
Thanks,
Armin Wolf
> +
> + platform_set_drvdata(pdev, ov05c10);
> + return 0;
> +}
> +
> +static void amd_isp_remove(struct platform_device *pdev)
> +{
> + struct amdisp_platform *ov05c10 = platform_get_drvdata(pdev);
> +
> + bus_unregister_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
> + i2c_unregister_device(ov05c10->i2c_dev);
> + software_node_unregister_node_group(ov05c10->swnodes);
> +}
> +
> +static struct platform_driver amd_isp_platform_driver = {
> + .driver = {
> + .name = AMD_ISP_PLAT_DRV_NAME,
> + .acpi_match_table = amdisp_sensor_ids,
> + },
> + .probe = amd_isp_probe,
> + .remove = amd_isp_remove,
> +};
> +
> +module_platform_driver(amd_isp_platform_driver);
> +
> +MODULE_AUTHOR("Benjamin Chan <benjamin.chan@amd.com>");
> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@amd.com>");
> +MODULE_DESCRIPTION("AMD ISP4 Platform Driver");
> +MODULE_LICENSE("GPL");
Hi Armin,
On 4/26/2025 8:25 PM, Armin Wolf wrote:
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> Am 17.04.25 um 20:28 schrieb Pratap Nirujogi:
>
>> ISP device specific configuration is not available in ACPI. Add
>> swnode graph to configure the missing device properties for the
>> OV05C10 camera device supported on amdisp platform.
>>
>> Add support to create i2c-client dynamically when amdisp i2c
>> adapter is available.
>>
>> Co-developed-by: Benjamin Chan <benjamin.chan@amd.com>
>> Signed-off-by: Benjamin Chan <benjamin.chan@amd.com>
>> Signed-off-by: Pratap Nirujogi <pratap.nirujogi@amd.com>
>> ---
>> Changes v6 -> v7:
>>
>> * Use devm_kzalloc() inplace of kmemdup()
>> * Use IS_ERR() inplace of i2c_client_has_driver()
>> * Remove the extra cast
>>
>> drivers/platform/x86/amd/Kconfig | 11 ++
>> drivers/platform/x86/amd/Makefile | 1 +
>> drivers/platform/x86/amd/amd_isp4.c | 269 ++++++++++++++++++++++++++++
>> 3 files changed, 281 insertions(+)
>> create mode 100644 drivers/platform/x86/amd/amd_isp4.c
>>
>> diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/
>> amd/Kconfig
>> index c3e086ea64fc..ec755b5fc93c 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
>> + tristate "AMD ISP4 platform driver"
>> + depends on I2C && X86_64 && ACPI && AMD_ISP4
>
> Hi,
>
> just a question: when will the CONFIG_AMD_ISP4 symbol be introduced?
>
CONFIG_AMD_ISP4 will be introduced in the V4L2 ISP driver patches. We
are working on isp driver patches and planning to submit once the review
for x86/platform and sensor driver patches completes.
>> + help
>> + Platform driver for AMD platforms containing image signal
>> processor
>> + gen 4. Provides camera sensor module board information to allow
>> + sensor and V4L drivers to work properly.
>> +
>> + This driver can also be built as a module. If so, the module
>> + will be called amd_isp4.
>> diff --git a/drivers/platform/x86/amd/Makefile b/drivers/platform/x86/
>> amd/Makefile
>> index c6c40bdcbded..b0e284b5d497 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_isp4.o
>> diff --git a/drivers/platform/x86/amd/amd_isp4.c b/drivers/platform/
>> x86/amd/amd_isp4.c
>> new file mode 100644
>> index 000000000000..461a10be5ccd
>> --- /dev/null
>> +++ b/drivers/platform/x86/amd/amd_isp4.c
>> @@ -0,0 +1,269 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * AMD ISP platform driver for sensor i2-client instantiation
>> + *
>> + * Copyright 2025 Advanced Micro Devices, Inc.
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/property.h>
>> +#include <linux/units.h>
>> +
>> +#define AMDISP_OV05C10_I2C_ADDR 0x10
>> +#define AMDISP_OV05C10_PLAT_NAME "amdisp_ov05c10_platform"
>> +#define AMDISP_OV05C10_HID "OMNI5C10"
>> +#define AMDISP_OV05C10_REMOTE_EP_NAME "ov05c10_isp_4_1_1"
>> +#define AMD_ISP_PLAT_DRV_NAME "amd-isp4"
>> +
>> +/*
>> + * AMD ISP platform definition to configure the device properties
>> + * missing in the ACPI table.
>> + */
>> +struct amdisp_platform {
>> + const char *name;
>> + u8 i2c_addr;
>> + u8 max_num_swnodes;
>> + struct i2c_board_info board_info;
>> + struct notifier_block i2c_nb;
>> + struct i2c_client *i2c_dev;
>> + const struct software_node **swnodes;
>> +};
>> +
>> +/* Top-level OV05C10 camera node property table */
>> +static const struct property_entry ov05c10_camera_props[] = {
>> + PROPERTY_ENTRY_U32("clock-frequency", 24 * HZ_PER_MHZ),
>> + { }
>> +};
>> +
>> +/* Root AMD ISP OV05C10 camera node definition */
>> +static const struct software_node camera_node = {
>> + .name = AMDISP_OV05C10_HID,
>> + .properties = ov05c10_camera_props,
>> +};
>> +
>> +/*
>> + * AMD ISP OV05C10 Ports node definition. No properties defined for
>> + * ports node for OV05C10.
>> + */
>> +static const struct software_node ports = {
>> + .name = "ports",
>> + .parent = &camera_node,
>> +};
>> +
>> +/*
>> + * AMD ISP OV05C10 Port node definition. No properties defined for
>> + * port node for OV05C10.
>> + */
>> +static const struct software_node port_node = {
>> + .name = "port@",
>> + .parent = &ports,
>> +};
>> +
>> +/*
>> + * Remote endpoint AMD ISP node definition. No properties defined for
>> + * remote endpoint node for OV05C10.
>> + */
>> +static const struct software_node remote_ep_isp_node = {
>> + .name = AMDISP_OV05C10_REMOTE_EP_NAME,
>> +};
>> +
>> +/*
>> + * Remote endpoint reference for isp node included in the
>> + * OV05C10 endpoint.
>> + */
>> +static const struct software_node_ref_args ov05c10_refs[] = {
>> + SOFTWARE_NODE_REFERENCE(&remote_ep_isp_node),
>> +};
>> +
>> +/* OV05C supports one single link frequency */
>> +static const u64 ov05c10_link_freqs[] = {
>> + 925 * HZ_PER_MHZ,
>> +};
>> +
>> +/* OV05C supports only 2-lane configuration */
>> +static const u32 ov05c10_data_lanes[] = {
>> + 1,
>> + 2,
>> +};
>> +
>> +/* OV05C10 endpoint node properties table */
>> +static const struct property_entry ov05c10_endpoint_props[] = {
>> + PROPERTY_ENTRY_U32("bus-type", 4),
>> + PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes", ov05c10_data_lanes,
>> + ARRAY_SIZE(ov05c10_data_lanes)),
>> + PROPERTY_ENTRY_U64_ARRAY_LEN("link-frequencies",
>> ov05c10_link_freqs,
>> + ARRAY_SIZE(ov05c10_link_freqs)),
>> + PROPERTY_ENTRY_REF_ARRAY("remote-endpoint", ov05c10_refs),
>> + { }
>> +};
>> +
>> +/* AMD ISP endpoint node definition */
>> +static const struct software_node endpoint_node = {
>> + .name = "endpoint",
>> + .parent = &port_node,
>> + .properties = ov05c10_endpoint_props,
>> +};
>> +
>> +/*
>> + * AMD ISP swnode graph uses 5 nodes and also its relationship is
>> + * fixed to align with the structure that v4l2 expects for successful
>> + * endpoint fwnode parsing.
>> + *
>> + * It is only the node property_entries that will vary for each platform
>> + * supporting different sensor modules.
>> + */
>> +#define NUM_SW_NODES 5
>> +
>> +static const struct software_node *ov05c10_nodes[NUM_SW_NODES + 1] = {
>> + &camera_node,
>> + &ports,
>> + &port_node,
>> + &endpoint_node,
>> + &remote_ep_isp_node,
>> + NULL
>> +};
>> +
>> +/* OV05C10 specific AMD ISP platform configuration */
>> +static const struct amdisp_platform amdisp_ov05c10_platform_config = {
>> + .name = AMDISP_OV05C10_PLAT_NAME,
>
> Where is this field being used?
>
>> + .board_info = {
>> + .dev_name = "ov05c10",
>> + I2C_BOARD_INFO("ov05c10", AMDISP_OV05C10_I2C_ADDR),
>> + },
>> + .i2c_addr = AMDISP_OV05C10_I2C_ADDR,
>
> Please reuse board_info->addr.
>
>> + .max_num_swnodes = NUM_SW_NODES,
>
> Where is max_num_swnodes being used?
>
>> + .swnodes = ov05c10_nodes,
>
> Why not drop .swnodes and referencing ov05c10_nodes directly?
>
Thanks. Some of the variables phased out with the ongoing development
and patching. I will take care of removing the unused and redudant
variables in 'struct amdisp_platform'.
>> +};
>> +
>> +static const struct acpi_device_id amdisp_sensor_ids[] = {
>> + { AMDISP_OV05C10_HID },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, amdisp_sensor_ids);
>> +
>> +static inline bool is_isp_i2c_adapter(struct i2c_adapter *adap)
>> +{
>> + return !strcmp(adap->owner->name, "i2c_designware_amdisp");
>> +}
>> +
>> +static void instantiate_isp_i2c_client(struct amdisp_platform
>> *ov05c10, struct i2c_adapter *adap)
>> +{
>> + struct i2c_board_info *info = &ov05c10->board_info;
>> + struct i2c_client *i2c_dev;
>> +
>> + if (ov05c10->i2c_dev)
>> + return;
>> +
>> + if (!info->addr) {
>> + dev_err(&adap->dev, "invalid i2c_addr 0x%x detected\n",
>> ov05c10->i2c_addr);
>> + return;
>> + }
>> +
>> + i2c_dev = i2c_new_client_device(adap, info);
>> + if (IS_ERR(i2c_dev)) {
>> + dev_err(&adap->dev, "error %pe registering isp
>> i2c_client\n", i2c_dev);
>> + return;
>> + }
>> + ov05c10->i2c_dev = i2c_dev;
>> +}
>> +
>> +static int isp_i2c_bus_notify(struct notifier_block *nb,
>> + unsigned long action, void *data)
>> +{
>> + struct amdisp_platform *ov05c10 = container_of(nb, struct
>> amdisp_platform, i2c_nb);
>> + struct device *dev = data;
>> + struct i2c_client *client;
>> + struct i2c_adapter *adap;
>> +
>> + switch (action) {
>> + case BUS_NOTIFY_ADD_DEVICE:
>> + adap = i2c_verify_adapter(dev);
>> + if (!adap)
>> + break;
>> + if (is_isp_i2c_adapter(adap))
>> + instantiate_isp_i2c_client(ov05c10, adap);
>> + break;
>> + case BUS_NOTIFY_REMOVED_DEVICE:
>> + client = i2c_verify_client(dev);
>> + if (!client)
>> + break;
>> + if (ov05c10->i2c_dev == client) {
>> + dev_dbg(&client->adapter->dev, "amdisp
>> i2c_client removed\n");
>> + ov05c10->i2c_dev = NULL;
>> + }
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + return NOTIFY_DONE;
>
> You still need to handle the situation where the AMD I2C adapter is
> already registered when
> registering the bus notifier. In this case you will miss the
> BUS_NOTIFY_ADD_DEVICE event.
>
Thanks. I will cover this case using the below sequence.
1. bus_register_notifier()
2. i2c_for_each_dev()
If at all i2c adapter is registered by the time bus_register_notifier()
is called, it should be detected in i2c_for_each_dev(). I will add
checks to avoid creation of duplicate i2c_client devices when both
notifier callback and i2c_for_each_dev() passes especially when i2c
adapter gets added between 1 and 2.
Please suggest if there is an alternate better approach that I should
use to handle this case.
>> +}
>> +
>> +static struct amdisp_platform *prepare_amdisp_platform(struct device
>> *dev,
>> + const struct
>> amdisp_platform *src)
>> +{
>> + struct amdisp_platform *isp_ov05c10;
>> + struct i2c_board_info *info;
>> + int ret;
>> +
>> + isp_ov05c10 = devm_kzalloc(dev, sizeof(*isp_ov05c10), GFP_KERNEL);
>> + if (!isp_ov05c10)
>> + return ERR_PTR(-ENOMEM);
>> + memcpy(isp_ov05c10, src, sizeof(*isp_ov05c10));
>
> This is not what i meant. I was complaining that
> amdisp_ov05c10_platform_config contains both
> static data (swnodes) and data assigned during runtime (board_info-
> >swnode, i2c_dev, ...).
>
> Please do not use a global instance of struct amdisp_platform for
> initialization. Instead initialize a
> fresh instance of this struct inside prepare_amdisp_platform().
>
sure, will remove the global variable 'amdisp_ov05c10_platform_config',
and will take care of initializing the amdisp_platform instance in the
prepare_amdisp_platform().
>> +
>> + info = &isp_ov05c10->board_info;
>> +
>> + ret = software_node_register_node_group(src->swnodes);
>> + if (ret)
>> + return ERR_PTR(ret);
>> +
>> + info->swnode = src->swnodes[0];
>> +
>> + return isp_ov05c10;
>> +}
>> +
>> +static int amd_isp_probe(struct platform_device *pdev)
>> +{
>> + struct amdisp_platform *ov05c10;
>> + int ret;
>> +
>> + ov05c10 = prepare_amdisp_platform(&pdev->dev,
>> &amdisp_ov05c10_platform_config);
>> + if (IS_ERR(ov05c10))
>> + return dev_err_probe(&pdev->dev, PTR_ERR(ov05c10),
>> + "failed to prepare AMD ISP platform
>> fwnode\n");
>> +
>> + ov05c10->i2c_nb.notifier_call = isp_i2c_bus_notify;
>> + ret = bus_register_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
>> + if (ret)
>> + return ret;
>
> You need to call software_node_unregister_node_group() here when
> bus_register_notifier() fails.
>
Thanks. I will fix this in the next V8 patch.
> Thanks,
> Armin Wolf>
Thanks,
Pratap
>> +
>> + platform_set_drvdata(pdev, ov05c10);
>> + return 0;
>> +}
>> +
>> +static void amd_isp_remove(struct platform_device *pdev)
>> +{
>> + struct amdisp_platform *ov05c10 = platform_get_drvdata(pdev);
>> +
>> + bus_unregister_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
>> + i2c_unregister_device(ov05c10->i2c_dev);
>> + software_node_unregister_node_group(ov05c10->swnodes);
>> +}
>> +
>> +static struct platform_driver amd_isp_platform_driver = {
>> + .driver = {
>> + .name = AMD_ISP_PLAT_DRV_NAME,
>> + .acpi_match_table = amdisp_sensor_ids,
>> + },
>> + .probe = amd_isp_probe,
>> + .remove = amd_isp_remove,
>> +};
>> +
>> +module_platform_driver(amd_isp_platform_driver);
>> +
>> +MODULE_AUTHOR("Benjamin Chan <benjamin.chan@amd.com>");
>> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@amd.com>");
>> +MODULE_DESCRIPTION("AMD ISP4 Platform Driver");
>> +MODULE_LICENSE("GPL");
Am 29.04.25 um 20:33 schrieb Nirujogi, Pratap:
> Hi Armin,
>
> On 4/26/2025 8:25 PM, Armin Wolf wrote:
>> Caution: This message originated from an External Source. Use proper
>> caution when opening attachments, clicking links, or responding.
>>
>>
>> Am 17.04.25 um 20:28 schrieb Pratap Nirujogi:
>>
>>> ISP device specific configuration is not available in ACPI. Add
>>> swnode graph to configure the missing device properties for the
>>> OV05C10 camera device supported on amdisp platform.
>>>
>>> Add support to create i2c-client dynamically when amdisp i2c
>>> adapter is available.
>>>
>>> Co-developed-by: Benjamin Chan <benjamin.chan@amd.com>
>>> Signed-off-by: Benjamin Chan <benjamin.chan@amd.com>
>>> Signed-off-by: Pratap Nirujogi <pratap.nirujogi@amd.com>
>>> ---
>>> Changes v6 -> v7:
>>>
>>> * Use devm_kzalloc() inplace of kmemdup()
>>> * Use IS_ERR() inplace of i2c_client_has_driver()
>>> * Remove the extra cast
>>>
>>> drivers/platform/x86/amd/Kconfig | 11 ++
>>> drivers/platform/x86/amd/Makefile | 1 +
>>> drivers/platform/x86/amd/amd_isp4.c | 269
>>> ++++++++++++++++++++++++++++
>>> 3 files changed, 281 insertions(+)
>>> create mode 100644 drivers/platform/x86/amd/amd_isp4.c
>>>
>>> diff --git a/drivers/platform/x86/amd/Kconfig
>>> b/drivers/platform/x86/ amd/Kconfig
>>> index c3e086ea64fc..ec755b5fc93c 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
>>> + tristate "AMD ISP4 platform driver"
>>> + depends on I2C && X86_64 && ACPI && AMD_ISP4
>>
>> Hi,
>>
>> just a question: when will the CONFIG_AMD_ISP4 symbol be introduced?
>>
> CONFIG_AMD_ISP4 will be introduced in the V4L2 ISP driver patches. We
> are working on isp driver patches and planning to submit once the
> review for x86/platform and sensor driver patches completes.
Alright.
>
>>> + help
>>> + Platform driver for AMD platforms containing image signal
>>> processor
>>> + gen 4. Provides camera sensor module board information to allow
>>> + sensor and V4L drivers to work properly.
>>> +
>>> + This driver can also be built as a module. If so, the module
>>> + will be called amd_isp4.
>>> diff --git a/drivers/platform/x86/amd/Makefile
>>> b/drivers/platform/x86/ amd/Makefile
>>> index c6c40bdcbded..b0e284b5d497 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_isp4.o
>>> diff --git a/drivers/platform/x86/amd/amd_isp4.c b/drivers/platform/
>>> x86/amd/amd_isp4.c
>>> new file mode 100644
>>> index 000000000000..461a10be5ccd
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/amd/amd_isp4.c
>>> @@ -0,0 +1,269 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * AMD ISP platform driver for sensor i2-client instantiation
>>> + *
>>> + * Copyright 2025 Advanced Micro Devices, Inc.
>>> + */
>>> +
>>> +#include <linux/i2c.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/property.h>
>>> +#include <linux/units.h>
>>> +
>>> +#define AMDISP_OV05C10_I2C_ADDR 0x10
>>> +#define AMDISP_OV05C10_PLAT_NAME "amdisp_ov05c10_platform"
>>> +#define AMDISP_OV05C10_HID "OMNI5C10"
>>> +#define AMDISP_OV05C10_REMOTE_EP_NAME "ov05c10_isp_4_1_1"
>>> +#define AMD_ISP_PLAT_DRV_NAME "amd-isp4"
>>> +
>>> +/*
>>> + * AMD ISP platform definition to configure the device properties
>>> + * missing in the ACPI table.
>>> + */
>>> +struct amdisp_platform {
>>> + const char *name;
>>> + u8 i2c_addr;
>>> + u8 max_num_swnodes;
>>> + struct i2c_board_info board_info;
>>> + struct notifier_block i2c_nb;
>>> + struct i2c_client *i2c_dev;
>>> + const struct software_node **swnodes;
>>> +};
>>> +
>>> +/* Top-level OV05C10 camera node property table */
>>> +static const struct property_entry ov05c10_camera_props[] = {
>>> + PROPERTY_ENTRY_U32("clock-frequency", 24 * HZ_PER_MHZ),
>>> + { }
>>> +};
>>> +
>>> +/* Root AMD ISP OV05C10 camera node definition */
>>> +static const struct software_node camera_node = {
>>> + .name = AMDISP_OV05C10_HID,
>>> + .properties = ov05c10_camera_props,
>>> +};
>>> +
>>> +/*
>>> + * AMD ISP OV05C10 Ports node definition. No properties defined for
>>> + * ports node for OV05C10.
>>> + */
>>> +static const struct software_node ports = {
>>> + .name = "ports",
>>> + .parent = &camera_node,
>>> +};
>>> +
>>> +/*
>>> + * AMD ISP OV05C10 Port node definition. No properties defined for
>>> + * port node for OV05C10.
>>> + */
>>> +static const struct software_node port_node = {
>>> + .name = "port@",
>>> + .parent = &ports,
>>> +};
>>> +
>>> +/*
>>> + * Remote endpoint AMD ISP node definition. No properties defined for
>>> + * remote endpoint node for OV05C10.
>>> + */
>>> +static const struct software_node remote_ep_isp_node = {
>>> + .name = AMDISP_OV05C10_REMOTE_EP_NAME,
>>> +};
>>> +
>>> +/*
>>> + * Remote endpoint reference for isp node included in the
>>> + * OV05C10 endpoint.
>>> + */
>>> +static const struct software_node_ref_args ov05c10_refs[] = {
>>> + SOFTWARE_NODE_REFERENCE(&remote_ep_isp_node),
>>> +};
>>> +
>>> +/* OV05C supports one single link frequency */
>>> +static const u64 ov05c10_link_freqs[] = {
>>> + 925 * HZ_PER_MHZ,
>>> +};
>>> +
>>> +/* OV05C supports only 2-lane configuration */
>>> +static const u32 ov05c10_data_lanes[] = {
>>> + 1,
>>> + 2,
>>> +};
>>> +
>>> +/* OV05C10 endpoint node properties table */
>>> +static const struct property_entry ov05c10_endpoint_props[] = {
>>> + PROPERTY_ENTRY_U32("bus-type", 4),
>>> + PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes", ov05c10_data_lanes,
>>> + ARRAY_SIZE(ov05c10_data_lanes)),
>>> + PROPERTY_ENTRY_U64_ARRAY_LEN("link-frequencies",
>>> ov05c10_link_freqs,
>>> + ARRAY_SIZE(ov05c10_link_freqs)),
>>> + PROPERTY_ENTRY_REF_ARRAY("remote-endpoint", ov05c10_refs),
>>> + { }
>>> +};
>>> +
>>> +/* AMD ISP endpoint node definition */
>>> +static const struct software_node endpoint_node = {
>>> + .name = "endpoint",
>>> + .parent = &port_node,
>>> + .properties = ov05c10_endpoint_props,
>>> +};
>>> +
>>> +/*
>>> + * AMD ISP swnode graph uses 5 nodes and also its relationship is
>>> + * fixed to align with the structure that v4l2 expects for successful
>>> + * endpoint fwnode parsing.
>>> + *
>>> + * It is only the node property_entries that will vary for each
>>> platform
>>> + * supporting different sensor modules.
>>> + */
>>> +#define NUM_SW_NODES 5
>>> +
>>> +static const struct software_node *ov05c10_nodes[NUM_SW_NODES + 1] = {
>>> + &camera_node,
>>> + &ports,
>>> + &port_node,
>>> + &endpoint_node,
>>> + &remote_ep_isp_node,
>>> + NULL
>>> +};
>>> +
>>> +/* OV05C10 specific AMD ISP platform configuration */
>>> +static const struct amdisp_platform amdisp_ov05c10_platform_config = {
>>> + .name = AMDISP_OV05C10_PLAT_NAME,
>>
>> Where is this field being used?
>>
>>> + .board_info = {
>>> + .dev_name = "ov05c10",
>>> + I2C_BOARD_INFO("ov05c10", AMDISP_OV05C10_I2C_ADDR),
>>> + },
>>> + .i2c_addr = AMDISP_OV05C10_I2C_ADDR,
>>
>> Please reuse board_info->addr.
>>
>>> + .max_num_swnodes = NUM_SW_NODES,
>>
>> Where is max_num_swnodes being used?
>>
>>> + .swnodes = ov05c10_nodes,
>>
>> Why not drop .swnodes and referencing ov05c10_nodes directly?
>>
> Thanks. Some of the variables phased out with the ongoing development
> and patching. I will take care of removing the unused and redudant
> variables in 'struct amdisp_platform'.
>
>>> +};
>>> +
>>> +static const struct acpi_device_id amdisp_sensor_ids[] = {
>>> + { AMDISP_OV05C10_HID },
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(acpi, amdisp_sensor_ids);
>>> +
>>> +static inline bool is_isp_i2c_adapter(struct i2c_adapter *adap)
>>> +{
>>> + return !strcmp(adap->owner->name, "i2c_designware_amdisp");
>>> +}
>>> +
>>> +static void instantiate_isp_i2c_client(struct amdisp_platform
>>> *ov05c10, struct i2c_adapter *adap)
>>> +{
>>> + struct i2c_board_info *info = &ov05c10->board_info;
>>> + struct i2c_client *i2c_dev;
>>> +
>>> + if (ov05c10->i2c_dev)
>>> + return;
>>> +
>>> + if (!info->addr) {
>>> + dev_err(&adap->dev, "invalid i2c_addr 0x%x
>>> detected\n", ov05c10->i2c_addr);
>>> + return;
>>> + }
>>> +
>>> + i2c_dev = i2c_new_client_device(adap, info);
>>> + if (IS_ERR(i2c_dev)) {
>>> + dev_err(&adap->dev, "error %pe registering isp
>>> i2c_client\n", i2c_dev);
>>> + return;
>>> + }
>>> + ov05c10->i2c_dev = i2c_dev;
>>> +}
>>> +
>>> +static int isp_i2c_bus_notify(struct notifier_block *nb,
>>> + unsigned long action, void *data)
>>> +{
>>> + struct amdisp_platform *ov05c10 = container_of(nb, struct
>>> amdisp_platform, i2c_nb);
>>> + struct device *dev = data;
>>> + struct i2c_client *client;
>>> + struct i2c_adapter *adap;
>>> +
>>> + switch (action) {
>>> + case BUS_NOTIFY_ADD_DEVICE:
>>> + adap = i2c_verify_adapter(dev);
>>> + if (!adap)
>>> + break;
>>> + if (is_isp_i2c_adapter(adap))
>>> + instantiate_isp_i2c_client(ov05c10, adap);
>>> + break;
>>> + case BUS_NOTIFY_REMOVED_DEVICE:
>>> + client = i2c_verify_client(dev);
>>> + if (!client)
>>> + break;
>>> + if (ov05c10->i2c_dev == client) {
>>> + dev_dbg(&client->adapter->dev, "amdisp
>>> i2c_client removed\n");
>>> + ov05c10->i2c_dev = NULL;
>>> + }
>>> + break;
>>> + default:
>>> + break;
>>> + }
>>> +
>>> + return NOTIFY_DONE;
>>
>> You still need to handle the situation where the AMD I2C adapter is
>> already registered when
>> registering the bus notifier. In this case you will miss the
>> BUS_NOTIFY_ADD_DEVICE event.
>>
> Thanks. I will cover this case using the below sequence.
>
> 1. bus_register_notifier()
> 2. i2c_for_each_dev()
>
> If at all i2c adapter is registered by the time
> bus_register_notifier() is called, it should be detected in
> i2c_for_each_dev(). I will add checks to avoid creation of duplicate
> i2c_client devices when both notifier callback and i2c_for_each_dev()
> passes especially when i2c adapter gets added between 1 and 2.
>
> Please suggest if there is an alternate better approach that I should
> use to handle this case.
>
This approach seems good to me, i2c-dev uses something similar.
Thanks,
Armin Wolf
>>> +}
>>> +
>>> +static struct amdisp_platform *prepare_amdisp_platform(struct
>>> device *dev,
>>> + const struct
>>> amdisp_platform *src)
>>> +{
>>> + struct amdisp_platform *isp_ov05c10;
>>> + struct i2c_board_info *info;
>>> + int ret;
>>> +
>>> + isp_ov05c10 = devm_kzalloc(dev, sizeof(*isp_ov05c10),
>>> GFP_KERNEL);
>>> + if (!isp_ov05c10)
>>> + return ERR_PTR(-ENOMEM);
>>> + memcpy(isp_ov05c10, src, sizeof(*isp_ov05c10));
>>
>> This is not what i meant. I was complaining that
>> amdisp_ov05c10_platform_config contains both
>> static data (swnodes) and data assigned during runtime (board_info-
>> >swnode, i2c_dev, ...).
>>
>> Please do not use a global instance of struct amdisp_platform for
>> initialization. Instead initialize a
>> fresh instance of this struct inside prepare_amdisp_platform().
>>
> sure, will remove the global variable
> 'amdisp_ov05c10_platform_config', and will take care of initializing
> the amdisp_platform instance in the prepare_amdisp_platform().
>
>>> +
>>> + info = &isp_ov05c10->board_info;
>>> +
>>> + ret = software_node_register_node_group(src->swnodes);
>>> + if (ret)
>>> + return ERR_PTR(ret);
>>> +
>>> + info->swnode = src->swnodes[0];
>>> +
>>> + return isp_ov05c10;
>>> +}
>>> +
>>> +static int amd_isp_probe(struct platform_device *pdev)
>>> +{
>>> + struct amdisp_platform *ov05c10;
>>> + int ret;
>>> +
>>> + ov05c10 = prepare_amdisp_platform(&pdev->dev,
>>> &amdisp_ov05c10_platform_config);
>>> + if (IS_ERR(ov05c10))
>>> + return dev_err_probe(&pdev->dev, PTR_ERR(ov05c10),
>>> + "failed to prepare AMD ISP
>>> platform fwnode\n");
>>> +
>>> + ov05c10->i2c_nb.notifier_call = isp_i2c_bus_notify;
>>> + ret = bus_register_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
>>> + if (ret)
>>> + return ret;
>>
>> You need to call software_node_unregister_node_group() here when
>> bus_register_notifier() fails.
>>
> Thanks. I will fix this in the next V8 patch.
>
>> Thanks,
>> Armin Wolf>
> Thanks,
> Pratap
>
>>> +
>>> + platform_set_drvdata(pdev, ov05c10);
>>> + return 0;
>>> +}
>>> +
>>> +static void amd_isp_remove(struct platform_device *pdev)
>>> +{
>>> + struct amdisp_platform *ov05c10 = platform_get_drvdata(pdev);
>>> +
>>> + bus_unregister_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
>>> + i2c_unregister_device(ov05c10->i2c_dev);
>>> + software_node_unregister_node_group(ov05c10->swnodes);
>>> +}
>>> +
>>> +static struct platform_driver amd_isp_platform_driver = {
>>> + .driver = {
>>> + .name = AMD_ISP_PLAT_DRV_NAME,
>>> + .acpi_match_table = amdisp_sensor_ids,
>>> + },
>>> + .probe = amd_isp_probe,
>>> + .remove = amd_isp_remove,
>>> +};
>>> +
>>> +module_platform_driver(amd_isp_platform_driver);
>>> +
>>> +MODULE_AUTHOR("Benjamin Chan <benjamin.chan@amd.com>");
>>> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@amd.com>");
>>> +MODULE_DESCRIPTION("AMD ISP4 Platform Driver");
>>> +MODULE_LICENSE("GPL");
>
>
On 4/29/2025 4:52 PM, Armin Wolf wrote:
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> Am 29.04.25 um 20:33 schrieb Nirujogi, Pratap:
>
>> Hi Armin,
>>
>> On 4/26/2025 8:25 PM, Armin Wolf wrote:
>>> Caution: This message originated from an External Source. Use proper
>>> caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> Am 17.04.25 um 20:28 schrieb Pratap Nirujogi:
>>>
>>>> ISP device specific configuration is not available in ACPI. Add
>>>> swnode graph to configure the missing device properties for the
>>>> OV05C10 camera device supported on amdisp platform.
>>>>
>>>> Add support to create i2c-client dynamically when amdisp i2c
>>>> adapter is available.
>>>>
>>>> Co-developed-by: Benjamin Chan <benjamin.chan@amd.com>
>>>> Signed-off-by: Benjamin Chan <benjamin.chan@amd.com>
>>>> Signed-off-by: Pratap Nirujogi <pratap.nirujogi@amd.com>
>>>> ---
>>>> Changes v6 -> v7:
>>>>
>>>> * Use devm_kzalloc() inplace of kmemdup()
>>>> * Use IS_ERR() inplace of i2c_client_has_driver()
>>>> * Remove the extra cast
>>>>
>>>> drivers/platform/x86/amd/Kconfig | 11 ++
>>>> drivers/platform/x86/amd/Makefile | 1 +
>>>> drivers/platform/x86/amd/amd_isp4.c | 269
>>>> ++++++++++++++++++++++++++++
>>>> 3 files changed, 281 insertions(+)
>>>> create mode 100644 drivers/platform/x86/amd/amd_isp4.c
>>>>
>>>> diff --git a/drivers/platform/x86/amd/Kconfig
>>>> b/drivers/platform/x86/ amd/Kconfig
>>>> index c3e086ea64fc..ec755b5fc93c 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
>>>> + tristate "AMD ISP4 platform driver"
>>>> + depends on I2C && X86_64 && ACPI && AMD_ISP4
>>>
>>> Hi,
>>>
>>> just a question: when will the CONFIG_AMD_ISP4 symbol be introduced?
>>>
>> CONFIG_AMD_ISP4 will be introduced in the V4L2 ISP driver patches. We
>> are working on isp driver patches and planning to submit once the
>> review for x86/platform and sensor driver patches completes.
>
> Alright.
>
>>
>>>> + help
>>>> + Platform driver for AMD platforms containing image signal
>>>> processor
>>>> + gen 4. Provides camera sensor module board information to allow
>>>> + sensor and V4L drivers to work properly.
>>>> +
>>>> + This driver can also be built as a module. If so, the module
>>>> + will be called amd_isp4.
>>>> diff --git a/drivers/platform/x86/amd/Makefile
>>>> b/drivers/platform/x86/ amd/Makefile
>>>> index c6c40bdcbded..b0e284b5d497 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_isp4.o
>>>> diff --git a/drivers/platform/x86/amd/amd_isp4.c b/drivers/platform/
>>>> x86/amd/amd_isp4.c
>>>> new file mode 100644
>>>> index 000000000000..461a10be5ccd
>>>> --- /dev/null
>>>> +++ b/drivers/platform/x86/amd/amd_isp4.c
>>>> @@ -0,0 +1,269 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * AMD ISP platform driver for sensor i2-client instantiation
>>>> + *
>>>> + * Copyright 2025 Advanced Micro Devices, Inc.
>>>> + */
>>>> +
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/property.h>
>>>> +#include <linux/units.h>
>>>> +
>>>> +#define AMDISP_OV05C10_I2C_ADDR 0x10
>>>> +#define AMDISP_OV05C10_PLAT_NAME "amdisp_ov05c10_platform"
>>>> +#define AMDISP_OV05C10_HID "OMNI5C10"
>>>> +#define AMDISP_OV05C10_REMOTE_EP_NAME "ov05c10_isp_4_1_1"
>>>> +#define AMD_ISP_PLAT_DRV_NAME "amd-isp4"
>>>> +
>>>> +/*
>>>> + * AMD ISP platform definition to configure the device properties
>>>> + * missing in the ACPI table.
>>>> + */
>>>> +struct amdisp_platform {
>>>> + const char *name;
>>>> + u8 i2c_addr;
>>>> + u8 max_num_swnodes;
>>>> + struct i2c_board_info board_info;
>>>> + struct notifier_block i2c_nb;
>>>> + struct i2c_client *i2c_dev;
>>>> + const struct software_node **swnodes;
>>>> +};
>>>> +
>>>> +/* Top-level OV05C10 camera node property table */
>>>> +static const struct property_entry ov05c10_camera_props[] = {
>>>> + PROPERTY_ENTRY_U32("clock-frequency", 24 * HZ_PER_MHZ),
>>>> + { }
>>>> +};
>>>> +
>>>> +/* Root AMD ISP OV05C10 camera node definition */
>>>> +static const struct software_node camera_node = {
>>>> + .name = AMDISP_OV05C10_HID,
>>>> + .properties = ov05c10_camera_props,
>>>> +};
>>>> +
>>>> +/*
>>>> + * AMD ISP OV05C10 Ports node definition. No properties defined for
>>>> + * ports node for OV05C10.
>>>> + */
>>>> +static const struct software_node ports = {
>>>> + .name = "ports",
>>>> + .parent = &camera_node,
>>>> +};
>>>> +
>>>> +/*
>>>> + * AMD ISP OV05C10 Port node definition. No properties defined for
>>>> + * port node for OV05C10.
>>>> + */
>>>> +static const struct software_node port_node = {
>>>> + .name = "port@",
>>>> + .parent = &ports,
>>>> +};
>>>> +
>>>> +/*
>>>> + * Remote endpoint AMD ISP node definition. No properties defined for
>>>> + * remote endpoint node for OV05C10.
>>>> + */
>>>> +static const struct software_node remote_ep_isp_node = {
>>>> + .name = AMDISP_OV05C10_REMOTE_EP_NAME,
>>>> +};
>>>> +
>>>> +/*
>>>> + * Remote endpoint reference for isp node included in the
>>>> + * OV05C10 endpoint.
>>>> + */
>>>> +static const struct software_node_ref_args ov05c10_refs[] = {
>>>> + SOFTWARE_NODE_REFERENCE(&remote_ep_isp_node),
>>>> +};
>>>> +
>>>> +/* OV05C supports one single link frequency */
>>>> +static const u64 ov05c10_link_freqs[] = {
>>>> + 925 * HZ_PER_MHZ,
>>>> +};
>>>> +
>>>> +/* OV05C supports only 2-lane configuration */
>>>> +static const u32 ov05c10_data_lanes[] = {
>>>> + 1,
>>>> + 2,
>>>> +};
>>>> +
>>>> +/* OV05C10 endpoint node properties table */
>>>> +static const struct property_entry ov05c10_endpoint_props[] = {
>>>> + PROPERTY_ENTRY_U32("bus-type", 4),
>>>> + PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes", ov05c10_data_lanes,
>>>> + ARRAY_SIZE(ov05c10_data_lanes)),
>>>> + PROPERTY_ENTRY_U64_ARRAY_LEN("link-frequencies",
>>>> ov05c10_link_freqs,
>>>> + ARRAY_SIZE(ov05c10_link_freqs)),
>>>> + PROPERTY_ENTRY_REF_ARRAY("remote-endpoint", ov05c10_refs),
>>>> + { }
>>>> +};
>>>> +
>>>> +/* AMD ISP endpoint node definition */
>>>> +static const struct software_node endpoint_node = {
>>>> + .name = "endpoint",
>>>> + .parent = &port_node,
>>>> + .properties = ov05c10_endpoint_props,
>>>> +};
>>>> +
>>>> +/*
>>>> + * AMD ISP swnode graph uses 5 nodes and also its relationship is
>>>> + * fixed to align with the structure that v4l2 expects for successful
>>>> + * endpoint fwnode parsing.
>>>> + *
>>>> + * It is only the node property_entries that will vary for each
>>>> platform
>>>> + * supporting different sensor modules.
>>>> + */
>>>> +#define NUM_SW_NODES 5
>>>> +
>>>> +static const struct software_node *ov05c10_nodes[NUM_SW_NODES + 1] = {
>>>> + &camera_node,
>>>> + &ports,
>>>> + &port_node,
>>>> + &endpoint_node,
>>>> + &remote_ep_isp_node,
>>>> + NULL
>>>> +};
>>>> +
>>>> +/* OV05C10 specific AMD ISP platform configuration */
>>>> +static const struct amdisp_platform amdisp_ov05c10_platform_config = {
>>>> + .name = AMDISP_OV05C10_PLAT_NAME,
>>>
>>> Where is this field being used?
>>>
>>>> + .board_info = {
>>>> + .dev_name = "ov05c10",
>>>> + I2C_BOARD_INFO("ov05c10", AMDISP_OV05C10_I2C_ADDR),
>>>> + },
>>>> + .i2c_addr = AMDISP_OV05C10_I2C_ADDR,
>>>
>>> Please reuse board_info->addr.
>>>
>>>> + .max_num_swnodes = NUM_SW_NODES,
>>>
>>> Where is max_num_swnodes being used?
>>>
>>>> + .swnodes = ov05c10_nodes,
>>>
>>> Why not drop .swnodes and referencing ov05c10_nodes directly?
>>>
>> Thanks. Some of the variables phased out with the ongoing development
>> and patching. I will take care of removing the unused and redudant
>> variables in 'struct amdisp_platform'.
>>
>>>> +};
>>>> +
>>>> +static const struct acpi_device_id amdisp_sensor_ids[] = {
>>>> + { AMDISP_OV05C10_HID },
>>>> + { }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(acpi, amdisp_sensor_ids);
>>>> +
>>>> +static inline bool is_isp_i2c_adapter(struct i2c_adapter *adap)
>>>> +{
>>>> + return !strcmp(adap->owner->name, "i2c_designware_amdisp");
>>>> +}
>>>> +
>>>> +static void instantiate_isp_i2c_client(struct amdisp_platform
>>>> *ov05c10, struct i2c_adapter *adap)
>>>> +{
>>>> + struct i2c_board_info *info = &ov05c10->board_info;
>>>> + struct i2c_client *i2c_dev;
>>>> +
>>>> + if (ov05c10->i2c_dev)
>>>> + return;
>>>> +
>>>> + if (!info->addr) {
>>>> + dev_err(&adap->dev, "invalid i2c_addr 0x%x
>>>> detected\n", ov05c10->i2c_addr);
>>>> + return;
>>>> + }
>>>> +
>>>> + i2c_dev = i2c_new_client_device(adap, info);
>>>> + if (IS_ERR(i2c_dev)) {
>>>> + dev_err(&adap->dev, "error %pe registering isp
>>>> i2c_client\n", i2c_dev);
>>>> + return;
>>>> + }
>>>> + ov05c10->i2c_dev = i2c_dev;
>>>> +}
>>>> +
>>>> +static int isp_i2c_bus_notify(struct notifier_block *nb,
>>>> + unsigned long action, void *data)
>>>> +{
>>>> + struct amdisp_platform *ov05c10 = container_of(nb, struct
>>>> amdisp_platform, i2c_nb);
>>>> + struct device *dev = data;
>>>> + struct i2c_client *client;
>>>> + struct i2c_adapter *adap;
>>>> +
>>>> + switch (action) {
>>>> + case BUS_NOTIFY_ADD_DEVICE:
>>>> + adap = i2c_verify_adapter(dev);
>>>> + if (!adap)
>>>> + break;
>>>> + if (is_isp_i2c_adapter(adap))
>>>> + instantiate_isp_i2c_client(ov05c10, adap);
>>>> + break;
>>>> + case BUS_NOTIFY_REMOVED_DEVICE:
>>>> + client = i2c_verify_client(dev);
>>>> + if (!client)
>>>> + break;
>>>> + if (ov05c10->i2c_dev == client) {
>>>> + dev_dbg(&client->adapter->dev, "amdisp
>>>> i2c_client removed\n");
>>>> + ov05c10->i2c_dev = NULL;
>>>> + }
>>>> + break;
>>>> + default:
>>>> + break;
>>>> + }
>>>> +
>>>> + return NOTIFY_DONE;
>>>
>>> You still need to handle the situation where the AMD I2C adapter is
>>> already registered when
>>> registering the bus notifier. In this case you will miss the
>>> BUS_NOTIFY_ADD_DEVICE event.
>>>
>> Thanks. I will cover this case using the below sequence.
>>
>> 1. bus_register_notifier()
>> 2. i2c_for_each_dev()
>>
>> If at all i2c adapter is registered by the time
>> bus_register_notifier() is called, it should be detected in
>> i2c_for_each_dev(). I will add checks to avoid creation of duplicate
>> i2c_client devices when both notifier callback and i2c_for_each_dev()
>> passes especially when i2c adapter gets added between 1 and 2.
>>
>> Please suggest if there is an alternate better approach that I should
>> use to handle this case.
>>
> This approach seems good to me, i2c-dev uses something similar.
>
Thanks Armin for confirming and providing the reference. I will address
the comments and submit the patch V8 soon.
> Thanks,
> Armin Wolf
>
>>>> +}
>>>> +
>>>> +static struct amdisp_platform *prepare_amdisp_platform(struct
>>>> device *dev,
>>>> + const struct
>>>> amdisp_platform *src)
>>>> +{
>>>> + struct amdisp_platform *isp_ov05c10;
>>>> + struct i2c_board_info *info;
>>>> + int ret;
>>>> +
>>>> + isp_ov05c10 = devm_kzalloc(dev, sizeof(*isp_ov05c10),
>>>> GFP_KERNEL);
>>>> + if (!isp_ov05c10)
>>>> + return ERR_PTR(-ENOMEM);
>>>> + memcpy(isp_ov05c10, src, sizeof(*isp_ov05c10));
>>>
>>> This is not what i meant. I was complaining that
>>> amdisp_ov05c10_platform_config contains both
>>> static data (swnodes) and data assigned during runtime (board_info-
>>> >swnode, i2c_dev, ...).
>>>
>>> Please do not use a global instance of struct amdisp_platform for
>>> initialization. Instead initialize a
>>> fresh instance of this struct inside prepare_amdisp_platform().
>>>
>> sure, will remove the global variable
>> 'amdisp_ov05c10_platform_config', and will take care of initializing
>> the amdisp_platform instance in the prepare_amdisp_platform().
>>
>>>> +
>>>> + info = &isp_ov05c10->board_info;
>>>> +
>>>> + ret = software_node_register_node_group(src->swnodes);
>>>> + if (ret)
>>>> + return ERR_PTR(ret);
>>>> +
>>>> + info->swnode = src->swnodes[0];
>>>> +
>>>> + return isp_ov05c10;
>>>> +}
>>>> +
>>>> +static int amd_isp_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct amdisp_platform *ov05c10;
>>>> + int ret;
>>>> +
>>>> + ov05c10 = prepare_amdisp_platform(&pdev->dev,
>>>> &amdisp_ov05c10_platform_config);
>>>> + if (IS_ERR(ov05c10))
>>>> + return dev_err_probe(&pdev->dev, PTR_ERR(ov05c10),
>>>> + "failed to prepare AMD ISP
>>>> platform fwnode\n");
>>>> +
>>>> + ov05c10->i2c_nb.notifier_call = isp_i2c_bus_notify;
>>>> + ret = bus_register_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
>>>> + if (ret)
>>>> + return ret;
>>>
>>> You need to call software_node_unregister_node_group() here when
>>> bus_register_notifier() fails.
>>>
>> Thanks. I will fix this in the next V8 patch.
>>
>>> Thanks,
>>> Armin Wolf>
>> Thanks,
>> Pratap
>>
>>>> +
>>>> + platform_set_drvdata(pdev, ov05c10);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void amd_isp_remove(struct platform_device *pdev)
>>>> +{
>>>> + struct amdisp_platform *ov05c10 = platform_get_drvdata(pdev);
>>>> +
>>>> + bus_unregister_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
>>>> + i2c_unregister_device(ov05c10->i2c_dev);
>>>> + software_node_unregister_node_group(ov05c10->swnodes);
>>>> +}
>>>> +
>>>> +static struct platform_driver amd_isp_platform_driver = {
>>>> + .driver = {
>>>> + .name = AMD_ISP_PLAT_DRV_NAME,
>>>> + .acpi_match_table = amdisp_sensor_ids,
>>>> + },
>>>> + .probe = amd_isp_probe,
>>>> + .remove = amd_isp_remove,
>>>> +};
>>>> +
>>>> +module_platform_driver(amd_isp_platform_driver);
>>>> +
>>>> +MODULE_AUTHOR("Benjamin Chan <benjamin.chan@amd.com>");
>>>> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@amd.com>");
>>>> +MODULE_DESCRIPTION("AMD ISP4 Platform Driver");
>>>> +MODULE_LICENSE("GPL");
>>
>>
>> Hi, >> >> just a question: when will the CONFIG_AMD_ISP4 symbol be introduced? >> > CONFIG_AMD_ISP4 will be introduced in the V4L2 ISP driver patches. We > are working on isp driver patches and planning to submit once the review > for x86/platform and sensor driver patches completes. > I had a side bar conversation with Pratap on this and that dependency isn't needed right now as no symbols are used from it. The drivers are coming in different subsystems and can be enabled independently. That dependency can be dropped for the next version.
On 4/29/2025 3:30 PM, Mario Limonciello wrote: >>> Hi, >>> >>> just a question: when will the CONFIG_AMD_ISP4 symbol be introduced? >>> >> CONFIG_AMD_ISP4 will be introduced in the V4L2 ISP driver patches. We >> are working on isp driver patches and planning to submit once the >> review for x86/platform and sensor driver patches completes. >> > > I had a side bar conversation with Pratap on this and that dependency > isn't needed right now as no symbols are used from it. > > The drivers are coming in different subsystems and can be enabled > independently. That dependency can be dropped for the next version. Thanks Mario. Yes, this driver has no strict dependency on ISP driver. I will drop this dependency in V8.
On 4/17/2025 1:28 PM, Pratap Nirujogi wrote:
> ISP device specific configuration is not available in ACPI. Add
> swnode graph to configure the missing device properties for the
> OV05C10 camera device supported on amdisp platform.
>
> Add support to create i2c-client dynamically when amdisp i2c
> adapter is available.
>
> Co-developed-by: Benjamin Chan <benjamin.chan@amd.com>
> Signed-off-by: Benjamin Chan <benjamin.chan@amd.com>
> Signed-off-by: Pratap Nirujogi <pratap.nirujogi@amd.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> Changes v6 -> v7:
>
> * Use devm_kzalloc() inplace of kmemdup()
> * Use IS_ERR() inplace of i2c_client_has_driver()
> * Remove the extra cast
>
> drivers/platform/x86/amd/Kconfig | 11 ++
> drivers/platform/x86/amd/Makefile | 1 +
> drivers/platform/x86/amd/amd_isp4.c | 269 ++++++++++++++++++++++++++++
> 3 files changed, 281 insertions(+)
> create mode 100644 drivers/platform/x86/amd/amd_isp4.c
>
> diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/amd/Kconfig
> index c3e086ea64fc..ec755b5fc93c 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
> + tristate "AMD ISP4 platform driver"
> + depends on I2C && X86_64 && ACPI && AMD_ISP4
> + help
> + Platform driver for AMD platforms containing image signal processor
> + gen 4. Provides camera sensor module board information to allow
> + sensor and V4L drivers to work properly.
> +
> + This driver can also be built as a module. If so, the module
> + will be called amd_isp4.
> diff --git a/drivers/platform/x86/amd/Makefile b/drivers/platform/x86/amd/Makefile
> index c6c40bdcbded..b0e284b5d497 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_isp4.o
> diff --git a/drivers/platform/x86/amd/amd_isp4.c b/drivers/platform/x86/amd/amd_isp4.c
> new file mode 100644
> index 000000000000..461a10be5ccd
> --- /dev/null
> +++ b/drivers/platform/x86/amd/amd_isp4.c
> @@ -0,0 +1,269 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AMD ISP platform driver for sensor i2-client instantiation
> + *
> + * Copyright 2025 Advanced Micro Devices, Inc.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/units.h>
> +
> +#define AMDISP_OV05C10_I2C_ADDR 0x10
> +#define AMDISP_OV05C10_PLAT_NAME "amdisp_ov05c10_platform"
> +#define AMDISP_OV05C10_HID "OMNI5C10"
> +#define AMDISP_OV05C10_REMOTE_EP_NAME "ov05c10_isp_4_1_1"
> +#define AMD_ISP_PLAT_DRV_NAME "amd-isp4"
> +
> +/*
> + * AMD ISP platform definition to configure the device properties
> + * missing in the ACPI table.
> + */
> +struct amdisp_platform {
> + const char *name;
> + u8 i2c_addr;
> + u8 max_num_swnodes;
> + struct i2c_board_info board_info;
> + struct notifier_block i2c_nb;
> + struct i2c_client *i2c_dev;
> + const struct software_node **swnodes;
> +};
> +
> +/* Top-level OV05C10 camera node property table */
> +static const struct property_entry ov05c10_camera_props[] = {
> + PROPERTY_ENTRY_U32("clock-frequency", 24 * HZ_PER_MHZ),
> + { }
> +};
> +
> +/* Root AMD ISP OV05C10 camera node definition */
> +static const struct software_node camera_node = {
> + .name = AMDISP_OV05C10_HID,
> + .properties = ov05c10_camera_props,
> +};
> +
> +/*
> + * AMD ISP OV05C10 Ports node definition. No properties defined for
> + * ports node for OV05C10.
> + */
> +static const struct software_node ports = {
> + .name = "ports",
> + .parent = &camera_node,
> +};
> +
> +/*
> + * AMD ISP OV05C10 Port node definition. No properties defined for
> + * port node for OV05C10.
> + */
> +static const struct software_node port_node = {
> + .name = "port@",
> + .parent = &ports,
> +};
> +
> +/*
> + * Remote endpoint AMD ISP node definition. No properties defined for
> + * remote endpoint node for OV05C10.
> + */
> +static const struct software_node remote_ep_isp_node = {
> + .name = AMDISP_OV05C10_REMOTE_EP_NAME,
> +};
> +
> +/*
> + * Remote endpoint reference for isp node included in the
> + * OV05C10 endpoint.
> + */
> +static const struct software_node_ref_args ov05c10_refs[] = {
> + SOFTWARE_NODE_REFERENCE(&remote_ep_isp_node),
> +};
> +
> +/* OV05C supports one single link frequency */
> +static const u64 ov05c10_link_freqs[] = {
> + 925 * HZ_PER_MHZ,
> +};
> +
> +/* OV05C supports only 2-lane configuration */
> +static const u32 ov05c10_data_lanes[] = {
> + 1,
> + 2,
> +};
> +
> +/* OV05C10 endpoint node properties table */
> +static const struct property_entry ov05c10_endpoint_props[] = {
> + PROPERTY_ENTRY_U32("bus-type", 4),
> + PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes", ov05c10_data_lanes,
> + ARRAY_SIZE(ov05c10_data_lanes)),
> + PROPERTY_ENTRY_U64_ARRAY_LEN("link-frequencies", ov05c10_link_freqs,
> + ARRAY_SIZE(ov05c10_link_freqs)),
> + PROPERTY_ENTRY_REF_ARRAY("remote-endpoint", ov05c10_refs),
> + { }
> +};
> +
> +/* AMD ISP endpoint node definition */
> +static const struct software_node endpoint_node = {
> + .name = "endpoint",
> + .parent = &port_node,
> + .properties = ov05c10_endpoint_props,
> +};
> +
> +/*
> + * AMD ISP swnode graph uses 5 nodes and also its relationship is
> + * fixed to align with the structure that v4l2 expects for successful
> + * endpoint fwnode parsing.
> + *
> + * It is only the node property_entries that will vary for each platform
> + * supporting different sensor modules.
> + */
> +#define NUM_SW_NODES 5
> +
> +static const struct software_node *ov05c10_nodes[NUM_SW_NODES + 1] = {
> + &camera_node,
> + &ports,
> + &port_node,
> + &endpoint_node,
> + &remote_ep_isp_node,
> + NULL
> +};
> +
> +/* OV05C10 specific AMD ISP platform configuration */
> +static const struct amdisp_platform amdisp_ov05c10_platform_config = {
> + .name = AMDISP_OV05C10_PLAT_NAME,
> + .board_info = {
> + .dev_name = "ov05c10",
> + I2C_BOARD_INFO("ov05c10", AMDISP_OV05C10_I2C_ADDR),
> + },
> + .i2c_addr = AMDISP_OV05C10_I2C_ADDR,
> + .max_num_swnodes = NUM_SW_NODES,
> + .swnodes = ov05c10_nodes,
> +};
> +
> +static const struct acpi_device_id amdisp_sensor_ids[] = {
> + { AMDISP_OV05C10_HID },
> + { }
> +};
> +MODULE_DEVICE_TABLE(acpi, amdisp_sensor_ids);
> +
> +static inline bool is_isp_i2c_adapter(struct i2c_adapter *adap)
> +{
> + return !strcmp(adap->owner->name, "i2c_designware_amdisp");
> +}
> +
> +static void instantiate_isp_i2c_client(struct amdisp_platform *ov05c10, struct i2c_adapter *adap)
> +{
> + struct i2c_board_info *info = &ov05c10->board_info;
> + struct i2c_client *i2c_dev;
> +
> + if (ov05c10->i2c_dev)
> + return;
> +
> + if (!info->addr) {
> + dev_err(&adap->dev, "invalid i2c_addr 0x%x detected\n", ov05c10->i2c_addr);
> + return;
> + }
> +
> + i2c_dev = i2c_new_client_device(adap, info);
> + if (IS_ERR(i2c_dev)) {
> + dev_err(&adap->dev, "error %pe registering isp i2c_client\n", i2c_dev);
> + return;
> + }
> + ov05c10->i2c_dev = i2c_dev;
> +}
> +
> +static int isp_i2c_bus_notify(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct amdisp_platform *ov05c10 = container_of(nb, struct amdisp_platform, i2c_nb);
> + struct device *dev = data;
> + struct i2c_client *client;
> + struct i2c_adapter *adap;
> +
> + switch (action) {
> + case BUS_NOTIFY_ADD_DEVICE:
> + adap = i2c_verify_adapter(dev);
> + if (!adap)
> + break;
> + if (is_isp_i2c_adapter(adap))
> + instantiate_isp_i2c_client(ov05c10, adap);
> + break;
> + case BUS_NOTIFY_REMOVED_DEVICE:
> + client = i2c_verify_client(dev);
> + if (!client)
> + break;
> + if (ov05c10->i2c_dev == client) {
> + dev_dbg(&client->adapter->dev, "amdisp i2c_client removed\n");
> + ov05c10->i2c_dev = NULL;
> + }
> + break;
> + default:
> + break;
> + }
> +
> + return NOTIFY_DONE;
> +}
> +
> +static struct amdisp_platform *prepare_amdisp_platform(struct device *dev,
> + const struct amdisp_platform *src)
> +{
> + struct amdisp_platform *isp_ov05c10;
> + struct i2c_board_info *info;
> + int ret;
> +
> + isp_ov05c10 = devm_kzalloc(dev, sizeof(*isp_ov05c10), GFP_KERNEL);
> + if (!isp_ov05c10)
> + return ERR_PTR(-ENOMEM);
> + memcpy(isp_ov05c10, src, sizeof(*isp_ov05c10));
> +
> + info = &isp_ov05c10->board_info;
> +
> + ret = software_node_register_node_group(src->swnodes);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + info->swnode = src->swnodes[0];
> +
> + return isp_ov05c10;
> +}
> +
> +static int amd_isp_probe(struct platform_device *pdev)
> +{
> + struct amdisp_platform *ov05c10;
> + int ret;
> +
> + ov05c10 = prepare_amdisp_platform(&pdev->dev, &amdisp_ov05c10_platform_config);
> + if (IS_ERR(ov05c10))
> + return dev_err_probe(&pdev->dev, PTR_ERR(ov05c10),
> + "failed to prepare AMD ISP platform fwnode\n");
> +
> + ov05c10->i2c_nb.notifier_call = isp_i2c_bus_notify;
> + ret = bus_register_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
> + if (ret)
> + return ret;
> +
> + platform_set_drvdata(pdev, ov05c10);
> + return 0;
> +}
> +
> +static void amd_isp_remove(struct platform_device *pdev)
> +{
> + struct amdisp_platform *ov05c10 = platform_get_drvdata(pdev);
> +
> + bus_unregister_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
> + i2c_unregister_device(ov05c10->i2c_dev);
> + software_node_unregister_node_group(ov05c10->swnodes);
> +}
> +
> +static struct platform_driver amd_isp_platform_driver = {
> + .driver = {
> + .name = AMD_ISP_PLAT_DRV_NAME,
> + .acpi_match_table = amdisp_sensor_ids,
> + },
> + .probe = amd_isp_probe,
> + .remove = amd_isp_remove,
> +};
> +
> +module_platform_driver(amd_isp_platform_driver);
> +
> +MODULE_AUTHOR("Benjamin Chan <benjamin.chan@amd.com>");
> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@amd.com>");
> +MODULE_DESCRIPTION("AMD ISP4 Platform Driver");
> +MODULE_LICENSE("GPL");
© 2016 - 2026 Red Hat, Inc.