bsp/atsam/i2c: Add error return and fix edge cases

The driver didn't return with an error on (for example) a NACK on the
bus. This adds the expected error return. Due to the new case that a
transfer can be interrupted on an error, there were some new edge cases.
This patch therefore also fixes these edge cases by removing the
transfer_state that more or less duplicated the interrupt states.

Fixes #4592

bsps/arm/atsam/i2c/atsam_i2c_bus.c

@@ -33,18 +33,18 @@
 static void
 atsam_i2c_disable_interrupts(Twihs *regs)
 {
-	regs->TWIHS_IDR = 0xFFFFFFFF;
+	TWI_DisableIt(regs, 0xFFFFFFFF);
 }
 
-static void
+/*
-atsam_i2c_set_transfer_status(atsam_i2c_bus *bus, transfer_state state)
+ * Return true if the message is done right after this. That is the case if all
-{
+ * bytes are received but no stop is requested.
-	bus->trans_state = state;
+ */
-}
+static bool
-
-static void
 atsam_i2c_continue_read(Twihs *regs, atsam_i2c_bus *bus)
 {
+	bool done = false;
+
 	*bus->current_msg_byte = TWI_ReadByte(regs);
 	++bus->current_msg_byte;
 	--bus->current_msg_todo;
@@ -54,35 +54,35 @@ atsam_i2c_continue_read(Twihs *regs, atsam_i2c_bus *bus)
 		if (bus->stop_request){
 			TWI_DisableIt(regs, TWIHS_IDR_RXRDY);
 			TWI_EnableIt(regs, TWIHS_IER_TXCOMP);
-			atsam_i2c_set_transfer_status(bus, TX_RX_STOP_SENT);
 		} else {
-			atsam_i2c_set_transfer_status(bus, RX_CONT_MESSAGE_NEEDED);
+			done = true;
 		}
 	}
 	/* Last byte? */
 	else if (bus->current_msg_todo == 1 && bus->stop_request) {
 		TWI_Stop(regs);
 	}
+
+	return done;
 }
 
+/*
+ * Return true if the message is done right after this. That is the case if all
+ * bytes are sent but no stop is requested.
+ */
 static bool
-atsam_i2c_is_state(atsam_i2c_bus *bus, transfer_state state)
-{
-	return (bus->trans_state == state);
-}
-
-static void
 atsam_i2c_continue_write(Twihs *regs, atsam_i2c_bus *bus)
 {
+	bool done = false;
+
 	/* Transfer finished ? */
 	if (bus->current_msg_todo == 0) {
 		TWI_DisableIt(regs, TWIHS_IDR_TXRDY);
 		if (bus->stop_request){
 			TWI_EnableIt(regs, TWIHS_IER_TXCOMP);
 			TWI_SendSTOPCondition(regs);
-			atsam_i2c_set_transfer_status(bus, TX_RX_STOP_SENT);
 		} else {
-			atsam_i2c_set_transfer_status(bus, TX_CONT_MESSAGE_NEEDED);
+			done = true;
 		}
 	}
 	/* Bytes remaining */
@@ -91,13 +91,7 @@ atsam_i2c_continue_write(Twihs *regs, atsam_i2c_bus *bus)
 		++bus->current_msg_byte;
 		--bus->current_msg_todo;
 	}
-}
+	return done;
-
-static void
-atsam_i2c_finish_write_transfer(Twihs *regs)
-{
-		TWI_ReadByte(regs);
-		TWI_DisableIt(regs, TWIHS_IDR_TXCOMP);
 }
 
 static void
@@ -202,11 +196,9 @@ atsam_i2c_setup_transfer(atsam_i2c_bus *bus, Twihs *regs)
 		if (bus->current_msg_todo == 1){
 			send_stop = true;
 		}
-		atsam_i2c_set_transfer_status(bus, RX_SEND_DATA);
 		atsam_i2c_setup_read_transfer(regs, ten_bit_addr,
 		    slave_addr, send_stop);
 	} else {
-		atsam_i2c_set_transfer_status(bus, TX_SEND_DATA);
 		atsam_i2c_setup_write_transfer(bus, regs, ten_bit_addr,
 		    slave_addr);
 	}
@@ -222,33 +214,24 @@ atsam_i2c_interrupt(void *arg)
 	Twihs *regs = bus->regs;
 
 	/* read interrupts */
-	irqstatus = regs->TWIHS_SR;
+	irqstatus = TWI_GetMaskedStatus(regs);
 
