* [PATCH RFC 1/5] video: add HDMI state notifier support
2016-08-12 14:14 [PATCH RFC 0/5] CEC drivers for iMX6 and TDA9950 Russell King - ARM Linux
@ 2016-08-12 14:14 ` Russell King
2016-08-12 15:26 ` Hans Verkuil
2016-08-12 14:15 ` [PATCH RFC 2/5] drm/bridge: dw_hdmi: remove CEC engine register definitions Russell King
` (3 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Russell King @ 2016-08-12 14:14 UTC (permalink / raw)
To: David Airlie, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Hans Verkuil
Cc: linux-fbdev, linux-arm-kernel, dri-devel
Add support for HDMI hotplug and EDID notifiers, which is used to convey
information from HDMI drivers to their CEC and audio counterparts.
Acked-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
drivers/video/Kconfig | 3 +++
drivers/video/Makefile | 1 +
drivers/video/hdmi-notifier.c | 61 +++++++++++++++++++++++++++++++++++++++++++
include/linux/hdmi-notifier.h | 44 +++++++++++++++++++++++++++++++
4 files changed, 109 insertions(+)
create mode 100644 drivers/video/hdmi-notifier.c
create mode 100644 include/linux/hdmi-notifier.h
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 3c20af999893..1ee7b9f9bb25 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -36,6 +36,9 @@ config VIDEOMODE_HELPERS
config HDMI
bool
+config HDMI_NOTIFIERS
+ bool
+
if VT
source "drivers/video/console/Kconfig"
endif
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 9ad3c17d6456..65f564906fb4 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -1,5 +1,6 @@
obj-$(CONFIG_VGASTATE) += vgastate.o
obj-$(CONFIG_HDMI) += hdmi.o
+obj-$(CONFIG_HDMI_NOTIFIERS) += hdmi-notifier.o
obj-$(CONFIG_VT) += console/
obj-$(CONFIG_LOGO) += logo/
diff --git a/drivers/video/hdmi-notifier.c b/drivers/video/hdmi-notifier.c
new file mode 100644
index 000000000000..f3b16552b0fe
--- /dev/null
+++ b/drivers/video/hdmi-notifier.c
@@ -0,0 +1,61 @@
+#include <linux/export.h>
+#include <linux/hdmi-notifier.h>
+#include <linux/notifier.h>
+#include <linux/string.h>
+
+static BLOCKING_NOTIFIER_HEAD(hdmi_notifier);
+
+int hdmi_register_notifier(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_register(&hdmi_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(hdmi_register_notifier);
+
+int hdmi_unregister_notifier(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_unregister(&hdmi_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(hdmi_unregister_notifier);
+
+void hdmi_event_connect(struct device *dev)
+{
+ struct hdmi_event_base base;
+
+ base.source = dev;
+
+ blocking_notifier_call_chain(&hdmi_notifier, HDMI_CONNECTED, &base);
+}
+EXPORT_SYMBOL_GPL(hdmi_event_connect);
+
+void hdmi_event_disconnect(struct device *dev)
+{
+ struct hdmi_event_base base;
+
+ base.source = dev;
+
+ blocking_notifier_call_chain(&hdmi_notifier, HDMI_DISCONNECTED, &base);
+}
+EXPORT_SYMBOL_GPL(hdmi_event_disconnect);
+
+void hdmi_event_new_edid(struct device *dev, const void *edid, size_t size)
+{
+ struct hdmi_event_new_edid new_edid;
+
+ new_edid.base.source = dev;
+ new_edid.edid = edid;
+ new_edid.size = size;
+
+ blocking_notifier_call_chain(&hdmi_notifier, HDMI_NEW_EDID, &new_edid);
+}
+EXPORT_SYMBOL_GPL(hdmi_event_new_edid);
+
+void hdmi_event_new_eld(struct device *dev, const u8 eld[128])
+{
+ struct hdmi_event_new_eld new_eld;
+
+ new_eld.base.source = dev;
+ memcpy(new_eld.eld, eld, sizeof(new_eld.eld));
+
+ blocking_notifier_call_chain(&hdmi_notifier, HDMI_NEW_ELD, &new_eld);
+}
+EXPORT_SYMBOL_GPL(hdmi_event_new_eld);
diff --git a/include/linux/hdmi-notifier.h b/include/linux/hdmi-notifier.h
new file mode 100644
index 000000000000..5fb710e5d68a
--- /dev/null
+++ b/include/linux/hdmi-notifier.h
@@ -0,0 +1,44 @@
+#ifndef LINUX_HDMI_NOTIFIER_H
+#define LINUX_HDMI_NOTIFIER_H
+
+#include <linux/types.h>
+
+enum {
+ HDMI_CONNECTED,
+ HDMI_DISCONNECTED,
+ HDMI_NEW_EDID,
+ HDMI_NEW_ELD,
+};
+
+struct hdmi_event_base {
+ struct device *source;
+};
+
+struct hdmi_event_new_edid {
+ struct hdmi_event_base base;
+ const void *edid;
+ size_t size;
+};
+
+struct hdmi_event_new_eld {
+ struct hdmi_event_base base;
+ unsigned char eld[128];
+};
+
+union hdmi_event {
+ struct hdmi_event_base base;
+ struct hdmi_event_new_edid edid;
+ struct hdmi_event_new_eld eld;
+};
+
+struct notifier_block;
+
+int hdmi_register_notifier(struct notifier_block *nb);
+int hdmi_unregister_notifier(struct notifier_block *nb);
+
+void hdmi_event_connect(struct device *dev);
+void hdmi_event_disconnect(struct device *dev);
+void hdmi_event_new_edid(struct device *dev, const void *edid, size_t size);
+void hdmi_event_new_eld(struct device *dev, const u8 eld[128]);
+
+#endif
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH RFC 1/5] video: add HDMI state notifier support
2016-08-12 14:14 ` [PATCH RFC 1/5] video: add HDMI state notifier support Russell King
@ 2016-08-12 15:26 ` Hans Verkuil
0 siblings, 0 replies; 21+ messages in thread
From: Hans Verkuil @ 2016-08-12 15:26 UTC (permalink / raw)
To: Russell King, David Airlie, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
Cc: linux-fbdev, linux-arm-kernel, dri-devel
On 08/12/2016 04:14 PM, Russell King wrote:
> Add support for HDMI hotplug and EDID notifiers, which is used to convey
> information from HDMI drivers to their CEC and audio counterparts.
>
> Acked-by: Philipp Zabel <p.zabel@pengutronix.de>
I still don't really like the void *, but not enough to block this, so:
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Regards,
Hans
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
> drivers/video/Kconfig | 3 +++
> drivers/video/Makefile | 1 +
> drivers/video/hdmi-notifier.c | 61 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/hdmi-notifier.h | 44 +++++++++++++++++++++++++++++++
> 4 files changed, 109 insertions(+)
> create mode 100644 drivers/video/hdmi-notifier.c
> create mode 100644 include/linux/hdmi-notifier.h
>
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 3c20af999893..1ee7b9f9bb25 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -36,6 +36,9 @@ config VIDEOMODE_HELPERS
> config HDMI
> bool
>
> +config HDMI_NOTIFIERS
> + bool
> +
> if VT
> source "drivers/video/console/Kconfig"
> endif
> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> index 9ad3c17d6456..65f564906fb4 100644
> --- a/drivers/video/Makefile
> +++ b/drivers/video/Makefile
> @@ -1,5 +1,6 @@
> obj-$(CONFIG_VGASTATE) += vgastate.o
> obj-$(CONFIG_HDMI) += hdmi.o
> +obj-$(CONFIG_HDMI_NOTIFIERS) += hdmi-notifier.o
>
> obj-$(CONFIG_VT) += console/
> obj-$(CONFIG_LOGO) += logo/
> diff --git a/drivers/video/hdmi-notifier.c b/drivers/video/hdmi-notifier.c
> new file mode 100644
> index 000000000000..f3b16552b0fe
> --- /dev/null
> +++ b/drivers/video/hdmi-notifier.c
> @@ -0,0 +1,61 @@
> +#include <linux/export.h>
> +#include <linux/hdmi-notifier.h>
> +#include <linux/notifier.h>
> +#include <linux/string.h>
> +
> +static BLOCKING_NOTIFIER_HEAD(hdmi_notifier);
> +
> +int hdmi_register_notifier(struct notifier_block *nb)
> +{
> + return blocking_notifier_chain_register(&hdmi_notifier, nb);
> +}
> +EXPORT_SYMBOL_GPL(hdmi_register_notifier);
> +
> +int hdmi_unregister_notifier(struct notifier_block *nb)
> +{
> + return blocking_notifier_chain_unregister(&hdmi_notifier, nb);
> +}
> +EXPORT_SYMBOL_GPL(hdmi_unregister_notifier);
> +
> +void hdmi_event_connect(struct device *dev)
> +{
> + struct hdmi_event_base base;
> +
> + base.source = dev;
> +
> + blocking_notifier_call_chain(&hdmi_notifier, HDMI_CONNECTED, &base);
> +}
> +EXPORT_SYMBOL_GPL(hdmi_event_connect);
> +
> +void hdmi_event_disconnect(struct device *dev)
> +{
> + struct hdmi_event_base base;
> +
> + base.source = dev;
> +
> + blocking_notifier_call_chain(&hdmi_notifier, HDMI_DISCONNECTED, &base);
> +}
> +EXPORT_SYMBOL_GPL(hdmi_event_disconnect);
> +
> +void hdmi_event_new_edid(struct device *dev, const void *edid, size_t size)
> +{
> + struct hdmi_event_new_edid new_edid;
> +
> + new_edid.base.source = dev;
> + new_edid.edid = edid;
> + new_edid.size = size;
> +
> + blocking_notifier_call_chain(&hdmi_notifier, HDMI_NEW_EDID, &new_edid);
> +}
> +EXPORT_SYMBOL_GPL(hdmi_event_new_edid);
> +
> +void hdmi_event_new_eld(struct device *dev, const u8 eld[128])
> +{
> + struct hdmi_event_new_eld new_eld;
> +
> + new_eld.base.source = dev;
> + memcpy(new_eld.eld, eld, sizeof(new_eld.eld));
> +
> + blocking_notifier_call_chain(&hdmi_notifier, HDMI_NEW_ELD, &new_eld);
> +}
> +EXPORT_SYMBOL_GPL(hdmi_event_new_eld);
> diff --git a/include/linux/hdmi-notifier.h b/include/linux/hdmi-notifier.h
> new file mode 100644
> index 000000000000..5fb710e5d68a
> --- /dev/null
> +++ b/include/linux/hdmi-notifier.h
> @@ -0,0 +1,44 @@
> +#ifndef LINUX_HDMI_NOTIFIER_H
> +#define LINUX_HDMI_NOTIFIER_H
> +
> +#include <linux/types.h>
> +
> +enum {
> + HDMI_CONNECTED,
> + HDMI_DISCONNECTED,
> + HDMI_NEW_EDID,
> + HDMI_NEW_ELD,
> +};
> +
> +struct hdmi_event_base {
> + struct device *source;
> +};
> +
> +struct hdmi_event_new_edid {
> + struct hdmi_event_base base;
> + const void *edid;
> + size_t size;
> +};
> +
> +struct hdmi_event_new_eld {
> + struct hdmi_event_base base;
> + unsigned char eld[128];
> +};
> +
> +union hdmi_event {
> + struct hdmi_event_base base;
> + struct hdmi_event_new_edid edid;
> + struct hdmi_event_new_eld eld;
> +};
> +
> +struct notifier_block;
> +
> +int hdmi_register_notifier(struct notifier_block *nb);
> +int hdmi_unregister_notifier(struct notifier_block *nb);
> +
> +void hdmi_event_connect(struct device *dev);
> +void hdmi_event_disconnect(struct device *dev);
> +void hdmi_event_new_edid(struct device *dev, const void *edid, size_t size);
> +void hdmi_event_new_eld(struct device *dev, const u8 eld[128]);
> +
> +#endif
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH RFC 2/5] drm/bridge: dw_hdmi: remove CEC engine register definitions
2016-08-12 14:14 [PATCH RFC 0/5] CEC drivers for iMX6 and TDA9950 Russell King - ARM Linux
2016-08-12 14:14 ` [PATCH RFC 1/5] video: add HDMI state notifier support Russell King
@ 2016-08-12 14:15 ` Russell King
2016-08-12 14:15 ` [PATCH RFC 3/5] drm/bridge: dw_hdmi: add HDMI notifier support Russell King
` (2 subsequent siblings)
4 siblings, 0 replies; 21+ messages in thread
From: Russell King @ 2016-08-12 14:15 UTC (permalink / raw)
To: David Airlie, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Hans Verkuil
Cc: linux-fbdev, linux-arm-kernel, dri-devel
We don't need the CEC engine register definitions, so let's remove them.
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
drivers/gpu/drm/bridge/dw-hdmi.h | 45 ----------------------------------------
1 file changed, 45 deletions(-)
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.h b/drivers/gpu/drm/bridge/dw-hdmi.h
index fc9a560429d6..26d6845efad6 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.h
+++ b/drivers/gpu/drm/bridge/dw-hdmi.h
@@ -478,51 +478,6 @@
#define HDMI_A_PRESETUP 0x501A
#define HDMI_A_SRM_BASE 0x5020
-/* CEC Engine Registers */
-#define HDMI_CEC_CTRL 0x7D00
-#define HDMI_CEC_STAT 0x7D01
-#define HDMI_CEC_MASK 0x7D02
-#define HDMI_CEC_POLARITY 0x7D03
-#define HDMI_CEC_INT 0x7D04
-#define HDMI_CEC_ADDR_L 0x7D05
-#define HDMI_CEC_ADDR_H 0x7D06
-#define HDMI_CEC_TX_CNT 0x7D07
-#define HDMI_CEC_RX_CNT 0x7D08
-#define HDMI_CEC_TX_DATA0 0x7D10
-#define HDMI_CEC_TX_DATA1 0x7D11
-#define HDMI_CEC_TX_DATA2 0x7D12
-#define HDMI_CEC_TX_DATA3 0x7D13
-#define HDMI_CEC_TX_DATA4 0x7D14
-#define HDMI_CEC_TX_DATA5 0x7D15
-#define HDMI_CEC_TX_DATA6 0x7D16
-#define HDMI_CEC_TX_DATA7 0x7D17
-#define HDMI_CEC_TX_DATA8 0x7D18
-#define HDMI_CEC_TX_DATA9 0x7D19
-#define HDMI_CEC_TX_DATA10 0x7D1a
-#define HDMI_CEC_TX_DATA11 0x7D1b
-#define HDMI_CEC_TX_DATA12 0x7D1c
-#define HDMI_CEC_TX_DATA13 0x7D1d
-#define HDMI_CEC_TX_DATA14 0x7D1e
-#define HDMI_CEC_TX_DATA15 0x7D1f
-#define HDMI_CEC_RX_DATA0 0x7D20
-#define HDMI_CEC_RX_DATA1 0x7D21
-#define HDMI_CEC_RX_DATA2 0x7D22
-#define HDMI_CEC_RX_DATA3 0x7D23
-#define HDMI_CEC_RX_DATA4 0x7D24
-#define HDMI_CEC_RX_DATA5 0x7D25
-#define HDMI_CEC_RX_DATA6 0x7D26
-#define HDMI_CEC_RX_DATA7 0x7D27
-#define HDMI_CEC_RX_DATA8 0x7D28
-#define HDMI_CEC_RX_DATA9 0x7D29
-#define HDMI_CEC_RX_DATA10 0x7D2a
-#define HDMI_CEC_RX_DATA11 0x7D2b
-#define HDMI_CEC_RX_DATA12 0x7D2c
-#define HDMI_CEC_RX_DATA13 0x7D2d
-#define HDMI_CEC_RX_DATA14 0x7D2e
-#define HDMI_CEC_RX_DATA15 0x7D2f
-#define HDMI_CEC_LOCK 0x7D30
-#define HDMI_CEC_WKUPCTRL 0x7D31
-
/* I2C Master Registers (E-DDC) */
#define HDMI_I2CM_SLAVE 0x7E00
#define HDMI_I2CM_ADDRESS 0x7E01
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH RFC 3/5] drm/bridge: dw_hdmi: add HDMI notifier support
2016-08-12 14:14 [PATCH RFC 0/5] CEC drivers for iMX6 and TDA9950 Russell King - ARM Linux
2016-08-12 14:14 ` [PATCH RFC 1/5] video: add HDMI state notifier support Russell King
2016-08-12 14:15 ` [PATCH RFC 2/5] drm/bridge: dw_hdmi: remove CEC engine register definitions Russell King
@ 2016-08-12 14:15 ` Russell King
2016-08-12 14:15 ` [PATCH RFC 4/5] drm/bridge: add dw-hdmi cec driver using Hans Verkil's CEC code Russell King
2016-08-12 14:15 ` [PATCH RFC 5/5] drm/i2c: add tda998x/tda9950 CEC driver Russell King
4 siblings, 0 replies; 21+ messages in thread
From: Russell King @ 2016-08-12 14:15 UTC (permalink / raw)
To: David Airlie, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Hans Verkuil
Cc: linux-fbdev, linux-arm-kernel, dri-devel
Add HDMI notifiers to the HDMI bridge driver.
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
drivers/gpu/drm/bridge/Kconfig | 1 +
drivers/gpu/drm/bridge/dw-hdmi.c | 9 +++++++++
2 files changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 8f7423f18da5..6d97d9f2b164 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -20,6 +20,7 @@ config DRM_ANALOGIX_ANX78XX
config DRM_DW_HDMI
tristate
select DRM_KMS_HELPER
+ select HDMI_NOTIFIERS
config DRM_DW_HDMI_AHB_AUDIO
tristate "Synopsis Designware AHB Audio interface"
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index c9d941283d30..cb6e03efbebf 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -16,6 +16,7 @@
#include <linux/err.h>
#include <linux/clk.h>
#include <linux/hdmi.h>
+#include <linux/hdmi-notifier.h>
#include <linux/mutex.h>
#include <linux/of_device.h>
#include <linux/spinlock.h>
@@ -1447,9 +1448,11 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
drm_mode_connector_update_edid_property(connector, edid);
+ hdmi_event_new_edid(hdmi->dev, edid, 0);
ret = drm_add_edid_modes(connector, edid);
/* Store the ELD */
drm_edid_to_eld(connector, edid);
+ hdmi_event_new_eld(hdmi->dev, connector->eld);
kfree(edid);
} else {
dev_dbg(hdmi->dev, "failed to get edid\n");
@@ -1601,6 +1604,12 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
dw_hdmi_update_phy_mask(hdmi);
}
mutex_unlock(&hdmi->mutex);
+
+ if ((phy_stat & (HDMI_PHY_RX_SENSE | HDMI_PHY_HPD)) = 0)
+ hdmi_event_disconnect(hdmi->dev);
+ else if ((phy_stat & (HDMI_PHY_RX_SENSE | HDMI_PHY_HPD)) =
+ (HDMI_IH_PHY_STAT0_RX_SENSE | HDMI_PHY_HPD))
+ hdmi_event_connect(hdmi->dev);
}
if (intr_stat & HDMI_IH_PHY_STAT0_HPD) {
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH RFC 4/5] drm/bridge: add dw-hdmi cec driver using Hans Verkil's CEC code
2016-08-12 14:14 [PATCH RFC 0/5] CEC drivers for iMX6 and TDA9950 Russell King - ARM Linux
` (2 preceding siblings ...)
2016-08-12 14:15 ` [PATCH RFC 3/5] drm/bridge: dw_hdmi: add HDMI notifier support Russell King
@ 2016-08-12 14:15 ` Russell King
2016-08-12 14:25 ` Hans Verkuil
` (2 more replies)
2016-08-12 14:15 ` [PATCH RFC 5/5] drm/i2c: add tda998x/tda9950 CEC driver Russell King
4 siblings, 3 replies; 21+ messages in thread
From: Russell King @ 2016-08-12 14:15 UTC (permalink / raw)
To: David Airlie, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Hans Verkuil
Cc: linux-fbdev, linux-arm-kernel, dri-devel
Add a CEC driver for the dw-hdmi hardware using Hans Verkil's CEC
implementation.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/gpu/drm/bridge/Kconfig | 7 +
drivers/gpu/drm/bridge/Makefile | 1 +
drivers/gpu/drm/bridge/dw-hdmi-cec.c | 344 ++++++++++++++++++++++++++++++
drivers/gpu/drm/bridge/dw-hdmi.c | 64 +++++-
include/linux/platform_data/dw_hdmi-cec.h | 16 ++
5 files changed, 421 insertions(+), 11 deletions(-)
create mode 100644 drivers/gpu/drm/bridge/dw-hdmi-cec.c
create mode 100644 include/linux/platform_data/dw_hdmi-cec.h
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 6d97d9f2b164..3ad19d16f4ca 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -33,6 +33,13 @@ config DRM_DW_HDMI_AHB_AUDIO
Designware HDMI block. This is used in conjunction with
the i.MX6 HDMI driver.
+config DRM_DW_HDMI_CEC
+ tristate "Synopsis Designware CEC interface"
+ depends on DRM_DW_HDMI && MEDIA_CEC
+ help
+ Support the CE interface which is part of the Synopsis
+ Designware HDMI block.
+
config DRM_NXP_PTN3460
tristate "NXP PTN3460 DP/LVDS bridge"
depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 96b13b30e6ab..4869441d35f4 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -3,6 +3,7 @@ ccflags-y := -Iinclude/drm
obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o
obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o
+obj-$(CONFIG_DRM_DW_HDMI_CEC) += dw-hdmi-cec.o
obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
diff --git a/drivers/gpu/drm/bridge/dw-hdmi-cec.c b/drivers/gpu/drm/bridge/dw-hdmi-cec.c
new file mode 100644
index 000000000000..9d6b8daa584d
--- /dev/null
+++ b/drivers/gpu/drm/bridge/dw-hdmi-cec.c
@@ -0,0 +1,344 @@
+/* http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/
+ * tree/drivers/mxc/hdmi-cec/mxc_hdmi-cec.c?h=imx_3.0.35_4.1.0 */
+#include <linux/hdmi-notifier.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/platform_data/dw_hdmi-cec.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+
+#include <drm/drm_edid.h>
+
+#include <media/cec.h>
+#include <media/cec-edid.h>
+
+#define DEV_NAME "mxc_hdmi_cec"
+
+enum {
+ HDMI_IH_CEC_STAT0 = 0x0106,
+ HDMI_IH_MUTE_CEC_STAT0 = 0x0186,
+
+ HDMI_CEC_CTRL = 0x7d00,
+ CEC_CTRL_START = BIT(0),
+ CEC_CTRL_NORMAL = 1 << 1,
+
+ HDMI_CEC_STAT = 0x7d01,
+ CEC_STAT_DONE = BIT(0),
+ CEC_STAT_EOM = BIT(1),
+ CEC_STAT_NACK = BIT(2),
+ CEC_STAT_ARBLOST = BIT(3),
+ CEC_STAT_ERROR_INIT = BIT(4),
+ CEC_STAT_ERROR_FOLL = BIT(5),
+ CEC_STAT_WAKEUP = BIT(6),
+
+ HDMI_CEC_MASK = 0x7d02,
+ HDMI_CEC_POLARITY = 0x7d03,
+ HDMI_CEC_INT = 0x7d04,
+ HDMI_CEC_ADDR_L = 0x7d05,
+ HDMI_CEC_ADDR_H = 0x7d06,
+ HDMI_CEC_TX_CNT = 0x7d07,
+ HDMI_CEC_RX_CNT = 0x7d08,
+ HDMI_CEC_TX_DATA0 = 0x7d10,
+ HDMI_CEC_RX_DATA0 = 0x7d20,
+ HDMI_CEC_LOCK = 0x7d30,
+ HDMI_CEC_WKUPCTRL = 0x7d31,
+};
+
+struct dw_hdmi_cec {
+ void __iomem *base;
+ u32 addresses;
+ struct cec_adapter *adap;
+ struct cec_msg rx_msg;
+ unsigned int tx_status;
+ bool tx_done;
+ bool rx_done;
+ const struct dw_hdmi_cec_ops *ops;
+ void *ops_data;
+ int retries;
+ int irq;
+ struct notifier_block nb;
+};
+
+static int dw_hdmi_cec_log_addr(struct cec_adapter *adap, u8 logical_addr)
+{
+ struct dw_hdmi_cec *cec = adap->priv;
+ u32 addresses;
+
+ if (logical_addr = CEC_LOG_ADDR_INVALID)
+ addresses = cec->addresses = BIT(15);
+ else
+ addresses = cec->addresses |= BIT(logical_addr);
+
+ writeb_relaxed(addresses & 255, cec->base + HDMI_CEC_ADDR_L);
+ writeb_relaxed(addresses >> 8, cec->base + HDMI_CEC_ADDR_H);
+
+ return 0;
+}
+
+static int dw_hdmi_cec_transmit(struct cec_adapter *adap, u8 attempts,
+ u32 signal_free_time, struct cec_msg *msg)
+{
+ struct dw_hdmi_cec *cec = adap->priv;
+ unsigned i;
+
+ cec->retries = attempts;
+
+ for (i = 0; i < msg->len; i++)
+ writeb_relaxed(msg->msg[i], cec->base + HDMI_CEC_TX_DATA0 + i);
+
+ writeb_relaxed(msg->len, cec->base + HDMI_CEC_TX_CNT);
+ writeb_relaxed(CEC_CTRL_NORMAL | CEC_CTRL_START, cec->base + HDMI_CEC_CTRL);
+
+ return 0;
+}
+
+static irqreturn_t dw_hdmi_cec_hardirq(int irq, void *data)
+{
+ struct cec_adapter *adap = data;
+ struct dw_hdmi_cec *cec = adap->priv;
+ unsigned stat = readb_relaxed(cec->base + HDMI_IH_CEC_STAT0);
+ irqreturn_t ret = IRQ_HANDLED;
+
+ if (stat = 0)
+ return IRQ_NONE;
+
+ writeb_relaxed(stat, cec->base + HDMI_IH_CEC_STAT0);
+
+ if (stat & CEC_STAT_ERROR_INIT) {
+ if (cec->retries) {
+ unsigned v = readb_relaxed(cec->base + HDMI_CEC_CTRL);
+ writeb_relaxed(v | CEC_CTRL_START, cec->base + HDMI_CEC_CTRL);
+ cec->retries -= 1;
+ } else {
+ cec->tx_status = CEC_TX_STATUS_MAX_RETRIES;
+ cec->tx_done = true;
+ ret = IRQ_WAKE_THREAD;
+ }
+ } else if (stat & CEC_STAT_DONE) {
+ cec->tx_status = CEC_TX_STATUS_OK;
+ cec->tx_done = true;
+ ret = IRQ_WAKE_THREAD;
+ } else if (stat & CEC_STAT_NACK) {
+ cec->tx_status = CEC_TX_STATUS_NACK;
+ cec->tx_done = true;
+ ret = IRQ_WAKE_THREAD;
+ }
+
+ if (stat & CEC_STAT_EOM) {
+ unsigned len, i;
+ void *base = cec->base;
+
+ len = readb_relaxed(base + HDMI_CEC_RX_CNT);
+ if (len > sizeof(cec->rx_msg.msg))
+ len = sizeof(cec->rx_msg.msg);
+
+ for (i = 0; i < len; i++)
+ cec->rx_msg.msg[i] + readb_relaxed(base + HDMI_CEC_RX_DATA0 + i);
+
+ writeb_relaxed(0, base + HDMI_CEC_LOCK);
+
+ cec->rx_msg.len = len;
+ smp_wmb();
+ cec->rx_done = true;
+
+ ret = IRQ_WAKE_THREAD;
+ }
+
+ return ret;
+}
+
+static irqreturn_t dw_hdmi_cec_thread(int irq, void *data)
+{
+ struct cec_adapter *adap = data;
+ struct dw_hdmi_cec *cec = adap->priv;
+
+ if (cec->tx_done) {
+ cec->tx_done = false;
+ cec_transmit_done(adap, cec->tx_status, 0, 0, 0, 0);
+ }
+ if (cec->rx_done) {
+ cec->rx_done = false;
+ smp_rmb();
+ cec_received_msg(adap, &cec->rx_msg);
+ }
+ return IRQ_HANDLED;
+}
+
+static int dw_hdmi_cec_enable(struct cec_adapter *adap, bool enable)
+{
+ struct dw_hdmi_cec *cec = adap->priv;
+
+ if (!enable) {
+ writeb_relaxed(~0, cec->base + HDMI_CEC_MASK);
+ writeb_relaxed(~0, cec->base + HDMI_IH_MUTE_CEC_STAT0);
+ writeb_relaxed(0, cec->base + HDMI_CEC_POLARITY);
+
+ cec->ops->disable(cec->ops_data);
+ } else {
+ unsigned irqs;
+
+ writeb_relaxed(0, cec->base + HDMI_CEC_CTRL);
+ writeb_relaxed(~0, cec->base + HDMI_IH_CEC_STAT0);
+ writeb_relaxed(0, cec->base + HDMI_CEC_LOCK);
+
+ dw_hdmi_cec_log_addr(cec->adap, CEC_LOG_ADDR_INVALID);
+
+ cec->ops->enable(cec->ops_data);
+
+ irqs = CEC_STAT_ERROR_INIT | CEC_STAT_NACK | CEC_STAT_EOM |
+ CEC_STAT_DONE;
+ writeb_relaxed(irqs, cec->base + HDMI_CEC_POLARITY);
+ writeb_relaxed(~irqs, cec->base + HDMI_CEC_MASK);
+ writeb_relaxed(~irqs, cec->base + HDMI_IH_MUTE_CEC_STAT0);
+ }
+ return 0;
+}
+
+static const struct cec_adap_ops dw_hdmi_cec_ops = {
+ .adap_enable = dw_hdmi_cec_enable,
+ .adap_log_addr = dw_hdmi_cec_log_addr,
+ .adap_transmit = dw_hdmi_cec_transmit,
+};
+
+static unsigned int parse_hdmi_addr(const struct edid *edid)
+{
+ if (!edid || edid->extensions = 0)
+ return (u16)~0;
+
+ return cec_get_edid_phys_addr((u8 *)edid,
+ EDID_LENGTH * (edid->extensions + 1), NULL);
+}
+
+static int dw_hdmi_cec_notify(struct notifier_block *nb, unsigned long event,
+ void *data)
+{
+ struct dw_hdmi_cec *cec = container_of(nb, struct dw_hdmi_cec, nb);
+ union hdmi_event *event_block = data;
+ unsigned int phys;
+
+ dev_info(event_block->base.source, "event %lu\n", event);
+
+ if (event_block->base.source != cec->adap->devnode.parent)
+ return NOTIFY_OK;
+
+ switch (event) {
+ case HDMI_CONNECTED:
+ break;
+
+ case HDMI_DISCONNECTED:
+ cec_s_phys_addr(cec->adap, CEC_PHYS_ADDR_INVALID, false);
+ break;
+
+ case HDMI_NEW_EDID:
+ phys = parse_hdmi_addr(event_block->edid.edid);
+ cec_s_phys_addr(cec->adap, phys, false);
+ break;
+ }
+
+ return NOTIFY_OK;
+}
+
+static void dw_hdmi_cec_del(void *data)
+{
+ struct dw_hdmi_cec *cec = data;
+
+ cec_delete_adapter(cec->adap);
+}
+
+static int dw_hdmi_cec_probe(struct platform_device *pdev)
+{
+ struct dw_hdmi_cec_data *data = dev_get_platdata(&pdev->dev);
+ struct dw_hdmi_cec *cec;
+ int ret;
+
+ if (!data)
+ return -ENXIO;
+
+ /*
+ * Our device is just a convenience - we want to link to the real
+ * hardware device here, so that userspace can see the association
+ * between the HDMI hardware and its associated CEC chardev.
+ */
+ cec = devm_kzalloc(&pdev->dev, sizeof(*cec), GFP_KERNEL);
+ if (!cec)
+ return -ENOMEM;
+
+ cec->base = data->base;
+ cec->irq = data->irq;
+ cec->ops = data->ops;
+ cec->ops_data = data->ops_data;
+ cec->nb.notifier_call = dw_hdmi_cec_notify;
+
+ platform_set_drvdata(pdev, cec);
+
+ writeb_relaxed(0, cec->base + HDMI_CEC_TX_CNT);
+ writeb_relaxed(~0, cec->base + HDMI_CEC_MASK);
+ writeb_relaxed(~0, cec->base + HDMI_IH_MUTE_CEC_STAT0);
+ writeb_relaxed(0, cec->base + HDMI_CEC_POLARITY);
+
+ cec->adap = cec_allocate_adapter(&dw_hdmi_cec_ops, cec, "dw_hdmi",
+ CEC_CAP_LOG_ADDRS | CEC_CAP_TRANSMIT |
+ CEC_CAP_RC, CEC_MAX_LOG_ADDRS,
+ pdev->dev.parent);
+ if (IS_ERR(cec->adap))
+ return PTR_ERR(cec->adap);
+
+ /* override the module pointer */
+ cec->adap->owner = THIS_MODULE;
+
+ ret = devm_add_action(&pdev->dev, dw_hdmi_cec_del, cec);
+ if (ret) {
+ cec_delete_adapter(cec->adap);
+ return ret;
+ }
+
+ ret = devm_request_threaded_irq(&pdev->dev, cec->irq,
+ dw_hdmi_cec_hardirq,
+ dw_hdmi_cec_thread, IRQF_SHARED,
+ DEV_NAME, cec->adap);
+ if (ret < 0)
+ return ret;
+
+ ret = cec_register_adapter(cec->adap);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * CEC documentation says we must not call cec_delete_adapter
+ * after a successful call to cec_register_adapter().
+ */
+ devm_remove_action(&pdev->dev, dw_hdmi_cec_del, cec);
+
+ hdmi_register_notifier(&cec->nb);
+
+ return 0;
+}
+
+static int dw_hdmi_cec_remove(struct platform_device *pdev)
+{
+ struct dw_hdmi_cec *cec = platform_get_drvdata(pdev);
+
+ hdmi_unregister_notifier(&cec->nb);
+ cec_unregister_adapter(cec->adap);
+
+ return 0;
+}
+
+static struct platform_driver dw_hdmi_cec_driver = {
+ .probe = dw_hdmi_cec_probe,
+ .remove = dw_hdmi_cec_remove,
+ .driver = {
+ .name = "dw-hdmi-cec",
+ .owner = THIS_MODULE,
+ },
+};
+module_platform_driver(dw_hdmi_cec_driver);
+
+MODULE_AUTHOR("Russell King <rmk+kernel@arm.linux.org.uk>");
+MODULE_DESCRIPTION("Synopsis Designware HDMI CEC driver for i.MX");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS(PLATFORM_MODULE_PREFIX "dw-hdmi-cec");
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index cb6e03efbebf..1ad58e37c0b3 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -19,6 +19,7 @@
#include <linux/hdmi-notifier.h>
#include <linux/mutex.h>
#include <linux/of_device.h>
+#include <linux/platform_data/dw_hdmi-cec.h>
#include <linux/spinlock.h>
#include <drm/drm_of.h>
@@ -108,6 +109,7 @@ struct dw_hdmi {
struct drm_bridge *bridge;
struct platform_device *audio;
+ struct platform_device *cec;
enum dw_hdmi_devtype dev_type;
struct device *dev;
struct clk *isfr_clk;
@@ -119,6 +121,7 @@ struct dw_hdmi {
int vic;
u8 edid[HDMI_EDID_LEN];
+ u8 mc_clkdis;
bool cable_plugin;
bool phy_enabled;
@@ -1108,8 +1111,6 @@ static void dw_hdmi_phy_disable(struct dw_hdmi *hdmi)
/* HDMI Initialization Step B.4 */
static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi)
{
- u8 clkdis;
-
/* control period minimum duration */
hdmi_writeb(hdmi, 12, HDMI_FC_CTRLDUR);
hdmi_writeb(hdmi, 32, HDMI_FC_EXCTRLDUR);
@@ -1121,23 +1122,28 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi)
hdmi_writeb(hdmi, 0x21, HDMI_FC_CH2PREAM);
/* Enable pixel clock and tmds data path */
- clkdis = 0x7F;
- clkdis &= ~HDMI_MC_CLKDIS_PIXELCLK_DISABLE;
- hdmi_writeb(hdmi, clkdis, HDMI_MC_CLKDIS);
+ hdmi->mc_clkdis |= HDMI_MC_CLKDIS_HDCPCLK_DISABLE |
+ HDMI_MC_CLKDIS_CSCCLK_DISABLE |
+ HDMI_MC_CLKDIS_AUDCLK_DISABLE |
+ HDMI_MC_CLKDIS_PREPCLK_DISABLE |
+ HDMI_MC_CLKDIS_TMDSCLK_DISABLE;
+ hdmi->mc_clkdis &= ~HDMI_MC_CLKDIS_PIXELCLK_DISABLE;
+ hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
- clkdis &= ~HDMI_MC_CLKDIS_TMDSCLK_DISABLE;
- hdmi_writeb(hdmi, clkdis, HDMI_MC_CLKDIS);
+ hdmi->mc_clkdis &= ~HDMI_MC_CLKDIS_TMDSCLK_DISABLE;
+ hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
/* Enable csc path */
if (is_color_space_conversion(hdmi)) {
- clkdis &= ~HDMI_MC_CLKDIS_CSCCLK_DISABLE;
- hdmi_writeb(hdmi, clkdis, HDMI_MC_CLKDIS);
+ hdmi->mc_clkdis &= ~HDMI_MC_CLKDIS_CSCCLK_DISABLE;
+ hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
}
}
static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi)
{
- hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
+ hdmi->mc_clkdis &= ~HDMI_MC_CLKDIS_AUDCLK_DISABLE;
+ hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
}
/* Workaround to clear the overflow condition */
@@ -1297,7 +1303,6 @@ static void initialize_hdmi_ih_mutes(struct dw_hdmi *hdmi)
hdmi_writeb(hdmi, 0xff, HDMI_AUD_HBR_MASK);
hdmi_writeb(hdmi, 0xff, HDMI_GP_MASK);
hdmi_writeb(hdmi, 0xff, HDMI_A_APIINTMSK);
- hdmi_writeb(hdmi, 0xff, HDMI_CEC_MASK);
hdmi_writeb(hdmi, 0xff, HDMI_I2CM_INT);
hdmi_writeb(hdmi, 0xff, HDMI_I2CM_CTLINT);
@@ -1666,6 +1671,27 @@ static int dw_hdmi_register(struct drm_device *drm, struct dw_hdmi *hdmi)
return 0;
}
+static void dw_hdmi_cec_enable(void *data)
+{
+ struct dw_hdmi *hdmi = data;
+
+ hdmi->mc_clkdis &= ~HDMI_MC_CLKDIS_CECCLK_DISABLE;
+ hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
+}
+
+static void dw_hdmi_cec_disable(void *data)
+{
+ struct dw_hdmi *hdmi = data;
+
+ hdmi->mc_clkdis |= HDMI_MC_CLKDIS_CECCLK_DISABLE;
+ hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
+}
+
+static const struct dw_hdmi_cec_ops dw_hdmi_cec_ops = {
+ .enable = dw_hdmi_cec_enable,
+ .disable = dw_hdmi_cec_disable,
+};
+
int dw_hdmi_bind(struct device *dev, struct device *master,
void *data, struct drm_encoder *encoder,
struct resource *iores, int irq,
@@ -1676,6 +1702,7 @@ int dw_hdmi_bind(struct device *dev, struct device *master,
struct platform_device_info pdevinfo;
struct device_node *ddc_node;
struct dw_hdmi_audio_data audio;
+ struct dw_hdmi_cec_data cec;
struct dw_hdmi *hdmi;
int ret;
u32 val = 1;
@@ -1694,6 +1721,7 @@ int dw_hdmi_bind(struct device *dev, struct device *master,
hdmi->disabled = true;
hdmi->rxsense = true;
hdmi->phy_mask = (u8)~(HDMI_PHY_HPD | HDMI_PHY_RX_SENSE);
+ hdmi->mc_clkdis = 0x7f;
mutex_init(&hdmi->mutex);
mutex_init(&hdmi->audio_mutex);
@@ -1820,6 +1848,18 @@ int dw_hdmi_bind(struct device *dev, struct device *master,
hdmi->audio = platform_device_register_full(&pdevinfo);
}
+ cec.base = hdmi->regs;
+ cec.irq = irq;
+ cec.ops = &dw_hdmi_cec_ops;
+ cec.ops_data = hdmi;
+
+ pdevinfo.name = "dw-hdmi-cec";
+ pdevinfo.data = &cec;
+ pdevinfo.size_data = sizeof(cec);
+ pdevinfo.dma_mask = 0;
+
+ hdmi->cec = platform_device_register_full(&pdevinfo);
+
dev_set_drvdata(dev, hdmi);
return 0;
@@ -1839,6 +1879,8 @@ void dw_hdmi_unbind(struct device *dev, struct device *master, void *data)
if (hdmi->audio && !IS_ERR(hdmi->audio))
platform_device_unregister(hdmi->audio);
+ if (!IS_ERR(hdmi->cec))
+ platform_device_unregister(hdmi->cec);
/* Disable all interrupts */
hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
diff --git a/include/linux/platform_data/dw_hdmi-cec.h b/include/linux/platform_data/dw_hdmi-cec.h
new file mode 100644
index 000000000000..5ff40cc237a8
--- /dev/null
+++ b/include/linux/platform_data/dw_hdmi-cec.h
@@ -0,0 +1,16 @@
+#ifndef DW_HDMI_CEC_H
+#define DW_HDMI_CEC_H
+
+struct dw_hdmi_cec_ops {
+ void (*enable)(void *);
+ void (*disable)(void *);
+};
+
+struct dw_hdmi_cec_data {
+ void __iomem *base;
+ int irq;
+ const struct dw_hdmi_cec_ops *ops;
+ void *ops_data;
+};
+
+#endif
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH RFC 4/5] drm/bridge: add dw-hdmi cec driver using Hans Verkil's CEC code
2016-08-12 14:15 ` [PATCH RFC 4/5] drm/bridge: add dw-hdmi cec driver using Hans Verkil's CEC code Russell King
@ 2016-08-12 14:25 ` Hans Verkuil
2016-08-12 14:26 ` Russell King - ARM Linux
2016-08-23 7:21 ` Hans Verkuil
2016-08-23 8:03 ` Hans Verkuil
2 siblings, 1 reply; 21+ messages in thread
From: Hans Verkuil @ 2016-08-12 14:25 UTC (permalink / raw)
To: Russell King, David Airlie, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
Cc: linux-fbdev, linux-arm-kernel, dri-devel
On 08/12/2016 04:15 PM, Russell King wrote:
> Add a CEC driver for the dw-hdmi hardware using Hans Verkil's CEC
That's Verkuil :-)
BTW, since cec is part of the media subsystem please include linux-media for the next round.
Regards,
Hans
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RFC 4/5] drm/bridge: add dw-hdmi cec driver using Hans Verkil's CEC code
2016-08-12 14:25 ` Hans Verkuil
@ 2016-08-12 14:26 ` Russell King - ARM Linux
0 siblings, 0 replies; 21+ messages in thread
From: Russell King - ARM Linux @ 2016-08-12 14:26 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-fbdev, David Airlie, dri-devel, Tomi Valkeinen,
Jean-Christophe Plagniol-Villard, linux-arm-kernel
On Fri, Aug 12, 2016 at 04:25:02PM +0200, Hans Verkuil wrote:
> On 08/12/2016 04:15 PM, Russell King wrote:
> >Add a CEC driver for the dw-hdmi hardware using Hans Verkil's CEC
>
> That's Verkuil :-)
Oops, sorry about that.
> BTW, since cec is part of the media subsystem please include linux-media
> for the next round.
I can't guarantee that, I'm quite forgetful.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RFC 4/5] drm/bridge: add dw-hdmi cec driver using Hans Verkil's CEC code
2016-08-12 14:15 ` [PATCH RFC 4/5] drm/bridge: add dw-hdmi cec driver using Hans Verkil's CEC code Russell King
2016-08-12 14:25 ` Hans Verkuil
@ 2016-08-23 7:21 ` Hans Verkuil
2016-08-23 7:59 ` Russell King - ARM Linux
2016-08-23 8:03 ` Hans Verkuil
2 siblings, 1 reply; 21+ messages in thread
From: Hans Verkuil @ 2016-08-23 7:21 UTC (permalink / raw)
To: Russell King, David Airlie, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Hans Verkuil
Cc: linux-fbdev, dri-devel, linux-arm-kernel
Hi Russell,
On 08/12/2016 04:15 PM, Russell King wrote:
> Add a CEC driver for the dw-hdmi hardware using Hans Verkil's CEC
> implementation.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> drivers/gpu/drm/bridge/Kconfig | 7 +
> drivers/gpu/drm/bridge/Makefile | 1 +
> drivers/gpu/drm/bridge/dw-hdmi-cec.c | 344 ++++++++++++++++++++++++++++++
> drivers/gpu/drm/bridge/dw-hdmi.c | 64 +++++-
> include/linux/platform_data/dw_hdmi-cec.h | 16 ++
> 5 files changed, 421 insertions(+), 11 deletions(-)
> create mode 100644 drivers/gpu/drm/bridge/dw-hdmi-cec.c
> create mode 100644 include/linux/platform_data/dw_hdmi-cec.h
>
<snip>
> +static unsigned int parse_hdmi_addr(const struct edid *edid)
> +{
> + if (!edid || edid->extensions = 0)
> + return (u16)~0;
> +
> + return cec_get_edid_phys_addr((u8 *)edid,
> + EDID_LENGTH * (edid->extensions + 1), NULL);
> +}
> +
> +static int dw_hdmi_cec_notify(struct notifier_block *nb, unsigned long event,
> + void *data)
> +{
> + struct dw_hdmi_cec *cec = container_of(nb, struct dw_hdmi_cec, nb);
> + union hdmi_event *event_block = data;
> + unsigned int phys;
> +
> + dev_info(event_block->base.source, "event %lu\n", event);
> +
> + if (event_block->base.source != cec->adap->devnode.parent)
> + return NOTIFY_OK;
> +
> + switch (event) {
> + case HDMI_CONNECTED:
> + break;
> +
> + case HDMI_DISCONNECTED:
> + cec_s_phys_addr(cec->adap, CEC_PHYS_ADDR_INVALID, false);
> + break;
> +
> + case HDMI_NEW_EDID:
> + phys = parse_hdmi_addr(event_block->edid.edid);
> + cec_s_phys_addr(cec->adap, phys, false);
> + break;
> + }
> +
> + return NOTIFY_OK;
> +}
Wouldn't it make a lot of sense to integrate this into the cec framework?
All you need is to pass an hdmi_notifier_dev as argument to cec_allocate_adapter()
and you can integrate this.
If you are OK with that, then I can make patches for that.
Regards,
Hans
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH RFC 4/5] drm/bridge: add dw-hdmi cec driver using Hans Verkil's CEC code
2016-08-23 7:21 ` Hans Verkuil
@ 2016-08-23 7:59 ` Russell King - ARM Linux
2016-08-23 8:05 ` Hans Verkuil
0 siblings, 1 reply; 21+ messages in thread
From: Russell King - ARM Linux @ 2016-08-23 7:59 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-fbdev, dri-devel, Hans Verkuil, Tomi Valkeinen,
Jean-Christophe Plagniol-Villard, linux-arm-kernel
On Tue, Aug 23, 2016 at 09:21:17AM +0200, Hans Verkuil wrote:
> Hi Russell,
>
> On 08/12/2016 04:15 PM, Russell King wrote:
> > Add a CEC driver for the dw-hdmi hardware using Hans Verkil's CEC
> > implementation.
> >
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> > drivers/gpu/drm/bridge/Kconfig | 7 +
> > drivers/gpu/drm/bridge/Makefile | 1 +
> > drivers/gpu/drm/bridge/dw-hdmi-cec.c | 344 ++++++++++++++++++++++++++++++
> > drivers/gpu/drm/bridge/dw-hdmi.c | 64 +++++-
> > include/linux/platform_data/dw_hdmi-cec.h | 16 ++
> > 5 files changed, 421 insertions(+), 11 deletions(-)
> > create mode 100644 drivers/gpu/drm/bridge/dw-hdmi-cec.c
> > create mode 100644 include/linux/platform_data/dw_hdmi-cec.h
> >
>
> <snip>
>
> > +static unsigned int parse_hdmi_addr(const struct edid *edid)
> > +{
> > + if (!edid || edid->extensions = 0)
> > + return (u16)~0;
> > +
> > + return cec_get_edid_phys_addr((u8 *)edid,
> > + EDID_LENGTH * (edid->extensions + 1), NULL);
> > +}
> > +
> > +static int dw_hdmi_cec_notify(struct notifier_block *nb, unsigned long event,
> > + void *data)
> > +{
> > + struct dw_hdmi_cec *cec = container_of(nb, struct dw_hdmi_cec, nb);
> > + union hdmi_event *event_block = data;
> > + unsigned int phys;
> > +
> > + dev_info(event_block->base.source, "event %lu\n", event);
> > +
> > + if (event_block->base.source != cec->adap->devnode.parent)
> > + return NOTIFY_OK;
> > +
> > + switch (event) {
> > + case HDMI_CONNECTED:
> > + break;
> > +
> > + case HDMI_DISCONNECTED:
> > + cec_s_phys_addr(cec->adap, CEC_PHYS_ADDR_INVALID, false);
> > + break;
> > +
> > + case HDMI_NEW_EDID:
> > + phys = parse_hdmi_addr(event_block->edid.edid);
> > + cec_s_phys_addr(cec->adap, phys, false);
> > + break;
> > + }
> > +
> > + return NOTIFY_OK;
> > +}
>
> Wouldn't it make a lot of sense to integrate this into the cec framework?
>
> All you need is to pass an hdmi_notifier_dev as argument to cec_allocate_adapter()
> and you can integrate this.
>
> If you are OK with that, then I can make patches for that.
It's not just about CEC. It's also used for passing information between
the video and audio parts, so tying this into CEC is wrong.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH RFC 4/5] drm/bridge: add dw-hdmi cec driver using Hans Verkil's CEC code
2016-08-23 7:59 ` Russell King - ARM Linux
@ 2016-08-23 8:05 ` Hans Verkuil
2016-08-31 12:28 ` Russell King - ARM Linux
0 siblings, 1 reply; 21+ messages in thread
From: Hans Verkuil @ 2016-08-23 8:05 UTC (permalink / raw)
To: Russell King - ARM Linux, Hans Verkuil
Cc: linux-fbdev, David Airlie, dri-devel, Tomi Valkeinen,
Jean-Christophe Plagniol-Villard, linux-arm-kernel
On 08/23/16 09:59, Russell King - ARM Linux wrote:
> On Tue, Aug 23, 2016 at 09:21:17AM +0200, Hans Verkuil wrote:
>> Hi Russell,
>>
>> On 08/12/2016 04:15 PM, Russell King wrote:
>>> Add a CEC driver for the dw-hdmi hardware using Hans Verkil's CEC
>>> implementation.
>>>
>>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>>> ---
>>> drivers/gpu/drm/bridge/Kconfig | 7 +
>>> drivers/gpu/drm/bridge/Makefile | 1 +
>>> drivers/gpu/drm/bridge/dw-hdmi-cec.c | 344 ++++++++++++++++++++++++++++++
>>> drivers/gpu/drm/bridge/dw-hdmi.c | 64 +++++-
>>> include/linux/platform_data/dw_hdmi-cec.h | 16 ++
>>> 5 files changed, 421 insertions(+), 11 deletions(-)
>>> create mode 100644 drivers/gpu/drm/bridge/dw-hdmi-cec.c
>>> create mode 100644 include/linux/platform_data/dw_hdmi-cec.h
>>>
>>
>> <snip>
>>
>>> +static unsigned int parse_hdmi_addr(const struct edid *edid)
>>> +{
>>> + if (!edid || edid->extensions = 0)
>>> + return (u16)~0;
>>> +
>>> + return cec_get_edid_phys_addr((u8 *)edid,
>>> + EDID_LENGTH * (edid->extensions + 1), NULL);
>>> +}
>>> +
>>> +static int dw_hdmi_cec_notify(struct notifier_block *nb, unsigned long event,
>>> + void *data)
>>> +{
>>> + struct dw_hdmi_cec *cec = container_of(nb, struct dw_hdmi_cec, nb);
>>> + union hdmi_event *event_block = data;
>>> + unsigned int phys;
>>> +
>>> + dev_info(event_block->base.source, "event %lu\n", event);
>>> +
>>> + if (event_block->base.source != cec->adap->devnode.parent)
>>> + return NOTIFY_OK;
>>> +
>>> + switch (event) {
>>> + case HDMI_CONNECTED:
>>> + break;
>>> +
>>> + case HDMI_DISCONNECTED:
>>> + cec_s_phys_addr(cec->adap, CEC_PHYS_ADDR_INVALID, false);
>>> + break;
>>> +
>>> + case HDMI_NEW_EDID:
>>> + phys = parse_hdmi_addr(event_block->edid.edid);
>>> + cec_s_phys_addr(cec->adap, phys, false);
>>> + break;
>>> + }
>>> +
>>> + return NOTIFY_OK;
>>> +}
>>
>> Wouldn't it make a lot of sense to integrate this into the cec framework?
>>
>> All you need is to pass an hdmi_notifier_dev as argument to cec_allocate_adapter()
>> and you can integrate this.
>>
>> If you are OK with that, then I can make patches for that.
>
> It's not just about CEC. It's also used for passing information between
> the video and audio parts, so tying this into CEC is wrong.
I'm not saying that the hdmi notifier should be integrated, just that the
CEC core can register itself as a notifier and handle cec_s_phys_addr.
Regards,
Hans
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH RFC 4/5] drm/bridge: add dw-hdmi cec driver using Hans Verkil's CEC code
2016-08-23 8:05 ` Hans Verkuil
@ 2016-08-31 12:28 ` Russell King - ARM Linux
0 siblings, 0 replies; 21+ messages in thread
From: Russell King - ARM Linux @ 2016-08-31 12:28 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-fbdev, David Airlie, dri-devel, Hans Verkuil,
Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
linux-arm-kernel
On Tue, Aug 23, 2016 at 10:05:45AM +0200, Hans Verkuil wrote:
>
>
> On 08/23/16 09:59, Russell King - ARM Linux wrote:
> > On Tue, Aug 23, 2016 at 09:21:17AM +0200, Hans Verkuil wrote:
> >> Hi Russell,
> >>
> >> On 08/12/2016 04:15 PM, Russell King wrote:
> >>> Add a CEC driver for the dw-hdmi hardware using Hans Verkil's CEC
> >>> implementation.
> >>>
> >>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> >>> ---
> >>> drivers/gpu/drm/bridge/Kconfig | 7 +
> >>> drivers/gpu/drm/bridge/Makefile | 1 +
> >>> drivers/gpu/drm/bridge/dw-hdmi-cec.c | 344 ++++++++++++++++++++++++++++++
> >>> drivers/gpu/drm/bridge/dw-hdmi.c | 64 +++++-
> >>> include/linux/platform_data/dw_hdmi-cec.h | 16 ++
> >>> 5 files changed, 421 insertions(+), 11 deletions(-)
> >>> create mode 100644 drivers/gpu/drm/bridge/dw-hdmi-cec.c
> >>> create mode 100644 include/linux/platform_data/dw_hdmi-cec.h
> >>>
> >>
> >> <snip>
> >>
> >>> +static unsigned int parse_hdmi_addr(const struct edid *edid)
> >>> +{
> >>> + if (!edid || edid->extensions = 0)
> >>> + return (u16)~0;
> >>> +
> >>> + return cec_get_edid_phys_addr((u8 *)edid,
> >>> + EDID_LENGTH * (edid->extensions + 1), NULL);
> >>> +}
> >>> +
> >>> +static int dw_hdmi_cec_notify(struct notifier_block *nb, unsigned long event,
> >>> + void *data)
> >>> +{
> >>> + struct dw_hdmi_cec *cec = container_of(nb, struct dw_hdmi_cec, nb);
> >>> + union hdmi_event *event_block = data;
> >>> + unsigned int phys;
> >>> +
> >>> + dev_info(event_block->base.source, "event %lu\n", event);
> >>> +
> >>> + if (event_block->base.source != cec->adap->devnode.parent)
> >>> + return NOTIFY_OK;
> >>> +
> >>> + switch (event) {
> >>> + case HDMI_CONNECTED:
> >>> + break;
> >>> +
> >>> + case HDMI_DISCONNECTED:
> >>> + cec_s_phys_addr(cec->adap, CEC_PHYS_ADDR_INVALID, false);
> >>> + break;
> >>> +
> >>> + case HDMI_NEW_EDID:
> >>> + phys = parse_hdmi_addr(event_block->edid.edid);
> >>> + cec_s_phys_addr(cec->adap, phys, false);
> >>> + break;
> >>> + }
> >>> +
> >>> + return NOTIFY_OK;
> >>> +}
> >>
> >> Wouldn't it make a lot of sense to integrate this into the cec framework?
> >>
> >> All you need is to pass an hdmi_notifier_dev as argument to cec_allocate_adapter()
> >> and you can integrate this.
> >>
> >> If you are OK with that, then I can make patches for that.
> >
> > It's not just about CEC. It's also used for passing information between
> > the video and audio parts, so tying this into CEC is wrong.
>
> I'm not saying that the hdmi notifier should be integrated, just that the
> CEC core can register itself as a notifier and handle cec_s_phys_addr.
Yes, that would decrease the amount of code duplication.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RFC 4/5] drm/bridge: add dw-hdmi cec driver using Hans Verkil's CEC code
2016-08-12 14:15 ` [PATCH RFC 4/5] drm/bridge: add dw-hdmi cec driver using Hans Verkil's CEC code Russell King
2016-08-12 14:25 ` Hans Verkuil
2016-08-23 7:21 ` Hans Verkuil
@ 2016-08-23 8:03 ` Hans Verkuil
2016-08-31 12:28 ` Russell King - ARM Linux
2 siblings, 1 reply; 21+ messages in thread
From: Hans Verkuil @ 2016-08-23 8:03 UTC (permalink / raw)
To: Russell King, David Airlie, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Hans Verkuil
Cc: linux-fbdev, dri-devel, linux-arm-kernel
Hi Russell,
On 08/12/16 16:15, Russell King wrote:
> + ret = devm_request_threaded_irq(&pdev->dev, cec->irq,
> + dw_hdmi_cec_hardirq,
> + dw_hdmi_cec_thread, IRQF_SHARED,
> + DEV_NAME, cec->adap);
> + if (ret < 0)
> + return ret;
> +
> + ret = cec_register_adapter(cec->adap);
> + if (ret < 0)
> + return ret;
> +
> + /*
> + * CEC documentation says we must not call cec_delete_adapter
> + * after a successful call to cec_register_adapter().
> + */
> + devm_remove_action(&pdev->dev, dw_hdmi_cec_del, cec);
> +
> + hdmi_register_notifier(&cec->nb);
The notifier is registered here, but who provides CEC with the initial
physical address? As I understand it, it only tells you when things change,
not what the initial state is.
Regards,
Hans
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RFC 4/5] drm/bridge: add dw-hdmi cec driver using Hans Verkil's CEC code
2016-08-23 8:03 ` Hans Verkuil
@ 2016-08-31 12:28 ` Russell King - ARM Linux
0 siblings, 0 replies; 21+ messages in thread
From: Russell King - ARM Linux @ 2016-08-31 12:28 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-fbdev, David Airlie, dri-devel, Hans Verkuil,
Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
linux-arm-kernel
On Tue, Aug 23, 2016 at 10:03:03AM +0200, Hans Verkuil wrote:
> Hi Russell,
>
> On 08/12/16 16:15, Russell King wrote:
> > + ret = devm_request_threaded_irq(&pdev->dev, cec->irq,
> > + dw_hdmi_cec_hardirq,
> > + dw_hdmi_cec_thread, IRQF_SHARED,
> > + DEV_NAME, cec->adap);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = cec_register_adapter(cec->adap);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /*
> > + * CEC documentation says we must not call cec_delete_adapter
> > + * after a successful call to cec_register_adapter().
> > + */
> > + devm_remove_action(&pdev->dev, dw_hdmi_cec_del, cec);
> > +
> > + hdmi_register_notifier(&cec->nb);
>
> The notifier is registered here, but who provides CEC with the initial
> physical address? As I understand it, it only tells you when things change,
> not what the initial state is.
Correct, that's a short-coming of this. Solving that is not really
an easy problem - the hdmi notifiers don't track the state (they
aren't really aware of state themselves), nor do they track who the
originator of the messages is (again, because they don't track state.)
Fixing that requires a much more complex solution - I don't think I've
time to solve that (as illustrated by this reply taking over a week.)
So, we can either decide that we're not going to merge any CEC drivers
and other HDMI audio drivers until we solve this problem, denying people
the ability to use CEC, or we can merge what we have now and work on
solving the problem later.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH RFC 5/5] drm/i2c: add tda998x/tda9950 CEC driver
2016-08-12 14:14 [PATCH RFC 0/5] CEC drivers for iMX6 and TDA9950 Russell King - ARM Linux
` (3 preceding siblings ...)
2016-08-12 14:15 ` [PATCH RFC 4/5] drm/bridge: add dw-hdmi cec driver using Hans Verkil's CEC code Russell King
@ 2016-08-12 14:15 ` Russell King
2016-08-12 14:38 ` Hans Verkuil
4 siblings, 1 reply; 21+ messages in thread
From: Russell King @ 2016-08-12 14:15 UTC (permalink / raw)
To: David Airlie, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Hans Verkuil
Cc: linux-fbdev, linux-arm-kernel, dri-devel
Add a CEC driver for the TDA9950, which is a stand-alone I2C CEC device.
The TDA9950 contains a command processor which handles retransmissions
and the low level bus protocol. The driver just has to read and write
the messages, and handle error conditions.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/gpu/drm/i2c/Kconfig | 5 +
drivers/gpu/drm/i2c/Makefile | 1 +
drivers/gpu/drm/i2c/tda9950.c | 514 ++++++++++++++++++++++++++++++++++
include/linux/platform_data/tda9950.h | 15 +
4 files changed, 535 insertions(+)
create mode 100644 drivers/gpu/drm/i2c/tda9950.c
create mode 100644 include/linux/platform_data/tda9950.h
diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig
index 22c7ed63a001..f4af13203993 100644
--- a/drivers/gpu/drm/i2c/Kconfig
+++ b/drivers/gpu/drm/i2c/Kconfig
@@ -31,4 +31,9 @@ config DRM_I2C_NXP_TDA998X
help
Support for NXP Semiconductors TDA998X HDMI encoders.
+config DRM_I2C_NXP_TDA9950
+ tristate "NXP Semiconductors TDA9950/TDA998X HDMI CEC"
+ depends on MEDIA_CEC
+ select HDMI_NOTIFIERS
+
endmenu
diff --git a/drivers/gpu/drm/i2c/Makefile b/drivers/gpu/drm/i2c/Makefile
index 2c72eb584ab7..07bed58410e9 100644
--- a/drivers/gpu/drm/i2c/Makefile
+++ b/drivers/gpu/drm/i2c/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_DRM_I2C_SIL164) += sil164.o
tda998x-y := tda998x_drv.o
obj-$(CONFIG_DRM_I2C_NXP_TDA998X) += tda998x.o
+obj-$(CONFIG_DRM_I2C_NXP_TDA9950) += tda9950.o
diff --git a/drivers/gpu/drm/i2c/tda9950.c b/drivers/gpu/drm/i2c/tda9950.c
new file mode 100644
index 000000000000..2a792067f3b2
--- /dev/null
+++ b/drivers/gpu/drm/i2c/tda9950.c
@@ -0,0 +1,514 @@
+/*
+ * TDA9950 Consumer Electronics Control driver
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * The NXP TDA9950 implements the HDMI Consumer Electronics Control
+ * interface. The host interface is similar to a mailbox: the data
+ * registers starting at REG_CDR0 are written to send a command to the
+ * internal CPU, and replies are read from these registers.
+ *
+ * As the data registers represent a mailbox, they must be accessed
+ * as a single I2C transaction. See the TDA9950 data sheet for details.
+ */
+#include <linux/delay.h>
+#include <linux/hdmi-notifier.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/platform_data/tda9950.h>
+#include <linux/slab.h>
+#include <drm/drm_edid.h>
+#include <media/cec.h>
+
+enum {
+ REG_CSR = 0x00,
+ CSR_BUSY = BIT(7),
+ CSR_INT = BIT(6),
+ CSR_ERR = BIT(5),
+
+ REG_CER = 0x01,
+
+ REG_CVR = 0x02,
+
+ REG_CCR = 0x03,
+ CCR_RESET = BIT(7),
+ CCR_ON = BIT(6),
+
+ REG_ACKH = 0x04,
+ REG_ACKL = 0x05,
+
+ REG_CCONR = 0x06,
+ CCONR_ENABLE_ERROR = BIT(4),
+
+ REG_CDR0 = 0x07,
+
+ CDR1_REQ = 0x00,
+ CDR1_CNF = 0x01,
+ CDR1_IND = 0x81,
+ CDR1_ERR = 0x82,
+ CDR1_IER = 0x83,
+
+ CDR2_CNF_SUCCESS = 0x00,
+ CDR2_CNF_OFF_STATE = 0x80,
+ CDR2_CNF_BAD_REQ = 0x81,
+ CDR2_CNF_CEC_ACCESS = 0x82,
+ CDR2_CNF_ARB_ERROR = 0x83,
+ CDR2_CNF_BAD_TIMING = 0x84,
+ CDR2_CNF_NACK_ADDR = 0x85,
+ CDR2_CNF_NACK_DATA = 0x86,
+};
+
+struct tda9950_priv {
+ struct i2c_client *client;
+ struct device *hdmi;
+ struct cec_adapter *adap;
+ struct tda9950_glue *glue;
+ u16 addresses;
+ struct cec_msg rx_msg;
+ struct notifier_block nb;
+ bool open;
+};
+
+static int tda9950_write_range(struct i2c_client *client, u8 addr, u8 *p, int cnt)
+{
+ struct i2c_msg msg;
+ u8 buf[cnt + 1];
+ int ret;
+
+ buf[0] = addr;
+ memcpy(buf + 1, p, cnt);
+
+ msg.addr = client->addr;
+ msg.flags = 0;
+ msg.len = cnt + 1;
+ msg.buf = buf;
+
+ dev_dbg(&client->dev, "wr 0x%02x: %*ph\n", addr, cnt, p);
+
+ ret = i2c_transfer(client->adapter, &msg, 1);
+ if (ret < 0)
+ dev_err(&client->dev, "Error %d writing to cec:0x%x\n", ret, addr);
+ return ret < 0 ? ret : 0;
+}
+
+static void tda9950_write(struct i2c_client *client, u8 addr, u8 val)
+{
+ tda9950_write_range(client, addr, &val, 1);
+}
+
+static int tda9950_read_range(struct i2c_client *client, u8 addr, u8 *p, int cnt)
+{
+ struct i2c_msg msg[2];
+ int ret;
+
+ msg[0].addr = client->addr;
+ msg[0].flags = 0;
+ msg[0].len = 1;
+ msg[0].buf = &addr;
+ msg[1].addr = client->addr;
+ msg[1].flags = I2C_M_RD;
+ msg[1].len = cnt;
+ msg[1].buf = p;
+
+ ret = i2c_transfer(client->adapter, msg, 2);
+ if (ret < 0)
+ dev_err(&client->dev, "Error %d reading from cec:0x%x\n", ret, addr);
+
+ dev_dbg(&client->dev, "rd 0x%02x: %*ph\n", addr, cnt, p);
+
+ return ret;
+}
+
+static u8 tda9950_read(struct i2c_client *client, u8 addr)
+{
+ int ret;
+ u8 val;
+
+ ret = tda9950_read_range(client, addr, &val, 1);
+ if (ret < 0)
+ val = 0;
+
+ return val;
+}
+
+static irqreturn_t tda9950_irq(int irq, void *data)
+{
+ struct tda9950_priv *priv = data;
+ unsigned int tx_status;
+ u8 csr, buf[19];
+
+ if (!priv->open)
+ return IRQ_NONE;
+
+ csr = tda9950_read(priv->client, REG_CSR);
+ if (!(csr & CSR_INT))
+ return IRQ_NONE;
+
+ tda9950_read_range(priv->client, REG_CDR0, buf, sizeof(buf));
+
+ /*
+ * This should never happen: the data sheet says that there will
+ * always be a valid message if the interrupt line is asserted.
+ */
+ if (buf[0] = 0) {
+ dev_warn(&priv->client->dev, "interrupt pending, but no message?\n");
+ return IRQ_NONE;
+ }
+
+ switch (buf[1]) {
+ case CDR1_CNF: /* transmit result */
+ switch (buf[2]) {
+ case CDR2_CNF_SUCCESS:
+ tx_status = CEC_TX_STATUS_OK;
+ break;
+
+ case CDR2_CNF_NACK_ADDR:
+ tx_status = CEC_TX_STATUS_NACK;
+ break;
+
+ default: /* some other error, refer to TDA9950 docs */
+ dev_err(&priv->client->dev, "CNF reply error 0x%02x\n",
+ buf[2]);
+ tx_status = CEC_TX_STATUS_ERROR;
+ break;
+ }
+ cec_transmit_done(priv->adap, tx_status, 0, 0, 0, 0);
+ break;
+
+ case CDR1_IND:
+ priv->rx_msg.len = buf[0] - 2;
+ memcpy(priv->rx_msg.msg, buf + 2, priv->rx_msg.len);
+ cec_received_msg(priv->adap, &priv->rx_msg);
+ break;
+
+ default: /* unknown */
+ dev_err(&priv->client->dev, "unknown service id 0x%02x\n",
+ buf[1]);
+ break;
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int tda9950_cec_transmit(struct cec_adapter *adap, u8 attempts,
+ u32 signal_free_time, struct cec_msg *msg)
+{
+ struct tda9950_priv *priv = adap->priv;
+ u8 buf[16 + 2];
+
+ buf[0] = 2 + msg->len;
+ buf[1] = CDR1_REQ;
+ memcpy(buf + 2, msg->msg, msg->len);
+
+ return tda9950_write_range(priv->client, REG_CDR0, buf, 2 + msg->len);
+}
+
+static int tda9950_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
+{
+ struct tda9950_priv *priv = adap->priv;
+ u16 addresses;
+ u8 buf[2];
+
+ if (addr = CEC_LOG_ADDR_INVALID)
+ addresses = priv->addresses = BIT(15);
+ else
+ addresses = priv->addresses |= BIT(addr);
+
+ /* TDA9950 doesn't want address 15 set */
+ addr &= 0x7fff;
+ buf[0] = addresses >> 8;
+ buf[1] = addresses;
+
+ return tda9950_write_range(priv->client, REG_ACKH, buf, 2);
+}
+
+/*
+ * When operating as part of the TDA998x, we need additional handling
+ * to initialise and shut down the TDA9950 part of the device. These
+ * two hooks are provided to allow the TDA998x code to perform those
+ * activities.
+ */
+static int tda9950_glue_open(struct tda9950_priv *priv)
+{
+ int ret = 0;
+
+ if (priv->glue && priv->glue->open)
+ ret = priv->glue->open(priv->glue->data);
+
+ priv->open = true;
+
+ return ret;
+}
+
+static void tda9950_glue_release(struct tda9950_priv *priv)
+{
+ priv->open = false;
+
+ if (priv->glue && priv->glue->release)
+ priv->glue->release(priv->glue->data);
+}
+
+static int tda9950_open(struct tda9950_priv *priv)
+{
+ struct i2c_client *client = priv->client;
+ int ret;
+
+ ret = tda9950_glue_open(priv);
+ if (ret)
+ return ret;
+
+ /* Reset the TDA9950, and wait 250ms for it to recover */
+ tda9950_write(client, REG_CCR, CCR_RESET);
+ msleep(250);
+
+ /* Configure for the standard 5 retries */
+ tda9950_write(client, REG_CCONR, 5);
+ tda9950_cec_adap_log_addr(priv->adap, CEC_LOG_ADDR_INVALID);
+
+ /* Start the command processor */
+ tda9950_write(client, REG_CCR, CCR_ON);
+
+ return 0;
+}
+
+static void tda9950_release(struct tda9950_priv *priv)
+{
+ struct i2c_client *client = priv->client;
+ int timeout = 50;
+ u8 csr;
+
+ /* Stop the command processor */
+ tda9950_write(client, REG_CCR, 0);
+
+ /* Wait up to .5s for it to signal non-busy */
+ do {
+ csr = tda9950_read(client, REG_CSR);
+ if (!(csr & CSR_BUSY) || --timeout)
+ break;
+ msleep(10);
+ } while (1);
+
+ /* Warn the user that their IRQ may die if it's shared. */
+ if (csr & CSR_BUSY)
+ dev_warn(&client->dev, "command processor failed to stop, irq%d may die (csr=0x%02x)\n",
+ client->irq, csr);
+
+ tda9950_glue_release(priv);
+}
+
+static int tda9950_cec_adap_enable(struct cec_adapter *adap, bool enable)
+{
+ struct tda9950_priv *priv = adap->priv;
+
+ if (!enable) {
+ tda9950_release(priv);
+ return 0;
+ } else {
+ return tda9950_open(priv);
+ }
+}
+
+static const struct cec_adap_ops tda9950_cec_ops = {
+ .adap_enable = tda9950_cec_adap_enable,
+ .adap_log_addr = tda9950_cec_adap_log_addr,
+ .adap_transmit = tda9950_cec_transmit,
+};
+
+static unsigned int parse_hdmi_addr(const struct edid *edid)
+{
+ if (!edid || edid->extensions = 0)
+ return (u16)~0;
+
+ return cec_get_edid_phys_addr((u8 *)edid,
+ EDID_LENGTH * (edid->extensions + 1), NULL);
+}
+
+static int tda9950_cec_notify(struct notifier_block *nb, unsigned long event,
+ void *data)
+{
+ struct tda9950_priv *priv = container_of(nb, struct tda9950_priv, nb);
+ union hdmi_event *event_block = data;
+ unsigned int phys;
+
+ if (event_block->base.source != priv->hdmi)
+ return NOTIFY_OK;
+
+ switch (event) {
+ case HDMI_DISCONNECTED:
+ cec_s_phys_addr(priv->adap, CEC_PHYS_ADDR_INVALID, false);
+ break;
+
+ case HDMI_NEW_EDID:
+ phys = parse_hdmi_addr(event_block->edid.edid);
+ cec_s_phys_addr(priv->adap, phys, false);
+ break;
+ }
+
+ return NOTIFY_OK;
+}
+
+/*
+ * When operating as part of the TDA998x, we need to claim additional
+ * resources. These two hooks permit the management of those resources.
+ */
+static void tda9950_devm_glue_exit(void *data)
+{
+ struct tda9950_glue *glue = data;
+
+ if (glue && glue->exit)
+ glue->exit(glue->data);
+}
+
+static int tda9950_devm_glue_init(struct device *dev, struct tda9950_glue *glue)
+{
+ int ret;
+
+ if (glue && glue->init) {
+ ret = glue->init(glue->data);
+ if (ret)
+ return ret;
+ }
+
+ ret = devm_add_action(dev, tda9950_devm_glue_exit, glue);
+ if (ret)
+ tda9950_devm_glue_exit(glue);
+
+ return ret;
+}
+
+static void tda9950_cec_del(void *data)
+{
+ struct tda9950_priv *priv = data;
+
+ cec_delete_adapter(priv->adap);
+}
+
+static int tda9950_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct tda9950_glue *glue = client->dev.platform_data;
+ struct device *dev = &client->dev;
+ struct tda9950_priv *priv;
+ int ret;
+ u8 cvr;
+
+ /*
+ * We must have I2C functionality: our multi-byte accesses
+ * must be performed as a single contiguous transaction.
+ */
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+ dev_err(&client->dev,
+ "adapter does not support I2C functionality\n");
+ return -ENXIO;
+ }
+
+ /* We must have an interrupt to be functional. */
+ if (client->irq <= 0) {
+ dev_err(&client->dev, "driver requires an interrupt\n");
+ return -ENXIO;
+ }
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->client = client;
+ priv->glue = glue;
+ priv->nb.notifier_call = tda9950_cec_notify;
+
+ i2c_set_clientdata(client, priv);
+
+ /*
+ * If we're part of a TDA998x, we want the class devices to be
+ * associated with the HDMI Tx so we have a tight relationship
+ * between the HDMI interface and the CEC interface.
+ */
+ priv->hdmi = dev;
+ if (glue && glue->parent)
+ priv->hdmi = glue->parent;
+
+ priv->adap = cec_allocate_adapter(&tda9950_cec_ops, priv, "tda9950",
+ CEC_CAP_LOG_ADDRS |
+ CEC_CAP_TRANSMIT | CEC_CAP_RC,
+ CEC_MAX_LOG_ADDRS, priv->hdmi);
+ if (IS_ERR(priv->adap))
+ return PTR_ERR(priv->adap);
+
+ ret = devm_add_action(dev, tda9950_cec_del, priv);
+ if (ret) {
+ cec_delete_adapter(priv->adap);
+ return ret;
+ }
+
+ ret = tda9950_devm_glue_init(dev, glue);
+ if (ret)
+ return ret;
+
+ ret = tda9950_glue_open(priv);
+ if (ret)
+ return ret;
+
+ cvr = tda9950_read(client, REG_CVR);
+
+ dev_info(&client->dev,
+ "TDA9950 CEC interface, hardware version %u.%u\n",
+ cvr >> 4, cvr & 15);
+
+ tda9950_glue_release(priv);
+
+ ret = devm_request_threaded_irq(dev, client->irq, NULL, tda9950_irq,
+ IRQF_TRIGGER_FALLING | IRQF_SHARED |
+ IRQF_ONESHOT,
+ dev_name(&client->dev), priv);
+ if (ret < 0)
+ return ret;
+
+ ret = cec_register_adapter(priv->adap);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * CEC documentation says we must not call cec_delete_adapter
+ * after a successful call to cec_register_adapter().
+ */
+ devm_remove_action(dev, tda9950_cec_del, priv);
+
+ hdmi_register_notifier(&priv->nb);
+
+ return ret;
+}
+
+static int tda9950_remove(struct i2c_client *client)
+{
+ struct tda9950_priv *priv = i2c_get_clientdata(client);
+
+ hdmi_unregister_notifier(&priv->nb);
+ cec_unregister_adapter(priv->adap);
+
+ return 0;
+}
+
+static struct i2c_device_id tda9950_ids[] = {
+ { "tda9950", 0 },
+ { },
+};
+MODULE_DEVICE_TABLE(i2c, tda9950_ids);
+
+static struct i2c_driver tda9950_driver = {
+ .probe = tda9950_probe,
+ .remove = tda9950_remove,
+ .driver = {
+ .name = "tda9950",
+ },
+ .id_table = tda9950_ids,
+};
+
+module_i2c_driver(tda9950_driver);
+
+MODULE_AUTHOR("Russell King <rmk+kernel@armlinux.org.uk>");
+MODULE_DESCRIPTION("TDA9950/TDA998x Consumer Electronics Control Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/platform_data/tda9950.h b/include/linux/platform_data/tda9950.h
new file mode 100644
index 000000000000..08da9c989dad
--- /dev/null
+++ b/include/linux/platform_data/tda9950.h
@@ -0,0 +1,15 @@
+#ifndef LINUX_PLATFORM_DATA_TDA9950_H
+#define LINUX_PLATFORM_DATA_TDA9950_H
+
+struct device;
+
+struct tda9950_glue {
+ struct device *parent;
+ void *data;
+ int (*init)(void *);
+ void (*exit)(void *);
+ int (*open)(void *);
+ void (*release)(void *);
+};
+
+#endif
--
2.1.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH RFC 5/5] drm/i2c: add tda998x/tda9950 CEC driver
2016-08-12 14:15 ` [PATCH RFC 5/5] drm/i2c: add tda998x/tda9950 CEC driver Russell King
@ 2016-08-12 14:38 ` Hans Verkuil
2016-08-12 14:52 ` Russell King - ARM Linux
2016-08-12 15:16 ` Hans Verkuil
0 siblings, 2 replies; 21+ messages in thread
From: Hans Verkuil @ 2016-08-12 14:38 UTC (permalink / raw)
To: Russell King, David Airlie, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
Cc: linux-fbdev, linux-arm-kernel, dri-devel
On 08/12/2016 04:15 PM, Russell King wrote:
> Add a CEC driver for the TDA9950, which is a stand-alone I2C CEC device.
> The TDA9950 contains a command processor which handles retransmissions
> and the low level bus protocol. The driver just has to read and write
> the messages, and handle error conditions.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> drivers/gpu/drm/i2c/Kconfig | 5 +
> drivers/gpu/drm/i2c/Makefile | 1 +
> drivers/gpu/drm/i2c/tda9950.c | 514 ++++++++++++++++++++++++++++++++++
> include/linux/platform_data/tda9950.h | 15 +
> 4 files changed, 535 insertions(+)
> create mode 100644 drivers/gpu/drm/i2c/tda9950.c
> create mode 100644 include/linux/platform_data/tda9950.h
>
<snip>
> +static int tda9950_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
> +{
> + struct tda9950_priv *priv = adap->priv;
> + u16 addresses;
> + u8 buf[2];
> +
> + if (addr = CEC_LOG_ADDR_INVALID)
> + addresses = priv->addresses = BIT(15);
I saw this in patch 4/5 as well: why set bit 15? I would expect that this
is just set to 0. And priv->addresses doesn't seem to be used anywhere.
> + else
> + addresses = priv->addresses |= BIT(addr);
> +
> + /* TDA9950 doesn't want address 15 set */
> + addr &= 0x7fff;
> + buf[0] = addresses >> 8;
> + buf[1] = addresses;
> +
> + return tda9950_write_range(priv->client, REG_ACKH, buf, 2);
> +}
> +
<snip>
> +static int tda9950_cec_adap_enable(struct cec_adapter *adap, bool enable)
> +{
> + struct tda9950_priv *priv = adap->priv;
> +
> + if (!enable) {
> + tda9950_release(priv);
> + return 0;
> + } else {
Nitpick: no need for 'else' here since the 'if' always returns.
> + return tda9950_open(priv);
> + }
> +}
Regards,
Hans
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH RFC 5/5] drm/i2c: add tda998x/tda9950 CEC driver
2016-08-12 14:38 ` Hans Verkuil
@ 2016-08-12 14:52 ` Russell King - ARM Linux
2016-08-12 15:16 ` Hans Verkuil
1 sibling, 0 replies; 21+ messages in thread
From: Russell King - ARM Linux @ 2016-08-12 14:52 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-fbdev, David Airlie, dri-devel, Tomi Valkeinen,
Jean-Christophe Plagniol-Villard, linux-arm-kernel
On Fri, Aug 12, 2016 at 04:38:04PM +0200, Hans Verkuil wrote:
> On 08/12/2016 04:15 PM, Russell King wrote:
> >Add a CEC driver for the TDA9950, which is a stand-alone I2C CEC device.
> >The TDA9950 contains a command processor which handles retransmissions
> >and the low level bus protocol. The driver just has to read and write
> >the messages, and handle error conditions.
> >
> >Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> >---
> > drivers/gpu/drm/i2c/Kconfig | 5 +
> > drivers/gpu/drm/i2c/Makefile | 1 +
> > drivers/gpu/drm/i2c/tda9950.c | 514 ++++++++++++++++++++++++++++++++++
> > include/linux/platform_data/tda9950.h | 15 +
> > 4 files changed, 535 insertions(+)
> > create mode 100644 drivers/gpu/drm/i2c/tda9950.c
> > create mode 100644 include/linux/platform_data/tda9950.h
> >
>
> <snip>
>
> >+static int tda9950_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
> >+{
> >+ struct tda9950_priv *priv = adap->priv;
> >+ u16 addresses;
> >+ u8 buf[2];
> >+
> >+ if (addr = CEC_LOG_ADDR_INVALID)
> >+ addresses = priv->addresses = BIT(15);
>
> I saw this in patch 4/5 as well: why set bit 15? I would expect that this
> is just set to 0. And priv->addresses doesn't seem to be used anywhere.
It's needed to be set so that the hardware receives messages sent
to the broadcast (15) address.
> >+ else
> >+ addresses = priv->addresses |= BIT(addr);
For the second point, read this line, paying close attention to the |=.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH RFC 5/5] drm/i2c: add tda998x/tda9950 CEC driver
2016-08-12 14:38 ` Hans Verkuil
2016-08-12 14:52 ` Russell King - ARM Linux
@ 2016-08-12 15:16 ` Hans Verkuil
2016-08-12 15:29 ` Russell King - ARM Linux
1 sibling, 1 reply; 21+ messages in thread
From: Hans Verkuil @ 2016-08-12 15:16 UTC (permalink / raw)
To: Russell King, David Airlie, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
Cc: linux-fbdev, linux-arm-kernel, dri-devel
On 08/12/2016 04:38 PM, Hans Verkuil wrote:
> On 08/12/2016 04:15 PM, Russell King wrote:
>> Add a CEC driver for the TDA9950, which is a stand-alone I2C CEC device.
>> The TDA9950 contains a command processor which handles retransmissions
>> and the low level bus protocol. The driver just has to read and write
>> the messages, and handle error conditions.
>>
>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>> ---
>> drivers/gpu/drm/i2c/Kconfig | 5 +
>> drivers/gpu/drm/i2c/Makefile | 1 +
>> drivers/gpu/drm/i2c/tda9950.c | 514 ++++++++++++++++++++++++++++++++++
>> include/linux/platform_data/tda9950.h | 15 +
>> 4 files changed, 535 insertions(+)
>> create mode 100644 drivers/gpu/drm/i2c/tda9950.c
>> create mode 100644 include/linux/platform_data/tda9950.h
>>
>
> <snip>
>
>> +static int tda9950_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
>> +{
>> + struct tda9950_priv *priv = adap->priv;
>> + u16 addresses;
>> + u8 buf[2];
>> +
>> + if (addr = CEC_LOG_ADDR_INVALID)
>> + addresses = priv->addresses = BIT(15);
>
> I saw this in patch 4/5 as well: why set bit 15? I would expect that this
> is just set to 0. And priv->addresses doesn't seem to be used anywhere.
Yeah, you are right, priv->addresses is used. I've been reviewing too many
patches today.
The whole BIT(15) part remains weird. If log_addr is called with LOG_ADDR_INVALID
as argument, then the intention is that no more messages are to be received (unless
the hardware is in snooping mode). So there is no need to receive broadcast messages
either.
That said, I now realize that if userspace wants to configure the CEC device as
'Unregistered', then adap_log_addr is never called, which would be required if the
hardware has to enable support to receive broadcast messages.
In addition cec_received_msg should ignore received messages if log_addr_mask of
struct cec_log_addrs is 0. I think it will just pass on messages right now, and
that's not right.
I will look at this tomorrow when my brain isn't fried and do some more testing
with 'Unregistered' scenarios.
>
>> + else
>> + addresses = priv->addresses |= BIT(addr);
>> +
>> + /* TDA9950 doesn't want address 15 set */
>> + addr &= 0x7fff;
Shouldn't this be 'addresses' instead of 'addr'? 'addr' makes no sense here.
And if so, then I still don't understand setting BIT(15), since that bit is
removed by the &=.
>> + buf[0] = addresses >> 8;
>> + buf[1] = addresses;
>> +
>> + return tda9950_write_range(priv->client, REG_ACKH, buf, 2);
>> +}
>> +
Regards,
Hans
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH RFC 5/5] drm/i2c: add tda998x/tda9950 CEC driver
2016-08-12 15:16 ` Hans Verkuil
@ 2016-08-12 15:29 ` Russell King - ARM Linux
2016-08-12 15:53 ` Hans Verkuil
0 siblings, 1 reply; 21+ messages in thread
From: Russell King - ARM Linux @ 2016-08-12 15:29 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-fbdev, David Airlie, dri-devel, Tomi Valkeinen,
Jean-Christophe Plagniol-Villard, linux-arm-kernel
On Fri, Aug 12, 2016 at 05:16:41PM +0200, Hans Verkuil wrote:
> On 08/12/2016 04:38 PM, Hans Verkuil wrote:
> >On 08/12/2016 04:15 PM, Russell King wrote:
> >>Add a CEC driver for the TDA9950, which is a stand-alone I2C CEC device.
> >>The TDA9950 contains a command processor which handles retransmissions
> >>and the low level bus protocol. The driver just has to read and write
> >>the messages, and handle error conditions.
> >>
> >>Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> >>---
> >> drivers/gpu/drm/i2c/Kconfig | 5 +
> >> drivers/gpu/drm/i2c/Makefile | 1 +
> >> drivers/gpu/drm/i2c/tda9950.c | 514 ++++++++++++++++++++++++++++++++++
> >> include/linux/platform_data/tda9950.h | 15 +
> >> 4 files changed, 535 insertions(+)
> >> create mode 100644 drivers/gpu/drm/i2c/tda9950.c
> >> create mode 100644 include/linux/platform_data/tda9950.h
> >>
> >
> ><snip>
> >
> >>+static int tda9950_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
> >>+{
> >>+ struct tda9950_priv *priv = adap->priv;
> >>+ u16 addresses;
> >>+ u8 buf[2];
> >>+
> >>+ if (addr = CEC_LOG_ADDR_INVALID)
> >>+ addresses = priv->addresses = BIT(15);
> >
> >I saw this in patch 4/5 as well: why set bit 15? I would expect that this
> >is just set to 0. And priv->addresses doesn't seem to be used anywhere.
>
> Yeah, you are right, priv->addresses is used. I've been reviewing too many
> patches today.
>
> The whole BIT(15) part remains weird. If log_addr is called with
> LOG_ADDR_INVALID as argument, then the intention is that no more
> messages are to be received (unless the hardware is in snooping mode).
> So there is no need to receive broadcast messages either.
What about hardware where you can't stop it receiving broadcast
addresses? Should drivers manually check for this in their
interrupt handler?
> That said, I now realize that if userspace wants to configure the CEC
> device as 'Unregistered', then adap_log_addr is never called, which
> would be required if the hardware has to enable support to receive
> broadcast messages.
I don't see how that could possibly work. The CEC specification
requires that we receive broadcast addressed messages so that (eg)
the current source can be tracked. Being present on the bus, and
participating as an active source requires the reception of
broadcast messages so that you know when you stop being an active
source.
Nothing (afaics) enables broadcast address reception in the kernel
side, nor using cec-ctl in userspace.
> >>+ else
> >>+ addresses = priv->addresses |= BIT(addr);
> >>+
> >>+ /* TDA9950 doesn't want address 15 set */
> >>+ addr &= 0x7fff;
>
> Shouldn't this be 'addresses' instead of 'addr'? 'addr' makes no sense here.
>
> And if so, then I still don't understand setting BIT(15), since that bit is
> removed by the &=.
You're right in this case, but for dw-hdmi, bit 15 must be set for
broadcast messages to be received.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH RFC 5/5] drm/i2c: add tda998x/tda9950 CEC driver
2016-08-12 15:29 ` Russell King - ARM Linux
@ 2016-08-12 15:53 ` Hans Verkuil
2016-08-12 15:59 ` Russell King - ARM Linux
0 siblings, 1 reply; 21+ messages in thread
From: Hans Verkuil @ 2016-08-12 15:53 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: linux-fbdev, David Airlie, dri-devel, Tomi Valkeinen,
Jean-Christophe Plagniol-Villard, linux-arm-kernel
On 08/12/2016 05:29 PM, Russell King - ARM Linux wrote:
> On Fri, Aug 12, 2016 at 05:16:41PM +0200, Hans Verkuil wrote:
>> On 08/12/2016 04:38 PM, Hans Verkuil wrote:
>>> On 08/12/2016 04:15 PM, Russell King wrote:
>>>> Add a CEC driver for the TDA9950, which is a stand-alone I2C CEC device.
>>>> The TDA9950 contains a command processor which handles retransmissions
>>>> and the low level bus protocol. The driver just has to read and write
>>>> the messages, and handle error conditions.
>>>>
>>>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>>>> ---
>>>> drivers/gpu/drm/i2c/Kconfig | 5 +
>>>> drivers/gpu/drm/i2c/Makefile | 1 +
>>>> drivers/gpu/drm/i2c/tda9950.c | 514 ++++++++++++++++++++++++++++++++++
>>>> include/linux/platform_data/tda9950.h | 15 +
>>>> 4 files changed, 535 insertions(+)
>>>> create mode 100644 drivers/gpu/drm/i2c/tda9950.c
>>>> create mode 100644 include/linux/platform_data/tda9950.h
>>>>
>>>
>>> <snip>
>>>
>>>> +static int tda9950_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
>>>> +{
>>>> + struct tda9950_priv *priv = adap->priv;
>>>> + u16 addresses;
>>>> + u8 buf[2];
>>>> +
>>>> + if (addr = CEC_LOG_ADDR_INVALID)
>>>> + addresses = priv->addresses = BIT(15);
>>>
>>> I saw this in patch 4/5 as well: why set bit 15? I would expect that this
>>> is just set to 0. And priv->addresses doesn't seem to be used anywhere.
>>
>> Yeah, you are right, priv->addresses is used. I've been reviewing too many
>> patches today.
>>
>> The whole BIT(15) part remains weird. If log_addr is called with
>> LOG_ADDR_INVALID as argument, then the intention is that no more
>> messages are to be received (unless the hardware is in snooping mode).
>> So there is no need to receive broadcast messages either.
>
> What about hardware where you can't stop it receiving broadcast
> addresses? Should drivers manually check for this in their
> interrupt handler?
>
>> That said, I now realize that if userspace wants to configure the CEC
>> device as 'Unregistered', then adap_log_addr is never called, which
>> would be required if the hardware has to enable support to receive
>> broadcast messages.
>
> I don't see how that could possibly work. The CEC specification
> requires that we receive broadcast addressed messages so that (eg)
> the current source can be tracked. Being present on the bus, and
> participating as an active source requires the reception of
> broadcast messages so that you know when you stop being an active
> source.
>
> Nothing (afaics) enables broadcast address reception in the kernel
> side, nor using cec-ctl in userspace.
There are three possible 'states' of a CEC adapter w.r.t. logical addresses:
1) There is no physical address or no logical addresses have been set
by the application (CEC_ADAP_S_LOG_ADDRS): in that case the device
will not participate on the bus, it doesn't care about receiving messages
let alone replying to them. The only exception is if the device can
snoop messages, that is allowed even if CEC_ADAP_S_LOG_ADDRS is never
called. That is ideal to have a 'neutral' observer of the bus that just
listens but never participates. Not all hardware supports snooping, though.
2) CEC_ADAP_S_LOG_ADDRS is called and the device becomes an 'Unregistered'
device (i.e. it gets logical address 15). In that case it should receive
broadcast messages and be able to transmit messages.
3) CEC_ADAP_S_LOG_ADDRS is called and the device claims one or more logical
addresses in the range 0-14. Then it can receive broadcast and directed
messages, and of course transmit messages.
From the point of view of the hardware state 1 is selected if adap_log_addr
is called with LOG_ADDR_INVALID. Some hardware might still receive broadcast
messages (because they can't turn that off) and those should be filtered out
by the CEC framework. But I think that doesn't happen, which would be a bug.
I'll verify that tomorrow.
State 2 is a problem for hardware that has to enable support to receive
broadcast messages since adap_log_addr is never called with
LOG_ADDR_UNREGISTERED as argument. I missed that, since the hardware I tested
with always accepts broadcast messages. I'll look into this tomorrow as well.
It should be easy to fix.
State 3 works OK.
So for the dw-hdmi cec driver that means that for state 1 you set 'addresses'
to 0, for state 2 you set it to BIT(15) and for state 3 you always
OR with BIT(15). At least, as I understand your hardware.
BTW, if either the tda or dw-hdmi supports snooping mode, then I strongly
recommend supporting that. It's great for debugging.
Regards,
Hans
>
>>>> + else
>>>> + addresses = priv->addresses |= BIT(addr);
>>>> +
>>>> + /* TDA9950 doesn't want address 15 set */
>>>> + addr &= 0x7fff;
>>
>> Shouldn't this be 'addresses' instead of 'addr'? 'addr' makes no sense here.
>>
>> And if so, then I still don't understand setting BIT(15), since that bit is
>> removed by the &=.
>
> You're right in this case, but for dw-hdmi, bit 15 must be set for
> broadcast messages to be received.
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH RFC 5/5] drm/i2c: add tda998x/tda9950 CEC driver
2016-08-12 15:53 ` Hans Verkuil
@ 2016-08-12 15:59 ` Russell King - ARM Linux
0 siblings, 0 replies; 21+ messages in thread
From: Russell King - ARM Linux @ 2016-08-12 15:59 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-fbdev, David Airlie, dri-devel, Tomi Valkeinen,
Jean-Christophe Plagniol-Villard, linux-arm-kernel
On Fri, Aug 12, 2016 at 05:53:17PM +0200, Hans Verkuil wrote:
> There are three possible 'states' of a CEC adapter w.r.t. logical addresses:
>
> 1) There is no physical address or no logical addresses have been set
> by the application (CEC_ADAP_S_LOG_ADDRS): in that case the device
> will not participate on the bus, it doesn't care about receiving messages
> let alone replying to them. The only exception is if the device can
> snoop messages, that is allowed even if CEC_ADAP_S_LOG_ADDRS is never
> called. That is ideal to have a 'neutral' observer of the bus that just
> listens but never participates. Not all hardware supports snooping, though.
>
> 2) CEC_ADAP_S_LOG_ADDRS is called and the device becomes an 'Unregistered'
> device (i.e. it gets logical address 15). In that case it should receive
> broadcast messages and be able to transmit messages.
>
> 3) CEC_ADAP_S_LOG_ADDRS is called and the device claims one or more logical
> addresses in the range 0-14. Then it can receive broadcast and directed
> messages, and of course transmit messages.
>
> From the point of view of the hardware state 1 is selected if adap_log_addr
> is called with LOG_ADDR_INVALID. Some hardware might still receive broadcast
> messages (because they can't turn that off) and those should be filtered out
> by the CEC framework. But I think that doesn't happen, which would be a bug.
> I'll verify that tomorrow.
>
> State 2 is a problem for hardware that has to enable support to receive
> broadcast messages since adap_log_addr is never called with
> LOG_ADDR_UNREGISTERED as argument. I missed that, since the hardware I tested
> with always accepts broadcast messages. I'll look into this tomorrow as well.
> It should be easy to fix.
>
> State 3 works OK.
>
> So for the dw-hdmi cec driver that means that for state 1 you set 'addresses'
> to 0, for state 2 you set it to BIT(15) and for state 3 you always
> OR with BIT(15). At least, as I understand your hardware.
Thanks, I think that needs documenting in Documentation/cec.txt.
> BTW, if either the tda or dw-hdmi supports snooping mode, then I strongly
> recommend supporting that. It's great for debugging.
Neither do.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 21+ messages in thread