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

["Nirujogi, Pratap" <pnirujog@amd.com>]

Hi Sakari,

On 5/8/2025 1:44 AM, Sakari Ailus wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> Hi Pratap,
> 
> On Wed, May 07, 2025 at 04:16:04PM -0400, Nirujogi, Pratap wrote:
>>>> diff --git a/drivers/platform/x86/amd/amd_isp4.c b/drivers/platform/x86/amd/amd_isp4.c
>>>> new file mode 100644
>>>> index 000000000000..1520ebb94507
>>>> --- /dev/null
>>>> +++ b/drivers/platform/x86/amd/amd_isp4.c
>>>> @@ -0,0 +1,280 @@
>>>> +// 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"
>>>
>>> What's the purpose of this _HID and is it present on actual firmware
>>> implementation? There's no such ACPI vendor ID as "OMNI".
>>>
>> The (_HID, "OMNI5C10") is used to check if the OV05C10 ACPI device is
>> actually present before creating the AMD ISP4 platform driver. Yes, ACPI
>> entry is present for OV05C10 sensor in _SB/CAMF.
>>
>>       Scope (_SB)
>>       {
>>           Device (CAMF)
>>           {
>>               Name (_HID, "OMNI5C10")  // _HID: Hardware ID
> 
> Please tell your BIOS folks this ACPI ID is invalid. In the future, either
> allocate one yourself with your own vendor ID or get one from the sensor
> vendor, which they would have allocated using their own ACPI vendor ID.
> 
Thanks for the feedback. I will share it with the BIOS team and make 
sure its taken into account for future models.

Thanks,
Pratap

>>               Name (_DDN, "OV05C-RGB")  // _DDN: DOS Device Name
>>               Name (_SUB, "OV05C")  // _SUB: Subsystem ID
>>
>>
>>>> +#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 {
>>>> +     struct i2c_board_info board_info;
>>>> +     struct notifier_block i2c_nb;
>>>> +     struct i2c_client *i2c_dev;
>>>> +     struct mutex lock; /* protects i2c client creation */
>>>> +};
>>>> +
>>>> +/* 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.
>>>
>>> How will this scale? Can you use other sensors with this ISP? Although if
>>> you get little from firmware, there's not much you can do. That being said,
>>> switching to DisCo for Imaging could be an easier step in this case.
>>>
>> the scope of this driver is limited to ov05c10, and it can be enhanced to
>> support other sensor modules in future.
>>
>> Sorry, I'm not familiar with the term DisCo. Could you please elaborate.
> 
> See my reply to Hans.
> 
> --
> Regards,
> 
> Sakari Ailus