[U-Boot] [PATCH v2 09/11] dm: imx: gpio: Support driver model in MXC gpio driver

[Simon Glass]

Hi Igor,

On 17 September 2014 07:00, Igor Grinberg <grinberg at compulab.co.il> wrote:

> On 09/17/14 06:51, 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>
> > ---
> >
> > Changes in v2:
> > - Change 'reserved' to 'requested'
> > - Add an internal function to check if a GPIO is requested
> > - Tidy up confusing code that creates names for gpio_request()
> >
> >  drivers/gpio/mxc_gpio.c | 302
> +++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 301 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
> > index 6a572d5..9435d2f 100644
> > --- a/drivers/gpio/mxc_gpio.c
> > +++ b/drivers/gpio/mxc_gpio.c
>
> [...]
>
> > +static int mxc_gpio_get_state(struct udevice *dev, unsigned int offset,
> > +                           char *buf, int bufsize)
> > +{
> > +     struct gpio_dev_priv *uc_priv = dev->uclass_priv;
> > +     struct mxc_bank_info *bank = dev_get_priv(dev);
> > +     const char *label;
> > +     bool is_output;
> > +     int size;
> > +
> > +     label = bank->label[offset];
> > +     is_output = mxc_gpio_is_output(bank->regs, offset);
> > +     size = snprintf(buf, bufsize, "%s%d: ",
> > +                     uc_priv->bank_name ? uc_priv->bank_name : "",
> offset);
> > +     buf += size;
> > +     bufsize -= size;
> > +     snprintf(buf, bufsize, "%s: %d [%c]%s%s",
> > +              is_output ? "out" : " in",
> > +              is_output ?
> > +                     mxc_gpio_bank_get_output_value(bank->regs, offset)
> :
> > +                     mxc_gpio_bank_get_value(bank->regs, offset),
> > +              *label ? 'x' : ' ',
> > +              *label ? " " : "",
>
> This is a check if the gpio is requested, right?
> I think the new function gpio_is_requested() can be of hand here
> instead of open coding this.
>

OK


>
> > +              label);
> > +
> > +     return 0;
> > +}
> > +
>
> [...]
>
> > +static int mxc_gpio_free(struct udevice *dev, unsigned offset)
> > +{
> > +     struct mxc_bank_info *bank = dev_get_priv(dev);
> > +     int ret;
> > +
> > +     ret = check_requested(dev, offset, __func__);
> > +     if (ret)
> > +             return ret;
>
> In case of not requested gpio,
> should we really print the error message and return -EPERM?
> Or may be adopt free() behavior and just return silently?
> Linux gpiolib gpio_free() uses WARN_ON(extra_checks) for
> "not requested" cases, so it shouts only in DEBUG cases.
>

I'm not sure - I intend to push this up to the DM layer at some point. I
feel that keeping track of GPIO requested/free should be a common feature
of the uclass rather than implemented in each driver. But until we have
enough drivers to make it clear that this is acceptable, I'm leaving it as
it is.

So for at least now I think this is correct. We might consider bringing in
some sort of WARN system to U-Boot. The caller can ignore the return value
anyway.

Regards,
Simon