[U-Boot] [PATCH 2/2] dm: gpio: introduce 74x164 driver

Simon Glass sjg at chromium.org
Sat Apr 9 20:33:40 CEST 2016


Hi Peng,

On 21 March 2016 at 02:29, Peng Fan <van.freenix at gmail.com> wrote:
> Introduce driver to support "fairchild,74hc595" devices.
> 1. Take linux drivers/drivers/gpio/gpio-74x164.c as reference.
> 2. Following the naming used in Linux driver with gen_7x164 as the prefix.
> 3. Enable CONFIG_DM_74X164 to use this driver.
> 4. Follow Documentation/devicetree/bindings/gpio/gpio-74x164.txt to add device
>    nodes
> 5. Tested on i.MX6 UltraLite with 74LV595 using gpio command and oscillograph.
>
> Signed-off-by: Peng Fan <van.freenix at gmail.com>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Masahiro Yamada <yamada.masahiro at socionext.com>
> Cc: Chin Liang See <clsee at altera.com>
> Cc: Bhuvanchandra DV <bhuvanchandra.dv at toradex.com>
> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
> Cc: Fabio Estevam <fabio.estevam at nxp.com>
> Cc: Stefano Babic <sbabic at denx.de>
> ---
>  drivers/gpio/74x164_gpio.c | 220 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/gpio/Kconfig       |   8 ++
>  drivers/gpio/Makefile      |   1 +
>  3 files changed, 229 insertions(+)
>  create mode 100644 drivers/gpio/74x164_gpio.c
>
> diff --git a/drivers/gpio/74x164_gpio.c b/drivers/gpio/74x164_gpio.c
> new file mode 100644
> index 0000000..5d12a9a
> --- /dev/null
> +++ b/drivers/gpio/74x164_gpio.c
> @@ -0,0 +1,220 @@
> +/*
> + * Take drivers/gpio/gpio-74x164.c as reference.
> + *
> + * 74Hx164 - Generic serial-in/parallel-out 8-bits shift register GPIO driver
> + *
> + * Copyright (C) 2016 Peng Fan <van.freenix at gmail.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + *
> + */
> +
> +#include <common.h>
> +#include <errno.h>
> +#include <dm.h>
> +#include <fdtdec.h>
> +#include <malloc.h>
> +#include <asm/gpio.h>
> +#include <asm/io.h>
> +#include <dt-bindings/gpio/gpio.h>
> +#include <spi.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/*
> + * struct gen_74x164_chip - Data for 74Hx164
> + *
> + * @dev: udevice structure for the device

