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

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

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.

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

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

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

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

> +MODULE_ALIAS("platform:" DRV_NAME);



Best regards,
Krzysztof