[PATCH 0/4] drm/bridge/adv7511: add CEC support

[PATCH 0/4] drm/bridge/adv7511: add CEC support

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This patch series adds CEC support to the drm adv7511/adv7533 drivers.

I have tested this with the Qualcomm Dragonboard C410 (adv7533 based)
and the Renesas R-Car Koelsch board (adv7511 based).

Note: the Dragonboard needs this patch:

https://patchwork.kernel.org/patch/9824773/

Archit, can you confirm that this patch will go to kernel 4.14?

And the Koelsch board needs this 4.13 fix:

https://patchwork.kernel.org/patch/9836865/

I only have the Koelsch board to test with, but it looks like other
R-Car boards use the same adv7511. It would be nice if someone can
add CEC support to the other R-Car boards as well. The main thing
to check is if they all use the same 12 MHz fixed CEC clock source.

Anyone who wants to test this will need the CEC utilities that
are part of the v4l-utils git repository:

git clone git://linuxtv.org/v4l-utils.git
cd v4l-utils
./bootstrap.sh
./configure
make
sudo make install

Now configure the CEC adapter as a Playback device:

cec-ctl --playback

Discover other CEC devices:

cec-ctl -S

Regards,

	Hans

Hans Verkuil (4):
  dt-bindings: adi,adv7511.txt: document cec clock
  arm: dts: qcom: add cec clock for apq8016 board
  arm: dts: renesas: add cec clock for Koelsch board
  drm: adv7511/33: add HDMI CEC support

 .../bindings/display/bridge/adi,adv7511.txt        |   4 +
 arch/arm/boot/dts/r8a7791-koelsch.dts              |   8 +
 arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi          |   2 +
 drivers/gpu/drm/bridge/adv7511/Kconfig             |   8 +
 drivers/gpu/drm/bridge/adv7511/Makefile            |   1 +
 drivers/gpu/drm/bridge/adv7511/adv7511.h           |  45 ++-
 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c       | 314 +++++++++++++++++++++
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 152 +++++++++-
 drivers/gpu/drm/bridge/adv7511/adv7533.c           |  30 +-
 9 files changed, 514 insertions(+), 50 deletions(-)
 create mode 100644 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c

-- 
2.13.1

[PATCH 1/4] dt-bindings: adi,adv7511.txt: document cec clock

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Document the cec clock binding.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
index 06668bca7ffc..4497ae054d49 100644
--- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
+++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
@@ -68,6 +68,8 @@ Optional properties:
 - adi,disable-timing-generator: Only for ADV7533. Disables the internal timing
   generator. The chip will rely on the sync signals in the DSI data lanes,
   rather than generate its own timings for HDMI output.
+- clocks: from common clock binding: handle to CEC clock.
+- clock-names: from common clock binding: must be "cec".
 
 Required nodes:
 
@@ -89,6 +91,8 @@ Example
 		reg = <39>;
 		interrupt-parent = <&gpio3>;
 		interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
+		clocks = <&cec_clock>;
+		clock-names = "cec";
 
 		adi,input-depth = <8>;
 		adi,input-colorspace = "rgb";
-- 
2.13.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] dt-bindings: adi,adv7511.txt: document cec clock

[Rob Herring]

On Sun, Jul 30, 2017 at 03:07:40PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
> 
> Document the cec clock binding.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 4 ++++
>  1 file changed, 4 insertions(+)

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] arm: dts: qcom: add cec clock for apq8016 board

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The adv7533 on this board needs a cec clock. Hook it up in the dtsi
to enable CEC for the HDMI transmitters.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
index eb513d625562..475d92d165ca 100644
--- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
@@ -88,6 +88,8 @@
 				interrupts = <31 2>;
 
 				adi,dsi-lanes = <4>;
+				clocks = <&rpmcc RPM_SMD_BB_CLK2>;
+				clock-names = "cec";
 
 				pd-gpios = <&msmgpio 32 0>;
 
-- 
2.13.1

[PATCH 3/4] arm: dts: renesas: add cec clock for Koelsch board

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The adv7511 on the Koelsch board has a 12 MHz fixed clock
for the CEC block. Specify this in the dts to enable CEC support.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 arch/arm/boot/dts/r8a7791-koelsch.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts b/arch/arm/boot/dts/r8a7791-koelsch.dts
index 001e6116c47c..88c8957b075b 100644
--- a/arch/arm/boot/dts/r8a7791-koelsch.dts
+++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
@@ -642,11 +642,19 @@
 		};
 	};
 
+	cec_clock: cec-clock {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <12000000>;
+	};
+
 	hdmi@39 {
 		compatible = "adi,adv7511w";
 		reg = <0x39>;
 		interrupt-parent = <&gpio3>;
 		interrupts = <29 IRQ_TYPE_LEVEL_LOW>;
+		clocks = <&cec_clock>;
+		clock-names = "cec";
 
 		adi,input-depth = <8>;
 		adi,input-colorspace = "rgb";
-- 
2.13.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] arm: dts: renesas: add cec clock for Koelsch board

[Geert Uytterhoeven]

On Sun, Jul 30, 2017 at 3:07 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>

Probably the one-line summary should be

    ARM: dts: koelsch: Add CEC clock  for HDMI transmitter

> The adv7511 on the Koelsch board has a 12 MHz fixed clock
> for the CEC block. Specify this in the dts to enable CEC support.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] arm: dts: renesas: add cec clock for Koelsch board

[Simon Horman]

On Mon, Aug 14, 2017 at 05:34:41PM +0200, Geert Uytterhoeven wrote:
> On Sun, Jul 30, 2017 at 3:07 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Probably the one-line summary should be
> 
>     ARM: dts: koelsch: Add CEC clock  for HDMI transmitter
> 
> > The adv7511 on the Koelsch board has a 12 MHz fixed clock
> > for the CEC block. Specify this in the dts to enable CEC support.
> >
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks, I have applied this patch with the updated one-line summary.

[PATCH 4/4] drm: adv7511/33: add HDMI CEC support

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Add support for HDMI CEC to the drm adv7511/adv7533 drivers.

The CEC registers that we need to use are identical for both drivers,
but they appear at different offsets in the register map.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/gpu/drm/bridge/adv7511/Kconfig       |   8 +
 drivers/gpu/drm/bridge/adv7511/Makefile      |   1 +
 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  45 +++-
 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 314 +++++++++++++++++++++++++++
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 152 +++++++++++--
 drivers/gpu/drm/bridge/adv7511/adv7533.c     |  30 +--
 6 files changed, 500 insertions(+), 50 deletions(-)
 create mode 100644 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c

diff --git a/drivers/gpu/drm/bridge/adv7511/Kconfig b/drivers/gpu/drm/bridge/adv7511/Kconfig
index 2fed567f9943..592b9d2ec034 100644
--- a/drivers/gpu/drm/bridge/adv7511/Kconfig
+++ b/drivers/gpu/drm/bridge/adv7511/Kconfig
@@ -21,3 +21,11 @@ config DRM_I2C_ADV7533
 	default y
 	help
 	  Support for the Analog Devices ADV7533 DSI to HDMI encoder.
+
+config DRM_I2C_ADV7511_CEC
+	bool "ADV7511/33 HDMI CEC driver"
+	depends on DRM_I2C_ADV7511
+	select CEC_CORE
+	default y
+	help
+	  When selected the HDMI transmitter will support the CEC feature.
diff --git a/drivers/gpu/drm/bridge/adv7511/Makefile b/drivers/gpu/drm/bridge/adv7511/Makefile
index 5ba675534f6e..5bb384938a71 100644
--- a/drivers/gpu/drm/bridge/adv7511/Makefile
+++ b/drivers/gpu/drm/bridge/adv7511/Makefile
@@ -1,4 +1,5 @@
 adv7511-y := adv7511_drv.o
 adv7511-$(CONFIG_DRM_I2C_ADV7511_AUDIO) += adv7511_audio.o
+adv7511-$(CONFIG_DRM_I2C_ADV7511_CEC) += adv7511_cec.o
 adv7511-$(CONFIG_DRM_I2C_ADV7533) += adv7533.o
 obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511.o
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index fe18a5d2d84b..4fd7b14f619b 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -195,6 +195,25 @@
 #define ADV7511_PACKET_GM(x)	    ADV7511_PACKET(5, x)
 #define ADV7511_PACKET_SPARE(x)	    ADV7511_PACKET(6, x)
 
+#define ADV7511_REG_CEC_TX_FRAME_HDR	0x00
+#define ADV7511_REG_CEC_TX_FRAME_DATA0	0x01
+#define ADV7511_REG_CEC_TX_FRAME_LEN	0x10
+#define ADV7511_REG_CEC_TX_ENABLE	0x11
+#define ADV7511_REG_CEC_TX_RETRY	0x12
+#define ADV7511_REG_CEC_TX_LOW_DRV_CNT	0x14
+#define ADV7511_REG_CEC_RX_FRAME_HDR	0x15
+#define ADV7511_REG_CEC_RX_FRAME_DATA0	0x16
+#define ADV7511_REG_CEC_RX_FRAME_LEN	0x25
+#define ADV7511_REG_CEC_RX_ENABLE	0x26
+#define ADV7511_REG_CEC_RX_BUFFERS	0x4a
+#define ADV7511_REG_CEC_LOG_ADDR_MASK	0x4b
+#define ADV7511_REG_CEC_LOG_ADDR_0_1	0x4c
+#define ADV7511_REG_CEC_LOG_ADDR_2	0x4d
+#define ADV7511_REG_CEC_CLK_DIV		0x4e
+#define ADV7511_REG_CEC_SOFT_RESET	0x50
+
+#define ADV7533_REG_CEC_OFFSET		0x70
+
 enum adv7511_input_clock {
 	ADV7511_INPUT_CLOCK_1X,
 	ADV7511_INPUT_CLOCK_2X,
@@ -297,6 +316,8 @@ enum adv7511_type {
 	ADV7533,
 };
 
+#define ADV7511_MAX_ADDRS 3
+
 struct adv7511 {
 	struct i2c_client *i2c_main;
 	struct i2c_client *i2c_edid;
@@ -343,15 +364,29 @@ struct adv7511 {
 
 	enum adv7511_type type;
 	struct platform_device *audio_pdev;
+
+	struct cec_adapter *cec_adap;
+	u8   cec_addr[ADV7511_MAX_ADDRS];
+	u8   cec_valid_addrs;
+	bool cec_enabled_adap;
+	struct clk *cec_clk;
+	u32 cec_clk_freq;
 };
 
+#ifdef CONFIG_DRM_I2C_ADV7511_CEC
+extern const struct cec_adap_ops adv7511_cec_adap_ops;
+
+void adv7511_cec_init(struct adv7511 *adv7511, unsigned int offset);
+int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511);
+void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1);
+#endif
+
 #ifdef CONFIG_DRM_I2C_ADV7533
 void adv7533_dsi_power_on(struct adv7511 *adv);
 void adv7533_dsi_power_off(struct adv7511 *adv);
 void adv7533_mode_set(struct adv7511 *adv, struct drm_display_mode *mode);
 int adv7533_patch_registers(struct adv7511 *adv);
-void adv7533_uninit_cec(struct adv7511 *adv);
-int adv7533_init_cec(struct adv7511 *adv);
+int adv7533_patch_cec_registers(struct adv7511 *adv);
 int adv7533_attach_dsi(struct adv7511 *adv);
 void adv7533_detach_dsi(struct adv7511 *adv);
 int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv);
@@ -374,11 +409,7 @@ static inline int adv7533_patch_registers(struct adv7511 *adv)
 	return -ENODEV;
 }
 
-static inline void adv7533_uninit_cec(struct adv7511 *adv)
-{
-}
-
-static inline int adv7533_init_cec(struct adv7511 *adv)
+static inline int adv7533_patch_cec_registers(struct adv7511 *adv)
 {
 	return -ENODEV;
 }
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
new file mode 100644
index 000000000000..74081cbfb5db
--- /dev/null
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
@@ -0,0 +1,314 @@
+/*
+ * adv7511_cec.c - Analog Devices ADV7511/33 cec driver
+ *
+ * Copyright 2017 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
+ *
+ * This program is free software; you may redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/slab.h>
+#include <linux/clk.h>
+
+#include <media/cec.h>
+
+#include "adv7511.h"
+
+#define ADV7511_INT1_CEC_MASK \
+	(ADV7511_INT1_CEC_TX_READY | ADV7511_INT1_CEC_TX_ARBIT_LOST | \
+	 ADV7511_INT1_CEC_TX_RETRY_TIMEOUT | ADV7511_INT1_CEC_RX_READY1)
+
+static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
+{
+	unsigned int offset = adv7511->type == ADV7533 ?
+					ADV7533_REG_CEC_OFFSET : 0;
+	unsigned int val;
+
+	if (regmap_read(adv7511->regmap_cec,
+			ADV7511_REG_CEC_TX_ENABLE + offset, &val))
+		return;
+
+	if ((val & 0x01) == 0)
+		return;
+
+	if (tx_raw_status & 0x10) {
+		cec_transmit_attempt_done(adv7511->cec_adap,
+					  CEC_TX_STATUS_ARB_LOST);
+		return;
+	}
+	if (tx_raw_status & 0x08) {
+		u8 status;
+		u8 err_cnt = 0;
+		u8 nack_cnt = 0;
+		u8 low_drive_cnt = 0;
+		unsigned int cnt;
+
+		/*
+		 * We set this status bit since this hardware performs
+		 * retransmissions.
+		 */
+		status = CEC_TX_STATUS_MAX_RETRIES;
+		if (regmap_read(adv7511->regmap_cec,
+			    ADV7511_REG_CEC_TX_LOW_DRV_CNT + offset, &cnt)) {
+			err_cnt = 1;
+			status |= CEC_TX_STATUS_ERROR;
+		} else {
+			nack_cnt = cnt & 0xf;
+			if (nack_cnt)
+				status |= CEC_TX_STATUS_NACK;
+			low_drive_cnt = cnt >> 4;
+			if (low_drive_cnt)
+				status |= CEC_TX_STATUS_LOW_DRIVE;
+		}
+		cec_transmit_done(adv7511->cec_adap, status,
+				  0, nack_cnt, low_drive_cnt, err_cnt);
+		return;
+	}
+	if (tx_raw_status & 0x20) {
+		cec_transmit_attempt_done(adv7511->cec_adap, CEC_TX_STATUS_OK);
+		return;
+	}
+}
+
+void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
+{
+	unsigned int offset = adv7511->type == ADV7533 ?
+					ADV7533_REG_CEC_OFFSET : 0;
+	struct cec_msg msg = {};
+	unsigned int len;
+	unsigned int val;
+	u8 i;
+
+	if (irq1 & 0x38)
+		adv_cec_tx_raw_status(adv7511, irq1);
+
+	if (!(irq1 & 1))
+		return;
+
+	if (regmap_read(adv7511->regmap_cec,
+			ADV7511_REG_CEC_RX_FRAME_LEN + offset, &len))
+		return;
+
+	msg.len = len & 0x1f;
+
+	if (msg.len > 16)
+		msg.len = 16;
+
+	if (!msg.len)
+		return;
+
+	for (i = 0; i < msg.len; i++) {
+		regmap_read(adv7511->regmap_cec,
+			    i + ADV7511_REG_CEC_RX_FRAME_HDR + offset, &val);
+		msg.msg[i] = val;
+	}
+
+	/* toggle to re-enable rx 1 */
+	regmap_write(adv7511->regmap_cec,
+		     ADV7511_REG_CEC_RX_BUFFERS + offset, 1);
+	regmap_write(adv7511->regmap_cec,
+		     ADV7511_REG_CEC_RX_BUFFERS + offset, 0);
+	cec_received_msg(adv7511->cec_adap, &msg);
+}
+
+static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
+{
+	struct adv7511 *adv7511 = cec_get_drvdata(adap);
+	unsigned int offset = adv7511->type == ADV7533 ?
+					ADV7533_REG_CEC_OFFSET : 0;
+
+	if (adv7511->i2c_cec == NULL)
+		return -EIO;
+
+	if (!adv7511->cec_enabled_adap && enable) {
+		/* power up cec section */
+		regmap_update_bits(adv7511->regmap_cec,
+				   ADV7511_REG_CEC_CLK_DIV + offset,
+				   0x03, 0x01);
+		/* legacy mode and clear all rx buffers */
+		regmap_write(adv7511->regmap_cec,
+			     ADV7511_REG_CEC_RX_BUFFERS + offset, 0x07);
+		regmap_write(adv7511->regmap_cec,
+			     ADV7511_REG_CEC_RX_BUFFERS + offset, 0);
+		/* initially disable tx */
+		regmap_update_bits(adv7511->regmap_cec,
+				   ADV7511_REG_CEC_TX_ENABLE + offset, 1, 0);
+		/* enabled irqs: */
+		/* tx: ready */
+		/* tx: arbitration lost */
+		/* tx: retry timeout */
+		/* rx: ready 1 */
+		regmap_update_bits(adv7511->regmap,
+				   ADV7511_REG_INT_ENABLE(1), 0x3f,
+				   ADV7511_INT1_CEC_MASK);
+	} else if (adv7511->cec_enabled_adap && !enable) {
+		regmap_update_bits(adv7511->regmap,
+				   ADV7511_REG_INT_ENABLE(1), 0x3f, 0);
+		/* disable address mask 1-3 */
+		regmap_update_bits(adv7511->regmap_cec,
+				   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
+				   0x70, 0x00);
+		/* power down cec section */
+		regmap_update_bits(adv7511->regmap_cec,
+				   ADV7511_REG_CEC_CLK_DIV + offset,
+				   0x03, 0x00);
+		adv7511->cec_valid_addrs = 0;
+	}
+	adv7511->cec_enabled_adap = enable;
+	return 0;
+}
+
+static int adv7511_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
+{
+	struct adv7511 *adv7511 = cec_get_drvdata(adap);
+	unsigned int offset = adv7511->type == ADV7533 ?
+					ADV7533_REG_CEC_OFFSET : 0;
+	unsigned int i, free_idx = ADV7511_MAX_ADDRS;
+
+	if (!adv7511->cec_enabled_adap)
+		return addr == CEC_LOG_ADDR_INVALID ? 0 : -EIO;
+
+	if (addr == CEC_LOG_ADDR_INVALID) {
+		regmap_update_bits(adv7511->regmap_cec,
+				   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
+				   0x70, 0);
+		adv7511->cec_valid_addrs = 0;
+		return 0;
+	}
+
+	for (i = 0; i < ADV7511_MAX_ADDRS; i++) {
+		bool is_valid = adv7511->cec_valid_addrs & (1 << i);
+
+		if (free_idx == ADV7511_MAX_ADDRS && !is_valid)
+			free_idx = i;
+		if (is_valid && adv7511->cec_addr[i] == addr)
+			return 0;
+	}
+	if (i == ADV7511_MAX_ADDRS) {
+		i = free_idx;
+		if (i == ADV7511_MAX_ADDRS)
+			return -ENXIO;
+	}
+	adv7511->cec_addr[i] = addr;
+	adv7511->cec_valid_addrs |= 1 << i;
+
+	switch (i) {
+	case 0:
+		/* enable address mask 0 */
+		regmap_update_bits(adv7511->regmap_cec,
+				   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
+				   0x10, 0x10);
+		/* set address for mask 0 */
+		regmap_update_bits(adv7511->regmap_cec,
+				   ADV7511_REG_CEC_LOG_ADDR_0_1 + offset,
+				   0x0f, addr);
+		break;
+	case 1:
+		/* enable address mask 1 */
+		regmap_update_bits(adv7511->regmap_cec,
+				   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
+				   0x20, 0x20);
+		/* set address for mask 1 */
+		regmap_update_bits(adv7511->regmap_cec,
+				   ADV7511_REG_CEC_LOG_ADDR_0_1 + offset,
+				   0xf0, addr << 4);
+		break;
+	case 2:
+		/* enable address mask 2 */
+		regmap_update_bits(adv7511->regmap_cec,
+				   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
+				   0x40, 0x40);
+		/* set address for mask 1 */
+		regmap_update_bits(adv7511->regmap_cec,
+				   ADV7511_REG_CEC_LOG_ADDR_2 + offset,
+				   0x0f, addr);
+		break;
+	}
+	return 0;
+}
+
+static int adv7511_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
+				     u32 signal_free_time, struct cec_msg *msg)
+{
+	struct adv7511 *adv7511 = cec_get_drvdata(adap);
+	unsigned int offset = adv7511->type == ADV7533 ?
+					ADV7533_REG_CEC_OFFSET : 0;
+	u8 len = msg->len;
+	unsigned int i;
+
+	/*
+	 * The number of retries is the number of attempts - 1, but retry
+	 * at least once. It's not clear if a value of 0 is allowed, so
+	 * let's do at least one retry.
+	 */
+	regmap_update_bits(adv7511->regmap_cec,
+			   ADV7511_REG_CEC_TX_RETRY + offset,
+			   0x70, max(1, attempts - 1) << 4);
+
+	/* blocking, clear cec tx irq status */
+	regmap_update_bits(adv7511->regmap, ADV7511_REG_INT(1), 0x38, 0x38);
+
+	/* write data */
+	for (i = 0; i < len; i++)
+		regmap_write(adv7511->regmap_cec, i + offset, msg->msg[i]);
+
+	/* set length (data + header) */
+	regmap_write(adv7511->regmap_cec,
+		     ADV7511_REG_CEC_TX_FRAME_LEN + offset, len);
+	/* start transmit, enable tx */
+	regmap_write(adv7511->regmap_cec,
+		     ADV7511_REG_CEC_TX_ENABLE + offset, 0x01);
+	return 0;
+}
+
+const struct cec_adap_ops adv7511_cec_adap_ops = {
+	.adap_enable = adv7511_cec_adap_enable,
+	.adap_log_addr = adv7511_cec_adap_log_addr,
+	.adap_transmit = adv7511_cec_adap_transmit,
+};
+
+void adv7511_cec_init(struct adv7511 *adv7511, unsigned int offset)
+{
+	regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, 0);
+	/* cec soft reset */
+	regmap_write(adv7511->regmap_cec,
+		     ADV7511_REG_CEC_SOFT_RESET + offset, 0x01);
+	regmap_write(adv7511->regmap_cec,
+		     ADV7511_REG_CEC_SOFT_RESET + offset, 0x00);
+
+	/* legacy mode */
+	regmap_write(adv7511->regmap_cec,
+		     ADV7511_REG_CEC_RX_BUFFERS + offset, 0x00);
+
+	regmap_write(adv7511->regmap_cec,
+		     ADV7511_REG_CEC_CLK_DIV + offset,
+		     ((adv7511->cec_clk_freq / 750000) - 1) << 2);
+}
+
+int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511)
+{
+	adv7511->cec_clk = devm_clk_get(dev, "cec");
+	if (IS_ERR(adv7511->cec_clk)) {
+		int ret = PTR_ERR(adv7511->cec_clk);
+
+		adv7511->cec_clk = NULL;
+		return ret;
+	}
+	clk_prepare_enable(adv7511->cec_clk);
+	adv7511->cec_clk_freq = clk_get_rate(adv7511->cec_clk);
+	return 0;
+}
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index f75ab6278113..1bef33e99358 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -11,12 +11,15 @@
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/slab.h>
+#include <linux/clk.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_edid.h>
 
+#include <media/cec.h>
+
 #include "adv7511.h"
 
 /* ADI recommended values for proper operation. */
@@ -339,8 +342,10 @@ static void __adv7511_power_on(struct adv7511 *adv7511)
 		 */
 		regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(0),
 			     ADV7511_INT0_EDID_READY | ADV7511_INT0_HPD);
-		regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(1),
-			     ADV7511_INT1_DDC_ERROR);
+		regmap_update_bits(adv7511->regmap,
+				   ADV7511_REG_INT_ENABLE(1),
+				   ADV7511_INT1_DDC_ERROR,
+				   ADV7511_INT1_DDC_ERROR);
 	}
 
 	/*
@@ -376,6 +381,9 @@ static void __adv7511_power_off(struct adv7511 *adv7511)
 	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
 			   ADV7511_POWER_POWER_DOWN,
 			   ADV7511_POWER_POWER_DOWN);
+	regmap_update_bits(adv7511->regmap,
+			   ADV7511_REG_INT_ENABLE(1),
+			   ADV7511_INT1_DDC_ERROR, 0);
 	regcache_mark_dirty(adv7511->regmap);
 }
 
@@ -426,6 +434,8 @@ static void adv7511_hpd_work(struct work_struct *work)
 
 	if (adv7511->connector.status != status) {
 		adv7511->connector.status = status;
+		if (status == connector_status_disconnected)
+			cec_phys_addr_invalidate(adv7511->cec_adap);
 		drm_kms_helper_hotplug_event(adv7511->connector.dev);
 	}
 }
@@ -456,6 +466,10 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
 			wake_up_all(&adv7511->wq);
 	}
 
+#ifdef CONFIG_DRM_I2C_ADV7511_CEC
+	adv7511_cec_irq_process(adv7511, irq1);
+#endif
+
 	return 0;
 }
 
@@ -599,6 +613,8 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
 
 	adv7511_set_config_csc(adv7511, connector, adv7511->rgb);
 
+	cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);
+
 	return count;
 }
 
@@ -920,6 +936,84 @@ static void adv7511_uninit_regulators(struct adv7511 *adv)
 	regulator_bulk_disable(adv->num_supplies, adv->supplies);
 }
 
+static bool adv7533_cec_register_volatile(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case ADV7511_REG_CEC_RX_FRAME_HDR + ADV7533_REG_CEC_OFFSET:
+	case ADV7511_REG_CEC_RX_FRAME_DATA0 + ADV7533_REG_CEC_OFFSET...
+		ADV7511_REG_CEC_RX_FRAME_DATA0 + ADV7533_REG_CEC_OFFSET + 14:
+	case ADV7511_REG_CEC_RX_FRAME_LEN + ADV7533_REG_CEC_OFFSET:
+	case ADV7511_REG_CEC_RX_BUFFERS + ADV7533_REG_CEC_OFFSET:
+	case ADV7511_REG_CEC_TX_LOW_DRV_CNT + ADV7533_REG_CEC_OFFSET:
+		return true;
+	}
+
+	return false;
+}
+
+static const struct regmap_config adv7533_cec_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = 0xff,
+	.cache_type = REGCACHE_RBTREE,
+	.volatile_reg = adv7533_cec_register_volatile,
+};
+
+static bool adv7511_cec_register_volatile(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case ADV7511_REG_CEC_RX_FRAME_HDR:
+	case ADV7511_REG_CEC_RX_FRAME_DATA0...
+		ADV7511_REG_CEC_RX_FRAME_DATA0 + 14:
+	case ADV7511_REG_CEC_RX_FRAME_LEN:
+	case ADV7511_REG_CEC_RX_BUFFERS:
+	case ADV7511_REG_CEC_TX_LOW_DRV_CNT:
+		return true;
+	}
+
+	return false;
+}
+
+static const struct regmap_config adv7511_cec_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = 0xff,
+	.cache_type = REGCACHE_RBTREE,
+	.volatile_reg = adv7511_cec_register_volatile,
+};
+
+static int adv7511_init_cec_regmap(struct adv7511 *adv)
+{
+	int ret;
+
+	adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
+				     adv->i2c_main->addr - 1);
+	if (!adv->i2c_cec)
+		return -ENOMEM;
+
+	adv->regmap_cec = devm_regmap_init_i2c(adv->i2c_cec,
+					       adv->type == ADV7533 ?
+					       &adv7533_cec_regmap_config :
+					&adv7511_cec_regmap_config);
+	if (IS_ERR(adv->regmap_cec)) {
+		ret = PTR_ERR(adv->regmap_cec);
+		goto err;
+	}
+
+	if (adv->type == ADV7533) {
+		ret = adv7533_patch_cec_registers(adv);
+		if (ret)
+			goto err;
+	}
+
+	return 0;
+err:
+	i2c_unregister_device(adv->i2c_cec);
+	return ret;
+}
+
 static int adv7511_parse_dt(struct device_node *np,
 			    struct adv7511_link_config *config)
 {
@@ -1010,6 +1104,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 	struct device *dev = &i2c->dev;
 	unsigned int main_i2c_addr = i2c->addr << 1;
 	unsigned int edid_i2c_addr = main_i2c_addr + 4;
+	unsigned int offset;
 	unsigned int val;
 	int ret;
 
@@ -1035,6 +1130,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 		ret = adv7511_parse_dt(dev->of_node, &link_config);
 	else
 		ret = adv7533_parse_dt(dev->of_node, adv7511);
+
 	if (ret)
 		return ret;
 
@@ -1093,11 +1189,9 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 		goto uninit_regulators;
 	}
 
-	if (adv7511->type == ADV7533) {
-		ret = adv7533_init_cec(adv7511);
-		if (ret)
-			goto err_i2c_unregister_edid;
-	}
+	ret = adv7511_init_cec_regmap(adv7511);
+	if (ret)
+		goto err_i2c_unregister_edid;
 
 	INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work);
 
@@ -1112,10 +1206,6 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 			goto err_unregister_cec;
 	}
 
-	/* CEC is unused for now */
-	regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL,
-		     ADV7511_CEC_CTRL_POWER_DOWN);
-
 	adv7511_power_off(adv7511);
 
 	i2c_set_clientdata(i2c, adv7511);
