Re: [PATCH 3/6] gpio: Add GPIO support for the ST Multi-Function eXpander

On 02/14/2018 04:30 PM, Andy Shevchenko wrote:

On Thu, Feb 8, 2018 at 4:27 PM, Amelie Delaunay [off-list ref] wrote:

quoted

ST Multi-Function eXpander (MFX) can be used as GPIO expander.
It has 16 fast GPIOs and can have 8 extra alternate GPIOs
when other MFX features are not enabled.

quoted

+config GPIO_ST_MFX
+       bool "ST-MFX GPIOs"
+       depends on MFD_ST_MFX
+       depends on OF_GPIO
+       select GPIOLIB_IRQCHIP
+       help
+         GPIO driver for STMicroelectronics Multi-Function eXpander.
+
+         This provides GPIO interface supporting inputs and outputs.

quoted

+/*
+ * STMicroelectronics Multi-Function eXpander (ST-MFX) - GPIO expander driver.
+ *
+ * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
+ * Author(s): Amelie Delaunay [off-list ref] for STMicroelectronics.

quoted

+ * License terms: GPL V2.0.
+ *
+ * st-mfx-gpio is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * st-mfx-gpio is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * st-mfx-gpio. If not, see <http://www.gnu.org/licenses/>.

Use SPDX instead.

OK.

quoted

+ */

quoted

+#include <linux/of_gpio.h>

quoted

+/* MFX has a maximum of 24 gpios, with 8 gpios per bank, so 3 banks maximum */
+#define NR_MAX_GPIOS           24
+#define NR_GPIOS_PER_BANK      8
+#define NR_MAX_BANKS           (NR_MAX_GPIOS / NR_GPIOS_PER_BANK)

quoted

+#define get_bank(offset)       ((u8)((offset) / NR_GPIOS_PER_BANK))
+#define get_mask(offset)       ((u8)BIT((offset) % NR_GPIOS_PER_BANK))

I would rather keep it consistent, i.e.
get_bank() [as is]
get_mask -> get_shift() [w/o BIT() macro]

#define get_shift(offset)       ((u8)((offset) % NR_GPIOS_PER_BANK))
u8 mask = (u8)BIT(get_shift(offset));
I'm not sure consistency is more important than readability and 
convenience ?

quoted

+enum { REG_IRQ_SRC, REG_IRQ_EVT, REG_IRQ_TYPE, NR_CACHE_IRQ_REGS };

Please, do one item per line.

OK.

quoted

+/*
+ * This is MFX-specific flags, overloading Linux-specific of_gpio_flags,
+ * needed in of_xlate callback.

quoted

+ * on MFX: - if GPIO is output:

Split to two lines.

OK.

quoted

+ *             * (0) means PUSH_PULL
+ *             * OF_GPIO_SINGLE_ENDED (=2) means OPEN-DRAIN
+ *        - if GPIO is input:
+ *             * (0) means NO_PULL
+ *             * OF_MFX_GPI_PUSH_PULL (=2) means PUSH_PULL
+ *
+ *             * OF_MFX_GPIO_PULL_UP programs pull up resistor on the GPIO,
+ *               whatever its direction, without this flag, depending on
+ *               GPIO type and direction, it programs either no pull or
+ *               pull down resistor.
+ */
+enum of_mfx_gpio_flags {
+       OF_MFX_GPI_PUSH_PULL = 0x2,
+       OF_MFX_GPIO_PULL_UP = 0x4,

These have misleading prefix. OF_ is reserved for general OF stuff.

You're right. I will fix it in V2.

quoted

+};
+
+struct mfx_gpio {
+       struct gpio_chip gc;
+       struct mfx *mfx;

quoted

+       struct device *dev;

Don't you have it in gc above as a parent device?

Yes, I agree to remove it.

quoted

+       struct mutex irq_lock; /* IRQ bus lock */
+       u32 flags[NR_MAX_GPIOS];
+       /* Caches of interrupt control registers for bus_lock */
+       u8 regs[NR_CACHE_IRQ_REGS][NR_MAX_BANKS];
+       u8 oldregs[NR_CACHE_IRQ_REGS][NR_MAX_BANKS];
+};

