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