Re: [PATCH v1 1/3] gpio: gpio-hisi: Add HiSilicon GPIO support
From: luojiaxing
Date: Fri Dec 04 2020 - 04:40:01 EST
Hi
On 2020/12/2 18:04, Andy Shevchenko wrote:
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?
sure, I think you mean that "ngpios" should be used here.
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)]
I think it should be
depends on (ARM64 || COMPILE_TEST) && ACPI
+ 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?
sorry, I didn't get what you mean. What module name should I add here?
...
+/*
+ * Copyright (c) 2020 HiSilicon Limited.
+ */
One line.
ok
...
+#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.
sure, let me check it.
+#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?
This should be deleted, I used to use
acpi_gpiochip_request_interrupts(), but delete it later.
...
+#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
ok
...
+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.
sure
+ 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.
sure
+
+ 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.
Sorry, what do you mean by Move ... this above?
...
+ /* Return 0 if output, 1 if input */
Useless comment.
will be deleted
...
+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?
Our GPIO controller uses two separate registers to enable/disable the
double-edge interrupt,
and once enable double-edge, the setting of edge and polarity register()
will be ignored by the hardware.
Therefore, each time we update trigger type, an extra check is required:
if the interrupt is not double-edges, ensure that it is disabled.
+ 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().
sure, it's better than fls
...
+ 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.
I looked at gpio-regmap.c, which is a newly uploaded feature. I think
it's good, but I'd rather keep the current design.
Because it means my code needs to be modified and re-tested, this will
take a while, but the actual business needs will not allow it.
...
+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.
sure
+ },
+ .probe = hisi_gpio_probe,
+};
+
Redundant blank line.
sure
Thanks
Jiaxing
+module_platform_driver(hisi_gpio_driver);