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@xxxxxxxxxxxxxxxxxxxxxx/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@xxxxxxxxxx> 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>; Sorry about that. Otherwise they look good - I compiled and tested and it gpios still work :-) 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
