Message-ID: <58a51e80-48d0-4c65-ad9a-50f164471b8f@gmx.de>
Date: Wed, 5 Mar 2025 02:11:23 +0100
From: Armin Wolf <W_Armin@....de>
To: "Nirujogi, Pratap" <pnirujog@....com>, Hans de Goede
 <hdegoede@...hat.com>, Pratap Nirujogi <pratap.nirujogi@....com>,
 ilpo.jarvinen@...ux.intel.com
Cc: platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
 benjamin.chan@....com, bin.du@....com, king.li@....com,
 gjorgji.rosikopulos@....com, dominic.antony@....com
Subject: Re: [PATCH] platform/x86: amd: Add ISP platform info

Am 04.03.25 um 18:44 schrieb Nirujogi, Pratap:

> Hi Armin,
>
> Thanks for your comment.
>
> Thanks,
> Pratap
>
> On 3/3/2025 6:56 PM, Armin Wolf wrote:
>> Caution: This message originated from an External Source. Use proper
>> caution when opening attachments, clicking links, or responding.
>>
>>
>> Am 04.03.25 um 00:14 schrieb Nirujogi, Pratap:
>>
>>> Hi Hans,
>>>
>>> Thanks for your review. Please see the inline comments and let us know
>>> your insights.
>>>
>>> Thanks,
>>> Pratap
>>>
>>>
>>> On 3/3/2025 8:41 AM, Hans de Goede wrote:
>>>> Caution: This message originated from an External Source. Use proper
>>>> caution when opening attachments, clicking links, or responding.
>>>>
>>>>
>>>> Hi Pratap,
>>>>
>>>> Thank you for your patch.
>>>>
>>>> On 28-Feb-25 18: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"
>>>>> +     depends on I2C && X86_64 && AMD_ISP4
>>>>> +     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
>>>>
>>>> I'm not a fan of using static i2c-bus numbers for this. static bus
>>>> numbers are
>>>> something of the past and we typically do not use these on x86
>>>> anymore.
>>>>
>>>> Using this static number + i2c_register_board_info() also requires
>>>> this code
>>>> to be builtin rather then modular which is also undesirable.
>>>>
>>>> For a more dynamic way of manually adding i2c-devices see:
>>>>
>>>> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/
>>>> linux.git/tree/drivers/platform/x86/dell/dell-lis3lv02d.c
>>>>
>>>>
>>>> But a better question here is why instantiate the sensor i2c device
>>>> manually at all.
>>>>
>>>> ACPI has a standardized way to describe I2C-clients which tyically
>>>> is used for all I2C devices on ACPI platforms like I2C touchscreens /
>>>> touchpads / audio-codecs / accelerometers / etc.
>>>> I don't see why the camera sensor on AMD platforms is so special that
>>>> it could not be described in ACPI using an ACPI child-device of the
>>>> i2c-controller with a ACPI resource (_CRS entry) of the
>>>> I2cSerialBusV2()
>>>> type.
>>>>
>>>> Likewise the sensor enable GPIO should also be described in the ACPI
>>>> table as a Gpio type resource in the same _CRS table.
>>>>
>>>
>>> We have to take this approach because ISP is a child to GFX PCI device
>>> in AMD HW architectures, and since it is not an independent device,
>>> its device specific configuration (gpio pin ids, i2c-bus number etc.)
>>> is not registered in ACPI.
>>>
>>>> Can you run acpidump -o acpidump.txt on a laptop with this camera
>>>> sensor and send me the acpidupm.txt offlist ? Please run this
>>>> on a production hardware laptop model using production firmware.
>>>>
>>>
>>> Please refer the attached acpidump.txt
>>>
>>>> I suspect that Windows will also be using the ACPI description
>>>> for the sensor so we really should figure out what Windows is doing
>>>> here.
>>>>
>>>
>>> Yes, same ACPI configuration for both Windows and Linux. Similar
>>> approach followed even on windows to control the isp gpio pins.
>>>
>> The OMNI5C10 ACPI device has a _DSM method that supports the GUID
>> f8fd3bff-21b7-4a99-bdc8-c414a3e9453c. Do you know
>> more about the purpose of this method?
>>
>> Thanks,
>> Armin Wolf
>>
>
> This GUID is windows specific. Sorry, I'm not completely sure on what
> exactly it is used for. I'm thinking it is used to distinguish the
> different sensor modules that OEMs supports on different SKUs. I confirm
> its not applicable for Linux.
>
Ok, thanks for checking :)

>>>> As Mario mentioned we cannot just assume that the GPIOs +
>>>> sensor address and model are valid for all laptops. Ideally we should
>>>> be getting this information from ACPI rather then hardcoding it
>>>> in the kernel.
>>>>
>>>
>>> Yes, we initially assumed CONFIG_AMD_ISP_PLATFORM=y will be set only
>>> on the intended platforms, but as that assumption is not valid, the
>>> below
>>> check is added in v2 patch checking the specific ov05c acpi hw id to
>>> present before running the driver.
>>>
>>> /* check for valid platform before configuring isp4 board resources */
>>>     if (!acpi_dev_found(AMDISP_ACPI_CAM_HID))
>>>         return -ENODEV;
>>>
>>>>> +
>>>>> +static struct gpiod_lookup_table isp_gpio_table = {
>>>>> +     .dev_id = "amd_isp_capture",
>>>>> +     .table = {
>>>>> +             GPIO_LOOKUP("AMDI0030:00", 85, "enable_isp",
>>>>> GPIO_ACTIVE_HIGH),
>>>>> +             { }
>>>>> +     },
>>>>> +};
>>>>
>>>> This too really should be an Gpio() type ACPI resource on the ACPI
>>>> device
>>>> node for the ISP.
>>>>
>>>> How/where is this "amd_isp_capture" device created ?
>>>>
>>>
>>> "amd_isp_capture" is the V4L2 ISP driver in this case. The patches for
>>> ISP driver are yet to be submitted. It will be loaded during AMDGPU
>>> device probe on the AMD platforms supporting isp4.2 HW. AMDGPU
>>> reference to trigger the isp device probe:
>>> https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/amd/
>>> amdgpu/isp_v4_1_1.c#L108
>>>
>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>>
>>>>> +
>>>>> +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)
>>>>> +{
>>>>> +     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");
>>>>> +MODULE_LICENSE("GPL and additional rights");
>>>>
>

Powered by blists - more mailing lists