[PATCH] drm/amd/amdgpu: Add GPIO resources required for amdisp

[Nirujogi, Pratap]

On 5/15/2025 2:06 PM, Alex Deucher wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Thu, May 15, 2025 at 2:03 PM Mario Limonciello
> <mario.limonciello at amd.com> wrote:
>>
>> On 5/15/2025 12:47 PM, Nirujogi, Pratap wrote:
>>> Hi Mario,
>>>
>>> On 5/14/2025 6:05 PM, Mario Limonciello wrote:
>>>> On 5/14/2025 4:35 PM, Pratap Nirujogi wrote:
>>>>> ISP is a child device to GFX, and its device specific information
>>>>> is not available in ACPI. Adding the 2 GPIO resources required for
>>>>> ISP_v4_1_1 in amdgpu_isp driver.
>>>>>
>>>>> - GPIO 0 to allow sensor driver to enable and disable sensor module.
>>>>> - GPIO 85 to allow ISP driver to enable and disable ISP RGB streaming
>>>>> mode.
>>>>>
>>>>> Signed-off-by: Pratap Nirujogi <pratap.nirujogi at amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c | 46 +++++++++++++++++++++++++
>>>>>    1 file changed, 46 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c b/drivers/gpu/
>>>>> drm/amd/amdgpu/isp_v4_1_1.c
>>>>> index 69dd92f6e86d..c488af6c8013 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c
>>>>> @@ -25,6 +25,7 @@
>>>>>     *
>>>>>     */
>>>>> +#include <linux/gpio/machine.h>
>>>>>    #include "amdgpu.h"
>>>>>    #include "isp_v4_1_1.h"
>>>>> @@ -39,15 +40,60 @@ static const unsigned int
>>>>> isp_4_1_1_int_srcid[MAX_ISP411_INT_SRC] = {
>>>>>        ISP_4_1__SRCID__ISP_RINGBUFFER_WPT16
>>>>>    };
>>>>> +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 = "i2c-ov05c10",
>>>>> +    .table = {
>>>>> +        GPIO_LOOKUP("amdisp-pinctrl", 0, "enable", GPIO_ACTIVE_HIGH),
>>>>> +        { }
>>>>> +    },
>>>>> +};
>>>>> +
>>>>> +static const struct acpi_device_id isp_sensor_ids[] = {
>>>>> +    { "OMNI5C10" },
>>>>> +    { }
>>>>> +};
>>>>> +
>>>>> +static int isp_match_acpi_device_ids(struct device *dev, const void
>>>>> *data)
>>>>> +{
>>>>> +    return acpi_match_device(data, dev) ? 1 : 0;
>>>>> +}
>>>>> +
>>>>>    static int isp_v4_1_1_hw_init(struct amdgpu_isp *isp)
>>>>>    {
>>>>>        struct amdgpu_device *adev = isp->adev;
>>>>> +    struct acpi_device *acpi_pdev;
>>>>>        int idx, int_idx, num_res, r;
>>>>> +    struct device *pdev;
>>>>>        u64 isp_base;
>>>>>        if (adev->rmmio_size == 0 || adev->rmmio_size < 0x5289)
>>>>>            return -EINVAL;
>>>>> +    pdev = bus_find_device(&platform_bus_type, NULL, isp_sensor_ids,
>>>>> +                   isp_match_acpi_device_ids);
>>>>> +    if (!pdev) {
>>>>> +        drm_dbg(&adev->ddev, "Invalid isp platform detected:%ld",
>>>>> +            PTR_ERR(pdev));
>>>>> +        /* allow GPU init to progress */
>>>>> +        return 0;
>>>>> +    }
>>>>> +    acpi_pdev = ACPI_COMPANION(pdev);
>>>>> +
>>>>> +    /* add GPIO resources required for OMNI5C10 sensor */
>>>>> +    if (!strcmp("OMNI5C10", acpi_device_hid(acpi_pdev))) {
>>>>> +        gpiod_add_lookup_table(&isp_gpio_table);
>>>>> +        gpiod_add_lookup_table(&isp_sensor_gpio_table);
>>>>> +    }
>>>>> +    put_device(pdev);
>>>>> +
>>>>
>>>> Can you please move this into a helper in amdgpu_acpi.c?  We try not
>>>> to have ACPI code outside of there in case someone compiles without
>>>> CONFIG_ACPI.
>>>>
>>>> Sorry I should have mentioned this sooner.
>>>>
>>> sure, I will add "int amdgpu_acpi_get_isp4_dev_hid(char **hid)", which
>>> takes care of:
>>>
>>> 1. finding the matching isp4 platform device(pdev) using bus_find_device()
>>> 2. gets acpi device handle(acpi_pdev) for the matching pdev and returns
>>> valid hid incase of no failures.
>>>
>>> Is this approach okay? hope this function signature makes sense. Please
>>> let me know if any comments or suggestions on this approach.
>>>
>>> Thanks,
>>> Pratap
>>
>> I think this approach is fine, but it would be good to get Alex's
>> comments on this.
> 
> Seems fine to me.
> 
> Alex
> 
Thanks Alex and Mario for confirming. I will post the new patch shortly.

Thanks,
Pratap
>>
>>>
>>>>>        isp_base = adev->rmmio_base;
>>>>>        isp->isp_cell = kcalloc(3, sizeof(struct mfd_cell), GFP_KERNEL);
>>>>
>>>
>>