From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <sergio.paracuellos@gmail.com>
Date: Sun, 20 May 2018 16:20:41 +0200
From: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Subject: Re: [PATCH] staging: mt7621-gpio: retrieve correct pointers in
 interrupt related functions
Message-ID: <20180520142041.GA10759@camaron>
References: <87tvr31d19.fsf@notabene.neil.brown.name>
 <1526803562-8369-1-git-send-email-sergio.paracuellos@gmail.com>
 <87d0xq1wmz.fsf@notabene.neil.brown.name>
 <20180520101823.GA10002@foobar>
 <878t8e1sj7.fsf@notabene.neil.brown.name>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <878t8e1sj7.fsf@notabene.neil.brown.name>
List-ID: <driverdev-devel.linuxdriverproject.org>
To: NeilBrown <neil@brown.name>
Cc: gregkh@linuxfoundation.org, driverdev-devel@linuxdriverproject.org

On Sun, May 20, 2018 at 09:21:48PM +1000, NeilBrown wrote:
> 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

I see. Thanks for your clear explanation. I'll try to make this fixes 
also and send them in next series in order to make this driver out 
of staging.

Best regards,
    Sergio Paracuellos