From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Wed, 13 May 2015 22:01:31 +0200
Subject: [U-Boot] [PATCH 2/6] dm: gpio: hi6220: Add a hi6220 GPIO driver
	model driver.
In-Reply-To: <20150513120013.GA29198@griffinp-ThinkPad-X1-Carbon-2nd>
References: <1431437912-18988-1-git-send-email-peter.griffin@linaro.org>
 <201505122036.30435.marex@denx.de>
 <20150513120013.GA29198@griffinp-ThinkPad-X1-Carbon-2nd>
Message-ID: <201505132201.31679.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Wednesday, May 13, 2015 at 02:00:13 PM, Peter Griffin wrote:
> Hi Marek,
> 
> Thanks for reviewing.

Hi!

> > > +#ifndef _HI6220_GPIO_H_
> > > +#define _HI6220_GPIO_H_
> > > +
> > > +#define HI6220_GPIO0_BASE	(void *)0xf8011000
> > 
> > You should drop the explicit cast, that's nasty.
> 
> If I drop the cast I get lots of
> 
> include/asm/arch/gpio.h:11:32: warning: initialization makes pointer from
> integer without a cast
> 
> compiler warnings.

But this is gpio.h header for your architecture, so maybe fix it there too?

> > Also, why don't you define this as a HI6220_GPIO_BASE(bank) instead?
> > That'd trim down the number of macros and from what I see, it should
> > be rather easy to do ...
> > 
> > #define HI6220_GPIO_BASE(bank)
> > 
> > 	(((bank < 4) ? 0xf8012000 : (0xf7020000 - 0x4000)) + (0x1000 * bank))
> 
> Yes good idea, I will do it like you suggest in V2. Currently I have
> 
> #define HI6220_GPIO_BASE(bank)	(void *)(((bank < 4) ? 0xf8011000 : \
> 				0xf7020000 - 0x4000) + (0x1000 * bank))
> 
> To avoid the warnings mentioned above.

Yup, or maybe even make it an inline function so you get proper typechecking?

> <snip>
> 
> > > +#define HI6220_GPIO17_BASE	(void *)0xf702d000
> > > +#define HI6220_GPIO18_BASE	(void *)0xf702e000
> > > +#define HI6220_GPIO19_BASE	(void *)0xf702f000
> > 
> > But are these even used in the driver anywhere ?
> 
> They are currently used in the hikey.c board file which defines the
> hikey_gpio_platdata structure.
> 
> Although thinking about it some more this should be moved from the hikey
> board file to the driver as it is SoC specific rather than board specific.

This is specific to the CPU, so this should be in arch-<something> .

> > > +#define BIT(x)			(1 << (x))
> > 
> > This macro should be placed into common header files.
> > 
> > > +#define HI6220_GPIO_PER_BANK	8
> > > +#define HI6220_GPIO_DIR		0x400
> > > +
> > > +struct gpio_bank {
> > > +	u8 *base;	/* address of registers in physical memory */
> > 
> > Should be void __iomem *, no ?
> 
> Will fix in v2.

Thanks! Also, please wait a bit for other reviews before sending V2 :)