Message-ID: <043d2044-f8a0-4fe7-8c98-7ba9cc394b33@amd.com>
Date: Tue, 29 Apr 2025 17:25:42 -0400
From: "Nirujogi, Pratap" <pnirujog@....com>
To: Armin Wolf <W_Armin@....de>, Pratap Nirujogi <pratap.nirujogi@....com>,
 hdegoede@...hat.com, ilpo.jarvinen@...ux.intel.com, mario.limonciello@....com
Cc: platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
 benjamin.chan@....com, bin.du@....com, gjorgji.rosikopulos@....com,
 king.li@....com, dantony@....com
Subject: Re: [PATCH v7] platform/x86: Add AMD ISP platform config for OV05C10



On 4/29/2025 4:52 PM, Armin Wolf wrote:
> Caution: This message originated from an External Source. Use proper 
> caution when opening attachments, clicking links, or responding.
> 
> 
> Am 29.04.25 um 20:33 schrieb Nirujogi, Pratap:
> 
>> Hi Armin,
>>
>> On 4/26/2025 8:25 PM, Armin Wolf wrote:
>>> Caution: This message originated from an External Source. Use proper
>>> caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> Am 17.04.25 um 20:28 schrieb Pratap Nirujogi:
>>>
>>>> ISP device specific configuration is not available in ACPI. Add
>>>> swnode graph to configure the missing device properties for the
>>>> OV05C10 camera device supported on amdisp platform.
>>>>
>>>> Add support to create i2c-client dynamically when amdisp i2c
>>>> adapter is available.
>>>>
>>>> Co-developed-by: Benjamin Chan <benjamin.chan@....com>
>>>> Signed-off-by: Benjamin Chan <benjamin.chan@....com>
>>>> Signed-off-by: Pratap Nirujogi <pratap.nirujogi@....com>
>>>> ---
>>>> Changes v6 -> v7:
>>>>
>>>> * Use devm_kzalloc() inplace of kmemdup()
>>>> * Use IS_ERR() inplace of i2c_client_has_driver()
>>>> * Remove the extra cast
>>>>
>>>>   drivers/platform/x86/amd/Kconfig    |  11 ++
>>>>   drivers/platform/x86/amd/Makefile   |   1 +
>>>>   drivers/platform/x86/amd/amd_isp4.c | 269
>>>> ++++++++++++++++++++++++++++
>>>>   3 files changed, 281 insertions(+)
>>>>   create mode 100644 drivers/platform/x86/amd/amd_isp4.c
>>>>
>>>> diff --git a/drivers/platform/x86/amd/Kconfig
>>>> b/drivers/platform/x86/ amd/Kconfig
>>>> index c3e086ea64fc..ec755b5fc93c 100644
>>>> --- a/drivers/platform/x86/amd/Kconfig
>>>> +++ b/drivers/platform/x86/amd/Kconfig
>>>> @@ -32,3 +32,14 @@ config AMD_WBRF
>>>>
>>>>         This mechanism will only be activated on platforms that
>>>> advertise a
>>>>         need for it.
>>>> +
>>>> +config AMD_ISP_PLATFORM
>>>> +     tristate "AMD ISP4 platform driver"
>>>> +     depends on I2C && X86_64 && ACPI && AMD_ISP4
>>>
>>> Hi,
>>>
>>> just a question: when will the CONFIG_AMD_ISP4 symbol be introduced?
>>>
>> CONFIG_AMD_ISP4 will be introduced in the V4L2 ISP driver patches. We
>> are working on isp driver patches and planning to submit once the
>> review for x86/platform and sensor driver patches completes.
> 
> Alright.
> 
>>
>>>> +     help
>>>> +       Platform driver for AMD platforms containing image signal
>>>> processor
>>>> +       gen 4. Provides camera sensor module board information to allow
>>>> +       sensor and V4L drivers to work properly.
>>>> +
>>>> +       This driver can also be built as a module.  If so, the module
>>>> +       will be called amd_isp4.
>>>> diff --git a/drivers/platform/x86/amd/Makefile
>>>> b/drivers/platform/x86/ amd/Makefile
>>>> index c6c40bdcbded..b0e284b5d497 100644
>>>> --- a/drivers/platform/x86/amd/Makefile
>>>> +++ b/drivers/platform/x86/amd/Makefile
>>>> @@ -10,3 +10,4 @@ obj-$(CONFIG_AMD_PMC)               += pmc/
>>>>   obj-$(CONFIG_AMD_HSMP)              += hsmp/
>>>>   obj-$(CONFIG_AMD_PMF)               += pmf/
>>>>   obj-$(CONFIG_AMD_WBRF)              += wbrf.o
>>>> +obj-$(CONFIG_AMD_ISP_PLATFORM)       += amd_isp4.o
>>>> diff --git a/drivers/platform/x86/amd/amd_isp4.c b/drivers/platform/
>>>> x86/amd/amd_isp4.c
>>>> new file mode 100644
>>>> index 000000000000..461a10be5ccd
>>>> --- /dev/null
>>>> +++ b/drivers/platform/x86/amd/amd_isp4.c
>>>> @@ -0,0 +1,269 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * AMD ISP platform driver for sensor i2-client instantiation
>>>> + *
>>>> + * Copyright 2025 Advanced Micro Devices, Inc.
>>>> + */
>>>> +
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/property.h>
>>>> +#include <linux/units.h>
>>>> +
>>>> +#define AMDISP_OV05C10_I2C_ADDR              0x10
>>>> +#define AMDISP_OV05C10_PLAT_NAME "amdisp_ov05c10_platform"
>>>> +#define AMDISP_OV05C10_HID           "OMNI5C10"
>>>> +#define AMDISP_OV05C10_REMOTE_EP_NAME "ov05c10_isp_4_1_1"
>>>> +#define AMD_ISP_PLAT_DRV_NAME                "amd-isp4"
>>>> +
>>>> +/*
>>>> + * AMD ISP platform definition to configure the device properties
>>>> + * missing in the ACPI table.
>>>> + */
>>>> +struct amdisp_platform {
>>>> +     const char *name;
>>>> +     u8 i2c_addr;
>>>> +     u8 max_num_swnodes;
>>>> +     struct i2c_board_info board_info;
>>>> +     struct notifier_block i2c_nb;
>>>> +     struct i2c_client *i2c_dev;
>>>> +     const struct software_node **swnodes;
>>>> +};
>>>> +
>>>> +/* Top-level OV05C10 camera node property table */
>>>> +static const struct property_entry ov05c10_camera_props[] = {
>>>> +     PROPERTY_ENTRY_U32("clock-frequency", 24 * HZ_PER_MHZ),
>>>> +     { }
>>>> +};
>>>> +
>>>> +/* Root AMD ISP OV05C10 camera node definition */
>>>> +static const struct software_node camera_node = {
>>>> +     .name = AMDISP_OV05C10_HID,
>>>> +     .properties = ov05c10_camera_props,
>>>> +};
>>>> +
>>>> +/*
>>>> + * AMD ISP OV05C10 Ports node definition. No properties defined for
>>>> + * ports node for OV05C10.
>>>> + */
>>>> +static const struct software_node ports = {
>>>> +     .name = "ports",
>>>> +     .parent = &camera_node,
>>>> +};
>>>> +
>>>> +/*
>>>> + * AMD ISP OV05C10 Port node definition. No properties defined for
>>>> + * port node for OV05C10.
>>>> + */
>>>> +static const struct software_node port_node = {
>>>> +     .name = "port@",
>>>> +     .parent = &ports,
>>>> +};
>>>> +
>>>> +/*
>>>> + * Remote endpoint AMD ISP node definition. No properties defined for
>>>> + * remote endpoint node for OV05C10.
>>>> + */
>>>> +static const struct software_node remote_ep_isp_node = {
>>>> +     .name = AMDISP_OV05C10_REMOTE_EP_NAME,
>>>> +};
>>>> +
>>>> +/*
>>>> + * Remote endpoint reference for isp node included in the
>>>> + * OV05C10 endpoint.
>>>> + */
>>>> +static const struct software_node_ref_args ov05c10_refs[] = {
>>>> +     SOFTWARE_NODE_REFERENCE(&remote_ep_isp_node),
>>>> +};
>>>> +
>>>> +/* OV05C supports one single link frequency */
>>>> +static const u64 ov05c10_link_freqs[] = {
>>>> +     925 * HZ_PER_MHZ,
>>>> +};
>>>> +
>>>> +/* OV05C supports only 2-lane configuration */
>>>> +static const u32 ov05c10_data_lanes[] = {
>>>> +     1,
>>>> +     2,
>>>> +};
>>>> +
>>>> +/* OV05C10 endpoint node properties table */
>>>> +static const struct property_entry ov05c10_endpoint_props[] = {
>>>> +     PROPERTY_ENTRY_U32("bus-type", 4),
>>>> +     PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes", ov05c10_data_lanes,
>>>> + ARRAY_SIZE(ov05c10_data_lanes)),
>>>> +     PROPERTY_ENTRY_U64_ARRAY_LEN("link-frequencies",
>>>> ov05c10_link_freqs,
>>>> + ARRAY_SIZE(ov05c10_link_freqs)),
>>>> +     PROPERTY_ENTRY_REF_ARRAY("remote-endpoint", ov05c10_refs),
>>>> +     { }
>>>> +};
>>>> +
>>>> +/* AMD ISP endpoint node definition */
>>>> +static const struct software_node endpoint_node = {
>>>> +     .name = "endpoint",
>>>> +     .parent = &port_node,
>>>> +     .properties = ov05c10_endpoint_props,
>>>> +};
>>>> +
>>>> +/*
>>>> + * AMD ISP swnode graph uses 5 nodes and also its relationship is
>>>> + * fixed to align with the structure that v4l2 expects for successful
>>>> + * endpoint fwnode parsing.
>>>> + *
>>>> + * It is only the node property_entries that will vary for each
>>>> platform
>>>> + * supporting different sensor modules.
>>>> + */
>>>> +#define NUM_SW_NODES 5
>>>> +
>>>> +static const struct software_node *ov05c10_nodes[NUM_SW_NODES + 1] = {
>>>> +     &camera_node,
>>>> +     &ports,
>>>> +     &port_node,
>>>> +     &endpoint_node,
>>>> +     &remote_ep_isp_node,
>>>> +     NULL
>>>> +};
>>>> +
>>>> +/* OV05C10 specific AMD ISP platform configuration */
>>>> +static const struct amdisp_platform amdisp_ov05c10_platform_config = {
>>>> +     .name = AMDISP_OV05C10_PLAT_NAME,
>>>
>>> Where is this field being used?
>>>
>>>> +     .board_info = {
>>>> +             .dev_name = "ov05c10",
>>>> +             I2C_BOARD_INFO("ov05c10", AMDISP_OV05C10_I2C_ADDR),
>>>> +     },
>>>> +     .i2c_addr = AMDISP_OV05C10_I2C_ADDR,
>>>
>>> Please reuse board_info->addr.
>>>
>>>> +     .max_num_swnodes = NUM_SW_NODES,
>>>
>>> Where is max_num_swnodes being used?
>>>
>>>> +     .swnodes = ov05c10_nodes,
>>>
>>> Why not drop .swnodes and referencing ov05c10_nodes directly?
>>>
>> Thanks. Some of the variables phased out with the ongoing development
>> and patching. I will take care of removing the unused and redudant
>> variables in 'struct amdisp_platform'.
>>
>>>> +};
>>>> +
>>>> +static const struct acpi_device_id amdisp_sensor_ids[] = {
>>>> +     { AMDISP_OV05C10_HID },
>>>> +     { }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(acpi, amdisp_sensor_ids);
>>>> +
>>>> +static inline bool is_isp_i2c_adapter(struct i2c_adapter *adap)
>>>> +{
>>>> +     return !strcmp(adap->owner->name, "i2c_designware_amdisp");
>>>> +}
>>>> +
>>>> +static void instantiate_isp_i2c_client(struct amdisp_platform
>>>> *ov05c10, struct i2c_adapter *adap)
>>>> +{
>>>> +     struct i2c_board_info *info = &ov05c10->board_info;
>>>> +     struct i2c_client *i2c_dev;
>>>> +
>>>> +     if (ov05c10->i2c_dev)
>>>> +             return;
>>>> +
>>>> +     if (!info->addr) {
>>>> +             dev_err(&adap->dev, "invalid i2c_addr 0x%x
>>>> detected\n", ov05c10->i2c_addr);
>>>> +             return;
>>>> +     }
>>>> +
>>>> +     i2c_dev = i2c_new_client_device(adap, info);
>>>> +     if (IS_ERR(i2c_dev)) {
>>>> +             dev_err(&adap->dev, "error %pe registering isp
>>>> i2c_client\n", i2c_dev);
>>>> +             return;
>>>> +     }
>>>> +     ov05c10->i2c_dev = i2c_dev;
>>>> +}
>>>> +
>>>> +static int isp_i2c_bus_notify(struct notifier_block *nb,
>>>> +                           unsigned long action, void *data)
>>>> +{
>>>> +     struct amdisp_platform *ov05c10 = container_of(nb, struct
>>>> amdisp_platform, i2c_nb);
>>>> +     struct device *dev = data;
>>>> +     struct i2c_client *client;
>>>> +     struct i2c_adapter *adap;
>>>> +
>>>> +     switch (action) {
>>>> +     case BUS_NOTIFY_ADD_DEVICE:
>>>> +             adap = i2c_verify_adapter(dev);
>>>> +             if (!adap)
>>>> +                     break;
>>>> +             if (is_isp_i2c_adapter(adap))
>>>> +                     instantiate_isp_i2c_client(ov05c10, adap);
>>>> +             break;
>>>> +     case BUS_NOTIFY_REMOVED_DEVICE:
>>>> +             client = i2c_verify_client(dev);
>>>> +             if (!client)
>>>> +                     break;
>>>> +             if (ov05c10->i2c_dev == client) {
>>>> +                     dev_dbg(&client->adapter->dev, "amdisp
>>>> i2c_client removed\n");
>>>> +                     ov05c10->i2c_dev = NULL;
>>>> +             }
>>>> +             break;
>>>> +     default:
>>>> +             break;
>>>> +     }
>>>> +
>>>> +     return NOTIFY_DONE;
>>>
>>> You still need to handle the situation where the AMD I2C adapter is
>>> already registered when
>>> registering the bus notifier. In this case you will miss the
>>> BUS_NOTIFY_ADD_DEVICE event.
>>>
>> Thanks. I will cover this case using the below sequence.
>>
>> 1. bus_register_notifier()
>> 2. i2c_for_each_dev()
>>
>> If at all i2c adapter is registered by the time
>> bus_register_notifier() is called, it should be detected in
>> i2c_for_each_dev(). I will add checks to avoid creation of duplicate
>> i2c_client devices when both notifier callback and i2c_for_each_dev()
>> passes especially when i2c adapter gets added between 1 and 2.
>>
>> Please suggest if there is an alternate better approach that I should
>> use to handle this case.
>>
> This approach seems good to me, i2c-dev uses something similar.
> 
Thanks Armin for confirming and providing the reference. I will address 
the comments and submit the patch V8 soon.

> Thanks,
> Armin Wolf
> 
>>>> +}
>>>> +
>>>> +static struct amdisp_platform *prepare_amdisp_platform(struct
>>>> device *dev,
>>>> +                                                    const struct
>>>> amdisp_platform *src)
>>>> +{
>>>> +     struct amdisp_platform *isp_ov05c10;
>>>> +     struct i2c_board_info *info;
>>>> +     int ret;
>>>> +
>>>> +     isp_ov05c10 = devm_kzalloc(dev, sizeof(*isp_ov05c10),
>>>> GFP_KERNEL);
>>>> +     if (!isp_ov05c10)
>>>> +             return ERR_PTR(-ENOMEM);
>>>> +     memcpy(isp_ov05c10, src, sizeof(*isp_ov05c10));
>>>
>>> This is not what i meant. I was complaining that
>>> amdisp_ov05c10_platform_config contains both
>>> static data (swnodes) and data assigned during runtime (board_info-
>>>  >swnode, i2c_dev, ...).
>>>
>>> Please do not use a global instance of struct amdisp_platform for
>>> initialization. Instead initialize a
>>> fresh instance of this struct inside prepare_amdisp_platform().
>>>
>> sure, will remove the global variable
>> 'amdisp_ov05c10_platform_config', and will take care of initializing
>> the amdisp_platform instance in the prepare_amdisp_platform().
>>
>>>> +
>>>> +     info = &isp_ov05c10->board_info;
>>>> +
>>>> +     ret = software_node_register_node_group(src->swnodes);
>>>> +     if (ret)
>>>> +             return ERR_PTR(ret);
>>>> +
>>>> +     info->swnode = src->swnodes[0];
>>>> +
>>>> +     return isp_ov05c10;
>>>> +}
>>>> +
>>>> +static int amd_isp_probe(struct platform_device *pdev)
>>>> +{
>>>> +     struct amdisp_platform *ov05c10;
>>>> +     int ret;
>>>> +
>>>> +     ov05c10 = prepare_amdisp_platform(&pdev->dev,
>>>> &amdisp_ov05c10_platform_config);
>>>> +     if (IS_ERR(ov05c10))
>>>> +             return dev_err_probe(&pdev->dev, PTR_ERR(ov05c10),
>>>> +                                  "failed to prepare AMD ISP
>>>> platform fwnode\n");
>>>> +
>>>> +     ov05c10->i2c_nb.notifier_call = isp_i2c_bus_notify;
>>>> +     ret = bus_register_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
>>>> +     if (ret)
>>>> +             return ret;
>>>
>>> You need to call software_node_unregister_node_group() here when
>>> bus_register_notifier() fails.
>>>
>> Thanks. I will fix this in the next V8 patch.
>>
>>> Thanks,
>>> Armin Wolf>
>> Thanks,
>> Pratap
>>
>>>> +
>>>> +     platform_set_drvdata(pdev, ov05c10);
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static void amd_isp_remove(struct platform_device *pdev)
>>>> +{
>>>> +     struct amdisp_platform *ov05c10 = platform_get_drvdata(pdev);
>>>> +
>>>> +     bus_unregister_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
>>>> +     i2c_unregister_device(ov05c10->i2c_dev);
>>>> + software_node_unregister_node_group(ov05c10->swnodes);
>>>> +}
>>>> +
>>>> +static struct platform_driver amd_isp_platform_driver = {
>>>> +     .driver = {
>>>> +             .name                   = AMD_ISP_PLAT_DRV_NAME,
>>>> +             .acpi_match_table       = amdisp_sensor_ids,
>>>> +     },
>>>> +     .probe  = amd_isp_probe,
>>>> +     .remove = amd_isp_remove,
>>>> +};
>>>> +
>>>> +module_platform_driver(amd_isp_platform_driver);
>>>> +
>>>> +MODULE_AUTHOR("Benjamin Chan <benjamin.chan@....com>");
>>>> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@....com>");
>>>> +MODULE_DESCRIPTION("AMD ISP4 Platform Driver");
>>>> +MODULE_LICENSE("GPL");
>>
>>

Powered by blists - more mailing lists