Hi Luo!
thanks for your patch! I see that Andy already provided a crowd
of comments, here are some more!
On Wed, Dec 2, 2020 at 10:32 AM Luo Jiaxing <luojiaxing@xxxxxxxxxx> wrote:
+config GPIO_HISIThanks for using the generic driver!
+ tristate "HISILICON GPIO controller driver"
+ depends on (ARM64 && ACPI) || COMPILE_TEST
+ select GPIO_GENERIC
+#include <linux/acpi.h>No GPIO drivers should include these files. If you need
+#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"
gpiolib.h then describe in a comment right above the include
why you have to do this.
I don't know about gpiolig-acpi.h but not unless Andy
says it is OK it is not OK.
+#define HISI_GPIO_SWPORT_DR_SET_WX 0x0Nice idea with the double edge register! Hats off to the hardware
+#define HISI_GPIO_SWPORT_DR_CLR_WX 0x4
+#define HISI_GPIO_SWPORT_DDR_SET_WX 0x10
+#define HISI_GPIO_SWPORT_DDR_CLR_WX 0x14
+#define HISI_GPIO_SWPORT_DDR_ST_WX 0x18
+#define HISI_GPIO_INTEN_SET_WX 0x20
+#define HISI_GPIO_INTEN_CLR_WX 0x24
+#define HISI_GPIO_INTMASK_SET_WX 0x30
+#define HISI_GPIO_INTMASK_CLR_WX 0x34
+#define HISI_GPIO_INTTYPE_EDGE_SET_WX 0x40
+#define HISI_GPIO_INTTYPE_EDGE_CLR_WX 0x44
+#define HISI_GPIO_INT_POLARITY_SET_WX 0x50
+#define HISI_GPIO_INT_POLARITY_CLR_WX 0x54
+#define HISI_GPIO_DEBOUNCE_SET_WX 0x60
+#define HISI_GPIO_DEBOUNCE_CLR_WX 0x64
+#define HISI_GPIO_INTSTATUS_WX 0x70
+#define HISI_GPIO_PORTA_EOI_WX 0x78
+#define HISI_GPIO_EXT_PORT_WX 0x80
+#define HISI_GPIO_INTCOMB_MASK_WX 0xa0
+#define HISI_GPIO_INT_DEDGE_SET 0xb0
+#define HISI_GPIO_INT_DEDGE_CLR 0xb4
+#define HISI_GPIO_INT_DEDGE_ST 0xb8
engineer who created this simple yet powerful GPIO.
+#define HISI_GPIO_REG_SIZE 0x4This seems like a bit surplus definitions, I prefer to just inline
+#define HISI_GPIO_REG_MAX 0x100
+#define HISI_GPIO_PIN_NUM_MAX 32
these. Some use cases will go away after you start using
bgpio_init().
+struct hisi_gpio {I prefer "line_num", the reason I usually use the term "lines"
+ struct device *dev;
+ void __iomem *reg_base;
+ unsigned int pin_num;
rather than "pins" is that some GPIO lines are internal in
chips and do not always go out to external pins.
+ struct gpio_chip chip;Do you need to keep irq around in the state?
+ struct irq_chip irq_chip;
+ int irq;
+static inline u32 hisi_gpio_read_reg(struct gpio_chip *chip,OK it is a bit of reusing the register accessors inside the
+ unsigned int off)
+{
+ struct hisi_gpio *hisi_gpio =
+ container_of(chip, struct hisi_gpio, chip);
+
+ return chip->read_reg(hisi_gpio->reg_base + off);
+}
+
+static inline void hisi_gpio_write_reg(struct gpio_chip *chip,
+ unsigned int off, u32 val)
+{
+ struct hisi_gpio *hisi_gpio =
+ container_of(chip, struct hisi_gpio, chip);
+
+ chip->write_reg(hisi_gpio->reg_base + off, val);
+}
GPIO chip generic MMIO abstraction, but to me it is really
better if you just address the registers directly.
The indirections through read_reg/write_reg doesn't
really buy you anything.
+static void hisi_gpio_set_debounce(struct gpio_chip *chip, unsigned int off,So debounce is just on/off? No ability to set "how much" or a timer?
+ u32 debounce)
+{
+ unsigned long mask = BIT(off);
+
+ 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);
+}
Someone must be guessing inside the block that a certain number
of ms is perfect(TM) debouncing or is there a register for this that
you are not showing us? Like register 0x68 or 0x6c...
+static int hisi_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio)These get replaced by the appropriate parameters to bgpio_init().
+static int hisi_gpio_direction_output(struct gpio_chip *chip,
+ unsigned int gpio,
+ int val)
+static int hisi_gpio_direction_input(struct gpio_chip *chip,
+ unsigned int gpio)
Read the doc in gpio-mmci.c above the function
bgpio_init() for help. If you still can't figure it out, ask and
describe your problem and we'll hash it out.
+ /*Nice with this comment and overall the IRQ code looks very
+ * 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.
+ */
+ 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);
+ }
good. Nice job!
+static void hisi_gpio_irq_enable(struct irq_data *d)Interesting with a GPIO hardware that both as enable and mask
+{
+ struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+
+ hisi_gpio_irq_clr_mask(d);
+ hisi_gpio_write_reg(chip, HISI_GPIO_INTEN_SET_WX, BIT(irqd_to_hwirq(d)));
+}
+
+static void hisi_gpio_irq_disable(struct irq_data *d)
+{
+ struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+
+ hisi_gpio_irq_set_mask(d);
+ hisi_gpio_write_reg(chip, HISI_GPIO_INTEN_CLR_WX, BIT(irqd_to_hwirq(d)));
+}
bits. I can't see why, usually they just have masks but I suppose
there is some reason.
+static void hisi_gpio_irq_handler(struct irq_desc *desc)What about just:
+{
+ struct hisi_gpio *hisi_gpio = irq_desc_get_handler_data(desc);
+ u32 irq_msk = hisi_gpio_read_reg(&hisi_gpio->chip,
+ HISI_GPIO_INTSTATUS_WX);
+ struct irq_chip *irq_c = irq_desc_get_chip(desc);
+
+ chained_irq_enter(irq_c, desc);
+ while (irq_msk) {
+ int hwirq = fls(irq_msk) - 1;
+ int gpio_irq = irq_find_mapping(hisi_gpio->chip.irq.domain,
+ hwirq);
+
+ generic_handle_irq(gpio_irq);
+ irq_msk &= ~BIT(hwirq);
+ }
for_each_set_bit(hwirq, &irq_msk, gc->ngpio)
generic_handle_irq(irq_find_mapping(hisi_gpio->chip.irq.domain,
hwirq));
+ device_for_each_child_node(dev, fwnode) {Since the registers are only 32 bits suppose this should emit a
+ if (fwnode_property_read_u32(fwnode, "hisi-ngpio",
+ &hisi_gpio->pin_num)) {
+ dev_err(dev,
+ "failed to get number of gpios for port%d and use default value instead\n",
+ idx);
+ hisi_gpio->pin_num = HISI_GPIO_PIN_NUM_MAX;
+ }
fat warning if line_num is > 32.
+ dat = hisi_gpio->reg_base + HISI_GPIO_EXT_PORT_WX;That's a lot of variables for my taste, I usually just pass the registers
+ set = hisi_gpio->reg_base + HISI_GPIO_SWPORT_DR_SET_WX;
+ clr = hisi_gpio->reg_base + HISI_GPIO_SWPORT_DR_CLR_WX;
+
+ res = bgpio_init(&hisi_gpio->chip, hisi_gpio->dev, HISI_GPIO_REG_SIZE, dat, set,
+ clr, NULL, NULL, 0);
to the bgpio_init() call directly, split across a few lines, but it is a
matter of taste. Make sure you get the direction setting to be handled
by gpio-mmio as well as described above.
+ hisi_gpio->chip.direction_output = hisi_gpio_direction_output;So these should be handled by generic GPIO.
+ hisi_gpio->chip.direction_input = hisi_gpio_direction_input;
+ hisi_gpio->chip.get_direction = hisi_gpio_get_direction;
+ hisi_gpio->chip.set_config = hisi_gpio_set_config;But these are fine.
+ hisi_gpio->chip.ngpio = hisi_gpio->pin_num;
+ hisi_gpio->chip.base = -1;
Apart from this address everything Andy commented on as well.
Yours,
Linus Walleij
.