Re: [PATCH 2/5] BCM2835: add gpio driver

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Carlo,

I had an additional look before applying it, but there are still
some things that deserve a cleanup.

On Tue, Oct 16, 2012 at 08:04:42PM +0200, Carlo Caione wrote:
> +#include <common.h>
> +#include <errno.h>
> +#include <malloc.h>
> +#include <io.h>
> +#include <gpio.h>
> +#include <init.h>
> +#include <mach/platform.h>

Is this needed? I think not as this file only defines some base
addresses.

> +static int bcm2835_gpio_probe(struct device_d *dev)
> +{
> +	struct bcm2835_gpio_chip *bcmgpio;
> +	int ret;
> +
> +	bcmgpio = xzalloc(sizeof(*bcmgpio));
> +	bcmgpio->base = dev_request_mem_region(dev, 0);
> +	bcmgpio->chip.ops = &bcm2835_gpio_ops;
> +	bcmgpio->chip.base = -1;

Better to use 0 for base of the SoC internal gpios (or something like
dev->id * 32) to get a known base for the gpios. Board code will likely
depend on fixed numbers.

> +
> +static __maybe_unused struct of_device_id bcm2835_gpio_dt_ids[] = {
> +	{
> +			.compatible = "brcm,bcm2835-gpio",

Too many whitespaces here.

Sascha

> +static int bcm2835_gpio_add(void)
> +{
> +	platform_driver_register(&bcm2835_gpio_driver);
> +	return 0;

return platform_device_register() please. We had a time when barebox
just paniced when an initcall failed, hence we never returned
unsuccesful. Now we just print an error message, so when the register
for some reason fails you will see it.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox