[PATCH v3 03/16] drm: sti: add HDMI driver
On Tue, May 20, 2014 at 03:56:13PM +0200, Benjamin Gaignard wrote:
[...]
> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
[...]
> +/* Reference to the hdmi device */
> +struct device *hdmi_dev;
No globals please.
> +/*
> + * Helper to write bit field
> + *
> + * @addr: register to update
> + * @val: value to write
> + * @mask: bit field mask to use
> + */
> +static inline void hdmi_reg_writemask(void __iomem *addr, u32 val, u32 mask)
Given the hdmi_ prefix I'd expect this to take a struct sti_hdmi * as
first argument. Maybe you can also leave out the _reg in there since it
should be pretty obvious what this writes.
> +/*
> + * HDMI interrupt handler
> + *
> + * @irq: irq number
> + * @arg: connector structure
> + */
> +static irqreturn_t hdmi_irq_thread(int irq, void *arg)
> +{
> + struct sti_hdmi *hdmi = arg;
> + u32 status;
> +
> + /* read interrupt status */
> + status = readl(hdmi->regs + HDMI_INT_STA);
> +
> + /* PLL lock interrupt */
> + if (status & HDMI_INT_DLL_LCK) {
> + hdmi->event_received = true;
> + wake_up_interruptible(>wait_event);
> + }
I think a more common way to do this is using a struct completion.
> + /* Hot plug detection */
> + if (status & HDMI_INT_HOT_PLUG) {
> + hdmi->hpd = gpio_get_value(hdmi->hpd_gpio);
Hmm... that's odd. You get an interrupt from the HDMI controller but
need to read a GPIO to find out what the status is?
> + if (hdmi->drm_dev)
> + drm_helper_hpd_irq_event(hdmi->drm_dev);
> + }
> +
> + /* Sw reset completed */
> + if (status & HDMI_INT_SW_RST) {
> + hdmi->event_received = true;
> + wake_up_interruptible(>wait_event);
> + }
There's no way to distinguish this from HDMI_INT_DLL_LCK.
> + /* clear interrupt status */
> + writel(status, hdmi->regs + HDMI_INT_CLR);
> +
> + /* TODO: check why this sync bus write solves the problem which
> + * is that without this line, the handler is sometimes called twice
> + */
> + /* sync bus write */
> + readl(hdmi->regs + HDMI_INT_STA);
This seems to be something that you're systematically doing wrong.
> +/*
> + * Start hdmi phy interface
> + *
> + * @hdmi: pointer on the hdmi internal structure
> + *
> + * Return -1 if error occurs
> + */
> +static int hdmi_phy_start(struct sti_hdmi *hdmi)
> +{
> + DRM_DEBUG_DRIVER("\n");
> +
> + if (hdmi->tx3g0c55phy)
> + return sti_hdmi_tx3g0c55phy_start(hdmi);
> +
> + return sti_hdmi_tx3g4c28phy_start(hdmi);
> +}
This looks like a prime candidate for the generic PHY framework. That
will allow you to simply store a struct phy * and use the generic API
rather than special case every possible type of PHY.
> +/*
> + * Stop hdmi phy interface
> + *
> + * @hdmi: pointer on the hdmi internal structure
> + */
> +static void hdmi_phy_stop(struct sti_hdmi *hdmi)
> +{
> + DRM_DEBUG_DRIVER("\n");
> +
> + if (hdmi->tx3g0c55phy)
> + sti_hdmi_tx3g0c55phy_stop(hdmi);
> + else
> + sti_hdmi_tx3g4c28phy_stop(hdmi);
> +}
Same here. The generic PHY equivalents for this would be phy_power_on()
and phy_power_off().
> +/*
> + * Set hdmi active area depending on the drm display mode selected
> + *
> + * @hdmi: pointer on the hdmi internal structure
> + */
> +static void hdmi_active_area(struct sti_hdmi *hdmi)
> +{
> + u32 xmin, xmax;
> + u32 ymin, ymax;
> +
> + DRM_DEBUG_DRIVER("\n");
> +
> + /*
> + * ActiveFrontSync Back Active
> + * RegionPorch Porch Region
> + * <---><>0<-><><->
> + *
> + * ///| | ///|
> + * /// | | /// |
> + * /// |...|/// |
> + *0___ x/ymin x/ymax
> + *
> + * <--[hv]display--> <--[hv]display-->
> + * <--[hv]sync_start-> <--[hv]sync_start-
> + * <--[hv]sync_end---> <--[hv]sync_end---
> + * <--[hv]total> <--[hv]total--
> + */
There's an instance of this picture somewhere else already, no need to
keep a copy in every subdriver. Also this is something that people are
supposed to know. Perhaps it should be moved to a central place for
reference?
> +
> + xmin = sti_vtg_get_pixel_number(hdmi->mode, 0);
> + xmax = sti_vtg_get_pixel_number(hdmi->mode, hdmi->mode.hdisplay - 1);
> + ymin = sti_vtg_get_line_number(hdmi->mode, 0);
> + ymax = sti_vtg_get_line_number(hdmi->mode, hdmi->mode.vdisplay - 1);
> +
> + writel(xmin, hdmi->regs + HDMI_ACTIVE_VID_XMIN);
[PATCH v3 03/16] drm: sti: add HDMI driver
Add driver for HDMI ouput
Signed-off-by: Benjamin Gaignard
---
drivers/gpu/drm/sti/Makefile | 5 +
drivers/gpu/drm/sti/sti_hdmi.c | 529 +
drivers/gpu/drm/sti/sti_hdmi.h | 195 +++
drivers/gpu/drm/sti/sti_hdmi_tx3g0c55phy.c | 398 ++
drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c | 224
5 files changed, 1351 insertions(+)
create mode 100644 drivers/gpu/drm/sti/sti_hdmi.c
create mode 100644 drivers/gpu/drm/sti/sti_hdmi.h
create mode 100644 drivers/gpu/drm/sti/sti_hdmi_tx3g0c55phy.c
create mode 100644 drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c
diff --git a/drivers/gpu/drm/sti/Makefile b/drivers/gpu/drm/sti/Makefile
index 79fdcb6..5295fc7 100644
--- a/drivers/gpu/drm/sti/Makefile
+++ b/drivers/gpu/drm/sti/Makefile
@@ -1,4 +1,9 @@
ccflags-y := -Iinclude/drm
+stidrm-y := sti_hdmi.o \
+ sti_hdmi_tx3g0c55phy.o \
+ sti_hdmi_tx3g4c28phy.o
+
obj-$(CONFIG_VTAC_STI) += sti_vtac_tx.o sti_vtac_rx.o
obj-$(CONFIG_VTG_STI) += sti_vtg.o sti_vtg_utils.o
+obj-$(CONFIG_DRM_STI) += stidrm.o
\ No newline at end of file
diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
new file mode 100644
index 000..02b0524
--- /dev/null
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -0,0 +1,529 @@
+/*
+ * Copyright (C) STMicroelectronics SA 2013
+ * Author: Vincent Abriou for STMicroelectronics.
+ * License terms: GNU General Public License (GPL), version 2
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+#include "sti_hdmi.h"
+#include "sti_vtg_utils.h"
+
+/* Reference to the hdmi device */
+struct device *hdmi_dev;
+
+/*
+ * Helper to write bit field
+ *
+ * @addr: register to update
+ * @val: value to write
+ * @mask: bit field mask to use
+ */
+static inline void hdmi_reg_writemask(void __iomem *addr, u32 val, u32 mask)
+{
+ u32 old = readl(addr);
+
+ val = (val & mask) | (old & ~mask);
+ writel(val, addr);
+}
+
+/*
+ * HDMI interrupt handler
+ *
+ * @irq: irq number
+ * @arg: connector structure
+ */
+static irqreturn_t hdmi_irq_thread(int irq, void *arg)
+{
+ struct sti_hdmi *hdmi = arg;
+ u32 status;
+
+ /* read interrupt status */
+ status = readl(hdmi->regs + HDMI_INT_STA);
+
+ /* PLL lock interrupt */
+ if (status & HDMI_INT_DLL_LCK) {
+ hdmi->event_received = true;
+ wake_up_interruptible(>wait_event);
+ }
+
+ /* Hot plug detection */
+ if (status & HDMI_INT_HOT_PLUG) {
+ hdmi->hpd = gpio_get_value(hdmi->hpd_gpio);
+ if (hdmi->drm_dev)
+ drm_helper_hpd_irq_event(hdmi->drm_dev);
+ }
+
+ /* Sw reset completed */
+ if (status & HDMI_INT_SW_RST) {
+ hdmi->event_received = true;
+ wake_up_interruptible(>wait_event);
+ }
+
+ /* clear interrupt status */
+ writel(status, hdmi->regs + HDMI_INT_CLR);
+
+ /* TODO: check why this sync bus write solves the problem which
+* is that without this line, the handler is sometimes called twice
+*/
+ /* sync bus write */
+ readl(hdmi->regs + HDMI_INT_STA);
+
+ return IRQ_HANDLED;
+}
+
+/*
+ * Start hdmi phy interface
+ *
+ * @hdmi: pointer on the hdmi internal structure
+ *
+ * Return -1 if error occurs
+ */
+static int hdmi_phy_start(struct sti_hdmi *hdmi)
+{
+ DRM_DEBUG_DRIVER("\n");
+
+ if (hdmi->tx3g0c55phy)
+ return sti_hdmi_tx3g0c55phy_start(hdmi);
+
+ return sti_hdmi_tx3g4c28phy_start(hdmi);
+}
+
+/*
+ * Stop hdmi phy interface
+ *
+ * @hdmi: pointer on the hdmi internal structure
+ */
+static void hdmi_phy_stop(struct sti_hdmi *hdmi)
+{
+ DRM_DEBUG_DRIVER("\n");
+
+ if (hdmi->tx3g0c55phy)
+ sti_hdmi_tx3g0c55phy_stop(hdmi);
+ else
+ sti_hdmi_tx3g4c28phy_stop(hdmi);
+}
+
+/*
+ * Set hdmi active area depending on the drm display mode selected
+ *
+ * @hdmi: pointer on the hdmi internal structure
+ */
+static void hdmi_active_area(struct sti_hdmi *hdmi)
+{
+ u32 xmin, xmax;
+ u32 ymin, ymax;
+
+ DRM_DEBUG_DRIVER("\n");
+
+ /*
+* ActiveFrontSync Back Active
+* RegionPorch Porch Region
+* <---><>0<-><><->
+*
+* ///| | ///|
+* /// | | /// |
+* /// |...|/// |
+*0___ x/ymin x/ymax
+*
+* <--[hv]display--> <--[hv]display-->
+*
