diff mbox

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

Message ID 1410785865-27946-9-git-send-email-sjg@chromium.org
State Superseded
Delegated to: Simon Glass
Headers show

Commit Message

Simon Glass Sept. 15, 2014, 12:57 p.m. UTC 
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(-)

Comments

Igor Grinberg Sept. 15, 2014, 6:32 p.m. UTC | #1 
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?

> +{
> +	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;
> +}

[...]
Simon Glass Sept. 17, 2014, 3:49 a.m. UTC | #2 
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;
> > +     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
diff mbox

Patch

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
@@ -8,16 +8,31 @@ 
  * SPDX-License-Identifier:	GPL-2.0+
  */
 #include <common.h>
+#include <errno.h>
+#include <dm.h>
+#include <malloc.h>
 #include <asm/arch/imx-regs.h>
 #include <asm/gpio.h>
 #include <asm/io.h>
-#include <errno.h>
 
 enum mxc_gpio_direction {
 	MXC_GPIO_DIRECTION_IN,
 	MXC_GPIO_DIRECTION_OUT,
 };
 
+#define GPIO_NAME_SIZE			20
+#define GPIO_PER_BANK			32
+
+struct mxc_gpio_plat {
+	struct gpio_regs *regs;
+};
+
+struct mxc_bank_info {
+	char label[GPIO_PER_BANK][GPIO_NAME_SIZE];
+	struct gpio_regs *regs;
+};
+
+#ifndef CONFIG_DM_GPIO
 #define GPIO_TO_PORT(n)		(n / 32)
 
 /* GPIO port description */
@@ -134,3 +149,277 @@  int gpio_direction_output(unsigned gpio, int value)
 
 	return mxc_gpio_direction(gpio, MXC_GPIO_DIRECTION_OUT);
 }