@@ -1134,10 +1224,39 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 
 	adv7511_audio_init(dev, adv7511);
 
+	offset = adv7511->type == ADV7533 ? ADV7533_REG_CEC_OFFSET : 0;
+
+#ifdef CONFIG_DRM_I2C_ADV7511_CEC
+	ret = adv7511_cec_parse_dt(dev, adv7511);
+	if (ret)
+		goto err_unregister_cec;
+
+	adv7511->cec_adap = cec_allocate_adapter(&adv7511_cec_adap_ops,
+		adv7511, dev_name(&i2c->dev), CEC_CAP_TRANSMIT |
+		CEC_CAP_LOG_ADDRS | CEC_CAP_PASSTHROUGH | CEC_CAP_RC,
+		ADV7511_MAX_ADDRS);
+	ret = PTR_ERR_OR_ZERO(adv7511->cec_adap);
+	if (ret)
+		goto err_unregister_cec;
+
+	adv7511_cec_init(adv7511, offset);
+
+	ret = cec_register_adapter(adv7511->cec_adap, &i2c->dev);
+	if (ret) {
+		cec_delete_adapter(adv7511->cec_adap);
+		goto err_unregister_cec;
+	}
+#else
+	regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
+		     ADV7511_CEC_CTRL_POWER_DOWN);
+#endif
+
 	return 0;
 
 err_unregister_cec:
-	adv7533_uninit_cec(adv7511);
+	i2c_unregister_device(adv7511->i2c_cec);
+	if (adv7511->cec_clk)
+		clk_disable_unprepare(adv7511->cec_clk);
 err_i2c_unregister_edid:
 	i2c_unregister_device(adv7511->i2c_edid);
 uninit_regulators:
@@ -1150,10 +1269,11 @@ static int adv7511_remove(struct i2c_client *i2c)
 {
 	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
 
-	if (adv7511->type == ADV7533) {
+	if (adv7511->type == ADV7533)
 		adv7533_detach_dsi(adv7511);
-		adv7533_uninit_cec(adv7511);
-	}
+	i2c_unregister_device(adv7511->i2c_cec);
+	if (adv7511->cec_clk)
+		clk_disable_unprepare(adv7511->cec_clk);
 
 	adv7511_uninit_regulators(adv7511);
 
@@ -1161,6 +1281,8 @@ static int adv7511_remove(struct i2c_client *i2c)
 
 	adv7511_audio_exit(adv7511);
 
+	cec_unregister_adapter(adv7511->cec_adap);
+
 	i2c_unregister_device(adv7511->i2c_edid);
 
 	kfree(adv7511->edid);
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c
index ac804f81e2f6..0e173abb913c 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7533.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c
@@ -145,37 +145,11 @@ int adv7533_patch_registers(struct adv7511 *adv)
 				     ARRAY_SIZE(adv7533_fixed_registers));
 }
 
-void adv7533_uninit_cec(struct adv7511 *adv)
+int adv7533_patch_cec_registers(struct adv7511 *adv)
 {
-	i2c_unregister_device(adv->i2c_cec);
-}
-
-int adv7533_init_cec(struct adv7511 *adv)
-{
-	int ret;
-
-	adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
-				     adv->i2c_main->addr - 1);
-	if (!adv->i2c_cec)
-		return -ENOMEM;
-
-	adv->regmap_cec = devm_regmap_init_i2c(adv->i2c_cec,
-					&adv7533_cec_regmap_config);
-	if (IS_ERR(adv->regmap_cec)) {
-		ret = PTR_ERR(adv->regmap_cec);
-		goto err;
-	}
-
-	ret = regmap_register_patch(adv->regmap_cec,
+	return regmap_register_patch(adv->regmap_cec,
 				    adv7533_cec_fixed_registers,
 				    ARRAY_SIZE(adv7533_cec_fixed_registers));
-	if (ret)
-		goto err;
-
-	return 0;
-err:
-	adv7533_uninit_cec(adv);
-	return ret;
 }
 
 int adv7533_attach_dsi(struct adv7511 *adv)
-- 
2.13.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm: adv7511/33: add HDMI CEC support

[Archit Taneja]

On 07/30/2017 06:37 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Add support for HDMI CEC to the drm adv7511/adv7533 drivers.
> 
> The CEC registers that we need to use are identical for both drivers,
> but they appear at different offsets in the register map.

Thanks for the patch. Some minor comments below.

> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>   drivers/gpu/drm/bridge/adv7511/Kconfig       |   8 +
>   drivers/gpu/drm/bridge/adv7511/Makefile      |   1 +
>   drivers/gpu/drm/bridge/adv7511/adv7511.h     |  45 +++-
>   drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 314 +++++++++++++++++++++++++++
>   drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 152 +++++++++++--
>   drivers/gpu/drm/bridge/adv7511/adv7533.c     |  30 +--
>   6 files changed, 500 insertions(+), 50 deletions(-)
>   create mode 100644 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/Kconfig b/drivers/gpu/drm/bridge/adv7511/Kconfig
> index 2fed567f9943..592b9d2ec034 100644
> --- a/drivers/gpu/drm/bridge/adv7511/Kconfig
> +++ b/drivers/gpu/drm/bridge/adv7511/Kconfig
> @@ -21,3 +21,11 @@ config DRM_I2C_ADV7533
>   	default y
>   	help
>   	  Support for the Analog Devices ADV7533 DSI to HDMI encoder.
> +
> +config DRM_I2C_ADV7511_CEC
> +	bool "ADV7511/33 HDMI CEC driver"
> +	depends on DRM_I2C_ADV7511
> +	select CEC_CORE
> +	default y
> +	help
> +	  When selected the HDMI transmitter will support the CEC feature.
> diff --git a/drivers/gpu/drm/bridge/adv7511/Makefile b/drivers/gpu/drm/bridge/adv7511/Makefile
> index 5ba675534f6e..5bb384938a71 100644
> --- a/drivers/gpu/drm/bridge/adv7511/Makefile
> +++ b/drivers/gpu/drm/bridge/adv7511/Makefile
> @@ -1,4 +1,5 @@
>   adv7511-y := adv7511_drv.o
>   adv7511-$(CONFIG_DRM_I2C_ADV7511_AUDIO) += adv7511_audio.o
> +adv7511-$(CONFIG_DRM_I2C_ADV7511_CEC) += adv7511_cec.o
>   adv7511-$(CONFIG_DRM_I2C_ADV7533) += adv7533.o
>   obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511.o
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> index fe18a5d2d84b..4fd7b14f619b 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -195,6 +195,25 @@
>   #define ADV7511_PACKET_GM(x)	    ADV7511_PACKET(5, x)
>   #define ADV7511_PACKET_SPARE(x)	    ADV7511_PACKET(6, x)
>   
> +#define ADV7511_REG_CEC_TX_FRAME_HDR	0x00
> +#define ADV7511_REG_CEC_TX_FRAME_DATA0	0x01
> +#define ADV7511_REG_CEC_TX_FRAME_LEN	0x10
> +#define ADV7511_REG_CEC_TX_ENABLE	0x11
> +#define ADV7511_REG_CEC_TX_RETRY	0x12
> +#define ADV7511_REG_CEC_TX_LOW_DRV_CNT	0x14
> +#define ADV7511_REG_CEC_RX_FRAME_HDR	0x15
> +#define ADV7511_REG_CEC_RX_FRAME_DATA0	0x16
> +#define ADV7511_REG_CEC_RX_FRAME_LEN	0x25
> +#define ADV7511_REG_CEC_RX_ENABLE	0x26
> +#define ADV7511_REG_CEC_RX_BUFFERS	0x4a
> +#define ADV7511_REG_CEC_LOG_ADDR_MASK	0x4b
> +#define ADV7511_REG_CEC_LOG_ADDR_0_1	0x4c
> +#define ADV7511_REG_CEC_LOG_ADDR_2	0x4d
> +#define ADV7511_REG_CEC_CLK_DIV		0x4e
> +#define ADV7511_REG_CEC_SOFT_RESET	0x50
> +
> +#define ADV7533_REG_CEC_OFFSET		0x70
> +
>   enum adv7511_input_clock {
>   	ADV7511_INPUT_CLOCK_1X,
>   	ADV7511_INPUT_CLOCK_2X,
> @@ -297,6 +316,8 @@ enum adv7511_type {
>   	ADV7533,
>   };
>   
> +#define ADV7511_MAX_ADDRS 3
> +
>   struct adv7511 {
>   	struct i2c_client *i2c_main;
>   	struct i2c_client *i2c_edid;
> @@ -343,15 +364,29 @@ struct adv7511 {
>   
>   	enum adv7511_type type;
>   	struct platform_device *audio_pdev;
> +
> +	struct cec_adapter *cec_adap;
> +	u8   cec_addr[ADV7511_MAX_ADDRS];
> +	u8   cec_valid_addrs;
> +	bool cec_enabled_adap;
> +	struct clk *cec_clk;
> +	u32 cec_clk_freq;
>   };
>   
> +#ifdef CONFIG_DRM_I2C_ADV7511_CEC
> +extern const struct cec_adap_ops adv7511_cec_adap_ops;
> +
> +void adv7511_cec_init(struct adv7511 *adv7511, unsigned int offset);
> +int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511);
> +void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1);
> +#endif
> +
>   #ifdef CONFIG_DRM_I2C_ADV7533
>   void adv7533_dsi_power_on(struct adv7511 *adv);
>   void adv7533_dsi_power_off(struct adv7511 *adv);
>   void adv7533_mode_set(struct adv7511 *adv, struct drm_display_mode *mode);
>   int adv7533_patch_registers(struct adv7511 *adv);
> -void adv7533_uninit_cec(struct adv7511 *adv);
> -int adv7533_init_cec(struct adv7511 *adv);
> +int adv7533_patch_cec_registers(struct adv7511 *adv);
>   int adv7533_attach_dsi(struct adv7511 *adv);
>   void adv7533_detach_dsi(struct adv7511 *adv);
>   int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv);
> @@ -374,11 +409,7 @@ static inline int adv7533_patch_registers(struct adv7511 *adv)
>   	return -ENODEV;
>   }
>   
> -static inline void adv7533_uninit_cec(struct adv7511 *adv)
> -{
> -}
> -
> -static inline int adv7533_init_cec(struct adv7511 *adv)
> +static inline int adv7533_patch_cec_registers(struct adv7511 *adv)
>   {
>   	return -ENODEV;
>   }
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> new file mode 100644
> index 000000000000..74081cbfb5db
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> @@ -0,0 +1,314 @@
> +/*
> + * adv7511_cec.c - Analog Devices ADV7511/33 cec driver
> + *
> + * Copyright 2017 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
> + *
> + * This program is free software; you may redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + *
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +
> +#include <media/cec.h>
> +
> +#include "adv7511.h"
> +
> +#define ADV7511_INT1_CEC_MASK \
> +	(ADV7511_INT1_CEC_TX_READY | ADV7511_INT1_CEC_TX_ARBIT_LOST | \
> +	 ADV7511_INT1_CEC_TX_RETRY_TIMEOUT | ADV7511_INT1_CEC_RX_READY1)
> +
> +static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
> +{
> +	unsigned int offset = adv7511->type == ADV7533 ?
> +					ADV7533_REG_CEC_OFFSET : 0;
> +	unsigned int val;
> +
> +	if (regmap_read(adv7511->regmap_cec,
> +			ADV7511_REG_CEC_TX_ENABLE + offset, &val))
> +		return;
> +
> +	if ((val & 0x01) == 0)
> +		return;
> +
> +	if (tx_raw_status & 0x10) {

Should we try to use IRQ1 masks (ADV7511_INT1_CEC_TX_ARBIT_LOST here) to make the
code more legible?

Same comments for the rest of this func and adv7511_cec_irq_process below.

> +		cec_transmit_attempt_done(adv7511->cec_adap,
> +					  CEC_TX_STATUS_ARB_LOST);
> +		return;
> +	}
> +	if (tx_raw_status & 0x08) {
> +		u8 status;
> +		u8 err_cnt = 0;
> +		u8 nack_cnt = 0;
> +		u8 low_drive_cnt = 0;
> +		unsigned int cnt;
> +
> +		/*
> +		 * We set this status bit since this hardware performs
> +		 * retransmissions.
> +		 */
> +		status = CEC_TX_STATUS_MAX_RETRIES;
> +		if (regmap_read(adv7511->regmap_cec,
> +			    ADV7511_REG_CEC_TX_LOW_DRV_CNT + offset, &cnt)) {
> +			err_cnt = 1;
> +			status |= CEC_TX_STATUS_ERROR;
> +		} else {
> +			nack_cnt = cnt & 0xf;
> +			if (nack_cnt)
> +				status |= CEC_TX_STATUS_NACK;
> +			low_drive_cnt = cnt >> 4;
> +			if (low_drive_cnt)
> +				status |= CEC_TX_STATUS_LOW_DRIVE;
> +		}
> +		cec_transmit_done(adv7511->cec_adap, status,
> +				  0, nack_cnt, low_drive_cnt, err_cnt);
> +		return;
> +	}
> +	if (tx_raw_status & 0x20) {
> +		cec_transmit_attempt_done(adv7511->cec_adap, CEC_TX_STATUS_OK);
> +		return;
> +	}
> +}
> +
> +void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
> +{
> +	unsigned int offset = adv7511->type == ADV7533 ?
> +					ADV7533_REG_CEC_OFFSET : 0;
> +	struct cec_msg msg = {};
> +	unsigned int len;
> +	unsigned int val;
> +	u8 i;
> +
> +	if (irq1 & 0x38) > +		adv_cec_tx_raw_status(adv7511, irq1);
> +
> +	if (!(irq1 & 1))
> +		return;
> +
> +	if (regmap_read(adv7511->regmap_cec,
> +			ADV7511_REG_CEC_RX_FRAME_LEN + offset, &len))
> +		return;
> +
> +	msg.len = len & 0x1f;
> +
> +	if (msg.len > 16)
> +		msg.len = 16;
> +
> +	if (!msg.len)
> +		return;
> +
> +	for (i = 0; i < msg.len; i++) {
> +		regmap_read(adv7511->regmap_cec,
> +			    i + ADV7511_REG_CEC_RX_FRAME_HDR + offset, &val);
> +		msg.msg[i] = val;
> +	}
> +
> +	/* toggle to re-enable rx 1 */
> +	regmap_write(adv7511->regmap_cec,
> +		     ADV7511_REG_CEC_RX_BUFFERS + offset, 1);
> +	regmap_write(adv7511->regmap_cec,
> +		     ADV7511_REG_CEC_RX_BUFFERS + offset, 0);
> +	cec_received_msg(adv7511->cec_adap, &msg);
> +}
> +
> +static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
> +{
> +	struct adv7511 *adv7511 = cec_get_drvdata(adap);
> +	unsigned int offset = adv7511->type == ADV7533 ?
> +					ADV7533_REG_CEC_OFFSET : 0;
> +
> +	if (adv7511->i2c_cec == NULL)
> +		return -EIO;
> +
> +	if (!adv7511->cec_enabled_adap && enable) {
> +		/* power up cec section */
> +		regmap_update_bits(adv7511->regmap_cec,
> +				   ADV7511_REG_CEC_CLK_DIV + offset,
> +				   0x03, 0x01);
> +		/* legacy mode and clear all rx buffers */
> +		regmap_write(adv7511->regmap_cec,
> +			     ADV7511_REG_CEC_RX_BUFFERS + offset, 0x07);
> +		regmap_write(adv7511->regmap_cec,
> +			     ADV7511_REG_CEC_RX_BUFFERS + offset, 0);
> +		/* initially disable tx */
> +		regmap_update_bits(adv7511->regmap_cec,
> +				   ADV7511_REG_CEC_TX_ENABLE + offset, 1, 0);
> +		/* enabled irqs: */
> +		/* tx: ready */
> +		/* tx: arbitration lost */
> +		/* tx: retry timeout */
> +		/* rx: ready 1 */
> +		regmap_update_bits(adv7511->regmap,
> +				   ADV7511_REG_INT_ENABLE(1), 0x3f,
> +				   ADV7511_INT1_CEC_MASK);
> +	} else if (adv7511->cec_enabled_adap && !enable) {
> +		regmap_update_bits(adv7511->regmap,
> +				   ADV7511_REG_INT_ENABLE(1), 0x3f, 0);
> +		/* disable address mask 1-3 */
> +		regmap_update_bits(adv7511->regmap_cec,
> +				   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
> +				   0x70, 0x00);
> +		/* power down cec section */
> +		regmap_update_bits(adv7511->regmap_cec,
> +				   ADV7511_REG_CEC_CLK_DIV + offset,
> +				   0x03, 0x00);
> +		adv7511->cec_valid_addrs = 0;
> +	}
> +	adv7511->cec_enabled_adap = enable;
> +	return 0;
> +}
> +
> +static int adv7511_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
> +{
> +	struct adv7511 *adv7511 = cec_get_drvdata(adap);
> +	unsigned int offset = adv7511->type == ADV7533 ?
> +					ADV7533_REG_CEC_OFFSET : 0;
> +	unsigned int i, free_idx = ADV7511_MAX_ADDRS;
> +
> +	if (!adv7511->cec_enabled_adap)
> +		return addr == CEC_LOG_ADDR_INVALID ? 0 : -EIO;
> +
> +	if (addr == CEC_LOG_ADDR_INVALID) {
> +		regmap_update_bits(adv7511->regmap_cec,
> +				   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
> +				   0x70, 0);
> +		adv7511->cec_valid_addrs = 0;
> +		return 0;
> +	}
> +
> +	for (i = 0; i < ADV7511_MAX_ADDRS; i++) {
> +		bool is_valid = adv7511->cec_valid_addrs & (1 << i);
> +
> +		if (free_idx == ADV7511_MAX_ADDRS && !is_valid)
> +			free_idx = i;
> +		if (is_valid && adv7511->cec_addr[i] == addr)
> +			return 0;
> +	}
> +	if (i == ADV7511_MAX_ADDRS) {
> +		i = free_idx;
> +		if (i == ADV7511_MAX_ADDRS)
> +			return -ENXIO;
> +	}
> +	adv7511->cec_addr[i] = addr;
> +	adv7511->cec_valid_addrs |= 1 << i;
> +
> +	switch (i) {
> +	case 0:
> +		/* enable address mask 0 */
> +		regmap_update_bits(adv7511->regmap_cec,
> +				   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
> +				   0x10, 0x10);
> +		/* set address for mask 0 */
> +		regmap_update_bits(adv7511->regmap_cec,
> +				   ADV7511_REG_CEC_LOG_ADDR_0_1 + offset,
> +				   0x0f, addr);
> +		break;
> +	case 1:
> +		/* enable address mask 1 */
> +		regmap_update_bits(adv7511->regmap_cec,
> +				   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
> +				   0x20, 0x20);
> +		/* set address for mask 1 */
> +		regmap_update_bits(adv7511->regmap_cec,
> +				   ADV7511_REG_CEC_LOG_ADDR_0_1 + offset,
> +				   0xf0, addr << 4);
> +		break;
> +	case 2:
> +		/* enable address mask 2 */
> +		regmap_update_bits(adv7511->regmap_cec,
> +				   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
> +				   0x40, 0x40);
> +		/* set address for mask 1 */
> +		regmap_update_bits(adv7511->regmap_cec,
> +				   ADV7511_REG_CEC_LOG_ADDR_2 + offset,
> +				   0x0f, addr);
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static int adv7511_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> +				     u32 signal_free_time, struct cec_msg *msg)
> +{
> +	struct adv7511 *adv7511 = cec_get_drvdata(adap);
> +	unsigned int offset = adv7511->type == ADV7533 ?
> +					ADV7533_REG_CEC_OFFSET : 0;
> +	u8 len = msg->len;
> +	unsigned int i;
> +
> +	/*
> +	 * The number of retries is the number of attempts - 1, but retry
> +	 * at least once. It's not clear if a value of 0 is allowed, so
> +	 * let's do at least one retry.
> +	 */
> +	regmap_update_bits(adv7511->regmap_cec,
> +			   ADV7511_REG_CEC_TX_RETRY + offset,
> +			   0x70, max(1, attempts - 1) << 4);
> +
> +	/* blocking, clear cec tx irq status */
> +	regmap_update_bits(adv7511->regmap, ADV7511_REG_INT(1), 0x38, 0x38);
> +
> +	/* write data */ > +	for (i = 0; i < len; i++)
> +		regmap_write(adv7511->regmap_cec, i + offset, msg->msg[i]);

Maybe "i + ADV7511_REG_CEC_TX_FRAME_HDR + offset" here for more clarity?

> +
> +	/* set length (data + header) */
> +	regmap_write(adv7511->regmap_cec,
> +		     ADV7511_REG_CEC_TX_FRAME_LEN + offset, len);
> +	/* start transmit, enable tx */
> +	regmap_write(adv7511->regmap_cec,
> +		     ADV7511_REG_CEC_TX_ENABLE + offset, 0x01);
> +	return 0;
> +}
> +
> +const struct cec_adap_ops adv7511_cec_adap_ops = {
> +	.adap_enable = adv7511_cec_adap_enable,
> +	.adap_log_addr = adv7511_cec_adap_log_addr,
> +	.adap_transmit = adv7511_cec_adap_transmit,
> +};
> +
> +void adv7511_cec_init(struct adv7511 *adv7511, unsigned int offset)
> +{
> +	regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, 0);
> +	/* cec soft reset */
> +	regmap_write(adv7511->regmap_cec,
> +		     ADV7511_REG_CEC_SOFT_RESET + offset, 0x01);
> +	regmap_write(adv7511->regmap_cec,
> +		     ADV7511_REG_CEC_SOFT_RESET + offset, 0x00);
> +
> +	/* legacy mode */
> +	regmap_write(adv7511->regmap_cec,
> +		     ADV7511_REG_CEC_RX_BUFFERS + offset, 0x00);
> +
> +	regmap_write(adv7511->regmap_cec,
> +		     ADV7511_REG_CEC_CLK_DIV + offset,
> +		     ((adv7511->cec_clk_freq / 750000) - 1) << 2);
> +}
> +
> +int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511)
> +{
> +	adv7511->cec_clk = devm_clk_get(dev, "cec");
> +	if (IS_ERR(adv7511->cec_clk)) {
> +		int ret = PTR_ERR(adv7511->cec_clk);
> +
> +		adv7511->cec_clk = NULL;
> +		return ret;
> +	}
> +	clk_prepare_enable(adv7511->cec_clk);
> +	adv7511->cec_clk_freq = clk_get_rate(adv7511->cec_clk);
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index f75ab6278113..1bef33e99358 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -11,12 +11,15 @@
>   #include <linux/module.h>
>   #include <linux/of_device.h>
>   #include <linux/slab.h>
> +#include <linux/clk.h>
>   
>   #include <drm/drmP.h>
>   #include <drm/drm_atomic.h>
>   #include <drm/drm_atomic_helper.h>
>   #include <drm/drm_edid.h>
>   
> +#include <media/cec.h>
> +
>   #include "adv7511.h"
>   
>   /* ADI recommended values for proper operation. */
> @@ -339,8 +342,10 @@ static void __adv7511_power_on(struct adv7511 *adv7511)
>   		 */
>   		regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(0),
>   			     ADV7511_INT0_EDID_READY | ADV7511_INT0_HPD);
> -		regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(1),
> -			     ADV7511_INT1_DDC_ERROR);
> +		regmap_update_bits(adv7511->regmap,
> +				   ADV7511_REG_INT_ENABLE(1),
> +				   ADV7511_INT1_DDC_ERROR,
> +				   ADV7511_INT1_DDC_ERROR);
>   	}
>   
>   	/*
> @@ -376,6 +381,9 @@ static void __adv7511_power_off(struct adv7511 *adv7511)
>   	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>   			   ADV7511_POWER_POWER_DOWN,
>   			   ADV7511_POWER_POWER_DOWN);
> +	regmap_update_bits(adv7511->regmap,
> +			   ADV7511_REG_INT_ENABLE(1),
> +			   ADV7511_INT1_DDC_ERROR, 0);
>   	regcache_mark_dirty(adv7511->regmap);
>   }
>   
> @@ -426,6 +434,8 @@ static void adv7511_hpd_work(struct work_struct *work)
>   
>   	if (adv7511->connector.status != status) {
>   		adv7511->connector.status = status;
> +		if (status == connector_status_disconnected)
> +			cec_phys_addr_invalidate(adv7511->cec_adap);
>   		drm_kms_helper_hotplug_event(adv7511->connector.dev);
>   	}
>   }
> @@ -456,6 +466,10 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
>   			wake_up_all(&adv7511->wq);
>   	}
>   
> +#ifdef CONFIG_DRM_I2C_ADV7511_CEC
> +	adv7511_cec_irq_process(adv7511, irq1);
> +#endif
> +
>   	return 0;
>   }
>   
> @@ -599,6 +613,8 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>   
>   	adv7511_set_config_csc(adv7511, connector, adv7511->rgb);
>   
> +	cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);
> +
>   	return count;
>   }
>   
> @@ -920,6 +936,84 @@ static void adv7511_uninit_regulators(struct adv7511 *adv)
>   	regulator_bulk_disable(adv->num_supplies, adv->supplies);
>   }
>   
> +static bool adv7533_cec_register_volatile(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case ADV7511_REG_CEC_RX_FRAME_HDR + ADV7533_REG_CEC_OFFSET:
> +	case ADV7511_REG_CEC_RX_FRAME_DATA0 + ADV7533_REG_CEC_OFFSET...
> +		ADV7511_REG_CEC_RX_FRAME_DATA0 + ADV7533_REG_CEC_OFFSET + 14:
> +	case ADV7511_REG_CEC_RX_FRAME_LEN + ADV7533_REG_CEC_OFFSET:
> +	case ADV7511_REG_CEC_RX_BUFFERS + ADV7533_REG_CEC_OFFSET:
> +	case ADV7511_REG_CEC_TX_LOW_DRV_CNT + ADV7533_REG_CEC_OFFSET:
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static const struct regmap_config adv7533_cec_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +
> +	.max_register = 0xff,
> +	.cache_type = REGCACHE_RBTREE,
> +	.volatile_reg = adv7533_cec_register_volatile,
> +};
> +
> +static bool adv7511_cec_register_volatile(struct device *dev, unsigned int reg)
> +{

Maybe we could combine the two register_volatile() funcs and the remap_config structs
for adv7511 and adv7533 by passing (reg + offset) to switch?

> +	switch (reg) {
> +	case ADV7511_REG_CEC_RX_FRAME_HDR:
> +	case ADV7511_REG_CEC_RX_FRAME_DATA0...
> +		ADV7511_REG_CEC_RX_FRAME_DATA0 + 14:
> +	case ADV7511_REG_CEC_RX_FRAME_LEN:
> +	case ADV7511_REG_CEC_RX_BUFFERS:
> +	case ADV7511_REG_CEC_TX_LOW_DRV_CNT:
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static const struct regmap_config adv7511_cec_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +
> +	.max_register = 0xff,
> +	.cache_type = REGCACHE_RBTREE,
> +	.volatile_reg = adv7511_cec_register_volatile,
> +};
> +
> +static int adv7511_init_cec_regmap(struct adv7511 *adv)
> +{
> +	int ret;
> +
> +	adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
> +				     adv->i2c_main->addr - 1);
> +	if (!adv->i2c_cec)
> +		return -ENOMEM;
> +
> +	adv->regmap_cec = devm_regmap_init_i2c(adv->i2c_cec,
> +					       adv->type == ADV7533 ?
> +					       &adv7533_cec_regmap_config :
> +					&adv7511_cec_regmap_config);
> +	if (IS_ERR(adv->regmap_cec)) {
> +		ret = PTR_ERR(adv->regmap_cec);
> +		goto err;
> +	}
> +
> +	if (adv->type == ADV7533) {
> +		ret = adv7533_patch_cec_registers(adv);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	return 0;
> +err:
> +	i2c_unregister_device(adv->i2c_cec);
> +	return ret;
> +}
> +
>   static int adv7511_parse_dt(struct device_node *np,
>   			    struct adv7511_link_config *config)
>   {
> @@ -1010,6 +1104,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>   	struct device *dev = &i2c->dev;
>   	unsigned int main_i2c_addr = i2c->addr << 1;
>   	unsigned int edid_i2c_addr = main_i2c_addr + 4;
> +	unsigned int offset;
>   	unsigned int val;
>   	int ret;
>   
> @@ -1035,6 +1130,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>   		ret = adv7511_parse_dt(dev->of_node, &link_config);
>   	else
>   		ret = adv7533_parse_dt(dev->of_node, adv7511);
> +

This line seems unnecessary.

>   	if (ret)
>   		return ret;
>   
> @@ -1093,11 +1189,9 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>   		goto uninit_regulators;
>   	}
>   
> -	if (adv7511->type == ADV7533) {
> -		ret = adv7533_init_cec(adv7511);
> -		if (ret)
> -			goto err_i2c_unregister_edid;
> -	}
> +	ret = adv7511_init_cec_regmap(adv7511);
> +	if (ret)
> +		goto err_i2c_unregister_edid;
>   
>   	INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work);
>   
> @@ -1112,10 +1206,6 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>   			goto err_unregister_cec;
>   	}
>   
> -	/* CEC is unused for now */
> -	regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL,
> -		     ADV7511_CEC_CTRL_POWER_DOWN);
> -
>   	adv7511_power_off(adv7511);
>   
>   	i2c_set_clientdata(i2c, adv7511);
> @@ -1134,10 +1224,39 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>   
>   	adv7511_audio_init(dev, adv7511);
>   
> +	offset = adv7511->type == ADV7533 ? ADV7533_REG_CEC_OFFSET : 0;
> +
> +#ifdef CONFIG_DRM_I2C_ADV7511_CEC
> +	ret = adv7511_cec_parse_dt(dev, adv7511);
> +	if (ret)
> +		goto err_unregister_cec;
> +
> +	adv7511->cec_adap = cec_allocate_adapter(&adv7511_cec_adap_ops,
> +		adv7511, dev_name(&i2c->dev), CEC_CAP_TRANSMIT |
> +		CEC_CAP_LOG_ADDRS | CEC_CAP_PASSTHROUGH | CEC_CAP_RC,
> +		ADV7511_MAX_ADDRS);
> +	ret = PTR_ERR_OR_ZERO(adv7511->cec_adap);
> +	if (ret)
> +		goto err_unregister_cec;
> +
> +	adv7511_cec_init(adv7511, offset);
> +
> +	ret = cec_register_adapter(adv7511->cec_adap, &i2c->dev);
> +	if (ret) {
> +		cec_delete_adapter(adv7511->cec_adap);
> +		goto err_unregister_cec;
> +	}

