On 07/24/2017 06:13 PM, Peter Maydell wrote:
> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> i2c_recv() returns -1 on error, if the I2CCON_ACK_GEN bit was not set this code
>> was setting i2cds = -1.
>>
>> i2c/exynos4210_i2c.c:117:20: warning: Loss of sign in implicit conversion
>> s->i2cds = ret;
>> ^~~
>>
>> Reported-by: Clang Static Analyzer
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> hw/i2c/exynos4210_i2c.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/i2c/exynos4210_i2c.c b/hw/i2c/exynos4210_i2c.c
>> index c96fa7d7be..4424dbd233 100644
>> --- a/hw/i2c/exynos4210_i2c.c
>> +++ b/hw/i2c/exynos4210_i2c.c
>> @@ -111,10 +111,12 @@ static void exynos4210_i2c_data_receive(void *opaque)
>> s->i2cstat &= ~I2CSTAT_LAST_BIT;
>> s->scl_free = false;
>> ret = i2c_recv(s->bus);
>> - if (ret < 0 && (s->i2ccon & I2CCON_ACK_GEN)) {
>> - s->i2cstat |= I2CSTAT_LAST_BIT; /* Data is not acknowledged */
>> - } else {
>> + if (ret >= 0) {
>> s->i2cds = ret;
>> + } else {
>> + if (s->i2ccon & I2CCON_ACK_GEN) {
>> + s->i2cstat |= I2CSTAT_LAST_BIT; /* Data is not acknowledged */
>> + }
>> }
>> exynos4210_i2c_raise_interrupt(s);
>> }
>> --
>
> Have you checked this change against the data sheet for
> the device?
Here is the relevant part of the Exynos4210_UM DS:
[*] 14.3.5 Data Transfer Format
... If the I2C-bus is operating in Master mode, master transmits the
address field. Each byte should be followed by an acknowledgement (ACK) bit.
[*] 14.3.6 ACK Signal Transmission
To complete a one-byte transfer operation, the receiver sends an ACK bit
to the transmitter. The ACK pulse occurs at the ninth clock of the SCL
line. ...
The software (I2CSTAT) enables or disables ACK bit transmit function.
However, the ACK pulse on the ninth clock of SCL is required to complete
the one-byte data transfer operation.
[*] 14.3.9 Abort Conditions
If a slave receiver cannot acknowledge the confirmation of the slave
address, it holds the level of the SDA line High. In this case, the
master generates a Stop condition and cancels the transfer.
If a master receiver is involved in the aborted transfer, it signals the
end of slave transmit operation by canceling the generation of an ACK
after the last data byte received from the slave. The slave transmitter
releases the SDA to allow a master to generate a Stop condition.
I2C-bus last-received bit status flag bit. (I2CSTAT_LAST_BIT)
0 = Last-received bit is 0 (ACK was received).
1 = Last-received bit is 1 (ACK was not received).
An I2C-bus interrupt occurs if 1) if a 1-byte transmit or receive
operation is complete. In other words, ack period is finished. 2) A
general call or a slave address match occurs, 3) Bus arbitration fails.
--
So the current code is not wrong and matches the datashit, crap is
shifted into I2CDS and the guest has to poll I2CSTAT to check the peer
ACK... Still it is weird to shift 0xff as of the current implementation,
but it might have some usefulness while debugging, who knows...
I might add few comments in that file during 2.11 cycle.
Thank for the review!
Patch dropped.
Regards,
Phil.