gpio/aspeed-sgpio: enable access to all 80 input & output sgpios

And also removing the enabling of IRQs. This part makes sense, as it's what the commit message says.

If you think a sensible default should be single edge (and I would agree with that change), perhaps update the comment to say "set single edge trigger mode" and mention it in your commit message.

Currently, the aspeed-sgpio driver exposes up to 80 GPIO lines, corresponding to the 80 status bits available in hardware. Each of these lines can be configured as either an input or an output.

However, each of these GPIOs is actually an input *and* an output; we actually have 80 inputs plus 80 outputs.

This change expands the maximum number of GPIOs to 160; the lower half of this range are the input-only GPIOs, the upper half are the outputs. We fix the GPIO directions to correspond to this mapping.

This also fixes a bug when setting GPIOs - we were reading from the input register, making it impossible to set more than one output GPIO.

Signed-off-by Jeremy Kerr <[email protected]>

---
 .../devicetree/bindings/gpio/sgpio-aspeed.txt |   5 +-
 drivers/gpio/gpio-aspeed-sgpio.c              | 115 +++++++++++-------
 2 files changed, 77 insertions(+), 43 deletions(-)

drivers/gpio/gpio-aspeed-sgpio.c +0 -0

index 8319812593e3..927d46f159b8 100644

drivers/gpio/gpio-aspeed-sgpio.c +4 -0

+static void aspeed_sgpio_irq_init_valid_mask(struct gpio_chip *gc,
+               unsigned long *valid_mask, unsigned int ngpios)
+{
+       struct aspeed_sgpio *sgpio = gpiochip_get_data(gc);

drivers/gpio/gpio-aspeed-sgpio.c +4 -2

+{
+       return offset < SGPIO_OUTPUT_OFFSET;
+}
 
 static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
 {
        struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
        unsigned long flags;
+       int rc;
 
-       spin_lock_irqsave(&gpio->lock, flags);
-

A Fixes: might be a good idea.

drivers/gpio/gpio-aspeed-sgpio.c line 21

Quoted 3 lines

drivers/gpio/gpio-aspeed-sgpio.c +2 -1

-#define MAX_NR_SGPIO                   80
+#define MAX_NR_HW_SGPIO                        80
+#define SGPIO_OUTPUT_OFFSET            MAX_NR_HW_SGPIO

A short comment explaining what's going on with these defines (as you did in your commit message) will help future reviewers.

Quoted 5 lines

drivers/gpio/gpio-aspeed-sgpio.c +4 -0

+static void aspeed_sgpio_irq_init_valid_mask(struct gpio_chip *gc,
+               unsigned long *valid_mask, unsigned int ngpios)
+{
+       struct aspeed_sgpio *sgpio = gpiochip_get_data(gc);

drivers/gpio/gpio-aspeed-sgpio.c static void __iomem *bank_reg(struct aspeed_sgpio *gpio, · line 162

Quoted 10 lines

drivers/gpio/gpio-aspeed-sgpio.c +10 -0

+       int n = sgpio->n_sgpio;
+
+       WARN_ON(ngpios < MAX_NR_HW_SGPIO * 2);
+
+       /* input GPIOs in the lower range */
+       bitmap_set(valid_mask, 0, n);
+       bitmap_clear(valid_mask, n, ngpios - n);
+}
+
+static const bool aspeed_sgpio_is_input(unsigned int offset)

The 0day bot complained about the 'const' here.

Quoted 16 lines

drivers/gpio/gpio-aspeed-sgpio.c +6 -4

+{
+       return offset < SGPIO_OUTPUT_OFFSET;
+}
 static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
 {
        struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
        unsigned long flags;
+       int rc;
 
-       spin_lock_irqsave(&gpio->lock, flags);
-
-       gpio->dir_in[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
-       sgpio_set_value(gc, offset, val);
+       /* No special action is required for setting the direction; we'll
+        * error-out in sgpio_set_value if this isn't an output GPIO */

drivers/gpio/gpio-aspeed-sgpio.c static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned int offset, int val) · line 242

Quoted 5 lines

drivers/gpio/gpio-aspeed-sgpio.c +2 -0

+       spin_lock_irqsave(&gpio->lock, flags);
+       rc = sgpio_set_value(gc, offset, val);
        spin_unlock_irqrestore(&gpio->lock, flags);
 
        return 0;

I think this should be 'return rc'

Hi Joel,

Thanks for the review!

Quoted 1 line

A Fixes: might be a good idea.

OK, given this isn't strictly (just) a fix, should I split that out?

Quoted 7 lines

drivers/gpio/gpio-aspeed-sgpio.c line 21

Quoted 4 lines

drivers/gpio/gpio-aspeed-sgpio.c +2 -1

-#define MAX_NR_SGPIO                   80
+#define MAX_NR_HW_SGPIO                        80
+#define SGPIO_OUTPUT_OFFSET            MAX_NR_HW_SGPIO

A short comment explaining what's going on with these defines (as you did in your commit message) will help future reviewers.

Sounds good, I'll add one.

Quoted 17 lines

Quoted 5 lines

drivers/gpio/gpio-aspeed-sgpio.c +4 -0

+static void aspeed_sgpio_irq_init_valid_mask(struct gpio_chip *gc,
+               unsigned long *valid_mask, unsigned int ngpios)
+{
+       struct aspeed_sgpio *sgpio = gpiochip_get_data(gc);

drivers/gpio/gpio-aspeed-sgpio.c static void __iomem *bank_reg(struct aspeed_sgpio *gpio, · line 162

Quoted 10 lines

drivers/gpio/gpio-aspeed-sgpio.c +10 -0

+       int n = sgpio->n_sgpio;
+
+       WARN_ON(ngpios < MAX_NR_HW_SGPIO * 2);
+
+       /* input GPIOs in the lower range */
+       bitmap_set(valid_mask, 0, n);
+       bitmap_clear(valid_mask, n, ngpios - n);
+}
+
+static const bool aspeed_sgpio_is_input(unsigned int offset)

The 0day bot complained about the 'const' here.

ack, will remove.

Quoted 16 lines

Quoted 16 lines

drivers/gpio/gpio-aspeed-sgpio.c +6 -4

+{
+       return offset < SGPIO_OUTPUT_OFFSET;
+}
 static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
 {
        struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
        unsigned long flags;
+       int rc;
 
-       spin_lock_irqsave(&gpio->lock, flags);
-
-       gpio->dir_in[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
-       sgpio_set_value(gc, offset, val);
+       /* No special action is required for setting the direction; we'll
+        * error-out in sgpio_set_value if this isn't an output GPIO */

Quoted 7 lines

drivers/gpio/gpio-aspeed-sgpio.c static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned int offset, int val) · line 242

Quoted 5 lines

drivers/gpio/gpio-aspeed-sgpio.c +2 -0

+       spin_lock_irqsave(&gpio->lock, flags);
+       rc = sgpio_set_value(gc, offset, val);
        spin_unlock_irqrestore(&gpio->lock, flags);
 
        return 0;

I think this should be 'return rc'

Yup. I'll send a v2 with these changes.

Cheers,

Jeremy