This doesn't appear to be u
> + * @slave: spi slave
> + * @oe: OE pin
> + * @nregs: number of registers
> + * @buffer: buffer for chained chips
> + */
> +#define GEN_74X164_NUMBER_GPIOS 8
> +
> +struct gen_74x164_info {

How about gen_74x164_priv?

> +       struct udevice *dev;

This doesn't seem to be used

> +       struct spi_slave *slave;
> +       struct gpio_desc oe;
> +       u32 nregs;
> +       /*
> +        * Since the nregs are chained, every byte sent will make
> +        * the previous byte shift to the next register in the
> +        * chain. Thus, the first byte sent will end up in the last
> +        * register at the end of the transfer. So, to have a logical
> +        * numbering, store the bytes in reverse order.
> +        */
> +       u8 *buffer;
> +};
> +
> +static int gen_74x164_write_conf(struct udevice *dev)
> +{
> +       struct gen_74x164_info *info = dev_get_platdata(dev);
> +       struct spi_slave *slave = info->slave;
> +       int ret;
> +
> +       ret = spi_claim_bus(slave);
> +       if (ret)
> +               return ret;
> +
> +       ret = spi_xfer(slave, info->nregs * 8, info->buffer, NULL,
> +                      SPI_XFER_BEGIN | SPI_XFER_END);
> +
> +       spi_release_bus(slave);
> +
> +       return ret;
> +}
> +
> +static int gen_74x164_get_value(struct udevice *dev, unsigned offset)
> +{
> +       struct gen_74x164_info *info = dev_get_platdata(dev);
> +       u8 bank = info->nregs - 1 - offset / 8;
> +       u8 pin = offset % 8;
> +
> +       return (info->buffer[bank] >> pin) & 0x1;
> +}
> +
> +static int gen_74x164_set_value(struct udevice *dev, unsigned offset,
> +                               int value)
> +{
> +       struct gen_74x164_info *info = dev_get_platdata(dev);
> +       u8 bank = info->nregs - 1 - offset / 8;
> +       u8 pin = offset % 8;

uint should be good enough for these

> +       int ret;
> +
> +       if (value)
> +               info->buffer[bank] |= 1 << pin;
> +       else
> +               info->buffer[bank] &= ~(1 << pin);
> +
> +       ret = gen_74x164_write_conf(dev);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +static int gen_74x164_direction_input(struct udevice *dev, unsigned offset)
> +{
> +       return -EINVAL;

-ENOSYS if it is not supported

> +}
> +
> +static int gen_74x164_direction_output(struct udevice *dev, unsigned offset,
> +                                     int value)
> +{

> +       return gen_74x164_set_value(dev, offset, value);
> +}
> +
> +static int gen_74x164_get_function(struct udevice *dev, unsigned offset)
> +{
> +       return GPIOF_OUTPUT;
> +}
> +
> +static int gen_74x164_xlate(struct udevice *dev, struct gpio_desc *desc,
> +                           struct fdtdec_phandle_args *args)
> +{
> +       desc->offset = args->args[0];
> +       desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0;
> +
> +       return 0;
> +}
> +
> +static const struct dm_gpio_ops gen_74x164_ops = {
> +       .direction_input        = gen_74x164_direction_input,
> +       .direction_output       = gen_74x164_direction_output,
> +       .get_value              = gen_74x164_get_value,
> +       .set_value              = gen_74x164_set_value,
> +       .get_function           = gen_74x164_get_function,
> +       .xlate                  = gen_74x164_xlate,
> +};
> +
> +static int gen_74x164_probe(struct udevice *dev)
> +{
> +       struct gen_74x164_info *info = dev_get_platdata(dev);
> +       struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> +       char *str, *str1;
> +       int ret;
> +       const void *fdt = gd->fdt_blob;
> +       int node = dev->of_offset;
> +       struct udevice *bus;
> +       int busnum;
> +       char name[30];
> +
> +       str = strdup(dev->name);
> +       if (!str)
> +               return -ENOMEM;
> +
> +       info->nregs = fdtdec_get_int(fdt, node, "registers-number", 1);

Do you have a device tree binding file you can add for this?

> +       info->buffer = calloc(info->nregs, sizeof(u8));
> +       if (!info->buffer) {
> +               ret = -ENOMEM;
> +               goto free_str;
> +       }

What's the largest number of registers you can have? If it is small,
like 32, consider making buffer a simple fixed-length array.

> +
> +       ret = fdtdec_get_byte_array(fdt, node, "registers-default",
> +                                   info->buffer, info->nregs);
> +       if (ret)
> +               dev_dbg(dev, "No registers-default property\n");
> +
> +       ret = gpio_request_by_name(dev, "oe-gpios", 0, &info->oe,
> +                                  GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
> +       if (ret) {
> +               dev_err(dev, "No oe-pins property\n");
> +               goto free_buf;
> +       }
> +
> +       /* output enable */
> +       ret = dm_gpio_set_value(&info->oe, 0);
> +       if (ret) {
> +               dev_err(dev, "Set oe-pins value failure\n");
> +               goto free_buf;
> +       }


The GPIOD_IS_OUT_ACTIVE above will already activate this GPIO.

> +
> +       uc_priv->bank_name = str;
> +       uc_priv->gpio_count = info->nregs * 8;
> +
> +       bus = dev_get_parent(dev);
> +       busnum = bus->seq;
> +
> +       snprintf(name, sizeof(name), "generic_%d:%d", busnum, 0);
> +       str1 = strdup(name);

What is str1 used for?

> +       if (!str1) {
> +               ret = -ENOMEM;
> +               goto free_buf;
> +       }
> +
> +       ret = spi_get_bus_and_cs(busnum, 0, 1000000, SPI_MODE_0,
> +                                "spi_generic_drv", str, &bus, &info->slave);
> +       if (ret)
> +               goto free_str1;

Why do you need this? The parent device is the SPI bus. See
drivers/misc/cros_ec_spi.c for how it probes. There really shouldn't
be much to do.

I'd suggest:

infp->slave = dev_get_parent_priv(dev);

> +
> +       ret = gen_74x164_write_conf(dev);
> +       if (ret)
> +               goto free_str1;
> +
> +       dev_dbg(dev, "%s is ready\n", dev->name);
> +
> +       return 0;
> +
> +free_str1:
> +       free(str1);
> +free_buf:
> +       free(info->buffer);
> +free_str:
> +       free(str);
> +       return ret;
> +}
> +
> +static const struct udevice_id gen_74x164_ids[] = {
> +       { .compatible = "fairchild,74hc595" },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(74x164) = {
> +       .name           = "74x164",
> +       .id             = UCLASS_GPIO,
> +       .ops            = &gen_74x164_ops,
> +       .probe          = gen_74x164_probe,
> +       .platdata_auto_alloc_size = sizeof(struct gen_74x164_info),

It does not seem to be used before probe(), so I think this should be
priv_auto_alloc_size.

> +       .of_match       = gen_74x164_ids,
> +};
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 51658f1..43fc9c4 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -96,6 +96,14 @@ config PIC32_GPIO
>         help
>           Say yes here to support Microchip PIC32 GPIOs.
>
> +config DM_74X164
> +       bool "74x164 serial-in/parallel-out 8-bits shift register"
> +       depends on DM_GPIO
> +       help
> +         Driver for 74x164 compatible serial-in/parallel-out 8-outputs

74x164-compatible

Should you mention the oriiginal manufacturer. Also you could mention
that it uses SPI.

> +         shift registers. This driver can be used to provide access
> +         to more gpio outputs.
> +
>  config DM_PCA953X
>         bool "PCA95[357]x, PCA9698, TCA64xx, and MAX7310 I/O ports"
>         depends on DM_GPIO
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index bfc67d2..393b148 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -12,6 +12,7 @@ endif
>  obj-$(CONFIG_DM_GPIO)          += gpio-uclass.o
>
>  obj-$(CONFIG_DM_PCA953X)       += pca953x_gpio.o
> +obj-$(CONFIG_DM_74X164)                += 74x164_gpio.o
>
>  obj-$(CONFIG_AT91_GPIO)        += at91_gpio.o
>  obj-$(CONFIG_ATMEL_PIO4)       += atmel_pio4.o
> --
> 2.6.2
>

Regards,
Simon


More information about the U-Boot mailing list