On Sun, May 20 2018, Sergio Paracuellos wrote:

> On Sun, May 20, 2018 at 07:53:08PM +1000, NeilBrown wrote:
>> On Sun, May 20 2018, Sergio Paracuellos wrote:
>> 
>> > The data passed between irq related functions and the ones which have
>> > been retrieved where different. Also first data haven't properly
>> > set on irq_domain_add_linear call where it was passing NULL instead.
>> >
>> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>> 
>> Reviewed-by: NeilBrown <neil@brown.name>
>> Tested-by: NeilBrown <neil@brown.name>
>> 
>> :-)
>> 
>> Thanks,
>> NeilBrown
>
> Thanks, Neil. 
>
> So I'll send v5 of this series to make things easier. 
> I'll join this PATCH with PATCH 5 and make TODO file
> empty.
>
> What is missing to make this driver out of staging after this changes?

Uhmmm.. I depends on how fussy the gpio maintainer is.
I think the code is pretty good, but there are probably things that
could be improved.
Looking through the code again with my picky-maintainer spectacles on:

- we could probably use module_platform_driver() instead of
  subsys_initcall().
- The various
	u32 mask = BIT(d->hwirq);
  calls look wrong.  hwirq can be as large as 95, and
     1UL << 95
  is unlikely to work well.  I cannot test with a gpio above
  32, but I suspect it wouldn't work.
  These should use something like
      #define PIN_MASK(nr) (1UL << ((nr % MTK_BANK_WIDTH)))

- Locking is a bit weird.  It makes sense to hold the lock across a
  read-modify-write like in mediatek_gpio_direction_input(), but
  holding across a simple read (e.g. mediatec_gpio_get_direction())
  doesn't make much sense, and holding only for the 'write' part
  of a read-modify-write (e.g. mediateck_gpio_irq_umask()) is down
  right weird.

- And now for my pet peeve.
  There are 3 banks - right?  And they are numbered '0' and '1' and
  '2'. Agreed?
  So what is the Maximum bank number?  I think it is "2".
  "3" is the number of banks, or the count of banks.
  Yet we have
  #define MTK_MAX_BANK		3

  claiming that the maximum bank number is 3, which it isn't.
  Dan Carpenter recently guided you to fix

	if (!id || be32_to_cpu(*id) >= MTK_MAX_BANK)
		return -EINVAL;
  which was
	if (!id || be32_to_cpu(*id) > MTK_MAX_BANK)
		return -EINVAL;

  The latter looks right because if something is bigger than the maximum,
  it is obviously a problem, but if it equals the maximum, it should be
  fine.  So the code looked right, but was wrong.  You changed it so
  that it looks wrong, but is right.
  I would prefer to get both - it should look right and be right.
  I would suggest changing MTK_MAX_BANK to MTK_BANK_CNT - which
  means the right thing, and has a name structure similar to
  MTK_BANK_WIDTH.

Now looking at the devicetree binding description:

- #gpio-cells : Should be two.
  - first cell is the pin number
  - second cell is used to specify optional parameters (unused)

I think the second cell can be GPIO_ACTIVE_HIGH or GPIO_ACTIVE_LOW and I
think that is used.
Also I think you would typically put
     interrupt-controller;
     #interrupt-cells = <1>;
in the there so that other devices can reference the GPIO interrupts
if necessary.



Once all that has landed in staging and I've done one final test, I
think it will be ready to submit to linux-gpio and the device-tree list.

>
> Should it be moved or you were thinking in move all the mt7621
> together?

No, I can see no need to keep them together.


Thanks,
NeilBrown