We could ideally put this code in a single func and make adv7511_cec_init,
adv7511_cec_parse_dt and adv7511_cec_adap_ops within the scope of adv7511_cec.c.
It's not necessary to do, though.

> +#else
> +	regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
> +		     ADV7511_CEC_CTRL_POWER_DOWN);
> +#endif
> +
>   	return 0;
>   
>   err_unregister_cec:
> -	adv7533_uninit_cec(adv7511);
> +	i2c_unregister_device(adv7511->i2c_cec);
> +	if (adv7511->cec_clk)
> +		clk_disable_unprepare(adv7511->cec_clk);
>   err_i2c_unregister_edid:
>   	i2c_unregister_device(adv7511->i2c_edid);
>   uninit_regulators:
> @@ -1150,10 +1269,11 @@ static int adv7511_remove(struct i2c_client *i2c)
>   {
>   	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
>   
> -	if (adv7511->type == ADV7533) {
> +	if (adv7511->type == ADV7533)
>   		adv7533_detach_dsi(adv7511);
> -		adv7533_uninit_cec(adv7511);
> -	}
> +	i2c_unregister_device(adv7511->i2c_cec);
> +	if (adv7511->cec_clk)
> +		clk_disable_unprepare(adv7511->cec_clk);
>   
>   	adv7511_uninit_regulators(adv7511);
>   
> @@ -1161,6 +1281,8 @@ static int adv7511_remove(struct i2c_client *i2c)
>   
>   	adv7511_audio_exit(adv7511);
>   
> +	cec_unregister_adapter(adv7511->cec_adap);
> +
>   	i2c_unregister_device(adv7511->i2c_edid);
>   
>   	kfree(adv7511->edid);
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c
> index ac804f81e2f6..0e173abb913c 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c
> @@ -145,37 +145,11 @@ int adv7533_patch_registers(struct adv7511 *adv)
>   				     ARRAY_SIZE(adv7533_fixed_registers));
>   }
>   
> -void adv7533_uninit_cec(struct adv7511 *adv)
> +int adv7533_patch_cec_registers(struct adv7511 *adv)
>   {
> -	i2c_unregister_device(adv->i2c_cec);
> -}
> -
> -int adv7533_init_cec(struct adv7511 *adv)
> -{
> -	int ret;
> -
> -	adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
> -				     adv->i2c_main->addr - 1);
> -	if (!adv->i2c_cec)
> -		return -ENOMEM;
> -
> -	adv->regmap_cec = devm_regmap_init_i2c(adv->i2c_cec,
> -					&adv7533_cec_regmap_config);

adv7533_cec_regmap_config struct isn't needed in the file anymore, that too
can be deleted.

Looks good to me otherwise.

Thanks,
Archit

> -	if (IS_ERR(adv->regmap_cec)) {
> -		ret = PTR_ERR(adv->regmap_cec);
> -		goto err;
> -	}
> -
> -	ret = regmap_register_patch(adv->regmap_cec,
> +	return regmap_register_patch(adv->regmap_cec,
>   				    adv7533_cec_fixed_registers,
>   				    ARRAY_SIZE(adv7533_cec_fixed_registers));
> -	if (ret)
> -		goto err;
> -
> -	return 0;
> -err:
> -	adv7533_uninit_cec(adv);
> -	return ret;
>   }
>   
>   int adv7533_attach_dsi(struct adv7511 *adv)
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm: adv7511/33: add HDMI CEC support

[Hans Verkuil]

