Re: [PATCH 2/2] media: cec: i2c: ch7322: Add ch7322 CEC controller driver

[Jeff Chase <jnchase@google.com>]

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