On Wed, Dec 02, 2020 at 05:32:21PM +0800, Luo Jiaxing wrote:
> This GPIO driver is for HiSilicon's ARM SoC.
>
> HiSilicon's GPIO controller support double-edge interrupt and multi-core
> concurrent access.
>
> ACPI table example for this GPIO controller:
> Device (GPO0)
> {
> Name (_HID, "HISI0184")
> Device (PRTA)
> {
> Name (_ADR, Zero)
> Name (_UID, Zero)
> Name (_DSD, Package (0x01)
> {
> Package (0x02)
> {
> "hisi-ngpio",
Can it be standard property?
Please, fix firmware.
> 0x20
> }
> })
> }
> }
...
> +config GPIO_HISI
> + tristate "HISILICON GPIO controller driver"
> + depends on (ARM64 && ACPI) || COMPILE_TEST
This is wrong. (Homework to understand why. Also see below)
> + select GPIO_GENERIC
> + select GENERIC_IRQ_CHIP
> + help
> + Say Y or M here to build support for the HiSilicon GPIO controller
> driver
> + GPIO block.
> + This controller support double-edge interrupt and multi-core
> concurrent
> + access.
No module name?
...
> +/*
> + * Copyright (c) 2020 HiSilicon Limited.
> + */
One line.
...
> +#include <linux/acpi.h>
Don't see user of it (but see above and below as well).
At the same time missed mod_devicetable.h.
> +#include <linux/gpio/driver.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
...
> +#include "gpiolib.h"
> +#include "gpiolib-acpi.h"
Any user of this?
...
> +#define HISI_GPIO_SWPORT_DR_SET_WX 0x0
...
> +#define HISI_GPIO_INT_DEDGE_SET 0xb0
...
> +#define HISI_GPIO_REG_MAX 0x100
Use fixed width for register offsets, like:
0x000
...
0x0b0
...
0x100
...
> +struct hisi_gpio {
> + struct device *dev;
> + void __iomem *reg_base;
> + unsigned int pin_num;
> + struct gpio_chip chip;
Moving this to be a first member of the struct will make corresponding
container_of() no-op.
> + struct irq_chip irq_chip;
> + int irq;
> +};
...
> + unsigned long mask = BIT(off);
No need to have temporary variable. Use directly BIT(off) which fits into 80.
> +
> + if (debounce)
> + hisi_gpio_write_reg(chip, HISI_GPIO_DEBOUNCE_SET_WX, mask);
> + else
> + hisi_gpio_write_reg(chip, HISI_GPIO_DEBOUNCE_CLR_WX, mask);
...
> + switch (config_para) {
> + case PIN_CONFIG_INPUT_DEBOUNCE:
> + config_arg = pinconf_to_config_argument(config);
> + hisi_gpio_set_debounce(chip, offset, config_arg);
> + break;
Move...
> + default:
> + return -ENOTSUPP;
> + }
> +
> + return 0;
...this above.
...
> + /* Return 0 if output, 1 if input */
Useless comment.
...
> +static int hisi_gpio_irq_set_type(struct irq_data *d, u32 type)
> +{
> + struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> + unsigned int mask = BIT(irqd_to_hwirq(d));
> +
> + switch (type) {
> + case IRQ_TYPE_EDGE_BOTH:
> + hisi_gpio_write_reg(chip, HISI_GPIO_INT_DEDGE_SET, mask);
> + break;
> + case IRQ_TYPE_EDGE_RISING:
> + hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_SET_WX, mask);
> + hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_SET_WX, mask);
> + break;
> + case IRQ_TYPE_EDGE_FALLING:
> + hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_SET_WX, mask);
> + hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_CLR_WX, mask);
> + break;
> + case IRQ_TYPE_LEVEL_HIGH:
> + hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_CLR_WX, mask);
> + hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_SET_WX, mask);
> + break;
> + case IRQ_TYPE_LEVEL_LOW:
> + hisi_gpio_write_reg(chip, HISI_GPIO_INTTYPE_EDGE_CLR_WX, mask);
> + hisi_gpio_write_reg(chip, HISI_GPIO_INT_POLARITY_CLR_WX, mask);
> + default:
> + return -EINVAL;
> + }
> +
> + /*
> + * The dual-edge interrupt and other interrupt's registers do not
> + * take effect at the same time. The registers of the two-edge
> + * interrupts have higher priorities, the configuration of
> + * the dual-edge interrupts must be disabled before the configuration
> + * of other kind of interrupts.
> + */
This comment sounds like below should be moved before switch-case. Can you
elaborate?
> + if (type != IRQ_TYPE_EDGE_BOTH) {
> + unsigned int both = hisi_gpio_read_reg(chip,
> HISI_GPIO_INT_DEDGE_ST);
> +
> + if (both & mask)
> + hisi_gpio_write_reg(chip, HISI_GPIO_INT_DEDGE_CLR,
> mask);
> + }
> +
> + if (type & IRQ_TYPE_LEVEL_MASK)
> + irq_set_handler_locked(d, handle_level_irq);
> + else if (type & IRQ_TYPE_EDGE_BOTH)
> + irq_set_handler_locked(d, handle_edge_irq);
> +
> + return 0;
> +}
...
> + while (irq_msk) {
> + int hwirq = fls(irq_msk) - 1;
> + irq_msk &= ~BIT(hwirq);
> + }
NIH of for_each_set_bit().
...
> + res = bgpio_init(&hisi_gpio->chip, hisi_gpio->dev, HISI_GPIO_REG_SIZE,
> dat, set,
> + clr, NULL, NULL, 0);
> + if (res) {
> + dev_err(dev, "failed to init\n");
> + return res;
> + }
Wondering if you can use regmap GPIO.
...
> +static struct platform_driver hisi_gpio_driver = {
> + .driver = {
> + .name = HISI_GPIO_DRIVER_NAME,
> + .acpi_match_table = ACPI_PTR(hisi_gpio_acpi_match),
This is wrong. If you use COMPILE_TEST the ACPI_PTR in !ACPI case is no op.
Compiler will warn you about unused variable. Have you compile tested it in
such conditions?
Hint: remove ACPI_PTR(). In 99% this macro shouldn't be used.
> + },
> + .probe = hisi_gpio_probe,
> +};
> +
Redundant blank line.
> +module_platform_driver(hisi_gpio_driver);
--
With Best Regards,
Andy Shevchenko
