Re: [PATCH 0/7] staging: mt7621-gpio: cleanups to move this driver out of staging

[Sergio Paracuellos <sergio.paracuellos@gmail.com>]

On Mon, May 28, 2018 at 1:09 AM, NeilBrown <neil@brown.name> wrote:
> On Fri, May 25 2018, Sergio Paracuellos wrote:
>
>> This patch series review all the probably missing stuff
>> in order to get this driver out of staging..
>>
>> All of these are described in the following mail by NeilBrown:
>>
>> - https://www.mail-archive.com/driverdev-devel@linuxdriverproject.org/msg76202.html
>>
>> I don't move the driver yet because I think is better to
>> review and test this before and if all is likely to be
>> alright just move all this stuff afterwards.
>>
>> Hope this helps.
>
> It certainly does - thanks.
> All:
>   Reviewed-by: NeilBrown <neil@brown.name>
>

Thanks, Neil.

> except... I'm wondering about
>    #interrupt-cells = <1>;
>
> There really need to be 2 cells - one to identify the interrupt and one
> to request a trigger (rising,falling,high,low).
> I think I copied the <1> from a poor example.  Most gpio chips have
>    #interrupt-cells = <2>;

I was thinking in this for a while looking through other drivers code but
finally I ended up with your suggestion :-).

>
> Sorry about that.
>
> Otherwise they look good - I compiled and tested and it gpios still work :-)

Good news for me, I haven't broken anything :-).

>
> I went through the code again -- there is always something else.  These
> a really trivial though:
>
> -------------
> struct mtk_data {
>         void __iomem *gpio_membase;
>         int gpio_irq;
>         struct irq_domain *gpio_irq_domain;
>         struct mtk_gc *gc_map[MTK_BANK_CNT];
> };
>
> I don't think there is any gain in having gc_map be pointers.  We may
> as well just allocate all 3 at once.
> so
> -       struct mtk_gc *gc_map[MTK_BANK_CNT];
> +       struct mtk_gc gc_map[MTK_BANK_CNT];
>
> and several consequent changes in the code.
>
> -------------
> static inline struct mtk_gc
> *to_mediatek_gpio(struct gpio_chip *chip)
> {
>         struct mtk_gc *mgc;
>
>         mgc = container_of(chip, struct mtk_gc, chip);
>
>         return mgc;
> }
>
> The '*' should be at the end of the first line, not the start of the
> second.  Also the body of the function can
>
>         return container_of(chip, struct mtk_gc, chip);
>
> ----------
>         return (t & BIT(offset)) ? 0 : 1;
>
> I think this would read better as
>
>         return (t & BIT(offset)) ? GPIOF_DIR_OUT : GPIOF_DIR_IN;
>
>
> Everything else looks perfect.
> Thanks,
> NeilBrown
>

Let's apply these patches first as they are if they are ok and I will send last
changes written here in a new series during this week. Hopefully
tonight if nothing happens.

Best regards,
    Sergio Paracuellos
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel