Hi,

On 12 October 2015 at 12:50, Hans Verkuil [off-list ref] wrote:

On 10/06/2015 12:32 AM, Russell King - ARM Linux wrote:

quoted

On Mon, Sep 07, 2015 at 03:44:43PM +0200, Hans Verkuil wrote:

quoted

+    if (status & CEC_STATUS_TX_DONE) {
+            if (status & CEC_STATUS_TX_ERROR) {
+                    dev_dbg(cec->dev, "CEC_STATUS_TX_ERROR set\n");
+                    cec->tx = STATE_ERROR;
+            } else {
+                    dev_dbg(cec->dev, "CEC_STATUS_TX_DONE\n");
+                    cec->tx = STATE_DONE;
+            }
+            s5p_clr_pending_tx(cec);
+    }

Your CEC implementation seems to be written around the idea that there
are only two possible outcomes from a CEC message - "done" and "error",
which get translated to:

This code is for the Samsung exynos CEC implementation. Marek, is this all
that the exynos CEC hardware returns?

The possible status values that are implemented in the CEC framework
are following:

+/* cec status field */
+#define CEC_TX_STATUS_OK               (0)
+#define CEC_TX_STATUS_ARB_LOST         (1 << 0)
+#define CEC_TX_STATUS_RETRY_TIMEOUT    (1 << 1)
+#define CEC_TX_STATUS_FEATURE_ABORT    (1 << 2)
+#define CEC_TX_STATUS_REPLY_TIMEOUT    (1 << 3)
+#define CEC_RX_STATUS_READY            (0)

The only two ones I could match with the Exynos CEC module status bits are
CEC_TX_STATUS_OK and  CEC_TX_STATUS_RETRY_TIMEOUT.

The status bits in Exynos HW are:
- Tx_Error
- Tx_Done
- Tx_Transferring
- Tx_Running
- Tx_Bytes_Transferred

- Tx_Wait
- Tx_Sending_Status_Bit
- Tx_Sending_Hdr_Blk
- Tx_Sending_Data_Blk
- Tx_Latest_Initiator

- Tx_Wait_SFT_Succ
- Tx_Wait_SFT_New
- Tx_Wait_SFT_Retran
- Tx_Retrans_Cnt
- Tx_ACK_Failed

quoted

quoted

+    case STATE_DONE:
+            cec_transmit_done(cec->adap, CEC_TX_STATUS_OK);
+            cec->tx = STATE_IDLE;
+            break;
+    case STATE_ERROR:
+            cec_transmit_done(cec->adap, CEC_TX_STATUS_RETRY_TIMEOUT);
+            cec->tx = STATE_IDLE;

"okay" and "retry_timeout".  So, if we have an adapter which can report
(eg) a NACK, we have to report it as the obscure "retry timeout" status?
Why this obscure naming - why can't we have something that uses the
terminology in the spec?

Actually, a NACK should lead to a re-transmission (up to 5 times), see CEC 7.1.
The assumption of the CEC framework is that this is done by the CEC adapter
driver, not by the framework. So if after repeated retransmissions there is
still no Ack, the CEC_TX_STATUS_RETRY_TIMEOUT error is returned. I could
change this to _MAX_RETRIES_REACHED if you prefer.

The CEC_TX_STATUS_ macros were based on what the adv drivers support (except
for CEC_TX_STATUS_REPLY_TIMEOUT which is specific to the framework).

Regards,

        Hans

Best wishes,
Kamil