quoted

+static int mfx_gpio_direction_input(struct gpio_chip *gc,
+                                   unsigned int offset)
+{
+       struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
+       struct mfx *mfx = mfx_gpio->mfx;
+       u8 reg_type = MFX_REG_GPIO_TYPE + get_bank(offset);
+       u8 reg_pupd = MFX_REG_GPIO_PUPD + get_bank(offset);
+       u8 reg_dir = MFX_REG_GPIO_DIR + get_bank(offset);
+       u8 mask = get_mask(offset);
+       int ret;
+
+       /*
+        * In case of input direction, there is actually no way to apply some
+        * configuration in hardware, as it can be done with
+        * .set_config in case of output direction. That's why we apply
+        * here the MFX specific-flags.
+        */
+       if (mfx_gpio->flags[offset] & OF_MFX_GPI_PUSH_PULL)
+               ret = mfx_set_bits(mfx, reg_type, mask, mask);
+       else
+               ret = mfx_set_bits(mfx, reg_type, mask, 0);

quoted

+

Redundant empty line.

OK.

quoted

+       if (ret)
+               return ret;
+
+       if (mfx_gpio->flags[offset] & OF_MFX_GPIO_PULL_UP)
+               ret = mfx_set_bits(mfx, reg_pupd, mask, mask);
+       else
+               ret = mfx_set_bits(mfx, reg_pupd, mask, 0);

quoted

+

Ditto.

OK.

quoted

+       if (ret)
+               return ret;
+
+       return mfx_set_bits(mfx, reg_dir, mask, 0);
+}

quoted

+static void mfx_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
+{
+       struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
+       struct mfx *mfx = mfx_gpio->mfx;
+       u16 i;
+
+       for (i = 0; i < gc->ngpio; i++) {
+               int gpio = i + gc->base;
+               const char *label = gpiochip_is_requested(gc, i);
+               int dir = mfx_gpio_get_direction(gc, i);
+               int val = mfx_gpio_get(gc, i);
+               u8 mask = get_mask(i);
+               u8 reg;
+               int type, pupd;
+               int irqsrc, irqevt, irqtype, irqpending;

quoted

+               if (!label)
+                       label = "Unrequested";

I would rather put label = gpiochip_...(); immediately before this
condition for better readability.

You're right.

quoted

+
+               seq_printf(s, " gpio-%-3d (%-20.20s) ", gpio, label);
+

quoted

+               reg = MFX_REG_IRQ_GPI_PENDING + get_bank(i);
+               irqpending = mfx_reg_read(mfx, reg);
+               if (irqpending < 0)
+                       return;
+               irqpending = !!(irqpending & mask);
+

quoted

+               if (!dir) {

Why not to use

if (dir) {

?

My brain orders things in ascending order, I think it is the reason why 
this code manages false cases (=0) before the others.

quoted

+                       seq_printf(s, "out %s ", val ? "high" : "low");
+                       if (type)
+                               seq_puts(s, "open drain ");
+                       else
+                               seq_puts(s, "push pull ");
+                       if (pupd && type)
+                               seq_puts(s, "with internal pull-up ");
+                       else
+                               seq_puts(s, "without internal pull-up ");
+               } else {
+                       seq_printf(s, "in %s ", val ? "high" : "low");
+                       if (type)
+                               seq_printf(s, "with internal pull-%s ",
+                                          pupd ? "up" : "down");
+                       else
+                               seq_printf(s, "%s ",
+                                          pupd ? "floating" : "analog");
+               }
+
+               if (irqsrc) {
+                       seq_printf(s, "IRQ generated on %s %s",
+                                  !irqevt ?
+                                  (!irqtype ? "Low level" : "High level") :
+                                  (!irqtype ? "Falling edge" : "Rising edge"),
+                                  irqpending ? "(pending)" : "");

Why do you use negative conditions? Use positive ones.

OK, I will make the effort in V2.

quoted

+               }
+
+               seq_puts(s, "\n");
+       }
+}

quoted

+static void mfx_gpio_irq_toggle_trigger(struct gpio_chip *gc, int offset)
+{
+       struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
+       struct mfx *mfx = mfx_gpio->mfx;
+       u8 bank = get_bank(offset);
+       u8 mask = get_mask(offset);
+       int val = mfx_gpio_get(gc, offset);
+

quoted

+       if (val < 0) {
+               dev_err(mfx_gpio->dev, "can't get value of gpio%d: ret=%d\n",
+                       offset, val);

Is it somehow useful on err level?

I can drop it.

quoted

+               return;
+       }

quoted

+}

quoted

+static int mfx_gpio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+       struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
+       int bank = get_bank(d->hwirq);
+       int mask = get_mask(d->hwirq);
+

quoted

+       if ((type & IRQ_TYPE_LEVEL_LOW) || (type & IRQ_TYPE_LEVEL_HIGH))

IRQ_TYPE_LEVEL_MASK?

You're right.

quoted

+               mfx_gpio->regs[REG_IRQ_EVT][bank] &= ~mask;
+       else
+               mfx_gpio->regs[REG_IRQ_EVT][bank] |= mask;
+
+       if ((type & IRQ_TYPE_EDGE_RISING) || (type & IRQ_TYPE_LEVEL_HIGH))

IRQ_TYPE_EDGE_BOTH ?

No, here I test EDGE_RISING and LEVEL_HIGH, so mixing EDGE and LEVEL type.

quoted

+               mfx_gpio->regs[REG_IRQ_TYPE][bank] |= mask;
+       else
+               mfx_gpio->regs[REG_IRQ_TYPE][bank] &= ~mask;
+

quoted

+}

quoted

+static void mfx_gpio_irq_lock(struct irq_data *d)

Missed annotation that this acquires a lock.

It is .irq_bus_lock and .irq_bus_sync_unlock. Description is available 
in include/linux/irq.h:
  * @irq_bus_lock:	function to lock access to slow bus (i2c) chips
  * @irq_bus_sync_unlock:function to sync and unlock slow bus (i2c) chips

quoted

+static void mfx_gpio_irq_sync_unlock(struct irq_data *d)

Ditto for releasing lock.

quoted

+static irqreturn_t mfx_gpio_irq(int irq, void *dev)
+{
+       struct mfx_gpio *mfx_gpio = dev;
+       struct mfx *mfx = mfx_gpio->mfx;
+       int num_banks = mfx->num_gpio / 8;
+       u8 status[num_banks];

quoted

+       int ret;
+       u8 bank;

Swap lines.

OK.

quoted

+
+       ret = mfx_block_read(mfx, MFX_REG_IRQ_GPI_PENDING, ARRAY_SIZE(status),
+                            status);
+       if (ret < 0) {

quoted

+               dev_err(mfx_gpio->dev, "can't get IRQ_GPI_PENDING: %d\n", ret);

In IRQ context on err level? Hmm...

OK I will remove it in V2.

quoted

+               return IRQ_NONE;
+       }
+
+       for (bank = 0; bank < ARRAY_SIZE(status); bank++) {
+               u8 stat = status[bank];
+
+               if (!stat)
+                       continue;
+

quoted

+               while (stat) {
+                       int bit = __ffs(stat);

for_each_set_bit() ?

You're right.

quoted

+                       int offset = bank * NR_GPIOS_PER_BANK + bit;
+                       int gpio_irq = irq_find_mapping(mfx_gpio->gc.irq.domain,
+                                                       offset);
+                       int irq_trigger = irq_get_trigger_type(gpio_irq);
+
+                       handle_nested_irq(gpio_irq);
+                       stat &= ~(BIT(bit));
+
+                       if (irq_trigger & IRQ_TYPE_EDGE_BOTH)
+                               mfx_gpio_irq_toggle_trigger(&mfx_gpio->gc,
+                                                           offset);
+               }
+
+               mfx_reg_write(mfx, MFX_REG_IRQ_GPI_ACK + bank, status[bank]);
+       }
+
+       return IRQ_HANDLED;
+}

Thanks for review,
Amelie