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

Igor Grinberg
2014-09-15T18:32:15Z

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.