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.
<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.
> + regmap_update_bits(cec->regmap, CEC_CFGR, LSTN, LSTN);
> + }
> +
> + regmap_update_bits(cec->regmap, CEC_CR, CECEN, CECEN);
> +
> + return 0;
> +}
<snip>
Regards,
Hans
