On 25/04/2020 11:16, Hans Verkuil 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.
>
> 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.
Reading through the product brief they mention "Supported Logical Type". It could
be that this device is trying to be smart and that it has to be programmed with
the desired type (which in the linux CEC API is equivalent to CEC_LOG_ADDR_TYPE_*),
and that it claims the logical address based on that.
If so, then that is currently not supported in the CEC framework and some work
would have to be done there.
Regards,
Hans
>
> 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
>>
>
