[PATCH RFC 5/5] drm/i2c: add tda998x/tda9950 CEC driver

[Hans Verkuil]

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