[PATCHv9 14/15] cec: s5p-cec: Add s5p-cec driver

[Kamil Debski]

Hi,

On 12 October 2015 at 12:50, Hans Verkuil <hverkuil at xs4all.nl> wrote:
> On 10/06/2015 12:32 AM, Russell King - ARM Linux wrote:
>> On Mon, Sep 07, 2015 at 03:44:43PM +0200, Hans Verkuil wrote:
>>> +    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

>
>>
>>> +    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