[PATCH v5] platform/x86: Add AMD ISP platform config for OV05C10

[PATCH v5] platform/x86: Add AMD ISP platform config for OV05C10

[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 v4 -> v5:

* Avoid global variables and make driver reentrant following the
state container design pattern.

* Fix coding error referencing i2c_dev variable of amdisp_platform
in instantiate_isp_i2c_client().

* Remove i2c_put_adapter(). Not required as i2c_get_adapter() is not called.

* Use i2c_board_info->swnode instead of fwnode to fix refcount imbalance
warnings when module is removed.

* Address review comments.

 drivers/platform/x86/amd/Kconfig    |  11 ++
 drivers/platform/x86/amd/Makefile   |   1 +
 drivers/platform/x86/amd/amd_isp4.c | 295 ++++++++++++++++++++++++++++
 3 files changed, 307 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..ad181ab96423
--- /dev/null
+++ b/drivers/platform/x86/amd/amd_isp4.c
@@ -0,0 +1,295 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * AMD ISP platform driver for sensor i2-client instantiation
+ *
+ * Copyright 2025 Advanced Micro Devices, Inc.
+ */
+
+#include <linux/acpi.h>
+#include <linux/device/bus.h>
+#include <linux/dmi.h>
+#include <linux/gpio/machine.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.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;
+	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 = (struct software_node **)ov05c10_nodes,
+};
+
+static const struct acpi_device_id amdisp_sensor_ids[] = {
+	{ AMDISP_OV05C10_HID },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, amdisp_sensor_ids);
+
+static 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(const struct amdisp_platform *src)
+{
+	struct amdisp_platform *isp_ov05c10;
+	const struct software_node **sw_nodes;
+	const struct software_node *sw_node;
+	struct i2c_board_info *info;
+	int ret;
+
+	isp_ov05c10 = kmemdup(src, sizeof(*isp_ov05c10), GFP_KERNEL);
+	if (!isp_ov05c10)
+		return ERR_PTR(-ENOMEM);
+
+	info = &isp_ov05c10->board_info;
+
+	sw_nodes = (const struct software_node **)src->swnodes;
+	ret = software_node_register_node_group(sw_nodes);
+	if (ret)
+		goto error_unregister_sw_node;
+
+	sw_node = (const struct software_node *)src->swnodes[0];
+	info->swnode = sw_node;
+
+	return isp_ov05c10;
+
+error_unregister_sw_node:
+	software_node_unregister_node_group(sw_nodes);
+	kfree(isp_ov05c10);
+	return ERR_PTR(ret);
+}
+
+static int amd_isp_probe(struct platform_device *pdev)
+{
+	struct amdisp_platform *ov05c10;
+	int ret;
+
+	ov05c10 = prepare_amdisp_platform(&amdisp_ov05c10_platform_config);
+	if (IS_ERR(ov05c10))
+		return dev_err_probe(&pdev->dev, PTR_ERR(ov05c10),
+				     "failed to prepare amdisp platform fw node\n");
+
+	ov05c10->i2c_nb.notifier_call = isp_i2c_bus_notify;
+
+	ret = bus_register_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
+	if (ret)
+		goto error_free_platform;
+
+	platform_set_drvdata(pdev, ov05c10);
+	return ret;
+
+error_free_platform:
+	kfree(ov05c10);
+	return ret;
+}
+
+static void amd_isp_remove(struct platform_device *pdev)
+{
+	struct amdisp_platform *ov05c10 = platform_get_drvdata(pdev);
+
+	if (IS_ERR_OR_NULL(ov05c10))
+		return;
+
+	bus_unregister_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
+	i2c_unregister_device(ov05c10->i2c_dev);
+	software_node_unregister_node_group((const struct software_node **)
+					    ov05c10->swnodes);
+	kfree(ov05c10);
+}
+
+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.0

Re: [PATCH v5] platform/x86: Add AMD ISP platform config for OV05C10

[Ilpo Järvinen]

