[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Ajay Bhargav <ajay.bhargav@einfochips.com>]

----- "Prafulla Wadaskar" <prafulla@marvell.com> wrote:

> You should define all GPIO register in a single struct
> And point them with base address offsets
> 

> 
> I suggest you to add mvgpio.c instead of armada100_gpio.c
> This will be used in future by other Marvell SoCs.
> 
GPIO registers might vary for other Marvell SoCs? How to deal with
that?

> > +int gpio_request(int gp, const char *label)
> > +{
> > +	/*
> > +	 * Assumes corresponding MFP is configured peoperly
> > +	 * for use as GPIO
> > +	 */
> 
> NAK, you should check here, respective MFP is being configured as
> GPIO, if not you should return error
> 
I checked datasheet and for most GPIOs, AF1 is given as GPIO but for few
its not, so adding a glue logic to check for specific GPIOs wont be a good idea.
Thats the reason i thought its good to keep MFP out of this. Please give suggestions.

> 
> Consider now this is generic GPIO driver for marvell SOCs, define this
> macro in gpio.h
> 

ok

> > +
> > +	if (gp < ARMD_MAX_GPIO) {
> > +		switch (GPIO_TO_REG(gp)) {
> 
> Some code comments are welcomed here.
> 
> > +		case 0:
> > +			writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr0);
> > +			break;
> > +		case 1:
> > +			writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr1);
> > +			break;
> > +		case 2:
> > +			writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr2);
> > +			break;
> > +		case 3:
> > +			writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr3);
> > +			break;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +	return 0;
> > +}
> > +
> > +int gpio_direction_output(int gp, int value)
> > +{
> > +	struct armdgpio_registers *gpio_regs =
> > +		(struct armdgpio_registers *)ARMD1_GPIO_BASE;
> > +
> > +	if (gp < ARMD_MAX_GPIO) {
> > +		switch (GPIO_TO_REG(gp)) {
> > +		case 0:
> > +			writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr0);
> 
> Call gpio_set_value(gp, value) here which is defined below doing the
> same
> 
thanks for suggestion.

> > +			panic("Invalid GPIO pin %u\n", gp);
> > +		}
> > +	} else {
> > +		panic("Invalid GPIO pin %u\n", gp);
> 
> Can you eliminate one panic line here?
> 
> Regards..
> Prafulla . .
> 
will do that... there is no return value from this function so i thought
it wont be good to continue from this point incase designed direction is not set.

Regards,
Ajay Bhargav