[U-Boot,v2,4/8] lpc32xx: add GPIO support

This driver only supports Driver Model, not legacy model.

Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud@3adev.fr>
---

Changes in v2:
- move from legacy to Driver Model support

 arch/arm/cpu/arm926ejs/lpc32xx/devices.c |   5 +
 arch/arm/include/asm/arch-lpc32xx/gpio.h |  43 +++++
 drivers/gpio/Makefile                    |   1 +
 drivers/gpio/lpc32xx_gpio.c              | 268 +++++++++++++++++++++++++++++++
 4 files changed, 317 insertions(+)
 create mode 100644 arch/arm/include/asm/arch-lpc32xx/gpio.h
 create mode 100644 drivers/gpio/lpc32xx_gpio.c

Hi Albert,

On 12 February 2015 at 10:37, Albert ARIBAUD (3ADEV)
<albert.aribaud@3adev.fr> wrote:
> This driver only supports Driver Model, not legacy model.
>
> Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud@3adev.fr>
> ---
>
> Changes in v2:
> - move from legacy to Driver Model support
>
>  arch/arm/cpu/arm926ejs/lpc32xx/devices.c |   5 +
>  arch/arm/include/asm/arch-lpc32xx/gpio.h |  43 +++++
>  drivers/gpio/Makefile                    |   1 +
>  drivers/gpio/lpc32xx_gpio.c              | 268 +++++++++++++++++++++++++++++++
>  4 files changed, 317 insertions(+)
>  create mode 100644 arch/arm/include/asm/arch-lpc32xx/gpio.h
>  create mode 100644 drivers/gpio/lpc32xx_gpio.c
>
> diff --git a/arch/arm/cpu/arm926ejs/lpc32xx/devices.c b/arch/arm/cpu/arm926ejs/lpc32xx/devices.c
> index 81b53ea..a407098 100644
> --- a/arch/arm/cpu/arm926ejs/lpc32xx/devices.c
> +++ b/arch/arm/cpu/arm926ejs/lpc32xx/devices.c
> @@ -9,6 +9,7 @@
>  #include <asm/arch/clk.h>
>  #include <asm/arch/uart.h>
>  #include <asm/io.h>
> +#include <dm.h>
>
>  static struct clk_pm_regs    *clk  = (struct clk_pm_regs *)CLK_PM_BASE;
>  static struct uart_ctrl_regs *ctrl = (struct uart_ctrl_regs *)UART_CTRL_BASE;
> @@ -61,3 +62,7 @@ void lpc32xx_i2c_init(unsigned int devnum)
>                 ctrl |= CLK_I2C2_ENABLE;
>         writel(ctrl, &clk->i2cclk_ctrl);
>  }
> +
> +U_BOOT_DEVICE(lpc32xx_gpios) = {
> +       .name = "gpio_lpc32xx"
> +};
> diff --git a/arch/arm/include/asm/arch-lpc32xx/gpio.h b/arch/arm/include/asm/arch-lpc32xx/gpio.h
> new file mode 100644
> index 0000000..3bd94e3
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-lpc32xx/gpio.h
> @@ -0,0 +1,43 @@
> +/*
> + * LPC32xx GPIO interface
> + *
> + * (C) Copyright 2014  DENX Software Engineering GmbH
> + * Written-by: Albert ARIBAUD <albert.aribaud@3adev.fr>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +/**
> + * GPIO Register map for LPC32xx
> + */
> +
> +struct gpio_regs {
> +       u32 p3_inp_state;
> +       u32 p3_outp_set;
> +       u32 p3_outp_clr;
> +       u32 p3_outp_state;
> +       /* Watch out! the following are shared between p2 and p3 */
> +       u32 p2_p3_dir_set;
> +       u32 p2_p3_dir_clr;
> +       u32 p2_p3_dir_state;
> +       /* Now back to 'one register for one port' */
> +       u32 p2_inp_state;
> +       u32 p2_outp_set;
> +       u32 p2_outp_clr;
> +       u32 reserved1[6];
> +       u32 p0_inp_state;
> +       u32 p0_outp_set;
> +       u32 p0_outp_clr;
> +       u32 p0_outp_state;
> +       u32 p0_dir_set;
> +       u32 p0_dir_clr;
> +       u32 p0_dir_state;
> +       u32 reserved2;
> +       u32 p1_inp_state;
> +       u32 p1_outp_set;
> +       u32 p1_outp_clr;
> +       u32 p1_outp_state;
> +       u32 p1_dir_set;
> +       u32 p1_dir_clr;
> +       u32 p1_dir_state;
> +};
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index aa11f15..559894a 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -37,3 +37,4 @@ obj-$(CONFIG_ADI_GPIO2)       += adi_gpio2.o
>  obj-$(CONFIG_TCA642X)          += tca642x.o
>  oby-$(CONFIG_SX151X)           += sx151x.o
>  obj-$(CONFIG_SUNXI_GPIO)       += sunxi_gpio.o
> +obj-$(CONFIG_LPC32XX_GPIO)     += lpc32xx_gpio.o
> diff --git a/drivers/gpio/lpc32xx_gpio.c b/drivers/gpio/lpc32xx_gpio.c
> new file mode 100644
> index 0000000..861975e
> --- /dev/null
> +++ b/drivers/gpio/lpc32xx_gpio.c
> @@ -0,0 +1,268 @@
> +/*
> + * LPC32xxGPIO driver
> + *
> + * (C) Copyright 2014  DENX Software Engineering GmbH
> + * Written-by: Albert ARIBAUD <albert.aribaud@3adev.fr>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +/**
> + * We only support driver model
> + */
> +#ifndef CONFIG_DM_GPIO
> +#error Please enable Driver Model GPIO in your target configuration.
> +#endif

Minor note - if you base this on dm/master you can use Kconfig
'depends on DM' in this driver's Kconfig bit.

(pull request to mainline coming soon)

> +
> +#include <asm/io.h>
> +#include <asm/arch-lpc32xx/cpu.h>
> +#include <asm/arch-lpc32xx/gpio.h>
> +#include <asm-generic/gpio.h>
> +#include <dm.h>
> +#include <malloc.h>
> +
> +/**
> + * LPC32xx GPIOs work in banks but are non-homogeneous:
> + * - each bank holds a different number of GPIOs
> + * - some GPIOs are input/ouput, some input only, some output only;
> + * - some GPIOs have different meanings as an input and as an output;
> + * - some GPIOs are controlled on a given port and bit index, but
> + *   read on another one.
> +*
> + * In order to keep this code simple, GPIOS are considered here as
> + * homogeneous and linear, from 0 to 127.
> + *
> + *     ** WARNING **
> + *
> + * Client code is responsible for properly using valid GPIO numbers,
> + * including cases where a single physical GPIO has differing numbers
> + * for setting its direction, reading it and/or writing to it.
> + */
> +
> +#define LPC32XX_GPIOS 128
> +
> +static struct gpio_regs *regs = (struct gpio_regs *)GPIO_BASE;

Normally this would go in a

struct lpc32xx_priv

in the driver.

> +
> +/**
> + * We have 4 GPIO ports of 32 bits each
> + */
> +
> +#define MAX_GPIO 128
> +
> +#define GPIO_TO_PORT(gpio) ((gpio / 32) & 3)
> +#define GPIO_TO_RANK(gpio) (gpio % 32)
> +#define GPIO_TO_MASK(gpio) (1 << (gpio % 32))
> +
> +/**
> + * Array of current GPIO functions. Allocated as unsigned chars to
> + * limit memory consumption.
> + */
> +
> +static signed char lpc32xx_function[LPC32XX_GPIOS];

...along with this.

> +
> +/**
> + * Configure a GPIO number 'offset' as input
> + */
> +
> +static int lpc32xx_gpio_direction_input(struct udevice *dev, unsigned offset)
> +{
> +       int port, mask;
> +
> +       port = GPIO_TO_PORT(offset);
> +       mask = GPIO_TO_MASK(offset);
> +
> +       switch (port) {
> +       case 0:
> +               writel(mask, &regs->p0_dir_clr);
> +               break;
> +       case 1:
> +               writel(mask, &regs->p1_dir_clr);
> +               break;
> +       case 2:
> +               /* ports 2 and 3 share a common direction */
> +       case 3:
> +               writel(mask, &regs->p2_p3_dir_clr);
> +               break;
> +       default:
> +               return -1;
> +       }
> +
> +       lpc32xx_function[offset] = GPIOF_INPUT;

Another way of doing this is to read the status from the hardware.
This might allow you to support GPIOF_FUNCTION - i.e. the GPIO is
currently used by a function.

> +
> +       return 0;
> +}
> +
> +/**
> + * Get the value of a GPIO
> + */
> +
> +static int lpc32xx_gpio_get_value(struct udevice *dev, unsigned offset)
> +{
> +       int port, rank, mask, value;
> +
> +       port = GPIO_TO_PORT(offset);
> +
> +       switch (port) {
> +       case 0:
> +               value = readl(&regs->p0_inp_state);
> +               break;
> +       case 1:
> +               value = readl(&regs->p1_inp_state);
> +               break;
> +       case 2:
> +               value = readl(&regs->p2_inp_state);
> +               break;
> +       case 3:
> +               value = readl(&regs->p3_inp_state);
> +               break;
> +       default:
> +               return -1;
> +       }
> +
> +       rank = GPIO_TO_RANK(offset);
> +       mask = GPIO_TO_MASK(offset);
> +
> +       return (value & mask) >> rank;
> +}
> +
> +/**
> + * Set a GPIO
> + */
> +
> +static int gpio_set(unsigned gpio)
> +{
> +       int port, mask;
> +
> +       port = GPIO_TO_PORT(gpio);
> +       mask = GPIO_TO_MASK(gpio);
> +
> +       switch (port) {
> +       case 0:
> +               writel(mask, &regs->p0_outp_set);
> +               break;
> +       case 1:
> +               writel(mask, &regs->p1_outp_set);
> +               break;
> +       case 2:
> +               writel(mask, &regs->p2_outp_set);
> +               break;
> +       case 3:
> +               writel(mask, &regs->p3_outp_set);
> +               break;
> +       default:
> +               return -1;
> +       }
> +       return 0;
> +}
> +
> +/**
> + * Clear a GPIO
> + */
> +
> +static int gpio_clr(unsigned gpio)
> +{
> +       int port, mask;
> +
> +       port = GPIO_TO_PORT(gpio);
> +       mask = GPIO_TO_MASK(gpio);
> +
> +       switch (port) {
> +       case 0:
> +               writel(mask, &regs->p0_outp_clr);
> +               break;
> +       case 1:
> +               writel(mask, &regs->p1_outp_clr);
> +               break;
> +       case 2:
> +               writel(mask, &regs->p2_outp_clr);
> +               break;
> +       case 3:
> +               writel(mask, &regs->p3_outp_clr);
> +               break;
> +       default:
> +               return -1;
> +       }
> +       return 0;
> +}
> +
> +/**
> + * Set the value of a GPIO
> + */
> +
> +static int lpc32xx_gpio_set_value(struct udevice *dev, unsigned offset,
> +                                int value)
> +{
> +       if (value)
> +               return gpio_set(offset);
> +       else
> +               return gpio_clr(offset);
> +}
> +
> +/**
> + * Configure a GPIO number 'offset' as output with given initial value.
> + */
> +
> +static int lpc32xx_gpio_direction_output(struct udevice *dev, unsigned offset,
> +                                      int value)
> +{
> +       int port, mask;
> +
> +       port = GPIO_TO_PORT(offset);
> +       mask = GPIO_TO_MASK(offset);
> +
> +       switch (port) {
> +       case 0:
> +               writel(mask, &regs->p0_dir_set);
> +               break;
> +       case 1:
> +               writel(mask, &regs->p1_dir_set);
> +               break;
> +       case 2:
> +               /* ports 2 and 3 share a common direction */
> +       case 3:
> +               writel(mask, &regs->p2_p3_dir_set);
> +               break;
> +       default:
> +               return -1;
> +       }
> +
> +       lpc32xx_function[offset] = GPIOF_OUTPUT;
> +
> +       return lpc32xx_gpio_set_value(dev, offset, value);
> +}
> +
> +static int lpc32xx_gpio_get_function(struct udevice *dev, unsigned offset)
> +{
> +       return lpc32xx_function[offset];
> +}
> +
> +static const struct dm_gpio_ops gpio_lpc32xx_ops = {
> +       .direction_input        = lpc32xx_gpio_direction_input,
> +       .direction_output       = lpc32xx_gpio_direction_output,
> +       .get_value              = lpc32xx_gpio_get_value,
> +       .set_value              = lpc32xx_gpio_set_value,
> +       .get_function           = lpc32xx_gpio_get_function,
> +};
> +
> +static int lpc32xx_gpio_probe(struct udevice *dev)
> +{
> +       struct gpio_dev_priv *uc_priv = dev->uclass_priv;
> +
> +       if (dev->of_offset == -1) {
> +               /* Tell the uclass how many GPIOs we have */
> +               uc_priv->gpio_count = LPC32XX_GPIOS;
> +       }
> +
> +       /* all GPIO functions are unknown until requested */
> +       memset(lpc32xx_function, GPIOF_UNKNOWN, LPC32XX_GPIOS);
> +
> +       return 0;
> +}
> +
> +U_BOOT_DRIVER(gpio_lpc32xx) = {
> +       .name   = "gpio_lpc32xx",
> +       .id     = UCLASS_GPIO,
> +       .ops    = &gpio_lpc32xx_ops,
> +       .probe  = lpc32xx_gpio_probe,
> +       .priv_auto_alloc_size = 0,

You can use sizeof(struct lpc32xx_priv) here and it will automatically
allocate your structure when the device is probed.

> +};
> --
> 2.1.0
>

