Re:Re: [PATCH V2] gpio: mmp: add GPIO driver for Marvell MMP series

[Chao Xie]

At 2015-03-17 18:25:24, "Linus Walleij" <linus.walleij@xxxxxxxxxx> wrote:
>On Fri, Mar 6, 2015 at 3:04 AM, Chao Xie <chao.xie@xxxxxxxxxxx> wrote:
>
>> Signed-off-by: Chao Xie <chao.xie@xxxxxxxxxxx>
>
>First can some of the MMP people comment on this driver please?
>(Eric/Haojian)
>
>So this driver duplicates drivers/gpio/gpio-pxa.c in some sense but
>is nicer and cleaner and thus an incentive for me to merge it.
>At the same time I don't want two drivers for essentially the same
>hardware and in that way I would prefer to clean up the PXA driver.
>
>But I don't know how close the PXA and MMP drivers really are.
>
>Will it also cover MMP2 that is currently using by the PXA driver?
>
>Is it really complicated to migrate gpio-pxa to GPIOLIB_IRQCHIP?
>
>I guess you should also delete the compatible string and
>compatibility code in drivers/gpio/gpio-pxa.c (as a separate patch
>in this series) and merge this along with some defconfig
>changes that activates this by default for the MMP machines?

>


I will ask Haojian to comment at it when send out V3 patch.
pxa and mmp are two different series.
First pxa is Intel's product, and after Intel sell its xscale to marvell, the SOC series become mmp.
The SOC design has many differences.
now mmp gpio has some registers kept in order to use the old pxa driver, but silicon designer
warned us that it is better to use the new register interfaces.
The major differences are:
For all the edge detecting setting/clearing, level setting/clearing, direction setting/clearing, mmp has
separated registers

>> +config GPIO_MMP
>> +       bool "MMP GPIO support"
>> +       depends on ARCH_MMP
>> +       select GPIOLIB_IRQCHIP
>
>NICE!
>
>> +struct mmp_gpio_bank {
>> +       void __iomem *reg_bank;
>> +       u32 irq_mask;
>> +       u32 irq_rising_edge;
>> +       u32 irq_falling_edge;
>
>I can't see why you're keeping all these settings of the edges and mask
>in these variables. Why can't you just keep the state in the hardware
>registers?

>
When other driver request gpio, it will set whether it is rising edge detect or
falling edge detect or both.
We need record it.
So when user mask/unmask the interrupt, we know whether we need clear/set
rising edge detect/falling edge detect or both.

>> +};
>> +
>> +struct mmp_gpio_chip {
>> +       struct gpio_chip chip;
>> +       void __iomem *reg_base;
>> +       int irq;
>
>Do you need to keep irq around if you use devm_* to request the
>IRQ?

>


Yes, it is right. I will remove it.

>> +       unsigned int ngpio;
>
>This is already stored in struct gpio_chip, why
>store it a second time in this struct?
>
>> +       unsigned int nbank;
>> +       struct mmp_gpio_bank *banks;
>
>To repeat an eternal story: why do you have to register several
>banks under the same gpio_chip? I guess it's because they share
>a single IRQ line, but wanna make sure....

>
Yes. they share same interrupt line.

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
>> +};>> +
>> +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);
>> +       struct mmp_gpio_bank *bank =
>> +                       &mmp_chip->banks[mmp_gpio_to_bank_idx(offset)];
>
>Rewrite this:
>
>> +       u32 bit = (1 << mmp_gpio_to_bank_offset(offset));
>> +
>> +       writel(bit, bank->reg_bank + GCDR);
>
>Like this:
>
>#include <linux/bitops.h>
>
>writel(BIT(mmp_gpio_to_bank_offset(offset)), bank->reg_bank + GCDR);

>
I will do it.>
>> +static int mmp_gpio_direction_output(struct gpio_chip *chip,
>> +                                    unsigned offset, int value)
>> +{
>
>Use the same pattern with BIT() as described above.

>
I will do it.>> +static int mmp_gpio_get(struct gpio_chip *chip, unsigned offset)
>> +{
>> +
>> +       return !!(gplr & bit);
>
>Use the same pattern with BIT() as described above.

>


I will do it.
I will use BIT() for all below comments you suggested. Thanks.>> +static struct irq_chip mmp_muxed_gpio_chip = {
>> +       .name           = "mmp-gpio-irqchip",
>> +       .irq_mask       = mmp_mask_muxed_gpio,
>> +       .irq_unmask     = mmp_unmask_muxed_gpio,
>> +       .irq_set_type   = mmp_gpio_irq_type,
>> +       .flags          = IRQCHIP_SKIP_SET_WAKE,
>> +};
>> +
>> +static struct of_device_id mmp_gpio_dt_ids[] = {
>> +       { .compatible = "marvell,mmp-gpio"},
>> +       {}
>> +};
>
>So the same functionality in the other compatible driver should
>be deleted as a separate patch.

>


Yes. a separated patch is needed.

>> +static int mmp_gpio_probe_dt(struct platform_device *pdev,
>> +                               struct mmp_gpio_chip *mmp_chip)
>> +{
>> +       struct device_node *np = pdev->dev.of_node;
>> +       struct device_node *child;
>> +       u32 offset;
>> +       int i, nbank, ret;
>> +
>> +       if (!np)
>> +               return -EINVAL;
>> +
>> +       nbank = of_get_child_count(np);
>> +       if (nbank == 0)
>> +               return -EINVAL;
>> +
>> +       mmp_chip->banks = devm_kzalloc(&pdev->dev,
>> +                               sizeof(*mmp_chip->banks) * nbank,
>> +                               GFP_KERNEL);
>> +       if (!mmp_chip->banks)
>> +               return -ENOMEM;
>> +
>> +       i = 0;
>> +       for_each_child_of_node(np, child) {
>> +               ret = of_property_read_u32(child, "reg-offset", &offset);
>> +               if (ret)
>> +                       return ret;
>> +               mmp_chip->banks[i].reg_bank = mmp_chip->reg_base + offset;
>> +               i++;
>> +       }
>> +
>> +       mmp_chip->nbank = nbank;
>> +       mmp_chip->ngpio = mmp_chip->nbank * BANK_GPIO_NUMBER;
>> +
>> +       return 0;
>> +}
>
>Soon we havce so many drivers like this that we should abstract out a
>routine like this into gpiolib-of.c

>


I will try to add it to gpiolin-of.c

>> +
>> +       mmp_chip->irq = irq;
>
>Why keep this around?

>
I will delete it.

>And why are you not calling devm_request_irq()???

>
>> +       gpiochip_set_chained_irqchip(&mmp_chip->chip, &mmp_muxed_gpio_chip,>> +                                    mmp_chip->irq, mmp_gpio_demux_handler);
>

>mmp_chip->irq has not been requested AFAICT!


Do i need it? The handler is registered by gpiochip_set_chained_irqchip.
I reference to the sample drivers in drivers/gpio/, i find that the drivers using gpiochip
do not invoke {devm}_request_irq
>>> +
>> +       return 0;
>> +postcore_initcall(mmp_gpio_init);
>
>Why do you have to have this so early?
>
>I *strongly* prefer module_* initcalls at device_initcall()
>level maybe using deferred probe if need be. It is better
>if the init order is not relied upon to synchronize drivers.

>
I will use module_init for it.

>Yours,
>Linus Walleij