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

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

 



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

Attachment: signature.asc
Description: PGP signature

_______________________________________________
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