On Tue, 8 Apr 2025, 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>
> ---
> Changes v4 -> v5:
> 
> * Avoid global variables and make driver reentrant following the
> state container design pattern.
> 
> * Fix coding error referencing i2c_dev variable of amdisp_platform
> in instantiate_isp_i2c_client().
> 
> * Remove i2c_put_adapter(). Not required as i2c_get_adapter() is not called.
> 
> * Use i2c_board_info->swnode instead of fwnode to fix refcount imbalance
> warnings when module is removed.
> 
> * Address review comments.
> 
>  drivers/platform/x86/amd/Kconfig    |  11 ++
>  drivers/platform/x86/amd/Makefile   |   1 +
>  drivers/platform/x86/amd/amd_isp4.c | 295 ++++++++++++++++++++++++++++
>  3 files changed, 307 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..ad181ab96423
> --- /dev/null
> +++ b/drivers/platform/x86/amd/amd_isp4.c
> @@ -0,0 +1,295 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AMD ISP platform driver for sensor i2-client instantiation
> + *
> + * Copyright 2025 Advanced Micro Devices, Inc.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/device/bus.h>
> +#include <linux/dmi.h>
> +#include <linux/gpio/machine.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.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;
> +	struct software_node **swnodes;

Can't this too be const struct so you can avoid a few casts below?

> +};
> +
> +/* 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 = (struct software_node **)ov05c10_nodes,
> +};
> +
> +static const struct acpi_device_id amdisp_sensor_ids[] = {
> +	{ AMDISP_OV05C10_HID },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, amdisp_sensor_ids);
> +
> +static 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);

Put on a single line, you've longer lines already than what this is.

> +		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);

Ditto.

> +		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(const struct amdisp_platform *src)
> +{
> +	struct amdisp_platform *isp_ov05c10;
> +	const struct software_node **sw_nodes;
> +	const struct software_node *sw_node;
> +	struct i2c_board_info *info;
> +	int ret;
> +
> +	isp_ov05c10 = kmemdup(src, sizeof(*isp_ov05c10), GFP_KERNEL);
> +	if (!isp_ov05c10)
> +		return ERR_PTR(-ENOMEM);
> +
> +	info = &isp_ov05c10->board_info;
> +
> +	sw_nodes = (const struct software_node **)src->swnodes;
> +	ret = software_node_register_node_group(sw_nodes);
> +	if (ret)
> +		goto error_unregister_sw_node;
> +
> +	sw_node = (const struct software_node *)src->swnodes[0];
> +	info->swnode = sw_node;
> +
> +	return isp_ov05c10;
> +
> +error_unregister_sw_node:
> +	software_node_unregister_node_group(sw_nodes);

If register failed, why you are calling unregister for it?? 

> +	kfree(isp_ov05c10);
> +	return ERR_PTR(ret);
> +}
> +
> +static int amd_isp_probe(struct platform_device *pdev)
> +{
> +	struct amdisp_platform *ov05c10;
> +	int ret;
> +
> +	ov05c10 = prepare_amdisp_platform(&amdisp_ov05c10_platform_config);
> +	if (IS_ERR(ov05c10))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(ov05c10),
> +				     "failed to prepare amdisp platform fw node\n");

fwnode

Please also capitalize properly as this is user visible message, AMD ISP ?

> +
> +	ov05c10->i2c_nb.notifier_call = isp_i2c_bus_notify;
> +
> +	ret = bus_register_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
> +	if (ret)
> +		goto error_free_platform;
> +
> +	platform_set_drvdata(pdev, ov05c10);
> +	return ret;

return 0

> +
> +error_free_platform:
> +	kfree(ov05c10);
> +	return ret;
> +}
> +
> +static void amd_isp_remove(struct platform_device *pdev)
> +{
> +	struct amdisp_platform *ov05c10 = platform_get_drvdata(pdev);
> +
> +	if (IS_ERR_OR_NULL(ov05c10))

How can this happen??

> +		return;
> +
> +	bus_unregister_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
> +	i2c_unregister_device(ov05c10->i2c_dev);
> +	software_node_unregister_node_group((const struct software_node **)

Unnecessary cast?

> +					    ov05c10->swnodes);
> +	kfree(ov05c10);
> +}
> +
> +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");
> 

-- 
 i.

Re: [PATCH v5] platform/x86: Add AMD ISP platform config for OV05C10

[Nirujogi, Pratap]

Hi Ilpo,

Thanks for the review feedback.

Thanks,
Pratap

On 4/15/2025 11:42 AM, Ilpo Järvinen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Tue, 8 Apr 2025, 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>
>> ---
>> Changes v4 -> v5:
>>
>> * Avoid global variables and make driver reentrant following the
>> state container design pattern.
>>
>> * Fix coding error referencing i2c_dev variable of amdisp_platform
>> in instantiate_isp_i2c_client().
>>
>> * Remove i2c_put_adapter(). Not required as i2c_get_adapter() is not called.
>>
>> * Use i2c_board_info->swnode instead of fwnode to fix refcount imbalance
>> warnings when module is removed.
>>
>> * Address review comments.
>>
>>   drivers/platform/x86/amd/Kconfig    |  11 ++
>>   drivers/platform/x86/amd/Makefile   |   1 +
>>   drivers/platform/x86/amd/amd_isp4.c | 295 ++++++++++++++++++++++++++++
>>   3 files changed, 307 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..ad181ab96423
>> --- /dev/null
>> +++ b/drivers/platform/x86/amd/amd_isp4.c
>> @@ -0,0 +1,295 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * AMD ISP platform driver for sensor i2-client instantiation
>> + *
>> + * Copyright 2025 Advanced Micro Devices, Inc.
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/device/bus.h>
>> +#include <linux/dmi.h>
>> +#include <linux/gpio/machine.h>
>> +#include <linux/init.h>
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mod_devicetable.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;
>> +     struct software_node **swnodes;
> 
> Can't this too be const struct so you can avoid a few casts below?
> 
Thanks. I will make it a const and remove the unnecessary casts in V6.

>> +};
>> +
>> +/* 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 = (struct software_node **)ov05c10_nodes,
>> +};
>> +
>> +static const struct acpi_device_id amdisp_sensor_ids[] = {
>> +     { AMDISP_OV05C10_HID },
>> +     { }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, amdisp_sensor_ids);
>> +
>> +static 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);
> 
> Put on a single line, you've longer lines already than what this is.
> 
sure, will fix this in V6.

>> +             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);
> 
> Ditto.
> 
sure, will fix this in V6.

>> +             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(const struct amdisp_platform *src)
>> +{
>> +     struct amdisp_platform *isp_ov05c10;
>> +     const struct software_node **sw_nodes;
>> +     const struct software_node *sw_node;
>> +     struct i2c_board_info *info;
>> +     int ret;
>> +
>> +     isp_ov05c10 = kmemdup(src, sizeof(*isp_ov05c10), GFP_KERNEL);
>> +     if (!isp_ov05c10)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     info = &isp_ov05c10->board_info;
>> +
>> +     sw_nodes = (const struct software_node **)src->swnodes;
>> +     ret = software_node_register_node_group(sw_nodes);
>> +     if (ret)
>> +             goto error_unregister_sw_node;
>> +
>> +     sw_node = (const struct software_node *)src->swnodes[0];
>> +     info->swnode = sw_node;
>> +
>> +     return isp_ov05c10;
>> +
>> +error_unregister_sw_node:
>> +     software_node_unregister_node_group(sw_nodes);
> 
> If register failed, why you are calling unregister for it??
> 
This is not intended, thanks for catching this, will fix it in V6.


>> +     kfree(isp_ov05c10);
>> +     return ERR_PTR(ret);
>> +}
>> +
>> +static int amd_isp_probe(struct platform_device *pdev)
>> +{
>> +     struct amdisp_platform *ov05c10;
>> +     int ret;
>> +
>> +     ov05c10 = prepare_amdisp_platform(&amdisp_ov05c10_platform_config);
>> +     if (IS_ERR(ov05c10))
>> +             return dev_err_probe(&pdev->dev, PTR_ERR(ov05c10),
>> +                                  "failed to prepare amdisp platform fw node\n");
> 
> fwnode
> 
> Please also capitalize properly as this is user visible message, AMD ISP ?
> 
sure, I will capitalize the words and fix the spacing as suggested in V6.

>> +
>> +     ov05c10->i2c_nb.notifier_call = isp_i2c_bus_notify;
>> +
>> +     ret = bus_register_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
>> +     if (ret)
>> +             goto error_free_platform;
>> +
>> +     platform_set_drvdata(pdev, ov05c10);
>> +     return ret;
> 
> return 0
> 
>> +
>> +error_free_platform:
>> +     kfree(ov05c10);
>> +     return ret;
>> +}
>> +
>> +static void amd_isp_remove(struct platform_device *pdev)
>> +{
>> +     struct amdisp_platform *ov05c10 = platform_get_drvdata(pdev);
>> +
>> +     if (IS_ERR_OR_NULL(ov05c10))
> 
> How can this happen??
> 
I agree this can never happen in the current driver. The only 
possibility that it can happen is when platform_set_drvdata() is missed 
in the probe().

>> +             return;
>> +
>> +     bus_unregister_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
>> +     i2c_unregister_device(ov05c10->i2c_dev);
>> +     software_node_unregister_node_group((const struct software_node **)
> 
> Unnecessary cast?
> 
agreed, will remove this in V6.
>> +                                         ov05c10->swnodes);
>> +     kfree(ov05c10);
>> +}
>> +
>> +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");
>>
> 
> --
>   i.
> 

Re: [PATCH v5] platform/x86: Add AMD ISP platform config for OV05C10

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 14383 bytes --]

On Tue, 15 Apr 2025, Nirujogi, Pratap wrote:

> Hi Ilpo,
> 
> Thanks for the review feedback.
> 
> Thanks,
> Pratap
> 
> On 4/15/2025 11:42 AM, Ilpo Järvinen wrote:
> > Caution: This message originated from an External Source. Use proper caution
> > when opening attachments, clicking links, or responding.
> > 
> > 
> > On Tue, 8 Apr 2025, 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>
> > > ---
> > > Changes v4 -> v5:
> > > 
> > > * Avoid global variables and make driver reentrant following the
> > > state container design pattern.
> > > 
> > > * Fix coding error referencing i2c_dev variable of amdisp_platform
> > > in instantiate_isp_i2c_client().
> > > 
> > > * Remove i2c_put_adapter(). Not required as i2c_get_adapter() is not
> > > called.
> > > 
> > > * Use i2c_board_info->swnode instead of fwnode to fix refcount imbalance
> > > warnings when module is removed.
> > > 
> > > * Address review comments.
> > > 
> > >   drivers/platform/x86/amd/Kconfig    |  11 ++
> > >   drivers/platform/x86/amd/Makefile   |   1 +
> > >   drivers/platform/x86/amd/amd_isp4.c | 295 ++++++++++++++++++++++++++++
> > >   3 files changed, 307 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..ad181ab96423
> > > --- /dev/null
> > > +++ b/drivers/platform/x86/amd/amd_isp4.c
> > > @@ -0,0 +1,295 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * AMD ISP platform driver for sensor i2-client instantiation
> > > + *
> > > + * Copyright 2025 Advanced Micro Devices, Inc.
> > > + */
> > > +
> > > +#include <linux/acpi.h>
> > > +#include <linux/device/bus.h>
> > > +#include <linux/dmi.h>
> > > +#include <linux/gpio/machine.h>
> > > +#include <linux/init.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/mod_devicetable.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;
> > > +     struct software_node **swnodes;
> > 
> > Can't this too be const struct so you can avoid a few casts below?
> > 
> Thanks. I will make it a const and remove the unnecessary casts in V6.
> 
> > > +};
> > > +
> > > +/* 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 = (struct software_node **)ov05c10_nodes,
> > > +};
> > > +
> > > +static const struct acpi_device_id amdisp_sensor_ids[] = {
> > > +     { AMDISP_OV05C10_HID },
> > > +     { }
> > > +};
> > > +MODULE_DEVICE_TABLE(acpi, amdisp_sensor_ids);
> > > +
> > > +static 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);
> > 
> > Put on a single line, you've longer lines already than what this is.
> > 
> sure, will fix this in V6.
> 
> > > +             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);
> > 
> > Ditto.
> > 
> sure, will fix this in V6.
> 
> > > +             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(const struct
> > > amdisp_platform *src)
> > > +{
> > > +     struct amdisp_platform *isp_ov05c10;
> > > +     const struct software_node **sw_nodes;
> > > +     const struct software_node *sw_node;
> > > +     struct i2c_board_info *info;
> > > +     int ret;
> > > +
> > > +     isp_ov05c10 = kmemdup(src, sizeof(*isp_ov05c10), GFP_KERNEL);
> > > +     if (!isp_ov05c10)
> > > +             return ERR_PTR(-ENOMEM);
> > > +
> > > +     info = &isp_ov05c10->board_info;
> > > +
> > > +     sw_nodes = (const struct software_node **)src->swnodes;
> > > +     ret = software_node_register_node_group(sw_nodes);
> > > +     if (ret)
> > > +             goto error_unregister_sw_node;
> > > +
> > > +     sw_node = (const struct software_node *)src->swnodes[0];
> > > +     info->swnode = sw_node;
> > > +
> > > +     return isp_ov05c10;
> > > +
> > > +error_unregister_sw_node:
> > > +     software_node_unregister_node_group(sw_nodes);
> > 
> > If register failed, why you are calling unregister for it??
> > 
> This is not intended, thanks for catching this, will fix it in V6.
> 
> 
> > > +     kfree(isp_ov05c10);
> > > +     return ERR_PTR(ret);
> > > +}
> > > +
> > > +static int amd_isp_probe(struct platform_device *pdev)
> > > +{
> > > +     struct amdisp_platform *ov05c10;
> > > +     int ret;
> > > +
> > > +     ov05c10 = prepare_amdisp_platform(&amdisp_ov05c10_platform_config);
> > > +     if (IS_ERR(ov05c10))
> > > +             return dev_err_probe(&pdev->dev, PTR_ERR(ov05c10),
> > > +                                  "failed to prepare amdisp platform fw
> > > node\n");
> > 
> > fwnode
> > 
> > Please also capitalize properly as this is user visible message, AMD ISP ?
> > 
> sure, I will capitalize the words and fix the spacing as suggested in V6.
> 
> > > +
> > > +     ov05c10->i2c_nb.notifier_call = isp_i2c_bus_notify;
> > > +
> > > +     ret = bus_register_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
> > > +     if (ret)
> > > +             goto error_free_platform;
> > > +
> > > +     platform_set_drvdata(pdev, ov05c10);
> > > +     return ret;
> > 
> > return 0
> > 
> > > +
> > > +error_free_platform:
> > > +     kfree(ov05c10);
> > > +     return ret;
> > > +}
> > > +
> > > +static void amd_isp_remove(struct platform_device *pdev)
> > > +{
> > > +     struct amdisp_platform *ov05c10 = platform_get_drvdata(pdev);
> > > +
> > > +     if (IS_ERR_OR_NULL(ov05c10))
> > 
> > How can this happen??
> > 
> I agree this can never happen in the current driver. The only possibility that
> it can happen is when platform_set_drvdata() is missed in the probe().

Just remove the unnecessary check then.

-- 
 i.

Re: [PATCH v5] platform/x86: Add AMD ISP platform config for OV05C10

[Nirujogi, Pratap]

On 4/15/2025 1:49 PM, Ilpo Järvinen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Tue, 15 Apr 2025, Nirujogi, Pratap wrote:
> 
>> Hi Ilpo,
>>
>> Thanks for the review feedback.
>>
>> Thanks,
>> Pratap
>>
>> On 4/15/2025 11:42 AM, Ilpo Järvinen wrote:
>>> Caution: This message originated from an External Source. Use proper caution
>>> when opening attachments, clicking links, or responding.
>>>
>>>
>>> On Tue, 8 Apr 2025, 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>
>>>> ---
>>>> Changes v4 -> v5:
>>>>
>>>> * Avoid global variables and make driver reentrant following the
>>>> state container design pattern.
>>>>
>>>> * Fix coding error referencing i2c_dev variable of amdisp_platform
>>>> in instantiate_isp_i2c_client().
>>>>
>>>> * Remove i2c_put_adapter(). Not required as i2c_get_adapter() is not
>>>> called.
>>>>
>>>> * Use i2c_board_info->swnode instead of fwnode to fix refcount imbalance
>>>> warnings when module is removed.
>>>>
>>>> * Address review comments.
>>>>
>>>>    drivers/platform/x86/amd/Kconfig    |  11 ++
>>>>    drivers/platform/x86/amd/Makefile   |   1 +
>>>>    drivers/platform/x86/amd/amd_isp4.c | 295 ++++++++++++++++++++++++++++
>>>>    3 files changed, 307 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..ad181ab96423
>>>> --- /dev/null
>>>> +++ b/drivers/platform/x86/amd/amd_isp4.c
>>>> @@ -0,0 +1,295 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * AMD ISP platform driver for sensor i2-client instantiation
>>>> + *
>>>> + * Copyright 2025 Advanced Micro Devices, Inc.
>>>> + */
>>>> +
>>>> +#include <linux/acpi.h>
>>>> +#include <linux/device/bus.h>
>>>> +#include <linux/dmi.h>
>>>> +#include <linux/gpio/machine.h>
>>>> +#include <linux/init.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/mod_devicetable.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;
>>>> +     struct software_node **swnodes;
>>>
>>> Can't this too be const struct so you can avoid a few casts below?
>>>
>> Thanks. I will make it a const and remove the unnecessary casts in V6.
>>
>>>> +};
>>>> +
>>>> +/* 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 = (struct software_node **)ov05c10_nodes,
>>>> +};
>>>> +
>>>> +static const struct acpi_device_id amdisp_sensor_ids[] = {
>>>> +     { AMDISP_OV05C10_HID },
>>>> +     { }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(acpi, amdisp_sensor_ids);
>>>> +
>>>> +static 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);
>>>
>>> Put on a single line, you've longer lines already than what this is.
>>>
>> sure, will fix this in V6.
>>
>>>> +             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);
>>>
>>> Ditto.
>>>
>> sure, will fix this in V6.
>>
>>>> +             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(const struct
>>>> amdisp_platform *src)
>>>> +{
>>>> +     struct amdisp_platform *isp_ov05c10;
>>>> +     const struct software_node **sw_nodes;
>>>> +     const struct software_node *sw_node;
>>>> +     struct i2c_board_info *info;
>>>> +     int ret;
>>>> +
>>>> +     isp_ov05c10 = kmemdup(src, sizeof(*isp_ov05c10), GFP_KERNEL);
>>>> +     if (!isp_ov05c10)
>>>> +             return ERR_PTR(-ENOMEM);
>>>> +
>>>> +     info = &isp_ov05c10->board_info;
>>>> +
>>>> +     sw_nodes = (const struct software_node **)src->swnodes;
>>>> +     ret = software_node_register_node_group(sw_nodes);
>>>> +     if (ret)
>>>> +             goto error_unregister_sw_node;
>>>> +
>>>> +     sw_node = (const struct software_node *)src->swnodes[0];
>>>> +     info->swnode = sw_node;
>>>> +
>>>> +     return isp_ov05c10;
>>>> +
>>>> +error_unregister_sw_node:
>>>> +     software_node_unregister_node_group(sw_nodes);
>>>
>>> If register failed, why you are calling unregister for it??
>>>
>> This is not intended, thanks for catching this, will fix it in V6.
>>
>>
>>>> +     kfree(isp_ov05c10);
>>>> +     return ERR_PTR(ret);
>>>> +}
>>>> +
>>>> +static int amd_isp_probe(struct platform_device *pdev)
>>>> +{
>>>> +     struct amdisp_platform *ov05c10;
>>>> +     int ret;
>>>> +
>>>> +     ov05c10 = prepare_amdisp_platform(&amdisp_ov05c10_platform_config);
>>>> +     if (IS_ERR(ov05c10))
>>>> +             return dev_err_probe(&pdev->dev, PTR_ERR(ov05c10),
>>>> +                                  "failed to prepare amdisp platform fw
>>>> node\n");
>>>
>>> fwnode
>>>
>>> Please also capitalize properly as this is user visible message, AMD ISP ?
>>>
>> sure, I will capitalize the words and fix the spacing as suggested in V6.
>>
>>>> +
>>>> +     ov05c10->i2c_nb.notifier_call = isp_i2c_bus_notify;
>>>> +
>>>> +     ret = bus_register_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
>>>> +     if (ret)
>>>> +             goto error_free_platform;
>>>> +
>>>> +     platform_set_drvdata(pdev, ov05c10);
>>>> +     return ret;
>>>
>>> return 0
>>>
>>>> +
>>>> +error_free_platform:
>>>> +     kfree(ov05c10);
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +static void amd_isp_remove(struct platform_device *pdev)
>>>> +{
>>>> +     struct amdisp_platform *ov05c10 = platform_get_drvdata(pdev);
>>>> +
>>>> +     if (IS_ERR_OR_NULL(ov05c10))
>>>
>>> How can this happen??
>>>
>> I agree this can never happen in the current driver. The only possibility that
>> it can happen is when platform_set_drvdata() is missed in the probe().
> 
> Just remove the unnecessary check then.
> 
ok sure, will remove this check in V6.

> --
>   i.