On 10/08/17 10:49, Archit Taneja wrote:
> 
> 
> On 07/30/2017 06:37 PM, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Add support for HDMI CEC to the drm adv7511/adv7533 drivers.
>>
>> The CEC registers that we need to use are identical for both drivers,
>> but they appear at different offsets in the register map.
> 
> Thanks for the patch. Some minor comments below.
> 
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>   drivers/gpu/drm/bridge/adv7511/Kconfig       |   8 +
>>   drivers/gpu/drm/bridge/adv7511/Makefile      |   1 +
>>   drivers/gpu/drm/bridge/adv7511/adv7511.h     |  45 +++-
>>   drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 314 +++++++++++++++++++++++++++
>>   drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 152 +++++++++++--
>>   drivers/gpu/drm/bridge/adv7511/adv7533.c     |  30 +--
>>   6 files changed, 500 insertions(+), 50 deletions(-)
>>   create mode 100644 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
>>
>> diff --git a/drivers/gpu/drm/bridge/adv7511/Kconfig b/drivers/gpu/drm/bridge/adv7511/Kconfig
>> index 2fed567f9943..592b9d2ec034 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/Kconfig
>> +++ b/drivers/gpu/drm/bridge/adv7511/Kconfig
>> @@ -21,3 +21,11 @@ config DRM_I2C_ADV7533
>>       default y
>>       help
>>         Support for the Analog Devices ADV7533 DSI to HDMI encoder.
>> +
>> +config DRM_I2C_ADV7511_CEC
>> +    bool "ADV7511/33 HDMI CEC driver"
>> +    depends on DRM_I2C_ADV7511
>> +    select CEC_CORE
>> +    default y
>> +    help
>> +      When selected the HDMI transmitter will support the CEC feature.
>> diff --git a/drivers/gpu/drm/bridge/adv7511/Makefile b/drivers/gpu/drm/bridge/adv7511/Makefile
>> index 5ba675534f6e..5bb384938a71 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/Makefile
>> +++ b/drivers/gpu/drm/bridge/adv7511/Makefile
>> @@ -1,4 +1,5 @@
>>   adv7511-y := adv7511_drv.o
>>   adv7511-$(CONFIG_DRM_I2C_ADV7511_AUDIO) += adv7511_audio.o
>> +adv7511-$(CONFIG_DRM_I2C_ADV7511_CEC) += adv7511_cec.o
>>   adv7511-$(CONFIG_DRM_I2C_ADV7533) += adv7533.o
>>   obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511.o
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> index fe18a5d2d84b..4fd7b14f619b 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> @@ -195,6 +195,25 @@
>>   #define ADV7511_PACKET_GM(x)        ADV7511_PACKET(5, x)
>>   #define ADV7511_PACKET_SPARE(x)        ADV7511_PACKET(6, x)
>>   +#define ADV7511_REG_CEC_TX_FRAME_HDR    0x00
>> +#define ADV7511_REG_CEC_TX_FRAME_DATA0    0x01
>> +#define ADV7511_REG_CEC_TX_FRAME_LEN    0x10
>> +#define ADV7511_REG_CEC_TX_ENABLE    0x11
>> +#define ADV7511_REG_CEC_TX_RETRY    0x12
>> +#define ADV7511_REG_CEC_TX_LOW_DRV_CNT    0x14
>> +#define ADV7511_REG_CEC_RX_FRAME_HDR    0x15
>> +#define ADV7511_REG_CEC_RX_FRAME_DATA0    0x16
>> +#define ADV7511_REG_CEC_RX_FRAME_LEN    0x25
>> +#define ADV7511_REG_CEC_RX_ENABLE    0x26
>> +#define ADV7511_REG_CEC_RX_BUFFERS    0x4a
>> +#define ADV7511_REG_CEC_LOG_ADDR_MASK    0x4b
>> +#define ADV7511_REG_CEC_LOG_ADDR_0_1    0x4c
>> +#define ADV7511_REG_CEC_LOG_ADDR_2    0x4d
>> +#define ADV7511_REG_CEC_CLK_DIV        0x4e
>> +#define ADV7511_REG_CEC_SOFT_RESET    0x50
>> +
>> +#define ADV7533_REG_CEC_OFFSET        0x70
>> +
>>   enum adv7511_input_clock {
>>       ADV7511_INPUT_CLOCK_1X,
>>       ADV7511_INPUT_CLOCK_2X,
>> @@ -297,6 +316,8 @@ enum adv7511_type {
>>       ADV7533,
>>   };
>>   +#define ADV7511_MAX_ADDRS 3
>> +
>>   struct adv7511 {
>>       struct i2c_client *i2c_main;
>>       struct i2c_client *i2c_edid;
>> @@ -343,15 +364,29 @@ struct adv7511 {
>>         enum adv7511_type type;
>>       struct platform_device *audio_pdev;
>> +
>> +    struct cec_adapter *cec_adap;
>> +    u8   cec_addr[ADV7511_MAX_ADDRS];
>> +    u8   cec_valid_addrs;
>> +    bool cec_enabled_adap;
>> +    struct clk *cec_clk;
>> +    u32 cec_clk_freq;
>>   };
>>   +#ifdef CONFIG_DRM_I2C_ADV7511_CEC
>> +extern const struct cec_adap_ops adv7511_cec_adap_ops;
>> +
>> +void adv7511_cec_init(struct adv7511 *adv7511, unsigned int offset);
>> +int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511);
>> +void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1);
>> +#endif
>> +
>>   #ifdef CONFIG_DRM_I2C_ADV7533
>>   void adv7533_dsi_power_on(struct adv7511 *adv);
>>   void adv7533_dsi_power_off(struct adv7511 *adv);
>>   void adv7533_mode_set(struct adv7511 *adv, struct drm_display_mode *mode);
>>   int adv7533_patch_registers(struct adv7511 *adv);
>> -void adv7533_uninit_cec(struct adv7511 *adv);
>> -int adv7533_init_cec(struct adv7511 *adv);
>> +int adv7533_patch_cec_registers(struct adv7511 *adv);
>>   int adv7533_attach_dsi(struct adv7511 *adv);
>>   void adv7533_detach_dsi(struct adv7511 *adv);
>>   int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv);
>> @@ -374,11 +409,7 @@ static inline int adv7533_patch_registers(struct adv7511 *adv)
>>       return -ENODEV;
>>   }
>>   -static inline void adv7533_uninit_cec(struct adv7511 *adv)
>> -{
>> -}
>> -
>> -static inline int adv7533_init_cec(struct adv7511 *adv)
>> +static inline int adv7533_patch_cec_registers(struct adv7511 *adv)
>>   {
>>       return -ENODEV;
>>   }
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
>> new file mode 100644
>> index 000000000000..74081cbfb5db
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
>> @@ -0,0 +1,314 @@
>> +/*
>> + * adv7511_cec.c - Analog Devices ADV7511/33 cec driver
>> + *
>> + * Copyright 2017 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
>> + *
>> + * This program is free software; you may redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; version 2 of the License.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
>> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
>> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
>> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
>> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
>> + * SOFTWARE.
>> + *
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/clk.h>
>> +
>> +#include <media/cec.h>
>> +
>> +#include "adv7511.h"
>> +
>> +#define ADV7511_INT1_CEC_MASK \
>> +    (ADV7511_INT1_CEC_TX_READY | ADV7511_INT1_CEC_TX_ARBIT_LOST | \
>> +     ADV7511_INT1_CEC_TX_RETRY_TIMEOUT | ADV7511_INT1_CEC_RX_READY1)
>> +
>> +static void adv_cec_tx_raw_status(struct adv7511 *adv7511, u8 tx_raw_status)
>> +{
>> +    unsigned int offset = adv7511->type == ADV7533 ?
>> +                    ADV7533_REG_CEC_OFFSET : 0;
>> +    unsigned int val;
>> +
>> +    if (regmap_read(adv7511->regmap_cec,
>> +            ADV7511_REG_CEC_TX_ENABLE + offset, &val))
>> +        return;
>> +
>> +    if ((val & 0x01) == 0)
>> +        return;
>> +
>> +    if (tx_raw_status & 0x10) {
> 
> Should we try to use IRQ1 masks (ADV7511_INT1_CEC_TX_ARBIT_LOST here) to make the
> code more legible?
> 
> Same comments for the rest of this func and adv7511_cec_irq_process below.

Done.

> 
>> +        cec_transmit_attempt_done(adv7511->cec_adap,
>> +                      CEC_TX_STATUS_ARB_LOST);
>> +        return;
>> +    }
>> +    if (tx_raw_status & 0x08) {
>> +        u8 status;
>> +        u8 err_cnt = 0;
>> +        u8 nack_cnt = 0;
>> +        u8 low_drive_cnt = 0;
>> +        unsigned int cnt;
>> +
>> +        /*
>> +         * We set this status bit since this hardware performs
>> +         * retransmissions.
>> +         */
>> +        status = CEC_TX_STATUS_MAX_RETRIES;
>> +        if (regmap_read(adv7511->regmap_cec,
>> +                ADV7511_REG_CEC_TX_LOW_DRV_CNT + offset, &cnt)) {
>> +            err_cnt = 1;
>> +            status |= CEC_TX_STATUS_ERROR;
>> +        } else {
>> +            nack_cnt = cnt & 0xf;
>> +            if (nack_cnt)
>> +                status |= CEC_TX_STATUS_NACK;
>> +            low_drive_cnt = cnt >> 4;
>> +            if (low_drive_cnt)
>> +                status |= CEC_TX_STATUS_LOW_DRIVE;
>> +        }
>> +        cec_transmit_done(adv7511->cec_adap, status,
>> +                  0, nack_cnt, low_drive_cnt, err_cnt);
>> +        return;
>> +    }
>> +    if (tx_raw_status & 0x20) {
>> +        cec_transmit_attempt_done(adv7511->cec_adap, CEC_TX_STATUS_OK);
>> +        return;
>> +    }
>> +}
>> +
>> +void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>> +{
>> +    unsigned int offset = adv7511->type == ADV7533 ?
>> +                    ADV7533_REG_CEC_OFFSET : 0;
>> +    struct cec_msg msg = {};
>> +    unsigned int len;
>> +    unsigned int val;
>> +    u8 i;
>> +
>> +    if (irq1 & 0x38) > +        adv_cec_tx_raw_status(adv7511, irq1);
>> +
>> +    if (!(irq1 & 1))
>> +        return;
>> +
>> +    if (regmap_read(adv7511->regmap_cec,
>> +            ADV7511_REG_CEC_RX_FRAME_LEN + offset, &len))
>> +        return;
>> +
>> +    msg.len = len & 0x1f;
>> +
>> +    if (msg.len > 16)
>> +        msg.len = 16;
>> +
>> +    if (!msg.len)
>> +        return;
>> +
>> +    for (i = 0; i < msg.len; i++) {
>> +        regmap_read(adv7511->regmap_cec,
>> +                i + ADV7511_REG_CEC_RX_FRAME_HDR + offset, &val);
>> +        msg.msg[i] = val;
>> +    }
>> +
>> +    /* toggle to re-enable rx 1 */
>> +    regmap_write(adv7511->regmap_cec,
>> +             ADV7511_REG_CEC_RX_BUFFERS + offset, 1);
>> +    regmap_write(adv7511->regmap_cec,
>> +             ADV7511_REG_CEC_RX_BUFFERS + offset, 0);
>> +    cec_received_msg(adv7511->cec_adap, &msg);
>> +}
>> +
>> +static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
>> +{
>> +    struct adv7511 *adv7511 = cec_get_drvdata(adap);
>> +    unsigned int offset = adv7511->type == ADV7533 ?
>> +                    ADV7533_REG_CEC_OFFSET : 0;
>> +
>> +    if (adv7511->i2c_cec == NULL)
>> +        return -EIO;
>> +
>> +    if (!adv7511->cec_enabled_adap && enable) {
>> +        /* power up cec section */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_CLK_DIV + offset,
>> +                   0x03, 0x01);
>> +        /* legacy mode and clear all rx buffers */
>> +        regmap_write(adv7511->regmap_cec,
>> +                 ADV7511_REG_CEC_RX_BUFFERS + offset, 0x07);
>> +        regmap_write(adv7511->regmap_cec,
>> +                 ADV7511_REG_CEC_RX_BUFFERS + offset, 0);
>> +        /* initially disable tx */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_TX_ENABLE + offset, 1, 0);
>> +        /* enabled irqs: */
>> +        /* tx: ready */
>> +        /* tx: arbitration lost */
>> +        /* tx: retry timeout */
>> +        /* rx: ready 1 */
>> +        regmap_update_bits(adv7511->regmap,
>> +                   ADV7511_REG_INT_ENABLE(1), 0x3f,
>> +                   ADV7511_INT1_CEC_MASK);
>> +    } else if (adv7511->cec_enabled_adap && !enable) {
>> +        regmap_update_bits(adv7511->regmap,
>> +                   ADV7511_REG_INT_ENABLE(1), 0x3f, 0);
>> +        /* disable address mask 1-3 */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
>> +                   0x70, 0x00);
>> +        /* power down cec section */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_CLK_DIV + offset,
>> +                   0x03, 0x00);
>> +        adv7511->cec_valid_addrs = 0;
>> +    }
>> +    adv7511->cec_enabled_adap = enable;
>> +    return 0;
>> +}
>> +
>> +static int adv7511_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
>> +{
>> +    struct adv7511 *adv7511 = cec_get_drvdata(adap);
>> +    unsigned int offset = adv7511->type == ADV7533 ?
>> +                    ADV7533_REG_CEC_OFFSET : 0;
>> +    unsigned int i, free_idx = ADV7511_MAX_ADDRS;
>> +
>> +    if (!adv7511->cec_enabled_adap)
>> +        return addr == CEC_LOG_ADDR_INVALID ? 0 : -EIO;
>> +
>> +    if (addr == CEC_LOG_ADDR_INVALID) {
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
>> +                   0x70, 0);
>> +        adv7511->cec_valid_addrs = 0;
>> +        return 0;
>> +    }
>> +
>> +    for (i = 0; i < ADV7511_MAX_ADDRS; i++) {
>> +        bool is_valid = adv7511->cec_valid_addrs & (1 << i);
>> +
>> +        if (free_idx == ADV7511_MAX_ADDRS && !is_valid)
>> +            free_idx = i;
>> +        if (is_valid && adv7511->cec_addr[i] == addr)
>> +            return 0;
>> +    }
>> +    if (i == ADV7511_MAX_ADDRS) {
>> +        i = free_idx;
>> +        if (i == ADV7511_MAX_ADDRS)
>> +            return -ENXIO;
>> +    }
>> +    adv7511->cec_addr[i] = addr;
>> +    adv7511->cec_valid_addrs |= 1 << i;
>> +
>> +    switch (i) {
>> +    case 0:
>> +        /* enable address mask 0 */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
>> +                   0x10, 0x10);
>> +        /* set address for mask 0 */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_LOG_ADDR_0_1 + offset,
>> +                   0x0f, addr);
>> +        break;
>> +    case 1:
>> +        /* enable address mask 1 */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
>> +                   0x20, 0x20);
>> +        /* set address for mask 1 */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_LOG_ADDR_0_1 + offset,
>> +                   0xf0, addr << 4);
>> +        break;
>> +    case 2:
>> +        /* enable address mask 2 */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_LOG_ADDR_MASK + offset,
>> +                   0x40, 0x40);
>> +        /* set address for mask 1 */
>> +        regmap_update_bits(adv7511->regmap_cec,
>> +                   ADV7511_REG_CEC_LOG_ADDR_2 + offset,
>> +                   0x0f, addr);
>> +        break;
>> +    }
>> +    return 0;
>> +}
>> +
>> +static int adv7511_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>> +                     u32 signal_free_time, struct cec_msg *msg)
>> +{
>> +    struct adv7511 *adv7511 = cec_get_drvdata(adap);
>> +    unsigned int offset = adv7511->type == ADV7533 ?
>> +                    ADV7533_REG_CEC_OFFSET : 0;
>> +    u8 len = msg->len;
>> +    unsigned int i;
>> +
>> +    /*
>> +     * The number of retries is the number of attempts - 1, but retry
>> +     * at least once. It's not clear if a value of 0 is allowed, so
>> +     * let's do at least one retry.
>> +     */
>> +    regmap_update_bits(adv7511->regmap_cec,
>> +               ADV7511_REG_CEC_TX_RETRY + offset,
>> +               0x70, max(1, attempts - 1) << 4);
>> +
>> +    /* blocking, clear cec tx irq status */
>> +    regmap_update_bits(adv7511->regmap, ADV7511_REG_INT(1), 0x38, 0x38);
>> +
>> +    /* write data */ > +    for (i = 0; i < len; i++)
>> +        regmap_write(adv7511->regmap_cec, i + offset, msg->msg[i]);
> 
> Maybe "i + ADV7511_REG_CEC_TX_FRAME_HDR + offset" here for more clarity?

Done.

> 
>> +
>> +    /* set length (data + header) */
>> +    regmap_write(adv7511->regmap_cec,
>> +             ADV7511_REG_CEC_TX_FRAME_LEN + offset, len);
>> +    /* start transmit, enable tx */
>> +    regmap_write(adv7511->regmap_cec,
>> +             ADV7511_REG_CEC_TX_ENABLE + offset, 0x01);
>> +    return 0;
>> +}
>> +
>> +const struct cec_adap_ops adv7511_cec_adap_ops = {
>> +    .adap_enable = adv7511_cec_adap_enable,
>> +    .adap_log_addr = adv7511_cec_adap_log_addr,
>> +    .adap_transmit = adv7511_cec_adap_transmit,
>> +};
>> +
>> +void adv7511_cec_init(struct adv7511 *adv7511, unsigned int offset)
>> +{
>> +    regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, 0);
>> +    /* cec soft reset */
>> +    regmap_write(adv7511->regmap_cec,
>> +             ADV7511_REG_CEC_SOFT_RESET + offset, 0x01);
>> +    regmap_write(adv7511->regmap_cec,
>> +             ADV7511_REG_CEC_SOFT_RESET + offset, 0x00);
>> +
>> +    /* legacy mode */
>> +    regmap_write(adv7511->regmap_cec,
>> +             ADV7511_REG_CEC_RX_BUFFERS + offset, 0x00);
>> +
>> +    regmap_write(adv7511->regmap_cec,
>> +             ADV7511_REG_CEC_CLK_DIV + offset,
>> +             ((adv7511->cec_clk_freq / 750000) - 1) << 2);
>> +}
>> +
>> +int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511)
>> +{
>> +    adv7511->cec_clk = devm_clk_get(dev, "cec");
>> +    if (IS_ERR(adv7511->cec_clk)) {
>> +        int ret = PTR_ERR(adv7511->cec_clk);
>> +
>> +        adv7511->cec_clk = NULL;
>> +        return ret;
>> +    }
>> +    clk_prepare_enable(adv7511->cec_clk);
>> +    adv7511->cec_clk_freq = clk_get_rate(adv7511->cec_clk);
>> +    return 0;
>> +}
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> index f75ab6278113..1bef33e99358 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -11,12 +11,15 @@
>>   #include <linux/module.h>
>>   #include <linux/of_device.h>
>>   #include <linux/slab.h>
>> +#include <linux/clk.h>
>>     #include <drm/drmP.h>
>>   #include <drm/drm_atomic.h>
>>   #include <drm/drm_atomic_helper.h>
>>   #include <drm/drm_edid.h>
>>   +#include <media/cec.h>
>> +
>>   #include "adv7511.h"
>>     /* ADI recommended values for proper operation. */
>> @@ -339,8 +342,10 @@ static void __adv7511_power_on(struct adv7511 *adv7511)
>>            */
>>           regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(0),
>>                    ADV7511_INT0_EDID_READY | ADV7511_INT0_HPD);
>> -        regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(1),
>> -                 ADV7511_INT1_DDC_ERROR);
>> +        regmap_update_bits(adv7511->regmap,
>> +                   ADV7511_REG_INT_ENABLE(1),
>> +                   ADV7511_INT1_DDC_ERROR,
>> +                   ADV7511_INT1_DDC_ERROR);
>>       }
>>         /*
>> @@ -376,6 +381,9 @@ static void __adv7511_power_off(struct adv7511 *adv7511)
>>       regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>>                  ADV7511_POWER_POWER_DOWN,
>>                  ADV7511_POWER_POWER_DOWN);
>> +    regmap_update_bits(adv7511->regmap,
>> +               ADV7511_REG_INT_ENABLE(1),
>> +               ADV7511_INT1_DDC_ERROR, 0);
>>       regcache_mark_dirty(adv7511->regmap);
>>   }
>>   @@ -426,6 +434,8 @@ static void adv7511_hpd_work(struct work_struct *work)
>>         if (adv7511->connector.status != status) {
>>           adv7511->connector.status = status;
>> +        if (status == connector_status_disconnected)
>> +            cec_phys_addr_invalidate(adv7511->cec_adap);
>>           drm_kms_helper_hotplug_event(adv7511->connector.dev);
>>       }
>>   }
>> @@ -456,6 +466,10 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
>>               wake_up_all(&adv7511->wq);
>>       }
>>   +#ifdef CONFIG_DRM_I2C_ADV7511_CEC
>> +    adv7511_cec_irq_process(adv7511, irq1);
>> +#endif
>> +
>>       return 0;
>>   }
>>   @@ -599,6 +613,8 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>>         adv7511_set_config_csc(adv7511, connector, adv7511->rgb);
>>   +    cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);
>> +
>>       return count;
>>   }
>>   @@ -920,6 +936,84 @@ static void adv7511_uninit_regulators(struct adv7511 *adv)
>>       regulator_bulk_disable(adv->num_supplies, adv->supplies);
>>   }
>>   +static bool adv7533_cec_register_volatile(struct device *dev, unsigned int reg)
>> +{
>> +    switch (reg) {
>> +    case ADV7511_REG_CEC_RX_FRAME_HDR + ADV7533_REG_CEC_OFFSET:
>> +    case ADV7511_REG_CEC_RX_FRAME_DATA0 + ADV7533_REG_CEC_OFFSET...
>> +        ADV7511_REG_CEC_RX_FRAME_DATA0 + ADV7533_REG_CEC_OFFSET + 14:
>> +    case ADV7511_REG_CEC_RX_FRAME_LEN + ADV7533_REG_CEC_OFFSET:
>> +    case ADV7511_REG_CEC_RX_BUFFERS + ADV7533_REG_CEC_OFFSET:
>> +    case ADV7511_REG_CEC_TX_LOW_DRV_CNT + ADV7533_REG_CEC_OFFSET:
>> +        return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +static const struct regmap_config adv7533_cec_regmap_config = {
>> +    .reg_bits = 8,
>> +    .val_bits = 8,
>> +
>> +    .max_register = 0xff,
>> +    .cache_type = REGCACHE_RBTREE,
>> +    .volatile_reg = adv7533_cec_register_volatile,
>> +};
>> +
>> +static bool adv7511_cec_register_volatile(struct device *dev, unsigned int reg)
>> +{
> 
> Maybe we could combine the two register_volatile() funcs and the remap_config structs
> for adv7511 and adv7533 by passing (reg + offset) to switch?

How? How would I know in the volatile function whether it is an adv7511 or adv7533?
Is there an easy way to go from the struct device to a struct adv7511?

> 
>> +    switch (reg) {
>> +    case ADV7511_REG_CEC_RX_FRAME_HDR:
>> +    case ADV7511_REG_CEC_RX_FRAME_DATA0...
>> +        ADV7511_REG_CEC_RX_FRAME_DATA0 + 14:
>> +    case ADV7511_REG_CEC_RX_FRAME_LEN:
>> +    case ADV7511_REG_CEC_RX_BUFFERS:
>> +    case ADV7511_REG_CEC_TX_LOW_DRV_CNT:
>> +        return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +static const struct regmap_config adv7511_cec_regmap_config = {
>> +    .reg_bits = 8,
>> +    .val_bits = 8,
>> +
>> +    .max_register = 0xff,
>> +    .cache_type = REGCACHE_RBTREE,
>> +    .volatile_reg = adv7511_cec_register_volatile,
>> +};
>> +
>> +static int adv7511_init_cec_regmap(struct adv7511 *adv)
>> +{
>> +    int ret;
>> +
>> +    adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
>> +                     adv->i2c_main->addr - 1);
>> +    if (!adv->i2c_cec)
>> +        return -ENOMEM;
>> +
>> +    adv->regmap_cec = devm_regmap_init_i2c(adv->i2c_cec,
>> +                           adv->type == ADV7533 ?
>> +                           &adv7533_cec_regmap_config :
>> +                    &adv7511_cec_regmap_config);
>> +    if (IS_ERR(adv->regmap_cec)) {
>> +        ret = PTR_ERR(adv->regmap_cec);
>> +        goto err;
>> +    }
>> +
>> +    if (adv->type == ADV7533) {
>> +        ret = adv7533_patch_cec_registers(adv);
>> +        if (ret)
>> +            goto err;
>> +    }
>> +
>> +    return 0;
>> +err:
>> +    i2c_unregister_device(adv->i2c_cec);
>> +    return ret;
>> +}
>> +
>>   static int adv7511_parse_dt(struct device_node *np,
>>                   struct adv7511_link_config *config)
>>   {
>> @@ -1010,6 +1104,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>>       struct device *dev = &i2c->dev;
>>       unsigned int main_i2c_addr = i2c->addr << 1;
>>       unsigned int edid_i2c_addr = main_i2c_addr + 4;
>> +    unsigned int offset;
>>       unsigned int val;
>>       int ret;
>>   @@ -1035,6 +1130,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>>           ret = adv7511_parse_dt(dev->of_node, &link_config);
>>       else
>>           ret = adv7533_parse_dt(dev->of_node, adv7511);
>> +
> 
> This line seems unnecessary.

Removed.

> 
>>       if (ret)
>>           return ret;
>>   @@ -1093,11 +1189,9 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>>           goto uninit_regulators;
>>       }
>>   -    if (adv7511->type == ADV7533) {
>> -        ret = adv7533_init_cec(adv7511);
>> -        if (ret)
>> -            goto err_i2c_unregister_edid;
>> -    }
>> +    ret = adv7511_init_cec_regmap(adv7511);
>> +    if (ret)
>> +        goto err_i2c_unregister_edid;
>>         INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work);
>>   @@ -1112,10 +1206,6 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>>               goto err_unregister_cec;
>>       }
>>   -    /* CEC is unused for now */
>> -    regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL,
>> -             ADV7511_CEC_CTRL_POWER_DOWN);
>> -
>>       adv7511_power_off(adv7511);
>>         i2c_set_clientdata(i2c, adv7511);
>> @@ -1134,10 +1224,39 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>>         adv7511_audio_init(dev, adv7511);
>>   +    offset = adv7511->type == ADV7533 ? ADV7533_REG_CEC_OFFSET : 0;
>> +
>> +#ifdef CONFIG_DRM_I2C_ADV7511_CEC
>> +    ret = adv7511_cec_parse_dt(dev, adv7511);
>> +    if (ret)
>> +        goto err_unregister_cec;
>> +
>> +    adv7511->cec_adap = cec_allocate_adapter(&adv7511_cec_adap_ops,
>> +        adv7511, dev_name(&i2c->dev), CEC_CAP_TRANSMIT |
>> +        CEC_CAP_LOG_ADDRS | CEC_CAP_PASSTHROUGH | CEC_CAP_RC,
>> +        ADV7511_MAX_ADDRS);
>> +    ret = PTR_ERR_OR_ZERO(adv7511->cec_adap);
>> +    if (ret)
>> +        goto err_unregister_cec;
>> +
>> +    adv7511_cec_init(adv7511, offset);
>> +
>> +    ret = cec_register_adapter(adv7511->cec_adap, &i2c->dev);
>> +    if (ret) {
>> +        cec_delete_adapter(adv7511->cec_adap);
>> +        goto err_unregister_cec;
>> +    }
> 
> We could ideally put this code in a single func and make adv7511_cec_init,
> adv7511_cec_parse_dt and adv7511_cec_adap_ops within the scope of adv7511_cec.c.
> It's not necessary to do, though.

