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