Re: [PATCH v1 1/5] siox: new driver framework for eckelmann SIOX

From: Greg Kroah-Hartman
Date: Mon Dec 18 2017 - 10:06:51 EST


On Thu, Dec 07, 2017 at 10:30:04AM +0100, Uwe Kleine-König wrote:
> SIOX is a bus system invented at Eckelmann AG to control their building
> management and refrigeration systems. Traditionally the bus was
> implemented on custom microcontrollers, today Linux based machines are
> in use, too.
>
> The topology on a SIOX bus looks as follows:
>
> ,------->--DCLK-->---------------+----------------------.
> ^ v v
> ,--------. ,----------------------. ,------
> | | | ,--------------. | |
> | |--->--DOUT-->---|->-|shift register|->-|--->---|
> | | | `--------------' | |
> | master | | device | | device
> | | | ,--------------. | |
> | |---<--DIN---<---|-<-|shift register|-<-|---<---|
> | | | `--------------' | |
> `--------' `----------------------' `------
> v ^ ^
> `----------DLD-------------------+----------------------'
>
> There are two control lines (DCLK and DLD) driven from the bus master to
> all devices in parallel and two daisy chained data lines, one for input
> and one for output. DCLK is the clock to shift both chains by a single
> bit. On an edge of DLD the devices latch both their input and output
> shift registers.
>
> This patch adds a framework for this bus type.

Looks very nice, great job!

Only one minor comment, and you can just send a follow-up patch for it:

> + /* prepare data pushed out to devices in buf[0..setbuf_len) */
> + list_for_each_entry(sdevice, &smaster->devices, node) {
> + struct siox_driver *sdriver =
> + to_siox_driver(sdevice->dev.driver);
> + sdevice->status_written = smaster->status;
> +
> + i -= sdevice->inbytes;
> +
> + /*
> + * If the device or a previous one is unsynced, don't pet the
> + * watchdog. This is done to ensure that the device is kept in
> + * reset when something is wrong.
> + */
> + if (!siox_device_synced(sdevice))
> + unsync_error = 1;
> +
> + if (sdriver && !unsync_error)
> + sdriver->set_data(sdevice, sdevice->status_written,
> + &smaster->buf[i + 1]);
> + else
> + /*
> + * Don't trigger watchdog if there is no driver or a
> + * sync problem
> + */
> + sdevice->status_written &= ~SIOX_STATUS_WDG;
> +
> + smaster->buf[i] = sdevice->status_written;
> + }
> +
> + WARN_ON(i);

What can someone do if this WARN_ON() triggers? Seems like it is
left-over debugging code? If not, make it a "real" error and handle it
properly. Or provide some sort of context as to what is going on here.

thanks,

greg k-h