On Mon, May 28, 2018 at 1:09 AM, NeilBrown <neil@xxxxxxxxxx> 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@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> > 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@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