Done. Not sure why I didn't do that in the first place...

> 
>> +#else
>> +    regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
>> +             ADV7511_CEC_CTRL_POWER_DOWN);
>> +#endif
>> +
>>       return 0;
>>     err_unregister_cec:
>> -    adv7533_uninit_cec(adv7511);
>> +    i2c_unregister_device(adv7511->i2c_cec);
>> +    if (adv7511->cec_clk)
>> +        clk_disable_unprepare(adv7511->cec_clk);
>>   err_i2c_unregister_edid:
>>       i2c_unregister_device(adv7511->i2c_edid);
>>   uninit_regulators:
>> @@ -1150,10 +1269,11 @@ static int adv7511_remove(struct i2c_client *i2c)
>>   {
>>       struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
>>   -    if (adv7511->type == ADV7533) {
>> +    if (adv7511->type == ADV7533)
>>           adv7533_detach_dsi(adv7511);
>> -        adv7533_uninit_cec(adv7511);
>> -    }
>> +    i2c_unregister_device(adv7511->i2c_cec);
>> +    if (adv7511->cec_clk)
>> +        clk_disable_unprepare(adv7511->cec_clk);
>>         adv7511_uninit_regulators(adv7511);
>>   @@ -1161,6 +1281,8 @@ static int adv7511_remove(struct i2c_client *i2c)
>>         adv7511_audio_exit(adv7511);
>>   +    cec_unregister_adapter(adv7511->cec_adap);
>> +
>>       i2c_unregister_device(adv7511->i2c_edid);
>>         kfree(adv7511->edid);
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c
>> index ac804f81e2f6..0e173abb913c 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c
>> @@ -145,37 +145,11 @@ int adv7533_patch_registers(struct adv7511 *adv)
>>                        ARRAY_SIZE(adv7533_fixed_registers));
>>   }
>>   -void adv7533_uninit_cec(struct adv7511 *adv)
>> +int adv7533_patch_cec_registers(struct adv7511 *adv)
>>   {
>> -    i2c_unregister_device(adv->i2c_cec);
>> -}
>> -
>> -int adv7533_init_cec(struct adv7511 *adv)
>> -{
>> -    int ret;
>> -
>> -    adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
>> -                     adv->i2c_main->addr - 1);
>> -    if (!adv->i2c_cec)
>> -        return -ENOMEM;
>> -
>> -    adv->regmap_cec = devm_regmap_init_i2c(adv->i2c_cec,
>> -                    &adv7533_cec_regmap_config);
> 
> adv7533_cec_regmap_config struct isn't needed in the file anymore, that too
> can be deleted.

Oops, must have missed that. Removed.

> 
> Looks good to me otherwise.

Thanks!

I will test the dragonboard again on Monday using your tree. Strange.

Regards,

	Hans

> 
> Thanks,
> Archit
> 
>> -    if (IS_ERR(adv->regmap_cec)) {
>> -        ret = PTR_ERR(adv->regmap_cec);
>> -        goto err;
>> -    }
>> -
>> -    ret = regmap_register_patch(adv->regmap_cec,
>> +    return regmap_register_patch(adv->regmap_cec,
>>                       adv7533_cec_fixed_registers,
>>                       ARRAY_SIZE(adv7533_cec_fixed_registers));
>> -    if (ret)
>> -        goto err;
>> -
>> -    return 0;
>> -err:
>> -    adv7533_uninit_cec(adv);
>> -    return ret;
>>   }
>>     int adv7533_attach_dsi(struct adv7511 *adv)
>>
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm: adv7511/33: add HDMI CEC support

[Hans Verkuil]

On 12/08/17 11:53, Hans Verkuil wrote:
> On 10/08/17 10:49, Archit Taneja wrote:
>>
>>
>> On 07/30/2017 06:37 PM, Hans Verkuil wrote:
>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>
>>> Add support for HDMI CEC to the drm adv7511/adv7533 drivers.
>>>
>>> The CEC registers that we need to use are identical for both drivers,
>>> but they appear at different offsets in the register map.
>>
>> Thanks for the patch. Some minor comments below.
>>
>>>
>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>> ---
>>>   drivers/gpu/drm/bridge/adv7511/Kconfig       |   8 +
>>>   drivers/gpu/drm/bridge/adv7511/Makefile      |   1 +
>>>   drivers/gpu/drm/bridge/adv7511/adv7511.h     |  45 +++-
>>>   drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 314 +++++++++++++++++++++++++++
>>>   drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 152 +++++++++++--
>>>   drivers/gpu/drm/bridge/adv7511/adv7533.c     |  30 +--
>>>   6 files changed, 500 insertions(+), 50 deletions(-)
>>>   create mode 100644 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
>>>

<snip>

>>>   +static bool adv7533_cec_register_volatile(struct device *dev, unsigned int reg)
>>> +{
>>> +    switch (reg) {
>>> +    case ADV7511_REG_CEC_RX_FRAME_HDR + ADV7533_REG_CEC_OFFSET:
>>> +    case ADV7511_REG_CEC_RX_FRAME_DATA0 + ADV7533_REG_CEC_OFFSET...
>>> +        ADV7511_REG_CEC_RX_FRAME_DATA0 + ADV7533_REG_CEC_OFFSET + 14:
>>> +    case ADV7511_REG_CEC_RX_FRAME_LEN + ADV7533_REG_CEC_OFFSET:
>>> +    case ADV7511_REG_CEC_RX_BUFFERS + ADV7533_REG_CEC_OFFSET:
>>> +    case ADV7511_REG_CEC_TX_LOW_DRV_CNT + ADV7533_REG_CEC_OFFSET:
>>> +        return true;
>>> +    }
>>> +
>>> +    return false;
>>> +}
>>> +
>>> +static const struct regmap_config adv7533_cec_regmap_config = {
>>> +    .reg_bits = 8,
>>> +    .val_bits = 8,
>>> +
>>> +    .max_register = 0xff,
>>> +    .cache_type = REGCACHE_RBTREE,
>>> +    .volatile_reg = adv7533_cec_register_volatile,
>>> +};
>>> +
>>> +static bool adv7511_cec_register_volatile(struct device *dev, unsigned int reg)
>>> +{
>>
>> Maybe we could combine the two register_volatile() funcs and the remap_config structs
>> for adv7511 and adv7533 by passing (reg + offset) to switch?
> 
> How? How would I know in the volatile function whether it is an adv7511 or adv7533?
> Is there an easy way to go from the struct device to a struct adv7511?

Never mind, I figured it out.

Implemented.

Regards,

	Hans

Re: [PATCH 0/4] drm/bridge/adv7511: add CEC support

[Archit Taneja]

On 07/30/2017 06:37 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> This patch series adds CEC support to the drm adv7511/adv7533 drivers.
> 
> I have tested this with the Qualcomm Dragonboard C410 (adv7533 based)
> and the Renesas R-Car Koelsch board (adv7511 based).
> 
> Note: the Dragonboard needs this patch:
> 
> https://patchwork.kernel.org/patch/9824773/
> 
> Archit, can you confirm that this patch will go to kernel 4.14?

Yes, it's been queued to clk-next.

Thanks,
Archit

> 
> And the Koelsch board needs this 4.13 fix:
> 
> https://patchwork.kernel.org/patch/9836865/
> 
> I only have the Koelsch board to test with, but it looks like other
> R-Car boards use the same adv7511. It would be nice if someone can
> add CEC support to the other R-Car boards as well. The main thing
> to check is if they all use the same 12 MHz fixed CEC clock source.
> 
> Anyone who wants to test this will need the CEC utilities that
> are part of the v4l-utils git repository:
> 
> git clone git://linuxtv.org/v4l-utils.git
> cd v4l-utils
> ./bootstrap.sh
> ./configure
> make
> sudo make install
> 
> Now configure the CEC adapter as a Playback device:
> 
> cec-ctl --playback
> 
> Discover other CEC devices:
> 
> cec-ctl -S
> 
> Regards,
> 
> 	Hans
> 
> Hans Verkuil (4):
>    dt-bindings: adi,adv7511.txt: document cec clock
>    arm: dts: qcom: add cec clock for apq8016 board
>    arm: dts: renesas: add cec clock for Koelsch board
>    drm: adv7511/33: add HDMI CEC support
> 
>   .../bindings/display/bridge/adi,adv7511.txt        |   4 +
>   arch/arm/boot/dts/r8a7791-koelsch.dts              |   8 +
>   arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi          |   2 +
>   drivers/gpu/drm/bridge/adv7511/Kconfig             |   8 +
>   drivers/gpu/drm/bridge/adv7511/Makefile            |   1 +
>   drivers/gpu/drm/bridge/adv7511/adv7511.h           |  45 ++-
>   drivers/gpu/drm/bridge/adv7511/adv7511_cec.c       | 314 +++++++++++++++++++++
>   drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 152 +++++++++-
>   drivers/gpu/drm/bridge/adv7511/adv7533.c           |  30 +-
>   9 files changed, 514 insertions(+), 50 deletions(-)
>   create mode 100644 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/4] drm/bridge/adv7511: add CEC support

[Archit Taneja]

Hi Hans,

On 07/30/2017 06:37 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> This patch series adds CEC support to the drm adv7511/adv7533 drivers.
> 
> I have tested this with the Qualcomm Dragonboard C410 (adv7533 based)
> and the Renesas R-Car Koelsch board (adv7511 based).
> 
> Note: the Dragonboard needs this patch:
> 
> https://patchwork.kernel.org/patch/9824773/
> 
> Archit, can you confirm that this patch will go to kernel 4.14?
> 
> And the Koelsch board needs this 4.13 fix:
> 
> https://patchwork.kernel.org/patch/9836865/
> 
> I only have the Koelsch board to test with, but it looks like other
> R-Car boards use the same adv7511. It would be nice if someone can
> add CEC support to the other R-Car boards as well. The main thing
> to check is if they all use the same 12 MHz fixed CEC clock source.
> 
> Anyone who wants to test this will need the CEC utilities that
> are part of the v4l-utils git repository:
> 
> git clone git://linuxtv.org/v4l-utils.git
> cd v4l-utils
> ./bootstrap.sh
> ./configure
> make
> sudo make install
> 
> Now configure the CEC adapter as a Playback device:
> 
> cec-ctl --playback
> 
> Discover other CEC devices:
> 
> cec-ctl -S

I tried the instructions, and I get the following output. I don't think I have
any CEC device connected, though. Is this the expected behaviour?

#cec-ctl -S
Driver Info:
         Driver Name                : adv7511
         Adapter Name               : 3-0039
         Capabilities               : 0x0000000e
                 Logical Addresses
                 Transmit
                 Passthrough
         Driver version             : 4.13.0
         Available Logical Addresses: 3
         Physical Address           : 1.0.0.0
         Logical Address Mask       : 0x0000
         CEC Version                : 2.0
         Logical Addresses          : 0

