linux-kernel - Re: [PATCH] platform/x86: amd: Add ISP platform info

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] [day] [month] [year] [list]
Message-ID: <b27adf7b-1618-405a-9036-665c4605e5aa@amd.com>
Date: Fri, 28 Feb 2025 12:01:00 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: Pratap Nirujogi <pratap.nirujogi@....com>, hdegoede@...hat.com,
 ilpo.jarvinen@...ux.intel.com
Cc: platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
 benjamin.chan@....com
Subject: Re: [PATCH] platform/x86: amd: Add ISP platform info

On 2/28/2025 11:02, Pratap Nirujogi wrote:
> Add ov05c i2c boardinfo and GPIO pin info for AMD ISP platform.
> 
> Details of the resources added:
> 
> - Added i2c bus number for AMD ISP platform is 99.
> - Added GPIO 85 to allow ISP driver to enable and disable ISP access.
> - Added GPIO 0 to allow sensor driver to enable and disable sensor module.
> 
> Signed-off-by: Pratap Nirujogi <pratap.nirujogi@....com>
> ---
>   drivers/platform/x86/amd/Kconfig   | 11 +++++
>   drivers/platform/x86/amd/Makefile  |  1 +
>   drivers/platform/x86/amd/amd_isp.c | 72 ++++++++++++++++++++++++++++++
>   3 files changed, 84 insertions(+)
>   create mode 100644 drivers/platform/x86/amd/amd_isp.c
> 
> diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/amd/Kconfig
> index c3e086ea64fc..4b373edd750d 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
> +	bool "AMD platform with ISP4 that supports Camera sensor device"

Thinking forward to a hypothetical "ISP5", since this is "ISP4" 
platform, should all cases also be ISP4?

IE
config AMD_ISP4_PLATFORM
amd_isp4.c
amd_isp4.o
> +	depends on I2C && X86_64 && AMD_ISP4

Doesn't this also need PINCTRL_AMD?

> +	help
> +	  For AMD platform that support Image signal processor generation 4, it
> +	  is necessary to add platform specific camera sensor module board info
> +	  which includes the sensor driver device id and the i2c address.
> +
> +	  If you have a AMD platform that support ISP4 and with a sensor
> +	  connected to it, say Y here
> diff --git a/drivers/platform/x86/amd/Makefile b/drivers/platform/x86/amd/Makefile
> index 56f62fc9c97b..0d89e2d4f7e6 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_isp.o
> diff --git a/drivers/platform/x86/amd/amd_isp.c b/drivers/platform/x86/amd/amd_isp.c
> new file mode 100644
> index 000000000000..751f209e9509
> --- /dev/null
> +++ b/drivers/platform/x86/amd/amd_isp.c
> @@ -0,0 +1,72 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright 2025 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/gpio/machine.h>
> +
> +#define AMDISP_I2C_BUS		99
> +
> +static struct gpiod_lookup_table isp_gpio_table = {
> +	.dev_id = "amd_isp_capture",
> +	.table = {
> +		GPIO_LOOKUP("AMDI0030:00", 85, "enable_isp", GPIO_ACTIVE_HIGH),
> +		{ }
> +	},
> +};
> +
> +static struct gpiod_lookup_table isp_sensor_gpio_table = {
> +	.dev_id = "ov05c",
> +	.table = {
> +		GPIO_LOOKUP("amdisp-pinctrl", 0, "sensor0_enable", GPIO_ACTIVE_HIGH),
> +		{ }
> +	},
> +};
> +
> +static struct i2c_board_info sensor_info = {
> +	.dev_name = "ov05c",
> +	I2C_BOARD_INFO("ov05c", 0x10),
> +};
> +
> +static int __init amd_isp_init(void)

Keep in mind that distros will prominently enable most configs like 
this.  So that's going to mean that anything compiled with this driver 
is going to run amd_isp_init().

With that thought in mind; I think you need some extra checks as a proxy 
to know that you have a relevant platform.

Can you do a PCI lookup for the PCI root port or PCI GPU perhaps?
If those aren't found to match the expected value then return -ENODEV.

As for the 0v05c sensor board, isn't it technically going to be possible 
to have different sensors?  Is it possible to do an identification query 
over I2C to validate that this is the correct sensor board before 
registering it?

If it's not discoverable in some way; I am afraid we will need some 
hardcoded quirks to only bind on the relevant system(s) that are known 
to have this sensor.

We don't want it being registered on a system either without the board 
present.

If quirks are the way we have to go I think it makes sense to also have 
a module parameter to allow it to be forced, to allow any other systems 
to be added to the quirk list.

> +{
> +	int ret;
> +
> +	gpiod_add_lookup_table(&isp_gpio_table);
> +	gpiod_add_lookup_table(&isp_sensor_gpio_table);
> +
> +	ret = i2c_register_board_info(AMDISP_I2C_BUS, &sensor_info, 1);
> +	if (ret)
> +		pr_err("%s: cannot register i2c board devices:%s",
> +		       __func__, sensor_info.dev_name);
> +
> +	return ret;
> +}
> +
> +arch_initcall(amd_isp_init);
> +
> +MODULE_AUTHOR("Benjamin Chan <benjamin.chan@....com>");
> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@....com>");
> +MODULE_DESCRIPTION("AMD ISP Platform parameters");

Should this be ISP4?

> +MODULE_LICENSE("GPL and additional rights");