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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html