Regards,
Simon

Hi Simon,

Le Thu, 12 Feb 2015 22:06:51 -0700, Simon Glass <sjg@chromium.org> a
écrit :

> Hi Albert,
> 
> On 12 February 2015 at 10:37, Albert ARIBAUD (3ADEV)
> <albert.aribaud@3adev.fr> wrote:
> > This driver only supports Driver Model, not legacy model.
> >
> > Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud@3adev.fr>
> > ---
> >
> > Changes in v2:
> > - move from legacy to Driver Model support
> >
> >  arch/arm/cpu/arm926ejs/lpc32xx/devices.c |   5 +
> >  arch/arm/include/asm/arch-lpc32xx/gpio.h |  43 +++++
> >  drivers/gpio/Makefile                    |   1 +
> >  drivers/gpio/lpc32xx_gpio.c              | 268 +++++++++++++++++++++++++++++++
> >  4 files changed, 317 insertions(+)
> >  create mode 100644 arch/arm/include/asm/arch-lpc32xx/gpio.h
> >  create mode 100644 drivers/gpio/lpc32xx_gpio.c
> >
> > diff --git a/arch/arm/cpu/arm926ejs/lpc32xx/devices.c b/arch/arm/cpu/arm926ejs/lpc32xx/devices.c
> > index 81b53ea..a407098 100644
> > --- a/arch/arm/cpu/arm926ejs/lpc32xx/devices.c
> > +++ b/arch/arm/cpu/arm926ejs/lpc32xx/devices.c
> > @@ -9,6 +9,7 @@
> >  #include <asm/arch/clk.h>
> >  #include <asm/arch/uart.h>
> >  #include <asm/io.h>
> > +#include <dm.h>
> >
> >  static struct clk_pm_regs    *clk  = (struct clk_pm_regs *)CLK_PM_BASE;
> >  static struct uart_ctrl_regs *ctrl = (struct uart_ctrl_regs *)UART_CTRL_BASE;
> > @@ -61,3 +62,7 @@ void lpc32xx_i2c_init(unsigned int devnum)
> >                 ctrl |= CLK_I2C2_ENABLE;
> >         writel(ctrl, &clk->i2cclk_ctrl);
> >  }
> > +
> > +U_BOOT_DEVICE(lpc32xx_gpios) = {
> > +       .name = "gpio_lpc32xx"
> > +};
> > diff --git a/arch/arm/include/asm/arch-lpc32xx/gpio.h b/arch/arm/include/asm/arch-lpc32xx/gpio.h
> > new file mode 100644
> > index 0000000..3bd94e3
> > --- /dev/null
> > +++ b/arch/arm/include/asm/arch-lpc32xx/gpio.h
> > @@ -0,0 +1,43 @@
> > +/*
> > + * LPC32xx GPIO interface
> > + *
> > + * (C) Copyright 2014  DENX Software Engineering GmbH
> > + * Written-by: Albert ARIBAUD <albert.aribaud@3adev.fr>
> > + *
> > + * SPDX-License-Identifier:    GPL-2.0+
> > + */
> > +
> > +/**
> > + * GPIO Register map for LPC32xx
> > + */
> > +
> > +struct gpio_regs {
> > +       u32 p3_inp_state;
> > +       u32 p3_outp_set;
> > +       u32 p3_outp_clr;
> > +       u32 p3_outp_state;
> > +       /* Watch out! the following are shared between p2 and p3 */
> > +       u32 p2_p3_dir_set;
> > +       u32 p2_p3_dir_clr;
> > +       u32 p2_p3_dir_state;
> > +       /* Now back to 'one register for one port' */
> > +       u32 p2_inp_state;
> > +       u32 p2_outp_set;
> > +       u32 p2_outp_clr;
> > +       u32 reserved1[6];
> > +       u32 p0_inp_state;
> > +       u32 p0_outp_set;
> > +       u32 p0_outp_clr;
> > +       u32 p0_outp_state;
> > +       u32 p0_dir_set;
> > +       u32 p0_dir_clr;
> > +       u32 p0_dir_state;
> > +       u32 reserved2;
> > +       u32 p1_inp_state;
> > +       u32 p1_outp_set;
> > +       u32 p1_outp_clr;
> > +       u32 p1_outp_state;
> > +       u32 p1_dir_set;
> > +       u32 p1_dir_clr;
> > +       u32 p1_dir_state;
> > +};
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index aa11f15..559894a 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -37,3 +37,4 @@ obj-$(CONFIG_ADI_GPIO2)       += adi_gpio2.o
> >  obj-$(CONFIG_TCA642X)          += tca642x.o
> >  oby-$(CONFIG_SX151X)           += sx151x.o
> >  obj-$(CONFIG_SUNXI_GPIO)       += sunxi_gpio.o
> > +obj-$(CONFIG_LPC32XX_GPIO)     += lpc32xx_gpio.o
> > diff --git a/drivers/gpio/lpc32xx_gpio.c b/drivers/gpio/lpc32xx_gpio.c
> > new file mode 100644
> > index 0000000..861975e
> > --- /dev/null
> > +++ b/drivers/gpio/lpc32xx_gpio.c
> > @@ -0,0 +1,268 @@
> > +/*
> > + * LPC32xxGPIO driver
> > + *
> > + * (C) Copyright 2014  DENX Software Engineering GmbH
> > + * Written-by: Albert ARIBAUD <albert.aribaud@3adev.fr>
> > + *
> > + * SPDX-License-Identifier:    GPL-2.0+
> > + */
> > +
> > +/**
> > + * We only support driver model
> > + */
> > +#ifndef CONFIG_DM_GPIO
> > +#error Please enable Driver Model GPIO in your target configuration.
> > +#endif
> 
> Minor note - if you base this on dm/master you can use Kconfig
> 'depends on DM' in this driver's Kconfig bit.
> 
> (pull request to mainline coming soon)