#cec-ctl --playback
[ 1038.761545] cec-3-0039: cec_thread_func: message 44 timed out!
Driver Info:
         Driver Name                : adv7511
         Adapter Name               : 3-0039
         Capabilities               : 0x0000000e
                 Logical Addresses
                 Transmit
                 Passthrough
         Driver version             : 4.13.0
         Available Logical Addresses: 3
         Physical Address           : 1.0.0.0
         Logical Address Mask       : 0x0010
         CEC Version                : 2.0
         Vendor ID                  : 0x000c03
         Logical Addresses          : 1 (Allow RC Passthrough)

           Logical Address          : 4
             Primary Device Type    : Playback
             Logical Address Type   : Playback
             All Device Types       : Playback
             RC TV Profile          : None
             Device Features        :
                 None


[ 1041.063605] cec-3-0039: cec_thread_func: message 4f a6 06 10 00 00 timed out!
[ 1043.367482] cec-3-0039: cec_thread_func: message 4f 84 10 00 04 timed out!

Thanks,
Archit

> 
> Regards,
> 
> 	Hans
> 
> Hans Verkuil (4):
>    dt-bindings: adi,adv7511.txt: document cec clock
>    arm: dts: qcom: add cec clock for apq8016 board
>    arm: dts: renesas: add cec clock for Koelsch board
>    drm: adv7511/33: add HDMI CEC support
> 
>   .../bindings/display/bridge/adi,adv7511.txt        |   4 +
>   arch/arm/boot/dts/r8a7791-koelsch.dts              |   8 +
>   arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi          |   2 +
>   drivers/gpu/drm/bridge/adv7511/Kconfig             |   8 +
>   drivers/gpu/drm/bridge/adv7511/Makefile            |   1 +
>   drivers/gpu/drm/bridge/adv7511/adv7511.h           |  45 ++-
>   drivers/gpu/drm/bridge/adv7511/adv7511_cec.c       | 314 +++++++++++++++++++++
>   drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 152 +++++++++-
>   drivers/gpu/drm/bridge/adv7511/adv7533.c           |  30 +-
>   9 files changed, 514 insertions(+), 50 deletions(-)
>   create mode 100644 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 0/4] drm/bridge/adv7511: add CEC support

[Hans Verkuil]

On 10/08/17 10:49, Archit Taneja wrote:
> Hi Hans,
> 
> On 07/30/2017 06:37 PM, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
>>
>> This patch series adds CEC support to the drm adv7511/adv7533 drivers.
>>
>> I have tested this with the Qualcomm Dragonboard C410 (adv7533 based)
>> and the Renesas R-Car Koelsch board (adv7511 based).
>>
>> Note: the Dragonboard needs this patch:
>>
>> https://patchwork.kernel.org/patch/9824773/
>>
>> Archit, can you confirm that this patch will go to kernel 4.14?
>>
>> And the Koelsch board needs this 4.13 fix:
>>
>> https://patchwork.kernel.org/patch/9836865/
>>
>> I only have the Koelsch board to test with, but it looks like other
>> R-Car boards use the same adv7511. It would be nice if someone can
>> add CEC support to the other R-Car boards as well. The main thing
>> to check is if they all use the same 12 MHz fixed CEC clock source.
>>
>> Anyone who wants to test this will need the CEC utilities that
>> are part of the v4l-utils git repository:
>>
>> git clone git://linuxtv.org/v4l-utils.git
>> cd v4l-utils
>> ./bootstrap.sh
>> ./configure
>> make
>> sudo make install
>>
>> Now configure the CEC adapter as a Playback device:
>>
>> cec-ctl --playback
>>
>> Discover other CEC devices:
>>
>> cec-ctl -S
> 
> I tried the instructions, and I get the following output. I don't think I have
> any CEC device connected, though. Is this the expected behaviour?
> 
> #cec-ctl -S
> Driver Info:
>         Driver Name                : adv7511
>         Adapter Name               : 3-0039
>         Capabilities               : 0x0000000e
>                 Logical Addresses
>                 Transmit
>                 Passthrough
>         Driver version             : 4.13.0
>         Available Logical Addresses: 3
>         Physical Address           : 1.0.0.0
>         Logical Address Mask       : 0x0000
>         CEC Version                : 2.0
>         Logical Addresses          : 0
> 
> #cec-ctl --playback
> [ 1038.761545] cec-3-0039: cec_thread_func: message 44 timed out!

This isn't right. You shouldn't see this. It never receives an interrupt
when the transmit has finished, which causes these time outs.

What are you testing this on? The Dragonboard c410?

Can you check the cec clock frequency? It should be 19.2 MHz.
Remember to apply this patch: https://patchwork.kernel.org/patch/9824773/
You probably did, but just in case...

Regards,

	Hans

> Driver Info:
>         Driver Name                : adv7511
>         Adapter Name               : 3-0039
>         Capabilities               : 0x0000000e
>                 Logical Addresses
>                 Transmit
>                 Passthrough
>         Driver version             : 4.13.0
>         Available Logical Addresses: 3
>         Physical Address           : 1.0.0.0
>         Logical Address Mask       : 0x0010
>         CEC Version                : 2.0
>         Vendor ID                  : 0x000c03
>         Logical Addresses          : 1 (Allow RC Passthrough)
> 
>           Logical Address          : 4
>             Primary Device Type    : Playback
>             Logical Address Type   : Playback
>             All Device Types       : Playback
>             RC TV Profile          : None
>             Device Features        :
>                 None
> 
> 
> [ 1041.063605] cec-3-0039: cec_thread_func: message 4f a6 06 10 00 00 timed out!
> [ 1043.367482] cec-3-0039: cec_thread_func: message 4f 84 10 00 04 timed out!
> 
> Thanks,
> Archit
> 
>>
>> Regards,
>>
>>     Hans
>>
>> Hans Verkuil (4):
>>    dt-bindings: adi,adv7511.txt: document cec clock
>>    arm: dts: qcom: add cec clock for apq8016 board
>>    arm: dts: renesas: add cec clock for Koelsch board
>>    drm: adv7511/33: add HDMI CEC support
>>
>>   .../bindings/display/bridge/adi,adv7511.txt        |   4 +
>>   arch/arm/boot/dts/r8a7791-koelsch.dts              |   8 +
>>   arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi          |   2 +
>>   drivers/gpu/drm/bridge/adv7511/Kconfig             |   8 +
>>   drivers/gpu/drm/bridge/adv7511/Makefile            |   1 +
>>   drivers/gpu/drm/bridge/adv7511/adv7511.h           |  45 ++-
>>   drivers/gpu/drm/bridge/adv7511/adv7511_cec.c       | 314 +++++++++++++++++++++
>>   drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 152 +++++++++-
>>   drivers/gpu/drm/bridge/adv7511/adv7533.c           |  30 +-
>>   9 files changed, 514 insertions(+), 50 deletions(-)
>>   create mode 100644 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] drm/bridge/adv7511: add CEC support

[Archit Taneja]

On 08/10/2017 02:26 PM, Hans Verkuil wrote:
> On 10/08/17 10:49, Archit Taneja wrote:
>> Hi Hans,
>>
>> On 07/30/2017 06:37 PM, Hans Verkuil wrote:
>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>
>>> This patch series adds CEC support to the drm adv7511/adv7533 drivers.
>>>
>>> I have tested this with the Qualcomm Dragonboard C410 (adv7533 based)
>>> and the Renesas R-Car Koelsch board (adv7511 based).
>>>
>>> Note: the Dragonboard needs this patch:
>>>
>>> https://patchwork.kernel.org/patch/9824773/
>>>
>>> Archit, can you confirm that this patch will go to kernel 4.14?
>>>
>>> And the Koelsch board needs this 4.13 fix:
>>>
>>> https://patchwork.kernel.org/patch/9836865/
>>>
>>> I only have the Koelsch board to test with, but it looks like other
>>> R-Car boards use the same adv7511. It would be nice if someone can
>>> add CEC support to the other R-Car boards as well. The main thing
>>> to check is if they all use the same 12 MHz fixed CEC clock source.
>>>
>>> Anyone who wants to test this will need the CEC utilities that
>>> are part of the v4l-utils git repository:
>>>
>>> git clone git://linuxtv.org/v4l-utils.git
>>> cd v4l-utils
>>> ./bootstrap.sh
>>> ./configure
>>> make
>>> sudo make install
>>>
>>> Now configure the CEC adapter as a Playback device:
>>>
>>> cec-ctl --playback
>>>
>>> Discover other CEC devices:
>>>
>>> cec-ctl -S
>>
>> I tried the instructions, and I get the following output. I don't think I have
>> any CEC device connected, though. Is this the expected behaviour?
>>
>> #cec-ctl -S
>> Driver Info:
>>          Driver Name                : adv7511
>>          Adapter Name               : 3-0039
>>          Capabilities               : 0x0000000e
>>                  Logical Addresses
>>                  Transmit
>>                  Passthrough
>>          Driver version             : 4.13.0
>>          Available Logical Addresses: 3
>>          Physical Address           : 1.0.0.0
>>          Logical Address Mask       : 0x0000
>>          CEC Version                : 2.0
>>          Logical Addresses          : 0
>>
>> #cec-ctl --playback
>> [ 1038.761545] cec-3-0039: cec_thread_func: message 44 timed out!
> 
> This isn't right. You shouldn't see this. It never receives an interrupt
> when the transmit has finished, which causes these time outs.
> 
> What are you testing this on? The Dragonboard c410?

Yes.

> 
> Can you check the cec clock frequency? It should be 19.2 MHz.
> Remember to apply this patch: https://patchwork.kernel.org/patch/9824773/
> You probably did, but just in case...

clk_summary does show bb_clk2 to be set to 19.2 Mhz.

I applied a newer version of this patch, which got merged in clk-next:

https://patchwork.kernel.org/patch/9845493/

I will try to apply the patch you use and check again.

Thanks,
Archit

> 
> Regards,
> 
> 	Hans
> 
>> Driver Info:
>>          Driver Name                : adv7511
>>          Adapter Name               : 3-0039
>>          Capabilities               : 0x0000000e
>>                  Logical Addresses
>>                  Transmit
>>                  Passthrough
>>          Driver version             : 4.13.0
>>          Available Logical Addresses: 3
>>          Physical Address           : 1.0.0.0
>>          Logical Address Mask       : 0x0010
>>          CEC Version                : 2.0
>>          Vendor ID                  : 0x000c03
>>          Logical Addresses          : 1 (Allow RC Passthrough)
>>
>>            Logical Address          : 4
>>              Primary Device Type    : Playback
>>              Logical Address Type   : Playback
>>              All Device Types       : Playback
>>              RC TV Profile          : None
>>              Device Features        :
>>                  None
>>
>>
>> [ 1041.063605] cec-3-0039: cec_thread_func: message 4f a6 06 10 00 00 timed out!
>> [ 1043.367482] cec-3-0039: cec_thread_func: message 4f 84 10 00 04 timed out!
>>
>> Thanks,
>> Archit
>>
>>>
>>> Regards,
>>>
>>>      Hans
>>>
>>> Hans Verkuil (4):
>>>     dt-bindings: adi,adv7511.txt: document cec clock
>>>     arm: dts: qcom: add cec clock for apq8016 board
>>>     arm: dts: renesas: add cec clock for Koelsch board
>>>     drm: adv7511/33: add HDMI CEC support
>>>
>>>    .../bindings/display/bridge/adi,adv7511.txt        |   4 +
>>>    arch/arm/boot/dts/r8a7791-koelsch.dts              |   8 +
>>>    arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi          |   2 +
>>>    drivers/gpu/drm/bridge/adv7511/Kconfig             |   8 +
>>>    drivers/gpu/drm/bridge/adv7511/Makefile            |   1 +
>>>    drivers/gpu/drm/bridge/adv7511/adv7511.h           |  45 ++-
>>>    drivers/gpu/drm/bridge/adv7511/adv7511_cec.c       | 314 +++++++++++++++++++++
>>>    drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 152 +++++++++-
>>>    drivers/gpu/drm/bridge/adv7511/adv7533.c           |  30 +-
>>>    9 files changed, 514 insertions(+), 50 deletions(-)
>>>    create mode 100644 drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
>>>
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/4] drm/bridge/adv7511: add CEC support

[Hans Verkuil]

On 10/08/17 11:08, Archit Taneja wrote:
> 
> 
> On 08/10/2017 02:26 PM, Hans Verkuil wrote:
>> On 10/08/17 10:49, Archit Taneja wrote:
>>> Hi Hans,
>>>
>>> On 07/30/2017 06:37 PM, Hans Verkuil wrote:
>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>
>>>> This patch series adds CEC support to the drm adv7511/adv7533 drivers.
>>>>
>>>> I have tested this with the Qualcomm Dragonboard C410 (adv7533 based)
>>>> and the Renesas R-Car Koelsch board (adv7511 based).
>>>>
>>>> Note: the Dragonboard needs this patch:
>>>>
>>>> https://patchwork.kernel.org/patch/9824773/
>>>>
>>>> Archit, can you confirm that this patch will go to kernel 4.14?
>>>>
>>>> And the Koelsch board needs this 4.13 fix:
>>>>
>>>> https://patchwork.kernel.org/patch/9836865/
>>>>
>>>> I only have the Koelsch board to test with, but it looks like other
>>>> R-Car boards use the same adv7511. It would be nice if someone can
>>>> add CEC support to the other R-Car boards as well. The main thing
>>>> to check is if they all use the same 12 MHz fixed CEC clock source.
>>>>
>>>> Anyone who wants to test this will need the CEC utilities that
>>>> are part of the v4l-utils git repository:
>>>>
>>>> git clone git://linuxtv.org/v4l-utils.git
>>>> cd v4l-utils
>>>> ./bootstrap.sh
>>>> ./configure
>>>> make
>>>> sudo make install
>>>>
>>>> Now configure the CEC adapter as a Playback device:
>>>>
>>>> cec-ctl --playback
>>>>
>>>> Discover other CEC devices:
>>>>
>>>> cec-ctl -S
>>>
>>> I tried the instructions, and I get the following output. I don't think I have
>>> any CEC device connected, though. Is this the expected behaviour?
>>>
>>> #cec-ctl -S
>>> Driver Info:
>>>          Driver Name                : adv7511
>>>          Adapter Name               : 3-0039
>>>          Capabilities               : 0x0000000e
>>>                  Logical Addresses
>>>                  Transmit
>>>                  Passthrough
>>>          Driver version             : 4.13.0
>>>          Available Logical Addresses: 3
>>>          Physical Address           : 1.0.0.0
>>>          Logical Address Mask       : 0x0000
>>>          CEC Version                : 2.0
>>>          Logical Addresses          : 0
>>>
>>> #cec-ctl --playback
>>> [ 1038.761545] cec-3-0039: cec_thread_func: message 44 timed out!
>>
>> This isn't right. You shouldn't see this. It never receives an interrupt
>> when the transmit has finished, which causes these time outs.
>>
>> What are you testing this on? The Dragonboard c410?
> 
> Yes.

On top of which kernel tree are these patches applied? I can try and reproduce
it.

Regards,

	Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel