Re: [PATCH v3 0/8] staging: mt7621-gpio: last cleanups
On Tue, Jun 12, 2018 at 12:33:42PM +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 v3:
> > - PATCH 7: refactor irq_type to make better code.
> > - Add PATCH 8 avoiding the use of custom domain and requesting
> > manually a 'IRQF_SHARED'. It should be working now??
>
> Yes, it is working now - thanks.
> With this series, the driver again works for all the tests I can
> perform - except that some names aren't unique, as I've mentioned
> separately.
Awesome. Thanks for testing a review! Now that al is working it is
time for small cleanups.
>
> Looking over the new code:
>
> - I don't think we need PIN_MASK() any more. We needed that
> when we had 1 irq_chip which handled 96 irqs. Now we have
> 3 irq_chips with 32 irqs each.
>
> - documentation for 'struct mtk_data' says it is a single
> irqchip, but I don't think it is any more - there is one
> per gpio chip.
> Related: doco for 'struct mtk_gc' contains data for both
>the gpio_chip and the irq_chip. I don't know if that
>needs to be spelled out.
>
> - In
> if (pending) {
> for_each_set_bit(bit, , MTK_BANK_WIDTH) {
> I wouldn't bother with the "if (pending)".
> If pending is zero, then find_each_set_bit() won't find anything.
> It is at most a minor optimization.
> This is a personal preference and if you like it that way, leave it.
> Though if you are keen to optimize, then instead of calling
> mtk_gpio_w32(...BIT(bit)) for every found bit, just call
> mtk_gpio_w32(... pending) once at the top.
>
> - to_mediatek_gpio() cannot return NULL, so testing "if (!rg)" in
> several places is pointless.
>
> - If the dts file doesn't specify an irq, the irq_of_parse_and_map()
> will return -1 (I think). This might deserve a warning and probably
> shouldn't cause the probe to fail, but it should cause
> mediatek_gpio_bank_probe to avoid trying to set up interrupts.
>
>
> Nothing serious, but some might be worth fixing.
Thanks for pointing out these in this mail also.
Hopefully I'll resend a new cleanups series in reply to this
mail.
>
> Thanks a lot,
> NeilBrown
Best regards,
Sergio Paracuellos
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 0/8] staging: mt7621-gpio: last cleanups
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 v3:
> - PATCH 7: refactor irq_type to make better code.
> - Add PATCH 8 avoiding the use of custom domain and requesting
> manually a 'IRQF_SHARED'. It should be working now??
Yes, it is working now - thanks.
With this series, the driver again works for all the tests I can
perform - except that some names aren't unique, as I've mentioned
separately.
Looking over the new code:
- I don't think we need PIN_MASK() any more. We needed that
when we had 1 irq_chip which handled 96 irqs. Now we have
3 irq_chips with 32 irqs each.
- documentation for 'struct mtk_data' says it is a single
irqchip, but I don't think it is any more - there is one
per gpio chip.
Related: doco for 'struct mtk_gc' contains data for both
the gpio_chip and the irq_chip. I don't know if that
needs to be spelled out.
- In
if (pending) {
for_each_set_bit(bit, , MTK_BANK_WIDTH) {
I wouldn't bother with the "if (pending)".
If pending is zero, then find_each_set_bit() won't find anything.
It is at most a minor optimization.
This is a personal preference and if you like it that way, leave it.
Though if you are keen to optimize, then instead of calling
mtk_gpio_w32(...BIT(bit)) for every found bit, just call
mtk_gpio_w32(... pending) once at the top.
- to_mediatek_gpio() cannot return NULL, so testing "if (!rg)" in
several places is pointless.
- If the dts file doesn't specify an irq, the irq_of_parse_and_map()
will return -1 (I think). This might deserve a warning and probably
shouldn't cause the probe to fail, but it should cause
mediatek_gpio_bank_probe to avoid trying to set up interrupts.
Nothing serious, but some might be worth fixing.
Thanks a lot,
NeilBrown
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 0/8] staging: mt7621-gpio: last cleanups
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 v3: - PATCH 7: refactor irq_type to make better code. - Add PATCH 8 avoiding the use of custom domain and requesting manually a 'IRQF_SHARED'. It should be working now?? 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. Hope this helps. Thanks in advance. Best regards, Sergio Paracuellos Sergio Paracuellos (8): staging: mt7621-gpio: make use 'bgpio_init' from GPIO_GENERIC staging: mt7621-gpio: avoid including 'gpio.h' staging: mt7621-gpio: make use of 'builtin_platform_driver' staging: mt7621-gpio: implement '.irq_[request|release]_resources' functions staging: mt7621-gpio: add COMPILE_TEST staging: mt7621-gpio: add kerneldoc for state data containers staging: mt7621-gpio: implement high level and low level irqs staging: mt7621-gpio: avoid custom irq_domain for gpio drivers/staging/mt7621-gpio/Kconfig | 4 +- drivers/staging/mt7621-gpio/gpio-mt7621.c | 369 +- 2 files changed, 168 insertions(+), 205 deletions(-) -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
