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
