Re: [Qemu-devel] [PATCH 3/5] exynos4210: add Exynos4210 i2c implementation

[Igor Mitsyanko <i.mitsyanko@samsung.com>]

On 03/06/2012 10:49 PM, Peter Maydell wrote:
> On 2 March 2012 11:35, Igor Mitsyanko<i.mitsyanko@samsung.com>  wrote:
>> Create 9 exynos4210 i2c interfaces.
>>
>> Signed-off-by: Igor Mitsyanko<i.mitsyanko@samsung.com>
>
>> +#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