Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1667888 > unrolled thread

[PATCH v3 0/4] Synopsys Designware HDMI Video Capture Controller + PHY

Started byJose Abreu <Jose.Abreu@synopsys.com>
First post2017-06-16 18:40 +0200
Last post2017-06-20 12:20 +0200
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/4] Synopsys Designware HDMI Video Capture Controller + PHY Jose Abreu <Jose.Abreu@synopsys.com> - 2017-06-16 18:40 +0200
    [PATCH v3 1/4] [media] platform: Add Synopsys Designware HDMI RX PHY e405 Driver Jose Abreu <Jose.Abreu@synopsys.com> - 2017-06-16 18:40 +0200
    Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX  Controller Driver Sylwester Nawrocki <snawrocki@kernel.org> - 2017-06-18 20:10 +0200
      Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX  Controller Driver Jose Abreu <Jose.Abreu@synopsys.com> - 2017-06-19 11:40 +0200
        Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX  Controller Driver Sylwester Nawrocki <snawrocki@kernel.org> - 2017-06-20 00:20 +0200
          Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX  Controller Driver Jose Abreu <Jose.Abreu@synopsys.com> - 2017-06-20 12:20 +0200

#1667888 — [PATCH v3 0/4] Synopsys Designware HDMI Video Capture Controller + PHY

FromJose Abreu <Jose.Abreu@synopsys.com>
Date2017-06-16 18:40 +0200
Subject[PATCH v3 0/4] Synopsys Designware HDMI Video Capture Controller + PHY
Message-ID<tT2rU-2yi-7@gated-at.bofh.it>
The Synopsys Designware HDMI RX controller is an HDMI receiver controller that
is responsible to process digital data that comes from a phy. The final result
is a stream of raw video data that can then be connected to a video DMA, for
example, and transfered into RAM so that it can be displayed.

The controller + phy available in this series natively support all HDMI 1.4 and
HDMI 2.0 modes, including deep color. Although, the driver is quite in its
initial stage and unfortunatelly only non deep color modes are supported. Also,
audio is not yet supported in the driver (the controller has several audio
output interfaces).

Version 3 addresses review comments from Hans Verkuil regarding CEC and also
uses a CEC notifier so that userspace does not have to manually set the CEC
physical address. SCDC support was also implemented.

This series was tested in a FPGA platform.

Jose Abreu (4):
  [media] platform: Add Synopsys Designware HDMI RX PHY e405 Driver
  [media] platform: Add Synopsys Designware HDMI RX Controller Driver
  MAINTAINERS: Add entry for Synopsys Designware HDMI drivers
  dt-bindings: media: Document Synopsys Designware HDMI RX

Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>

 .../devicetree/bindings/media/snps,dw-hdmi-rx.txt  |   45 +
 MAINTAINERS                                        |    7 +
 drivers/media/platform/Kconfig                     |    2 +
 drivers/media/platform/Makefile                    |    2 +
 drivers/media/platform/dwc/Kconfig                 |   23 +
 drivers/media/platform/dwc/Makefile                |    2 +
 drivers/media/platform/dwc/dw-hdmi-phy-e405.c      |  783 +++++++++
 drivers/media/platform/dwc/dw-hdmi-phy-e405.h      |   63 +
 drivers/media/platform/dwc/dw-hdmi-rx.c            | 1779 ++++++++++++++++++++
 drivers/media/platform/dwc/dw-hdmi-rx.h            |  441 +++++
 include/media/dwc/dw-hdmi-phy-pdata.h              |  131 ++
 include/media/dwc/dw-hdmi-rx-pdata.h               |   63 +
 12 files changed, 3341 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt
 create mode 100644 drivers/media/platform/dwc/Kconfig
 create mode 100644 drivers/media/platform/dwc/Makefile
 create mode 100644 drivers/media/platform/dwc/dw-hdmi-phy-e405.c
 create mode 100644 drivers/media/platform/dwc/dw-hdmi-phy-e405.h
 create mode 100644 drivers/media/platform/dwc/dw-hdmi-rx.c
 create mode 100644 drivers/media/platform/dwc/dw-hdmi-rx.h
 create mode 100644 include/media/dwc/dw-hdmi-phy-pdata.h
 create mode 100644 include/media/dwc/dw-hdmi-rx-pdata.h

-- 
1.9.1

[toc] | [next] | [standalone]

#1667890 — [PATCH v3 1/4] [media] platform: Add Synopsys Designware HDMI RX PHY e405 Driver

FromJose Abreu <Jose.Abreu@synopsys.com>
Date2017-06-16 18:40 +0200
Subject[PATCH v3 1/4] [media] platform: Add Synopsys Designware HDMI RX PHY e405 Driver
Message-ID<tT2rV-2yi-27@gated-at.bofh.it>
In reply to#1667888
This adds support for the Synopsys Designware HDMI RX PHY e405. This
phy receives and decodes HDMI video that is delivered to a controller.

Main features included in this driver are:
	- Equalizer algorithm that chooses the phy best settings
	according to the detected HDMI cable characteristics.
	- Support for scrambling
	- Support for HDMI 2.0 modes up to 6G (HDMI 4k@60Hz).

The driver was implemented as a standalone V4L2 subdevice and the
phy interface with the controller was implemented using V4L2 ioctls. I
do not know if this is the best option but it is not possible to use the
existing API functions directly as we need specific functions that will
be called by the controller at specific configuration stages.

There is also a bidirectional communication between controller and phy:
The phy must provide functions that the controller will call (i.e.
configuration functions) and the controller must provide read/write
callbacks, as well as other specific functions.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Hans Verkuil <hans.verkuil@cisco.com>

Changes from RFC:
	- Remove a bunch of functions that can be collapsed into
	a single config() function
	- Add comments for the callbacks and structures (Hans)
---
 drivers/media/platform/Kconfig                |   2 +
 drivers/media/platform/Makefile               |   2 +
 drivers/media/platform/dwc/Kconfig            |   8 +
 drivers/media/platform/dwc/Makefile           |   1 +
 drivers/media/platform/dwc/dw-hdmi-phy-e405.c | 783 ++++++++++++++++++++++++++
 drivers/media/platform/dwc/dw-hdmi-phy-e405.h |  63 +++
 include/media/dwc/dw-hdmi-phy-pdata.h         | 131 +++++
 7 files changed, 990 insertions(+)
 create mode 100644 drivers/media/platform/dwc/Kconfig
 create mode 100644 drivers/media/platform/dwc/Makefile
 create mode 100644 drivers/media/platform/dwc/dw-hdmi-phy-e405.c
 create mode 100644 drivers/media/platform/dwc/dw-hdmi-phy-e405.h
 create mode 100644 include/media/dwc/dw-hdmi-phy-pdata.h

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 8da521a..786e4a2 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -33,6 +33,8 @@ source "drivers/media/platform/omap/Kconfig"
 
 source "drivers/media/platform/blackfin/Kconfig"
 
+source "drivers/media/platform/dwc/Kconfig"
+
 config VIDEO_SH_VOU
 	tristate "SuperH VOU video output driver"
 	depends on MEDIA_CAMERA_SUPPORT
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 6bbdf94..43ec040b 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -81,3 +81,5 @@ obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC)	+= mtk-vcodec/
 obj-$(CONFIG_VIDEO_MEDIATEK_MDP)	+= mtk-mdp/
 
 obj-$(CONFIG_VIDEO_MEDIATEK_JPEG)	+= mtk-jpeg/
