On Thu, Jul 05, 2018 at 10:51:39AM +1000, NeilBrown wrote:
> On Sat, Jun 30 2018, Sergio Paracuellos wrote:
>
> > On Sat, Jun 30, 2018 at 7:47 AM, NeilBrown <neil@xxxxxxxxxx> wrote:
> >> On Mon, Jun 18 2018, Sergio Paracuellos wrote:
> >>
> >>> Banks shouldn't be defined in DT if number of resources
> >>> per bank is not variable. We actually know that this SoC
> >>> has three banks so take that into account in order to don't
> >>> overspecify the device tree. Device tree will only have one
> >>> node making it simple. Update device tree, binding doc and
> >>> code accordly.
> >>>
> >>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
> >>
> >> I'm sorry that I've been silent one these for a while - busy any all
> >> that.
> >>
> >> This last patch doesn't work. My test case works with all but this one
> >> applied, but this breaks it. I haven't had a chance to look into why
> >> yet. Sorry.
> >
> > Thanks for pointing this out. All of them were applied so I though
> > there were no problem at all with any of them.
> > We should revert the last one or wait to your feedback about what is
> > breaking it and send a new patch fixing the problem.
> >
>
> OK, I finally made time to dig into this.
> The problem is that the default gpio.of_xlate function assumes there is
> one gpio chip for each devicetree node. With this patch applied, the
> one device tree node corresponds to 3 different gpio chips. For that to
> work we need an xlate function.
> See below for what I wrote to get it working.
> With this in place:
> /sys/class/gpio still contains:
> export gpiochip416 gpiochip448 gpiochip480 unexport
>
> which is a little annoying, but unavoidable I guess.
> The labels on these are:
>
> # grep . /sys/class/gpio/gpiochip4*/label
> /sys/class/gpio/gpiochip416/label:1e000600.gpio
> /sys/class/gpio/gpiochip448/label:1e000600.gpio
> /sys/class/gpio/gpiochip480/label:1e000600.gpio
>
> .. all the same, which is not ideal.
>
> Your attempt to change the names doesn't work because bgpio_init() sets
> the names itself. If you move the assignment to rg->chip.label to
> *after* the call to bgpio_init(), the new names work:
>
> # grep . /sys/class/gpio/gpiochip4*/label
> /sys/class/gpio/gpiochip416/label:mt7621-bank2
> /sys/class/gpio/gpiochip448/label:mt7621-bank1
> /sys/class/gpio/gpiochip480/label:mt7621-bank0
>
> Also with that change /proc/interrupts contains:
>
> 17: 0 0 0 0 MIPS GIC 19 mt7621-bank0, mt7621-bank1, mt7621-bank2
>
> and
>
> 26: 0 0 0 0 mt7621-bank2 18 reset
>
> The first line looks good - though having "gpio" in the name might be
> good. Should they be 1e000600.gpio-bankN" ??
>
> The second is a bit weird, as this isn't bank2.
> It probably makes sense to have "1e000600.gpio 18 reset" there...
>
> There is only 1 irq chip, compared with 3 gpio chips, so a different
> name is appropriate. I changed the irq chip name:
>
> mediatek_gpio_irq_chip.name = dev_name(&pdev->dev);
>
> though maybe that assignment should go elsewhere - maybe in
> mediatek_gpio_probe() as it is only needed once.
>
>
> Thanks,
> NeilBrown
>
Thanks for your time in looking into this and the explanation about
what is happening there, Neil.
I will send a new patch series including all the stuff pointed out here.
Best regards,
Sergio Paracuellos
>
>
>
>
> diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> index 281e6214d543..814af9342d25 100644
> --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> @@ -205,6 +205,22 @@ static inline const char * const mediatek_gpio_bank_name(int bank)
> return bank_names[bank];
> }
>
> +static int mediatek_gpio_xlate(struct gpio_chip *chip,
> + const struct of_phandle_args *spec,
> + u32 *flags)
> +{
> + int gpio = spec->args[0];
> + struct mtk_gc *rq = container_of(chip, struct mtk_gc, chip);
> +
> + if (rq->bank != gpio / MTK_BANK_WIDTH)
> + return -EINVAL;
> +
> + if (flags)
> + *flags = spec->args[1];
> +
> + return gpio % MTK_BANK_WIDTH;
> +}
> +
> static int
> mediatek_gpio_bank_probe(struct platform_device *pdev,
> struct device_node *node, int bank)
> @@ -221,6 +237,8 @@ mediatek_gpio_bank_probe(struct platform_device *pdev,
> rg->chip.of_node = node;
> rg->bank = bank;
> rg->chip.label = mediatek_gpio_bank_name(rg->bank);
> + rg->chip.of_gpio_n_cells = 2;
> + rg->chip.of_xlate = mediatek_gpio_xlate;
>
> dat = gpio->gpio_membase + GPIO_REG_DATA + (rg->bank * GPIO_BANK_WIDE);
> set = gpio->gpio_membase + GPIO_REG_DSET + (rg->bank * GPIO_BANK_WIDE);
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
