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@xxxxxxxxx>
>>
>> Reviewed-by: NeilBrown <neil@xxxxxxxxxx>
>> Tested-by: NeilBrown <neil@xxxxxxxxxx>
>>
>> :-)
>>
>> 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
