From: Andy Shevchenko <andriy.shevchenko@intel.com>
To: Asmaa Mnebhi <asmaa@nvidia.com>
Cc: linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-acpi@vger.kernel.org
Subject: Re: [PATCH v3 1/2] Support NVIDIA BlueField-3 GPIO controller
Date: Sat, 11 Feb 2023 13:51:32 +0200 [thread overview]
Message-ID: <Y+eBRLB0Q38vGtjR@smile.fi.intel.com> (raw)
In-Reply-To: <c1bf45a868edcd3df5263fa76a32b28e6c9ca3d1.1676042188.git.asmaa@nvidia.com>
On Fri, Feb 10, 2023 at 10:39:40AM -0500, Asmaa Mnebhi wrote:
> This patch adds support for the BlueField-3 SoC GPIO driver
The Submitting Patches states that imperative speech should be used.
> which allows:
> - setting certain GPIOs as interrupts from other dependent drivers
> - ability to manipulate certain GPIO pins via libgpiod tools
>
> BlueField-3 has 56 GPIOs but the user is only allowed to change some
> of them into GPIO mode. Use valid_mask to make it impossible to alter
> the rest of the GPIOs.
...
> + help
> + Say Y here if you want GPIO support on Mellanox BlueField 3 SoC.
Have you run checkpatch? Nowadays 3+ lines of help is recommended.
I would suggest to add a standard phrase about module name in case
the module build is chosen.
...
> +// SPDX-License-Identifier: GPL-2.0-only or BSD-3-Clause
> +
Redundant blank line
> +/*
> + * Copyright (C) 2022 NVIDIA CORPORATION & AFFILIATES
> + */
This can be on one line.
...
> +#include <linux/acpi.h>
No user of this header.
> +#include <linux/gpio/driver.h>
> +#include <linux/platform_device.h>
Approx dozen of header inclusions are missing.
(bits.h, types.h, spinlock.h, ...)
...
> +struct mlxbf3_gpio_context {
> + struct gpio_chip gc;
> + struct irq_chip irq_chip;
Have you run it on v6.1+ kernels? This should not be here, i.e. it must be
static and const.
> + /* YU GPIO block address */
> + void __iomem *gpio_io;
> +
> + /* YU GPIO cause block address */
> + void __iomem *gpio_cause_io;
> +
> + /* Mask of valid gpios that can be accessed by software */
> + unsigned int valid_mask;
> +};
...
> + generic_handle_irq(irq_find_mapping(gc->irq.domain, level));
There is a helper that unites these two calls together.
...
> + bool fall = false;
> + bool rise = false;
Instead, just assign each of the in the corresponding switch-case.
> + switch (type & IRQ_TYPE_SENSE_MASK) {
> + case IRQ_TYPE_EDGE_BOTH:
> + fall = true;
> + rise = true;
> + break;
> + case IRQ_TYPE_EDGE_RISING:
> + rise = true;
> + break;
> + case IRQ_TYPE_EDGE_FALLING:
> + fall = true;
> + break;
> + default:
> + return -EINVAL;
> + }
...
> + raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> + if (fall) {
> + val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
> + val |= BIT(offset);
> + writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN);
> + }
> +
> + if (rise) {
> + val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
> + val |= BIT(offset);
> + writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN);
> + }
> + raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
Don't you need to choose and lock the IRQ handler here?
> +}
...
> +static int mlxbf3_gpio_init_valid_mask(struct gpio_chip *gc,
> + unsigned long *valid_mask,
> + unsigned int ngpios)
> +{
> + struct mlxbf3_gpio_context *gs = gpiochip_get_data(gc);
> +
> + *valid_mask = gs->valid_mask;
> +
> + return 0;
> +}
Why do you need this?
> + struct resource *res;
Useless variable, see below.
...
> + const char *name;
> + name = dev_name(dev);
Useless, just call dev_name() where it's needed.
...
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENODEV;
> +
> + /* Resource shared with pinctrl driver */
> + gs->gpio_io = devm_ioremap(dev, res->start, resource_size(res));
> + if (!gs->gpio_io)
> + return -ENOMEM;
> +
> + /* YU GPIO block address */
> + gs->gpio_cause_io = devm_platform_ioremap_resource(pdev, 1);
> + if (IS_ERR(gs->gpio_cause_io))
> + return PTR_ERR(gs->gpio_cause_io);
These can be folded in a single devm_platform_ioremap_resource() call.
...
> + if (device_property_read_u32(dev, "npins", &npins))
> + npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
You can get of conditional.
npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
device_property_read_u32(dev, "npins", &npins);
...
> + if (device_property_read_u32(dev, "valid_mask", &valid_mask))
> + valid_mask = 0x0;
Besides that you can move this directly to the respective call and drop
redundant private copy of valid mask, you mean that without that property
no valid GPIOs?
> + gs->valid_mask = valid_mask;
...
> + girq->handler = handle_simple_irq;
Should be handle_bad_irq() to avoid some subtle issues that hard to debug.
...
> + ret = devm_request_irq(dev, irq, mlxbf3_gpio_irq_handler,
> + IRQF_SHARED, name, gs);
> + if (ret) {
> + dev_err(dev, "failed to request IRQ");
> + return ret;
return dev_err_probe(...);
> + }
> + }
...
> + ret = devm_gpiochip_add_data(dev, &gs->gc, gs);
> + if (ret) {
> + dev_err(dev, "Failed adding memory mapped gpiochip\n");
> + return ret;
Ditto.
> + }
...
> +static const struct acpi_device_id __maybe_unused mlxbf3_gpio_acpi_match[] = {
> + { "MLNXBF33", 0 },
> + {},
No comma for termination entry.
> +};
...
> + .probe = mlxbf3_gpio_probe,
> +};
> +
Redundant blank line.
> +module_platform_driver(mlxbf3_gpio_driver);
--
With Best Regards,
Andy Shevchenko
next prev parent reply other threads:[~2023-02-11 11:51 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-10 15:39 [PATCH v3 0/2] Add NVIDIA BlueField-3 GPIO driver and pin controller Asmaa Mnebhi
2023-02-10 15:39 ` [PATCH v3 1/2] Support NVIDIA BlueField-3 GPIO controller Asmaa Mnebhi
2023-02-11 7:37 ` kernel test robot
2023-02-11 11:51 ` Andy Shevchenko [this message]
2023-02-13 23:14 ` Asmaa Mnebhi
2023-02-13 23:39 ` andy.shevchenko
2023-02-10 15:39 ` [PATCH v3 2/2] Support NVIDIA BlueField-3 pinctrl driver Asmaa Mnebhi
2023-02-11 11:55 ` Andy Shevchenko
2023-02-16 17:50 ` Asmaa Mnebhi
2023-02-16 18:38 ` Andy Shevchenko
2023-02-16 18:44 ` Asmaa Mnebhi
2023-02-16 18:53 ` Andy Shevchenko
2023-02-16 19:26 ` Asmaa Mnebhi
2023-02-11 11:58 ` [PATCH v3 0/2] Add NVIDIA BlueField-3 GPIO driver and pin controller Andy Shevchenko
2023-02-17 21:26 ` [PATCH v4 0/2] Support Nvidia " Asmaa Mnebhi
2023-02-17 21:26 ` [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support Asmaa Mnebhi
2023-02-17 23:07 ` Andy Shevchenko
2023-02-22 18:40 ` Asmaa Mnebhi
2023-02-23 11:06 ` Andy Shevchenko
2023-02-23 19:08 ` Asmaa Mnebhi
2023-02-23 19:58 ` Andy Shevchenko
2023-02-23 22:51 ` Asmaa Mnebhi
2023-02-24 9:47 ` Andy Shevchenko
2023-02-24 14:42 ` Asmaa Mnebhi
2023-02-24 15:12 ` Andy Shevchenko
2023-02-24 15:59 ` Asmaa Mnebhi
2023-02-24 15:13 ` Andy Shevchenko
2023-02-24 15:14 ` Andy Shevchenko
2023-02-18 1:05 ` kernel test robot
2023-02-23 12:29 ` Linus Walleij
2023-02-17 21:26 ` [PATCH v4 2/2] pinctrl: pinctrl-mlxbf: Add pinctrl " Asmaa Mnebhi
2023-02-17 23:24 ` Andy Shevchenko
2023-02-23 21:07 ` Asmaa Mnebhi
2023-02-24 9:44 ` Andy Shevchenko
2023-03-14 20:30 ` Asmaa Mnebhi
[not found] <20230210150319.200799-1-asmaa@nvidia.com>
[not found] ` <20230210150319.200799-2-asmaa@nvidia.com>
2023-02-13 10:17 ` [PATCH v3 1/2] Support NVIDIA BlueField-3 GPIO controller Linus Walleij
2023-02-13 23:18 ` Asmaa Mnebhi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y+eBRLB0Q38vGtjR@smile.fi.intel.com \
--to=andriy.shevchenko@intel.com \
--cc=asmaa@nvidia.com \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).