On Tue, Jun 12, 2018 at 12:01:38PM +1000, NeilBrown wrote: > On Mon, Jun 11 2018, Sergio Paracuellos wrote: > > > On Mon, Jun 11, 2018 at 06:33:44PM +1000, NeilBrown wrote: > >> On Mon, Jun 11 2018, Sergio Paracuellos wrote: > >> > >> > After submiting this driver to try to get mainlined and get > >> > out of staging some new cleanups seems to be necessary. > >> > According to this main of Linus Walleij: > >> > > >> > http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-June/121742.html > >> > > >> > this series tries to fix all of the issues in order to send > >> > v2 and give it a new try. Because I don't have to hardware to > >> > test my changes I send new cleanups first in staging to make > >> > easier to NeilBrown test it and get a feedback about them. > >> > > >> > Changes in v2: > >> > - Patch where GPIOLIB_IRQCHIP was used avoiding > >> > the use of a custom irq domain has been dropped to > >> > be sure after this changes all is working properly. > >> > (This was PATCH 7 in previous series) > >> > - PATCH 1: > >> > * avoid introducing new macros and use 'bank' > >> > field of mtk_gc with register offset. > >> > * Make correct use of bgpio_init passing new > >> > void __iomem pointers instead of use the > >> > macros. > >> > - Previous series PATCH 8 now is PATCH 7. Avoid the > >> > use of a switch-case statement which was wrong and > >> > distinc if we have RISSING AND FALLING EDGE interrupt > >> > or HIGH LOW level ones. This last two are exclusive and > >> > cannot be generated at the same time. > >> > > >> > Also, I think is we finally avoid to use a new irq_domain the > >> > need for the new functions introduced for request and release > >> > resources dissapears. I was diving down the other drivers code > >> > and I see that these two only are used in drivers which use its > >> > own irq_domain. Correct me if I am wrong, please. > >> > > >> > Hope this helps. > >> > > >> > Thanks in advance. > >> > >> Thanks a lot. > >> This series appears to work, though I sent a separate comment on one > >> piece of code. > > > > Thanks for testing, review and feedback. > > > > I have resent a complete v3 of this series taking into account > > your comments there. > > > >> However the gpio are numbers > >> 480-511 > >> 448-479 > >> 416-447 > >> > >> instead of > >> 0-31 > >> 32-63 > >> 64-95 > >> > >> which would be more normal. > >> Maybe when you resubmit I'll raid it with Linus Walleij and see if he > >> can explain why I can't have 0-95. > > > > This change is because the chip.base property changed to be -1 for > > dynamic enumeration of the gpio's. In this mail there is some explanation > > about it: > > > > http://lists.infradead.org/pipermail/linux-rpi-kernel/2014-October/001045.html > > Interesting - thanks for the link. > > It seems that the goal is to focus more on names than on numbers, and > that probably makes sense. > It means that we need to make sure that the names are useful. > > Currently the driver defines 3 gpiochips, one for each bank. > > # 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 > > Unfortunately they all have the same label :-( > It would be good if there was a way to add 0, 1, and 2 to the labels, or > something like that. There might be a way to set that labels from dts. I'll check other drivers code to change those. > > In /proc/interrupts we now have: > > 17: 0 0 0 0 MIPS GIC 19 mt7621, mt7621, mt7621 > > which is the interrupt from the GPIO controller. > It is a little weird that all three banks are named "mt7621" here. This is the name which is being set from devm_request_irq to mark line as shared. Maybe set mt7621-bank[0-2] is more usefull. > We also have: > > 26: 0 0 0 0 GPIO 18 reset This is the name from the one from the static initialization of the gpio_chip where callbacks are defined. Let's see what can I do to change that also. > > which is the interrupt from GPIO which provides the "reset" button. > I suspect that if I had interrupts form two different banks they would > both be called "GPIO" which would be a little confusing. > We could declare three different 'struct irq_chip' with three different > names, but that would be ugly. Hopefully there is a better way. > > Thanks, > NeilBrown Best regards, Sergio Paracuellos _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
