| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <acfb7d13-2aad-4841-bf85-b99fe405bc79@amd.com>
Date: Tue, 4 Mar 2025 18:22:31 -0500
From: "Nirujogi, Pratap" <pnirujog@....com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
Pratap Nirujogi <pratap.nirujogi@....com>, linus.walleij@...aro.org
Cc: linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
benjamin.chan@....com, bin.du@....com, king.li@....com,
gjorgji.rosikopulos@....com, dominic.antony@....com
Subject: Re: [PATCH] pinctrl: amd: isp411: Add amdisp GPIO pinctrl
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@....com>");
>> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@....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
Powered by blists - more mailing lists