Message-ID: <9b8c9eb7-c8d5-4c12-9ce5-c4b4df3b4223@redhat.com>
Date: Wed, 5 Mar 2025 16:26:46 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: "Nirujogi, Pratap" <pnirujog@....com>,
 Pratap Nirujogi <pratap.nirujogi@....com>, ilpo.jarvinen@...ux.intel.com,
 "Limonciello, Mario" <Mario.Limonciello@....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

Hi Pratap,

On 4-Mar-25 12:14 AM, Nirujogi, Pratap wrote:
> 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.

The ISP still could and really should be an ACPI child device of
the GFX PCI device in this case with its own _CRS for for example
the enable ISP GPIO.

>> 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

Thanks.

So looking at this there are ACPI devices for the sensors, which
unfortunately lack a _CRS with an I2CSerialBusV2 resource pointing
to the ISP childdevice as bus-controller. So that i2c-client
instantiating would be instant.

+Cc Mario

Mario any chance that for the next (or the next-next) generation of
AMD devices we can get the ACPI tables fixed to properly describe
the sensors as having an I2cSerialBusV2 resource, just like how e.g.
I2C touchpads / touchscreens have this ?  I suspect this will benefit
Windows too. Likewise any enable GPIOs for the sensor really also
should be proper ACPi Gpio resources in the ACPI device describing
the sensor.

Ok, back to the current generation devices. So there is an ACPI
device for the sensor there. This should lead to a:
/dev/bus/platform/devices/OMNI5C10:00 device getting created
(please check this).

So this driver for adding the sensor GPIO lookup + creating
the i2c_client should be rewritten to be a platform_driver
binding to that device and it should be a module rather then
being builtin using module_platform_driver():

- Binding using a struct acpi_device_id table to match the ACPI HID of
  OMNI5C10 + using MODULE_DEVICE_TABLE(acpi, table_name) for auto module
  loading.
  The driver_data of the acpi_device_id should point to i2c_board_info to
  use for that HID to future proof the driver for adding support for other
  sensor models

- Loading as module means this can be loaded after the i2c adapter driver,
  so instead of registering board-info it should use the mechanism used
  in drivers/platform/x86/dell/dell-lis3lv02d.c combined with a unique
  adapter name, then the module load ordering does not matter and it is
  also unnecessary to have a magic fixed i2c bus-number of 99

- probe() should copy the const i2c_board_info info from
  acpi_device_id.driver_data and then set the fwnode so that the sensor
  driver can e.g. get to the _PLD info to determine sensor location
  (e.g. front vs back)

- The GPIO sensor lookup for the ISP enable should be registered by
  the ISP driver itself. Also this seems to be something which might be
  board specific so maybe this needs DMI matching?

I'm looking forward to see a new version implementing the above approach
which would be a big improvement IMHO.

Regards,

Hans

Powered by blists - more mailing lists