On 06/05/2020 01:26, Jeff Chase wrote:
> On Sat, Apr 25, 2020 at 5:16 AM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>>
>> On 24/04/2020 21:33, Jeff Chase wrote:
>>> Hi Hans,
>>>
>>> Thank you for the quick review.
>>>
>>>> Is the register documentation available somewhere? I only found the product brief.
>>>
>>> No, it's not publicly available.
>>>
>>>> The chip can only detect OK vs NACK? There are no error states for Arbitration Lost
>>>> or Low Drive conditions? Just checking, not all hardware has support for that.
>>>
>>> Correct, message transmit completion just has a one-bit status.
>>>
>>>>> +static int ch7322_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
>>>>> +{
>>>>> + struct ch7322 *ch7322 = cec_get_drvdata(adap);
>>>>> +
>>>>> + dev_dbg(&ch7322->i2c->dev, "cec log addr: %x\n", log_addr);
>>>>> +
>>>>> + return 0;
>>>>
>>>> This can't be right. I expect that logical addresses are set/cleared here,
>>>> because the device needs to know that so that it can ignore messages not
>>>> intended for it.
>>>
>>> As far as I can tell the device doesn't filter based on logical
>>> address. I'll have to save
>>> the logical address to the driver and filter manually.
>>
>> That can't be right. If this CEC adapter is assigned logical address 4, and
>> it has to Ack any received messages from other CEC devices with destination 4,
>> and ignore (i.e. not explicitly Ack) messages with other destinations.
>>
>> If the CEC adapter wouldn't know what LA to use, then it would have to Ack
>> all messages, regardless of the destination, which would make this a complete
>> mess.
>>
>> There must be a register that tells the CEC adapter which logical address(es)
>> should be Acked. It's usually a bitmask (one bit for each possible LA) or the
>> LA itself is stored.
>
> Sorry, you're right, of course. The register isn't in the
> documentation I have but I found it referenced in some sample code. By
> default it seems the device automatically stores the logical address
> if it recognizes a polling message (with src == dst) that was not
> ack'd. The behavior can be configured to allow explicit logical
> address assignment instead. I assume that would be preferred?
Yes, that's preferred (and also the only thing the CEC framework supports).
Very odd that that register is undocumented.
Regards,
Hans
>
> Thanks,
> Jeff
>
>
>>
>> It might be that you still receive all messages (in which case monitor_all
>> is effectively always enabled), but it really needs to be told which LAs should
>> be Acked.
>>
>> Regards,
>>
>> Hans
>>
>>>
>>>>> +}
>>>>> +
>>>>> +static int ch7322_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>>>>> + u32 signal_free_time, struct cec_msg *msg)
>>>>> +{
>>>>
>>>> Does the hardware correctly handle Signal Free Time? If this isn't handled right
>>>> then one CEC device can flood the CEC bus, preventing anyone else from using it.
>>>>
>>>> In some devices it has to be programmed, in others it is hardwired.
>>>
>>> It must be hardwired -- I don't see a way to program it.
>>>
>>>>> + struct ch7322 *ch7322 = cec_get_drvdata(adap);
>>>>> + int ret;
>>>>> +
>>>>> + dev_dbg(&ch7322->i2c->dev, "cec transmit: %x->%x: %x\n",
>>>>> + cec_msg_initiator(msg), cec_msg_destination(msg),
>>>>> + cec_msg_opcode(msg));
>>>>> +
>>>>> + mutex_lock(&ch7322->mutex);
>>>>> + ret = ch7322_send_message(ch7322, msg);
>>>>> + mutex_unlock(&ch7322->mutex);
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static const struct cec_adap_ops ch7322_cec_adap_ops = {
>>>>> + .adap_enable = ch7322_cec_adap_enable,
>>>>> + .adap_log_addr = ch7322_cec_adap_log_addr,
>>>>> + .adap_transmit = ch7322_cec_adap_transmit,
>>>>
>>>> If the HW supports CEC monitoring (aka snooping), then I recommend that
>>>> adap_monitor_all_enable is also implemented. It's very useful for debugging
>>>> CEC in userspace. Not all HW supports it, though.
>>>
>>> Okay, I'll add this along with the logical address filtering I mentioned above.
>>>
>>> Thanks,
>>> Jeff
>>>
>>
