[U-Boot] [PATCH 08/10] dm: imx: gpio: Support driver model in MXC gpio driver

[Igor Grinberg <grinberg@compulab.co.il>]

Hi Simon,

On 09/15/14 15:57, Simon Glass wrote:
> Add driver model support with this driver. In this case the platform data
> is in the driver. It would be better to put this into an SOC-specific file,
> but this is best attempted when more boards are moved over to use driver
> model.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  drivers/gpio/mxc_gpio.c | 291 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 290 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
> index 6a572d5..8669cf0 100644
> --- a/drivers/gpio/mxc_gpio.c
> +++ b/drivers/gpio/mxc_gpio.c

[...]

> +static int check_reserved(struct udevice *dev, unsigned offset,
> +			  const char *func)

Wouldn't "check_requested" be a better name here?
And then also in the error message below?

> +{
> +	struct mxc_bank_info *bank = dev_get_priv(dev);
> +	struct gpio_dev_priv *uc_priv = dev->uclass_priv;
> +
> +	if (!*bank->label[offset]) {

Can we have here a more explicit (and descriptive) check for '\0'?

> +		printf("mxc_gpio: %s: error: gpio %s%d not reserved\n",
> +		       func, uc_priv->bank_name, offset);
> +		return -EPERM;
> +	}
> +
> +	return 0;
> +}

[...]

> +static int mxc_gpio_request(struct udevice *dev, unsigned offset,
> +			      const char *label)
> +{
> +	struct mxc_bank_info *bank = dev_get_priv(dev);
> +
> +	if (*bank->label[offset])

Same here?

> +		return -EBUSY;
> +
> +	strncpy(bank->label[offset], label, GPIO_NAME_SIZE);
> +	bank->label[offset][GPIO_NAME_SIZE - 1] = '\0';
> +
> +	return 0;
> +}

[...]

> +static int mxc_gpio_get_function(struct udevice *dev, unsigned offset)
> +{
> +	struct mxc_bank_info *bank = dev_get_priv(dev);
> +
> +	if (!*bank->label[offset])

and here.

> +		return GPIOF_UNUSED;
> +
> +	/* GPIOF_FUNC is not implemented yet */
> +	if (mxc_gpio_is_output(bank->regs, offset))
> +		return GPIOF_OUTPUT;
> +	else
> +		return GPIOF_INPUT;
> +}

[...]

> +static int mxc_gpio_probe(struct udevice *dev)
> +{
> +	struct mxc_bank_info *bank = dev_get_priv(dev);
> +	struct mxc_gpio_plat *plat = dev_get_platdata(dev);
> +	struct gpio_dev_priv *uc_priv = dev->uclass_priv;
> +	int banknum;
> +	char *name = "";

I think you can skip this initialization,
malloc will write into this pointer anyway.

> +
> +	bank->regs = plat->regs;
> +	uc_priv->gpio_count = 32;

Isn't this GPIO_PER_BANK is defined for?

> +	name = malloc(8);
> +	if (!name)
> +		return -ENOMEM;
> +	banknum = plat - mxc_plat;
> +	if (banknum < 98)
> +		sprintf(name, "GPIO%d_", banknum + 1);

The logic here (and the magic 98) is unclear.
Can we have a bit more explanation here please?

> +	uc_priv->bank_name = name;
> +
> +	return 0;
> +}

[...]


-- 
Regards,
Igor.