dm: spi: soft_spi: support linux way and bug fix

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Chin Liang See <[email protected]>

Cc Bhuvanchandra DV <[email protected]>

Cc Daniel Schwierzeck <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Stefano Babic <[email protected]>

---
 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

drivers/gpio/74x164_gpio.c new file +17 -0

new file mode 100644
index 0000000..5d12a9a
@@ -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 <[email protected]>
+ *
+ * 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>

drivers/gpio/74x164_gpio.c new file +36 -0

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

drivers/gpio/74x164_gpio.c new file +7 -0

+       int ret;
+
+       if (value)
+               info->buffer[bank] |= 1 << pin;
+       else
+               info->buffer[bank] &= ~(1 << pin);
+

drivers/gpio/74x164_gpio.c new file +38 -0

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

drivers/gpio/74x164_gpio.c new file +9 -0

+
+       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) {

drivers/gpio/74x164_gpio.c new file +19 -0

+
+       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[] = {

drivers/gpio/74x164_gpio.c new file +2 -0

+       .of_match       = gen_74x164_ids,
+};

drivers/gpio/Kconfig +0 -0

index 51658f1..43fc9c4 100644

drivers/gpio/Kconfig +3 -0

+         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

drivers/gpio/Makefile +1 -1

index bfc67d2..393b148 100644
@@ -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

Hi Peng,

Quoted 46 lines · Peng Fan

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Chin Liang See <[email protected]>

Cc Bhuvanchandra DV <[email protected]>

Cc Daniel Schwierzeck <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Stefano Babic <[email protected]>

---
 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

drivers/gpio/74x164_gpio.c new file +17 -0

new file mode 100644
index 0000000..5d12a9a
@@ -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 <[email protected]>
+ *
+ * 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>

drivers/gpio/74x164_gpio.c line 27

Quoted 10 lines

drivers/gpio/74x164_gpio.c new file +10 -0

+#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

drivers/gpio/74x164_gpio.c line 35

Quoted 8 lines

drivers/gpio/74x164_gpio.c new file +8 -0

+ * @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?

drivers/gpio/74x164_gpio.c line 36

Quoted 1 line

drivers/gpio/74x164_gpio.c new file +1 -0

+       struct udevice *dev;

This doesn't seem to be used

Quoted 41 lines

drivers/gpio/74x164_gpio.c new file +40 -0

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

drivers/gpio/74x164_gpio.c line 82

Quoted 6 lines

drivers/gpio/74x164_gpio.c new file +6 -0

+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

Quoted 12 lines

drivers/gpio/74x164_gpio.c new file +11 -0

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

drivers/gpio/74x164_gpio.c line 99

Quoted 6 lines

drivers/gpio/74x164_gpio.c new file +6 -0

+       return 0;
+}
+
+static int gen_74x164_direction_input(struct udevice *dev, unsigned offset)
+{
+       return -EINVAL;

-ENOSYS if it is not supported

Quoted 44 lines

drivers/gpio/74x164_gpio.c new file +42 -0

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

drivers/gpio/74x164_gpio.c line 147

Quoted 6 lines

drivers/gpio/74x164_gpio.c new file +6 -0

+
+       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?

drivers/gpio/74x164_gpio.c line 152

Quoted 6 lines

drivers/gpio/74x164_gpio.c new file +5 -0

+       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.

Quoted 15 lines

drivers/gpio/74x164_gpio.c new file +14 -0

+
+       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 */

drivers/gpio/74x164_gpio.c line 171

Quoted 5 lines

drivers/gpio/74x164_gpio.c new file +5 -0

+       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.

drivers/gpio/74x164_gpio.c line 180

Quoted 10 lines

drivers/gpio/74x164_gpio.c new file +9 -0

+
+       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?

drivers/gpio/74x164_gpio.c line 189

Quoted 10 lines

drivers/gpio/74x164_gpio.c new file +9 -0

+       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);

Quoted 25 lines

drivers/gpio/74x164_gpio.c new file +24 -0

+
+       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) = {

drivers/gpio/74x164_gpio.c line 218

Quoted 5 lines

drivers/gpio/74x164_gpio.c new file +5 -0

+       .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.

Quoted 8 lines

drivers/gpio/74x164_gpio.c new file +2 -0

+       .of_match       = gen_74x164_ids,
+};

drivers/gpio/Kconfig +0 -0

index 51658f1..43fc9c4 100644

drivers/gpio/Kconfig config PIC32_GPIO · line 103

Quoted 8 lines

drivers/gpio/Kconfig +5 -0

@@ -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.

Quoted 20 lines

drivers/gpio/Kconfig +3 -0

+         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

drivers/gpio/Makefile +1 -1

index bfc67d2..393b148 100644
@@ -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
--

Quoted 2 lines

2.6.2

Regards,
Simon

1. Support compatible string "spi-gpio" which is used by Linux Linux use different bindings, so use UBOOT_COMPAT and LINUX_COMPAT to differentiate them.

2. Bug fix
When doing xfer, should use device->parent, but not device When doing bit xfer, should use "!!(tmpdout & 0x80)", but not "(tmpdout & 0x80)"
3. Introduce SPI_MASTER_NO_RX and SPI_MASTER_NO_TX to handle no rx or no tx case.

4. Tested on i.MX6 UltraLite board with 74LV595 spi-gpio chip.

Signed-off-by Peng Fan <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jagan Teki <[email protected]>

Cc Stefano Babic <[email protected]>

---
 drivers/spi/soft_spi.c | 75 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 57 insertions(+), 18 deletions(-)

Hi Peng,

Quoted 3 lines · Peng Fan

1. Support compatible string "spi-gpio" which is used by Linux Linux use different bindings, so use UBOOT_COMPAT and LINUX_COMPAT to differentiate them.

There is only one user - instead of this, can you please drop the U-Boot binding and move the universal board over to use your new binding? There should be only one.

Quoted 7 lines

2. Bug fix
When doing xfer, should use device->parent, but not device When doing bit xfer, should use "!!(tmpdout & 0x80)", but not "(tmpdout & 0x80)"
3. Introduce SPI_MASTER_NO_RX and SPI_MASTER_NO_TX to handle no rx or no tx case.

Perhaps put these two in separate patches?

Quoted 10 lines

4. Tested on i.MX6 UltraLite board with 74LV595 spi-gpio chip.

Signed-off-by Peng Fan <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jagan Teki <[email protected]>

Cc Stefano Babic <[email protected]>

---
 drivers/spi/soft_spi.c | 75 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 57 insertions(+), 18 deletions(-)

Regards,
Simon