Re: [PATCH] pinctrl: amd: isp411: Add amdisp GPIO pinctrl

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Pratap,

thanks for your patch!

On Fri, Feb 28, 2025 at 5:58 PM Pratap Nirujogi <pratap.nirujogi@xxxxxxx> wrote:

> +config PINCTRL_AMDISP
> +       tristate "AMDISP GPIO pin control"
> +       depends on HAS_IOMEM
> +       select GPIOLIB
> +       select PINCONF
> +       select GENERIC_PINCONF
> +       help
> +         The driver for memory mapped GPIO functionality on AMD platforms
> +         with ISP support. All the pins are output controlled only
> +
> +         Requires AMDGPU to MFD add device for enumeration to set up as
> +         platform device.

> +/* SPDX-License-Identifier: MIT */

OK we have this...

> +/*
> + * Copyright (C) 2025 Advanced Micro Devices, Inc. All rights reserved.
> + * All Rights Reserved.
> + *

That can be kept

> + * 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, sub license, 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 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 NON-INFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS 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.
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.

This is already in:
LICENSES/preferred/MIT

Which is referenced by the SPDX tag.

Can we just drop it? It's very annoying with all this boilerplate.

> +#ifdef CONFIG_GPIOLIB

You select GPIOLIB in the Kconfig so drop the ifdef, it's always
available.

> +static int amdisp_gpio_set_config(struct gpio_chip *gc, unsigned int gpio,
> +                                 unsigned long config)
> +{
> +       /* Nothing to do, amdisp gpio has no other config */
> +       return 0;
> +}

Don't even assign the callback then, that's fine.

> +static int amdisp_gpiochip_add(struct platform_device *pdev,
> +                              struct amdisp_pinctrl *pctrl)
> +{
> +       struct gpio_chip *gc = &pctrl->gc;
> +       struct pinctrl_gpio_range *grange = &pctrl->gpio_range;
> +       int ret;
> +
> +       gc->label               = dev_name(pctrl->dev);
> +       gc->owner               = THIS_MODULE;

I think the core default-assigns owner so you don't need to
assign this.

> +       gc->parent              = &pdev->dev;
> +       gc->names               = amdisp_range_pins_name;
> +       gc->request             = gpiochip_generic_request;
> +       gc->free                = gpiochip_generic_free;
> +       gc->get_direction       = amdisp_gpio_get_direction;
> +       gc->direction_input     = amdisp_gpio_direction_input;
> +       gc->direction_output    = amdisp_gpio_direction_output;
> +       gc->get                 = amdisp_gpio_get;
> +       gc->set                 = amdisp_gpio_set;
> +       gc->set_config          = amdisp_gpio_set_config;

I.e. drop this.

> +       gc->base                = -1;
> +       gc->ngpio               = ARRAY_SIZE(amdisp_range_pins);
> +#if defined(CONFIG_OF_GPIO)
> +       gc->of_node             = pdev->dev.of_node;
> +       gc->of_gpio_n_cells     = 2;
> +#endif

Drop the ifdefs.

> +#ifdef CONFIG_GPIOLIB
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (IS_ERR(res))
> +               return PTR_ERR(res);
> +
> +       pctrl->gpiobase = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(pctrl->gpiobase))
> +               return PTR_ERR(pctrl->gpiobase);
> +#endif

Drop ifdefs

> +#ifdef CONFIG_GPIOLIB
> +       ret = amdisp_gpiochip_add(pdev, pctrl);
> +       if (ret)
> +               return ret;
> +#endif

Drop ifdefs

> +static int __init amdisp_pinctrl_init(void)
> +{
> +       return platform_driver_register(&amdisp_pinctrl_driver);
> +}
> +arch_initcall(amdisp_pinctrl_init);
> +
> +static void __exit amdisp_pinctrl_exit(void)
> +{
> +       platform_driver_unregister(&amdisp_pinctrl_driver);
> +}
> +module_exit(amdisp_pinctrl_exit);

Why do you need arch_initcall()?

Try to just use module_platform_driver() for the
whole module.

> +MODULE_AUTHOR("Benjamin Chan <benjamin.chan@xxxxxxx>");
> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@xxxxxxx>");
> +MODULE_DESCRIPTION("AMDISP pinctrl driver");
> +MODULE_LICENSE("GPL and additional rights");

Well that does not correspond to MIT does it?

> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (C) 2025 Advanced Micro Devices, Inc. All rights reserved.
> + * All Rights Reserved.
> + *
> + * 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, sub license, 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 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 NON-INFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS 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.
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.

Can we drop this?

Yours,
Linus Walleij