On 08/12/2016 04:38 PM, Hans Verkuil wrote:
On 08/12/2016 04:15 PM, Russell King wrote:
Add a CEC driver for the TDA9950, which is a stand-alone I2C CEC device. The TDA9950 contains a command processor which handles retransmissions and the low level bus protocol. The driver just has to read and write the messages, and handle error conditions.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk
drivers/gpu/drm/i2c/Kconfig | 5 + drivers/gpu/drm/i2c/Makefile | 1 + drivers/gpu/drm/i2c/tda9950.c | 514 ++++++++++++++++++++++++++++++++++ include/linux/platform_data/tda9950.h | 15 + 4 files changed, 535 insertions(+) create mode 100644 drivers/gpu/drm/i2c/tda9950.c create mode 100644 include/linux/platform_data/tda9950.h
<snip>
+static int tda9950_cec_adap_log_addr(struct cec_adapter *adap, u8 addr) +{
- struct tda9950_priv *priv = adap->priv;
- u16 addresses;
- u8 buf[2];
- if (addr == CEC_LOG_ADDR_INVALID)
addresses = priv->addresses = BIT(15);I saw this in patch 4/5 as well: why set bit 15? I would expect that this is just set to 0. And priv->addresses doesn't seem to be used anywhere.
Yeah, you are right, priv->addresses is used. I've been reviewing too many patches today.
The whole BIT(15) part remains weird. If log_addr is called with LOG_ADDR_INVALID as argument, then the intention is that no more messages are to be received (unless the hardware is in snooping mode). So there is no need to receive broadcast messages either.
That said, I now realize that if userspace wants to configure the CEC device as 'Unregistered', then adap_log_addr is never called, which would be required if the hardware has to enable support to receive broadcast messages.
In addition cec_received_msg should ignore received messages if log_addr_mask of struct cec_log_addrs is 0. I think it will just pass on messages right now, and that's not right.
I will look at this tomorrow when my brain isn't fried and do some more testing with 'Unregistered' scenarios.
- else
addresses = priv->addresses |= BIT(addr);- /* TDA9950 doesn't want address 15 set */
- addr &= 0x7fff;
Shouldn't this be 'addresses' instead of 'addr'? 'addr' makes no sense here.
And if so, then I still don't understand setting BIT(15), since that bit is removed by the &=.
- buf[0] = addresses >> 8;
- buf[1] = addresses;
- return tda9950_write_range(priv->client, REG_ACKH, buf, 2);
+}
Regards,
Hans