+
+obj-y	+= dwc/
diff --git a/drivers/media/platform/dwc/Kconfig b/drivers/media/platform/dwc/Kconfig
new file mode 100644
index 0000000..361d38d
--- /dev/null
+++ b/drivers/media/platform/dwc/Kconfig
@@ -0,0 +1,8 @@
+config VIDEO_DWC_HDMI_PHY_E405
+	tristate "Synopsys Designware HDMI RX PHY e405 driver"
+	depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+	help
+	  Support for Synopsys Designware HDMI RX PHY. Version is e405.
+
+	  To compile this driver as a module, choose M here. The module
+	  will be called dw-hdmi-phy-e405.
diff --git a/drivers/media/platform/dwc/Makefile b/drivers/media/platform/dwc/Makefile
new file mode 100644
index 0000000..fc3b62c
--- /dev/null
+++ b/drivers/media/platform/dwc/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_VIDEO_DWC_HDMI_PHY_E405) += dw-hdmi-phy-e405.o
diff --git a/drivers/media/platform/dwc/dw-hdmi-phy-e405.c b/drivers/media/platform/dwc/dw-hdmi-phy-e405.c
new file mode 100644
index 0000000..b6a5b4b
--- /dev/null
+++ b/drivers/media/platform/dwc/dw-hdmi-phy-e405.c
@@ -0,0 +1,783 @@
+/*
+ * Synopsys Designware HDMI PHY E405 driver
+ *
+ * This Synopsys dw-phy-e405 software and associated documentation
+ * (hereinafter the "Software") is an unsupported proprietary work of
+ * Synopsys, Inc. unless otherwise expressly agreed to in writing between
+ * Synopsys and you. The Software IS NOT an item of Licensed Software or a
+ * Licensed Product under any End User Software License Agreement or
+ * Agreement for Licensed Products with Synopsys or any supplement thereto.
+ * Synopsys is a registered trademark of Synopsys, Inc. Other names included
+ * in the SOFTWARE may be the trademarks of their respective owners.
+ *
+ * The contents of this file are dual-licensed; you may select either version 2
+ * of the GNU General Public License (“GPL”) or the MIT license (“MIT”).
+ *
+ * Copyright (c) 2017 Synopsys, Inc. and/or its affiliates.
+ *
+ * THIS 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 THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include <media/v4l2-subdev.h>
+#include <media/dwc/dw-hdmi-phy-pdata.h>
+#include "dw-hdmi-phy-e405.h"
+
+MODULE_AUTHOR("Jose Abreu <joabreu@synopsys.com>");
+MODULE_DESCRIPTION("Designware HDMI PHY e405 driver");
+MODULE_LICENSE("Dual MIT/GPL");
+MODULE_ALIAS("platform:" DW_PHY_E405_DRVNAME);
+
+#define PHY_EQ_WAIT_TIME_START			3
+#define PHY_EQ_SLEEP_TIME_CDR			30
+#define PHY_EQ_SLEEP_TIME_ACQ			1
+#define PHY_EQ_BOUNDSPREAD			20
+#define PHY_EQ_MIN_ACQ_STABLE			3
+#define PHY_EQ_ACC_LIMIT			360
+#define PHY_EQ_ACC_MIN_LIMIT			0
+#define PHY_EQ_MAX_SETTING			13
+#define PHY_EQ_SHORT_CABLE_SETTING		4
+#define PHY_EQ_ERROR_CABLE_SETTING		4
+#define PHY_EQ_MIN_SLOPE			50
+#define PHY_EQ_AVG_ACQ				5
+#define PHY_EQ_MINMAX_NTRIES			3
+#define PHY_EQ_EQUALIZED_COUNTER_VAL		512
+#define PHY_EQ_EQUALIZED_COUNTER_VAL_HDMI20	512
+#define PHY_EQ_MINMAX_MAXDIFF			4
+#define PHY_EQ_MINMAX_MAXDIFF_HDMI20		2
+#define PHY_EQ_FATBIT_MASK			0x0000
+#define PHY_EQ_FATBIT_MASK_4K			0x0c03
+#define PHY_EQ_FATBIT_MASK_HDMI20		0x0e03
+
+struct dw_phy_eq_ch {
+	u16 best_long_setting;
+	u8 valid_long_setting;
+	u16 best_short_setting;
+	u8 valid_short_setting;
+	u16 best_setting;
+	u16 acc;
+	u16 acq;
+	u16 last_acq;
+	u16 upper_bound_acq;
+	u16 lower_bound_acq;
+	u16 out_bound_acq;
+	u16 read_acq;
+};
+
+static const struct dw_phy_mpll_config {
+	u16 addr;
+	u16 val;
+} dw_phy_e405_mpll_cfg[] = {
+	{ 0x27, 0x1B94 },
+	{ 0x28, 0x16D2 },
+	{ 0x29, 0x12D9 },
+	{ 0x2A, 0x3249 },
+	{ 0x2B, 0x3653 },
+	{ 0x2C, 0x3436 },
+	{ 0x2D, 0x124D },
+	{ 0x2E, 0x0001 },
+	{ 0xCE, 0x0505 },
+	{ 0xCF, 0x0505 },
+	{ 0xD0, 0x0000 },
+	{ 0x00, 0x0000 },
+};
+
+struct dw_phy_dev {
+	struct device *dev;
+	struct dw_phy_pdata *config;
+	bool phy_enabled;
+	struct v4l2_subdev sd;
+	u16 mpll_status;
+	unsigned char color_depth;
+	bool hdmi2;
+	bool scrambling;
+	struct mutex lock;
+};
+
+static inline struct dw_phy_dev *to_dw_dev(struct v4l2_subdev *sd)
+{
+	return container_of(sd, struct dw_phy_dev, sd);
+}
+
+static void phy_write(struct dw_phy_dev *dw_dev, u16 val, u16 addr)
+{
+	void *arg = dw_dev->config->funcs_arg;
+
+	dw_dev->config->funcs->write(arg, val, addr);
+}
+
+static u16 phy_read(struct dw_phy_dev *dw_dev, u16 addr)
+{
+	void *arg = dw_dev->config->funcs_arg;
+
+	return dw_dev->config->funcs->read(arg, addr);
+}
+
+static void phy_reset(struct dw_phy_dev *dw_dev, int enable)
+{
+	void *arg = dw_dev->config->funcs_arg;
+
+	dw_dev->config->funcs->reset(arg, enable);
+}
+
+static void phy_pddq(struct dw_phy_dev *dw_dev, int enable)
+{
+	void *arg = dw_dev->config->funcs_arg;
+
+	dw_dev->config->funcs->pddq(arg, enable);
+}
+
+static void phy_svsmode(struct dw_phy_dev *dw_dev, int enable)
+{
+	void *arg = dw_dev->config->funcs_arg;
+
+	dw_dev->config->funcs->svsmode(arg, enable);
+}
+
+static void phy_zcal_reset(struct dw_phy_dev *dw_dev)
+{
+	void *arg = dw_dev->config->funcs_arg;
+
+	dw_dev->config->funcs->zcal_reset(arg);
+}
+
+static bool phy_zcal_done(struct dw_phy_dev *dw_dev)
+{
+	void *arg = dw_dev->config->funcs_arg;
+
+	return dw_dev->config->funcs->zcal_done(arg);
+}
+
+static bool phy_tmds_valid(struct dw_phy_dev *dw_dev)
+{
+	void *arg = dw_dev->config->funcs_arg;
+
+	return dw_dev->config->funcs->tmds_valid(arg);
+}
+
+static int dw_phy_eq_test(struct dw_phy_dev *dw_dev,
+		u16 *fat_bit_mask, int *min_max_length)
+{
+	u16 main_fsm_status, val;
+	int i;
+
+	for (i = 0; i < PHY_EQ_WAIT_TIME_START; i++) {
+		main_fsm_status = phy_read(dw_dev, PHY_MAINFSM_STATUS1);
+		if (main_fsm_status & 0x100)
+			break;
+		mdelay(PHY_EQ_SLEEP_TIME_CDR);
+	}
+
+	if (i == PHY_EQ_WAIT_TIME_START) {
+		dev_err(dw_dev->dev, "phy start conditions not achieved\n");
+		return -ETIMEDOUT;
+	}
+
+	if (main_fsm_status & 0x400) {
+		dev_err(dw_dev->dev, "invalid pll rate\n");
+		return -EINVAL;
+	}
+
+	val = (phy_read(dw_dev, PHY_CDR_CTRL_CNT) & 0x300) >> 8;
+	if (val == 0x1) {
+		/* HDMI 2.0 */
+		*fat_bit_mask = PHY_EQ_FATBIT_MASK_HDMI20;
+		*min_max_length = PHY_EQ_MINMAX_MAXDIFF_HDMI20;
+		dev_dbg(dw_dev->dev, "[EQUALIZER] using HDMI 2.0 values\n");
+	} else if (!(main_fsm_status & 0x600)) {
+		/* HDMI 1.4 (pll rate = 0) */
+		*fat_bit_mask = PHY_EQ_FATBIT_MASK_4K;
+		*min_max_length = PHY_EQ_MINMAX_MAXDIFF;
+		dev_dbg(dw_dev->dev, "[EQUALIZER] using HDMI 1.4@4k values\n");
+	} else {
+		/* HDMI 1.4 */
+		*fat_bit_mask = PHY_EQ_FATBIT_MASK;
+		*min_max_length = PHY_EQ_MINMAX_MAXDIFF;
+		dev_dbg(dw_dev->dev, "[EQUALIZER] using HDMI 1.4 values\n");
+	}
+
+	return 0;
+}
+
+static void dw_phy_eq_default(struct dw_phy_dev *dw_dev)
+{
+	phy_write(dw_dev, 0x08A8, PHY_CH0_EQ_CTRL1);
+	phy_write(dw_dev, 0x0020, PHY_CH0_EQ_CTRL2);
+	phy_write(dw_dev, 0x08A8, PHY_CH1_EQ_CTRL1);
+	phy_write(dw_dev, 0x0020, PHY_CH1_EQ_CTRL2);
+	phy_write(dw_dev, 0x08A8, PHY_CH2_EQ_CTRL1);
+	phy_write(dw_dev, 0x0020, PHY_CH2_EQ_CTRL2);
+}
+
+static void dw_phy_eq_single(struct dw_phy_dev *dw_dev)
+{
+	phy_write(dw_dev, 0x0211, PHY_CH0_EQ_CTRL1);
+	phy_write(dw_dev, 0x0211, PHY_CH1_EQ_CTRL1);
+	phy_write(dw_dev, 0x0211, PHY_CH2_EQ_CTRL1);
+}
+
+static void dw_phy_eq_equal_setting(struct dw_phy_dev *dw_dev,
+		u16 lock_vector)
+{
+	phy_write(dw_dev, lock_vector, PHY_CH0_EQ_CTRL4);
+	phy_write(dw_dev, 0x0024, PHY_CH0_EQ_CTRL2);
+	phy_write(dw_dev, 0x0026, PHY_CH0_EQ_CTRL2);
+	phy_read(dw_dev, PHY_CH0_EQ_STATUS2);
+	phy_write(dw_dev, lock_vector, PHY_CH1_EQ_CTRL4);
+	phy_write(dw_dev, 0x0024, PHY_CH1_EQ_CTRL2);
+	phy_write(dw_dev, 0x0026, PHY_CH1_EQ_CTRL2);
+	phy_read(dw_dev, PHY_CH1_EQ_STATUS2);
+	phy_write(dw_dev, lock_vector, PHY_CH2_EQ_CTRL4);
+	phy_write(dw_dev, 0x0024, PHY_CH2_EQ_CTRL2);
+	phy_write(dw_dev, 0x0026, PHY_CH2_EQ_CTRL2);
+	phy_read(dw_dev, PHY_CH2_EQ_STATUS2);
+}
+
+static void dw_phy_eq_equal_setting_ch0(struct dw_phy_dev *dw_dev,
+		u16 lock_vector)
+{
+	phy_write(dw_dev, lock_vector, PHY_CH0_EQ_CTRL4);
+	phy_write(dw_dev, 0x0024, PHY_CH0_EQ_CTRL2);
+	phy_write(dw_dev, 0x0026, PHY_CH0_EQ_CTRL2);
+	phy_read(dw_dev, PHY_CH0_EQ_STATUS2);
+}
+
+static void dw_phy_eq_equal_setting_ch1(struct dw_phy_dev *dw_dev,
+		u16 lock_vector)
+{
+	phy_write(dw_dev, lock_vector, PHY_CH1_EQ_CTRL4);
+	phy_write(dw_dev, 0x0024, PHY_CH1_EQ_CTRL2);
+	phy_write(dw_dev, 0x0026, PHY_CH1_EQ_CTRL2);
+	phy_read(dw_dev, PHY_CH1_EQ_STATUS2);
+}
+
+static void dw_phy_eq_equal_setting_ch2(struct dw_phy_dev *dw_dev,
+		u16 lock_vector)
+{
+	phy_write(dw_dev, lock_vector, PHY_CH2_EQ_CTRL4);
+	phy_write(dw_dev, 0x0024, PHY_CH2_EQ_CTRL2);
+	phy_write(dw_dev, 0x0026, PHY_CH2_EQ_CTRL2);
+	phy_read(dw_dev, PHY_CH2_EQ_STATUS2);
+}
+
+static void dw_phy_eq_auto_calib(struct dw_phy_dev *dw_dev)
+{
+	phy_write(dw_dev, 0x1809, PHY_MAINFSM_CTRL);
+	phy_write(dw_dev, 0x1819, PHY_MAINFSM_CTRL);
+	phy_write(dw_dev, 0x1809, PHY_MAINFSM_CTRL);
+}
+
+static void dw_phy_eq_init_vars(struct dw_phy_eq_ch *ch)
+{
+	ch->acc = 0;
+	ch->acq = 0;
+	ch->last_acq = 0;
+	ch->valid_long_setting = 0;
+	ch->valid_short_setting = 0;
+	ch->best_setting = PHY_EQ_SHORT_CABLE_SETTING;
+}
+
+static bool dw_phy_eq_acquire_early_cnt(struct dw_phy_dev *dw_dev,
+		u16 setting, u16 acq, struct dw_phy_eq_ch *ch0,
+		struct dw_phy_eq_ch *ch1, struct dw_phy_eq_ch *ch2)
+{
+	u16 lock_vector = 0x1;
+	int i;
+
+	lock_vector <<= setting;
+	ch0->out_bound_acq = 0;
+	ch1->out_bound_acq = 0;
+	ch2->out_bound_acq = 0;
+	ch0->acq = 0;
+	ch1->acq = 0;
+	ch2->acq = 0;
+
+	dw_phy_eq_equal_setting(dw_dev, lock_vector);
+	dw_phy_eq_auto_calib(dw_dev);
+
+	mdelay(PHY_EQ_SLEEP_TIME_CDR);
+	if (!phy_tmds_valid(dw_dev))
+		dev_dbg(dw_dev->dev, "TMDS is NOT valid\n");
+
+	ch0->read_acq = phy_read(dw_dev, PHY_CH0_EQ_STATUS3);
+	ch1->read_acq = phy_read(dw_dev, PHY_CH1_EQ_STATUS3);
+	ch2->read_acq = phy_read(dw_dev, PHY_CH2_EQ_STATUS3);
+
+	ch0->acq += ch0->read_acq;
+	ch1->acq += ch1->read_acq;
+	ch2->acq += ch2->read_acq;
+
+	ch0->upper_bound_acq = ch0->read_acq + PHY_EQ_BOUNDSPREAD;
+	ch0->lower_bound_acq = ch0->read_acq - PHY_EQ_BOUNDSPREAD;
+	ch1->upper_bound_acq = ch1->read_acq + PHY_EQ_BOUNDSPREAD;
+	ch1->lower_bound_acq = ch1->read_acq - PHY_EQ_BOUNDSPREAD;
+	ch2->upper_bound_acq = ch2->read_acq + PHY_EQ_BOUNDSPREAD;
+	ch2->lower_bound_acq = ch2->read_acq - PHY_EQ_BOUNDSPREAD;
+
+	for (i = 1; i < acq; i++) {
+		dw_phy_eq_auto_calib(dw_dev);
+		mdelay(PHY_EQ_SLEEP_TIME_ACQ);
+
+		if ((ch0->read_acq > ch0->upper_bound_acq) ||
+				(ch0->read_acq < ch0->lower_bound_acq))
+			ch0->out_bound_acq++;
+		if ((ch1->read_acq > ch1->upper_bound_acq) ||
+				(ch1->read_acq < ch1->lower_bound_acq))
+			ch1->out_bound_acq++;
+		if ((ch2->read_acq > ch2->upper_bound_acq) ||
+				(ch2->read_acq < ch1->lower_bound_acq))
+			ch2->out_bound_acq++;
+
+		if (i == PHY_EQ_MIN_ACQ_STABLE) {
+			if ((ch0->out_bound_acq == 0) &&
+					(ch1->out_bound_acq == 0) &&
+					(ch2->out_bound_acq == 0)) {
+				acq = 3;
+				break;
+			}
+		}
+
+		ch0->read_acq = phy_read(dw_dev, PHY_CH0_EQ_STATUS3);
+		ch1->read_acq = phy_read(dw_dev, PHY_CH1_EQ_STATUS3);
+		ch2->read_acq = phy_read(dw_dev, PHY_CH2_EQ_STATUS3);
+
+		ch0->acq += ch0->read_acq;
+		ch1->acq += ch1->read_acq;
+		ch2->acq += ch2->read_acq;
+	}
+
+	ch0->acq = ch0->acq / acq;
+	ch1->acq = ch1->acq / acq;
+	ch2->acq = ch2->acq / acq;
+
+	return true;
+}
+
+static int dw_phy_eq_test_type(u16 setting, bool tmds_valid,
+		struct dw_phy_eq_ch *ch)
+{
+	u16 step_slope = 0;
+
+	if ((ch->acq < ch->last_acq) && tmds_valid) {
+		/* Long cable equalization */
+		ch->acc += ch->last_acq - ch->acq;
+		if ((ch->valid_long_setting == 0) && (ch->acq < 512) &&
+				(ch->acc > 0)) {
+			ch->best_long_setting = setting;
+			ch->valid_long_setting = 1;
+		}
+		step_slope = ch->last_acq - ch->acq;
+	}
+
+	if (tmds_valid && (ch->valid_short_setting == 0)) {
+		/* Short cable equalization */
+		if ((setting < PHY_EQ_SHORT_CABLE_SETTING) &&
+				(ch->acq < PHY_EQ_EQUALIZED_COUNTER_VAL)) {
+			ch->best_short_setting= setting;
+			ch->valid_short_setting = 1;
+		}
+
+		if (setting == PHY_EQ_SHORT_CABLE_SETTING) {
+			ch->best_short_setting = PHY_EQ_SHORT_CABLE_SETTING;
+			ch->valid_short_setting = 1;
+		}
+	}
+
+	if (ch->valid_long_setting && (ch->acc > PHY_EQ_ACC_LIMIT)) {
+		ch->best_setting = ch->best_long_setting;
+		return 1;
+	}
+
+	if ((setting == PHY_EQ_MAX_SETTING) && (ch->acc < PHY_EQ_ACC_LIMIT) &&
+			ch->valid_short_setting) {
+		ch->best_setting = ch->best_short_setting;
+		return 2;
+	}
+
+	if ((setting == PHY_EQ_MAX_SETTING) && tmds_valid &&
+			(ch->acc > PHY_EQ_ACC_LIMIT) &&
+			(step_slope > PHY_EQ_MIN_SLOPE)) {
+		ch->best_setting = PHY_EQ_MAX_SETTING;
+		return 3;
+	}
+
+	if (setting == PHY_EQ_MAX_SETTING) {
+		ch->best_setting = PHY_EQ_ERROR_CABLE_SETTING;
+		return 255;
+	}
+
+	return 0;
+}
+
+static bool dw_phy_eq_setting_finder(struct dw_phy_dev *dw_dev, u16 acq,
+		struct dw_phy_eq_ch *ch0, struct dw_phy_eq_ch *ch1,
+		struct dw_phy_eq_ch *ch2)
+{
+	u16 act = 0;
+	int ret_ch0 = 0, ret_ch1 = 0, ret_ch2 = 0;
+	bool tmds_valid = false;
+
+	dw_phy_eq_init_vars(ch0);
+	dw_phy_eq_init_vars(ch1);
+	dw_phy_eq_init_vars(ch2);
+
+	tmds_valid = dw_phy_eq_acquire_early_cnt(dw_dev, act, acq,
+			ch0, ch1, ch2);
+
+	while ((ret_ch0 == 0) || (ret_ch1 == 0) || (ret_ch2 == 0)) {
+		act++;
+
+		ch0->last_acq = ch0->acq;
+		ch1->last_acq = ch1->acq;
+		ch2->last_acq = ch2->acq;
+
+		tmds_valid = dw_phy_eq_acquire_early_cnt(dw_dev, act, acq,
+				ch0, ch1, ch2);
+
+		if (!ret_ch0)
+			ret_ch0 = dw_phy_eq_test_type(act, tmds_valid, ch0);
+		if (!ret_ch1)
+			ret_ch1 = dw_phy_eq_test_type(act, tmds_valid, ch1);
+		if (!ret_ch2)
+			ret_ch2 = dw_phy_eq_test_type(act, tmds_valid, ch2);
+	}
+
+	if ((ret_ch0 == 255) || (ret_ch1 == 255) || (ret_ch2 == 255))
+		return false;
+	return true;
+}
+
+static bool dw_phy_eq_maxvsmin(u16 ch0_setting, u16 ch1_setting,
+		u16 ch2_setting, u16 min_max_length)
+{
+	u16 min = ch0_setting, max = ch0_setting;
+
+	if (ch1_setting > max)
+		max = ch1_setting;
+	if (ch2_setting > max)
+		max = ch2_setting;
+	if (ch1_setting < min)
+		min = ch1_setting;
+	if (ch2_setting < min)
+		min = ch2_setting;
+
+	if ((max - min) > min_max_length)
+		return false;
+	return true;
+}
+
+static int dw_phy_eq_init(struct dw_phy_dev *dw_dev, u16 acq, bool force)
+{
+	struct dw_phy_pdata *phy = dw_dev->config;
+	struct dw_phy_eq_ch ch0, ch1, ch2;
+	u16 fat_bit_mask, lock_vector = 0x1;
+	int min_max_length, i, ret = 0;
+	u16 mpll_status;
+
+	if (phy->version < 401)
+		return ret;
+	if (!dw_dev->phy_enabled)
+		return -EINVAL;
+
+	mpll_status = phy_read(dw_dev, PHY_CLK_MPLL_STATUS);
+	if (mpll_status == dw_dev->mpll_status && !force)
+		return ret;
+	dw_dev->mpll_status = mpll_status;
+
+	phy_write(dw_dev, 0x00, PHY_MAINFSM_OVR2);
+	phy_write(dw_dev, 0x00, PHY_CH0_EQ_CTRL3);
+	phy_write(dw_dev, 0x00, PHY_CH1_EQ_CTRL3);
+	phy_write(dw_dev, 0x00, PHY_CH2_EQ_CTRL3);
+
+	ret = dw_phy_eq_test(dw_dev, &fat_bit_mask, &min_max_length);
+	if (ret) {
+		if (ret == -EINVAL) /* Means equalizer is not needed */
+			ret = 0;
+
+		/* Do not change values if we don't have clock */
+		if (ret != -ETIMEDOUT) {
+			dw_phy_eq_default(dw_dev);
+			phy_pddq(dw_dev, 1);
+			phy_pddq(dw_dev, 0);
+		}
+	} else {
+		dw_phy_eq_single(dw_dev);
+		dw_phy_eq_equal_setting(dw_dev, 0x0001);
+		phy_write(dw_dev, fat_bit_mask, PHY_CH0_EQ_CTRL6);
+		phy_write(dw_dev, fat_bit_mask, PHY_CH1_EQ_CTRL6);
+		phy_write(dw_dev, fat_bit_mask, PHY_CH2_EQ_CTRL6);
+
+		for (i = 0; i < PHY_EQ_MINMAX_NTRIES; i++) {
+			if (dw_phy_eq_setting_finder(dw_dev, acq,
+						&ch0, &ch1, &ch2)) {
+				if (dw_phy_eq_maxvsmin(ch0.best_setting,
+							ch1.best_setting,
+							ch2.best_setting,
+							min_max_length))
+					break;
+			}
+
+			ch0.best_setting = PHY_EQ_ERROR_CABLE_SETTING;
+			ch1.best_setting = PHY_EQ_ERROR_CABLE_SETTING;
+			ch2.best_setting = PHY_EQ_ERROR_CABLE_SETTING;
+		}
+
+		dev_dbg(dw_dev->dev, "equalizer settings: "
+				"ch0=0x%x, ch1=0x%x, ch1=0x%x\n",
+				ch0.best_setting, ch1.best_setting,
+				ch2.best_setting);
+
+		if (i == PHY_EQ_MINMAX_NTRIES)
+			ret = -EINVAL;
+
+		lock_vector = 0x1;
+		lock_vector <<= ch0.best_setting;
+		dw_phy_eq_equal_setting_ch0(dw_dev, lock_vector);
+
+		lock_vector = 0x1;
+		lock_vector <<= ch1.best_setting;
+		dw_phy_eq_equal_setting_ch1(dw_dev, lock_vector);
+
+		lock_vector = 0x1;
+		lock_vector <<= ch2.best_setting;
+		dw_phy_eq_equal_setting_ch2(dw_dev, lock_vector);
+
+		phy_pddq(dw_dev, 1);
+		phy_pddq(dw_dev, 0);
+	}
+
+	return ret;
+}
+
+static int dw_phy_config(struct dw_phy_dev *dw_dev, unsigned char color_depth,
+		bool hdmi2, bool scrambling)
+{
+	struct device *dev = dw_dev->dev;
+	struct dw_phy_pdata *phy = dw_dev->config;
+	const struct dw_phy_mpll_config *mpll_cfg = dw_phy_e405_mpll_cfg;
+	bool zcal_done;
+	u16 val, res_idx;
+	int timeout = 100;
+
+	dev_dbg(dev, "%s: color_depth=%d, hdmi2=%d, scrambling=%d\n",
+			__func__, color_depth, hdmi2, scrambling);
+
+	switch (color_depth) {
+	case 8:
+		res_idx = 0x0;
+		break;
+	case 10:
+		res_idx = 0x1;
+		break;
+	case 12:
+		res_idx = 0x2;
+		break;
+	case 16:
+		res_idx = 0x3;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	phy_reset(dw_dev, 1);
+	phy_pddq(dw_dev, 1);
+	phy_svsmode(dw_dev, 1);
+
+	phy_zcal_reset(dw_dev);
+	do {
+		udelay(1000);
+		zcal_done = phy_zcal_done(dw_dev);
+	} while (!zcal_done && timeout--);
+
+	if (!zcal_done) {
+		dev_err(dw_dev->dev, "Zcal calibration failed\n");
+		return -ETIMEDOUT;
+	}
+
+	phy_reset(dw_dev, 0);
+
+	/* CMU */
+	val = (0x08 << 10) | (0x01 << 9);
+	val |= (phy->cfg_clk * 4) & GENMASK(8, 0);
+	phy_write(dw_dev, val, PHY_CMU_CONFIG);
+
+	/* Color Depth and enable fast switching */
+	val = phy_read(dw_dev, PHY_SYSTEM_CONFIG);
+	val = (val & ~0x60) | (res_idx << 5) | BIT(11);
+	phy_write(dw_dev, val, PHY_SYSTEM_CONFIG);
+
+	/* MPLL */
+	for (; mpll_cfg->addr != 0x0; mpll_cfg++)
+		phy_write(dw_dev, mpll_cfg->val, mpll_cfg->addr);
+
+	/* Operation for data rates between 3.4Gbps and 6Gbps */
+	val = phy_read(dw_dev, PHY_CDR_CTRL_CNT);
+	if (hdmi2)
+		val |= BIT(8);
+	else
+		val &= ~BIT(8);
+	phy_write(dw_dev, val, PHY_CDR_CTRL_CNT);
+
+	/* Scrambling */
+	val = phy_read(dw_dev, PHY_OVL_PROT_CTRL);
+	if (scrambling)
+		val |= GENMASK(7,6);
+	else
+		val &= ~GENMASK(7,6);
+	phy_write(dw_dev, val, PHY_OVL_PROT_CTRL);
+
+	/* Enable phy */
+	phy_pddq(dw_dev, 0);
+
+	dw_dev->color_depth = color_depth;
+	dw_dev->hdmi2 = hdmi2;
+	dw_dev->scrambling = scrambling;
+	return 0;
+}
+
+static int dw_phy_enable(struct dw_phy_dev *dw_dev, unsigned char color_depth,
+		bool hdmi2, bool scrambling)
+{
+	int ret;
+
+	if (dw_dev->phy_enabled &&
+	    dw_dev->color_depth == color_depth &&
+	    dw_dev->hdmi2 == hdmi2 &&
+	    dw_dev->scrambling == scrambling)
+		return 0;
+
+	ret = dw_phy_config(dw_dev, color_depth, hdmi2, scrambling);
+	if (ret)
+		return ret;
+
+	phy_reset(dw_dev, 0);
+	phy_pddq(dw_dev, 0);
+	dw_dev->phy_enabled = true;
+	return 0;
+}
+
+static void dw_phy_disable(struct dw_phy_dev *dw_dev)
+{
+	if (!dw_dev->phy_enabled)
+		return;
+
+	phy_reset(dw_dev, 1);
+	phy_pddq(dw_dev, 1);
+	phy_svsmode(dw_dev, 0);
+	dw_dev->mpll_status = 0xFFFF;
+	dw_dev->phy_enabled = false;
+}
+
+static long dw_phy_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg)
+{
+	struct dw_phy_dev *dw_dev = to_dw_dev(sd);
+	struct dw_phy_config_command *ccmd;
+	struct dw_phy_eq_command *ecmd;
+	int ret = 0;
+
+	dev_dbg(dw_dev->dev, "%s: cmd=%d\n", __func__, cmd);
+
+	mutex_lock(&dw_dev->lock);
+	switch (cmd) {
+	case DW_PHY_IOCTL_EQ_INIT:
+		ecmd = (struct dw_phy_eq_command *)arg; 
+		ecmd->result = dw_phy_eq_init(dw_dev, ecmd->nacq, ecmd->force);
+		break;
+	case DW_PHY_IOCTL_CONFIG:
+		ccmd = (struct dw_phy_config_command *)arg;
+		ccmd->result = dw_phy_enable(dw_dev, ccmd->color_depth,
+				ccmd->hdmi2, ccmd->scrambling);
+		break;
+	default:
+		ret = -ENOIOCTLCMD;
+		break;
+	}
+	mutex_unlock(&dw_dev->lock);
+
+	return ret;
+}
+
+static int dw_phy_s_power(struct v4l2_subdev *sd, int on)
+{
+	struct dw_phy_dev *dw_dev = to_dw_dev(sd);
+
+	dev_dbg(dw_dev->dev, "%s: on=%d\n", __func__, on);
+
+	mutex_lock(&dw_dev->lock);
+	if (!on)
+		dw_phy_disable(dw_dev);
+	mutex_unlock(&dw_dev->lock);
+	return 0;
+}
+
+static const struct v4l2_subdev_core_ops dw_phy_core_ops = {
+	.ioctl = dw_phy_ioctl,
+	.s_power = dw_phy_s_power,
+};
+
+static const struct v4l2_subdev_ops dw_phy_sd_ops = {
+	.core = &dw_phy_core_ops,
+};
+
+static int dw_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct dw_phy_dev *dw_dev;
+	struct dw_phy_pdata *pdata = pdev->dev.platform_data;
+	struct v4l2_subdev *sd;
+
+	dev_dbg(dev, "probe start\n");
+
+	/* Resource allocation */
+	dw_dev = devm_kzalloc(dev, sizeof(*dw_dev), GFP_KERNEL);
+	if (!dw_dev)
+		return -ENOMEM;
+
+	/* Resource initialization */
+	if (!pdata)
+		return -EINVAL;
+
+	dw_dev->dev = dev;
+	dw_dev->config = pdata;
+	mutex_init(&dw_dev->lock);
+
+	/* V4L2 initialization */
+	sd = &dw_dev->sd;
+	v4l2_subdev_init(sd, &dw_phy_sd_ops);
+	strlcpy(sd->name, dev_name(dev), sizeof(sd->name));
+
+	/* Force phy disabling */
+	dw_dev->phy_enabled = true;
+	dw_phy_disable(dw_dev);
+
+	/* All done */
+	dev_set_drvdata(dev, sd);
+	dev_info(dev, "driver probed\n");
+	return 0;
+}
+
+static int dw_phy_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	dev_info(dev, "driver removed\n");
+	return 0;
+}
+
+static struct platform_driver dw_phy_e405_driver = {
+	.probe = dw_phy_probe,
+	.remove = dw_phy_remove,
+	.driver = {
+		.name = DW_PHY_E405_DRVNAME,
+	}
+};
+module_platform_driver(dw_phy_e405_driver);
diff --git a/drivers/media/platform/dwc/dw-hdmi-phy-e405.h b/drivers/media/platform/dwc/dw-hdmi-phy-e405.h
new file mode 100644
index 0000000..3e6c8da
--- /dev/null
+++ b/drivers/media/platform/dwc/dw-hdmi-phy-e405.h
@@ -0,0 +1,63 @@
+/*
+ * Synopsys Designware HDMI PHY E405 driver
+ *
+ * This Synopsys dw-phy-e405 software and associated documentation
+ * (hereinafter the "Software") is an unsupported proprietary work of
+ * Synopsys, Inc. unless otherwise expressly agreed to in writing between
+ * Synopsys and you. The Software IS NOT an item of Licensed Software or a
+ * Licensed Product under any End User Software License Agreement or
+ * Agreement for Licensed Products with Synopsys or any supplement thereto.
+ * Synopsys is a registered trademark of Synopsys, Inc. Other names included
+ * in the SOFTWARE may be the trademarks of their respective owners.
+ *
+ * The contents of this file are dual-licensed; you may select either version 2
+ * of the GNU General Public License (“GPL”) or the MIT license (“MIT”).
+ *
+ * Copyright (c) 2017 Synopsys, Inc. and/or its affiliates.
+ *
+ * THIS 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 THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef __DW_HDMI_PHY_E405_H__
+#define __DW_HDMI_PHY_E405_H__
+
+#define PHY_CMU_CONFIG				0x02
+#define PHY_SYSTEM_CONFIG			0x03
+#define PHY_MAINFSM_CTRL			0x05
+#define PHY_MAINFSM_OVR2			0x08
+#define PHY_MAINFSM_STATUS1			0x09
+#define PHY_OVL_PROT_CTRL			0x0D
+#define PHY_CDR_CTRL_CNT			0x0E
+#define PHY_CLK_MPLL_STATUS			0x2F
+#define PHY_CH0_EQ_CTRL1			0x32
+#define PHY_CH0_EQ_CTRL2			0x33
+#define PHY_CH0_EQ_STATUS			0x34
+#define PHY_CH0_EQ_CTRL3			0x3E
+#define PHY_CH0_EQ_CTRL4			0x3F
+#define PHY_CH0_EQ_STATUS2			0x40
+#define PHY_CH0_EQ_STATUS3			0x42
+#define PHY_CH0_EQ_CTRL6			0x43
+#define PHY_CH1_EQ_CTRL1			0x52
+#define PHY_CH1_EQ_CTRL2			0x53
+#define PHY_CH1_EQ_STATUS			0x54
+#define PHY_CH1_EQ_CTRL3			0x5E
+#define PHY_CH1_EQ_CTRL4			0x5F
+#define PHY_CH1_EQ_STATUS2			0x60
+#define PHY_CH1_EQ_STATUS3			0x62
+#define PHY_CH1_EQ_CTRL6			0x63
+#define PHY_CH2_EQ_CTRL1			0x72
+#define PHY_CH2_EQ_CTRL2			0x73
+#define PHY_CH2_EQ_STATUS			0x74
+#define PHY_CH2_EQ_CTRL3			0x7E
+#define PHY_CH2_EQ_CTRL4			0x7F
+#define PHY_CH2_EQ_STATUS2			0x80
+#define PHY_CH2_EQ_STATUS3			0x82
+#define PHY_CH2_EQ_CTRL6			0x83
+
+#endif /* __DW_HDMI_PHY_E405_H__ */
diff --git a/include/media/dwc/dw-hdmi-phy-pdata.h b/include/media/dwc/dw-hdmi-phy-pdata.h
new file mode 100644
index 0000000..1be1cc3
--- /dev/null
+++ b/include/media/dwc/dw-hdmi-phy-pdata.h
@@ -0,0 +1,131 @@
+/*
+ * Synopsys Designware HDMI PHY platform data
+ *
+ * This Synopsys dw-hdmi-phy software and associated documentation
+ * (hereinafter the "Software") is an unsupported proprietary work of
+ * Synopsys, Inc. unless otherwise expressly agreed to in writing between
+ * Synopsys and you. The Software IS NOT an item of Licensed Software or a
+ * Licensed Product under any End User Software License Agreement or
+ * Agreement for Licensed Products with Synopsys or any supplement thereto.
+ * Synopsys is a registered trademark of Synopsys, Inc. Other names included
+ * in the SOFTWARE may be the trademarks of their respective owners.
+ *
+ * The contents of this file are dual-licensed; you may select either version 2
+ * of the GNU General Public License (“GPL”) or the MIT license (“MIT”).
+ *
+ * Copyright (c) 2017 Synopsys, Inc. and/or its affiliates.
+ *
+ * THIS 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 THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef __DW_HDMI_PHY_PDATA_H__
+#define __DW_HDMI_PHY_PDATA_H__
+
+#define DW_PHY_E405_DRVNAME	"dw-hdmi-phy-e405"
+
+#define DW_PHY_IOCTL_EQ_INIT		_IOW('R',1,int)
+#define DW_PHY_IOCTL_CONFIG		_IOW('R',2,int)
+
+/**
+ * struct dw_phy_eq_command - Command arguments for HDMI PHY equalizer
+ * 	algorithm.
+ *
+ * @nacq: Number of acquisitions to get.
+ *
+ * @force: Force equalizer algorithm even if the MPLL status didn't change
+ * from previous run.
+ *
+ * @result: Result from the equalizer algorithm. Shall be zero if equalizer
+ * ran with success or didn't run because the video mode does not need
+ * equalizer (for low clock values).
+ */
+struct dw_phy_eq_command {
+	u16 nacq;
+	bool force;
+	int result;
+};
+
+/**
+ * struct dw_phy_config_command - Command arguments for HDMI PHY configuration
+ * function.
+ *
+ * @color_depth: Color depth of the video mode being received.
+ *
+ * @hdmi2: Must be set to true if the video mode being received has a data
+ * rate above 3.4Gbps (generally HDMI 2.0 video modes, like 4k@60Hz).
+ *
+ * @scrambling: Must be set to true if scrambling is currently enabled.
+ * Scrambling is enabled by source when SCDC scrambling_en field is set to 1.
+ */
+struct dw_phy_config_command {
+	unsigned char color_depth;
+	bool hdmi2;
+	bool scrambling;
+	int result;
+};
+
+/**
+ * struct dw_phy_funcs - Set of callbacks used to communicate between phy
+ * 	and hdmi controller. Controller must correctly fill these callbacks
+ * 	before probbing the phy driver.
+ *
+ * @write: write callback. Write value 'val' into address 'addr' of phy.
+ *
+ * @read: read callback. Read address 'addr' and return the value.
+ *
+ * @reset: reset callback. Activate phy reset. Active high.
+ *
+ * @pddq: pddq callback. Activate phy configuration mode. Active high.
+ *
+ * @svsmode: svsmode callback. Activate phy retention mode. Active low.
+ *
+ * @zcal_reset: zcal reset callback. Restart the impedance calibration
+ * procedure. Active high. This is only used in prototyping and not in real
+ * ASIC. Callback shall be empty (but non NULL) in ASIC cases.
+ *
+ * @zcal_done: zcal done callback. Return the current status of impedance
+ * calibration procedure. This is only used in prototyping and not in real
+ * ASIC. Shall return always true in ASIC cases.
+ *
+ * @tmds_valid: TMDS valid callback. Return the current status of TMDS signal
+ * that comes from phy and feeds controller. This is read from a controller
+ * register.
+ */
+struct dw_phy_funcs {
+	void (*write) (void *arg, u16 val, u16 addr);
+	u16 (*read) (void *arg, u16 addr);
+	void (*reset) (void *arg, int enable);
+	void (*pddq) (void *arg, int enable);
+	void (*svsmode) (void *arg, int enable);
+	void (*zcal_reset) (void *arg);
+	bool (*zcal_done) (void *arg);
+	bool (*tmds_valid) (void *arg);
+};
+
+/**
+ * struct dw_phy_pdata - Platform data definition for Synopsys HDMI PHY.
+ *
+ * @version: phy version number.
+ *
+ * @cfg_clk: value of cfg_clk that is fed to the phy.
+ *
+ * @funcs: set of callbacks that must be correctly filled and supplied to phy.
+ * See @dw_phy_funcs.
+ *
+ * @funcs_arg: parameter that is supplied to callbacks along with the function
+ * parameters.
+ */
+struct dw_phy_pdata {
+	unsigned int version;
+	unsigned int cfg_clk;
+	const struct dw_phy_funcs *funcs;
+	void *funcs_arg;
+};
+
+#endif /* __DW_HDMI_PHY_PDATA_H__ */
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1668639 — Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver

