[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8d892845-e134-4553-a6af-55d785c1ae98@amd.com>
Date: Thu, 8 May 2025 19:05:52 -0400
From: "Nirujogi, Pratap" <pnirujog@....com>
To: Sakari Ailus <sakari.ailus@....fi>
Cc: Pratap Nirujogi <pratap.nirujogi@....com>, hdegoede@...hat.com,
W_Armin@....de, ilpo.jarvinen@...ux.intel.com, mario.limonciello@....com,
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 v12] platform/x86: Add AMD ISP platform config for OV05C10
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
Powered by blists - more mailing lists