Hi Simon,
Thanks for reviewing.
On Sat, Apr 09, 2016 at 12:33:40PM -0600, Simon Glass wrote:
Hi Peng,
On 21 March 2016 at 02:29, Peng Fan van.freenix@gmail.com wrote:
Introduce driver to support "fairchild,74hc595" devices.
- Take linux drivers/drivers/gpio/gpio-74x164.c as reference.
- Following the naming used in Linux driver with gen_7x164 as the prefix.
- Enable CONFIG_DM_74X164 to use this driver.
- Follow Documentation/devicetree/bindings/gpio/gpio-74x164.txt to add device nodes
- Tested on i.MX6 UltraLite with 74LV595 using gpio command and oscillograph.
Signed-off-by: Peng Fan van.freenix@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Masahiro Yamada yamada.masahiro@socionext.com Cc: Chin Liang See clsee@altera.com Cc: Bhuvanchandra DV bhuvanchandra.dv@toradex.com Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Fabio Estevam fabio.estevam@nxp.com Cc: Stefano Babic sbabic@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@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
Will drop it in V2.
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
Fix in V2.
int ret;if (value)info->buffer[bank] |= 1 << pin;elseinfo->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
Fix in V2.
+}
+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?
In Kernel: Documentation/devicetree/bindings/gpio/gpio-74x164.txt
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.
In general usage, only 1, but follow kernel, designed to be chained. So we do not have fixed value.
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.
Will fix in V2.
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?
Oh. Should be used for spi_get_bus_and_cs, will fix in V2.
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);
Ok. Will try this. Fix in V2.
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.
Fix in V2.
.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_GPIOhelpDriver for 74x164 compatible serial-in/parallel-out 8-outputs74x164-compatible
Should you mention the oriiginal manufacturer. Also you could mention that it uses SPI.
Will fix in V2.
shift registers. This driver can be used to provide accessto 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