From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35186) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S5BdT-0007qA-Ny for qemu-devel@nongnu.org; Wed, 07 Mar 2012 02:50:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S5BdD-0007wV-Q1 for qemu-devel@nongnu.org; Wed, 07 Mar 2012 02:50:11 -0500 Received: from plane.gmane.org ([80.91.229.3]:45381) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S5BdD-0007wA-J9 for qemu-devel@nongnu.org; Wed, 07 Mar 2012 02:49:55 -0500 Received: from list by plane.gmane.org with local (Exim 4.69) (envelope-from ) id 1S5BdB-0007BW-Cg for qemu-devel@nongnu.org; Wed, 07 Mar 2012 08:49:53 +0100 Received: from 213.33.220.118 ([213.33.220.118]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Wed, 07 Mar 2012 08:49:53 +0100 Received: from i.mitsyanko by 213.33.220.118 with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Wed, 07 Mar 2012 08:49:53 +0100 From: Igor Mitsyanko Date: Wed, 07 Mar 2012 11:49:40 +0400 Message-ID: <4F571314.70000@samsung.com> References: <1330688145-22944-1-git-send-email-i.mitsyanko@samsung.com> <1330688145-22944-4-git-send-email-i.mitsyanko@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 3/5] exynos4210: add Exynos4210 i2c implementation Reply-To: i.mitsyanko@samsung.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: kyungmin.park@samsung.com, m.kozlov@samsung.com, e.voevodin@samsung.com, d.solodkiy@samsung.com On 03/06/2012 10:49 PM, Peter Maydell wrote: > On 2 March 2012 11:35, Igor Mitsyanko wrote: >> Create 9 exynos4210 i2c interfaces. >> >> Signed-off-by: Igor Mitsyanko > >> +#define EXYNOS4_I2C(obj) \ >> +OBJECT_CHECK(Exynos4210I2CState, (obj), TYPE_EXYNOS4_I2C) >> +#define EXYNOS4_I2C_SLAVE(obj) \ >> +OBJECT_CHECK(Exynos4210I2CSlave, (obj), TYPE_EXYNOS4_I2C_SLAVE) > > Can you indent the second and subsequent lines of multi-line > macro definitions, please? OK >> +enum { >> + idle, >> + start_bit_received, >> + sending_data, >> + receiving_data >> +}; >> + >> +struct Exynos4210I2CSlave { >> + I2CSlave i2c; >> + Exynos4210I2CState *host; >> + uint32_t status; >> +}; > > I'm confused about the purpose of this class. My (possibly naive) > model of i2c is that there's a controller (in this case TYPE_EXYNOS4_I2C > , with its state in the struct Exynos4210I2CState) which is a subclass > of SysBus or whatever, and then there are i2c slaves, which are > subclasses of TYPE_I2C_SLAVE and communicate with the controller > strictly via the methods provided by the I2C_SLAVE class. But > this seems to be an I2CSlave subclass which has a direct pointer to > the internal state of the controller and very little state of its own. > What's it for? > The reasoning behind this is that Exynos i2c controller can operate as i2c master or i2c slave (not simultaneously), so it must be a SysBusDevice and I2CSlave at the same time. There shouldn't be any communications of controller and some abstract I2C_SLAVE because controller is in fact I2C_SLAVE by itself, internal state of TYPE_EXYNOS4_I2C and TYPE_EXYNOS4_I2C_SLAVE is the same set of registers (hence the pointer). I took this approach from pxa2xx i2c implementation. >> +static const VMStateDescription exynos4210_i2c_vmstate = { >> + .name = TYPE_EXYNOS4_I2C, >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .fields = (VMStateField[]) { > > Why the big space? > I don't know :) -- Mitsyanko Igor ASWG, Moscow R&D center, Samsung Electronics email: i.mitsyanko@samsung.com