Message-ID: <1e0f68b4-4558-409e-92dd-d46d85ca0139@amd.com>
Date: Mon, 31 Mar 2025 15:09:21 -0400
From: "Nirujogi, Pratap" <pnirujog@....com>
To: Mario Limonciello <mario.limonciello@....com>,
 Pratap Nirujogi <pratap.nirujogi@....com>, hdegoede@...hat.com,
 ilpo.jarvinen@...ux.intel.com
Cc: platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
 benjamin.chan@....com, bin.du@....com, grosikop@....com, king.li@....com,
 dantony@....com
Subject: Re: [PATCH v3] platform/x86: Add AMD ISP platform config for OV05C10

Hi Mario,

Thanks for the review.

Thanks,
Pratap

On 3/27/2025 8:43 PM, Mario Limonciello wrote:
> On 3/27/2025 17:11, 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.
>>
>> Signed-off-by: Pratap Nirujogi <pratap.nirujogi@....com>
>> Signed-off-by: Benjamin Chan <benjamin.chan@....com>
>> ---
>> v2 -> v3:
>>
>> - Remodelled as ACPI driver to bind the device when matching
>> acpi sensor device is detected. This eliminated the need for
>> dmi checks for platform validation.
>>
>> - Added software nodes to align with camera device-tree format
>> to capture the camera device properties missing in the ACPI.
>>
>> - Constructed swnode graph in the below format to successfully
>> parse the endpoint node using v4l2_fwnode_endpoint_alloc_parse().
>>
>> OMNI5C10 {
>>     refclk = 24MHz;
>>     ports {
>>         port@ {
>>             endpoint {
>>                 bus-type = 4;
>>                 data-lanes = <1 2>;
>>                 link-frequencies = 925MHz;
>>                 remote-endpoint = &ov05c10_refs;
>>             };
>>         };
>>     };
>> };
>>
>> - GPIO sensor lookup is moved out of this driver scope, it is taken care
>> in isp driver module init.
>>
>> - This platform driver is specific to AMD ISP4, hence renamed driver
>> name as "amd-isp4".
>>
>>   drivers/platform/x86/amd/Kconfig    |  11 +
>>   drivers/platform/x86/amd/Makefile   |   1 +
>>   drivers/platform/x86/amd/amd_isp4.c | 354 ++++++++++++++++++++++++++++
>>   3 files changed, 366 insertions(+)
>>   create mode 100644 drivers/platform/x86/amd/amd_isp4.c
> 
> Pratap,
> 
> Overall I like the direction of this improvement.  A few small comments 
> I notice below.
> 
Thanks Mario. Sure, will address them in V4.

>>
>> diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/ 
>> amd/Kconfig
>> index c3e086ea64fc..4b373edd750d 100644
>> --- a/drivers/platform/x86/amd/Kconfig
>> +++ b/drivers/platform/x86/amd/Kconfig
>> @@ -32,3 +32,14 @@ config AMD_WBRF
>>         This mechanism will only be activated on platforms that 
>> advertise a
>>         need for it.
>> +
>> +config AMD_ISP_PLATFORM
>> +    bool "AMD platform with ISP4 that supports Camera sensor device"
> 
> Now that this is an ACPI driver this should be tristate because it can 
> now be a module.
> 
Thanks. Will fix this in V4.

>> +    depends on I2C && X86_64 && AMD_ISP4
> 
> Now that this is an ACPI driver there should be a:
> 
> depends on ACPI
> 
>> +    help
>> +      For AMD platform that support Image signal processor generation 
>> 4, it
>> +      is necessary to add platform specific camera sensor module 
>> board info
>> +      which includes the sensor driver device id and the i2c address.
>> +
>> +      If you have a AMD platform that support ISP4 and with a sensor
>> +      connected to it, say Y here
>> diff --git a/drivers/platform/x86/amd/Makefile b/drivers/platform/x86/ 
>> amd/Makefile
>> index 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..c4dce3779543
>> --- /dev/null
>> +++ b/drivers/platform/x86/amd/amd_isp4.c
>> @@ -0,0 +1,354 @@
>> +// 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 properites
> 
> properties
> 
sure, will correct the typo error in V4.

