Messages in this thread | | | Date | Mon, 18 Dec 2017 16:06:48 +0100 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH v1 1/5] siox: new driver framework for eckelmann SIOX |
| |
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
| |