* Re: [PATCH v6] platform/x86: Add AMD ISP platform config for OV05C10
2025-04-16 19:13 [PATCH v6] platform/x86: Add AMD ISP platform config for OV05C10 Pratap Nirujogi
@ 2025-04-16 19:17 ` Mario Limonciello
2025-04-16 23:22 ` Armin Wolf
2025-04-17 9:29 ` Ilpo Järvinen
2 siblings, 0 replies; 6+ messages in thread
From: Mario Limonciello @ 2025-04-16 19:17 UTC (permalink / raw)
To: Pratap Nirujogi, hdegoede, ilpo.jarvinen, W_Armin
Cc: platform-driver-x86, linux-kernel, benjamin.chan, bin.du,
gjorgji.rosikopulos, king.li, dantony
On 4/16/2025 2:13 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>
Thanks Pratap.
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> Changes v5 -> v6:
>
> * Cleanup header files
> * Avoid unnecessary casts
> * Fix coding error with software_node_unregister_node_group()
> * Use i2c_client_has_driver() to validate i2c client handle
> * Address other cosmetic errors
>
> drivers/platform/x86/amd/Kconfig | 11 ++
> drivers/platform/x86/amd/Makefile | 1 +
> drivers/platform/x86/amd/amd_isp4.c | 278 ++++++++++++++++++++++++++++
> 3 files changed, 290 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..9d1abbcc915f
> --- /dev/null
> +++ b/drivers/platform/x86/amd/amd_isp4.c
> @@ -0,0 +1,278 @@
> +// 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 (!i2c_client_has_driver(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;
> + 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_free_platform;
> +
> + info->swnode = src->swnodes[0];
> +
> + return isp_ov05c10;
> +
> +error_free_platform:
> + 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 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)
> + goto error_free_platform;
> +
> + platform_set_drvdata(pdev, ov05c10);
> + 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);
> +
> + bus_unregister_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
> + i2c_unregister_device(ov05c10->i2c_dev);
> + software_node_unregister_node_group(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");
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v6] platform/x86: Add AMD ISP platform config for OV05C10
2025-04-16 19:13 [PATCH v6] platform/x86: Add AMD ISP platform config for OV05C10 Pratap Nirujogi
2025-04-16 19:17 ` Mario Limonciello
@ 2025-04-16 23:22 ` Armin Wolf
2025-04-17 16:48 ` Nirujogi, Pratap
2025-04-17 9:29 ` Ilpo Järvinen
2 siblings, 1 reply; 6+ messages in thread
From: Armin Wolf @ 2025-04-16 23:22 UTC (permalink / raw)
To: Pratap Nirujogi, hdegoede, ilpo.jarvinen, mario.limonciello
Cc: platform-driver-x86, linux-kernel, benjamin.chan, bin.du,
gjorgji.rosikopulos, king.li, dantony
Am 16.04.25 um 21:13 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 v5 -> v6:
>
> * Cleanup header files
> * Avoid unnecessary casts
> * Fix coding error with software_node_unregister_node_group()
> * Use i2c_client_has_driver() to validate i2c client handle
> * Address other cosmetic errors
>
> drivers/platform/x86/amd/Kconfig | 11 ++
> drivers/platform/x86/amd/Makefile | 1 +
> drivers/platform/x86/amd/amd_isp4.c | 278 ++++++++++++++++++++++++++++
> 3 files changed, 290 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..9d1abbcc915f
> --- /dev/null
> +++ b/drivers/platform/x86/amd/amd_isp4.c
> @@ -0,0 +1,278 @@
> +// 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 (!i2c_client_has_driver(i2c_dev)) {
Hi,
i think you meant to use IS_ERR() here instead of i2c_client_has_driver().
> + 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;
> + struct i2c_board_info *info;
> + int ret;
> +
> + isp_ov05c10 = kmemdup(src, sizeof(*isp_ov05c10), GFP_KERNEL);
> + if (!isp_ov05c10)
> + return ERR_PTR(-ENOMEM);
Please allocate a fresh instance of amd_platform using devm_kzalloc() and perform the initialization
there. Using kmemdup() means that we waste memory by having a useless struct amd_platform in memory.
Thanks,
Armin Wolf
> +
> + 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_free_platform;
> +
> + info->swnode = src->swnodes[0];
> +
> + return isp_ov05c10;
> +
> +error_free_platform:
> + 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 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)
> + goto error_free_platform;
> +
> + platform_set_drvdata(pdev, ov05c10);
> + 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);
> +
> + bus_unregister_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
> + i2c_unregister_device(ov05c10->i2c_dev);
> + software_node_unregister_node_group(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");
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v6] platform/x86: Add AMD ISP platform config for OV05C10
2025-04-16 23:22 ` Armin Wolf
@ 2025-04-17 16:48 ` Nirujogi, Pratap
0 siblings, 0 replies; 6+ messages in thread
From: Nirujogi, Pratap @ 2025-04-17 16:48 UTC (permalink / raw)
To: Armin Wolf, Pratap Nirujogi, hdegoede, ilpo.jarvinen,
mario.limonciello
Cc: platform-driver-x86, linux-kernel, benjamin.chan, bin.du,
gjorgji.rosikopulos, king.li, dantony
Hi Armin,
On 4/16/2025 7:22 PM, Armin Wolf wrote:
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> Am 16.04.25 um 21:13 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 v5 -> v6:
>>
>> * Cleanup header files
>> * Avoid unnecessary casts
>> * Fix coding error with software_node_unregister_node_group()
>> * Use i2c_client_has_driver() to validate i2c client handle
>> * Address other cosmetic errors
>>
>> drivers/platform/x86/amd/Kconfig | 11 ++
>> drivers/platform/x86/amd/Makefile | 1 +
>> drivers/platform/x86/amd/amd_isp4.c | 278 ++++++++++++++++++++++++++++
>> 3 files changed, 290 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..9d1abbcc915f
>> --- /dev/null
>> +++ b/drivers/platform/x86/amd/amd_isp4.c
>> @@ -0,0 +1,278 @@
>> +// 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 (!i2c_client_has_driver(i2c_dev)) {
>
> Hi,
>
> i think you meant to use IS_ERR() here instead of i2c_client_has_driver().
>
sure, will revert to IS_ERR() check in V7.
>> + 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;
>> + struct i2c_board_info *info;
>> + int ret;
>> +
>> + isp_ov05c10 = kmemdup(src, sizeof(*isp_ov05c10), GFP_KERNEL);
>> + if (!isp_ov05c10)
>> + return ERR_PTR(-ENOMEM);
>
> Please allocate a fresh instance of amd_platform using devm_kzalloc()
> and perform the initialization
> there. Using kmemdup() means that we waste memory by having a useless
> struct amd_platform in memory.
>
Thanks. Will update using devm_kzalloc() inplace of kmemdup() in V7.
> Thanks,
> Armin Wolf
>
>> +
>> + 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_free_platform;
>> +
>> + info->swnode = src->swnodes[0];
>> +
>> + return isp_ov05c10;
>> +
>> +error_free_platform:
>> + 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 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)
>> + goto error_free_platform;
>> +
>> + platform_set_drvdata(pdev, ov05c10);
>> + 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);
>> +
>> + bus_unregister_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
>> + i2c_unregister_device(ov05c10->i2c_dev);
>> + software_node_unregister_node_group(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");
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v6] platform/x86: Add AMD ISP platform config for OV05C10
2025-04-16 19:13 [PATCH v6] platform/x86: Add AMD ISP platform config for OV05C10 Pratap Nirujogi
2025-04-16 19:17 ` Mario Limonciello
2025-04-16 23:22 ` Armin Wolf
@ 2025-04-17 9:29 ` Ilpo Järvinen
2025-04-17 16:53 ` Nirujogi, Pratap
2 siblings, 1 reply; 6+ messages in thread
From: Ilpo Järvinen @ 2025-04-17 9:29 UTC (permalink / raw)
To: Pratap Nirujogi
Cc: Hans de Goede, W_Armin, mario.limonciello, platform-driver-x86,
LKML, benjamin.chan, bin.du, gjorgji.rosikopulos, king.li,
dantony
On Wed, 16 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 v5 -> v6:
>
> * Cleanup header files
> * Avoid unnecessary casts
> * Fix coding error with software_node_unregister_node_group()
> * Use i2c_client_has_driver() to validate i2c client handle
> * Address other cosmetic errors
>
> drivers/platform/x86/amd/Kconfig | 11 ++
> drivers/platform/x86/amd/Makefile | 1 +
> drivers/platform/x86/amd/amd_isp4.c | 278 ++++++++++++++++++++++++++++
> 3 files changed, 290 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..9d1abbcc915f
> --- /dev/null
> +++ b/drivers/platform/x86/amd/amd_isp4.c
> @@ -0,0 +1,278 @@
> +// 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 (!i2c_client_has_driver(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;
> + 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;
Did you forget to remove this cast?
--
i.
> + ret = software_node_register_node_group(sw_nodes);
> + if (ret)
> + goto error_free_platform;
> +
> + info->swnode = src->swnodes[0];
> +
> + return isp_ov05c10;
> +
> +error_free_platform:
> + 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 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)
> + goto error_free_platform;
> +
> + platform_set_drvdata(pdev, ov05c10);
> + 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);
> +
> + bus_unregister_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
> + i2c_unregister_device(ov05c10->i2c_dev);
> + software_node_unregister_node_group(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");
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v6] platform/x86: Add AMD ISP platform config for OV05C10
2025-04-17 9:29 ` Ilpo Järvinen
@ 2025-04-17 16:53 ` Nirujogi, Pratap
0 siblings, 0 replies; 6+ messages in thread
From: Nirujogi, Pratap @ 2025-04-17 16:53 UTC (permalink / raw)
To: Ilpo Järvinen, Pratap Nirujogi
Cc: Hans de Goede, W_Armin, mario.limonciello, platform-driver-x86,
LKML, benjamin.chan, bin.du, gjorgji.rosikopulos, king.li,
dantony
Hi Ilpo,
On 4/17/2025 5:29 AM, Ilpo Järvinen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Wed, 16 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 v5 -> v6:
>>
>> * Cleanup header files
>> * Avoid unnecessary casts
>> * Fix coding error with software_node_unregister_node_group()
>> * Use i2c_client_has_driver() to validate i2c client handle
>> * Address other cosmetic errors
>>
>> drivers/platform/x86/amd/Kconfig | 11 ++
>> drivers/platform/x86/amd/Makefile | 1 +
>> drivers/platform/x86/amd/amd_isp4.c | 278 ++++++++++++++++++++++++++++
>> 3 files changed, 290 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..9d1abbcc915f
>> --- /dev/null
>> +++ b/drivers/platform/x86/amd/amd_isp4.c
>> @@ -0,0 +1,278 @@
>> +// 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 (!i2c_client_has_driver(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;
>> + 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;
>
> Did you forget to remove this cast?
>
Oops, sorry I missed this one. Will fix it in V7. Thank you.
> --
> i.
>
>> + ret = software_node_register_node_group(sw_nodes);
>> + if (ret)
>> + goto error_free_platform;
>> +
>> + info->swnode = src->swnodes[0];
>> +
>> + return isp_ov05c10;
>> +
>> +error_free_platform:
>> + 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 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)
>> + goto error_free_platform;
>> +
>> + platform_set_drvdata(pdev, ov05c10);
>> + 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);
>> +
>> + bus_unregister_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
>> + i2c_unregister_device(ov05c10->i2c_dev);
>> + software_node_unregister_node_group(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");
>>
^ permalink raw reply [flat|nested] 6+ messages in thread