+#endif
+
+#ifdef CONFIG_DM_GPIO
+static int mxc_gpio_is_output(struct gpio_regs *regs, int offset)
+{
+	u32 val;
+
+	val = readl(&regs->gpio_dir);
+
+	return val & (1 << offset) ? 1 : 0;
+}
+
+static void mxc_gpio_bank_direction(struct gpio_regs *regs, int offset,
+				    enum mxc_gpio_direction direction)
+{
+	u32 l;
+
+	l = readl(&regs->gpio_dir);
+
+	switch (direction) {
+	case MXC_GPIO_DIRECTION_OUT:
+		l |= 1 << offset;
+		break;
+	case MXC_GPIO_DIRECTION_IN:
+		l &= ~(1 << offset);
+	}
+	writel(l, &regs->gpio_dir);
+}
+
+static void mxc_gpio_bank_set_value(struct gpio_regs *regs, int offset,
+				    int value)
+{
+	u32 l;
+
+	l = readl(&regs->gpio_dr);
+	if (value)
+		l |= 1 << offset;
+	else
+		l &= ~(1 << offset);
+	writel(l, &regs->gpio_dr);
+}
+
+static int mxc_gpio_bank_get_value(struct gpio_regs *regs, int offset)
+{
+	return (readl(&regs->gpio_psr) >> offset) & 0x01;
+}
+
+static int mxc_gpio_bank_get_output_value(struct gpio_regs *regs, int offset)
+{
+	return (readl(&regs->gpio_dr) >> offset) & 0x01;
+}
+
+static int check_reserved(struct udevice *dev, unsigned offset,
+			  const char *func)
+{
+	struct mxc_bank_info *bank = dev_get_priv(dev);
+	struct gpio_dev_priv *uc_priv = dev->uclass_priv;
+
+	if (!*bank->label[offset]) {
+		printf("mxc_gpio: %s: error: gpio %s%d not reserved\n",
+		       func, uc_priv->bank_name, offset);
+		return -EPERM;
+	}
+
+	return 0;
+}
+
+/* set GPIO pin 'gpio' as an input */
+static int mxc_gpio_direction_input(struct udevice *dev, unsigned offset)
+{
+	struct mxc_bank_info *bank = dev_get_priv(dev);
+	int ret;
+
+	ret = check_reserved(dev, offset, __func__);
+	if (ret)
+		return ret;
+
+	/* Configure GPIO direction as input. */
+	mxc_gpio_bank_direction(bank->regs, offset, MXC_GPIO_DIRECTION_IN);
+
+	return 0;
+}
+
+/* set GPIO pin 'gpio' as an output, with polarity 'value' */
+static int mxc_gpio_direction_output(struct udevice *dev, unsigned offset,
+				       int value)
+{
+	struct mxc_bank_info *bank = dev_get_priv(dev);
+	int ret;
+
+	ret = check_reserved(dev, offset, __func__);
+	if (ret)
+		return ret;
+
+	/* Configure GPIO output value. */
+	mxc_gpio_bank_set_value(bank->regs, offset, value);
+
+	/* Configure GPIO direction as output. */
+	mxc_gpio_bank_direction(bank->regs, offset, MXC_GPIO_DIRECTION_OUT);
+
+	return 0;
+}
+
+/* read GPIO IN value of pin 'gpio' */
+static int mxc_gpio_get_value(struct udevice *dev, unsigned offset)
+{
+	struct mxc_bank_info *bank = dev_get_priv(dev);
+	int ret;
+
+	ret = check_reserved(dev, offset, __func__);
+	if (ret)
+		return ret;
+
+	return mxc_gpio_bank_get_value(bank->regs, offset);
+}
+
+/* write GPIO OUT value to pin 'gpio' */
+static int mxc_gpio_set_value(struct udevice *dev, unsigned offset,
+				 int value)
+{
+	struct mxc_bank_info *bank = dev_get_priv(dev);
+	int ret;
+
+	ret = check_reserved(dev, offset, __func__);
+	if (ret)
+		return ret;
+
+	mxc_gpio_bank_set_value(bank->regs, offset, value);
+
+	return 0;
+}
+
+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 ? " " : "",
+		 label);
+
+	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])
+		return -EBUSY;
+
+	strncpy(bank->label[offset], label, GPIO_NAME_SIZE);
+	bank->label[offset][GPIO_NAME_SIZE - 1] = '\0';
+
+	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_reserved(dev, offset, __func__);
+	if (ret)
+		return ret;
+	bank->label[offset][0] = '\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])
+		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 const struct dm_gpio_ops gpio_mxc_ops = {
+	.request		= mxc_gpio_request,
+	.free			= mxc_gpio_free,
+	.direction_input	= mxc_gpio_direction_input,
+	.direction_output	= mxc_gpio_direction_output,
+	.get_value		= mxc_gpio_get_value,
+	.set_value		= mxc_gpio_set_value,
+	.get_function		= mxc_gpio_get_function,
+	.get_state		= mxc_gpio_get_state,
+};
+
+static const struct mxc_gpio_plat mxc_plat[] = {
+	{ (struct gpio_regs *)GPIO1_BASE_ADDR },
+	{ (struct gpio_regs *)GPIO2_BASE_ADDR },
+	{ (struct gpio_regs *)GPIO3_BASE_ADDR },
+#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
+		defined(CONFIG_MX53) || defined(CONFIG_MX6)
+	{ (struct gpio_regs *)GPIO4_BASE_ADDR },
+#endif
+#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
+	{ (struct gpio_regs *)GPIO5_BASE_ADDR },
+	{ (struct gpio_regs *)GPIO6_BASE_ADDR },
+#endif
+#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
+	{ (struct gpio_regs *)GPIO7_BASE_ADDR },
+#endif
+};
+
+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 = "";
+
+	bank->regs = plat->regs;
+	uc_priv->gpio_count = 32;
+	name = malloc(8);
+	if (!name)
+		return -ENOMEM;
+	banknum = plat - mxc_plat;
+	if (banknum < 98)
+		sprintf(name, "GPIO%d_", banknum + 1);
+	uc_priv->bank_name = name;
+
+	return 0;
+}
+
+U_BOOT_DRIVER(gpio_mxc) = {
+	.name	= "gpio_mxc",
+	.id	= UCLASS_GPIO,
+	.ops	= &gpio_mxc_ops,
+	.probe	= mxc_gpio_probe,
+	.priv_auto_alloc_size = sizeof(struct mxc_bank_info),
+};
+
+U_BOOT_DEVICES(mxc_gpios) = {
+	{ "gpio_mxc", &mxc_plat[0] },
+	{ "gpio_mxc", &mxc_plat[1] },
+	{ "gpio_mxc", &mxc_plat[2] },
+#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
+		defined(CONFIG_MX53) || defined(CONFIG_MX6)
+	{ "gpio_mxc", &mxc_plat[3] },
+#endif
+#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
+	{ "gpio_mxc", &mxc_plat[4] },
+	{ "gpio_mxc", &mxc_plat[5] },
+#endif
+#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
+	{ "gpio_mxc", &mxc_plat[6] },
+#endif
+};
+#endif