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

[NeilBrown <neil@brown.name>]

[-- Attachment #1.1: Type: text/plain, Size: 1957 bytes --]

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>

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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel