Hi Igor, On 15 September 2014 12:32, Igor Grinberg <[email protected]> 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 <[email protected]> > > --- > > > > 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
_______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
