On Sun, Nov 12, 2017 at 10:04:47PM +0100, Hans-Frieder Vogt wrote:
> Hi,
>
> attached is the revised version of a driver that supports the serial OneWire
> masters from iButtonLink(TM) in the w1 kernel driver. In order to be usable
> it needs an updated userland tool (patch included in documentation file). The
> kernel patch is against linux-next. Please try and comment.
>
Please do not put this in the changelog text, look at how all of the
other kernel commits are done.
> v2 of the patch set changes the following:
> - based on review input from Evgeniy (thanks!), introduced a helper function
> to re-use more code and generally improved the style of the code (couldn't
> get rid of all problems checkpatch identified, but many of them)
> - introduced a version check to make sure the hardware belongs to the
> supported devices
> - removed a dependency on little endian system
> - further testing revealed re-entry problems. The protection of critical code
> areas has therefore been changed from local spinlocks to a more toplevel
> mutex-based system to enforce serialisation. I have not run into any problems
> with unexpected feedback from the link device since.
This goes below the --- line.
>
> Signed-off by: Hans-Frieder Vogt <hfv...@gmx.net>
This needs to be fixed.
>
> Documentation/w1/masters/00-INDEX | 2
> Documentation/w1/masters/linkserial | 44 ++
> drivers/w1/masters/Kconfig | 10
> drivers/w1/masters/Makefile | 1
> drivers/w1/masters/linkserial.c | 538
> ++++++++++++++++++++++++++++++++++++
> include/uapi/linux/serio.h | 1
> 6 files changed, 596 insertions(+)
You need a --- line above this, did you hand-edit the patch?
>
> diff --git a/Documentation/w1/masters/00-INDEX
> b/Documentation/w1/masters/00-INDEX
> index 8330cf9325f0..33c522854126 100644
> --- a/Documentation/w1/masters/00-INDEX
> +++ b/Documentation/w1/masters/00-INDEX
> @@ -4,6 +4,8 @@ ds2482
> - The Maxim/Dallas Semiconductor DS2482 provides 1-wire busses.
> ds2490
> - The Maxim/Dallas Semiconductor DS2490 builds USB <-> W1 bridges.
> +linkserial
> + - Serial to W1 bridge provided by iButtonLink devices.
> mxc-w1
> - W1 master controller driver found on Freescale MX2/MX3 SoCs
> omap-hdq
> diff --git a/Documentation/w1/masters/linkserial
> b/Documentation/w1/masters/linkserial
> new file mode 100644
> index 000000000000..d8b0a209ce4a
> --- /dev/null
> +++ b/Documentation/w1/masters/linkserial
> @@ -0,0 +1,44 @@
> +Kernel driver linkserial
> +========================
> +
> +Supported devices
> + * LinkUSB(TM)
> + * LinkOEM(TM)
> + * LINK(TM) untested
> +
> +Author: Hans-Frieder Vogt <hfv...@gmx.net>
> +
> +
> +Description
> +-----------
> +
> +1-wire bus master driver for iButtonLink LLC devices. These devices can
> operate
> +in 2 modes: DS9097U Emulation and ASCII mode. The driver linkserial uses the
> +ASCII command interface only.
> +
> +This driver needs an adapted userspace program inputattach to be used. The
> +following patch modifies inputattach as needed:
> +
> +--- a/inputattach.c 2016-08-09 13:04:05.000000000 +0200
> ++++ b/inputattach.c 2017-10-14 23:49:08.594165130 +0200
> +@@ -49,6 +49,9 @@
> + #ifdef SYSTEMD_SUPPORT
> + #include <systemd/sd-daemon.h>
> + #endif
> ++#ifndef SERIO_ILINK
> ++#define SERIO_ILINK 0x42
> ++#endif
> +
> + static int readchar(int fd, unsigned char *c, int timeout)
> + {
> +@@ -867,6 +870,9 @@ static struct input_types input_types[]
> + { "--pulse8-cec", "-pulse8-cec", "Pulse Eight HDMI CEC
> dongle",
> + B9600, CS8,
> + SERIO_PULSE8_CEC, 0x00, 0x00, 0, NULL },
> ++{ "--linkserial", "-linkserial", "Link Onewire master",
> ++ B9600, CS8,
> ++ SERIO_ILINK, 0x00, 0x00, 0, NULL },
> + { NULL, NULL, NULL, 0, 0, 0, 0, 0, 0, NULL }
> + };
> +
> +
> diff --git a/drivers/w1/masters/Kconfig b/drivers/w1/masters/Kconfig
> index 1708b2300c7a..eba57c2f0751 100644
> --- a/drivers/w1/masters/Kconfig
> +++ b/drivers/w1/masters/Kconfig
> @@ -34,6 +34,16 @@ config W1_MASTER_DS2482
> This driver can also be built as a module. If so, the module
> will be called ds2482.
>
> +config W1_MASTER_ILINK
> + tristate "iButtonLink(TM) serial to 1-Wire bridge"
> + depends on SERIO
> + help
> + Say Y here to get support for the iButtonLink(TM) serial to 1-Wire
> + bridges like the LINK(TM), the LinkUSB(TM) and the LinkOEM(TM).
> +
> + This driver can also be built as a module. If so, the module
> + will be called linkserial.
> +
> config W1_MASTER_MXC
> tristate "Freescale MXC 1-wire busmaster"
> depends on ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/w1/masters/Makefile b/drivers/w1/masters/Makefile
> index 18954cae4256..3fe656187c17 100644
> --- a/drivers/w1/masters/Makefile
> +++ b/drivers/w1/masters/Makefile
> @@ -6,6 +6,7 @@
> obj-$(CONFIG_W1_MASTER_MATROX) += matrox_w1.o
> obj-$(CONFIG_W1_MASTER_DS2490) += ds2490.o
> obj-$(CONFIG_W1_MASTER_DS2482) += ds2482.o
> +obj-$(CONFIG_W1_MASTER_ILINK) += linkserial.o
> obj-$(CONFIG_W1_MASTER_MXC) += mxc_w1.o
>
> obj-$(CONFIG_W1_MASTER_DS1WM) += ds1wm.o
> diff --git a/drivers/w1/masters/linkserial.c b/drivers/w1/masters/linkserial.c
> new file mode 100644
> index 000000000000..69aceacda6c2
> --- /dev/null
> +++ b/drivers/w1/masters/linkserial.c
> @@ -0,0 +1,538 @@
> +/*
> + * driver for W1 master based on iButtonLink(TM) Link devices
> + *
> + * Copyright (C) 2017 Hans-Frieder Vogt <hfv...@gmx.net>
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/serio.h>
> +#include <linux/string.h>
> +#include <linux/mutex.h>
> +
> +#include <linux/w1.h>
> +
> +#define BUFFER_SIZE 133
> +
> +#define LINK_STATE_IDLE 0
> +#define LINK_STATE_BUSY -1
> +
> +#define MODE_ASCII 0
> +#define MODE_BYTE 1
> +
> +#define DRV_VERSION "0.1"
> +#define MAX_LINK_VERSION_LENGTH 36
> +
> +#define TIMEOUT 100
> +
> +static DECLARE_WAIT_QUEUE_HEAD(wq);
> +static DEFINE_MUTEX(link_mutex);
> +
> +struct link_data {
> + struct serio *serio;
> + struct w1_bus_master master;
> +
> + int state;
> + int mode;
> + unsigned char buffer[BUFFER_SIZE];
> + unsigned int pos;
> + int pull_up;
> + int expected;
> +};
> +
> +static char val2char[] = {'0', '1', '2', '3', '4', '5', '6', '7',
> + '8', '9', 'A', 'B', 'C', 'D', 'E', 'F'};
> +
> +static u8 char2val[] = {/* 0x30 */ 0, 1, 2, 3, 4, 5, 6, 7,
> + /* 0x38 */ 8, 9, 0x0f, 0x0f, 0x0f, 0x0f, 0x0f, 0x0f,
> + /* 0x40 */ 0x0f, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f};
> +
> +static inline int conv_hex(char c)
> +{
> + int ret;
> +
> + if ((c >= 0x30) && (c <= 0x46))
> + ret = char2val[c-0x30];
> + else
> + ret = 0x0f;
> + return ret;
> +}
What's wrong with the in-kernel functions that do this type of thing?
> + dev_info(&serio->dev, "Connected to device\n");
Driver should be "quiet" if all is fine, please do not be chatty.
> diff --git a/include/uapi/linux/serio.h b/include/uapi/linux/serio.h
> index a0cac1d8670d..14482bf6d789 100644
> --- a/include/uapi/linux/serio.h
> +++ b/include/uapi/linux/serio.h
> @@ -82,5 +82,6 @@
> #define SERIO_EGALAX 0x3f
> #define SERIO_PULSE8_CEC 0x40
> #define SERIO_RAINSHADOW_CEC 0x41
> +#define SERIO_ILINK 0x42
Does this really need to be serio? What about using the serial bus
interface instead?
thanks,
greg k-h