FromSylwester Nawrocki <snawrocki@kernel.org>
Date2017-06-18 20:10 +0200
SubjectRe: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver
Message-ID<tTMO5-6h-7@gated-at.bofh.it>
In reply to#1667888
On 06/16/2017 06:38 PM, Jose Abreu wrote:
> This is an initial submission for the Synopsys Designware HDMI RX
> Controller Driver. This driver interacts with a phy driver so that
> a communication between them is created and a video pipeline is
> configured.
> 
> The controller + phy pipeline can then be integrated into a fully
> featured system that can be able to receive video up to 4k@60Hz
> with deep color 48bit RGB, depending on the platform. Although,
> this initial version does not yet handle deep color modes.

> Signed-off-by: Jose Abreu <joabreu@synopsys.com>

> +static int dw_hdmi_phy_init(struct dw_hdmi_dev *dw_dev)
> +{
> +	struct dw_phy_pdata *phy = &dw_dev->phy_config;
> +	struct platform_device_info pdevinfo;
> +
> +	memset(&pdevinfo, 0, sizeof(pdevinfo));
> +
> +	phy->funcs = &dw_hdmi_phy_funcs;
> +	phy->funcs_arg = dw_dev;
> +
> +	pdevinfo.parent = dw_dev->dev;
> +	pdevinfo.id = PLATFORM_DEVID_NONE;
> +	pdevinfo.name = dw_dev->phy_drv;
> +	pdevinfo.data = phy;
> +	pdevinfo.size_data = sizeof(*phy);
> +	pdevinfo.dma_mask = DMA_BIT_MASK(32);
> +
> +	request_module(pdevinfo.name);
> +
> +	dw_dev->phy_pdev = platform_device_register_full(&pdevinfo);
> +	if (IS_ERR(dw_dev->phy_pdev)) {
> +		dev_err(dw_dev->dev, "failed to register phy device\n");
> +		return PTR_ERR(dw_dev->phy_pdev);
> +	}
> +
> +	if (!dw_dev->phy_pdev->dev.driver) {
> +		dev_err(dw_dev->dev, "failed to initialize phy driver\n");
> +		goto err;
> +	}

I think this is not safe because there is nothing preventing unbinding 
or unloading the driver at this point.

> +	if (!try_module_get(dw_dev->phy_pdev->dev.driver->owner)) {

So dw_dev->phy_pdev->dev.driver may be already NULL here.

> +		dev_err(dw_dev->dev, "failed to get phy module\n");
> +		goto err;
> +	}
> +
> +	dw_dev->phy_sd = dev_get_drvdata(&dw_dev->phy_pdev->dev);
> +	if (!dw_dev->phy_sd) {
> +		dev_err(dw_dev->dev, "failed to get phy subdev\n");
> +		goto err_put;
> +	}
> +
> +	if (v4l2_device_register_subdev(&dw_dev->v4l2_dev, dw_dev->phy_sd)) {
> +		dev_err(dw_dev->dev, "failed to register phy subdev\n");
> +		goto err_put;
> +	}

I'd suggest usign v4l2-async API, so we use a common pattern for sub-device
registration.  And with recent change [1] you could handle this PHY subdev
in a standard way.  That might be more complicated than it is now but should 
make any future platform integration easier.

[1] https://patchwork.linuxtv.org/patch/41834

> +	module_put(dw_dev->phy_pdev->dev.driver->owner);
> +	return 0;
> +
> +err_put:
> +	module_put(dw_dev->phy_pdev->dev.driver->owner);
> +err:
> +	platform_device_unregister(dw_dev->phy_pdev);
> +	return -EINVAL;
> +}
> +
> +static void dw_hdmi_phy_exit(struct dw_hdmi_dev *dw_dev)
> +{
> +	if (!IS_ERR(dw_dev->phy_pdev))
> +		platform_device_unregister(dw_dev->phy_pdev);
> +}

> +static int dw_hdmi_config_hdcp(struct dw_hdmi_dev *dw_dev)
> +{

> +	for (i = 0; i < DW_HDMI_HDCP14_KEYS_SIZE; i += 2) {
> +		for (j = 0; j < key_write_tries; j++) {
> +			if (is_hdcp14_key_write_allowed(dw_dev))
> +				break;
> +			mdelay(10);

usleep_range()? I've seen more (busy waiting) mdelay() calls in this
patch series.


> +static int __dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	ret = dw_hdmi_config(dw_dev, input);
> +
> +	spin_lock_irqsave(&dw_dev->lock, flags);
> +	dw_dev->pending_config = false;
> +	spin_unlock_irqrestore(&dw_dev->lock, flags);
> +
> +	return ret;
> +}
> +
> +struct dw_hdmi_work_data {
> +	struct dw_hdmi_dev *dw_dev;
> +	struct work_struct work;
> +	u32 input;
> +};
> +
> +static void dw_hdmi_work_handler(struct work_struct *work)
> +{
> +	struct dw_hdmi_work_data *data = container_of(work,
> +			struct dw_hdmi_work_data, work);
> +
> +	__dw_hdmi_power_on(data->dw_dev, data->input);
> +	devm_kfree(data->dw_dev->dev, data);
> +}
> +
> +static int dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
> +{
> +	struct dw_hdmi_work_data *data;
> +	unsigned long flags;
> +
> +	data = devm_kzalloc(dw_dev->dev, sizeof(*data), GFP_KERNEL);

Why use devm_{kzalloc, kfree} when dw_hdmi_power_on() is not only called
in the device's probe() callback, but in other places, including interrupt 
handler?  devm_* API is normally used when life time of a resource is more 
or less equal to life time of struct device or its matched driver.  Were 
there any specific reasons to not just use kzalloc()/kfree() ?

> +	if (!data)
> +		return -ENOMEM;
> +
> +	INIT_WORK(&data->work, dw_hdmi_work_handler);
> +	data->dw_dev = dw_dev;
> +	data->input = input;
> +
> +	spin_lock_irqsave(&dw_dev->lock, flags);
> +	if (dw_dev->pending_config) {
> +		devm_kfree(dw_dev->dev, data);
> +		spin_unlock_irqrestore(&dw_dev->lock, flags);
> +		return 0;
> +	}
> +
> +	queue_work(dw_dev->wq, &data->work);
> +	dw_dev->pending_config = true;
> +	spin_unlock_irqrestore(&dw_dev->lock, flags);
> +	return 0;
> +}

> +static irqreturn_t dw_hdmi_irq_handler(int irq, void *dev_data)
> +{
> +	struct dw_hdmi_dev *dw_dev = dev_data;
> +	u32 hdmi_ists = dw_hdmi_get_int_val(dw_dev, HDMI_ISTS, HDMI_IEN);
> +	u32 md_ists = dw_hdmi_get_int_val(dw_dev, HDMI_MD_ISTS, HDMI_MD_IEN);
> +
> +	dw_hdmi_clear_ints(dw_dev);
> +
> +	if ((hdmi_ists & HDMI_ISTS_CLK_CHANGE) ||
> +	    (hdmi_ists & HDMI_ISTS_PLL_LCK_CHG) || md_ists) {
> +		dw_hdmi_power_off(dw_dev);
> +		if (has_signal(dw_dev, dw_dev->configured_input))
> +			dw_hdmi_power_on(dw_dev, dw_dev->configured_input);

> +	}
> +	return IRQ_HANDLED;
> +}

> +static int dw_hdmi_registered(struct v4l2_subdev *sd)
> +{
> +	struct dw_hdmi_dev *dw_dev = to_dw_dev(sd);
> +	int ret;
> +
> +	ret = cec_register_adapter(dw_dev->cec_adap, dw_dev->dev);
> +	if (ret) {
> +		dev_err(dw_dev->dev, "failed to register CEC adapter\n");
> +		cec_delete_adapter(dw_dev->cec_adap);
> +		return ret;
> +	}
> +
> +	cec_register_cec_notifier(dw_dev->cec_adap, dw_dev->cec_notifier);
> +	dw_dev->registered = true;
> +	return ret;
> +}
> +
> +static void dw_hdmi_unregistered(struct v4l2_subdev *sd)
> +{
> +	struct dw_hdmi_dev *dw_dev = to_dw_dev(sd);
> +
> +	cec_unregister_adapter(dw_dev->cec_adap);
> +	cec_notifier_put(dw_dev->cec_notifier);
> +}
> +
> +static const struct v4l2_subdev_core_ops dw_hdmi_sd_core_ops = {
> +	.log_status = dw_hdmi_log_status,

> +	.subscribe_event = dw_hdmi_subscribe_event,
> +};
> +
> +static const struct v4l2_subdev_video_ops dw_hdmi_sd_video_ops = {
> +	.s_routing = dw_hdmi_s_routing,
> +	.g_input_status = dw_hdmi_g_input_status,
> +	.g_parm = dw_hdmi_g_parm,
> +	.g_dv_timings = dw_hdmi_g_dv_timings,
> +	.query_dv_timings = dw_hdmi_query_dv_timings,
> +};
> +
> +static const struct v4l2_subdev_pad_ops dw_hdmi_sd_pad_ops = {
> +	.enum_mbus_code = dw_hdmi_enum_mbus_code,
> +	.get_fmt = dw_hdmi_get_fmt,
> +	.set_fmt = dw_hdmi_set_fmt,
> +	.dv_timings_cap = dw_hdmi_dv_timings_cap,
> +	.enum_dv_timings = dw_hdmi_enum_dv_timings,
> +};
> +
> +static const struct v4l2_subdev_ops dw_hdmi_sd_ops = {
> +	.core = &dw_hdmi_sd_core_ops,
> +	.video = &dw_hdmi_sd_video_ops,
> +	.pad = &dw_hdmi_sd_pad_ops,
> +};
> +
> +static const struct v4l2_subdev_internal_ops dw_hdmi_internal_ops = {
> +	.registered = dw_hdmi_registered,
> +	.unregistered = dw_hdmi_unregistered,
> +};
> +
> +static int dw_hdmi_parse_dt(struct dw_hdmi_dev *dw_dev)
> +{
> +	struct device_node *notifier, *np = dw_dev->of_node;
> +	struct dw_phy_pdata *phy = &dw_dev->phy_config;
> +
> +	if (!np) {
> +		dev_err(dw_dev->dev, "missing DT node\n");
> +		return -EINVAL;
> +	}
> +
> +	/* PHY properties parsing */
> +	of_property_read_u8(np, "snps,hdmi-phy-jtag-addr",
> +			&dw_dev->phy_jtag_addr);
> +	if (!dw_dev->phy_jtag_addr) {
> +		dev_err(dw_dev->dev, "missing hdmi-phy-jtag-addr in DT\n");
> +		return -EINVAL;
> +	}
> +
> +	of_property_read_u32(np, "snps,hdmi-phy-version", &phy->version);
> +	if (!phy->version) {
> +		dev_err(dw_dev->dev, "missing hdmi-phy-version in DT\n");
> +		return -EINVAL;
> +	}
> +
> +	of_property_read_u32(np, "snps,hdmi-phy-cfg-clk", &phy->cfg_clk);
> +	if (!phy->cfg_clk) {
> +		dev_err(dw_dev->dev, "missing hdmi-phy-cfg-clk in DT\n");
> +		return -EINVAL;
> +	}

With changes as proposed in comments to patch "4/4 dt-bindings: ..." 
you could use the common clk API for retrieving the clock rate, e.g. 
devm_clk_get(), clk_get_rate().

When the HDMI RX IP block gets integrated within some SoC I'd expect 
the system clock controller to be already using the common clk DT 
bindings. Unless for some reason the platform doesn't support CCF.


> +	if (of_property_read_string_index(np, "snps,hdmi-phy-driver", 0,
> +				&dw_dev->phy_drv) < 0) {
> +		dev_err(dw_dev->dev, "missing hdmi-phy-driver in DT\n");

I don't think we can put Linux driver names in DT like this, it seems rather 
a serious abuse.  With proposed changes to the DT binding you could reference 
the PHY device by DT phandle or child node.

> +		return -EINVAL;
> +	}
> +
> +	/* Controller properties parsing */
> +	of_property_read_u32(np, "snps,hdmi-ctl-cfg-clk", &dw_dev->cfg_clk);
> +	if (!dw_dev->cfg_clk) {
> +		dev_err(dw_dev->dev, "missing hdmi-ctl-cfg-clk in DT\n");
> +		return -EINVAL;
> +	}
> +
> +#if IS_ENABLED(CONFIG_VIDEO_DWC_HDMI_RX_CEC)
> +	/* Notifier device parsing */
> +	notifier = of_parse_phandle(np, "edid-phandle", 0);
> +	if (!notifier) {
> +		dev_err(dw_dev->dev, "missing edid-phandle in DT\n");
> +		return -EINVAL;
> +	}
> +
> +	dw_dev->notifier_pdev = of_find_device_by_node(np);

Shouldn't this be:
	dw_dev->notifier_pdev = of_find_device_by_node(notifier);
?

The caller of dw_hdmi_parse_dt() already knows about the device 
associated with np.

> +	if (!dw_dev->notifier_pdev)
> +		return -EPROBE_DEFER;
> +#endif
> +
> +	return 0;
> +}
> +
> +static int dw_hdmi_rx_probe(struct platform_device *pdev)
> +{

> +	/* Deferred work */
> +	dw_dev->wq = create_workqueue(DW_HDMI_RX_DRVNAME);

Have you considered using create_singlethread_workqueue() ? create_workqueue() 
will spawn one thread per CPU.

> +	if (!dw_dev->wq)
> +		return -ENOMEM;
> +
> +	/* Registers mapping */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

> +	if (!res) {
> +		ret = -ENXIO;
> +		goto err_wq;
> +	}

You can drop res testing here, devm_ioremap_resource() verifies internally 
if res is valid and returns proper error code.

> +	dw_dev->regs = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(dw_dev->regs)) {
> +		ret = PTR_ERR(dw_dev->regs);
> +		goto err_wq;
> +	}


> +	/* V4L2 initialization */
> +	sd = &dw_dev->sd;
> +	v4l2_subdev_init(sd, &dw_hdmi_sd_ops);
> +	strlcpy(sd->name, DW_HDMI_RX_DRVNAME, sizeof(sd->name));

sd->name should be unique, you could, for instance, do something like

	strlcpy(sd->name, dev_name(&pdev->dev), sizeof(sd->name));

> +	sd->internal_ops = &dw_hdmi_internal_ops;
> +	sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS;

> +}
> +
> +static int dw_hdmi_rx_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct dw_hdmi_dev *dw_dev = to_dw_dev(sd);
> +
> +	dev_dbg(dev, "%s\n", __func__);
> +
> +	dw_hdmi_disable_ints(dw_dev);
> +	dw_hdmi_disable_hpd(dw_dev);
> +	dw_hdmi_disable_scdc(dw_dev);
> +	dw_hdmi_power_off(dw_dev);
> +	dw_hdmi_phy_s_power(dw_dev, false);
> +	flush_workqueue(dw_dev->wq);
> +	destroy_workqueue(dw_dev->wq);
> +	v4l2_device_unregister(&dw_dev->v4l2_dev);
> +	dw_hdmi_phy_exit(dw_dev);> +	dev_info(dev, "driver removed\n");
> +	return 0;
> +}
> +
> +static struct platform_driver dw_hdmi_rx_driver = {
> +	.probe = dw_hdmi_rx_probe,
> +	.remove = dw_hdmi_rx_remove,

I think we need also .of_match_table here.

> +	.driver = {
> +		.name = DW_HDMI_RX_DRVNAME,
> +	}
> +};
> +module_platform_driver(dw_hdmi_rx_driver);

