Re: [PATCH v2] w1: driver for serial linkUSB, linkOEM, LINK devices

From: Greg KH
Date: Wed Dec 06 2017 - 04:24:57 EST

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 <hfvogt@xxxxxxx>

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 <hfvogt@xxxxxxx>
> +
> +
> +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 <hfvogt@xxxxxxx>
> + * 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