On Mon, May 28, 2018 at 1:09 AM, NeilBrown <[email protected]> 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/[email protected]/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 <[email protected]>
>
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
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
