linux-kernel - Re: [PATCH v12] 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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aBxEzgtNs8JD3TEt@valkosipuli.retiisi.eu>
Date: Thu, 8 May 2025 05:44:46 +0000
From: Sakari Ailus <sakari.ailus@....fi>
To: "Nirujogi, Pratap" <pnirujog@....com>
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 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.

>              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