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

[NeilBrown]

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, &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 a lot,
NeilBrown
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/attachments/20180612/67845972/attachment-0001.asc>