Hi Igor,
On 15 September 2014 12:32, Igor Grinberg grinberg@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@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;elsereturn 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