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

["Nirujogi, Pratap" <pnirujog@amd.com>]

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@amd.com> 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@amd.com>");
>> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@amd.com>");
>> +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