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

[Nirujogi, Pratap]

Hi Linus,

Thanks for your review. Please help reviewing the new v2 patch with 
comments addressed and will look forward for your feedback.

Thanks,
Pratap

On 3/4/2025 3:34 AM, Linus Walleij wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> 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.

Done. Updated copright header in v2.

> > +#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.
Done.

> > +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.
Done.

> > +       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.
Done.

> > +       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.
Done.

> > +#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
Done.

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

> > +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.
Done. thanks for this recommendation, replaced with module_platform_driver.

> > +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?
Done. Updated license info in v2.

> > +/* 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?
Done.

> Yours,
> Linus Walleij