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

[Simon Glass]

Hi Igor,

On 15 September 2014 12:32, Igor Grinberg <grinberg at compulab.co.il> wrote:

> 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 at 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?
>

The problem is that we then get into 'is_request' which is really not quite
as good a name as 'is_reserved'. Still, I'll change it to make things
consistent.

>
> > +{
> > +     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'?
>

I'll add a function.

>
> > +             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.
>

OK


>
> > +
> > +     bank->regs = plat->regs;
> > +     uc_priv->gpio_count = 32;
>
> Isn't this GPIO_PER_BANK is defined for?
>

OK

>
> > +     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?
>

It's icky, trying to avoid a string overflow. I'll rewrite it.


>
> > +     uc_priv->bank_name = name;
> > +
> > +     return 0;
> > +}
>
> [...]
>
>
> --
> Regards,
> Igor.
>

Regards,
Simon