Re: [PATCH v5 18/18] staging: mt7621-gpio: avoid use banks in device tree

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux