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

[Nirujogi, Pratap]

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