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

[Evgeniy Polyakov <zbr@ioremap.net>]

Hi

17.10.2017, 23:01, "Hans-Frieder Vogt" <hfvogt@gmx.net>:
> attached is 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 patch is against linux-next. Please try and comment.

This looks good from w1 point of view except minor nits, but I know nothing about serio links, so it might worth asking appropriate parties

> +static int link_mode_ascii(void *data)

You always have link_data when calling this function, no need to dereference void pointer all the time

> +static int link_mode_hex(void *data)

Same here

> +static void link_w1_write_byte(void *data, u8 byte)
> +{
> + struct link_data *link = data;
> + struct serio *serio = link->serio;
> + int len;
> + unsigned long fl;
> +
> + /* make sure we are in hex mode */
> + link_mode_hex(data);
> +
> + spin_lock_irqsave(&link->lock, fl);
> + serio_write(serio, val2char[byte >> 4]);
> + serio_write(serio, val2char[byte & 0x0f]);
> + link->pos = 0;
> + link->state = LINK_STATE_BUSY;
> + link->expected = 2;
> + spin_unlock_irqrestore(&link->lock, fl);

This pattern (with 2 different control characters) repeats many times across the code, should it live in its own helper function?

> +static void link_w1_write_block(void *data, const u8 *buf, int len)
> +{
> + struct link_data *link = data;
> + struct serio *serio = link->serio;
> + int i, l;
> + const u8 *p = buf;
> + unsigned long fl;
> +
> + if (len <= 0)
> + return;
> + if (len*2 > BUFFER_SIZE)
> + return;
> +
> + /* make sure we are in hex mode */
> + link_mode_hex(data);
> +
> + spin_lock_irqsave(&link->lock, fl);
> + for (i=0; i<len; i++, p++) {
> + serio_write(serio, val2char[*p >> 4]);
> + serio_write(serio, val2char[*p & 0x0f]);
> + }
> + link->pos = 0;
> + link->state = LINK_STATE_BUSY;
> + link->expected = 2*len;
> + spin_unlock_irqrestore(&link->lock, fl);

Is this 100% overflow-free?
Since serio interrupt checks whether the number of consumed equals to expected field and writes 0 past the buffer...

> +static u8 link_w1_set_pullup(void *data, int delay)
> +{
> + struct link_data *link = data;
> + int result = 0;
> + unsigned long fl;
> +
> + printk(KERN_INFO "pullup: %d\n", delay);

Please drop this print

> +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_HEX:
> + if (link->pos >= link->expected) {
> + link->buffer[link->pos] = '\0';

I meant this write in my comment in link_w1_write_block()