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

[Simon Glass]

Hi Maxime,

On 8 November 2016 at 03:06, Maxime Ripard
<maxime.ripard at free-electrons.com> wrote:
> Add a bus driver for bitbanging a 1-Wire bus over a GPIO.
>
> Signed-off-by: Maxime Ripard <maxime.ripard at 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 at 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