2017-05-16 15:09 GMT+02:00 Hans Verkuil <hverkuil@xxxxxxxxx>:
> Looks good, except for the logical address handling that I think is wrong:
>
> On 16/05/17 14:56, Benjamin Gaignard wrote:
>> This patch add cec driver for STM32 platforms.
>> cec hardware block isn't not always used with hdmi so
>> cec notifier is not implemented. That will be done later
>> when STM32 DSI driver will be available.
>>
>> Driver compliance has been tested with cec-ctl and cec-compliance
>> tools.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxx>
>> Signed-off-by: Yannick Fertre <yannick.fertre@xxxxxx>
>> ---
>> drivers/media/platform/Kconfig | 11 +
>> drivers/media/platform/Makefile | 2 +
>> drivers/media/platform/stm32/Makefile | 1 +
>> drivers/media/platform/stm32/stm32-cec.c | 384 +++++++++++++++++++++++++++++++
>> 4 files changed, 398 insertions(+)
>> create mode 100644 drivers/media/platform/stm32/Makefile
>> create mode 100644 drivers/media/platform/stm32/stm32-cec.c
>>
>
> <snip>
>
>> +static int stm32_cec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
>> +{
>> + struct stm32_cec *cec = adap->priv;
>> +
>> + regmap_update_bits(cec->regmap, CEC_CR, CECEN, 0);
>> +
>> + if (logical_addr == CEC_LOG_ADDR_INVALID)
>> + regmap_update_bits(cec->regmap, CEC_CFGR, OAR, 0);
>> + else
>> + regmap_update_bits(cec->regmap, CEC_CFGR, OAR,
>> + (1 << logical_addr) << 16);
>> +
>> + regmap_update_bits(cec->regmap, CEC_CR, CECEN, CECEN);
>> +
>> + return 0;
>> +}
>> +
>
> If you allocate more than one logical address, then stm32_cec_adap_log_addr()
> is called once for each LA. But right now the second call would overwrite
> the first LA. Right?
>
> Try 'cec-ctl --audio --playback' to allocate two logical addresses.
I will fix that in v3
>
> <snip>
>
>> +static int stm32_cec_monitor_all(struct cec_adapter *adap, bool enable)
>> +{
>> + struct stm32_cec *cec = adap->priv;
>> +
>> + regmap_update_bits(cec->regmap, CEC_CR, CECEN, 0);
>> +
>> + if (enable) {
>> + regmap_update_bits(cec->regmap, CEC_CFGR, OAR, OAR)
>
> You shouldn't have to change the OAR mask. This would have the adapter
> Ack all logical addresses.
>
>> + regmap_update_bits(cec->regmap, CEC_CFGR, LSTN, 0);
>> + } else {
>> + regmap_update_bits(cec->regmap, CEC_CFGR, OAR, 0);
>
> And this would disable all logical addresses.
>
> I would expect that only the LSTN bit was changed.
>
> In monitoring mode it should still Ack any messages directed to us.
This is an impossible mix in my hardware: messages are received if
corresponding OAR
bit is set and acked if LSTN is set to 1.
It can't receive all messages and only ack some of them....
>
>> + regmap_update_bits(cec->regmap, CEC_CFGR, LSTN, LSTN);
>> + }
>> +
>> + regmap_update_bits(cec->regmap, CEC_CR, CECEN, CECEN);
>> +
>> + return 0;
>> +}
>
> <snip>
>
> Regards,
>
> Hans
--
Benjamin Gaignard
Graphic Study Group
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
