[PATCH 3/6] gpio: Add GPIO support for the ST Multi-Function eXpander
Andy Shevchenko
andy.shevchenko at gmail.com
Wed Feb 14 07:30:22 PST 2018
On Thu, Feb 8, 2018 at 4:27 PM, Amelie Delaunay <amelie.delaunay at st.com> wrote:
> 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.
> +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.
> +/*
> + * STMicroelectronics Multi-Function eXpander (ST-MFX) - GPIO expander driver.
> + *
> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> + * Author(s): Amelie Delaunay <amelie.delaunay at st.com> for STMicroelectronics.
> + * 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.
> + */
> +#include <linux/of_gpio.h>
> +/* 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)
> +#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]
> +enum { REG_IRQ_SRC, REG_IRQ_EVT, REG_IRQ_TYPE, NR_CACHE_IRQ_REGS };
Please, do one item per line.
> +/*
> + * This is MFX-specific flags, overloading Linux-specific of_gpio_flags,
> + * needed in of_xlate callback.
> + * on MFX: - if GPIO is output:
Split to two lines.
> + * * (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.
> +};
> +
> +struct mfx_gpio {
> + struct gpio_chip gc;
> + struct mfx *mfx;
> + struct device *dev;
Don't you have it in gc above as a parent device?
> + 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];
> +};
> +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);
> +
Redundant empty line.
> + 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);
> +
Ditto.
> + if (ret)
> + return ret;
> +
> + return mfx_set_bits(mfx, reg_dir, mask, 0);
> +}
> +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;
> + if (!label)
> + label = "Unrequested";
I would rather put label = gpiochip_...(); immediately before this
condition for better readability.
> +
> + seq_printf(s, " gpio-%-3d (%-20.20s) ", gpio, label);
> +
> + reg = MFX_REG_IRQ_GPI_PENDING + get_bank(i);
> + irqpending = mfx_reg_read(mfx, reg);
> + if (irqpending < 0)
> + return;
> + irqpending = !!(irqpending & mask);
> +
> + if (!dir) {
Why not to use
if (dir) {
?
> + 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.
> + }
> +
> + seq_puts(s, "\n");
> + }
> +}
> +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);
> +
> + 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?
> + return;
> + }
> +}
> +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);
> +
> + if ((type & IRQ_TYPE_LEVEL_LOW) || (type & IRQ_TYPE_LEVEL_HIGH))
IRQ_TYPE_LEVEL_MASK?
> + 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 ?
> + mfx_gpio->regs[REG_IRQ_TYPE][bank] |= mask;
> + else
> + mfx_gpio->regs[REG_IRQ_TYPE][bank] &= ~mask;
> +
> +}
> +static void mfx_gpio_irq_lock(struct irq_data *d)
Missed annotation that this acquires a lock.
> +static void mfx_gpio_irq_sync_unlock(struct irq_data *d)
Ditto for releasing lock.
> +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];
> + int ret;
> + u8 bank;
Swap lines.
> +
> + ret = mfx_block_read(mfx, MFX_REG_IRQ_GPI_PENDING, ARRAY_SIZE(status),
> + status);
> + if (ret < 0) {
> + dev_err(mfx_gpio->dev, "can't get IRQ_GPI_PENDING: %d\n", ret);
In IRQ context on err level? Hmm...
> + return IRQ_NONE;
> + }
> +
> + for (bank = 0; bank < ARRAY_SIZE(status); bank++) {
> + u8 stat = status[bank];
> +
> + if (!stat)
> + continue;
> +
> + while (stat) {
> + int bit = __ffs(stat);
for_each_set_bit() ?
> + 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;
> +}
--
With Best Regards,
Andy Shevchenko
More information about the linux-arm-kernel
mailing list