I will rebase then for v3.

> > +#include <asm/io.h>
> > +#include <asm/arch-lpc32xx/cpu.h>
> > +#include <asm/arch-lpc32xx/gpio.h>
> > +#include <asm-generic/gpio.h>
> > +#include <dm.h>
> > +#include <malloc.h>
> > +
> > +/**
> > + * LPC32xx GPIOs work in banks but are non-homogeneous:
> > + * - each bank holds a different number of GPIOs
> > + * - some GPIOs are input/ouput, some input only, some output only;
> > + * - some GPIOs have different meanings as an input and as an output;
> > + * - some GPIOs are controlled on a given port and bit index, but
> > + *   read on another one.
> > +*
> > + * In order to keep this code simple, GPIOS are considered here as
> > + * homogeneous and linear, from 0 to 127.
> > + *
> > + *     ** WARNING **
> > + *
> > + * Client code is responsible for properly using valid GPIO numbers,
> > + * including cases where a single physical GPIO has differing numbers
> > + * for setting its direction, reading it and/or writing to it.
> > + */
> > +
> > +#define LPC32XX_GPIOS 128
> > +
> > +static struct gpio_regs *regs = (struct gpio_regs *)GPIO_BASE;
> 
> Normally this would go in a
> 
> struct lpc32xx_priv
> 
> in the driver.