-	if((irqstatus & (TWIHS_SR_ARBLST | TWIHS_SR_NACK)) != 0) {
+	if((irqstatus & ATSAMV_I2C_IRQ_ERROR) != 0) {
 		done = true;
-	}
+	} else if ((irqstatus & TWIHS_SR_RXRDY) != 0) {
-
+		done = atsam_i2c_continue_read(regs, bus);
-	if (((irqstatus & TWIHS_SR_RXRDY) != 0) &&
+	} else if ((irqstatus & TWIHS_SR_TXCOMP) != 0) {
-	    (atsam_i2c_is_state(bus, RX_SEND_DATA))){
+		TWI_DisableIt(regs, TWIHS_IDR_TXCOMP);
-		/* carry on read transfer */
-		atsam_i2c_continue_read(regs, bus);
-	} else if (((irqstatus & TWIHS_SR_TXCOMP) != 0) &&
-	    (atsam_i2c_is_state(bus, TX_RX_STOP_SENT))){
-		atsam_i2c_finish_write_transfer(regs);
 		done = true;
-	} else if (((irqstatus & TWIHS_SR_TXRDY) != 0) &&
+	} else if ((irqstatus & TWIHS_SR_TXRDY) != 0) {
-	    (atsam_i2c_is_state(bus, TX_SEND_DATA))){
+		done = atsam_i2c_continue_write(regs, bus);
-		atsam_i2c_continue_write(regs, bus);
-		if (atsam_i2c_is_state(bus, TX_CONT_MESSAGE_NEEDED)){
-			done = true;
-		}
 	}
 
 	if(done){
-		uint32_t err = irqstatus & ATSAMV_I2C_IRQ_ERROR;
+		bus->err = irqstatus & ATSAMV_I2C_IRQ_ERROR;
 
 		atsam_i2c_next_packet(bus);
-		if (bus->msg_todo == 0 || err != 0) {
+		if (bus->msg_todo == 0 || bus->err != 0) {
 			atsam_i2c_disable_interrupts(regs);
 			rtems_binary_semaphore_post(&bus->sem);
 		} else {
@@ -273,28 +256,39 @@ atsam_i2c_transfer(i2c_bus *base, i2c_msg *msgs, uint32_t msg_count)
 		if ((msgs[i].flags & I2C_M_RECV_LEN) != 0) {
 			return -EINVAL;
 		}
+		if (msgs[i].len == 0) {
+			/* Hardware doesn't support zero length messages */
+			return -EINVAL;
+		}
 	}
 
 	bus->msgs = &msgs[0];
 	bus->msg_todo = msg_count;
 	bus->current_msg_todo = msgs[0].len;
 	bus->current_msg_byte = msgs[0].buf;
+	bus->err = 0;
 
 	regs = bus->regs;
 
-	atsam_i2c_setup_transfer(bus, regs);
+	/* Start with a clean start. Enable error interrupts. */
+	TWI_ConfigureMaster(bus->regs, bus->output_clock, BOARD_MCK);
+	TWI_EnableIt(regs, ATSAMV_I2C_IRQ_ERROR);
 
-	regs->TWIHS_IER = ATSAMV_I2C_IRQ_ERROR;
+	atsam_i2c_setup_transfer(bus, regs);
 
 	eno = rtems_binary_semaphore_wait_timed_ticks(
 		&bus->sem,
 		bus->base.timeout
 	);
-	if (eno != 0) {
+	if (eno != 0 || bus->err != 0) {
 		TWI_ConfigureMaster(bus->regs, bus->output_clock, BOARD_MCK);
 		rtems_binary_semaphore_try_wait(&bus->sem);
+		if (bus->err != 0) {
+			return -EIO;
+		} else {
 			return -ETIMEDOUT;
 		}
+	}
 
 	return 0;
 }

bsps/arm/atsam/include/bsp/atsam-i2c.h

@@ -28,14 +28,6 @@ extern "C" {
 
 #define TWI_AMOUNT_PINS 2
 
-typedef enum {
-	TX_SEND_DATA,
-	TX_CONT_MESSAGE_NEEDED,
-	RX_SEND_DATA,
-	RX_CONT_MESSAGE_NEEDED,
-	TX_RX_STOP_SENT
-}transfer_state;
-
 typedef struct {
 	i2c_bus base;
 	Twihs *regs;
@@ -46,10 +38,12 @@ typedef struct {
 
 	/* Information about the current transfer. */
 	bool stop_request;
-	transfer_state trans_state;
 	uint32_t current_msg_todo;
 	uint8_t *current_msg_byte;
 
+	/* Error information that can be returned to the task */
+	uint32_t err;
+
 	uint32_t output_clock;
 	rtems_binary_semaphore sem;
 	rtems_vector_number irq;