Date:	Wed, 4 Feb 2015 10:10:02 +0800 (CST)
From:	"Chao Xie" <xiechao_mail@....com>
To:	"Linus Walleij" <linus.walleij@...aro.org>
Cc:	"Chao Xie" <chao.xie@...vell.com>,
	"Alexandre Courbot" <gnurou@...il.com>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Haojian Zhuang" <haojian.zhuang@...il.com>
Subject: Re:Re: [PATCH] gpio: mmp: add GPIO driver for Marvell MMP series


At 2015-02-03 21:21:43, "Linus Walleij" <linus.walleij@...aro.org> wrote:
>On Wed, Jan 28, 2015 at 3:30 AM, Chao Xie <chao.xie@...vell.com> wrote:
>
>> From: Chao Xie <chao.xie@...vell.com>
>>
>> For some old PXA series, they used PXA GPIO driver.
>> The IP of GPIO changes since PXA988 which is Marvell MMP
>> series.
>> It will use new way to control the GPIO level, direction
>> and edge status.
>>
>> Signed-off-by: Chao Xie <chao.xie@...vell.com>
>
>(...)
>
>> +config GPIO_MMP
>> +       bool "MMP GPIO support"
>> +       depends on ARCH_MMP
>
>All new simple drivers with IRQ should
>
>select GPIOLIB_IRQCHIP
>

Thanks for review.

I will check GPIOLIB_IRQCHIP and make use of it.

>Since this looks like a basic MMIO driver I think
>you should also use:
>
>select GPIO_GENERIC
>

I think the gpio-mmp is not same as gpio-generic.
gpio-mmp need control the level/direction/rising edge detect enable/falling edge detect enable.
For each of them, there are three registers: status register,  setting register and clear register.
For example, for direction, if you configure it as output.
You need SET the bit in setting register.
If you want to configure it as input
You need SET the bit in clear register.
The bits will be cleared by hardware if the operation is completed by hardware.

It is same for level/rising edege detect enable/falling edge detect enable.

If you want to read the status of the pin. For example, the current level of the pin.
You CAN NOT read the setting/clear register. You need read the level status register.

>And set up simple getter/setter functions with a
>
>
>> +       help
>> +         Say yes here to support the MMP GPIO device at PXA1088/PXA1908/PXA1928.
>> +         Comparing with PXA GPIO device, the IP of MMP GPIO changes a lot.
>> +
>
>(...)
>
>> +++ b/drivers/gpio/gpio-mmp.c
>
>> +#include <linux/err.h>
>> +#include <linux/init.h>
>> +#include <linux/irq.h>
>
>You don't need this include with GPIOLIB_IRQCHIP
>

I will fix it.

>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/gpio.h>
>
>#include <linux/gpio/driver.h>
>
>> +#include <linux/clk.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/module.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/irqchip/chained_irq.h>
>
>get rid of these two includes in favor of using GPIOLIB_IRQCHIP
>

I will fix it.

>> +#include <linux/platform_data/gpio-mmp.h>
>
>Add:
>#include <linux/basic_mmio_gpio.h>
>
>And implement generic GPIO using bgpio_init()
>
>(...)

The reasons are listed above

>> +#define GPLR           0x0
>> +#define GPDR           0xc
>> +#define GPSR           0x18
>> +#define GPCR           0x24
>> +#define GRER           0x30
>> +#define GFER           0x3c
>> +#define GEDR           0x48
>> +#define GSDR           0x54
>> +#define GCDR           0x60
>> +#define GSRER          0x6c
>> +#define GCRER          0x78
>> +#define GSFER          0x84
>> +#define GCFER          0x90
>> +#define GAPMASK                0x9c
>> +#define GCPMASK                0xa8
>> +
>> +/* Bank will have 2^n GPIOes, and for mmp-gpio n = 5 */
>> +#define BANK_GPIO_ORDER                5
>> +#define BANK_GPIO_NUMBER       (1 << BANK_GPIO_ORDER)
>> +#define BANK_GPIO_MASK         (BANK_GPIO_NUMBER - 1)
>> +
>> +#define mmp_gpio_to_bank_idx(gpio)     ((gpio) >> BANK_GPIO_ORDER)
>> +#define mmp_gpio_to_bank_offset(gpio)  ((gpio) & BANK_GPIO_MASK)
>> +#define mmp_bank_to_gpio(idx, offset)  (((idx) << BANK_GPIO_ORDER)     \
>> +                                               | ((offset) & BANK_GPIO_MASK))
>> +
>
>This looks convoluted. Why not just register each bank as a separate
>device instead of trying to figure out bank index like this?
>

There are the following reasons
1. There is only one IRQ for the whole GPIO, even there are 3 or more banks.
2. The registers are not formatted into group. They are interleaved.
    For example, there are three banks, So for the registers order are
    LEVEL_STATUS_BANK0, LEVEL_STASTUS_BANK1, LEVEL_STATUS_BANK2
    DIRECTION_STATUS_BANK0, DIRECTION_STATUS_BANK1, DIRECTION_STATUS_BANK2
3. each bank has 32 bits. Formatting them into one driver will make other driver easier.
    For example, the MMC driver has GPIO detection for card. So it knows the GPIO is GPIO56.
    In the device tree of MMC driver, you can simple add as
    cd-gpios = <&gpio 56 X>
    if you format them into different devices, the mmc driver owner need to know how much bits a bank is, and calculate out correct GPIOx and offset
    cd-gpios = <&gpio1 24 X>

