* [U-Boot] [PATCH 3/9] w1: Add 1-Wire uclass
[not found] ` <de0f795a85e3a17f639ed2d23dd8b9936bc0b332.1478599446.git-series.maxime.ripard@free-electrons.com>
@ 2016-11-11 16:17 ` Simon Glass
0 siblings, 0 replies; 4+ messages in thread
From: Simon Glass @ 2016-11-11 16:17 UTC (permalink / raw)
To: u-boot
Hi Maxime,
On 8 November 2016 at 03:06, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> We might want to use 1-Wire devices connected on boards such as EEPROMs in
> U-Boot.
>
> Provide a framework to be able to do that.
Also please can you add a sandbox driver and a test for this uclass.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
> drivers/Kconfig | 2 +-
> drivers/Makefile | 1 +-
> drivers/w1/Kconfig | 17 +++-
> drivers/w1/Makefile | 2 +-
> drivers/w1/w1-uclass.c | 259 ++++++++++++++++++++++++++++++++++++++++++-
> include/dm/uclass-id.h | 1 +-
> include/w1.h | 47 ++++++++-
> 7 files changed, 329 insertions(+), 0 deletions(-)
> create mode 100644 drivers/w1/Kconfig
> create mode 100644 drivers/w1/Makefile
> create mode 100644 drivers/w1/w1-uclass.c
> create mode 100644 include/w1.h
>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index e8c9e0a32626..74194b0d6f7f 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -84,6 +84,8 @@ source "drivers/usb/Kconfig"
>
> source "drivers/video/Kconfig"
>
> +source "drivers/w1/Kconfig"
> +
> source "drivers/watchdog/Kconfig"
>
> config PHYS_TO_BUS
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 761d0b3e85b4..543c43bb0d23 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -91,6 +91,7 @@ obj-y += input/
> obj-y += soc/
> obj-$(CONFIG_REMOTEPROC) += remoteproc/
> obj-y += thermal/
> +obj-y += w1/
>
> obj-$(CONFIG_MACH_PIC32) += ddr/microchip/
> endif
> diff --git a/drivers/w1/Kconfig b/drivers/w1/Kconfig
> new file mode 100644
> index 000000000000..0c056b4c06a9
> --- /dev/null
> +++ b/drivers/w1/Kconfig
> @@ -0,0 +1,17 @@
> +#
> +# W1 subsystem configuration
> +#
> +
> +menu "1-Wire support"
> +
> +config W1
> + bool "Enable 1-Wire controllers support"
> + depends on DM
> + help
> + Support for the Dallas 1-Wire bus.
Can you add more explanation of what this is and what support is provided?
> +
> +if W1
> +
> +endif
> +
> +endmenu
> diff --git a/drivers/w1/Makefile b/drivers/w1/Makefile
> new file mode 100644
> index 000000000000..26820fa209e1
> --- /dev/null
> +++ b/drivers/w1/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_W1) += w1-uclass.o
> +
> diff --git a/drivers/w1/w1-uclass.c b/drivers/w1/w1-uclass.c
> new file mode 100644
> index 000000000000..f9224746806c
> --- /dev/null
> +++ b/drivers/w1/w1-uclass.c
> @@ -0,0 +1,259 @@
> +/*
> + * 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 <dm/device-internal.h>
> +
> +#define W1_MATCH_ROM 0x55
> +#define W1_SKIP_ROM 0xcc
> +#define W1_SEARCH 0xf0
> +
> +struct w1_bus {
> + u64 search_id;
> +};
> +
> +struct w1_device {
> + u64 id;
> +};
> +
> +static int w1_new_device(struct udevice *bus, u64 id)
Please add function comment - i.e. what this does, args.
> +{
> + struct w1_driver_entry *start, *entry;
> + int n_ents, ret = -ENODEV;
> + u8 family = id & 0xff;
> +
> + debug("%s: Detected new device 0x%llx (family 0x%x)\n",
> + bus->name, id, family);
> +
> + start = ll_entry_start(struct w1_driver_entry, w1_driver_entry);
> + n_ents = ll_entry_count(struct w1_driver_entry, w1_driver_entry);
> +
> + for (entry = start; entry != start + n_ents; entry++) {
> + const struct driver *drv;
> + struct w1_device *w1;
> + struct udevice *dev;
> +
> + if (entry->family != family)
> + continue;
> +
> + drv = entry->driver;
> + ret = device_bind(bus, drv, drv->name, NULL, -1,
> + &dev);
> + if (ret)
> + goto error;
> +
> + debug("%s: Match found: %s\n", __func__, drv->name);
> +
> + w1 = dev_get_parent_platdata(dev);
> + w1->id = id;
> +
> + return 0;
> + }
> +
> +error:
> + debug("%s: No matches found: error %d\n", __func__, ret);
> + return ret;
> +}
> +
> +static int w1_enumerate(struct udevice *bus)
Please add function comment - i.e. what this does, args.
> +{
> + const struct w1_ops *ops = device_get_ops(bus);
> + struct w1_bus *w1 = dev_get_uclass_priv(bus);
> + u64 last_rn, rn = w1->search_id, tmp64;
> + bool last_device = false;
> + int search_bit, desc_bit = 64;
> + int last_zero = -1;
> + u8 triplet_ret = 0;
> + int i;
> +
> + if (!ops->reset || !ops->write_byte || !ops->triplet)
> + return -ENOSYS;
> +
> + while ( !last_device ) {
> + last_rn = rn;
> + rn = 0;
> +
> + /*
> + * Reset bus and all 1-wire device state machines
> + * so they can respond to our requests.
> + *
> + * Return 0 - device(s) present, 1 - no devices present.
> + */
> + if (ops->reset(bus)) {
> + debug("%s: No devices present on the wire.\n",
> + __func__);
> + break;
> + }
> +
> + /* Start the search */
> + ops->write_byte(bus, W1_SEARCH);
> + for (i = 0; i < 64; ++i) {
> + /* Determine the direction/search bit */
> + if (i == desc_bit)
> + search_bit = 1; /* took the 0 path last time, so take the 1 path */
> + else if (i > desc_bit)
> + search_bit = 0; /* take the 0 path on the next branch */
> + else
> + search_bit = ((last_rn >> i) & 0x1);
> +
> + /* Read two bits and write one bit */
> + triplet_ret = ops->triplet(bus, search_bit);
> +
> + /* quit if no device responded */
> + if ( (triplet_ret & 0x03) == 0x03 )
No space before bracket. Also what is 3? Can this and the mask be an
enum / #define?
> + break;
> +
> + /* If both directions were valid, and we took the 0 path... */
> + if (triplet_ret == 0)
> + last_zero = i;
> +
> + /* extract the direction taken & update the device number */
> + tmp64 = (triplet_ret >> 2);
> + rn |= (tmp64 << i);
> + }
> +
> + if ( (triplet_ret & 0x03) != 0x03 ) {
> + if ((desc_bit == last_zero) || (last_zero < 0)) {
> + last_device = 1;
> + w1->search_id = 0;
> + } else {
> + w1->search_id = rn;
> + }
> + desc_bit = last_zero;
> +
> + w1_new_device(bus, rn);
Check error?
> + }
> + }
> +
> + return 0;
> +}
> +
> +int w1_get_bus(int busnum, struct udevice **busp)
> +{
> + int ret;
> +
> + ret = uclass_get_device_by_seq(UCLASS_W1, busnum, busp);
> + if (ret) {
> + debug("Cannot find w1 bus %d\n", busnum);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +u8 w1_get_device_family(struct udevice *dev)
> +{
> + struct w1_device *w1 = dev_get_parent_platdata(dev);
> +
> + return w1->id & 0xff;
Probably need a #define for the 0xff. W1_FAMILY_MASK?
> +}
> +
> +int w1_reset_select(struct udevice *dev)
> +{
> + struct w1_device *w1 = dev_get_parent_platdata(dev);
> + struct udevice *bus = dev_get_parent(dev);
> + const struct w1_ops *ops = device_get_ops(bus);
> + int i;
> +
> + if (!ops->reset || !ops->write_byte)
> + return -ENOSYS;
> +
> + ops->reset(bus);
Error check?
> +
> + ops->write_byte(bus, W1_MATCH_ROM);
Error check?
> +
> + for (i = 0; i < sizeof(w1->id); i++)
> + ops->write_byte(bus, (w1->id >> (i * 8)) & 0xff);
Error check? And below.
> +
> + return 0;
> +}
> +
> +int w1_read_byte(struct udevice *dev)
> +{
> + struct udevice *bus = dev_get_parent(dev);
> + const struct w1_ops *ops = device_get_ops(bus);
> +
> + if (!ops->read_byte)
> + return -ENOSYS;
> +
> + return ops->read_byte(bus);
> +}
> +
> +int w1_read_buf(struct udevice *dev, u8 *buf, unsigned count)
> +{
> + int i;
> +
> + for (i = 0; i < count; i++) {
> + int ret = w1_read_byte(dev);
> + if (ret < 0)
> + return ret;
> +
> + buf[i] = ret & 0xff;
> + }
> +
> + return 0;
> +}
> +
> +int w1_write_byte(struct udevice *dev, u8 byte)
> +{
> + struct udevice *bus = dev_get_parent(dev);
> + const struct w1_ops *ops = device_get_ops(bus);
> +
> + if (!ops->write_byte)
> + return -ENOSYS;
> +
> + ops->write_byte(bus, byte);
> +
> + return 0;
> +}
> +
> +static int w1_post_probe(struct udevice *bus)
> +{
> + w1_enumerate(bus);
> +
> + return 0;
> +}
> +
> +UCLASS_DRIVER(w1) = {
> + .name = "w1",
> + .id = UCLASS_W1,
> + .flags = DM_UC_FLAG_SEQ_ALIAS,
> + .per_device_auto_alloc_size = sizeof(struct w1_bus),
> + .per_child_platdata_auto_alloc_size = sizeof(struct w1_device),
> + .post_probe = w1_post_probe,
> +};
> +
> +int w1_init(void)
Why do we need this? Is it because we cannot discover the devices dynamically?
Also, can we use device tree to specify the devices? At present it
seems to only support probing.
> +{
> + struct udevice *bus;
> + struct uclass *uc;
> + int ret;
> +
> + ret = uclass_get(UCLASS_W1, &uc);
> + if (ret)
> + return ret;
> +
> + uclass_foreach_dev(bus, uc) {
> + ret = device_probe(bus);
> + if (ret == -ENODEV) { /* No such device. */
> + printf("W1 controller not available.\n");
> + continue;
> + }
> +
> + if (ret) { /* Other error. */
> + printf("W1 controller probe failed, error %d\n", ret);
> + continue;
> + }
> + }
> +
> + return 0;
> +}
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index eb78c4dac485..b88adcbe802f 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -82,6 +82,7 @@ enum uclass_id {
> UCLASS_VIDEO, /* Video or LCD device */
> UCLASS_VIDEO_BRIDGE, /* Video bridge, e.g. DisplayPort to LVDS */
> UCLASS_VIDEO_CONSOLE, /* Text console driver for video device */
> + UCLASS_W1, /* Dallas 1-Wire bus */
>
> UCLASS_COUNT,
> UCLASS_INVALID = -1,
> diff --git a/include/w1.h b/include/w1.h
> new file mode 100644
> index 000000000000..d03509bedbff
> --- /dev/null
> +++ b/include/w1.h
> @@ -0,0 +1,47 @@
> +/*
> + * Copyright (c) 2015 Free Electrons
> + * Copyright (c) 2015 NextThing Co
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#ifndef __W1_H
> +#define __W1_H
> +
> +#include <dm.h>
> +
> +#define W1_FAMILY_DS2431 0x2d
> +
> +/**
> + * struct w1_driver_entry - Matches a driver to its w1 family
> + * @driver: Driver to use
> + * @family: W1 family handled by this driver
> + */
> +struct w1_driver_entry {
> + struct driver *driver;
> + u8 family;
> +};
> +
> +#define U_BOOT_W1_DEVICE(__name, __family) \
> + ll_entry_declare(struct w1_driver_entry, __name, w1_driver_entry) = { \
> + .driver = llsym(struct driver, __name, driver), \
> + .family = __family, \
> + }
> +
> +struct w1_ops {
Please can you add function comments here and below.
> + u8 (*read_byte)(struct udevice *dev);
Please don't use u8 as an arg or return type. It adds masking and can
bloat the code. int or uint is fine.
> + bool (*reset)(struct udevice *dev);
> + u8 (*triplet)(struct udevice *dev, bool bdir);
What does this do?
> + void (*write_byte)(struct udevice *dev, u8 byte);
> +};
> +
> +int w1_get_bus(int busnum, struct udevice **busp);
> +u8 w1_get_device_family(struct udevice *dev);
> +
> +int w1_read_buf(struct udevice *dev, u8 *buf, unsigned count);
> +int w1_read_byte(struct udevice *dev);
> +int w1_reset_select(struct udevice *dev);
> +int w1_write_buf(struct udevice *dev, u8 *buf, unsigned count);
> +int w1_write_byte(struct udevice *dev, u8 byte);
> +
> +#endif
> --
> git-series 0.8.11
Regards,
Simon
^ permalink raw reply [flat|nested] 4+ messages in thread