> +#endif /* __DW_HDMI_RX_H__ */
> diff --git a/include/media/dwc/dw-hdmi-rx-pdata.h b/include/media/dwc/dw-hdmi-rx-pdata.h
> new file mode 100644
> index 0000000..ff8554d
> --- /dev/null
> +++ b/include/media/dwc/dw-hdmi-rx-pdata.h
> @@ -0,0 +1,63 @@

> +#ifndef __DW_HDMI_RX_PDATA_H__
> +#define __DW_HDMI_RX_PDATA_H__
> +
> +#define DW_HDMI_RX_DRVNAME			"dw-hdmi-rx"
> +
> +/* Notify events */
> +#define DW_HDMI_NOTIFY_IS_OFF		1
> +#define DW_HDMI_NOTIFY_INPUT_CHANGED	2
> +#define DW_HDMI_NOTIFY_AUDIO_CHANGED	3
> +#define DW_HDMI_NOTIFY_IS_STABLE	4
> +
> +/* HDCP 1.4 */
> +#define DW_HDMI_HDCP14_BKSV_SIZE	2
> +#define DW_HDMI_HDCP14_KEYS_SIZE	(2 * 40)
> +
> +struct dw_hdmi_hdcp14_key {
> +	u32 seed;
> +	u32 bksv[DW_HDMI_HDCP14_BKSV_SIZE];
> +	u32 keys[DW_HDMI_HDCP14_KEYS_SIZE];
> +	bool keys_valid;
> +};
> +
> +struct dw_hdmi_rx_pdata {
> +	/* Controller configuration */
> +	unsigned int iref_clk; /* MHz */

Is this field unused?

> +	struct dw_hdmi_hdcp14_key hdcp14_keys;

> +	/* 5V sense interface */
> +	bool (*dw_5v_status)(void __iomem *regs, int input);
> +	void (*dw_5v_clear)(void __iomem *regs);
> +	void __iomem *dw_5v_arg;> +	/* Zcal interface */
> +	void (*dw_zcal_reset)(void __iomem *regs);
> +	bool (*dw_zcal_done)(void __iomem *regs);
> +	void __iomem *dw_zcal_arg;

I'm just wondering if these operations could be modeled with the regmap,
so we could avoid callbacks in the platform data structure.
 
> +};

