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
