Re: [U-Boot] [PATCH 4/9] w1: Add 1-Wire gpio driver

Hi Maxime,

On 8 November 2016 at 03:06, Maxime Ripard maxime.ripard@free-electrons.com wrote:

Add a bus driver for bitbanging a 1-Wire bus over a GPIO.

Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com

drivers/w1/Kconfig | 6 ++- drivers/w1/Makefile | 1 +- drivers/w1/w1-gpio.c | 160 ++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 167 insertions(+), 0 deletions(-) create mode 100644 drivers/w1/w1-gpio.c

diff --git a/drivers/w1/Kconfig b/drivers/w1/Kconfig index 0c056b4c06a9..ccc3ae15db86 100644 --- a/drivers/w1/Kconfig +++ b/drivers/w1/Kconfig @@ -12,6 +12,12 @@ config W1

if W1

+config W1_GPIO

•   bool "Enable 1-Wire GPIO bitbanging"
  

•   depends on DM_GPIO
  

•   help
  

•     Emulate a 1-Wire bus using a GPIO.
  

Any more details? How many GPIOs? Any particular chips that are supported?

endif

endmenu diff --git a/drivers/w1/Makefile b/drivers/w1/Makefile index 26820fa209e1..7fd8697f8419 100644 --- a/drivers/w1/Makefile +++ b/drivers/w1/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_W1) += w1-uclass.o

+obj-$(CONFIG_W1_GPIO) += w1-gpio.o diff --git a/drivers/w1/w1-gpio.c b/drivers/w1/w1-gpio.c new file mode 100644 index 000000000000..091849162533 --- /dev/null +++ b/drivers/w1/w1-gpio.c @@ -0,0 +1,160 @@ +/*

• • Copyright (c) 2015 Free Electrons
• • Copyright (c) 2015 NextThing Co
• • Maxime Ripard maxime.ripard@free-electrons.com
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <dm.h> +#include <w1.h>

+#include <asm/gpio.h>

+#define W1_TIMING_A 6 +#define W1_TIMING_B 64 +#define W1_TIMING_C 60 +#define W1_TIMING_D 10 +#define W1_TIMING_E 9 +#define W1_TIMING_F 55 +#define W1_TIMING_G 0 +#define W1_TIMING_H 480 +#define W1_TIMING_I 70 +#define W1_TIMING_J 410

What do these mean? Perhaps add a _US suffix?

+struct w1_gpio_pdata {

•   struct gpio_desc        gpio;
  

•   u64                     search_id;
  

+};

+static bool w1_gpio_read_bit(struct udevice *dev) +{

•   struct w1_gpio_pdata *pdata = dev_get_platdata(dev);
  

•   int val;
  

•   dm_gpio_set_dir_flags(&pdata->gpio, GPIOD_IS_OUT);
  

Error checks here?

•   udelay(W1_TIMING_A);
  

•   dm_gpio_set_dir_flags(&pdata->gpio, GPIOD_IS_IN);
  

•   udelay(W1_TIMING_E);
  

•   val = dm_gpio_get_value(&pdata->gpio);
  

•   udelay(W1_TIMING_F);
  

•   return val;
  

+}

+static u8 w1_gpio_read_byte(struct udevice *dev) +{

•   int i;
  

•   u8 ret = 0;
  

•   for (i = 0; i < 8; ++i)
  

•           ret |= (w1_gpio_read_bit(dev) ? 1 : 0) << i;
  

•   return ret;
  

+}

+static void w1_gpio_write_bit(struct udevice *dev, bool bit) +{

•   struct w1_gpio_pdata *pdata = dev_get_platdata(dev);
  

•   dm_gpio_set_dir_flags(&pdata->gpio, GPIOD_IS_OUT);
  

•   bit ? udelay(W1_TIMING_A) : udelay(W1_TIMING_C);
  

Can you convert this and below to if/else?

•   dm_gpio_set_value(&pdata->gpio, 1);
  

•   bit ? udelay(W1_TIMING_B) : udelay(W1_TIMING_D);
  

+}

+static void w1_gpio_write_byte(struct udevice *dev, u8 byte) +{

•   int i;
  

•   for (i = 0; i < 8; ++i)
  

•           w1_gpio_write_bit(dev, (byte >> i) & 0x1);
  

+}

+static bool w1_gpio_reset(struct udevice *dev) +{

•   struct w1_gpio_pdata *pdata = dev_get_platdata(dev);
  

•   int val;
  

•   dm_gpio_set_dir_flags(&pdata->gpio, GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
  

•   udelay(W1_TIMING_G);
  

•   dm_gpio_set_value(&pdata->gpio, 0);
  

•   udelay(W1_TIMING_H);
  

•   dm_gpio_set_dir_flags(&pdata->gpio, GPIOD_IS_IN);
  

•   udelay(W1_TIMING_I);
  

•   val = dm_gpio_get_value(&pdata->gpio);
  

•   udelay(W1_TIMING_J);
  

•   return val;
  

+}

+static u8 w1_gpio_triplet(struct udevice *dev, bool bdir) +{

•   u8 id_bit   = w1_gpio_read_bit(dev);
  

•   u8 comp_bit = w1_gpio_read_bit(dev);
  

•   u8 retval;
  

•   if (id_bit && comp_bit)
  

•           return 0x03;  /* error */
  

•   if (!id_bit && !comp_bit) {
  

•           /* Both bits are valid, take the direction given */
  

•           retval = bdir ? 0x04 : 0;
  

•   } else {
  

•           /* Only one bit is valid, take that direction */
  

•           bdir = id_bit;
  

•           retval = id_bit ? 0x05 : 0x02;
  

Can you use enums / #defines for these magic values?

•   }
  

•   w1_gpio_write_bit(dev, bdir);
  

•   return retval;
  

+}

+static const struct w1_ops w1_gpio_ops = {

•   .read_byte      = w1_gpio_read_byte,
  

•   .reset          = w1_gpio_reset,
  

•   .triplet        = w1_gpio_triplet,
  

•   .write_byte     = w1_gpio_write_byte,
  

+};

+static int w1_gpio_ofdata_to_platdata(struct udevice *dev)

This should probably go in probe() since it actually probes the GPIOs.

+{

•   struct w1_gpio_pdata *pdata = dev_get_platdata(dev);
  

•   int ret;
  

•   ret = gpio_request_by_name(dev, "gpios", 0, &pdata->gpio, 0);
  

•   if (ret < 0)
  

•           goto error;
  

•   return 0;
  

+error:

•   printf("Error claiming GPIO %d\n", ret);
  

debug()?

•   return ret;
  

+};

+static const struct udevice_id w1_gpio_id[] = {

•   { "w1-gpio", 0 },
  

•   { },
  

+};

+U_BOOT_DRIVER(w1_gpio_drv) = {

•   .id                             = UCLASS_W1,
  

•   .name                           = "w1_gpio_drv",
  

•   .of_match                       = w1_gpio_id,
  

•   .ofdata_to_platdata             = w1_gpio_ofdata_to_platdata,
  

•   .ops                            = &w1_gpio_ops,
  

•   .platdata_auto_alloc_size       = sizeof(struct w1_gpio_pdata),
  

+};

git-series 0.8.11

Regards, SImon