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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d2bdc8a6-1907-77cd-43a2-fb28439bd37f@linux.intel.com>
Date: Tue, 13 May 2025 00:00:22 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: "Nirujogi, Pratap" <pnirujog@....com>
cc: Pratap Nirujogi <pratap.nirujogi@....com>, 
    Hans de Goede <hdegoede@...hat.com>, W_Armin@....de, 
    mario.limonciello@....com, platform-driver-x86@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>, benjamin.chan@....com, bin.du@....com, 
    gjorgji.rosikopulos@....com, king.li@....com, dantony@....com
Subject: Re: [PATCH v13] platform/x86: Add AMD ISP platform config for
 OV05C10

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@....com>
> > > Signed-off-by: Benjamin Chan <benjamin.chan@....com>
> > > Reviewed-by: Mario Limonciello <mario.limonciello@....com>
> > > Reviewed-by: Hans de Goede <hdegoede@...hat.com>
> > > Reviewed-by: Armin Wolf <W_Armin@....de>
> > > Signed-off-by: Pratap Nirujogi <pratap.nirujogi@....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.

-- 
 i.