Re: [PATCH v2 2/3] pinctrl: amd: Get and update IOMUX details

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Tue, May 24, 2022 at 10:01 AM Basavaraj Natikar
<Basavaraj.Natikar@xxxxxxx> wrote:
>
> Presently there is no way to change pinmux configuration runtime.
> Hence add IOMUX details which can be used to configure IOMUX
> gpio pins runtime to different functionalities.

GPIO

run time

...

> +#include "../acpi/acpica/accommon.h"

One shoudn't ever include this except ACPICA code.

...

> +static acpi_status acpi_get_iomux_region(acpi_handle handle, u32 level,
> +                                        void *ctx, void **return_value)
> +{
> +       struct acpi_namespace_node *node = handle;
> +       union acpi_operand_object *region_obj;
> +       struct amd_gpio *gpio_dev = ctx;
> +
> +       /* Already mapped the IOMUX base */
> +       if (gpio_dev->iomux_base)
> +               return AE_OK;
> +
> +       /* Valid object */
> +       if (!node || !node->object)
> +               return AE_OK;
> +
> +       /* Valid operand or namespace node*/
> +       if ((ACPI_GET_DESCRIPTOR_TYPE(node->object) != ACPI_DESC_TYPE_OPERAND) &&
> +           (ACPI_GET_DESCRIPTOR_TYPE(node->object) != ACPI_DESC_TYPE_NAMED))
> +               return AE_OK;
> +
> +       /* Valid object type*/
> +       if (node->object->common.type == ACPI_TYPE_LOCAL_DATA)
> +               return AE_OK;
> +
> +       region_obj = node->object;
> +       if (!region_obj->region.handler)
> +               return AE_OK;
> +
> +       if (region_obj->region.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY)
> +               return AE_OK;
> +
> +       if (strncmp("IOMX", region_obj->region.node->name.ascii, strlen("IOMX")))
> +               return AE_OK;
> +
> +       gpio_dev->iomux_base = devm_ioremap(&gpio_dev->pdev->dev,
> +                                           region_obj->region.address,
> +                                           region_obj->region.length);
> +       if (!gpio_dev->iomux_base)
> +               dev_err(&gpio_dev->pdev->dev, "failed to devm_ioremap() iomux_base\n");
> +
> +       return AE_OK;
> +}

Your commit message is quite poor, can you add a lot of missed
details, i.e. how it's done in ACPI (DSDT), what the version of ACPI
specification have you in mind when implementing this, etc.

...

> +       acpi_handle sys_bus_handle;
> +       int status = acpi_get_handle(NULL, "\\_SB", &sys_bus_handle);
> +
> +       if (ACPI_FAILURE(status)) {
> +               dev_err(&gpio_dev->pdev->dev, "Failed to get SB handle\n");
> +               return;
> +       }

Usually you don't need an \\_SB handle. Why is it here?

-- 
With Best Regards,
Andy Shevchenko