Message-Id: <4482311512511009@web58g.yandex.ru>
Date:   Wed, 06 Dec 2017 00:56:49 +0300
From:   Evgeniy Polyakov <zbr@...emap.net>
To:     Hans-Frieder Vogt <hfvogt@....net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v2] w1: driver for serial linkUSB, linkOEM, LINK devices

Hi

Sorry for delay

13.11.2017, 00:05, "Hans-Frieder Vogt" <hfvogt@....net>:
> 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.
>
> 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 looks good to me, thank you!
Greg, please pull it into your tree

> Signed-off by: Hans-Frieder Vogt <hfvogt@....net>

Acked-by: Evgeniy Polyakov <zbr@...emap.net>

>  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(+)
>
> 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@....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 <hfvogt@....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;
> +}
> +
> +static int link_read(struct link_data *link)
> +{
> + wait_event_interruptible_timeout(wq, link->state == LINK_STATE_IDLE,
> + msecs_to_jiffies(TIMEOUT));
> +
> + if (link->state != LINK_STATE_IDLE) {
> + dev_err(&link->serio->dev, "%s: Timeout (pos=%d)!\n", __func__,
> + link->pos);
> + link->pos = 0;
> + return 0;
> + }
> + if (link->mode == MODE_ASCII) {
> + /* remove <CR><LF> */
> + if ((link->pos >= 2) && (link->buffer[link->pos-2] == 0x0d) &&
> + (link->buffer[link->pos-1] == 0x0a)) {
> + link->pos -= 2;
> + link->buffer[link->pos] = '\0';
> + }
> + }
> +
> + return link->pos;
> +}
> +
> +static void link_write_raw(struct link_data *link, u8 val)
> +{
> + struct serio *serio = link->serio;
> +
> + link->pos = 0;
> + link->state = LINK_STATE_BUSY;
> + link->mode = MODE_ASCII;
> + serio_write(serio, val);
> +}
> +
> +/* change to ASCII mode
> + * => send <CR>
> + * <= <CR><LF>
> + */
> +static int link_mode_ascii(struct link_data *link)
> +{
> + int len;
> +
> + if (link->mode != MODE_ASCII) {
> + /* back to ASCII mode */
> + link_write_raw(link, 0x0d); /* cr */
> + /* ignore return value */
> + len = link_read(link);
> + }
> + return 0;
> +}
> +
> +/* change to BYTE mode
> + * => send 'p' or 'b'
> + * <= none
> + */
> +static int link_mode_byte(struct link_data *link)
> +{
> + struct serio *serio = link->serio;
> +
> + if (link->mode != MODE_BYTE) {
> + link->mode = MODE_BYTE;
> + /* change to byte mode */
> + if (link->pull_up)
> + serio_write(serio, 'p');
> + else
> + serio_write(serio, 'b');
> + }
> + return 0;
> +}
> +
> +static void link_write_hex_bytes(struct link_data *link, const u8 *buf, int len)
> +{
> + struct serio *serio = link->serio;
> + const u8 *p = buf;
> + int i;
> +
> + /* make sure we are in hex mode */
> + link_mode_byte(link);
> +
> + link->pos = 0;
> + link->state = LINK_STATE_BUSY;
> + link->expected = 2*len;
> + for (i = 0; i < len; i++, p++) {
> + serio_write(serio, val2char[*p >> 4]);
> + serio_write(serio, val2char[*p & 0x0f]);
> + }
> +}
> +
> +static void link_w1_write_byte(void *data, u8 byte)
> +{
> + struct link_data *link = data;
> + struct serio *serio = link->serio;
> + int len, err;
> +
> + err = mutex_lock_interruptible(&link_mutex);
> + if (err)
> + return;
> + link_write_hex_bytes(link, &byte, 1);
> +
> + /* get back byte as confirmation */
> + len = link_read(link);
> + if (len != 2)
> + dev_err(&serio->dev, "%s: Read %d bytes, expected 2!\n",
> + __func__, len);
> + mutex_unlock(&link_mutex);
> +}
> +
> +static u8 link_w1_read_byte(void *data)
> +{
> + struct link_data *link = data;
> + struct serio *serio = link->serio;
> + const u8 byte = 0xff;
> + u8 ret;
> + int len, err;
> +
> + err = mutex_lock_interruptible(&link_mutex);
> + if (err)
> + return 0xff;
> + link_write_hex_bytes(link, &byte, 1);
> +
> + len = link_read(link);
> + if (len == 2)
> + ret = (conv_hex(link->buffer[0]) << 4) +
> + conv_hex(link->buffer[1]);
> + else {
> + dev_err(&serio->dev, "%s: Read %d bytes, expected 2!\n",
> + __func__, len);
> + ret = 0xff;
> + }
> + mutex_unlock(&link_mutex);
> +
> + return ret;
> +}
> +
> +static void link_w1_write_block(void *data, const u8 *buf, int len)
> +{
> + struct link_data *link = data;
> + struct serio *serio = link->serio;
> + int l, err;
> +
> + if (len <= 0)
> + return;
> + if (len*2 > BUFFER_SIZE)
> + return;
> +
> + err = mutex_lock_interruptible(&link_mutex);
> + if (err)
> + return;
> + link_write_hex_bytes(link, buf, len);
> +
> + l = link_read(link);
> + if (l != 2*len)
> + dev_err(&serio->dev, "%s: Read %d bytes, expected %d!\n",
> + __func__, l, 2*len);
> + mutex_unlock(&link_mutex);
> +}
> +
> +static u8 link_w1_read_block(void *data, u8 *buf, int len)
> +{
> + struct link_data *link = data;
> + struct serio *serio = link->serio;
> + u8 ret;
> + int l, i, err;
> + u8 *wbuf;
> + u8 *p = buf;
> +
> + if (len <= 0)
> + return 0;
> + if (len*2 > BUFFER_SIZE)
> + return -E2BIG;
> +
> + wbuf = kzalloc(len, GFP_KERNEL);
> + if (!wbuf)
> + return -ENOMEM;
> + memset(wbuf, 0xff, len);
> +
> + err = mutex_lock_interruptible(&link_mutex);
> + if (err)
> + return 0;
> + link_write_hex_bytes(link, wbuf, len);
> +
> + l = link_read(link);
> + if (l == 2*len) {
> + for (i = 0; i < l; i += 2, p++) {
> + *p = (conv_hex(link->buffer[i]) << 4) +
> + conv_hex(link->buffer[i+1]);
> + }
> + ret = len;
> + } else {
> + dev_err(&serio->dev, "%s: Read %d bytes, expected %d!\n",
> + __func__, l, len);
> + ret = 0;
> + }
> + mutex_unlock(&link_mutex);
> +
> + kfree(wbuf);
> + return ret;
> +}
> +
> +static void link_w1_search(void *data, struct w1_master *master,
> + u8 search_type, w1_slave_found_callback callback)
> +{
> + struct link_data *link = data;
> + struct serio *serio = link->serio;
> + int len, err;
> + u64 id, idle;
> +
> + err = mutex_lock_interruptible(&link_mutex);
> + if (err)
> + return;
> + link_mode_ascii(link);
> +
> + link->pos = 0;
> + link->state = LINK_STATE_BUSY;
> + serio_write(serio, 'f');
> + while (1) {
> + len = link_read(link);
> + if (len != 18) {
> + dev_err(&serio->dev, "%s: Read %d bytes, expected 18!\n",
> + __func__, len);
> + break;
> + }
> + /* buffer should contain now something like
> + * "+,1234567890123456"
> + */
> + err = kstrtou64(&link->buffer[2], 16, &idle);
> + if (err)
> + break;
> + id = le64_to_cpu(idle);
> + callback(master, id);
> + if (link->buffer[0] == '+') {
> + link->pos = 0;
> + link->state = LINK_STATE_BUSY;
> + serio_write(serio, 'n');
> + } else
> + break;
> + }
> +
> + master->search_id = 0;
> + mutex_unlock(&link_mutex);
> +}
> +
> +static u8 link_w1_reset_bus(void *data)
> +{
> + struct link_data *link = data;
> + struct serio *serio = link->serio;
> + u8 ret = 0, len, status;
> + int err;
> +
> + err = mutex_lock_interruptible(&link_mutex);
> + if (err)
> + return 0xff;
> + link_mode_ascii(link);
> + link_write_raw(link, 'r');
> +
> + len = link_read(link);
> + if (len <= 0) {
> + dev_err(&serio->dev, "%s: Zero read!\n", __func__);
> + goto common_out;
> + }
> + if (len > 1)
> + dev_err(&serio->dev, "%s: Read %d bytes, expected 1!\n",
> + __func__, len);
> + status = link->buffer[0];
> + switch (status) {
> + case 'P':
> + break;
> + case 'N':
> + dev_warn(&serio->dev, "%s: No devices on bus\n", __func__);
> + ret = 1;
> + break;
> + case 'S':
> + dev_err(&serio->dev, "%s: Shortened bus!\n", __func__);
> + ret = 0xff;
> + break;
> + default:
> + dev_err(&serio->dev, "%s: Unknown link response '%c'(0x%02x)!\n",
> + __func__, status, status);
> + ret = 0xff;
> + }
> +
> +common_out:
> + mutex_unlock(&link_mutex);
> + return ret;
> +}
> +
> +static u8 link_w1_set_pullup(void *data, int delay)
> +{
> + struct link_data *link = data;
> + int err;
> +
> + err = mutex_lock_interruptible(&link_mutex);
> + if (err)
> + return 0xff;
> + link->pull_up = delay;
> + mutex_unlock(&link_mutex);
> +
> + return 0;
> +}
> +
> +static void linkserial_disconnect(struct serio *serio)
> +{
> + struct link_data *link = serio_get_drvdata(serio);
> +
> + w1_remove_master_device(&link->master);
> + serio_close(serio);
> + kfree(link);
> +
> + dev_info(&serio->dev, "Disconnected from device\n");
> +}
> +
> +static int link_version(struct link_data *link)
> +{
> + struct serio *serio = link->serio;
> + int len, err;
> +
> + /* version command */
> + link_write_raw(link, ' ');
> +
> + len = link_read(link);
> + if (len >= MAX_LINK_VERSION_LENGTH) {
> + err = -ENODEV;
> + dev_err(&serio->dev, "%s: Too long version string read (i=%d)\n",
> + __func__, len);
> + goto err_close;
> + }
> + if (strncmp(link->buffer, "LINK v1", 7) &&
> + strncmp(link->buffer, "LINK VM1", 8) &&
> + strncmp(link->buffer, "LinkUSB V1", 10)) {
> + err = -ENODEV;
> + dev_err(&serio->dev, "Unknown version of adapter \"%s\"\n",
> + link->buffer);
> + goto err_close;
> + }
> + dev_info(&serio->dev, "Version: \"%s\" (%d bytes)\n", link->buffer,
> + len);
> +
> + return 0;
> +
> +err_close:
> + return err;
> +}
> +
> +static int link_init(struct link_data *link)
> +{
> + struct serio *serio = link->serio;
> + int len;
> +
> + dev_info(&serio->dev, "%s: Ignore pot. following timeout message\n",
> + __func__);
> +
> + /* send break, reset with 9600 baud */
> + link_write_raw(link, 0);
> +
> + /* slurp in initial bytes */
> + len = link_read(link);
> + if (!len)
> + return -1;
> + else
> + return 0;
> +}
> +
> +static irqreturn_t linkserial_interrupt(struct serio *serio, unsigned char data,
> + unsigned int flags)
> +{
> + struct link_data *link = serio_get_drvdata(serio);
> +
> + link->buffer[link->pos++] = data;
> +
> + switch (link->mode) {
> + case MODE_BYTE:
> + if (link->pos >= link->expected) {
> + link->buffer[link->pos] = '\0';
> + link->state = LINK_STATE_IDLE;
> + wake_up_interruptible(&wq);
> + }
> + break;
> + case MODE_ASCII:
> + if ((data == 0x0a) || (link->pos == BUFFER_SIZE-1)) {
> + link->buffer[link->pos] = '\0';
> + link->state = LINK_STATE_IDLE;
> + wake_up_interruptible(&wq);
> + }
> + break;
> + default:
> + break;
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int linkserial_connect(struct serio *serio, struct serio_driver *drv)
> +{
> + struct link_data *link;
> + int err;
> +
> + link = kzalloc(sizeof(struct link_data), GFP_KERNEL);
> + if (!link) {
> + err = -ENOMEM;
> + goto exit;
> + }
> + serio_set_drvdata(serio, link);
> +
> + err = serio_open(serio, drv);
> + if (err)
> + goto exit_free;
> +
> + link->serio = serio;
> +
> + err = link_init(link);
> + usleep_range(10000, 50000);
> + err = link_version(link);
> + if (err)
> + goto exit_close;
> +
> + link->master.data = link;
> + link->master.read_byte = link_w1_read_byte;
> + link->master.write_byte = link_w1_write_byte;
> + link->master.read_block = link_w1_read_block;
> + link->master.write_block = link_w1_write_block;
> + link->master.reset_bus = link_w1_reset_bus;
> + link->master.set_pullup = link_w1_set_pullup;
> + link->master.search = link_w1_search;
> +
> + err = w1_add_master_device(&link->master);
> + if (err)
> + goto exit_close;
> +
> + dev_info(&serio->dev, "Connected to device\n");
> + return 0;
> +
> +exit_close:
> + serio_close(serio);
> +exit_free:
> + kfree(link);
> +exit:
> + return err;
> +}
> +
> +static struct serio_device_id linkserial_serio_ids[] = {
> + {
> + .type = SERIO_RS232,
> + .proto = SERIO_ILINK,
> + .id = SERIO_ANY,
> + .extra = SERIO_ANY,
> + },
> + { 0 }
> +};
> +MODULE_DEVICE_TABLE(serio, linkserial_serio_ids);
> +
> +static struct serio_driver linkserial_drv = {
> + .driver = {
> + .name = "linkserial",
> + },
> + .description = "W1 bus master driver for iButtonLink(TM) devices",
> + .id_table = linkserial_serio_ids,
> + .connect = linkserial_connect,
> + .disconnect = linkserial_disconnect,
> + .interrupt = linkserial_interrupt,
> +};
> +
> +module_serio_driver(linkserial_drv);
> +
> +MODULE_AUTHOR("Hans-Frieder Vogt <hfvogt@....net>");
> +MODULE_DESCRIPTION("W1 bus master driver for iButtonLink(TM) devices");
> +MODULE_VERSION(DRV_VERSION);
> +MODULE_LICENSE("GPL");
> +
> 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
>
>  #endif /* _UAPI_SERIO_H */

Powered by blists - more mailing lists