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 <n...@brown.name>
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
signature.asc
Description: PGP signature
_______________________________________________ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
