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 <s...@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
