On Thu, May 15, 2025 at 2:03 PM Mario Limonciello
<mario.limoncie...@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.niruj...@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
>
> >
> >>> isp_base = adev->rmmio_base;
> >>> isp->isp_cell = kcalloc(3, sizeof(struct mfd_cell), GFP_KERNEL);
> >>
> >
>