> +#endif /* __DW_HDMI_RX_PDATA_H__ */

--
Regards,
Sylwester

[toc] | [prev] | [next] | [standalone]

#1668944 — Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver

FromJose Abreu <Jose.Abreu@synopsys.com>
Date2017-06-19 11:40 +0200
SubjectRe: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver
Message-ID<tU1k6-S2-15@gated-at.bofh.it>
In reply to#1668639
Hi Sylwester,


Thanks again for the feedback!


On 18-06-2017 19:04, Sylwester Nawrocki wrote:
> On 06/16/2017 06:38 PM, Jose Abreu wrote:
>> This is an initial submission for the Synopsys Designware HDMI RX
>> Controller Driver. This driver interacts with a phy driver so that
>> a communication between them is created and a video pipeline is
>> configured.
>>
>> The controller + phy pipeline can then be integrated into a fully
>> featured system that can be able to receive video up to 4k@60Hz
>> with deep color 48bit RGB, depending on the platform. Although,
>> this initial version does not yet handle deep color modes.
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> +static int dw_hdmi_phy_init(struct dw_hdmi_dev *dw_dev)
>> +{
>> +	struct dw_phy_pdata *phy = &dw_dev->phy_config;
>> +	struct platform_device_info pdevinfo;
>> +
>> +	memset(&pdevinfo, 0, sizeof(pdevinfo));
>> +
>> +	phy->funcs = &dw_hdmi_phy_funcs;
>> +	phy->funcs_arg = dw_dev;
>> +
>> +	pdevinfo.parent = dw_dev->dev;
>> +	pdevinfo.id = PLATFORM_DEVID_NONE;
>> +	pdevinfo.name = dw_dev->phy_drv;
>> +	pdevinfo.data = phy;
>> +	pdevinfo.size_data = sizeof(*phy);
>> +	pdevinfo.dma_mask = DMA_BIT_MASK(32);
>> +
>> +	request_module(pdevinfo.name);
>> +
>> +	dw_dev->phy_pdev = platform_device_register_full(&pdevinfo);
>> +	if (IS_ERR(dw_dev->phy_pdev)) {
>> +		dev_err(dw_dev->dev, "failed to register phy device\n");
>> +		return PTR_ERR(dw_dev->phy_pdev);
>> +	}
>> +
>> +	if (!dw_dev->phy_pdev->dev.driver) {
>> +		dev_err(dw_dev->dev, "failed to initialize phy driver\n");
>> +		goto err;
>> +	}
> I think this is not safe because there is nothing preventing unbinding 
> or unloading the driver at this point.
>
>> +	if (!try_module_get(dw_dev->phy_pdev->dev.driver->owner)) {
> So dw_dev->phy_pdev->dev.driver may be already NULL here.

How can I make sure it wont be NULL? Because I've seen other
media drivers do this and I don't think they do any kind of
locking, but they do this mainly for I2C subdevs.

>
>> +		dev_err(dw_dev->dev, "failed to get phy module\n");
>> +		goto err;
>> +	}
>> +
>> +	dw_dev->phy_sd = dev_get_drvdata(&dw_dev->phy_pdev->dev);
>> +	if (!dw_dev->phy_sd) {
>> +		dev_err(dw_dev->dev, "failed to get phy subdev\n");
>> +		goto err_put;
>> +	}
>> +
>> +	if (v4l2_device_register_subdev(&dw_dev->v4l2_dev, dw_dev->phy_sd)) {
>> +		dev_err(dw_dev->dev, "failed to register phy subdev\n");
>> +		goto err_put;
>> +	}
> I'd suggest usign v4l2-async API, so we use a common pattern for sub-device
> registration.  And with recent change [1] you could handle this PHY subdev
> in a standard way.  That might be more complicated than it is now but should 
> make any future platform integration easier.
>
> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.linuxtv.org_patch_41834&d=DwICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=uHTyp6WsEj6vN19Zc09HqSNUhCx62OI8u-tAgi-EVts&s=WjyPjIwN1uGvPoV7ZlcmzOgdptakzluHywuKRA8ZG8M&e= 

So I will instantiate phy driver and then wait for phy driver to
register into v4l2 core?

>
>> +	module_put(dw_dev->phy_pdev->dev.driver->owner);
>> +	return 0;
>> +
>> +err_put:
>> +	module_put(dw_dev->phy_pdev->dev.driver->owner);
>> +err:
>> +	platform_device_unregister(dw_dev->phy_pdev);
>> +	return -EINVAL;
>> +}
>> +
>> +static void dw_hdmi_phy_exit(struct dw_hdmi_dev *dw_dev)
>> +{
>> +	if (!IS_ERR(dw_dev->phy_pdev))
>> +		platform_device_unregister(dw_dev->phy_pdev);
>> +}
>> +static int dw_hdmi_config_hdcp(struct dw_hdmi_dev *dw_dev)
>> +{
>> +	for (i = 0; i < DW_HDMI_HDCP14_KEYS_SIZE; i += 2) {
>> +		for (j = 0; j < key_write_tries; j++) {
>> +			if (is_hdcp14_key_write_allowed(dw_dev))
>> +				break;
>> +			mdelay(10);
> usleep_range()? I've seen more (busy waiting) mdelay() calls in this
> patch series.

I will change.

>
>
>> +static int __dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
>> +{
>> +	unsigned long flags;
>> +	int ret;
>> +
>> +	ret = dw_hdmi_config(dw_dev, input);
>> +
>> +	spin_lock_irqsave(&dw_dev->lock, flags);
>> +	dw_dev->pending_config = false;
>> +	spin_unlock_irqrestore(&dw_dev->lock, flags);
>> +
>> +	return ret;
>> +}
>> +
>> +struct dw_hdmi_work_data {
>> +	struct dw_hdmi_dev *dw_dev;
>> +	struct work_struct work;
>> +	u32 input;
>> +};
>> +
>> +static void dw_hdmi_work_handler(struct work_struct *work)
>> +{
>> +	struct dw_hdmi_work_data *data = container_of(work,
>> +			struct dw_hdmi_work_data, work);
>> +
>> +	__dw_hdmi_power_on(data->dw_dev, data->input);
>> +	devm_kfree(data->dw_dev->dev, data);
>> +}
>> +
>> +static int dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
>> +{
>> +	struct dw_hdmi_work_data *data;
>> +	unsigned long flags;
>> +
>> +	data = devm_kzalloc(dw_dev->dev, sizeof(*data), GFP_KERNEL);
> Why use devm_{kzalloc, kfree} when dw_hdmi_power_on() is not only called
> in the device's probe() callback, but in other places, including interrupt 
> handler?  devm_* API is normally used when life time of a resource is more 
> or less equal to life time of struct device or its matched driver.  Were 
> there any specific reasons to not just use kzalloc()/kfree() ?

No specific reason, I just thought it would be safer because if I
cancel a work before it started then memory will remain
allocated. But I will change to kzalloc().

>
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	INIT_WORK(&data->work, dw_hdmi_work_handler);
>> +	data->dw_dev = dw_dev;
>> +	data->input = input;
>> +
>> +	spin_lock_irqsave(&dw_dev->lock, flags);
>> +	if (dw_dev->pending_config) {
>> +		devm_kfree(dw_dev->dev, data);
>> +		spin_unlock_irqrestore(&dw_dev->lock, flags);
>> +		return 0;
>> +	}
>> +
>> +	queue_work(dw_dev->wq, &data->work);
>> +	dw_dev->pending_config = true;
>> +	spin_unlock_irqrestore(&dw_dev->lock, flags);
>> +	return 0;
>> +}
>> +static irqreturn_t dw_hdmi_irq_handler(int irq, void *dev_data)
>> +{
>> +	struct dw_hdmi_dev *dw_dev = dev_data;
>> +	u32 hdmi_ists = dw_hdmi_get_int_val(dw_dev, HDMI_ISTS, HDMI_IEN);
>> +	u32 md_ists = dw_hdmi_get_int_val(dw_dev, HDMI_MD_ISTS, HDMI_MD_IEN);
>> +
>> +	dw_hdmi_clear_ints(dw_dev);
>> +
>> +	if ((hdmi_ists & HDMI_ISTS_CLK_CHANGE) ||
>> +	    (hdmi_ists & HDMI_ISTS_PLL_LCK_CHG) || md_ists) {
>> +		dw_hdmi_power_off(dw_dev);
>> +		if (has_signal(dw_dev, dw_dev->configured_input))
>> +			dw_hdmi_power_on(dw_dev, dw_dev->configured_input);
>> +	}
>> +	return IRQ_HANDLED;
>> +}
>> +static int dw_hdmi_registered(struct v4l2_subdev *sd)
>> +{
>> +	struct dw_hdmi_dev *dw_dev = to_dw_dev(sd);
>> +	int ret;
>> +
>> +	ret = cec_register_adapter(dw_dev->cec_adap, dw_dev->dev);
>> +	if (ret) {
>> +		dev_err(dw_dev->dev, "failed to register CEC adapter\n");
>> +		cec_delete_adapter(dw_dev->cec_adap);
>> +		return ret;
>> +	}
>> +
>> +	cec_register_cec_notifier(dw_dev->cec_adap, dw_dev->cec_notifier);
>> +	dw_dev->registered = true;
>> +	return ret;
>> +}
>> +
>> +static void dw_hdmi_unregistered(struct v4l2_subdev *sd)
>> +{
>> +	struct dw_hdmi_dev *dw_dev = to_dw_dev(sd);
>> +
>> +	cec_unregister_adapter(dw_dev->cec_adap);
>> +	cec_notifier_put(dw_dev->cec_notifier);
>> +}
>> +
>> +static const struct v4l2_subdev_core_ops dw_hdmi_sd_core_ops = {
>> +	.log_status = dw_hdmi_log_status,
>> +	.subscribe_event = dw_hdmi_subscribe_event,
>> +};
>> +
>> +static const struct v4l2_subdev_video_ops dw_hdmi_sd_video_ops = {
>> +	.s_routing = dw_hdmi_s_routing,
>> +	.g_input_status = dw_hdmi_g_input_status,
>> +	.g_parm = dw_hdmi_g_parm,
>> +	.g_dv_timings = dw_hdmi_g_dv_timings,
>> +	.query_dv_timings = dw_hdmi_query_dv_timings,
>> +};
>> +
>> +static const struct v4l2_subdev_pad_ops dw_hdmi_sd_pad_ops = {
>> +	.enum_mbus_code = dw_hdmi_enum_mbus_code,
>> +	.get_fmt = dw_hdmi_get_fmt,
>> +	.set_fmt = dw_hdmi_set_fmt,
>> +	.dv_timings_cap = dw_hdmi_dv_timings_cap,
>> +	.enum_dv_timings = dw_hdmi_enum_dv_timings,
>> +};
>> +
>> +static const struct v4l2_subdev_ops dw_hdmi_sd_ops = {
>> +	.core = &dw_hdmi_sd_core_ops,
>> +	.video = &dw_hdmi_sd_video_ops,
>> +	.pad = &dw_hdmi_sd_pad_ops,
>> +};
>> +
>> +static const struct v4l2_subdev_internal_ops dw_hdmi_internal_ops = {
>> +	.registered = dw_hdmi_registered,
>> +	.unregistered = dw_hdmi_unregistered,
>> +};
>> +
>> +static int dw_hdmi_parse_dt(struct dw_hdmi_dev *dw_dev)
>> +{
>> +	struct device_node *notifier, *np = dw_dev->of_node;
>> +	struct dw_phy_pdata *phy = &dw_dev->phy_config;
>> +
>> +	if (!np) {
>> +		dev_err(dw_dev->dev, "missing DT node\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* PHY properties parsing */
>> +	of_property_read_u8(np, "snps,hdmi-phy-jtag-addr",
>> +			&dw_dev->phy_jtag_addr);
>> +	if (!dw_dev->phy_jtag_addr) {
>> +		dev_err(dw_dev->dev, "missing hdmi-phy-jtag-addr in DT\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	of_property_read_u32(np, "snps,hdmi-phy-version", &phy->version);
>> +	if (!phy->version) {
>> +		dev_err(dw_dev->dev, "missing hdmi-phy-version in DT\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	of_property_read_u32(np, "snps,hdmi-phy-cfg-clk", &phy->cfg_clk);
>> +	if (!phy->cfg_clk) {
>> +		dev_err(dw_dev->dev, "missing hdmi-phy-cfg-clk in DT\n");
>> +		return -EINVAL;
>> +	}
> With changes as proposed in comments to patch "4/4 dt-bindings: ..." 
> you could use the common clk API for retrieving the clock rate, e.g. 
> devm_clk_get(), clk_get_rate().
>
> When the HDMI RX IP block gets integrated within some SoC I'd expect 
> the system clock controller to be already using the common clk DT 
> bindings. Unless for some reason the platform doesn't support CCF.

Yes, I will change.

>
>
>> +	if (of_property_read_string_index(np, "snps,hdmi-phy-driver", 0,
>> +				&dw_dev->phy_drv) < 0) {
>> +		dev_err(dw_dev->dev, "missing hdmi-phy-driver in DT\n");
> I don't think we can put Linux driver names in DT like this, it seems rather 
> a serious abuse.  With proposed changes to the DT binding you could reference 
> the PHY device by DT phandle or child node.
>
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Controller properties parsing */
>> +	of_property_read_u32(np, "snps,hdmi-ctl-cfg-clk", &dw_dev->cfg_clk);
>> +	if (!dw_dev->cfg_clk) {
>> +		dev_err(dw_dev->dev, "missing hdmi-ctl-cfg-clk in DT\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +#if IS_ENABLED(CONFIG_VIDEO_DWC_HDMI_RX_CEC)
>> +	/* Notifier device parsing */
>> +	notifier = of_parse_phandle(np, "edid-phandle", 0);
>> +	if (!notifier) {
>> +		dev_err(dw_dev->dev, "missing edid-phandle in DT\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	dw_dev->notifier_pdev = of_find_device_by_node(np);
> Shouldn't this be:
> 	dw_dev->notifier_pdev = of_find_device_by_node(notifier);
> ?
>
> The caller of dw_hdmi_parse_dt() already knows about the device 
> associated with np.

Yeah, its a typo, it works by luck because in my setup np ==
notifier.

>
>> +	if (!dw_dev->notifier_pdev)
>> +		return -EPROBE_DEFER;
>> +#endif
>> +
>> +	return 0;
>> +}
>> +
>> +static int dw_hdmi_rx_probe(struct platform_device *pdev)
>> +{
>> +	/* Deferred work */
>> +	dw_dev->wq = create_workqueue(DW_HDMI_RX_DRVNAME);
> Have you considered using create_singlethread_workqueue() ? create_workqueue() 
> will spawn one thread per CPU.

Ok, will change.

>
>> +	if (!dw_dev->wq)
>> +		return -ENOMEM;
>> +
>> +	/* Registers mapping */
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		ret = -ENXIO;
>> +		goto err_wq;
>> +	}
> You can drop res testing here, devm_ioremap_resource() verifies internally 
> if res is valid and returns proper error code.

Ok.

>
>> +	dw_dev->regs = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(dw_dev->regs)) {
>> +		ret = PTR_ERR(dw_dev->regs);
>> +		goto err_wq;
>> +	}
>
>> +	/* V4L2 initialization */
>> +	sd = &dw_dev->sd;
>> +	v4l2_subdev_init(sd, &dw_hdmi_sd_ops);
>> +	strlcpy(sd->name, DW_HDMI_RX_DRVNAME, sizeof(sd->name));
> sd->name should be unique, you could, for instance, do something like
>
> 	strlcpy(sd->name, dev_name(&pdev->dev), sizeof(sd->name));

Ok.

>
>> +	sd->internal_ops = &dw_hdmi_internal_ops;
>> +	sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS;
>> +}
>> +
>> +static int dw_hdmi_rx_remove(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> +	struct dw_hdmi_dev *dw_dev = to_dw_dev(sd);
>> +
>> +	dev_dbg(dev, "%s\n", __func__);
>> +
>> +	dw_hdmi_disable_ints(dw_dev);
>> +	dw_hdmi_disable_hpd(dw_dev);
>> +	dw_hdmi_disable_scdc(dw_dev);
>> +	dw_hdmi_power_off(dw_dev);
>> +	dw_hdmi_phy_s_power(dw_dev, false);
>> +	flush_workqueue(dw_dev->wq);
>> +	destroy_workqueue(dw_dev->wq);
>> +	v4l2_device_unregister(&dw_dev->v4l2_dev);
>> +	dw_hdmi_phy_exit(dw_dev);> +	dev_info(dev, "driver removed\n");
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver dw_hdmi_rx_driver = {
>> +	.probe = dw_hdmi_rx_probe,
>> +	.remove = dw_hdmi_rx_remove,
> I think we need also .of_match_table here.
>
>> +	.driver = {
>> +		.name = DW_HDMI_RX_DRVNAME,
>> +	}
>> +};
>> +module_platform_driver(dw_hdmi_rx_driver);
>> +#endif /* __DW_HDMI_RX_H__ */
>> diff --git a/include/media/dwc/dw-hdmi-rx-pdata.h b/include/media/dwc/dw-hdmi-rx-pdata.h
>> new file mode 100644
>> index 0000000..ff8554d
>> --- /dev/null
>> +++ b/include/media/dwc/dw-hdmi-rx-pdata.h
>> @@ -0,0 +1,63 @@
>> +#ifndef __DW_HDMI_RX_PDATA_H__
>> +#define __DW_HDMI_RX_PDATA_H__
>> +
>> +#define DW_HDMI_RX_DRVNAME			"dw-hdmi-rx"
>> +
>> +/* Notify events */
>> +#define DW_HDMI_NOTIFY_IS_OFF		1
>> +#define DW_HDMI_NOTIFY_INPUT_CHANGED	2
>> +#define DW_HDMI_NOTIFY_AUDIO_CHANGED	3
>> +#define DW_HDMI_NOTIFY_IS_STABLE	4
>> +
>> +/* HDCP 1.4 */
>> +#define DW_HDMI_HDCP14_BKSV_SIZE	2
>> +#define DW_HDMI_HDCP14_KEYS_SIZE	(2 * 40)
>> +
>> +struct dw_hdmi_hdcp14_key {
>> +	u32 seed;
>> +	u32 bksv[DW_HDMI_HDCP14_BKSV_SIZE];
>> +	u32 keys[DW_HDMI_HDCP14_KEYS_SIZE];
>> +	bool keys_valid;
>> +};
>> +
>> +struct dw_hdmi_rx_pdata {
>> +	/* Controller configuration */
>> +	unsigned int iref_clk; /* MHz */
> Is this field unused?

Yes, left overs from previous versions.

>
>> +	struct dw_hdmi_hdcp14_key hdcp14_keys;
>> +	/* 5V sense interface */
>> +	bool (*dw_5v_status)(void __iomem *regs, int input);
>> +	void (*dw_5v_clear)(void __iomem *regs);
>> +	void __iomem *dw_5v_arg;> +	/* Zcal interface */
>> +	void (*dw_zcal_reset)(void __iomem *regs);
>> +	bool (*dw_zcal_done)(void __iomem *regs);
>> +	void __iomem *dw_zcal_arg;
> I'm just wondering if these operations could be modeled with the regmap,
> so we could avoid callbacks in the platform data structure.

Hmm, I don't think that is safe because registers may not be
adjacent to each other. And maybe I was a little generous in
passing a __iomem argument, maybe it should be just void instead
because this can be not a regmap at all.

Best regards,
Jose Miguel Abreu

>  
>> +};
>> +#endif /* __DW_HDMI_RX_PDATA_H__ */
> --
> Regards,
> Sylwester

[toc] | [prev] | [next] | [standalone]

#1670004 — Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver

FromSylwester Nawrocki <snawrocki@kernel.org>
Date2017-06-20 00:20 +0200
SubjectRe: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver
Message-ID<tUdbB-7o-21@gated-at.bofh.it>
In reply to#1668944
On 06/19/2017 11:33 AM, Jose Abreu wrote:
> On 18-06-2017 19:04, Sylwester Nawrocki wrote:
>> On 06/16/2017 06:38 PM, Jose Abreu wrote:
>>> This is an initial submission for the Synopsys Designware HDMI RX
>>> Controller Driver. This driver interacts with a phy driver so that
>>> a communication between them is created and a video pipeline is
>>> configured.
>>>
>>> The controller + phy pipeline can then be integrated into a fully
>>> featured system that can be able to receive video up to 4k@60Hz
>>> with deep color 48bit RGB, depending on the platform. Although,
>>> this initial version does not yet handle deep color modes.
>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>>>
>>> +static int dw_hdmi_phy_init(struct dw_hdmi_dev *dw_dev)
>>> +{

>>> +	request_module(pdevinfo.name);
>>> +
>>> +	dw_dev->phy_pdev = platform_device_register_full(&pdevinfo);
>>> +	if (IS_ERR(dw_dev->phy_pdev)) {
>>> +		dev_err(dw_dev->dev, "failed to register phy device\n");
>>> +		return PTR_ERR(dw_dev->phy_pdev);
>>> +	}
>>> +
>>> +	if (!dw_dev->phy_pdev->dev.driver) {
>>> +		dev_err(dw_dev->dev, "failed to initialize phy driver\n");
>>> +		goto err;
>>> +	}
>> I think this is not safe because there is nothing preventing unbinding
>> or unloading the driver at this point.
>>
>>> +	if (!try_module_get(dw_dev->phy_pdev->dev.driver->owner)) {
>> So dw_dev->phy_pdev->dev.driver may be already NULL here.
> 
> How can I make sure it wont be NULL? Because I've seen other
> media drivers do this and I don't think they do any kind of
> locking, but they do this mainly for I2C subdevs.

You could do device_lock(dev)/device_unlock(dev) to avoid possible races. 
And setting 'suppress_bind_attrs' field in the sub-device drivers would 
disable sysfs unbind attributes, so sub-device driver wouldn't get unbound
unexpectedly trough sysfs.
 
>>> +		dev_err(dw_dev->dev, "failed to get phy module\n");
>>> +		goto err;
>>> +	}
>>> +
>>> +	dw_dev->phy_sd = dev_get_drvdata(&dw_dev->phy_pdev->dev);
>>> +	if (!dw_dev->phy_sd) {
>>> +		dev_err(dw_dev->dev, "failed to get phy subdev\n");
>>> +		goto err_put;
>>> +	}
>>> +
>>> +	if (v4l2_device_register_subdev(&dw_dev->v4l2_dev, dw_dev->phy_sd)) {
>>> +		dev_err(dw_dev->dev, "failed to register phy subdev\n");
>>> +		goto err_put;
>>> +	}
>>
>> I'd suggest usign v4l2-async API, so we use a common pattern for sub-device
>> registration.  And with recent change [1] you could handle this PHY subdev
>> in a standard way.  That might be more complicated than it is now but should
>> make any future platform integration easier.

> So I will instantiate phy driver and then wait for phy driver to
> register into v4l2 core?

Yes, for instance the RX controller driver registers a notifier, instantiates
the child PHY device and then waits until the PHY driver completes initialization.

>>> +	module_put(dw_dev->phy_pdev->dev.driver->owner);
>>> +	return 0;
>>> +
>>> +err_put:
>>> +	module_put(dw_dev->phy_pdev->dev.driver->owner);
>>> +err:
>>> +	platform_device_unregister(dw_dev->phy_pdev);
>>> +	return -EINVAL;
>>> +}

>>> +static int dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
>>> +{
>>> +	struct dw_hdmi_work_data *data;
>>> +	unsigned long flags;
>>> +
>>> +	data = devm_kzalloc(dw_dev->dev, sizeof(*data), GFP_KERNEL);
>>
>> Why use devm_{kzalloc, kfree} when dw_hdmi_power_on() is not only called
>> in the device's probe() callback, but in other places, including interrupt
>> handler?  devm_* API is normally used when life time of a resource is more
>> or less equal to life time of struct device or its matched driver.  Were
>> there any specific reasons to not just use kzalloc()/kfree() ?
> 
> No specific reason, I just thought it would be safer because if I
> cancel a work before it started then memory will remain
> allocated. But I will change to kzalloc().

OK, I overlooked such situation. Since you allow one job queued maybe
just embed struct work_struct in struct dw_hdmi_dev and retrieve it with
container_of() macro in the work handler and use additional field in
struct dw_hdmi_dev protected with dw_dev->lock for passing the input 
index?

>>> +	if (!data)
>>> +		return -ENOMEM;
>>> +
>>> +	INIT_WORK(&data->work, dw_hdmi_work_handler);
>>> +	data->dw_dev = dw_dev;
>>> +	data->input = input;
>>> +
>>> +	spin_lock_irqsave(&dw_dev->lock, flags);
>>> +	if (dw_dev->pending_config) {
>>> +		devm_kfree(dw_dev->dev, data);
>>> +		spin_unlock_irqrestore(&dw_dev->lock, flags);
>>> +		return 0;
>>> +	}
>>> +
>>> +	queue_work(dw_dev->wq, &data->work);
>>> +	dw_dev->pending_config = true;
>>> +	spin_unlock_irqrestore(&dw_dev->lock, flags);
>>> +	return 0;
>>> +}

>>> +struct dw_hdmi_rx_pdata {
>>> +	/* Controller configuration */

>>> +	struct dw_hdmi_hdcp14_key hdcp14_keys;
>>> +	/* 5V sense interface */
>>> +	bool (*dw_5v_status)(void __iomem *regs, int input);
>>> +	void (*dw_5v_clear)(void __iomem *regs);
>>> +	void __iomem *dw_5v_arg;> +	/* Zcal interface */
>>> +	void (*dw_zcal_reset)(void __iomem *regs);
>>> +	bool (*dw_zcal_done)(void __iomem *regs);
>>> +	void __iomem *dw_zcal_arg;
>>
>> I'm just wondering if these operations could be modeled with the regmap,
>> so we could avoid callbacks in the platform data structure.
> 
> Hmm, I don't think that is safe because registers may not be
> adjacent to each other. And maybe I was a little generous in
> passing a __iomem argument, maybe it should be just void instead
> because this can be not a regmap at all.

I meant two separate regmaps, but it's not that good anyway, since
register address and register bit fields not specific to the HDMI RX
block would need to be handled in this driver.

--
Regards,
Sylwester

[toc] | [prev] | [next] | [standalone]

#1670639 — Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver

FromJose Abreu <Jose.Abreu@synopsys.com>
Date2017-06-20 12:20 +0200
SubjectRe: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver
Message-ID<tUoqm-7jN-15@gated-at.bofh.it>
In reply to#1670004
HI Sylwester,


On 19-06-2017 23:10, Sylwester Nawrocki wrote:
> On 06/19/2017 11:33 AM, Jose Abreu wrote:
>> On 18-06-2017 19:04, Sylwester Nawrocki wrote:
>>> On 06/16/2017 06:38 PM, Jose Abreu wrote:
>>>> This is an initial submission for the Synopsys Designware HDMI RX
>>>> Controller Driver. This driver interacts with a phy driver so that
>>>> a communication between them is created and a video pipeline is
>>>> configured.
>>>>
>>>> The controller + phy pipeline can then be integrated into a fully
>>>> featured system that can be able to receive video up to 4k@60Hz
>>>> with deep color 48bit RGB, depending on the platform. Although,
>>>> this initial version does not yet handle deep color modes.
>>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>>>>
>>>> +static int dw_hdmi_phy_init(struct dw_hdmi_dev *dw_dev)
>>>> +{
>>>> +	request_module(pdevinfo.name);
>>>> +
>>>> +	dw_dev->phy_pdev = platform_device_register_full(&pdevinfo);
>>>> +	if (IS_ERR(dw_dev->phy_pdev)) {
>>>> +		dev_err(dw_dev->dev, "failed to register phy device\n");
>>>> +		return PTR_ERR(dw_dev->phy_pdev);
>>>> +	}
>>>> +
>>>> +	if (!dw_dev->phy_pdev->dev.driver) {
>>>> +		dev_err(dw_dev->dev, "failed to initialize phy driver\n");
>>>> +		goto err;
>>>> +	}
>>> I think this is not safe because there is nothing preventing unbinding
>>> or unloading the driver at this point.
>>>
>>>> +	if (!try_module_get(dw_dev->phy_pdev->dev.driver->owner)) {
>>> So dw_dev->phy_pdev->dev.driver may be already NULL here.
>> How can I make sure it wont be NULL? Because I've seen other
>> media drivers do this and I don't think they do any kind of
>> locking, but they do this mainly for I2C subdevs.
> You could do device_lock(dev)/device_unlock(dev) to avoid possible races. 
> And setting 'suppress_bind_attrs' field in the sub-device drivers would 
> disable sysfs unbind attributes, so sub-device driver wouldn't get unbound
> unexpectedly trough sysfs.

Hmm, ok. I changed this, now I'm using driver_find() +
driver_for_each_device(). Its working but I'm starting to think
about whether this should go into v4l2 core because I've seen
other drivers also do this.

>  
>>>> +		dev_err(dw_dev->dev, "failed to get phy module\n");
>>>> +		goto err;
>>>> +	}
>>>> +
>>>> +	dw_dev->phy_sd = dev_get_drvdata(&dw_dev->phy_pdev->dev);
>>>> +	if (!dw_dev->phy_sd) {
>>>> +		dev_err(dw_dev->dev, "failed to get phy subdev\n");
>>>> +		goto err_put;
>>>> +	}
>>>> +
>>>> +	if (v4l2_device_register_subdev(&dw_dev->v4l2_dev, dw_dev->phy_sd)) {
>>>> +		dev_err(dw_dev->dev, "failed to register phy subdev\n");
>>>> +		goto err_put;
>>>> +	}
>>> I'd suggest usign v4l2-async API, so we use a common pattern for sub-device
>>> registration.  And with recent change [1] you could handle this PHY subdev
>>> in a standard way.  That might be more complicated than it is now but should
>>> make any future platform integration easier.
>> So I will instantiate phy driver and then wait for phy driver to
>> register into v4l2 core?
> Yes, for instance the RX controller driver registers a notifier, instantiates
> the child PHY device and then waits until the PHY driver completes initialization.
>
>>>> +	module_put(dw_dev->phy_pdev->dev.driver->owner);
>>>> +	return 0;
>>>> +
>>>> +err_put:
>>>> +	module_put(dw_dev->phy_pdev->dev.driver->owner);
>>>> +err:
>>>> +	platform_device_unregister(dw_dev->phy_pdev);
>>>> +	return -EINVAL;
>>>> +}
>>>> +static int dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
>>>> +{
>>>> +	struct dw_hdmi_work_data *data;
>>>> +	unsigned long flags;
>>>> +
>>>> +	data = devm_kzalloc(dw_dev->dev, sizeof(*data), GFP_KERNEL);
>>> Why use devm_{kzalloc, kfree} when dw_hdmi_power_on() is not only called
>>> in the device's probe() callback, but in other places, including interrupt
>>> handler?  devm_* API is normally used when life time of a resource is more
>>> or less equal to life time of struct device or its matched driver.  Were
>>> there any specific reasons to not just use kzalloc()/kfree() ?
>> No specific reason, I just thought it would be safer because if I
>> cancel a work before it started then memory will remain
>> allocated. But I will change to kzalloc().
> OK, I overlooked such situation. Since you allow one job queued maybe
> just embed struct work_struct in struct dw_hdmi_dev and retrieve it with
> container_of() macro in the work handler and use additional field in
> struct dw_hdmi_dev protected with dw_dev->lock for passing the input 
> index?

Yes, seems ok. As I'm already locking before queuing work and
also checking if theres is a pending work I can just overwrite
configured_input earlier.

Best regards,
Jose Miguel Abreu


>
>>>> +	if (!data)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	INIT_WORK(&data->work, dw_hdmi_work_handler);
>>>> +	data->dw_dev = dw_dev;
>>>> +	data->input = input;
>>>> +
>>>> +	spin_lock_irqsave(&dw_dev->lock, flags);
>>>> +	if (dw_dev->pending_config) {
>>>> +		devm_kfree(dw_dev->dev, data);
>>>> +		spin_unlock_irqrestore(&dw_dev->lock, flags);
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	queue_work(dw_dev->wq, &data->work);
>>>> +	dw_dev->pending_config = true;
>>>> +	spin_unlock_irqrestore(&dw_dev->lock, flags);
>>>> +	return 0;
>>>> +}
>>>> +struct dw_hdmi_rx_pdata {
>>>> +	/* Controller configuration */
>>>> +	struct dw_hdmi_hdcp14_key hdcp14_keys;
>>>> +	/* 5V sense interface */
>>>> +	bool (*dw_5v_status)(void __iomem *regs, int input);
>>>> +	void (*dw_5v_clear)(void __iomem *regs);
>>>> +	void __iomem *dw_5v_arg;> +	/* Zcal interface */
>>>> +	void (*dw_zcal_reset)(void __iomem *regs);
>>>> +	bool (*dw_zcal_done)(void __iomem *regs);
>>>> +	void __iomem *dw_zcal_arg;
>>> I'm just wondering if these operations could be modeled with the regmap,
>>> so we could avoid callbacks in the platform data structure.
>> Hmm, I don't think that is safe because registers may not be
>> adjacent to each other. And maybe I was a little generous in
>> passing a __iomem argument, maybe it should be just void instead
>> because this can be not a regmap at all.
> I meant two separate regmaps, but it's not that good anyway, since
> register address and register bit fields not specific to the HDMI RX
> block would need to be handled in this driver.
>
> --
> Regards,
> Sylwester

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web