Re: [PATCH v2 2/2] cec: add STM32 cec driver

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

 



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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux