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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux