On 12/07/17 21:43, Hans Verkuil wrote:
> On 12/07/17 21:02, Eric Anholt wrote:
>>> +static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>>> + u32 signal_free_time, struct cec_msg *msg)
>>> +{
>>> + struct vc4_dev *vc4 = cec_get_drvdata(adap);
>>> + u32 val;
>>> + unsigned int i;
>>> +
>>> + for (i = 0; i < msg->len; i += 4)
>>> + HDMI_WRITE(VC4_HDMI_CEC_TX_DATA_1 + i,
>>> + (msg->msg[i]) |
>>> + (msg->msg[i + 1] << 8) |
>>> + (msg->msg[i + 2] << 16) |
>>> + (msg->msg[i + 3] << 24));
>>> +
>>> + val = HDMI_READ(VC4_HDMI_CEC_CNTRL_1);
>>> + val &= ~VC4_HDMI_CEC_START_XMIT_BEGIN;
>>> + HDMI_WRITE(VC4_HDMI_CEC_CNTRL_1, val);
>>> + val &= ~VC4_HDMI_CEC_MESSAGE_LENGTH_MASK;
>>> + val |= (msg->len - 1) << VC4_HDMI_CEC_MESSAGE_LENGTH_SHIFT;
>>> + val |= VC4_HDMI_CEC_START_XMIT_BEGIN;
>>
>> It doesn't look to me like len should have 1 subtracted from it. The
>> field has 4 bits for our up-to-16-byte length, and the firmware seems to
>> be setting it to the same value as a memcpy for the message data uses.
>
> You need to subtract by one. The CEC protocol supports messages of 1-16
> bytes in length. Since the message length mask is only 4 bits you need to
> encode this in the value 0-15. Hence the '-1', otherwise you would never
> be able to send 16 byte messages.
>
> I actually found this when debugging the messages it was transmitting: they
> were one too long.
>
> This suggests that the firmware does this wrong. I don't have time tomorrow,
> but I'll see if I can do a quick test on Friday to verify that.
I double-checked this and both the driver and the firmware do the right thing.
Just to be certain I also tried sending a message that uses the full 16 byte
payload and that too went well. So the code is definitely correct.
Regards,
Hans
