From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751444AbbCTHmc (ORCPT ); Fri, 20 Mar 2015 03:42:32 -0400 Received: from metis.ext.pengutronix.de ([92.198.50.35]:56598 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750977AbbCTHma (ORCPT ); Fri, 20 Mar 2015 03:42:30 -0400 Date: Fri, 20 Mar 2015 08:42:14 +0100 From: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= To: Wolfram Sang Cc: linux-i2c@vger.kernel.org, linux-sh@vger.kernel.org, Magnus Damm , Simon Horman , Laurent Pinchart , Geert Uytterhoeven , Andrey Danin , Marc Dietrich , Debora Grosse , linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/3] Documentation: i2c: describe the new slave mode Message-ID: <20150320074214.GD10068@pengutronix.de> References: <1426164123-8853-1-git-send-email-wsa@the-dreams.de> <1426164123-8853-3-git-send-email-wsa@the-dreams.de> <20150319201137.GY10068@pengutronix.de> <20150320073012.GB906@katana> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20150320073012.GB906@katana> User-Agent: Mutt/1.5.21 (2010-09-15) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c0 X-SA-Exim-Mail-From: ukl@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Wolfram, On Fri, Mar 20, 2015 at 08:30:13AM +0100, Wolfram Sang wrote: > > > > +Finally, Linux can also be an I2C slave in case I2C controllers have slave > > > +support. Besides this HW requirement, one also needs a software backend > > I wouldn't have written "Finally, ...". While it's great that we have > > slave support now, being enthusiastic here looks strange if someone > > reads it while slave support has become "normal". > > OK. > > > > +providing the actual functionality. An example for this is the slave-eeprom > > > +driver, which acts as a dual memory driver. While another I2C master on the bus > > > +can access it like a regular eeprom, the Linux I2C slave can access the content > > > +via sysfs and retrieve/provide information as needed. The software backend > > > +driver and the I2C bus driver communicate via events. Here is a small graph > > > +visualizing the data flow and the means by which data is transported. The > > > +dotted line marks only one example. The backend could also use e.g. a character > > > +device, or use in-kernel mechanisms only, or something completely different: > > Or something self contained, so the userspace part is actually optional > > (but probably present most of the time). > > With "in-kernel mechanisms" I meant self-contained. Maybe "be in-kernel > only"? I'm sure "in-kernel mechanisms" wasn't in the mail I replied to. (Hmm, or I must have missed that while reading.) > > Another slave backend I have in mind is a bus-driver tester. That > > wouldn't necessarily need a userspace part. > > Yes, I envisioned that, too. > > > > > > + e.g. sysfs I2C slave events I/O registers > > > + +-----------+ v +---------+ v +--------+ v +------------+ > > > + | Userspace +........+ Backend +-----------+ Driver +-----+ Controller | > > > + +-----------+ +---------+ +--------+ +------------+ > > > + | | > > > + ----------------------------------------------------------------+-- I2C > > > + --------------------------------------------------------------+---- Bus > > > + > > > +Note: Technically, there is also the I2C core between the backend and the > > > +driver. However, at this time of writing, the layer is transparent. > > s/this/the/ ? > > Maybe. > > > > +The bus driver sends an event to the backend using the following function: > > > + > > > + ret = i2c_slave_event(client, event, &val) > > > + > > > +'client' describes the i2c slave device. 'event' is one of the special event > > > +types described hereafter. 'val' holds an u8 value for the data byte to be > > > +read/written and is thus bidirectional. The pointer to val must always be > > > +provided even if val is not used for an event. 'ret' is the return value from > > Does that mean that I have to pass a valid address, or can I use NULL, > > too? > > Is NULL a valid pointer to val? NULL is a pointer and you didn't wrote about "valid" above. I just wondered if the necessity just comes from the fact that the function takes 3 parameters and so you have to give it 3 (this wouldn't needed to be pointed out IMHO) or if the value must be valid (then the wording isn't optimal). Is there a technical reason to require val to be valid? > > I didn't look into the actual implementation yet, but if I understand > > correctly a slave driver only sees I2C_SLAVE_READ_REQUESTED once and > > then only one I2C_SLAVE_READ_PROCESSED per byte, right? Does the slave > > Right. > > > driver gets noticed somehow if a previously requested byte doesn't make > > it on the wire? Otherwise you cannot correctly maintain e.g. the current > > Some HW can do this, but not all. That would maybe be another candidate > for an optional event. Although, people should try hard to not need it. > > > read position of the eeprom driver, do you? (That's a bit like one of > > the problems with buffer support you pointed out further down.) > > You need to assume that if the next byte is requested, the previous byte > made it to the bus. So, you should do pre-increment in > I2C_SLAVE_READ_PROCESSED, not post-increment. I didn't want to write This might be a correctness problem, right? If we cannot fix it (with the current slave abstraction) should this be pointed out somewhere; at least in the eeprom driver as this will probably be the reference for the next backend? > this example so explicit because not all slave-backends will have a > position pointer. And besides, it might make sense to extract the code for > managing a position pointer from the slave-eeprom driver. Then, we'd > have only one trusted implementation of EEPROM-alike behaviour and > people can concentrate on reacting to reads/writes. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |