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