Hi
17.10.2017, 23:01, "Hans-Frieder Vogt" <hfv...@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()
