gpio: ADI GPIO2 driver for BF54x and BF60x.
PATCH 1/1 gpio: ADI GPIO2 driver for BF54x and BF60x.
From: Sonic Zhang <[email protected]>
The new ADI GPIO2 controller was introduced since the BF548 and BF60x processors. It differs a lot from the old one on BF5xx processors. So, create a ADI GPIO2 platform driver in the GPIO driver folder.
- Define gpio ports as individual platform devices.
- Define gpio interrupt controllers as individual platform devices.
- Register the platform pint driver before the platform port driver.
- Probe pint devices before port devices. Put device instances into respective gpio and pint lists.
- Suport peripheral function request API via seeking proper devices instances in the gpio and pint lists.
- Define peripheral, irq and gpio reservation bit masks for each gpio port as runtime resources.
- Static resource request in early platform probe stage is not supported.
- Save and restore gpio port and pint status MMRs in syscore PM functions.
--- drivers/gpio/Makefile | 1 + drivers/gpio/gpio-adi2.c | 1287 +++++++++++++++++++++++++++++++ include/linux/platform_data/gpio-adi2.h | 26 + 3 files changed, 1314 insertions(+) create mode 100644 drivers/gpio/gpio-adi2.c create mode 100644 include/linux/platform_data/gpio-adi2.h
drivers/gpio/Makefile +1 -0
@@ -11,6 +11,7 @@ obj-$(CONFIG_GPIO_ACPI) += gpiolib-acpi.o obj-$(CONFIG_GPIO_GENERIC) += gpio-generic.o obj-$(CONFIG_GPIO_74X164) += gpio-74x164.o +obj-$(CONFIG_GPIO_ADI2) += gpio-adi2.o obj-$(CONFIG_GPIO_ADNP) += gpio-adnp.o obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
drivers/gpio/gpio-adi2.c new file +1287 -0
1,287 lines not on this page · raw message
include/linux/platform_data/gpio-adi2.h new file +26 -0
@@ -0,0 +1,26 @@ +/* +* GPIO Driver for ADI GPIO2 controller +* +* Copyright 2007-2013 Analog Devices Inc. +* +* Licensed under the GPL-2 or later +*/ + + +#ifndef GPIO_ADI2_H +#define GPIO_ADI2_H + +#include <linux/io.h> +#include <linux/platform_device.h> + +struct adi_gpio_platform_data { +int port_pin_base;/* optional, 0 - driver decides */ +int port_width; +u8 pint_id;/* which pint to map the gpio port */ +u8 pint_assign;/* 0 - assgin to 1ow 16 bits +* 1 - assign to high 16 bits +*/ +u8 pint_map;/* port mapping mask in pint */ +}; + +#endif
Signature · 1 line
1.8.2.3
drivers/gpio/Makefile obj-$(CONFIG_GPIO_ACPI) += gpiolib-acpi.o · line 14
Quoted 2 lines · Sonic Zhang
drivers/gpio/Makefile +1
-0+obj-$(CONFIG_GPIO_ADI2) += gpio-adi2.o
Where is CONFIG_GPIO_ADI2 defines?
You didn't include any change to Kconfig.
(...)
drivers/gpio/Makefile line 6
Quoted 8 lines
+++
b/drivers/gpio/gpio-adi2.cdrivers/gpio/Makefile +6
-0@@ -0,0 +1,1287 @@ +/* + * GPIO Driver for ADI GPIO2 controller + * + * Copyright 2007-2013 Analog Devices Inc. + * + * Licensed under the GPL-2 or later
Nitpick: we usually write "GPLv2"
drivers/gpio/Makefile line 12
Quoted 6 lines
drivers/gpio/Makefile +6
-0+struct gpio_reserve_map { + unsigned char owner[RESOURCE_LABEL_SIZE]; + unsigned char irq_enabled:1; + unsigned char rsv_gpio:1; + unsigned char rsv_int:1; + unsigned char rsv_peri:1;
All these should be bool instead.
drivers/gpio/Makefile line 13
Quoted 1 line
drivers/gpio/Makefile +1
-0+};
Write some piece of kerneldoc describing what this struct is for.
drivers/gpio/Makefile line 18
Quoted 5 lines
drivers/gpio/Makefile +5
-0+struct gpio_port_saved { + unsigned short fer; + unsigned short data; + unsigned short dir; + unsigned short inen;
Switch all to type u16.
drivers/gpio/Makefile line 19
Quoted 1 line
drivers/gpio/Makefile +1
-0+ unsigned int mux;
Switch to u32?
drivers/gpio/Makefile line 20
Quoted 1 line
drivers/gpio/Makefile +1
-0+};
Kerneldoc this thing. Is it a sleep register state container?
drivers/gpio/Makefile line 24
Quoted 4 lines
drivers/gpio/Makefile +4
-0+struct gpio_pint { + struct list_head node; + void __iomem *base; /* Port interrupt controller MMR base */ + int irq; /* Port interrupt controller demux IRQ */
What is a "demux IRQ"? Don't you mean it is a parent IRQ for the entire port, that need to be demuxed to figure out which line fired?
drivers/gpio/Makefile line 28
Quoted 4 lines
drivers/gpio/Makefile +4
-0+ int gpio_irq_base[2]; /* [0] is gpio irq base in low 16 bit pint. + * [1] is gpio irq base in high 16 bit pint. + */ + struct gpio_pint_regs *regs;
This struct gpio_pint_regs is not part of the patch!
Plus it seems to use the RTOS common style of mapping a struct over a register range and assigning members in the code. Get rid of the struct altogether and use read[l|w|b] and write[l|w|b] from <linux/io.h> or possibly the _relaxed variants to hammer the registers directly.
drivers/gpio/Makefile line 30
Quoted 2 lines
drivers/gpio/Makefile +2
-0+ struct adi_pm_pint_save saved_data; + int map_count;
This looks like it should be unsigned.
drivers/gpio/Makefile line 31
Quoted 1 line
drivers/gpio/Makefile +1
-0+ spinlock_t lock;
Define what this is locking. The list?
drivers/gpio/Makefile line 34
Quoted 3 lines
drivers/gpio/Makefile +3
-0+ + int (*pint_map_port)(struct gpio_pint *pint, u8 assign, + u8 map, int irq_base);
This looks like some attempt to reimplement irqdomains.
drivers/gpio/Makefile line 35
Quoted 1 line
drivers/gpio/Makefile +1
-0+};
Write proper kerneldoc instead of inline comments.
drivers/gpio/Makefile line 39
Quoted 4 lines
drivers/gpio/Makefile +4
-0+struct gpio_port { + struct list_head node; + void __iomem *base; + int pin_base;
If you're referencing pins to GPIOs this driver should be in drivers/pinctrl, read Documentation/pinctrl.txt
drivers/gpio/Makefile line 40
Quoted 1 line
drivers/gpio/Makefile +1
-0+ int irq_base;
This is an indication that you should be using irqdomain.
drivers/gpio/Makefile line 41
Quoted 1 line
drivers/gpio/Makefile +1
-0+ int width;
Are these three unsigned?
drivers/gpio/Makefile line 47
Quoted 6 lines
drivers/gpio/Makefile +6
-0+ struct gpio_port_t *regs; + struct gpio_port_saved saved_data; + struct device *dev; + + struct gpio_pint *pint; + u8 pint_map; /* port mapping mask in pint */
Masking exactly what? Details here.
drivers/gpio/Makefile line 50
Quoted 3 lines
drivers/gpio/Makefile +3
-0+ u8 pint_assign; /* 0 - assgin to pint 1ow 16 bits + * 1 - assign to pint high 16 bits + */
Speling is strange, I don't understand anything of this comment. Suspect this is part of the irqdomain reimplementation.
drivers/gpio/Makefile line 56
Quoted 6 lines
drivers/gpio/Makefile +6
-0+ + spinlock_t lock; + struct gpio_chip chip; + + struct gpio_reserve_map rsvmap[]; +};
Use kerneldoc again.
Quoted 10 lines
drivers/gpio/Makefile +10
-0+static inline unsigned offset_to_gpio(struct gpio_port *port, unsigned offset) +{ + return offset + port->chip.base; +} + +static inline unsigned gpio_to_offset(struct gpio_port *port, unsigned gpio) +{ + return gpio - port->chip.base; +} +
drivers/gpio/Makefile line 76
Quoted 10 lines
drivers/gpio/Makefile +10
-0+static inline unsigned irq_to_offset(struct gpio_port *port, int irq) +{ + int offset; + + offset = irq - port->irq_base; + if (offset >= 0 && offset < port->width) + return offset; + else + return 0; +}
Switch all to type u8.
Use irqdomain instead. Check other GPIO drivers for examples.
drivers/gpio/Makefile line 80
Quoted 4 lines
drivers/gpio/Makefile +4
-0+static inline unsigned irq_to_pintbit(struct gpio_port *port, int irq) +{ + return (1 << irq_to_offset(port, irq)) << (port->pint_assign * 16); +}
Dito, needs refactoring.
drivers/gpio/Makefile line 85
Quoted 5 lines
drivers/gpio/Makefile +5
-0+static void gpio_error(struct gpio_port *port, unsigned offset) +{ + dev_err(port->dev, "gpio-adi2: GPIO %d wasn't requested!\n", + offset_to_gpio(port, offset)); +}
No need for a simple helper like that, inline this into the code wherever it's needed.
drivers/gpio/Makefile line 95
Quoted 10 lines
drivers/gpio/Makefile +10
-0+static void set_label(struct gpio_port *port, unsigned offset, + const char *label) +{ + char *pch = port->rsvmap[offset].owner; + + if (label) { + strncpy(pch, label, RESOURCE_LABEL_SIZE); + pch[RESOURCE_LABEL_SIZE - 1] = 0; + } +}
Dito.
drivers/gpio/Makefile line 101
Quoted 6 lines
drivers/gpio/Makefile +6
-0+static char *get_label(struct gpio_port *port, unsigned offset) +{ + char *pch = port->rsvmap[offset].owner; + + return *pch ? pch : "UNKNOWN"; +}
Dito.
drivers/gpio/Makefile line 113
Quoted 12 lines
drivers/gpio/Makefile +12
-0+static int cmp_label(struct gpio_port *port, unsigned offset, const char *label) +{ + if (label == NULL) { + dump_stack(); + dev_err(port->dev, "Please provide none-null label\n"); + } + + if (label) + return strcmp(port->rsvmap[offset].owner, label); + else + return -EINVAL; +}
Dito.
Just too simple helpers.
Quoted 5 lines
drivers/gpio/Makefile +5
-0+static inline unsigned int is_gpio_irq_enabled(struct gpio_port *port, + unsigned offset) +{ + return port->rsvmap[offset].irq_enabled; +}
drivers/gpio/Makefile line 128
Quoted 10 lines
drivers/gpio/Makefile +10
-0+ +static inline void enable_gpio_irq(struct gpio_port *port, unsigned offset) +{ + port->rsvmap[offset].irq_enabled = 1; +} + +static inline void disable_gpio_irq(struct gpio_port *port, unsigned offset) +{ + port->rsvmap[offset].irq_enabled = 0; +}
SInce you're only reading/writing to the memory cache of these registers these functions are not really doing what they say they are doing.
Why are you not reading/writing the real registers?
Quoted 4 lines
drivers/gpio/Makefile +4
-0+static inline unsigned int is_reserved(struct gpio_port *port, char type, + unsigned offset) +{ + switch (type) {
drivers/gpio/Makefile line 142
Quoted 10 lines
drivers/gpio/Makefile +10
-0+ case RSV_GPIO: + return port->rsvmap[offset].rsv_gpio == 1; + case RSV_INT: + return port->rsvmap[offset].rsv_int == 1; + case RSV_PERI: + return port->rsvmap[offset].rsv_peri == 1; + } + + return 0; +}
After switching to bool use = true assignment instead.
Quoted 4 lines
drivers/gpio/Makefile +4
-0+static inline void reserve(struct gpio_port *port, char type, unsigned offset) +{ + switch (type) { + case RSV_GPIO:
drivers/gpio/Makefile line 156
Quoted 10 lines
drivers/gpio/Makefile +10
-0+ port->rsvmap[offset].rsv_gpio = 1; + break; + case RSV_INT: + port->rsvmap[offset].rsv_int = 1; + break; + case RSV_PERI: + port->rsvmap[offset].rsv_peri = 1; + break; + } +}
Dito.
Quoted 4 lines
drivers/gpio/Makefile +4
-0+static inline void unreserve(struct gpio_port *port, char type, unsigned offset) +{ + switch (type) { + case RSV_GPIO:
drivers/gpio/Makefile line 170
Quoted 10 lines
drivers/gpio/Makefile +10
-0+ port->rsvmap[offset].rsv_gpio = 0; + break; + case RSV_INT: + port->rsvmap[offset].rsv_int = 0; + break; + case RSV_PERI: + port->rsvmap[offset].rsv_peri = 0; + break; + } +}
= false;
drivers/gpio/Makefile line 171
Quoted 1 line
drivers/gpio/Makefile +1
-0+static struct gpio_port *find_gpio_port(unsigned gpio)
The parameter should be named "offset" because this cannot be a global Linux GPIO number.
Quoted 26 lines
drivers/gpio/Makefile +26
-0+{ + struct gpio_port *port; + + list_for_each_entry(port, &adi_gpio_list, node) + if (gpio >= port->chip.base && + gpio < port->chip.base + port->width) + break; + + if (&port->node != &adi_gpio_list) + return port; + else + return NULL; +} + +static struct gpio_pint *find_gpio_pint(unsigned pint_id) +{ + struct gpio_pint *pint; + int i = 0; + + list_for_each_entry(pint, &adi_pint_list, node) { + if (pint_id == i) + break; + i++; + } + + if (&pint->node != &adi_pint_list)
drivers/gpio/Makefile line 207
Quoted 10 lines
drivers/gpio/Makefile +10
-0+ return pint; + else + return NULL; +} + + +static void __adi_gpio_irq_prepare(struct gpio_port *port, unsigned offset); +static int __adi_gpio_irq_request(struct gpio_port *port, unsigned offset, + const char *label); +static void __adi_gpio_irq_free(struct gpio_port *port, unsigned offset);
Can you rearrange the code to avoid forward-declarations like these?
drivers/gpio/Makefile line 216
Quoted 9 lines
drivers/gpio/Makefile +9
-0+static void adi_gpio_ack_irq(struct irq_data *d) +{ + unsigned long flags; + struct gpio_port *port = irq_data_get_irq_chip_data(d); + struct gpio_pint_regs *regs = port->pint->regs; + unsigned pintbit = irq_to_pintbit(port, d->irq); + + spin_lock_irqsave(&port->lock, flags); + spin_lock_irqsave(&port->pint->lock, flags);
Two locks to ACK an IRQ? Seems overengineered.
Quoted 12 lines
drivers/gpio/Makefile +12
-0+ if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH) { + if (regs->invert_set & pintbit) + regs->invert_clear = pintbit; + else + regs->invert_set = pintbit; + } + + regs->request = pintbit; + + spin_unlock_irqrestore(&port->pint->lock, flags); + spin_unlock_irqrestore(&port->lock, flags); +}
drivers/gpio/Makefile line 238
Quoted 10 lines
drivers/gpio/Makefile +10
-0+ +static void adi_gpio_mask_ack_irq(struct irq_data *d) +{ + unsigned long flags; + struct gpio_port *port = irq_data_get_irq_chip_data(d); + struct gpio_pint_regs *regs = port->pint->regs; + unsigned pintbit = irq_to_pintbit(port, d->irq); + + spin_lock_irqsave(&port->lock, flags); + spin_lock_irqsave(&port->pint->lock, flags);
Dito.
Then this stuff:
Quoted 4 lines
drivers/gpio/Makefile +4
-0+ if (regs->invert_set & pintbit) + regs->invert_clear = pintbit; + else + regs->invert_set = pintbit;
(...)
Quoted 2 lines
drivers/gpio/Makefile +2
-0+static void __adi_gpio_mask_irq(struct gpio_port *port, int irq) + regs->mask_clear = pintbit;
(...)
drivers/gpio/Makefile
Quoted 2 lines
drivers/gpio/Makefile +2
-0+static void __adi_gpio_unmask_irq(struct gpio_port *port, int irq) + regs->mask_set = pintbit;
This as mentioned shall be replaced with proper IO accessor macros (read[lwb]/write[lwb]).
(..)
drivers/gpio/Makefile
Quoted 11 lines
drivers/gpio/Makefile +11
-0+/*********************************************************** +* +* FUNCTIONS: ADI Processor Peripheral Resource Allocation +* and PortMux Setup +* +* INPUTS/OUTPUTS: +* per Peripheral Identifier +* label String +* +* DESCRIPTION: ADI Processor Peripheral Resource Allocation and Setup API +**************************************************************/
Skip this large boilerplate stuff. Looks like some extract from $RTOS codebase. We are short and concise in Linux.
Quoted 69 lines
drivers/gpio/Makefile +69
-0+int peripheral_request(unsigned short per, const char *label) +{ + unsigned long flags; + unsigned short ident = P_IDENT(per); + struct gpio_port *port; + unsigned offset; + + /* + * Don't cares are pins with only one dedicated function + */ + + if (per & P_DONTCARE) + return 0; + + if (!(per & P_DEFINED)) + return -ENODEV; + + port = find_gpio_port(ident); + if (port == NULL) + return -ENODEV; + + spin_lock_irqsave(&port->lock, flags); + + offset = gpio_to_offset(port, ident); + + /* If a pin can be muxed as either GPIO or peripheral, make + * sure it is not already a GPIO pin when we request it. + */ + if (unlikely(is_reserved(port, RSV_GPIO, offset))) { + if (system_state == SYSTEM_BOOTING) + dump_stack(); + dev_err(port->dev, + "%s: Peripheral %d is already reserved as GPIO by %s !\n", + __func__, ident, get_label(port, offset)); + spin_unlock_irqrestore(&port->lock, flags); + return -EBUSY; + } + + if (unlikely(is_reserved(port, RSV_PERI, offset))) { + + /* + * Pin functions like AMC address strobes my + * be requested and used by several drivers + */ + + if (!((per & P_MAYSHARE) && get_portmux(port, offset) == + P_FUNCT2MUX(per))) { + /* + * Allow that the identical pin function can + * be requested from the same driver twice + */ + + if (cmp_label(port, offset, label) == 0) + goto anyway; + + if (system_state == SYSTEM_BOOTING) + dump_stack(); + dev_err(port->dev, + "%s: Peripheral %d function %d is already reserved by %s !\n", + __func__, ident, P_FUNCT2MUX(per), + get_label(port, offset)); + spin_unlock_irqrestore(&port->lock, flags); + return -EBUSY; + } + } + + anyway: + reserve(port, RSV_PERI, offset); +
drivers/gpio/Makefile
Quoted 10 lines
drivers/gpio/Makefile +10
-0+ portmux_setup(port, offset, P_FUNCT2MUX(per)); + port_setup(port, offset, PERIPHERAL_USAGE); + + set_label(port, offset, label); + + spin_unlock_irqrestore(&port->lock, flags); + + return 0; +} +EXPORT_SYMBOL(peripheral_request);
This entire functionality shall be rewritten to use the pinctrl subsystem.
Don't bother to send anything resembling this again, custom accessor functions for muxing pins is a thing of the past.
(...)
+++ b/include/linux/platform_data/gpio-adi2.h
(...)
drivers/gpio/Makefile
Quoted 2 lines
drivers/gpio/Makefile +2
-0+struct adi_gpio_platform_data { + int port_pin_base; /* optional, 0 - driver decides */
Use pinctrl subsystem.
Quoted 1 line
+ int
port_width;
unsigned.
drivers/gpio/Makefile
Quoted 4 lines
drivers/gpio/Makefile +4
-0+ u8 pint_id; /* which pint to map the gpio port */ + u8 pint_assign; /* 0 - assgin to 1ow 16 bits + * 1 - assign to high 16 bits + */
Still not getting this.
Quoted 1 line
+ u8
pint_map; /* port mapping mask in pint */
Use irqdomain.
Overall this code has very many issues, even if you actually manage to get it through checkpatch it doesn't mean the code is in good shape.
Please do atleast the following before the next iteration:
- Put driver in drivers/pinctrl, implement a proper muxing interface instead of the custom stuff. Create a combined pinctrl+GPIO driver, look at other drivers for guidance.
- Make sure the code can compile! This is missing critical structs etc, and cannot possibly build.
- Get rid of the horrid struct gpio_pint_regs and replace with proper IO accessors.
- Use irqdomain for IRQ demuxing.
- Remove overengineering like needlessly splitting simple oneliners into helper functions.
Yours,
Linus Walleij
Hi Linus,
Quoted 8 lines · Linus Walleij
drivers/gpio/Makefile obj-$(CONFIG_GPIO_ACPI) += gpiolib-acpi.o · line 14
Quoted 2 lines · Sonic Zhang
drivers/gpio/Makefile +1
-0+obj-$(CONFIG_GPIO_ADI2) += gpio-adi2.oWhere is
CONFIG_GPIO_ADI2defines?You didn't include any change to
Kconfig.
The CONFIG_GPIO_ADI2 is selected automatically in arch/blackfin/Kconfig if CPU families BF54x and BF60x are selected.
Quoted 12 lines
(...)
drivers/gpio/Makefile line 6
Quoted 8 lines
+++
b/drivers/gpio/gpio-adi2.cdrivers/gpio/Makefile +6
-0@@ -0,0 +1,1287 @@ +/* + * GPIO Driver for ADI GPIO2 controller + * + * Copyright 2007-2013 Analog Devices Inc. + * + * Licensed under the GPL-2 or laterNitpick: we usually write "GPLv2"
OK
Quoted 9 lines
drivers/gpio/Makefile line 12
Quoted 7 lines
drivers/gpio/Makefile +6
-0+struct gpio_reserve_map { + unsigned char owner[RESOURCE_LABEL_SIZE]; + unsigned char irq_enabled:1; + unsigned char rsv_gpio:1; + unsigned char rsv_int:1; + unsigned char rsv_peri:1;All these should be bool instead.
OK
Quoted 5 lines
drivers/gpio/Makefile line 13
Quoted 2 lines
drivers/gpio/Makefile +1
-0+};Write some piece of kerneldoc describing what this struct is for.
Quoted 7 lines
drivers/gpio/Makefile line 18
Quoted 5 lines
drivers/gpio/Makefile +5
-0+struct gpio_port_saved { + unsigned short fer; + unsigned short data; + unsigned short dir; + unsigned short inen;Switch all to type u16.
OK
Quoted 4 lines
drivers/gpio/Makefile line 19
Quoted 2 lines
drivers/gpio/Makefile +1
-0+ unsigned int mux;Switch to u32?
OK
Quoted 4 lines
drivers/gpio/Makefile line 20
Quoted 2 lines
drivers/gpio/Makefile +1
-0+};Kerneldoc this thing. Is it a sleep register state container?
Yes, it is used in power suspend and resume functions.
Quoted 9 lines
drivers/gpio/Makefile line 24
Quoted 5 lines
drivers/gpio/Makefile +4
-0+struct gpio_pint { + struct list_head node; + void __iomem *base; /* Port interrupt controller MMR base */ + int irq; /* Port interrupt controller demux IRQ */What is a "demux IRQ"? Don't you mean it is a parent IRQ for the entire port, that need to be demuxed to figure out which line fired?
Yes, maybe parent IRQ is more clear.
Quoted 7 lines
drivers/gpio/Makefile line 28
Quoted 5 lines
drivers/gpio/Makefile +4
-0+ int gpio_irq_base[2]; /* [0] is gpio irq base in low 16 bit pint. + * [1] is gpio irq base in high 16 bit pint. + */ + struct gpio_pint_regs *regs;This struct
gpio_pint_regsis not part of the patch!
This structure is defined under blackfin arch folder. See file arch/blackfin/mach-bf609/include/mach/irq.h:303 , file arch/blackfin/mach-bf609/include/mach/irq.h:438 and arch/blackfin/include/asm/gpio.h:123 .
Quoted 6 lines
Plus it seems to use the RTOS common style of mapping a struct over a register range and assigning members in the code. Get rid of the struct altogether and use read[l|w|b] and write[l|w|b] from <
linux/io.h> or possibly the_relaxedvariants to hammer the registers directly.
OK. I will change all register access into read[l|w|b] and write[l|w|b].
Quoted 5 lines
drivers/gpio/Makefile line 30
Quoted 3 lines
drivers/gpio/Makefile +2
-0+ struct adi_pm_pint_save saved_data; + int map_count;This looks like it should be unsigned.
OK
Quoted 4 lines
drivers/gpio/Makefile line 31
Quoted 2 lines
drivers/gpio/Makefile +1
-0+ spinlock_t lock;Define what this is locking. The list?
This lock is used to protect the access to each GPIO interrupt controller. Multiple ADI GPIO banks can be mapped into one GPIO interrupt controller. This lock make sure the irq_chip operations to one GPIO interrupt controller for different GPIO interrrupts are atomic.
Please fine the details in bf609 HRM page 535 and 536
http://www.analog.com/static/imported-files/processor_manuals/blackfin_hwr_bf60x_rev0.5.pdf
Quoted 6 lines
drivers/gpio/Makefile line 34
Quoted 4 lines
drivers/gpio/Makefile +3
-0+ + int (*pint_map_port)(struct gpio_pint *pint, u8 assign, + u8 map, int irq_base);This looks like some attempt to reimplement irqdomains.
No. This functions is to set up the mapping between one GPIO interrupt controller and multiple GPIO banks other than to set up the irq_chip and handler for each GPIO IRQ.
Quoted 4 lines
drivers/gpio/Makefile line 35
Quoted 2 lines
drivers/gpio/Makefile +1
-0+};Write proper kerneldoc instead of inline comments.
OK
Quoted 8 lines
drivers/gpio/Makefile line 39
Quoted 5 lines
drivers/gpio/Makefile +4
-0+struct gpio_port { + struct list_head node; + void __iomem *base; + int pin_base;If you're referencing pins to GPIOs this driver should be in
drivers/pinctrl, readDocumentation/pinctrl.txt
OK, I am trying to understand how to adapt this driver into pinctrl.
Quoted 4 lines
drivers/gpio/Makefile line 40
Quoted 2 lines
drivers/gpio/Makefile +1
-0+ int irq_base;This is an indication that you should be using irqdomain.
OK
Quoted 4 lines
drivers/gpio/Makefile line 41
Quoted 2 lines
drivers/gpio/Makefile +1
-0+ int width;Are these three unsigned?
OK
Quoted 9 lines
drivers/gpio/Makefile line 47
Quoted 7 lines
drivers/gpio/Makefile +6
-0+ struct gpio_port_t *regs; + struct gpio_port_saved saved_data; + struct device *dev; + + struct gpio_pint *pint; + u8 pint_map; /* port mapping mask in pint */Masking exactly what? Details here.
Different GPIO bank has different mapping code in the GPIO interrupt controller. See bf609 HRM page 609.
Quoted 8 lines
drivers/gpio/Makefile line 50
Quoted 4 lines
drivers/gpio/Makefile +3
-0+ u8 pint_assign; /* 0 - assgin to pint 1ow 16 bits + * 1 - assign to pint high 16 bits + */Speling is strange, I don't understand anything of this comment. Suspect this is part of the irqdomain reimplementation.
The 32-bit GPIO interrupt registers can be divided into 2 parts. A 16-pin GPIO bank can be mapped into either low 16 bits or high 16 bits of each interrupt register.
Quoted 8 lines
drivers/gpio/Makefile line 56
Quoted 6 lines
drivers/gpio/Makefile +6
-0+ + spinlock_t lock; + struct gpio_chip chip; + + struct gpio_reserve_map rsvmap[]; +};Use kerneldoc again.
OK
Quoted 14 lines
Quoted 14 lines
drivers/gpio/Makefile +13
-0+static inline unsigned offset_to_gpio(struct gpio_port *port, unsigned offset) +{ + return offset + port->chip.base; +} + +static inline unsigned gpio_to_offset(struct gpio_port *port, unsigned gpio) +{ + return gpio - port->chip.base; +} + +static inline unsigned irq_to_offset(struct gpio_port *port, int irq) +{ + int offset;
Quoted 8 lines
drivers/gpio/Makefile line 76
Quoted 7 lines
drivers/gpio/Makefile +7
-0+ + offset = irq - port->irq_base; + if (offset >= 0 && offset < port->width) + return offset; + else + return 0; +}Switch all to type u8.
OK
Quoted 10 lines
Use irqdomain instead. Check other GPIO drivers for examples.
drivers/gpio/Makefile line 80
Quoted 4 lines
drivers/gpio/Makefile +4
-0+static inline unsigned irq_to_pintbit(struct gpio_port *port, int irq) +{ + return (1 << irq_to_offset(port, irq)) << (port->pint_assign * 16); +}Dito, needs refactoring.
Quoted 8 lines
drivers/gpio/Makefile line 85
Quoted 5 lines
drivers/gpio/Makefile +5
-0+static void gpio_error(struct gpio_port *port, unsigned offset) +{ + dev_err(port->dev, "gpio-adi2: GPIO %d wasn't requested!\n", + offset_to_gpio(port, offset)); +}No need for a simple helper like that, inline this into the code wherever it's needed.
OK
Quoted 38 lines
drivers/gpio/Makefile line 95
Quoted 11 lines
drivers/gpio/Makefile +10
-0+static void set_label(struct gpio_port *port, unsigned offset, + const char *label) +{ + char *pch = port->rsvmap[offset].owner; + + if (label) { + strncpy(pch, label, RESOURCE_LABEL_SIZE); + pch[RESOURCE_LABEL_SIZE - 1] = 0; + } +}Dito.
drivers/gpio/Makefile line 101
Quoted 6 lines
drivers/gpio/Makefile +6
-0+static char *get_label(struct gpio_port *port, unsigned offset) +{ + char *pch = port->rsvmap[offset].owner; + + return *pch ? pch : "UNKNOWN"; +}Dito.
drivers/gpio/Makefile line 113
Quoted 12 lines
drivers/gpio/Makefile +12
-0+static int cmp_label(struct gpio_port *port, unsigned offset, const char *label) +{ + if (label == NULL) { + dump_stack(); + dev_err(port->dev, "Please provide none-null label\n"); + } + + if (label) + return strcmp(port->rsvmap[offset].owner, label); + else + return -EINVAL; +}Dito.
Quoted 1 line
Just too simple helpers.
Yes, but these helpers make the caller easier to read and understand.
Quoted 20 lines
Quoted 6 lines
drivers/gpio/Makefile +5
-0+static inline unsigned int is_gpio_irq_enabled(struct gpio_port *port, + unsigned offset) +{ + return port->rsvmap[offset].irq_enabled; +}drivers/gpio/Makefile line 128
Quoted 10 lines
drivers/gpio/Makefile +10
-0+ +static inline void enable_gpio_irq(struct gpio_port *port, unsigned offset) +{ + port->rsvmap[offset].irq_enabled = 1; +} + +static inline void disable_gpio_irq(struct gpio_port *port, unsigned offset) +{ + port->rsvmap[offset].irq_enabled = 0; +}SInce you're only reading/writing to the memory cache of these registers these functions are not really doing what they say they are doing.
Quoted 1 line
Why are you not reading/writing the real registers?
Yes, reading the real register is fine although a bit slower.
Quoted 17 lines
Quoted 5 lines
drivers/gpio/Makefile +4
-0+static inline unsigned int is_reserved(struct gpio_port *port, char type, + unsigned offset) +{ + switch (type) {drivers/gpio/Makefile line 142
Quoted 10 lines
drivers/gpio/Makefile +10
-0+ case RSV_GPIO: + return port->rsvmap[offset].rsv_gpio == 1; + case RSV_INT: + return port->rsvmap[offset].rsv_int == 1; + case RSV_PERI: + return port->rsvmap[offset].rsv_peri == 1; + } + + return 0; +}After switching to bool use = true assignment instead.
OK
Quoted 35 lines
Quoted 5 lines
drivers/gpio/Makefile +4
-0+static inline void reserve(struct gpio_port *port, char type, unsigned offset) +{ + switch (type) { + case RSV_GPIO:drivers/gpio/Makefile line 156
Quoted 10 lines
drivers/gpio/Makefile +10
-0+ port->rsvmap[offset].rsv_gpio = 1; + break; + case RSV_INT: + port->rsvmap[offset].rsv_int = 1; + break; + case RSV_PERI: + port->rsvmap[offset].rsv_peri = 1; + break; + } +}Dito.
Quoted 4 lines
drivers/gpio/Makefile +4
-0+static inline void unreserve(struct gpio_port *port, char type, unsigned offset) +{ + switch (type) { + case RSV_GPIO:drivers/gpio/Makefile line 170
Quoted 10 lines
drivers/gpio/Makefile +10
-0+ port->rsvmap[offset].rsv_gpio = 0; + break; + case RSV_INT: + port->rsvmap[offset].rsv_int = 0; + break; + case RSV_PERI: + port->rsvmap[offset].rsv_peri = 0; + break; + } +}= false;
Quoted 4 lines
drivers/gpio/Makefile line 171
Quoted 1 line
drivers/gpio/Makefile +1
-0+static struct gpio_port *find_gpio_port(unsigned gpio)The parameter should be named "offset" because this cannot be a global Linux GPIO number.
This helper is used by peripheral_request() which passes a global GPIO number according to the blackfin legacy design. This is useless if we migrate to the pinctrl framework successfully.
Quoted 31 lines
Quoted 31 lines
drivers/gpio/Makefile +30
-0+{ + struct gpio_port *port; + + list_for_each_entry(port, &adi_gpio_list, node) + if (gpio >= port->chip.base && + gpio < port->chip.base + port->width) + break; + + if (&port->node != &adi_gpio_list) + return port; + else + return NULL; +} + +static struct gpio_pint *find_gpio_pint(unsigned pint_id) +{ + struct gpio_pint *pint; + int i = 0; + + list_for_each_entry(pint, &adi_pint_list, node) { + if (pint_id == i) + break; + i++; + } + + if (&pint->node != &adi_pint_list) + return pint; + else + return NULL; +}
Quoted 8 lines
drivers/gpio/Makefile line 207
Quoted 6 lines
drivers/gpio/Makefile +6
-0+ + +static void __adi_gpio_irq_prepare(struct gpio_port *port, unsigned offset); +static int __adi_gpio_irq_request(struct gpio_port *port, unsigned offset, + const char *label); +static void __adi_gpio_irq_free(struct gpio_port *port, unsigned offset);Can you rearrange the code to avoid forward-declarations like these?
OK
Quoted 12 lines
drivers/gpio/Makefile line 216
Quoted 10 lines
drivers/gpio/Makefile +9
-0+static void adi_gpio_ack_irq(struct irq_data *d) +{ + unsigned long flags; + struct gpio_port *port = irq_data_get_irq_chip_data(d); + struct gpio_pint_regs *regs = port->pint->regs; + unsigned pintbit = irq_to_pintbit(port, d->irq); + + spin_lock_irqsave(&port->lock, flags); + spin_lock_irqsave(&port->pint->lock, flags);Two locks to ACK an IRQ? Seems overengineered.
That's the result of mapping multiple GPIO banks to one GPIO interrupt controller. See former explanation.
Quoted 33 lines
Quoted 13 lines
drivers/gpio/Makefile +12
-0+ if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH) { + if (regs->invert_set & pintbit) + regs->invert_clear = pintbit; + else + regs->invert_set = pintbit; + } + + regs->request = pintbit; + + spin_unlock_irqrestore(&port->pint->lock, flags); + spin_unlock_irqrestore(&port->lock, flags); +}drivers/gpio/Makefile line 238
Quoted 10 lines
drivers/gpio/Makefile +10
-0+ +static void adi_gpio_mask_ack_irq(struct irq_data *d) +{ + unsigned long flags; + struct gpio_port *port = irq_data_get_irq_chip_data(d); + struct gpio_pint_regs *regs = port->pint->regs; + unsigned pintbit = irq_to_pintbit(port, d->irq); + + spin_lock_irqsave(&port->lock, flags); + spin_lock_irqsave(&port->pint->lock, flags);Dito.
Then this stuff:
Quoted 4 lines
drivers/gpio/Makefile +4
-0+ if (regs->invert_set & pintbit) + regs->invert_clear = pintbit; + else + regs->invert_set = pintbit;(...)
Quoted 8 lines
Quoted 2 lines
drivers/gpio/Makefile +2
-0+static void __adi_gpio_mask_irq(struct gpio_port *port, int irq) + regs->mask_clear = pintbit;(...)
drivers/gpio/Makefile
Quoted 2 lines
drivers/gpio/Makefile +2
-0+static void __adi_gpio_unmask_irq(struct gpio_port *port, int irq) + regs->mask_set = pintbit;This as mentioned shall be replaced with proper IO accessor macros (read[lwb]/write[lwb]).
OK
Quoted 16 lines
(..)
drivers/gpio/Makefile
Quoted 11 lines
drivers/gpio/Makefile +11
-0+/*********************************************************** +* +* FUNCTIONS: ADI Processor Peripheral Resource Allocation +* and PortMux Setup +* +* INPUTS/OUTPUTS: +* per Peripheral Identifier +* label String +* +* DESCRIPTION: ADI Processor Peripheral Resource Allocation and Setup API +**************************************************************/Skip this large boilerplate stuff. Looks like some extract from $RTOS codebase. We are short and concise in Linux.
OK
Quoted 83 lines
Quoted 70 lines
drivers/gpio/Makefile +69
-0+int peripheral_request(unsigned short per, const char *label) +{ + unsigned long flags; + unsigned short ident = P_IDENT(per); + struct gpio_port *port; + unsigned offset; + + /* + * Don't cares are pins with only one dedicated function + */ + + if (per & P_DONTCARE) + return 0; + + if (!(per & P_DEFINED)) + return -ENODEV; + + port = find_gpio_port(ident); + if (port == NULL) + return -ENODEV; + + spin_lock_irqsave(&port->lock, flags); + + offset = gpio_to_offset(port, ident); + + /* If a pin can be muxed as either GPIO or peripheral, make + * sure it is not already a GPIO pin when we request it. + */ + if (unlikely(is_reserved(port, RSV_GPIO, offset))) { + if (system_state == SYSTEM_BOOTING) + dump_stack(); + dev_err(port->dev, + "%s: Peripheral %d is already reserved as GPIO by %s !\n", + __func__, ident, get_label(port, offset)); + spin_unlock_irqrestore(&port->lock, flags); + return -EBUSY; + } + + if (unlikely(is_reserved(port, RSV_PERI, offset))) { + + /* + * Pin functions like AMC address strobes my + * be requested and used by several drivers + */ + + if (!((per & P_MAYSHARE) && get_portmux(port, offset) == + P_FUNCT2MUX(per))) { + /* + * Allow that the identical pin function can + * be requested from the same driver twice + */ + + if (cmp_label(port, offset, label) == 0) + goto anyway; + + if (system_state == SYSTEM_BOOTING) + dump_stack(); + dev_err(port->dev, + "%s: Peripheral %d function %d is already reserved by %s !\n", + __func__, ident, P_FUNCT2MUX(per), + get_label(port, offset)); + spin_unlock_irqrestore(&port->lock, flags); + return -EBUSY; + } + } + + anyway: + reserve(port, RSV_PERI, offset); +drivers/gpio/Makefile
Quoted 10 lines
drivers/gpio/Makefile +10
-0+ portmux_setup(port, offset, P_FUNCT2MUX(per)); + port_setup(port, offset, PERIPHERAL_USAGE); + + set_label(port, offset, label); + + spin_unlock_irqrestore(&port->lock, flags); + + return 0; +} +EXPORT_SYMBOL(peripheral_request);This entire functionality shall be rewritten to use the pinctrl subsystem.
Quoted 2 lines
Don't bother to send anything resembling this again, custom accessor functions for muxing pins is a thing of the past.
Yes, I am looking at how to do it now.
Quoted 4 lines
(...)
+++b/include/linux/platform_data/gpio-adi2.h
(...)
Quoted 8 lines
drivers/gpio/Makefile
Quoted 2 lines
drivers/gpio/Makefile +2
-0+struct adi_gpio_platform_data { + int port_pin_base; /* optional, 0 - driver decides */Use pinctrl subsystem.
Quoted 1 line
+ int
port_width;unsigned.
OK
Quoted 11 lines
drivers/gpio/Makefile
Quoted 5 lines
drivers/gpio/Makefile +4
-0+ u8 pint_id; /* which pint to map the gpio port */ + u8 pint_assign; /* 0 - assgin to 1ow 16 bits + * 1 - assign to high 16 bits + */Still not getting this.
Quoted 1 line
+ u8
pint_map; /* port mapping mask in pint */Use irqdomain.
See my explanation upper.
Quoted 10 lines
Overall this code has very many issues, even if you actually manage to get it through checkpatch it doesn't mean the code is in good shape.
Please do atleast the following before the next iteration:
- Put driver in
drivers/pinctrl, implement a proper muxing interface instead of the custom stuff. Create a combined pinctrl+GPIO driver, look at other drivers for guidance.
I am looking at how to do it now.
Quoted 4 lines
- Make sure the code can compile! This is missing critical structs etc, and cannot possibly build.
I can include the patch for blackfin arch folder and then you can compile for Blackfin.
Quoted 2 lines
- Get rid of the horrid struct
gpio_pint_regsand replace with proper IO accessors.
I guess readl() writel() can access the register addresses defined in struct gpio_pint_regs?
Quoted 3 lines
- Use irqdomain for IRQ demuxing.
OK
Quoted 2 lines
- Remove overengineering like needlessly splitting simple oneliners into helper functions.
OK
Thanks
Sonic Zhang