Will change this.

> > +
> > +/**
> > + * We have 4 GPIO ports of 32 bits each
> > + */
> > +
> > +#define MAX_GPIO 128
> > +
> > +#define GPIO_TO_PORT(gpio) ((gpio / 32) & 3)
> > +#define GPIO_TO_RANK(gpio) (gpio % 32)
> > +#define GPIO_TO_MASK(gpio) (1 << (gpio % 32))
> > +
> > +/**
> > + * Array of current GPIO functions. Allocated as unsigned chars to
> > + * limit memory consumption.
> > + */
> > +
> > +static signed char lpc32xx_function[LPC32XX_GPIOS];
> 
> ...along with this.

Ditto.

> > +
> > +/**
> > + * Configure a GPIO number 'offset' as input
> > + */
> > +
> > +static int lpc32xx_gpio_direction_input(struct udevice *dev, unsigned offset)
> > +{
> > +       int port, mask;
> > +
> > +       port = GPIO_TO_PORT(offset);
> > +       mask = GPIO_TO_MASK(offset);
> > +
> > +       switch (port) {
> > +       case 0:
> > +               writel(mask, &regs->p0_dir_clr);
> > +               break;
> > +       case 1:
> > +               writel(mask, &regs->p1_dir_clr);
> > +               break;
> > +       case 2:
> > +               /* ports 2 and 3 share a common direction */
> > +       case 3:
> > +               writel(mask, &regs->p2_p3_dir_clr);
> > +               break;
> > +       default:
> > +               return -1;
> > +       }
> > +
> > +       lpc32xx_function[offset] = GPIOF_INPUT;
> 
> Another way of doing this is to read the status from the hardware.
> This might allow you to support GPIOF_FUNCTION - i.e. the GPIO is
> currently used by a function.

Even under the simplified model I used for LPC32XX GPIOs, finding out
the state of an individual gpio would be quite complicated and would
incur much work for a feature which is actually only useful for the
'gpio' command -- a commodity but not a necessity as far a the actual
board user is concerned.

> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * Get the value of a GPIO
> > + */
> > +
> > +static int lpc32xx_gpio_get_value(struct udevice *dev, unsigned offset)
> > +{
> > +       int port, rank, mask, value;
> > +
> > +       port = GPIO_TO_PORT(offset);
> > +
> > +       switch (port) {
> > +       case 0:
> > +               value = readl(&regs->p0_inp_state);
> > +               break;
> > +       case 1:
> > +               value = readl(&regs->p1_inp_state);
> > +               break;
> > +       case 2:
> > +               value = readl(&regs->p2_inp_state);
> > +               break;
> > +       case 3:
> > +               value = readl(&regs->p3_inp_state);
> > +               break;
> > +       default:
> > +               return -1;
> > +       }
> > +
> > +       rank = GPIO_TO_RANK(offset);
> > +       mask = GPIO_TO_MASK(offset);
> > +
> > +       return (value & mask) >> rank;
> > +}
> > +
> > +/**
> > + * Set a GPIO
> > + */
> > +
> > +static int gpio_set(unsigned gpio)
> > +{
> > +       int port, mask;
> > +
> > +       port = GPIO_TO_PORT(gpio);
> > +       mask = GPIO_TO_MASK(gpio);
> > +
> > +       switch (port) {
> > +       case 0:
> > +               writel(mask, &regs->p0_outp_set);
> > +               break;
> > +       case 1:
> > +               writel(mask, &regs->p1_outp_set);
> > +               break;
> > +       case 2:
> > +               writel(mask, &regs->p2_outp_set);
> > +               break;
> > +       case 3:
> > +               writel(mask, &regs->p3_outp_set);
> > +               break;
> > +       default:
> > +               return -1;
> > +       }
> > +       return 0;
> > +}
> > +
> > +/**
> > + * Clear a GPIO
> > + */
> > +
> > +static int gpio_clr(unsigned gpio)
> > +{
> > +       int port, mask;
> > +
> > +       port = GPIO_TO_PORT(gpio);
> > +       mask = GPIO_TO_MASK(gpio);
> > +
> > +       switch (port) {
> > +       case 0:
> > +               writel(mask, &regs->p0_outp_clr);
> > +               break;
> > +       case 1:
> > +               writel(mask, &regs->p1_outp_clr);
> > +               break;
> > +       case 2:
> > +               writel(mask, &regs->p2_outp_clr);
> > +               break;
> > +       case 3:
> > +               writel(mask, &regs->p3_outp_clr);
> > +               break;
> > +       default:
> > +               return -1;
> > +       }
> > +       return 0;
> > +}
> > +
> > +/**
> > + * Set the value of a GPIO
> > + */
> > +
> > +static int lpc32xx_gpio_set_value(struct udevice *dev, unsigned offset,
> > +                                int value)
> > +{
> > +       if (value)
> > +               return gpio_set(offset);
> > +       else
> > +               return gpio_clr(offset);
> > +}
> > +
> > +/**
> > + * Configure a GPIO number 'offset' as output with given initial value.
> > + */
> > +
> > +static int lpc32xx_gpio_direction_output(struct udevice *dev, unsigned offset,
> > +                                      int value)
> > +{
> > +       int port, mask;
> > +
> > +       port = GPIO_TO_PORT(offset);
> > +       mask = GPIO_TO_MASK(offset);
> > +
> > +       switch (port) {
> > +       case 0:
> > +               writel(mask, &regs->p0_dir_set);
> > +               break;
> > +       case 1:
> > +               writel(mask, &regs->p1_dir_set);
> > +               break;
> > +       case 2:
> > +               /* ports 2 and 3 share a common direction */
> > +       case 3:
> > +               writel(mask, &regs->p2_p3_dir_set);
> > +               break;
> > +       default:
> > +               return -1;
> > +       }
> > +
> > +       lpc32xx_function[offset] = GPIOF_OUTPUT;
> > +
> > +       return lpc32xx_gpio_set_value(dev, offset, value);
> > +}
> > +
> > +static int lpc32xx_gpio_get_function(struct udevice *dev, unsigned offset)
> > +{
> > +       return lpc32xx_function[offset];
> > +}
> > +
> > +static const struct dm_gpio_ops gpio_lpc32xx_ops = {
> > +       .direction_input        = lpc32xx_gpio_direction_input,
> > +       .direction_output       = lpc32xx_gpio_direction_output,
> > +       .get_value              = lpc32xx_gpio_get_value,
> > +       .set_value              = lpc32xx_gpio_set_value,
> > +       .get_function           = lpc32xx_gpio_get_function,
> > +};
> > +
> > +static int lpc32xx_gpio_probe(struct udevice *dev)
> > +{
> > +       struct gpio_dev_priv *uc_priv = dev->uclass_priv;
> > +
> > +       if (dev->of_offset == -1) {
> > +               /* Tell the uclass how many GPIOs we have */
> > +               uc_priv->gpio_count = LPC32XX_GPIOS;
> > +       }
> > +
> > +       /* all GPIO functions are unknown until requested */
> > +       memset(lpc32xx_function, GPIOF_UNKNOWN, LPC32XX_GPIOS);
> > +
> > +       return 0;
> > +}
> > +
> > +U_BOOT_DRIVER(gpio_lpc32xx) = {
> > +       .name   = "gpio_lpc32xx",
> > +       .id     = UCLASS_GPIO,
> > +       .ops    = &gpio_lpc32xx_ops,
> > +       .probe  = lpc32xx_gpio_probe,
> > +       .priv_auto_alloc_size = 0,
> 
> You can use sizeof(struct lpc32xx_priv) here and it will automatically
> allocate your structure when the device is probed.

Yep, that'll get in along with moving the base address and intenal
gpio state array.

> > +};
> > --
> > 2.1.0
> >
> 
> Regards,
> Simon

Thanks for your review!

Cordialement,
Albert ARIBAUD
3ADEV

Hi Albert,

On 12 February 2015 at 23:30, Albert ARIBAUD <albert.aribaud@3adev.fr> wrote:
> Hi Simon,
>
> Le Thu, 12 Feb 2015 22:06:51 -0700, Simon Glass <sjg@chromium.org> a
> écrit :
>
>> Hi Albert,
>>
>> On 12 February 2015 at 10:37, Albert ARIBAUD (3ADEV)
>> <albert.aribaud@3adev.fr> wrote:
>> > This driver only supports Driver Model, not legacy model.
>> >
>> > Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud@3adev.fr>
>> > ---
>> >
>> > Changes in v2:
>> > - move from legacy to Driver Model support
[snip]
>> > +static int lpc32xx_gpio_direction_input(struct udevice *dev, unsigned offset)
>> > +{
>> > +       int port, mask;
>> > +
>> > +       port = GPIO_TO_PORT(offset);
>> > +       mask = GPIO_TO_MASK(offset);
>> > +
>> > +       switch (port) {
>> > +       case 0:
>> > +               writel(mask, &regs->p0_dir_clr);
>> > +               break;
>> > +       case 1:
>> > +               writel(mask, &regs->p1_dir_clr);
>> > +               break;
>> > +       case 2:
>> > +               /* ports 2 and 3 share a common direction */
>> > +       case 3:
>> > +               writel(mask, &regs->p2_p3_dir_clr);
>> > +               break;
>> > +       default:
>> > +               return -1;
>> > +       }
>> > +
>> > +       lpc32xx_function[offset] = GPIOF_INPUT;
>>
>> Another way of doing this is to read the status from the hardware.
>> This might allow you to support GPIOF_FUNCTION - i.e. the GPIO is
>> currently used by a function.
>
> Even under the simplified model I used for LPC32XX GPIOs, finding out
> the state of an individual gpio would be quite complicated and would
> incur much work for a feature which is actually only useful for the
> 'gpio' command -- a commodity but not a necessity as far a the actual
> board user is concerned.

Yes it's only a nicety and can easily be added later if someone wants
it. But it might be worth adding a comment to that effect - someone
using this driver for inspiration might get the wrong idea.

Regards,
Simon