From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758574AbdLRPGr (ORCPT ); Mon, 18 Dec 2017 10:06:47 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:51314 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752720AbdLRPGq (ORCPT ); Mon, 18 Dec 2017 10:06:46 -0500 Date: Mon, 18 Dec 2017 16:06:48 +0100 From: Greg Kroah-Hartman To: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= Cc: linux-kernel@vger.kernel.org, kernel@pengutronix.de, Gavin Schenk Subject: Re: [PATCH v1 1/5] siox: new driver framework for eckelmann SIOX Message-ID: <20171218150648.GA25024@kroah.com> References: <20171207093008.20688-1-u.kleine-koenig@pengutronix.de> <20171207093008.20688-2-u.kleine-koenig@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20171207093008.20688-2-u.kleine-koenig@pengutronix.de> User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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