>> +struct mmp_gpio_bank {
>> +       void __iomem *reg_bank;
>> +       u32 irq_mask;
>> +       u32 irq_rising_edge;
>> +       u32 irq_falling_edge;
>> +};
>> +
>> +struct mmp_gpio_chip {
>> +       struct gpio_chip chip;
>
>That should then be
>struct bgpio_chip       bgc;
>
>For generic GPIO.
>
>> +       void __iomem *reg_base;
>> +       int irq;
>> +       struct irq_domain *domain;
>
>These two will not be necessary to keep around with
>GPIOLIB_IRQCHIP
>
>> +       unsigned int ngpio;
>
>This is part of struct gpio_chip so do not duplicate it.
>
>> +       unsigned int nbank;
>> +       struct mmp_gpio_bank *banks;
>
>And those two I think you should get rid of by creating one
>chip per bank.
>
>> +};
>
>So merge these two into one struct and instantiate one device for
>each bank.
>

The reasons of why can not separate the banks are described above.

>> +static int mmp_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>> +{
>> +       struct mmp_gpio_chip *mmp_chip =
>> +                       container_of(chip, struct mmp_gpio_chip, chip);
>> +
>> +       return irq_create_mapping(mmp_chip->domain, offset);
>> +}
>
>This function goes away with GPIOLIB_IRQCHIP.
>Just leave it unassigned and let the core handle this translation.
>

I will check GPIOLIB_IRQCHIP.

>> +static int mmp_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
>> +{
>> +       struct mmp_gpio_chip *mmp_chip =
>> +                       container_of(chip, struct mmp_gpio_chip, chip);
>
>Create a static inline to cast the gpio_chip to a mmp_chip like
>this:
>
>static inline struct mmp_gpio_chip *to_mmp(struct gpio_chip *gc)
>{
>    return container_of(chip, struct mmp_gpio_chip, chip);
>}
>
>Use that everywhere to simplify.
>
>> +       struct mmp_gpio_bank *bank =
>> +                       &mmp_chip->banks[mmp_gpio_to_bank_idx(offset)];
>> +       u32 bit = (1 << mmp_gpio_to_bank_offset(offset));
>
>And get rid of this by using one device per bank.
>
>> +static int mmp_gpio_direction_output(struct gpio_chip *chip,
>> +static int mmp_gpio_get(struct gpio_chip *chip, unsigned offset)
>> +static void mmp_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>
>Looks like generic GPIO will do the job.
>
>> +#ifdef CONFIG_OF_GPIO
>> +static int mmp_gpio_of_xlate(struct gpio_chip *chip,
>> +                            const struct of_phandle_args *gpiospec,
>> +                            u32 *flags)
>> +{
>> +       struct mmp_gpio_chip *mmp_chip =
>> +                       container_of(chip, struct mmp_gpio_chip, chip);
>> +
>> +       /* GPIO index start from 0. */
>> +       if (gpiospec->args[0] >= mmp_chip->ngpio)
>> +               return -EINVAL;
>> +
>> +       if (flags)
>> +               *flags = gpiospec->args[1];
>> +
>> +       return gpiospec->args[0];
>> +}
>> +#endif
>
>This also goes to the generic xlate with one device per bank.
>

Yes, it is done by GPIOLIB_IRQCHIP.

>> +static int mmp_gpio_irq_type(struct irq_data *d, unsigned int type)
>> +static void mmp_gpio_demux_handler(unsigned int irq, struct irq_desc *desc)
>> +static void mmp_ack_muxed_gpio(struct irq_data *d)
>> +static void mmp_mask_muxed_gpio(struct irq_data *d)
>> +static void mmp_unmask_muxed_gpio(struct irq_data *d)
>
>Looks OK but make sure to convert to GPIOLIB_IRQCHIP and convert from
>struct gpio_chip * passed as irq_data *d to the internal chip type
>with the new to_mmp().
>
>(...)
>
>From here:
>
>> +static int mmp_irq_domain_map(struct irq_domain *d, unsigned int irq,
>> +                             irq_hw_number_t hw)
>> +{
>> +       irq_set_chip_and_handler(irq, &mmp_muxed_gpio_chip,
>> +                                handle_edge_irq);
>> +       irq_set_chip_data(irq, d->host_data);
>> +       set_irq_flags(irq, IRQ_TYPE_NONE);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct irq_domain_ops mmp_gpio_irq_domain_ops = {
>> +       .map    = mmp_irq_domain_map,
>> +       .xlate  = irq_domain_xlate_twocell,
>> +};
>
>
>To here goes away with GPIOLIB_IRQCHIP (moved to core).
>

I will check GPIOLIB_IRQCHIP.

>> +#ifdef CONFIG_OF_GPIO
>> +       mmp_chip->chip.of_node = np;
>> +       mmp_chip->chip.of_xlate = mmp_gpio_of_xlate;
>> +       mmp_chip->chip.of_gpio_n_cells = 2;
>> +#endif
>
>Can't we just select or depend on OF_GPIO for this
>driver and get rid of the #fidef:s?
>
Sure, we can depend on OF_GPIO.

>> +static int __init mmp_gpio_init(void)
>> +{
>> +       return platform_driver_register(&mmp_gpio_driver);
>> +}
>> +postcore_initcall(mmp_gpio_init);
>
>Why does this nees to be postcore? A normal module
>would be nice.
>

I want to make it as module, but some devices need control the GPIO, for example, PMIC will make use of GPIO to control voltage.
The voltage setting will be done before GPIO driver is initialized. So it need make sure that the GPIO APIs can work.

>Yours,
>Linus Walleij

Powered by blists - more mailing lists