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

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

On 5/12/2025 5:00 PM, Ilpo Järvinen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Mon, 12 May 2025, Nirujogi, Pratap wrote:
> 
>> Hi Ilpo,
>>
>> On 5/11/2025 6:54 PM, Ilpo Järvinen wrote:
>>> Caution: This message originated from an External Source. Use proper caution
>>> when opening attachments, clicking links, or responding.
>>>
>>>
>>> On Fri, 9 May 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>
>>>> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>>> Reviewed-by: Armin Wolf <W_Armin@gmx.de>
>>>> Signed-off-by: Pratap Nirujogi <pratap.nirujogi@amd.com>
>>>> ---
>>>> Changes v12 -> v13:
>>>>
>>>> * Add "struct amdisp_platform_info" to pass sensor specific
>>>> configuration and make the driver generic to support OV05C10
>>>> and other supported sensor modules in future.
>>>>
>>>> * Address cosmetic and other review comments.
>>>>
>>>>    drivers/platform/x86/amd/Kconfig    |  11 +
>>>>    drivers/platform/x86/amd/Makefile   |   1 +
>>>>    drivers/platform/x86/amd/amd_isp4.c | 309 ++++++++++++++++++++++++++++
>>>>    3 files changed, 321 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..152a68a470e8 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
>>>> +     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..27939020634c
>>>> --- /dev/null
>>>> +++ b/drivers/platform/x86/amd/amd_isp4.c
>>>> @@ -0,0 +1,309 @@
>>>> +// 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"
>>>
>>> This is not used anywhere?
>>>
>> Thanks. Will remove it, its not used anymore.
>>
>>>> +#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 info definition to initialize sensor
>>>> + * specific platform configuration to prepare the amdisp
>>>> + * platform.
>>>> + */
>>>> +struct amdisp_platform_info {
>>>> +     struct i2c_board_info board_info;
>>>> +     const struct software_node **swnodes;
>>>> +};
>>>> +
>>>> +/*
>>>> + * AMD ISP platform definition to configure the device properties
>>>> + * missing in the ACPI table.
>>>> + */
>>>> +struct amdisp_platform {
>>>> +     const struct amdisp_platform_info *pinfo;
>>>> +     struct i2c_board_info board_info;
>>>> +     struct notifier_block i2c_nb;
>>>> +     struct i2c_client *i2c_dev;
>>>> +     struct mutex lock;      /* protects i2c client creation */
>>>
>>> Missing #include.
>>>
>> ok, will #include <linux/mutex.h>. I have not included some of these header
>> files seprately as they are already part of one or the other existing headers
>> in the patch.
> 
> We try to include what is used by the file itself but as there's no
> ready to use tool to enforce it automatically, it largely depends on devs /
> reviewers noticing what should be added. Whenever adding a call to
> anything outside the .c file itself or use of non-local macro, it's good
> to check if another #include needs to be added (but I understand devs, me
> included, will often forget it).
> 
> There are a few obvious includes where the other one is not needed as the
> path is practically guaranteed, typical examples: linux/xx.h including
> asm/xx.h or uapi/linux/xx.h.
> 
> Relying intentionally on indirect includes creates very hard to track
> dependencies making it complex to remove any header from another header
> when the headerfile itself no longer needs that include. Build testing
> will catch some resulting fallout from such removal but it's coverage is
> not perfect.
> 
Thanks Ilpo for the detailed explanation and clarifying the significance 
of avoiding the indirect includes. Its really helpful. Thanks.

> --
>   i.