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 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





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);

Attachment: signature.asc
Description: PGP signature

_______________________________________________
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