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

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

Hi Krzysztof,

Thanks for your review.

I pushed the new v2 patch addressing the comments.

Please help reviewing the new v2 and will look forward for your feedback.

Thanks,
Pratap

On 3/1/2025 8:32 AM, Krzysztof Kozlowski wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On 28/02/2025 17:57, Pratap Nirujogi wrote:
>> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
>> index fba1c56624c0..ac27e88677d1 100644
>> --- a/drivers/pinctrl/Makefile
>> +++ b/drivers/pinctrl/Makefile
>> @@ -10,6 +10,7 @@ obj-$(CONFIG_GENERIC_PINCONF)       += pinconf-generic.o
>>   obj-$(CONFIG_OF)             += devicetree.o
>>
>>   obj-$(CONFIG_PINCTRL_AMD)    += pinctrl-amd.o
>> +obj-$(CONFIG_PINCTRL_AMDISP) += pinctrl-amdisp.o
>>   obj-$(CONFIG_PINCTRL_APPLE_GPIO) += pinctrl-apple-gpio.o
>>   obj-$(CONFIG_PINCTRL_ARTPEC6)        += pinctrl-artpec6.o
>>   obj-$(CONFIG_PINCTRL_AS3722) += pinctrl-as3722.o
>> diff --git a/drivers/pinctrl/pinctrl-amdisp.c b/drivers/pinctrl/pinctrl-amdisp.c
>> new file mode 100644
>> index 000000000000..659406586cb2
>> --- /dev/null
>> +++ b/drivers/pinctrl/pinctrl-amdisp.c
>> @@ -0,0 +1,290 @@
>> +/* 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.
> 
> Same problems as with rest of AMD patches.
> 

Done. Updated copyright header.

>> + *
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
> 
> Where do you use it?
> 
>> +#include <linux/platform_device.h>
>> +#include <linux/gpio/driver.h>
>> +#include <linux/pinctrl/machine.h>
>> +#include <linux/slab.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/interrupt.h>
> 
> Where do you use half of these headers?
> 

Done. Yes, most of them are not used, removed in v2.

>> +#include "core.h"
>> +#include "pinctrl-utils.h"
>> +#include "pinctrl-amd.h"
>> +#include "pinctrl-amdisp.h"
>> +
>> +#define DRV_NAME             "amdisp-pinctrl"
>> +#define GPIO_CONTROL_PIN     4
>> +#define GPIO_OFFSET_0                0x0
>> +#define GPIO_OFFSET_1                0x4
>> +#define GPIO_OFFSET_2                0x50
> 
> ...
> 
>> +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;
>> +     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;
>> +     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
>> +
>> +     grange->id              = 0;
>> +     grange->pin_base        = 0;
>> +     grange->base            = 0;
>> +     grange->pins            = amdisp_range_pins;
>> +     grange->npins           = ARRAY_SIZE(amdisp_range_pins);
>> +     grange->name            = gc->label;
>> +     grange->gc              = gc;
>> +
>> +     ret = devm_gpiochip_add_data(&pdev->dev, gc, pctrl);
>> +     if (ret)
>> +             return ret;
>> +
>> +     pinctrl_add_gpio_range(pctrl->pctrl, grange);
>> +
>> +     dev_info(&pdev->dev, "register amdisp gpio controller\n");
> 
> Drop
> 

Done. Removed noisy success prints.

>> +     return 0;
>> +}
>> +#endif
>> +
>> +static int amdisp_pinctrl_probe(struct platform_device *pdev)
>> +{
>> +     struct amdisp_pinctrl *pctrl;
>> +     struct resource *res;
>> +     int ret;
>> +
>> +     pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
>> +     if (!pctrl)
>> +             return -ENOMEM;
>> +
>> +     pdev->dev.init_name = DRV_NAME;
>> +#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
>> +     platform_set_drvdata(pdev, pctrl);
>> +
>> +     pctrl->dev = &pdev->dev;
>> +     pctrl->data = &amdisp_pinctrl_data;
>> +     pctrl->desc.owner = THIS_MODULE;
>> +     pctrl->desc.pctlops = &amdisp_pinctrl_ops;
>> +     pctrl->desc.pmxops = NULL;
>> +     pctrl->desc.name = dev_name(&pdev->dev);
>> +     pctrl->desc.pins = pctrl->data->pins;
>> +     pctrl->desc.npins = pctrl->data->npins;
>> +     ret = devm_pinctrl_register_and_init(&pdev->dev, &pctrl->desc,
>> +                                          pctrl, &pctrl->pctrl);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = pinctrl_enable(pctrl->pctrl);
>> +     if (ret)
>> +             return ret;
>> +
>> +#ifdef CONFIG_GPIOLIB
>> +     ret = amdisp_gpiochip_add(pdev, pctrl);
>> +     if (ret)
>> +             return ret;
>> +#endif
>> +     dev_info(&pdev->dev, "amdisp pinctrl init successful\n");
> 
> Drop, useless. Not mentioning that drivers should be silent on success.
> 
Done.


>> +     return 0;
>> +}
>> +
>> +static struct platform_driver amdisp_pinctrl_driver = {
>> +     .driver = {
>> +             .name = DRV_NAME,
>> +     },
>> +     .probe = amdisp_pinctrl_probe,
>> +};
>> +
>> +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);
>> +
>> +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");
> 
> No, that's not true. Just like in other patch...
> 
Done. Updated license info.

>> +MODULE_ALIAS("platform:" DRV_NAME);
> 
> 
> 
> Best regards,
> Krzysztof