Re: [PATCH v3 0/8] staging: mt7621-gpio: last cleanups

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

 



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, &pending, 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
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