>> + * missing in the ACPI table.
>> + */
>> +struct amdisp_platform {
>> +    const char *name;
>> +    struct i2c_board_info board_info;
>> +    u8 i2c_addr;
>> +    struct i2c_client *i2c_dev;
>> +    u8 max_num_swnodes;
>> +    struct software_node **swnodes;
>> +};
>> +
>> +static struct amdisp_platform *ov05c10_amdisp;
>> +
>> +/* Top-level OV05C10 camera node property table */
>> +static const struct property_entry ov05c10_camera_props[] __initconst 
>> = {
>> +    PROPERTY_ENTRY_U32("refclk", 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[] __initconst 
>> = {
>> +    SOFTWARE_NODE_REFERENCE(&remote_ep_isp_node),
>> +};
>> +
>> +/* OV05C supports one single link frequency */
>> +static const u64 ov05c10_link_freqs[] __initconst = {
>> +    925 * HZ_PER_MHZ,
>> +};
>> +
>> +/* OV05C supports only 2-lane configuration */
>> +static const u32 ov05c10_data_lanes[] __initconst = {
>> +    1,
>> +    2,
>> +};
>> +
>> +/* OV05C10 endpoint node properties table */
>> +static const struct property_entry ov05c10_endpoint_props[] 
>> __initconst = {
>> +    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 !strncmp(adap->owner->name, "i2c_designware_amdisp", 48);
>> +}
>> +
>> +static void instantiate_isp_i2c_client(struct i2c_adapter *adap)
>> +{
>> +    struct i2c_board_info *info = &ov05c10_amdisp->board_info;
>> +    struct i2c_client *i2c_dev = ov05c10_amdisp->i2c_dev;
>> +
>> +    if (i2c_dev)
>> +        return;
>> +
>> +    if (!info->addr) {
>> +        dev_err(&adap->dev, "invalid i2c_addr 0x%x detected\n",
>> +            ov05c10_amdisp->i2c_addr);
>> +        return;
>> +    }
>> +
>> +    i2c_dev = i2c_new_client_device(adap, info);
>> +    if (IS_ERR(i2c_dev)) {
>> +        dev_err(&adap->dev, "error %ld registering isp i2c_client\n",
>> +            PTR_ERR(i2c_dev));
>> +        i2c_dev = NULL;
>> +    } else {
>> +        dev_dbg(&adap->dev, "registered amdisp i2c_client on address 
>> 0x%02x\n",
>> +            info->addr);
>> +    }
>> +    i2c_put_adapter(adap);
>> +}
>> +
>> +static int isp_i2c_bus_notify(struct notifier_block *nb,
>> +                  unsigned long action, void *data)
>> +{
>> +    struct device *dev = data;
>> +    struct i2c_client *client;
>> +    struct i2c_adapter *adap;
>> +    struct i2c_client *i2c_dev = ov05c10_amdisp->i2c_dev;
>> +
>> +    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(adap);
>> +        break;
>> +    case BUS_NOTIFY_REMOVED_DEVICE:
>> +        client = i2c_verify_client(dev);
>> +        if (!client)
>> +            break;
>> +        if (i2c_dev == client) {
>> +            dev_dbg(&client->adapter->dev, "amdisp i2c_client 
>> removed\n");
>> +            i2c_dev = NULL;
>> +        }
>> +        break;
>> +    default:
>> +        break;
>> +    }
>> +
>> +    return NOTIFY_DONE;
>> +}
>> +
>> +static struct notifier_block isp_i2c_nb = { .notifier_call = 
>> isp_i2c_bus_notify };
>> +
>> +static int release_amdisp_swnode_props(struct software_node **swnodes,
>> +                       u8 num_swnodes)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < num_swnodes; i++) {
>> +        if (swnodes[i] &&
>> +            !IS_ERR_OR_NULL(swnodes[i]->properties)) {
>> +            property_entries_free(swnodes[i]->properties);
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int initialize_ov05c10_swnode_props(struct amdisp_platform *p)
>> +{
>> +    struct software_node *sw_node;
>> +    int ret;
>> +    int i;
>> +
>> +    if (strncmp(p->name, AMDISP_OV05C10_PLAT_NAME,
>> +            strlen(AMDISP_OV05C10_PLAT_NAME)))
>> +        return -EINVAL;
>> +
>> +    /* Initialize platform specific props for each node */
>> +    for (i = 0; i < p->max_num_swnodes; i++) {
>> +        sw_node = p->swnodes[i];
>> +
>> +        if (sw_node->properties) {
>> +            sw_node->properties = property_entries_dup(sw_node- 
>> >properties);
>> +            if (IS_ERR(sw_node->properties)) {
>> +                ret = PTR_ERR(sw_node->properties);
>> +                goto error_release_node_props;
>> +            }
>> +        }
>> +    }
>> +
>> +    return 0;
>> +
>> +error_release_node_props:
>> +    release_amdisp_swnode_props(p->swnodes, p->max_num_swnodes);
>> +    return ret;
>> +}
>> +
>> +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);
>> +
>> +    ret = initialize_ov05c10_swnode_props(isp_ov05c10);
>> +    if (ret)
>> +        goto error_free_platform;
>> +
>> +    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->fwnode = software_node_fwnode(sw_node);
>> +    if (IS_ERR(info->fwnode)) {
>> +        ret = PTR_ERR(info->fwnode);
>> +        goto error_unregister_sw_node;
>> +    }
>> +
>> +    return isp_ov05c10;
>> +
>> +error_unregister_sw_node:
>> +    software_node_unregister_node_group(sw_nodes);
>> +
>> +error_free_platform:
>> +    kfree(isp_ov05c10);
>> +    return ERR_PTR(ret);
>> +}
>> +
>> +static int amd_isp_probe(struct platform_device *pdev)
>> +{
>> +    int ret;
>> +
>> +    ov05c10_amdisp = 
>> prepare_amdisp_platform(&amdisp_ov05c10_platform_config);
>> +    if (IS_ERR(ov05c10_amdisp)) {
>> +        pr_err("failed to prepare amdisp platform fw node");
>> +        return PTR_ERR(ov05c10_amdisp);
>> +    }
>> +
>> +    /*
>> +     * Register notifier to instantiate i2c client when AMD ISP adapter
>> +     * is ready.
>> +     */
>> +    ret = bus_register_notifier(&i2c_bus_type, &isp_i2c_nb);
>> +    if (ret)
>> +        goto error_free_platform;
>> +
>> +    return ret;
>> +
>> +error_free_platform:
>> +    kfree(ov05c10_amdisp);
>> +    return ret;
>> +}
>> +
>> +static void amd_isp_remove(struct platform_device *pdev)
>> +{
>> +    bus_unregister_notifier(&i2c_bus_type, &isp_i2c_nb);
>> +    i2c_unregister_device(ov05c10_amdisp->i2c_dev);
>> +    release_amdisp_swnode_props(ov05c10_amdisp->swnodes,
>> +                    ov05c10_amdisp->max_num_swnodes);
>> +    software_node_unregister_node_group((const struct software_node **)
>> +                        ov05c10_amdisp->swnodes);
>> +    kfree(ov05c10_amdisp);
>> +}
>> +
>> +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@....com>");
>> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@....com>");
>> +MODULE_DESCRIPTION("AMD ISP4 Platform Driver");
>> +MODULE_LICENSE("GPL");
> 

Powered by blists - more mailing lists