[PATCH v2 00/15] tc358767 driver improvements

[PATCH v2 00/15] tc358767 driver improvements

[Andrey Smirnov]

Everyone:

This series contains various improvements (at least in my mind) and
fixes that I made to tc358767 while working with the code of the
driver. Hopefuly each patch is self explanatory.

Feedback is welcome!

Thanks,
Andrey Smirnov

Changes since [v1]:

    - Patchset rebased on top of
      https://patchwork.freedesktop.org/series/58176/
      
    - Patches to remove both tc_write() and tc_read() helpers added

    - Patches to rework AUX transfer code added

    - Both "drm/bridge: tc358767: Simplify polling in
      tc_main_link_setup()" and "drm/bridge: tc358767: Simplify
      polling in tc_link_training()" changed to use tc_poll_timeout()
      instead of regmap_read_poll_timeout()

[v1] lkml.kernel.org/r/20190226193609.9862-1-andrew.smirnov@gmail.com

Andrey Smirnov (15):
  drm/bridge: tc358767: Simplify tc_poll_timeout()
  drm/bridge: tc358767: Simplify polling in tc_main_link_setup()
  drm/bridge: tc358767: Simplify polling in tc_link_training()
  drm/bridge: tc358767: Simplify tc_set_video_mode()
  drm/bridge: tc358767: Drop custom tc_write()/tc_read() accessors
  drm/bridge: tc358767: Simplify AUX data read
  drm/bridge: tc358767: Simplify AUX data write
  drm/bridge: tc358767: Increase AUX transfer length limit
  drm/bridge: tc358767: Use reported AUX transfer size
  drm/bridge: tc358767: Add support for address-only I2C transfers
  drm/bridge: tc358767: Introduce tc_set_syspllparam()
  drm/bridge: tc358767: Introduce tc_pllupdate_pllen()
  drm/bridge: tc358767: Simplify tc_aux_wait_busy()
  drm/bridge: tc358767: Drop unnecessary 8 byte buffer
  drm/bridge: tc358767: Replace magic number in tc_main_link_enable()

 drivers/gpu/drm/bridge/tc358767.c | 648 ++++++++++++++++--------------
 1 file changed, 349 insertions(+), 299 deletions(-)

-- 
2.20.1

[PATCH v2 01/15] drm/bridge: tc358767: Simplify tc_poll_timeout()

[Andrey Smirnov]

Implementation of tc_poll_timeout() is almost a 100% copy-and-paste of
the code for regmap_read_poll_timeout(). Replace copied code with a
call to the original. While at it change tc_poll_timeout to accept
"struct tc_data *" instead of "struct regmap *" for brevity. No
functional change intended.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/tc358767.c | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 51095ab1e996..89a4392dfa35 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -250,34 +250,21 @@ static inline struct tc_data *connector_to_tc(struct drm_connector *c)
 			goto err;				\
 	} while (0)
 
-static inline int tc_poll_timeout(struct regmap *map, unsigned int addr,
+static inline int tc_poll_timeout(struct tc_data *tc, unsigned int addr,
 				  unsigned int cond_mask,
 				  unsigned int cond_value,
 				  unsigned long sleep_us, u64 timeout_us)
 {
-	ktime_t timeout = ktime_add_us(ktime_get(), timeout_us);
 	unsigned int val;
-	int ret;
 
-	for (;;) {
-		ret = regmap_read(map, addr, &val);
-		if (ret)
-			break;
-		if ((val & cond_mask) == cond_value)
-			break;
-		if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) {
-			ret = regmap_read(map, addr, &val);
-			break;
-		}
-		if (sleep_us)
-			usleep_range((sleep_us >> 2) + 1, sleep_us);
-	}
-	return ret ?: (((val & cond_mask) == cond_value) ? 0 : -ETIMEDOUT);
+	return regmap_read_poll_timeout(tc->regmap, addr, val,
+					(val & cond_mask) == cond_value,
+					sleep_us, timeout_us);
 }
 
 static int tc_aux_wait_busy(struct tc_data *tc, unsigned int timeout_ms)
 {
-	return tc_poll_timeout(tc->regmap, DP0_AUXSTATUS, AUX_BUSY, 0,
+	return tc_poll_timeout(tc, DP0_AUXSTATUS, AUX_BUSY, 0,
 			       1000, 1000 * timeout_ms);
 }
 
@@ -584,8 +571,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
 	tc_write(DP1_PLLCTRL, PLLUPDATE | PLLEN);
 	tc_wait_pll_lock(tc);
 
-	ret = tc_poll_timeout(tc->regmap, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1,
-			      1000);
+	ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1, 1000);
 	if (ret == -ETIMEDOUT) {
 		dev_err(tc->dev, "Timeout waiting for PHY to become ready");
 		return ret;
-- 
2.20.1

Re: [PATCH v2 01/15] drm/bridge: tc358767: Simplify tc_poll_timeout()

[Tomi Valkeinen]

On 22/03/2019 05:28, Andrey Smirnov wrote:
> Implementation of tc_poll_timeout() is almost a 100% copy-and-paste of
> the code for regmap_read_poll_timeout(). Replace copied code with a
> call to the original. While at it change tc_poll_timeout to accept
> "struct tc_data *" instead of "struct regmap *" for brevity. No
> functional change intended.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 26 ++++++--------------------
>  1 file changed, 6 insertions(+), 20 deletions(-)

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH v2 02/15] drm/bridge: tc358767: Simplify polling in tc_main_link_setup()

[Andrey Smirnov]

Replace explicit polling loop with equivalent call to
tc_poll_timeout() for brevity. No functional change intended.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/tc358767.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 89a4392dfa35..2531f4dadbf8 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -761,7 +761,6 @@ static int tc_main_link_enable(struct tc_data *tc)
 	struct device *dev = tc->dev;
 	unsigned int rate;
 	u32 dp_phy_ctrl;
-	int timeout;
 	u32 value;
 	int ret;
 	u8 tmp[8];
@@ -817,15 +816,11 @@ static int tc_main_link_enable(struct tc_data *tc)
 	dp_phy_ctrl &= ~(DP_PHY_RST | PHY_M1_RST | PHY_M0_RST);
 	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
 
-	timeout = 1000;
-	do {
-		tc_read(DP_PHY_CTRL, &value);
-		udelay(1);
-	} while ((!(value & PHY_RDY)) && (--timeout));
-
-	if (timeout == 0) {
-		dev_err(dev, "timeout waiting for phy become ready");
-		return -ETIMEDOUT;
+	ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1, 1000);
+	if (ret) {
+		if (ret == -ETIMEDOUT)
+			dev_err(dev, "timeout waiting for phy become ready");
+		return ret;
 	}
 
 	/* Set misc: 8 bits per color */
-- 
2.20.1

Re: [PATCH v2 02/15] drm/bridge: tc358767: Simplify polling in tc_main_link_setup()

[Tomi Valkeinen]

On 22/03/2019 05:28, Andrey Smirnov wrote:
> Replace explicit polling loop with equivalent call to
> tc_poll_timeout() for brevity. No functional change intended.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH v2 03/15] drm/bridge: tc358767: Simplify polling in tc_link_training()

[Andrey Smirnov]

Replace explicit polling in tc_link_training() with equivalent call to
tc_poll_timeout() for simplicity. No functional change intended (not
including slightly altered debug output).

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/tc358767.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 2531f4dadbf8..38d542f553cd 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -733,21 +733,18 @@ static int tc_set_video_mode(struct tc_data *tc, struct drm_display_mode *mode)
 
 static int tc_wait_link_training(struct tc_data *tc, u32 *error)
 {
-	u32 timeout = 1000;
 	u32 value;
 	int ret;
 
-	do {
-		udelay(1);
-		tc_read(DP0_LTSTAT, &value);
-	} while ((!(value & LT_LOOPDONE)) && (--timeout));
-
-	if (timeout == 0) {
+	ret = tc_poll_timeout(tc, DP0_LTSTAT, LT_LOOPDONE,
+			      LT_LOOPDONE, 1, 1000);
+	if (ret) {
 		dev_err(tc->dev, "Link training timeout waiting for LT_LOOPDONE!\n");
-		ret = -ETIMEDOUT;
 		goto err;
 	}
 
+	tc_read(DP0_LTSTAT, &value);
+
 	*error = (value >> 8) & 0x7;
 
 	return 0;
-- 
2.20.1

Re: [PATCH v2 03/15] drm/bridge: tc358767: Simplify polling in tc_link_training()

[Tomi Valkeinen]

On 22/03/2019 05:28, Andrey Smirnov wrote:
> Replace explicit polling in tc_link_training() with equivalent call to
> tc_poll_timeout() for simplicity. No functional change intended (not
> including slightly altered debug output).
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH v2 04/15] drm/bridge: tc358767: Simplify tc_set_video_mode()

[Andrey Smirnov]

Simplify tc_set_video_mode() by replacing repreated calls to
tc_write()/regmap_write() with a single call to
regmap_multi_reg_write(). While at it, simplify explicit shifting by
using macros from <linux/bitfield.h>. No functional change intended.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/tc358767.c | 146 +++++++++++++++++-------------
 1 file changed, 85 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 38d542f553cd..d99c9f32a133 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -24,6 +24,7 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/gpio/consumer.h>
@@ -56,6 +57,7 @@
 
 /* Video Path */
 #define VPCTRL0			0x0450
+#define VSDELAY			GENMASK(31, 20)
 #define OPXLFMT_RGB666			(0 << 8)
 #define OPXLFMT_RGB888			(1 << 8)
 #define FRMSYNC_DISABLED		(0 << 4) /* Video Timing Gen Disabled */
@@ -63,9 +65,17 @@
 #define MSF_DISABLED			(0 << 0) /* Magic Square FRC disabled */
 #define MSF_ENABLED			(1 << 0) /* Magic Square FRC enabled */
 #define HTIM01			0x0454
+#define HPW			GENMASK(8, 0)
+#define HBPR			GENMASK(24, 16)
 #define HTIM02			0x0458
+#define HDISPR			GENMASK(10, 0)
+#define HFPR			GENMASK(24, 16)
 #define VTIM01			0x045c
+#define VSPR			GENMASK(7, 0)
+#define VBPR			GENMASK(23, 16)
 #define VTIM02			0x0460
+#define VFPR			GENMASK(23, 16)
+#define VDISPR			GENMASK(10, 0)
 #define VFUEN0			0x0464
 #define VFUEN				BIT(0)   /* Video Frame Timing Upload */
 
@@ -100,14 +110,28 @@
 /* Main Channel */
 #define DP0_SECSAMPLE		0x0640
 #define DP0_VIDSYNCDELAY	0x0644
+#define VID_SYNC_DLY		GENMASK(15, 0)
+#define THRESH_DLY		GENMASK(31, 16)
+
 #define DP0_TOTALVAL		0x0648
+#define H_TOTAL			GENMASK(15, 0)
+#define V_TOTAL			GENMASK(31, 16)
 #define DP0_STARTVAL		0x064c
+#define H_START			GENMASK(15, 0)
+#define V_START			GENMASK(31, 16)
 #define DP0_ACTIVEVAL		0x0650
+#define H_ACT			GENMASK(15, 0)
+#define V_ACT			GENMASK(31, 16)
+
 #define DP0_SYNCVAL		0x0654
+#define VS_WIDTH		GENMASK(30, 16)
+#define HS_WIDTH		GENMASK(14, 0)
 #define SYNCVAL_HS_POL_ACTIVE_LOW	(1 << 15)
 #define SYNCVAL_VS_POL_ACTIVE_LOW	(1 << 31)
 #define DP0_MISC		0x0658
 #define TU_SIZE_RECOMMENDED		(63) /* LSCLK cycles per TU */
+#define MAX_TU_SYMBOL		GENMASK(28, 23)
+#define TU_SIZE			GENMASK(21, 16)
 #define BPC_6				(0 << 5)
 #define BPC_8				(1 << 5)
 
@@ -184,6 +208,12 @@
 
 /* Test & Debug */
 #define TSTCTL			0x0a00
+#define COLOR_R			GENMASK(31, 24)
+#define COLOR_G			GENMASK(23, 16)
+#define COLOR_B			GENMASK(15, 8)
+#define ENI2CFILTER		BIT(4)
+#define COLOR_BAR_MODE		GENMASK(1, 0)
+#define COLOR_BAR_MODE_BARS	2
 #define PLL_DBG			0x0a04
 
 static bool tc_test_pattern;
@@ -647,10 +677,6 @@ static int tc_get_display_props(struct tc_data *tc)
 
 static int tc_set_video_mode(struct tc_data *tc, struct drm_display_mode *mode)
 {
-	int ret;
-	int vid_sync_dly;
-	int max_tu_symbol;
-
 	int left_margin = mode->htotal - mode->hsync_end;
 	int right_margin = mode->hsync_start - mode->hdisplay;
 	int hsync_len = mode->hsync_end - mode->hsync_start;
@@ -659,76 +685,74 @@ static int tc_set_video_mode(struct tc_data *tc, struct drm_display_mode *mode)
 	int vsync_len = mode->vsync_end - mode->vsync_start;
 
 	/*
-	 * Recommended maximum number of symbols transferred in a transfer unit:
+	 * Recommended maximum number of symbols transferred in a
+	 * transfer unit:
 	 * DIV_ROUND_UP((input active video bandwidth in bytes) * tu_size,
 	 *              (output active video bandwidth in bytes))
 	 * Must be less than tu_size.
 	 */
-	max_tu_symbol = TU_SIZE_RECOMMENDED - 1;
-
-	dev_dbg(tc->dev, "set mode %dx%d\n",
-		mode->hdisplay, mode->vdisplay);
-	dev_dbg(tc->dev, "H margin %d,%d sync %d\n",
-		left_margin, right_margin, hsync_len);
-	dev_dbg(tc->dev, "V margin %d,%d sync %d\n",
-		upper_margin, lower_margin, vsync_len);
-	dev_dbg(tc->dev, "total: %dx%d\n", mode->htotal, mode->vtotal);
-
+	int max_tu_symbol = TU_SIZE_RECOMMENDED - 1;
 
+	/* DP Main Stream Attributes */
+	int vid_sync_dly = hsync_len + left_margin + mode->hdisplay;
 	/*
 	 * LCD Ctl Frame Size
 	 * datasheet is not clear of vsdelay in case of DPI
 	 * assume we do not need any delay when DPI is a source of
 	 * sync signals
 	 */
-	tc_write(VPCTRL0, (0 << 20) /* VSDELAY */ |
-		 OPXLFMT_RGB888 | FRMSYNC_DISABLED | MSF_DISABLED);
-	tc_write(HTIM01, (ALIGN(left_margin, 2) << 16) | /* H back porch */
-			 (ALIGN(hsync_len, 2) << 0));	 /* Hsync */
-	tc_write(HTIM02, (ALIGN(right_margin, 2) << 16) |  /* H front porch */
-			 (ALIGN(mode->hdisplay, 2) << 0)); /* width */
-	tc_write(VTIM01, (upper_margin << 16) |		/* V back porch */
-			 (vsync_len << 0));		/* Vsync */
-	tc_write(VTIM02, (lower_margin << 16) |		/* V front porch */
-			 (mode->vdisplay << 0));	/* height */
-	tc_write(VFUEN0, VFUEN);		/* update settings */
-
-	/* Test pattern settings */
-	tc_write(TSTCTL,
-		 (120 << 24) |	/* Red Color component value */
-		 (20 << 16) |	/* Green Color component value */
-		 (99 << 8) |	/* Blue Color component value */
-		 (1 << 4) |	/* Enable I2C Filter */
-		 (2 << 0) |	/* Color bar Mode */
-		 0);
-
-	/* DP Main Stream Attributes */
-	vid_sync_dly = hsync_len + left_margin + mode->hdisplay;
-	tc_write(DP0_VIDSYNCDELAY,
-		 (max_tu_symbol << 16) |	/* thresh_dly */
-		 (vid_sync_dly << 0));
+	const u32 vs_pol = mode->flags & DRM_MODE_FLAG_NVSYNC ?
+		SYNCVAL_VS_POL_ACTIVE_LOW : 0;
+	const u32 hs_pol = mode->flags & DRM_MODE_FLAG_NHSYNC ?
+		SYNCVAL_HS_POL_ACTIVE_LOW : 0;
+	const struct reg_sequence video_mode_settings[] = {
+		{ VPCTRL0, FIELD_PREP(VSDELAY, 0) |
+			   OPXLFMT_RGB888 | FRMSYNC_DISABLED | MSF_DISABLED },
+		{ HTIM01, FIELD_PREP(HBPR, ALIGN(left_margin, 2)) |
+			  FIELD_PREP(HPW, ALIGN(hsync_len, 2)) },
+		{ HTIM02, FIELD_PREP(HDISPR, ALIGN(mode->hdisplay, 2)) |
+			  FIELD_PREP(HFPR, ALIGN(right_margin, 2)) },
+		{ VTIM01, FIELD_PREP(VBPR, upper_margin) |
+			  FIELD_PREP(VSPR, vsync_len) },
+		{ VTIM02, FIELD_PREP(VFPR, lower_margin) |
+			  FIELD_PREP(VDISPR, mode->vdisplay) },
+		{ VFUEN0, VFUEN },
+		{ TSTCTL, FIELD_PREP(COLOR_R, 120) |
+			  FIELD_PREP(COLOR_G, 20) |
+			  FIELD_PREP(COLOR_B, 99) |
+			  ENI2CFILTER |
+			  FIELD_PREP(COLOR_BAR_MODE, COLOR_BAR_MODE_BARS) },
+		{ DP0_VIDSYNCDELAY, FIELD_PREP(THRESH_DLY, max_tu_symbol) |
+				    FIELD_PREP(VID_SYNC_DLY, vid_sync_dly) },
+		{ DP0_TOTALVAL, FIELD_PREP(H_TOTAL, mode->htotal) |
+				FIELD_PREP(V_TOTAL, mode->vtotal) },
+		{ DP0_STARTVAL, FIELD_PREP(H_START, left_margin + hsync_len) |
+				FIELD_PREP(V_START,
+					   upper_margin + vsync_len) },
+		{ DP0_ACTIVEVAL, FIELD_PREP(V_ACT, mode->vdisplay) |
+				 FIELD_PREP(H_ACT, mode->hdisplay) },
+		{ DP0_SYNCVAL, FIELD_PREP(VS_WIDTH, vsync_len) |
+			       FIELD_PREP(HS_WIDTH, hsync_len) |
+			       hs_pol | vs_pol },
+		{ DPIPXLFMT, VS_POL_ACTIVE_LOW | HS_POL_ACTIVE_LOW |
+			     DE_POL_ACTIVE_HIGH | SUB_CFG_TYPE_CONFIG1 |
+			     DPI_BPP_RGB888 },
+		{ DP0_MISC, FIELD_PREP(MAX_TU_SYMBOL, max_tu_symbol) |
+			    FIELD_PREP(TU_SIZE, TU_SIZE_RECOMMENDED) |
+			    BPC_8 },
+	};
 
-	tc_write(DP0_TOTALVAL, (mode->vtotal << 16) | (mode->htotal));
-
-	tc_write(DP0_STARTVAL,
-		 ((upper_margin + vsync_len) << 16) |
-		 ((left_margin + hsync_len) << 0));
-
-	tc_write(DP0_ACTIVEVAL, (mode->vdisplay << 16) | (mode->hdisplay));
-
-	tc_write(DP0_SYNCVAL, (vsync_len << 16) | (hsync_len << 0) |
-		 ((mode->flags & DRM_MODE_FLAG_NHSYNC) ? SYNCVAL_HS_POL_ACTIVE_LOW : 0) |
-		 ((mode->flags & DRM_MODE_FLAG_NVSYNC) ? SYNCVAL_VS_POL_ACTIVE_LOW : 0));
-
-	tc_write(DPIPXLFMT, VS_POL_ACTIVE_LOW | HS_POL_ACTIVE_LOW |
-		 DE_POL_ACTIVE_HIGH | SUB_CFG_TYPE_CONFIG1 | DPI_BPP_RGB888);
-
-	tc_write(DP0_MISC, (max_tu_symbol << 23) | (TU_SIZE_RECOMMENDED << 16) |
-			   BPC_8);
+	dev_dbg(tc->dev, "set mode %dx%d\n",
+		mode->hdisplay, mode->vdisplay);
+	dev_dbg(tc->dev, "H margin %d,%d sync %d\n",
+		left_margin, right_margin, hsync_len);
+	dev_dbg(tc->dev, "V margin %d,%d sync %d\n",
+		upper_margin, lower_margin, vsync_len);
+	dev_dbg(tc->dev, "total: %dx%d\n", mode->htotal, mode->vtotal);
 
-	return 0;
-err:
-	return ret;
+	return regmap_multi_reg_write(tc->regmap,
+				      video_mode_settings,
+				      ARRAY_SIZE(video_mode_settings));
 }
 
 static int tc_wait_link_training(struct tc_data *tc, u32 *error)
-- 
2.20.1

Re: [PATCH v2 04/15] drm/bridge: tc358767: Simplify tc_set_video_mode()

[Tomi Valkeinen]

On 22/03/2019 05:28, Andrey Smirnov wrote:
> Simplify tc_set_video_mode() by replacing repreated calls to
> tc_write()/regmap_write() with a single call to
> regmap_multi_reg_write(). While at it, simplify explicit shifting by
> using macros from <linux/bitfield.h>. No functional change intended.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 146 +++++++++++++++++-------------
>  1 file changed, 85 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 38d542f553cd..d99c9f32a133 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -24,6 +24,7 @@
>   * GNU General Public License for more details.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/clk.h>
>  #include <linux/device.h>
>  #include <linux/gpio/consumer.h>
> @@ -56,6 +57,7 @@
>  
>  /* Video Path */
>  #define VPCTRL0			0x0450
> +#define VSDELAY			GENMASK(31, 20)
>  #define OPXLFMT_RGB666			(0 << 8)
>  #define OPXLFMT_RGB888			(1 << 8)
>  #define FRMSYNC_DISABLED		(0 << 4) /* Video Timing Gen Disabled */
> @@ -63,9 +65,17 @@
>  #define MSF_DISABLED			(0 << 0) /* Magic Square FRC disabled */
>  #define MSF_ENABLED			(1 << 0) /* Magic Square FRC enabled */
>  #define HTIM01			0x0454
> +#define HPW			GENMASK(8, 0)
> +#define HBPR			GENMASK(24, 16)
>  #define HTIM02			0x0458
> +#define HDISPR			GENMASK(10, 0)
> +#define HFPR			GENMASK(24, 16)
>  #define VTIM01			0x045c
> +#define VSPR			GENMASK(7, 0)
> +#define VBPR			GENMASK(23, 16)
>  #define VTIM02			0x0460
> +#define VFPR			GENMASK(23, 16)
> +#define VDISPR			GENMASK(10, 0)
>  #define VFUEN0			0x0464
>  #define VFUEN				BIT(0)   /* Video Frame Timing Upload */
>  
> @@ -100,14 +110,28 @@
>  /* Main Channel */
>  #define DP0_SECSAMPLE		0x0640
>  #define DP0_VIDSYNCDELAY	0x0644
> +#define VID_SYNC_DLY		GENMASK(15, 0)
> +#define THRESH_DLY		GENMASK(31, 16)
> +
>  #define DP0_TOTALVAL		0x0648
> +#define H_TOTAL			GENMASK(15, 0)
> +#define V_TOTAL			GENMASK(31, 16)
>  #define DP0_STARTVAL		0x064c
> +#define H_START			GENMASK(15, 0)
> +#define V_START			GENMASK(31, 16)
>  #define DP0_ACTIVEVAL		0x0650
> +#define H_ACT			GENMASK(15, 0)
> +#define V_ACT			GENMASK(31, 16)
> +
>  #define DP0_SYNCVAL		0x0654
> +#define VS_WIDTH		GENMASK(30, 16)
> +#define HS_WIDTH		GENMASK(14, 0)
>  #define SYNCVAL_HS_POL_ACTIVE_LOW	(1 << 15)
>  #define SYNCVAL_VS_POL_ACTIVE_LOW	(1 << 31)
>  #define DP0_MISC		0x0658
>  #define TU_SIZE_RECOMMENDED		(63) /* LSCLK cycles per TU */
> +#define MAX_TU_SYMBOL		GENMASK(28, 23)
> +#define TU_SIZE			GENMASK(21, 16)
>  #define BPC_6				(0 << 5)
>  #define BPC_8				(1 << 5)
>  
> @@ -184,6 +208,12 @@
>  
>  /* Test & Debug */
>  #define TSTCTL			0x0a00
> +#define COLOR_R			GENMASK(31, 24)
> +#define COLOR_G			GENMASK(23, 16)
> +#define COLOR_B			GENMASK(15, 8)
> +#define ENI2CFILTER		BIT(4)
> +#define COLOR_BAR_MODE		GENMASK(1, 0)
> +#define COLOR_BAR_MODE_BARS	2
>  #define PLL_DBG			0x0a04
>  
>  static bool tc_test_pattern;
> @@ -647,10 +677,6 @@ static int tc_get_display_props(struct tc_data *tc)
>  
>  static int tc_set_video_mode(struct tc_data *tc, struct drm_display_mode *mode)
>  {
> -	int ret;
> -	int vid_sync_dly;
> -	int max_tu_symbol;
> -
>  	int left_margin = mode->htotal - mode->hsync_end;
>  	int right_margin = mode->hsync_start - mode->hdisplay;
>  	int hsync_len = mode->hsync_end - mode->hsync_start;
> @@ -659,76 +685,74 @@ static int tc_set_video_mode(struct tc_data *tc, struct drm_display_mode *mode)
>  	int vsync_len = mode->vsync_end - mode->vsync_start;
>  
>  	/*
> -	 * Recommended maximum number of symbols transferred in a transfer unit:
> +	 * Recommended maximum number of symbols transferred in a
> +	 * transfer unit:
>  	 * DIV_ROUND_UP((input active video bandwidth in bytes) * tu_size,
>  	 *              (output active video bandwidth in bytes))
>  	 * Must be less than tu_size.
>  	 */
> -	max_tu_symbol = TU_SIZE_RECOMMENDED - 1;
> -
> -	dev_dbg(tc->dev, "set mode %dx%d\n",
> -		mode->hdisplay, mode->vdisplay);
> -	dev_dbg(tc->dev, "H margin %d,%d sync %d\n",
> -		left_margin, right_margin, hsync_len);
> -	dev_dbg(tc->dev, "V margin %d,%d sync %d\n",
> -		upper_margin, lower_margin, vsync_len);
> -	dev_dbg(tc->dev, "total: %dx%d\n", mode->htotal, mode->vtotal);
> -
> +	int max_tu_symbol = TU_SIZE_RECOMMENDED - 1;
>  
> +	/* DP Main Stream Attributes */
> +	int vid_sync_dly = hsync_len + left_margin + mode->hdisplay;
>  	/*
>  	 * LCD Ctl Frame Size
>  	 * datasheet is not clear of vsdelay in case of DPI
>  	 * assume we do not need any delay when DPI is a source of
>  	 * sync signals
>  	 */
> -	tc_write(VPCTRL0, (0 << 20) /* VSDELAY */ |
> -		 OPXLFMT_RGB888 | FRMSYNC_DISABLED | MSF_DISABLED);
> -	tc_write(HTIM01, (ALIGN(left_margin, 2) << 16) | /* H back porch */
> -			 (ALIGN(hsync_len, 2) << 0));	 /* Hsync */
> -	tc_write(HTIM02, (ALIGN(right_margin, 2) << 16) |  /* H front porch */
> -			 (ALIGN(mode->hdisplay, 2) << 0)); /* width */
> -	tc_write(VTIM01, (upper_margin << 16) |		/* V back porch */
> -			 (vsync_len << 0));		/* Vsync */
> -	tc_write(VTIM02, (lower_margin << 16) |		/* V front porch */
> -			 (mode->vdisplay << 0));	/* height */
> -	tc_write(VFUEN0, VFUEN);		/* update settings */
> -
> -	/* Test pattern settings */
> -	tc_write(TSTCTL,
> -		 (120 << 24) |	/* Red Color component value */
> -		 (20 << 16) |	/* Green Color component value */
> -		 (99 << 8) |	/* Blue Color component value */
> -		 (1 << 4) |	/* Enable I2C Filter */
> -		 (2 << 0) |	/* Color bar Mode */
> -		 0);
> -
> -	/* DP Main Stream Attributes */
> -	vid_sync_dly = hsync_len + left_margin + mode->hdisplay;
> -	tc_write(DP0_VIDSYNCDELAY,
> -		 (max_tu_symbol << 16) |	/* thresh_dly */
> -		 (vid_sync_dly << 0));
> +	const u32 vs_pol = mode->flags & DRM_MODE_FLAG_NVSYNC ?
> +		SYNCVAL_VS_POL_ACTIVE_LOW : 0;
> +	const u32 hs_pol = mode->flags & DRM_MODE_FLAG_NHSYNC ?
> +		SYNCVAL_HS_POL_ACTIVE_LOW : 0;
> +	const struct reg_sequence video_mode_settings[] = {
> +		{ VPCTRL0, FIELD_PREP(VSDELAY, 0) |
> +			   OPXLFMT_RGB888 | FRMSYNC_DISABLED | MSF_DISABLED },
> +		{ HTIM01, FIELD_PREP(HBPR, ALIGN(left_margin, 2)) |
> +			  FIELD_PREP(HPW, ALIGN(hsync_len, 2)) },
> +		{ HTIM02, FIELD_PREP(HDISPR, ALIGN(mode->hdisplay, 2)) |
> +			  FIELD_PREP(HFPR, ALIGN(right_margin, 2)) },
> +		{ VTIM01, FIELD_PREP(VBPR, upper_margin) |
> +			  FIELD_PREP(VSPR, vsync_len) },
> +		{ VTIM02, FIELD_PREP(VFPR, lower_margin) |
> +			  FIELD_PREP(VDISPR, mode->vdisplay) },
> +		{ VFUEN0, VFUEN },
> +		{ TSTCTL, FIELD_PREP(COLOR_R, 120) |
> +			  FIELD_PREP(COLOR_G, 20) |
> +			  FIELD_PREP(COLOR_B, 99) |
> +			  ENI2CFILTER |
> +			  FIELD_PREP(COLOR_BAR_MODE, COLOR_BAR_MODE_BARS) },
> +		{ DP0_VIDSYNCDELAY, FIELD_PREP(THRESH_DLY, max_tu_symbol) |
> +				    FIELD_PREP(VID_SYNC_DLY, vid_sync_dly) },
> +		{ DP0_TOTALVAL, FIELD_PREP(H_TOTAL, mode->htotal) |
> +				FIELD_PREP(V_TOTAL, mode->vtotal) },
> +		{ DP0_STARTVAL, FIELD_PREP(H_START, left_margin + hsync_len) |
> +				FIELD_PREP(V_START,
> +					   upper_margin + vsync_len) },
> +		{ DP0_ACTIVEVAL, FIELD_PREP(V_ACT, mode->vdisplay) |
> +				 FIELD_PREP(H_ACT, mode->hdisplay) },
> +		{ DP0_SYNCVAL, FIELD_PREP(VS_WIDTH, vsync_len) |
> +			       FIELD_PREP(HS_WIDTH, hsync_len) |
> +			       hs_pol | vs_pol },
> +		{ DPIPXLFMT, VS_POL_ACTIVE_LOW | HS_POL_ACTIVE_LOW |
> +			     DE_POL_ACTIVE_HIGH | SUB_CFG_TYPE_CONFIG1 |
> +			     DPI_BPP_RGB888 },
> +		{ DP0_MISC, FIELD_PREP(MAX_TU_SYMBOL, max_tu_symbol) |
> +			    FIELD_PREP(TU_SIZE, TU_SIZE_RECOMMENDED) |
> +			    BPC_8 },
> +	};
>  
> -	tc_write(DP0_TOTALVAL, (mode->vtotal << 16) | (mode->htotal));
> -
> -	tc_write(DP0_STARTVAL,
> -		 ((upper_margin + vsync_len) << 16) |
> -		 ((left_margin + hsync_len) << 0));
> -
> -	tc_write(DP0_ACTIVEVAL, (mode->vdisplay << 16) | (mode->hdisplay));
> -
> -	tc_write(DP0_SYNCVAL, (vsync_len << 16) | (hsync_len << 0) |
> -		 ((mode->flags & DRM_MODE_FLAG_NHSYNC) ? SYNCVAL_HS_POL_ACTIVE_LOW : 0) |
> -		 ((mode->flags & DRM_MODE_FLAG_NVSYNC) ? SYNCVAL_VS_POL_ACTIVE_LOW : 0));
> -
> -	tc_write(DPIPXLFMT, VS_POL_ACTIVE_LOW | HS_POL_ACTIVE_LOW |
> -		 DE_POL_ACTIVE_HIGH | SUB_CFG_TYPE_CONFIG1 | DPI_BPP_RGB888);
> -
> -	tc_write(DP0_MISC, (max_tu_symbol << 23) | (TU_SIZE_RECOMMENDED << 16) |
> -			   BPC_8);
> +	dev_dbg(tc->dev, "set mode %dx%d\n",
> +		mode->hdisplay, mode->vdisplay);
> +	dev_dbg(tc->dev, "H margin %d,%d sync %d\n",
> +		left_margin, right_margin, hsync_len);
> +	dev_dbg(tc->dev, "V margin %d,%d sync %d\n",
> +		upper_margin, lower_margin, vsync_len);
> +	dev_dbg(tc->dev, "total: %dx%d\n", mode->htotal, mode->vtotal);
>  
> -	return 0;
> -err:
> -	return ret;
> +	return regmap_multi_reg_write(tc->regmap,
> +				      video_mode_settings,
> +				      ARRAY_SIZE(video_mode_settings));
>  }
>  
>  static int tc_wait_link_training(struct tc_data *tc, u32 *error)

I don't like this change. I think multi_reg_write is good for writing
things like, say, color conversion table. But the data for video mode is
more complex and needs more logic (e.g. here ?: operator is used). You
need to stuff all that logic into variable initializers. You can't use
e.g. if() anymore, and you can't even insert a debug print between the
writes if you need to.

So, in my opinion, this doesn't look (much) cleaner, and makes life more
difficult.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 04/15] drm/bridge: tc358767: Simplify tc_set_video_mode()

[Andrey Smirnov]

On Fri, Mar 22, 2019 at 3:19 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> On 22/03/2019 05:28, Andrey Smirnov wrote:
> > Simplify tc_set_video_mode() by replacing repreated calls to
> > tc_write()/regmap_write() with a single call to
> > regmap_multi_reg_write(). While at it, simplify explicit shifting by
> > using macros from <linux/bitfield.h>. No functional change intended.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > Cc: Archit Taneja <architt@codeaurora.org>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > Cc: Chris Healy <cphealy@gmail.com>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  drivers/gpu/drm/bridge/tc358767.c | 146 +++++++++++++++++-------------
> >  1 file changed, 85 insertions(+), 61 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > index 38d542f553cd..d99c9f32a133 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -24,6 +24,7 @@
> >   * GNU General Public License for more details.
> >   */
> >
> > +#include <linux/bitfield.h>
> >  #include <linux/clk.h>
> >  #include <linux/device.h>
> >  #include <linux/gpio/consumer.h>
> > @@ -56,6 +57,7 @@
> >
> >  /* Video Path */
> >  #define VPCTRL0                      0x0450
> > +#define VSDELAY                      GENMASK(31, 20)
> >  #define OPXLFMT_RGB666                       (0 << 8)
> >  #define OPXLFMT_RGB888                       (1 << 8)
> >  #define FRMSYNC_DISABLED             (0 << 4) /* Video Timing Gen Disabled */
> > @@ -63,9 +65,17 @@
> >  #define MSF_DISABLED                 (0 << 0) /* Magic Square FRC disabled */
> >  #define MSF_ENABLED                  (1 << 0) /* Magic Square FRC enabled */
> >  #define HTIM01                       0x0454
> > +#define HPW                  GENMASK(8, 0)
> > +#define HBPR                 GENMASK(24, 16)
> >  #define HTIM02                       0x0458
> > +#define HDISPR                       GENMASK(10, 0)
> > +#define HFPR                 GENMASK(24, 16)
> >  #define VTIM01                       0x045c
> > +#define VSPR                 GENMASK(7, 0)
> > +#define VBPR                 GENMASK(23, 16)
> >  #define VTIM02                       0x0460
> > +#define VFPR                 GENMASK(23, 16)
> > +#define VDISPR                       GENMASK(10, 0)
> >  #define VFUEN0                       0x0464
> >  #define VFUEN                                BIT(0)   /* Video Frame Timing Upload */
> >
> > @@ -100,14 +110,28 @@
> >  /* Main Channel */
> >  #define DP0_SECSAMPLE                0x0640
> >  #define DP0_VIDSYNCDELAY     0x0644
> > +#define VID_SYNC_DLY         GENMASK(15, 0)
> > +#define THRESH_DLY           GENMASK(31, 16)
> > +
> >  #define DP0_TOTALVAL         0x0648
> > +#define H_TOTAL                      GENMASK(15, 0)
> > +#define V_TOTAL                      GENMASK(31, 16)
> >  #define DP0_STARTVAL         0x064c
> > +#define H_START                      GENMASK(15, 0)
> > +#define V_START                      GENMASK(31, 16)
> >  #define DP0_ACTIVEVAL                0x0650
> > +#define H_ACT                        GENMASK(15, 0)
> > +#define V_ACT                        GENMASK(31, 16)
> > +
> >  #define DP0_SYNCVAL          0x0654
> > +#define VS_WIDTH             GENMASK(30, 16)
> > +#define HS_WIDTH             GENMASK(14, 0)
> >  #define SYNCVAL_HS_POL_ACTIVE_LOW    (1 << 15)
> >  #define SYNCVAL_VS_POL_ACTIVE_LOW    (1 << 31)
> >  #define DP0_MISC             0x0658
> >  #define TU_SIZE_RECOMMENDED          (63) /* LSCLK cycles per TU */
> > +#define MAX_TU_SYMBOL                GENMASK(28, 23)
> > +#define TU_SIZE                      GENMASK(21, 16)
> >  #define BPC_6                                (0 << 5)
> >  #define BPC_8                                (1 << 5)
> >
> > @@ -184,6 +208,12 @@
> >
> >  /* Test & Debug */
> >  #define TSTCTL                       0x0a00
> > +#define COLOR_R                      GENMASK(31, 24)
> > +#define COLOR_G                      GENMASK(23, 16)
> > +#define COLOR_B                      GENMASK(15, 8)
> > +#define ENI2CFILTER          BIT(4)
> > +#define COLOR_BAR_MODE               GENMASK(1, 0)
> > +#define COLOR_BAR_MODE_BARS  2
> >  #define PLL_DBG                      0x0a04
> >
> >  static bool tc_test_pattern;
> > @@ -647,10 +677,6 @@ static int tc_get_display_props(struct tc_data *tc)
> >
> >  static int tc_set_video_mode(struct tc_data *tc, struct drm_display_mode *mode)
> >  {
> > -     int ret;
> > -     int vid_sync_dly;
> > -     int max_tu_symbol;
> > -
> >       int left_margin = mode->htotal - mode->hsync_end;
> >       int right_margin = mode->hsync_start - mode->hdisplay;
> >       int hsync_len = mode->hsync_end - mode->hsync_start;
> > @@ -659,76 +685,74 @@ static int tc_set_video_mode(struct tc_data *tc, struct drm_display_mode *mode)
> >       int vsync_len = mode->vsync_end - mode->vsync_start;
> >
> >       /*
> > -      * Recommended maximum number of symbols transferred in a transfer unit:
> > +      * Recommended maximum number of symbols transferred in a
> > +      * transfer unit:
> >        * DIV_ROUND_UP((input active video bandwidth in bytes) * tu_size,
> >        *              (output active video bandwidth in bytes))
> >        * Must be less than tu_size.
> >        */
> > -     max_tu_symbol = TU_SIZE_RECOMMENDED - 1;
> > -
> > -     dev_dbg(tc->dev, "set mode %dx%d\n",
> > -             mode->hdisplay, mode->vdisplay);
> > -     dev_dbg(tc->dev, "H margin %d,%d sync %d\n",
> > -             left_margin, right_margin, hsync_len);
> > -     dev_dbg(tc->dev, "V margin %d,%d sync %d\n",
> > -             upper_margin, lower_margin, vsync_len);
> > -     dev_dbg(tc->dev, "total: %dx%d\n", mode->htotal, mode->vtotal);
> > -
> > +     int max_tu_symbol = TU_SIZE_RECOMMENDED - 1;
> >
> > +     /* DP Main Stream Attributes */
> > +     int vid_sync_dly = hsync_len + left_margin + mode->hdisplay;
> >       /*
> >        * LCD Ctl Frame Size
> >        * datasheet is not clear of vsdelay in case of DPI
> >        * assume we do not need any delay when DPI is a source of
> >        * sync signals
> >        */
> > -     tc_write(VPCTRL0, (0 << 20) /* VSDELAY */ |
> > -              OPXLFMT_RGB888 | FRMSYNC_DISABLED | MSF_DISABLED);
> > -     tc_write(HTIM01, (ALIGN(left_margin, 2) << 16) | /* H back porch */
> > -                      (ALIGN(hsync_len, 2) << 0));    /* Hsync */
> > -     tc_write(HTIM02, (ALIGN(right_margin, 2) << 16) |  /* H front porch */
> > -                      (ALIGN(mode->hdisplay, 2) << 0)); /* width */
> > -     tc_write(VTIM01, (upper_margin << 16) |         /* V back porch */
> > -                      (vsync_len << 0));             /* Vsync */
> > -     tc_write(VTIM02, (lower_margin << 16) |         /* V front porch */
> > -                      (mode->vdisplay << 0));        /* height */
> > -     tc_write(VFUEN0, VFUEN);                /* update settings */
> > -
> > -     /* Test pattern settings */
> > -     tc_write(TSTCTL,
> > -              (120 << 24) |  /* Red Color component value */
> > -              (20 << 16) |   /* Green Color component value */
> > -              (99 << 8) |    /* Blue Color component value */
> > -              (1 << 4) |     /* Enable I2C Filter */
> > -              (2 << 0) |     /* Color bar Mode */
> > -              0);
> > -
> > -     /* DP Main Stream Attributes */
> > -     vid_sync_dly = hsync_len + left_margin + mode->hdisplay;
> > -     tc_write(DP0_VIDSYNCDELAY,
> > -              (max_tu_symbol << 16) |        /* thresh_dly */
> > -              (vid_sync_dly << 0));
> > +     const u32 vs_pol = mode->flags & DRM_MODE_FLAG_NVSYNC ?
> > +             SYNCVAL_VS_POL_ACTIVE_LOW : 0;
> > +     const u32 hs_pol = mode->flags & DRM_MODE_FLAG_NHSYNC ?
> > +             SYNCVAL_HS_POL_ACTIVE_LOW : 0;
> > +     const struct reg_sequence video_mode_settings[] = {
> > +             { VPCTRL0, FIELD_PREP(VSDELAY, 0) |
> > +                        OPXLFMT_RGB888 | FRMSYNC_DISABLED | MSF_DISABLED },
> > +             { HTIM01, FIELD_PREP(HBPR, ALIGN(left_margin, 2)) |
> > +                       FIELD_PREP(HPW, ALIGN(hsync_len, 2)) },
> > +             { HTIM02, FIELD_PREP(HDISPR, ALIGN(mode->hdisplay, 2)) |
> > +                       FIELD_PREP(HFPR, ALIGN(right_margin, 2)) },
> > +             { VTIM01, FIELD_PREP(VBPR, upper_margin) |
> > +                       FIELD_PREP(VSPR, vsync_len) },
> > +             { VTIM02, FIELD_PREP(VFPR, lower_margin) |
> > +                       FIELD_PREP(VDISPR, mode->vdisplay) },
> > +             { VFUEN0, VFUEN },
> > +             { TSTCTL, FIELD_PREP(COLOR_R, 120) |
> > +                       FIELD_PREP(COLOR_G, 20) |
> > +                       FIELD_PREP(COLOR_B, 99) |
> > +                       ENI2CFILTER |
> > +                       FIELD_PREP(COLOR_BAR_MODE, COLOR_BAR_MODE_BARS) },
> > +             { DP0_VIDSYNCDELAY, FIELD_PREP(THRESH_DLY, max_tu_symbol) |
> > +                                 FIELD_PREP(VID_SYNC_DLY, vid_sync_dly) },
> > +             { DP0_TOTALVAL, FIELD_PREP(H_TOTAL, mode->htotal) |
> > +                             FIELD_PREP(V_TOTAL, mode->vtotal) },
> > +             { DP0_STARTVAL, FIELD_PREP(H_START, left_margin + hsync_len) |
> > +                             FIELD_PREP(V_START,
> > +                                        upper_margin + vsync_len) },
> > +             { DP0_ACTIVEVAL, FIELD_PREP(V_ACT, mode->vdisplay) |
> > +                              FIELD_PREP(H_ACT, mode->hdisplay) },
> > +             { DP0_SYNCVAL, FIELD_PREP(VS_WIDTH, vsync_len) |
> > +                            FIELD_PREP(HS_WIDTH, hsync_len) |
> > +                            hs_pol | vs_pol },
> > +             { DPIPXLFMT, VS_POL_ACTIVE_LOW | HS_POL_ACTIVE_LOW |
> > +                          DE_POL_ACTIVE_HIGH | SUB_CFG_TYPE_CONFIG1 |
> > +                          DPI_BPP_RGB888 },
> > +             { DP0_MISC, FIELD_PREP(MAX_TU_SYMBOL, max_tu_symbol) |
> > +                         FIELD_PREP(TU_SIZE, TU_SIZE_RECOMMENDED) |
> > +                         BPC_8 },
> > +     };
> >
> > -     tc_write(DP0_TOTALVAL, (mode->vtotal << 16) | (mode->htotal));
> > -
> > -     tc_write(DP0_STARTVAL,
> > -              ((upper_margin + vsync_len) << 16) |
> > -              ((left_margin + hsync_len) << 0));
> > -
> > -     tc_write(DP0_ACTIVEVAL, (mode->vdisplay << 16) | (mode->hdisplay));
> > -
> > -     tc_write(DP0_SYNCVAL, (vsync_len << 16) | (hsync_len << 0) |
> > -              ((mode->flags & DRM_MODE_FLAG_NHSYNC) ? SYNCVAL_HS_POL_ACTIVE_LOW : 0) |
> > -              ((mode->flags & DRM_MODE_FLAG_NVSYNC) ? SYNCVAL_VS_POL_ACTIVE_LOW : 0));
> > -
> > -     tc_write(DPIPXLFMT, VS_POL_ACTIVE_LOW | HS_POL_ACTIVE_LOW |
> > -              DE_POL_ACTIVE_HIGH | SUB_CFG_TYPE_CONFIG1 | DPI_BPP_RGB888);
> > -
> > -     tc_write(DP0_MISC, (max_tu_symbol << 23) | (TU_SIZE_RECOMMENDED << 16) |
> > -                        BPC_8);
> > +     dev_dbg(tc->dev, "set mode %dx%d\n",
> > +             mode->hdisplay, mode->vdisplay);
> > +     dev_dbg(tc->dev, "H margin %d,%d sync %d\n",
> > +             left_margin, right_margin, hsync_len);
> > +     dev_dbg(tc->dev, "V margin %d,%d sync %d\n",
> > +             upper_margin, lower_margin, vsync_len);
> > +     dev_dbg(tc->dev, "total: %dx%d\n", mode->htotal, mode->vtotal);
> >
> > -     return 0;
> > -err:
> > -     return ret;
> > +     return regmap_multi_reg_write(tc->regmap,
> > +                                   video_mode_settings,
> > +                                   ARRAY_SIZE(video_mode_settings));
> >  }
> >
> >  static int tc_wait_link_training(struct tc_data *tc, u32 *error)
>
> I don't like this change. I think multi_reg_write is good for writing
> things like, say, color conversion table. But the data for video mode is
> more complex and needs more logic (e.g. here ?: operator is used).

The logic was stuffed into function arguments before, so this patch
didn't really change anything.

> You need to stuff all that logic into variable initializers. You can't use
> e.g. if() anymore,

Where you planning on doing so, or is it just hypothetical?

> and you can't even insert a debug print between the
> writes if you need to.
>

Same here.

> So, in my opinion, this doesn't look (much) cleaner, and makes life more
> difficult.
>

Sure, I'll drop this patch since it generates so much resistance.

Thanks,
Andrey Smirnov

[PATCH v2 05/15] drm/bridge: tc358767: Drop custom tc_write()/tc_read() accessors

[Andrey Smirnov]

A very unfortunate aspect of tc_write()/tc_read() macro helpers is
that they capture quite a bit of context around them and thus require
the caller to have magic variables 'ret' and 'tc' as well as label
'err'. That makes a number of code paths rather counterintuitive and
somewhat clunky, for example tc_stream_clock_calc() ends up being like
this:

	int ret;

	tc_write(DP0_VIDMNGEN1, 32768);

	return 0;
err:
	return ret;

which is rather surprising when you read the code for the first
time. Since those helpers arguably aren't really saving that much code
and there's no way of fixing them without making them too verbose to
be worth it change the driver code to not use them at all.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/tc358767.c | 249 +++++++++++++++++-------------
 1 file changed, 144 insertions(+), 105 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index d99c9f32a133..060e4b05095a 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -266,20 +266,6 @@ static inline struct tc_data *connector_to_tc(struct drm_connector *c)
 	return container_of(c, struct tc_data, connector);
 }
 
-/* Simple macros to avoid repeated error checks */
-#define tc_write(reg, var)					\
-	do {							\
-		ret = regmap_write(tc->regmap, reg, var);	\
-		if (ret)					\
-			goto err;				\
-	} while (0)
-#define tc_read(reg, var)					\
-	do {							\
-		ret = regmap_read(tc->regmap, reg, var);	\
-		if (ret)					\
-			goto err;				\
-	} while (0)
-
 static inline int tc_poll_timeout(struct tc_data *tc, unsigned int addr,
 				  unsigned int cond_mask,
 				  unsigned int cond_value,
@@ -337,7 +323,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
 
 	ret = tc_aux_wait_busy(tc, 100);
 	if (ret)
-		goto err;
+		return ret;
 
 	if (request == DP_AUX_I2C_WRITE || request == DP_AUX_NATIVE_WRITE) {
 		/* Store data */
@@ -348,7 +334,11 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
 				tmp = (tmp << 8) | buf[i];
 			i++;
 			if (((i % 4) == 0) || (i == size)) {
-				tc_write(DP0_AUXWDATA((i - 1) >> 2), tmp);
+				ret = regmap_write(tc->regmap,
+						   DP0_AUXWDATA((i - 1) >> 2),
+						   tmp);
+				if (ret)
+					return ret;
 				tmp = 0;
 			}
 		}
@@ -358,23 +348,32 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
 	}
 
 	/* Store address */
-	tc_write(DP0_AUXADDR, msg->address);
+	ret = regmap_write(tc->regmap, DP0_AUXADDR, msg->address);
+	if (ret)
+		return ret;
 	/* Start transfer */
-	tc_write(DP0_AUXCFG0, ((size - 1) << 8) | request);
+	ret = regmap_write(tc->regmap, DP0_AUXCFG0,
+			   ((size - 1) << 8) | request);
+	if (ret)
+		return ret;
 
 	ret = tc_aux_wait_busy(tc, 100);
 	if (ret)
-		goto err;
+		return ret;
 
 	ret = tc_aux_get_status(tc, &msg->reply);
 	if (ret)
-		goto err;
+		return ret;
 
 	if (request == DP_AUX_I2C_READ || request == DP_AUX_NATIVE_READ) {
 		/* Read data */
 		while (i < size) {
-			if ((i % 4) == 0)
-				tc_read(DP0_AUXRDATA(i >> 2), &tmp);
+			if ((i % 4) == 0) {
+				ret = regmap_read(tc->regmap,
+						  DP0_AUXRDATA(i >> 2), &tmp);
+				if (ret)
+					return ret;
+			}
 			buf[i] = tmp & 0xff;
 			tmp = tmp >> 8;
 			i++;
@@ -382,8 +381,6 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
 	}
 
 	return size;
-err:
-	return ret;
 }
 
 static const char * const training_pattern1_errors[] = {
@@ -440,6 +437,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
 	int ext_div[] = {1, 2, 3, 5, 7};
 	int best_pixelclock = 0;
 	int vco_hi = 0;
+	u32 pxl_pllparam;
 
 	dev_dbg(tc->dev, "PLL: requested %d pixelclock, ref %d\n", pixelclock,
 		refclk);
@@ -509,24 +507,29 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
 		best_mul = 0;
 
 	/* Power up PLL and switch to bypass */
-	tc_write(PXL_PLLCTRL, PLLBYP | PLLEN);
+	ret = regmap_write(tc->regmap, PXL_PLLCTRL, PLLBYP | PLLEN);
+	if (ret)
+		return ret;
 
-	tc_write(PXL_PLLPARAM,
-		 (vco_hi << 24) |		/* For PLL VCO >= 300 MHz = 1 */
-		 (ext_div[best_pre] << 20) |	/* External Pre-divider */
-		 (ext_div[best_post] << 16) |	/* External Post-divider */
-		 IN_SEL_REFCLK |		/* Use RefClk as PLL input */
-		 (best_div << 8) |		/* Divider for PLL RefClk */
-		 (best_mul << 0));		/* Multiplier for PLL */
+	pxl_pllparam  = vco_hi << 24; /* For PLL VCO >= 300 MHz = 1 */
+	pxl_pllparam |= ext_div[best_pre] << 20; /* External Pre-divider */
+	pxl_pllparam |= ext_div[best_post] << 16; /* External Post-divider */
+	pxl_pllparam |= IN_SEL_REFCLK; /* Use RefClk as PLL input */
+	pxl_pllparam |= best_div << 8; /* Divider for PLL RefClk */
+	pxl_pllparam |= best_mul; /* Multiplier for PLL */
+
+	ret = regmap_write(tc->regmap, PXL_PLLPARAM, pxl_pllparam);
+	if (ret)
+		return ret;
 
 	/* Force PLL parameter update and disable bypass */
-	tc_write(PXL_PLLCTRL, PLLUPDATE | PLLEN);
+	ret = regmap_write(tc->regmap, PXL_PLLCTRL, PLLUPDATE | PLLEN);
+	if (ret)
+		return ret;
 
 	tc_wait_pll_lock(tc);
 
 	return 0;
-err:
-	return ret;
 }
 
 static int tc_pxl_pll_dis(struct tc_data *tc)
@@ -537,7 +540,6 @@ static int tc_pxl_pll_dis(struct tc_data *tc)
 
 static int tc_stream_clock_calc(struct tc_data *tc)
 {
-	int ret;
 	/*
 	 * If the Stream clock and Link Symbol clock are
 	 * asynchronous with each other, the value of M changes over
@@ -553,16 +555,13 @@ static int tc_stream_clock_calc(struct tc_data *tc)
 	 * M/N = f_STRMCLK / f_LSCLK
 	 *
 	 */
-	tc_write(DP0_VIDMNGEN1, 32768);
-
-	return 0;
-err:
-	return ret;
+	return regmap_write(tc->regmap, DP0_VIDMNGEN1, 32768);
 }
 
 static int tc_aux_link_setup(struct tc_data *tc)
 {
 	unsigned long rate;
+	u32 dp0_auxcfg1;
 	u32 value;
 	int ret;
 
@@ -587,18 +586,25 @@ static int tc_aux_link_setup(struct tc_data *tc)
 
 	/* Setup DP-PHY / PLL */
 	value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
-	tc_write(SYS_PLLPARAM, value);
-
-	tc_write(DP_PHY_CTRL, BGREN | PWR_SW_EN | PHY_A0_EN);
+	ret = regmap_write(tc->regmap, SYS_PLLPARAM, value);
+	if (ret)
+		goto err;
 
+	ret = regmap_write(tc->regmap, DP_PHY_CTRL, BGREN | PWR_SW_EN | PHY_A0_EN);
+	if (ret)
+		goto err;
 	/*
 	 * Initially PLLs are in bypass. Force PLL parameter update,
 	 * disable PLL bypass, enable PLL
 	 */
-	tc_write(DP0_PLLCTRL, PLLUPDATE | PLLEN);
+	ret = regmap_write(tc->regmap, DP0_PLLCTRL, PLLUPDATE | PLLEN);
+	if (ret)
+		goto err;
 	tc_wait_pll_lock(tc);
 
-	tc_write(DP1_PLLCTRL, PLLUPDATE | PLLEN);
+	ret = regmap_write(tc->regmap, DP1_PLLCTRL, PLLUPDATE | PLLEN);
+	if (ret)
+		goto err;
 	tc_wait_pll_lock(tc);
 
 	ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1, 1000);
@@ -610,9 +616,13 @@ static int tc_aux_link_setup(struct tc_data *tc)
 	}
 
 	/* Setup AUX link */
-	tc_write(DP0_AUXCFG1, AUX_RX_FILTER_EN |
-		 (0x06 << 8) |	/* Aux Bit Period Calculator Threshold */
-		 (0x3f << 0));	/* Aux Response Timeout Timer */
+	dp0_auxcfg1  = AUX_RX_FILTER_EN;
+	dp0_auxcfg1 |= 0x06 << 8; /* Aux Bit Period Calculator Threshold */
+	dp0_auxcfg1 |= 0x3f << 0; /* Aux Response Timeout Timer */
+
+	ret = regmap_write(tc->regmap, DP0_AUXCFG1, dp0_auxcfg1);
+	if (ret)
+		goto err;
 
 	return 0;
 err:
@@ -764,16 +774,16 @@ static int tc_wait_link_training(struct tc_data *tc, u32 *error)
 			      LT_LOOPDONE, 1, 1000);
 	if (ret) {
 		dev_err(tc->dev, "Link training timeout waiting for LT_LOOPDONE!\n");
-		goto err;
+		return ret;
 	}
 
-	tc_read(DP0_LTSTAT, &value);
+	ret = regmap_read(tc->regmap, DP0_LTSTAT, &value);
+	if (ret)
+		return ret;
 
 	*error = (value >> 8) & 0x7;
 
 	return 0;
-err:
-	return ret;
 }
 
 static int tc_main_link_enable(struct tc_data *tc)
@@ -789,13 +799,19 @@ static int tc_main_link_enable(struct tc_data *tc)
 
 	dev_dbg(tc->dev, "link enable\n");
 
-	tc_write(DP0CTL, 0);
+	ret = regmap_write(tc->regmap, DP0CTL, 0);
+	if (ret)
+		return ret;
 
-	tc_write(DP0_SRCCTRL, tc_srcctrl(tc));
+	ret = regmap_write(tc->regmap, DP0_SRCCTRL, tc_srcctrl(tc));
+	if (ret)
+		return ret;
 	/* SSCG and BW27 on DP1 must be set to the same as on DP0 */
-	tc_write(DP1_SRCCTRL,
+	ret = regmap_write(tc->regmap, DP1_SRCCTRL,
 		 (tc->link.spread ? DP0_SRCCTRL_SSCG : 0) |
 		 ((tc->link.base.rate != 162000) ? DP0_SRCCTRL_BW27 : 0));
+	if (ret)
+		return ret;
 
 	rate = clk_get_rate(tc->refclk);
 	switch (rate) {
@@ -815,27 +831,36 @@ static int tc_main_link_enable(struct tc_data *tc)
 		return -EINVAL;
 	}
 	value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
-	tc_write(SYS_PLLPARAM, value);
+	ret = regmap_write(tc->regmap, SYS_PLLPARAM, value);
+	if (ret)
+		return ret;
 
 	/* Setup Main Link */
 	dp_phy_ctrl = BGREN | PWR_SW_EN | PHY_A0_EN | PHY_M0_EN;
 	if (tc->link.base.num_lanes == 2)
 		dp_phy_ctrl |= PHY_2LANE;
-	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
+
+	ret = regmap_write(tc->regmap, DP_PHY_CTRL, dp_phy_ctrl);
+	if (ret)
+		return ret;
 
 	/* PLL setup */
-	tc_write(DP0_PLLCTRL, PLLUPDATE | PLLEN);
+	ret = regmap_write(tc->regmap, DP0_PLLCTRL, PLLUPDATE | PLLEN);
+	if (ret)
+		return ret;
 	tc_wait_pll_lock(tc);
 
-	tc_write(DP1_PLLCTRL, PLLUPDATE | PLLEN);
+	ret = regmap_write(tc->regmap, DP1_PLLCTRL, PLLUPDATE | PLLEN);
+	if (ret)
+		return ret;
 	tc_wait_pll_lock(tc);
 
 	/* Reset/Enable Main Links */
 	dp_phy_ctrl |= DP_PHY_RST | PHY_M1_RST | PHY_M0_RST;
-	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
+	ret = regmap_write(tc->regmap, DP_PHY_CTRL, dp_phy_ctrl);
 	usleep_range(100, 200);
 	dp_phy_ctrl &= ~(DP_PHY_RST | PHY_M1_RST | PHY_M0_RST);
-	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
+	ret = regmap_write(tc->regmap, DP_PHY_CTRL, dp_phy_ctrl);
 
 	ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1, 1000);
 	if (ret) {
@@ -847,7 +872,7 @@ static int tc_main_link_enable(struct tc_data *tc)
 	/* Set misc: 8 bits per color */
 	ret = regmap_update_bits(tc->regmap, DP0_MISC, BPC_8, BPC_8);
 	if (ret)
-		goto err;
+		return ret;
 
 	/*
 	 * ASSR mode
@@ -899,55 +924,68 @@ static int tc_main_link_enable(struct tc_data *tc)
 	/* LINK TRAINING PATTERN 1 */
 
 	/* Set DPCD 0x102 for Training Pattern 1 */
-	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_1);
+	ret = regmap_write(tc->regmap, DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_1);
+	if (ret)
+		return ret;
 
-	tc_write(DP0_LTLOOPCTRL,
-		 (15 << 28) |	/* Defer Iteration Count */
-		 (15 << 24) |	/* Loop Iteration Count */
-		 (0xd << 0));	/* Loop Timer Delay */
+	ret = regmap_write(tc->regmap, DP0_LTLOOPCTRL,
+			   (15 << 28) |	/* Defer Iteration Count */
+			   (15 << 24) |	/* Loop Iteration Count */
+			   (0xd << 0));	/* Loop Timer Delay */
+	if (ret)
+		return ret;
 
-	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
-		 DP0_SRCCTRL_TP1);
+	ret = regmap_write(tc->regmap, DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
+			   DP0_SRCCTRL_TP1);
+	if (ret)
+		return ret;
 
 	/* Enable DP0 to start Link Training */
-	tc_write(DP0CTL,
-		 ((tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ? EF_EN : 0) |
-		 DP_EN);
+	ret = regmap_write(tc->regmap, DP0CTL,
+			   ((tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ? EF_EN : 0) |
+			   DP_EN);
+	if (ret)
+		return ret;
 
 	/* wait */
 	ret = tc_wait_link_training(tc, &error);
 	if (ret)
-		goto err;
+		return ret;
 
 	if (error) {
 		dev_err(tc->dev, "Link training phase 1 failed: %s\n",
 			training_pattern1_errors[error]);
-		ret = -ENODEV;
-		goto err;
+		return -ENODEV;
 	}
 
 	/* LINK TRAINING PATTERN 2 */
 
 	/* Set DPCD 0x102 for Training Pattern 2 */
-	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_2);
+	ret = regmap_write(tc->regmap, DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_2);
+	if (ret)
+		return ret;
 
-	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
-		 DP0_SRCCTRL_TP2);
+	ret = regmap_write(tc->regmap, DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
+			   DP0_SRCCTRL_TP2);
+	if (ret)
+		return ret;
 
 	/* wait */
 	ret = tc_wait_link_training(tc, &error);
 	if (ret)
-		goto err;
+		return ret;
 
 	if (error) {
 		dev_err(tc->dev, "Link training phase 2 failed: %s\n",
 			training_pattern2_errors[error]);
-		ret = -ENODEV;
-		goto err;
+		return -ENODEV;
 	}
 
 	/* Clear Training Pattern, set AutoCorrect Mode = 1 */
-	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_AUTOCORRECT);
+	ret = regmap_write(tc->regmap, DP0_SRCCTRL, tc_srcctrl(tc) |
+			   DP0_SRCCTRL_AUTOCORRECT);
+	if (ret)
+		return ret;
 
 	/* Clear DPCD 0x102 */
 	/* Note: Can Not use DP0_SNKLTCTRL (0x06E4) short cut */
@@ -991,7 +1029,7 @@ static int tc_main_link_enable(struct tc_data *tc)
 		dev_err(dev, "0x0205 SINK_STATUS:               0x%02x\n", tmp[3]);
 		dev_err(dev, "0x0206 ADJUST_REQUEST_LANE0_1:    0x%02x\n", tmp[4]);
 		dev_err(dev, "0x0207 ADJUST_REQUEST_LANE2_3:    0x%02x\n", tmp[5]);
-		goto err;
+		return ret;
 	}
 
 	return 0;
@@ -1000,7 +1038,6 @@ static int tc_main_link_enable(struct tc_data *tc)
 	return ret;
 err_dpcd_write:
 	dev_err(tc->dev, "Failed to write DPCD: %d\n", ret);
-err:
 	return ret;
 }
 
@@ -1010,12 +1047,11 @@ static int tc_main_link_disable(struct tc_data *tc)
 
 	dev_dbg(tc->dev, "link disable\n");
 
-	tc_write(DP0_SRCCTRL, 0);
-	tc_write(DP0CTL, 0);
+	ret = regmap_write(tc->regmap, DP0_SRCCTRL, 0);
+	if (ret)
+		return ret;
 
-	return 0;
-err:
-	return ret;
+	return regmap_write(tc->regmap, DP0CTL, 0);
 }
 
 static int tc_stream_enable(struct tc_data *tc)
@@ -1030,22 +1066,24 @@ static int tc_stream_enable(struct tc_data *tc)
 		ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
 				    1000 * tc->mode.clock);
 		if (ret)
-			goto err;
+			return ret;
 	}
 
 	ret = tc_set_video_mode(tc, &tc->mode);
 	if (ret)
-		goto err;
+		return ret;
 
 	/* Set M/N */
 	ret = tc_stream_clock_calc(tc);
 	if (ret)
-		goto err;
+		return ret;
 
 	value = VID_MN_GEN | DP_EN;
 	if (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
 		value |= EF_EN;
-	tc_write(DP0CTL, value);
+	ret = regmap_write(tc->regmap, DP0CTL, value);
+	if (ret)
+		return ret;
 	/*
 	 * VID_EN assertion should be delayed by at least N * LSCLK
 	 * cycles from the time VID_MN_GEN is enabled in order to
@@ -1055,18 +1093,20 @@ static int tc_stream_enable(struct tc_data *tc)
 	 */
 	usleep_range(500, 1000);
 	value |= VID_EN;
-	tc_write(DP0CTL, value);
+	ret = regmap_write(tc->regmap, DP0CTL, value);
+	if (ret)
+		return ret;
 	/* Set input interface */
 	value = DP0_AUDSRC_NO_INPUT;
 	if (tc_test_pattern)
 		value |= DP0_VIDSRC_COLOR_BAR;
 	else
 		value |= DP0_VIDSRC_DPI_RX;
-	tc_write(SYSCTRL, value);
+	ret = regmap_write(tc->regmap, SYSCTRL, value);
+	if (ret)
+		return ret;
 
 	return 0;
-err:
-	return ret;
 }
 
 static int tc_stream_disable(struct tc_data *tc)
@@ -1075,13 +1115,13 @@ static int tc_stream_disable(struct tc_data *tc)
 
 	dev_dbg(tc->dev, "stream disable\n");
 
-	tc_write(DP0CTL, 0);
+	ret = regmap_write(tc->regmap, DP0CTL, 0);
+	if (ret)
+		return ret;
 
 	tc_pxl_pll_dis(tc);
 
 	return 0;
-err:
-	return ret;
 }
 
 static void tc_bridge_pre_enable(struct drm_bridge *bridge)
@@ -1258,7 +1298,9 @@ static enum drm_connector_status tc_connector_detect(struct drm_connector *conne
 	int ret;
 	bool conn;
 
-	tc_read(GPIOI, &val);
+	ret = regmap_read(tc->regmap, GPIOI, &val);
+	if (ret)
+		return connector_status_unknown;
 
 	conn = val & BIT(0);
 
@@ -1269,9 +1311,6 @@ static enum drm_connector_status tc_connector_detect(struct drm_connector *conne
 		return connector_status_connected;
 	else
 		return connector_status_disconnected;
-
-err:
-	return connector_status_unknown;
 }
 
 static const struct drm_connector_funcs tc_connector_funcs = {
-- 
2.20.1

Re: [PATCH v2 05/15] drm/bridge: tc358767: Drop custom tc_write()/tc_read() accessors

[Tomi Valkeinen]

On 22/03/2019 05:28, Andrey Smirnov wrote:
> A very unfortunate aspect of tc_write()/tc_read() macro helpers is
> that they capture quite a bit of context around them and thus require
> the caller to have magic variables 'ret' and 'tc' as well as label
> 'err'. That makes a number of code paths rather counterintuitive and
> somewhat clunky, for example tc_stream_clock_calc() ends up being like
> this:
> 
> 	int ret;
> 
> 	tc_write(DP0_VIDMNGEN1, 32768);
> 
> 	return 0;
> err:
> 	return ret;
> 
> which is rather surprising when you read the code for the first
> time. Since those helpers arguably aren't really saving that much code
> and there's no way of fixing them without making them too verbose to
> be worth it change the driver code to not use them at all.

I fully agree with this patch and thought about the same thing during my
work.

However, the timing of this patch is not too good, as this one will
totally conflict with any other patch for tc358767, and my series is
still evolving.

We need to figure out how to combine this series and mine, but I think
either this patch should be dropped for now, and reapplied after the
other patches have stabilized, or I think preferably, this one could be
rebased on top of 5.1-rc1, and used as a base for all other tc358767 work.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 05/15] drm/bridge: tc358767: Drop custom tc_write()/tc_read() accessors

[Andrey Smirnov]

On Fri, Mar 22, 2019 at 3:29 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> On 22/03/2019 05:28, Andrey Smirnov wrote:
> > A very unfortunate aspect of tc_write()/tc_read() macro helpers is
> > that they capture quite a bit of context around them and thus require
> > the caller to have magic variables 'ret' and 'tc' as well as label
> > 'err'. That makes a number of code paths rather counterintuitive and
> > somewhat clunky, for example tc_stream_clock_calc() ends up being like
> > this:
> >
> >       int ret;
> >
> >       tc_write(DP0_VIDMNGEN1, 32768);
> >
> >       return 0;
> > err:
> >       return ret;
> >
> > which is rather surprising when you read the code for the first
> > time. Since those helpers arguably aren't really saving that much code
> > and there's no way of fixing them without making them too verbose to
> > be worth it change the driver code to not use them at all.
>
> I fully agree with this patch and thought about the same thing during my
> work.
>
> However, the timing of this patch is not too good, as this one will
> totally conflict with any other patch for tc358767, and my series is
> still evolving.
>

The reason I rebased this series on top of yours is because I think
mine should go after yours gets accepted and lands in the tree. I am
more than happy to wait for your series to mature. This submission was
done mostly for the sake of discussion, looping original authors in as
well as making you aware of its existence.

> We need to figure out how to combine this series and mine, but I think
> either this patch should be dropped for now, and reapplied after the
> other patches have stabilized, or I think preferably, this one could be
> rebased on top of 5.1-rc1, and used as a base for all other tc358767 work.
>

I think waiting for your work to be done before proceeding to apply
this series, should solve this problem.

Thanks,
Andrey Smirnov

[PATCH v2 07/15] drm/bridge: tc358767: Simplify AUX data write

[Andrey Smirnov]

Simplify AUX data write by dropping index arithmetic and shifting and
replacing it with a call to a helper function that does three things:

    1. Copies user-provided data into a write buffer
    2. Optionally fixes the endianness of the write buffer (not needed
       on LE hosts)
    3. Transfers contenst of the write buffer to up to 4 32-bit
       registers on the chip

Note that separate data endianness fix:

    tmp = (tmp << 8) | buf[i];

that was reserved for DP_AUX_I2C_WRITE looks really strange, since it
will place data differently depending on the passed user-data
size. E.g. for a write of 1 byte, data transferred to the chip would
look like:

[byte0] [dummy1] [dummy2] [dummy3]

whereas for a write of 4 bytes we'd get:

[byte3] [byte2] [byte1] [byte0]

Since there's no indication in the datasheet that I2C write buffer
should be treated differently than AUX write buffer and no comment in
the original code explaining why it was done this way, that special
I2C write buffer transformation was dropped in this patch.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/tc358767.c | 58 +++++++++++++++++++------------
 1 file changed, 36 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 81c10a5d8106..21374565585d 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -307,6 +307,31 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
 	return 0;
 }
 
+static int tc_aux_write_data(struct tc_data *tc, const void *data,
+			     size_t size)
+{
+	u32 auxwdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)] = { 0 };
+	int ret, i, count = DIV_ROUND_UP(size, 4);
+
+	memcpy(auxwdata, data, size);
+
+	for (i = 0; i < count; i++) {
+		ret = regmap_write(tc->regmap, DP0_AUXWDATA(i),
+				   /*
+				    * Our regmap is configured as LE
+				    * for register data, so we need
+				    * undo any byte swapping that will
+				    * happened to preserve original
+				    * byte order.
+				    */
+				   cpu_to_le32(auxwdata[i]));
+		if (ret)
+			return ret;
+	}
+
+	return size;
+}
+
 static int tc_aux_read_data(struct tc_data *tc, void *data, size_t size)
 {
 	u32 auxrdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)];
@@ -335,9 +360,6 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
 	struct tc_data *tc = aux_to_tc(aux);
 	size_t size = min_t(size_t, 8, msg->size);
 	u8 request = msg->request & ~DP_AUX_I2C_MOT;
-	u8 *buf = msg->buffer;
-	u32 tmp = 0;
-	int i = 0;
 	int ret;
 
 	if (size == 0)
@@ -347,25 +369,17 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
 	if (ret)
 		return ret;
 
-	if (request == DP_AUX_I2C_WRITE || request == DP_AUX_NATIVE_WRITE) {
-		/* Store data */
-		while (i < size) {
-			if (request == DP_AUX_NATIVE_WRITE)
-				tmp = tmp | (buf[i] << (8 * (i & 0x3)));
-			else
-				tmp = (tmp << 8) | buf[i];
-			i++;
-			if (((i % 4) == 0) || (i == size)) {
-				ret = regmap_write(tc->regmap,
-						   DP0_AUXWDATA((i - 1) >> 2),
-						   tmp);
-				if (ret)
-					return ret;
-				tmp = 0;
-			}
-		}
-	} else if (request != DP_AUX_I2C_READ &&
-		   request != DP_AUX_NATIVE_READ) {
+	switch (request) {
+	case DP_AUX_NATIVE_READ:
+	case DP_AUX_I2C_READ:
+		break;
+	case DP_AUX_NATIVE_WRITE:
+	case DP_AUX_I2C_WRITE:
+		ret = tc_aux_write_data(tc, msg->buffer, size);
+		if (ret < 0)
+			return ret;
+		break;
+	default:
 		return -EINVAL;
 	}
 
-- 
2.20.1

Re: [PATCH v2 07/15] drm/bridge: tc358767: Simplify AUX data write

[Tomi Valkeinen]

On 22/03/2019 05:28, Andrey Smirnov wrote:
> Simplify AUX data write by dropping index arithmetic and shifting and
> replacing it with a call to a helper function that does three things:
> 
>     1. Copies user-provided data into a write buffer
>     2. Optionally fixes the endianness of the write buffer (not needed
>        on LE hosts)
>     3. Transfers contenst of the write buffer to up to 4 32-bit
>        registers on the chip
> 
> Note that separate data endianness fix:
> 
>     tmp = (tmp << 8) | buf[i];
> 
> that was reserved for DP_AUX_I2C_WRITE looks really strange, since it
> will place data differently depending on the passed user-data
> size. E.g. for a write of 1 byte, data transferred to the chip would
> look like:
> 
> [byte0] [dummy1] [dummy2] [dummy3]
> 
> whereas for a write of 4 bytes we'd get:
> 
> [byte3] [byte2] [byte1] [byte0]
> 
> Since there's no indication in the datasheet that I2C write buffer
> should be treated differently than AUX write buffer and no comment in
> the original code explaining why it was done this way, that special
> I2C write buffer transformation was dropped in this patch.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 58 +++++++++++++++++++------------
>  1 file changed, 36 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 81c10a5d8106..21374565585d 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -307,6 +307,31 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
>  	return 0;
>  }
>  
> +static int tc_aux_write_data(struct tc_data *tc, const void *data,
> +			     size_t size)
> +{
> +	u32 auxwdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)] = { 0 };
> +	int ret, i, count = DIV_ROUND_UP(size, 4);

Should 4 there be sizeof(u32)?

> +
> +	memcpy(auxwdata, data, size);
> +
> +	for (i = 0; i < count; i++) {
> +		ret = regmap_write(tc->regmap, DP0_AUXWDATA(i),
> +				   /*
> +				    * Our regmap is configured as LE
> +				    * for register data, so we need
> +				    * undo any byte swapping that will
> +				    * happened to preserve original
> +				    * byte order.
> +				    */
> +				   cpu_to_le32(auxwdata[i]));

Can regmap_bulk_write or regmap_raw_write be used here?

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 07/15] drm/bridge: tc358767: Simplify AUX data write

[Andrey Smirnov]

On Fri, Mar 22, 2019 at 3:51 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> On 22/03/2019 05:28, Andrey Smirnov wrote:
> > Simplify AUX data write by dropping index arithmetic and shifting and
> > replacing it with a call to a helper function that does three things:
> >
> >     1. Copies user-provided data into a write buffer
> >     2. Optionally fixes the endianness of the write buffer (not needed
> >        on LE hosts)
> >     3. Transfers contenst of the write buffer to up to 4 32-bit
> >        registers on the chip
> >
> > Note that separate data endianness fix:
> >
> >     tmp = (tmp << 8) | buf[i];
> >
> > that was reserved for DP_AUX_I2C_WRITE looks really strange, since it
> > will place data differently depending on the passed user-data
> > size. E.g. for a write of 1 byte, data transferred to the chip would
> > look like:
> >
> > [byte0] [dummy1] [dummy2] [dummy3]
> >
> > whereas for a write of 4 bytes we'd get:
> >
> > [byte3] [byte2] [byte1] [byte0]
> >
> > Since there's no indication in the datasheet that I2C write buffer
> > should be treated differently than AUX write buffer and no comment in
> > the original code explaining why it was done this way, that special
> > I2C write buffer transformation was dropped in this patch.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > Cc: Archit Taneja <architt@codeaurora.org>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > Cc: Chris Healy <cphealy@gmail.com>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  drivers/gpu/drm/bridge/tc358767.c | 58 +++++++++++++++++++------------
> >  1 file changed, 36 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > index 81c10a5d8106..21374565585d 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -307,6 +307,31 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
> >       return 0;
> >  }
> >
> > +static int tc_aux_write_data(struct tc_data *tc, const void *data,
> > +                          size_t size)
> > +{
> > +     u32 auxwdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)] = { 0 };
> > +     int ret, i, count = DIV_ROUND_UP(size, 4);
>
> Should 4 there be sizeof(u32)?

Yup, will replace.

>
> > +
> > +     memcpy(auxwdata, data, size);
> > +
> > +     for (i = 0; i < count; i++) {
> > +             ret = regmap_write(tc->regmap, DP0_AUXWDATA(i),
> > +                                /*
> > +                                 * Our regmap is configured as LE
> > +                                 * for register data, so we need
> > +                                 * undo any byte swapping that will
> > +                                 * happened to preserve original
> > +                                 * byte order.
> > +                                 */
> > +                                cpu_to_le32(auxwdata[i]));
>
> Can regmap_bulk_write or regmap_raw_write be used here?
>

Not sure, will give it a try.

Thanks,
Andrey Smirnov

[PATCH v2 08/15] drm/bridge: tc358767: Increase AUX transfer length limit

[Andrey Smirnov]

According to the datasheet tc358767 can transfer up to 16 bytes via
its AUX channel, so the artificial limit of 8 apperas to be too
low. However only up to 15-bytes seem to be actually supported and
trying to use 16-byte transfers results in transfers failing
sporadically (with bogus status in case of I2C transfers), so limit it
to 15.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/tc358767.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 21374565585d..8adaac5ca271 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -358,7 +358,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
 			       struct drm_dp_aux_msg *msg)
 {
 	struct tc_data *tc = aux_to_tc(aux);
-	size_t size = min_t(size_t, 8, msg->size);
+	size_t size = min_t(size_t, DP_AUX_MAX_PAYLOAD_BYTES - 1, msg->size);
 	u8 request = msg->request & ~DP_AUX_I2C_MOT;
 	int ret;
 
-- 
2.20.1

Re: [PATCH v2 08/15] drm/bridge: tc358767: Increase AUX transfer length limit

[Tomi Valkeinen]

On 22/03/2019 05:28, Andrey Smirnov wrote:
> According to the datasheet tc358767 can transfer up to 16 bytes via
> its AUX channel, so the artificial limit of 8 apperas to be too
> low. However only up to 15-bytes seem to be actually supported and
> trying to use 16-byte transfers results in transfers failing
> sporadically (with bogus status in case of I2C transfers), so limit it
> to 15.

16 is the limit from the DP spec. I agree, 8 looks odd.

15 looks odd too, so I think it warrants a comment there in the code.

Does 15 byte transfers ever work? Or mostly works but sometimes fails?

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 08/15] drm/bridge: tc358767: Increase AUX transfer length limit

[Andrey Smirnov]

On Fri, Mar 22, 2019 at 6:14 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> On 22/03/2019 05:28, Andrey Smirnov wrote:
> > According to the datasheet tc358767 can transfer up to 16 bytes via
> > its AUX channel, so the artificial limit of 8 apperas to be too
> > low. However only up to 15-bytes seem to be actually supported and
> > trying to use 16-byte transfers results in transfers failing
> > sporadically (with bogus status in case of I2C transfers), so limit it
> > to 15.
>
> 16 is the limit from the DP spec. I agree, 8 looks odd.
>
> 15 looks odd too, so I think it warrants a comment there in the code.
>

Crap, was going to add that, but forgot. Will do in v2.

> Does 15 byte transfers ever work? Or mostly works but sometimes fails?
>

15 bytes transfers work every time (at least to extent I tested it).
For 16 byte transfers it depends on the transfer type. AUX transfers
work for a while but then fail (as tested by dd'ing AUX chardev). I2C
transfers work intermittently and when they fail return completely
bogus status.

Thanks,
Andrey Smirnov

[PATCH v2 09/15] drm/bridge: tc358767: Use reported AUX transfer size

[Andrey Smirnov]

Don't assume that requested data transfer size is the same as amount
of data that was transferred. Change the code to get that information
from DP0_AUXSTATUS instead.

Since the check for AUX_BUSY in tc_aux_get_status() is pointless (it
will always called after tc_aux_wait_busy()) and there's only one user
of it, inline its code into tc_aux_transfer() instead of trying to
accommodate the change above.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/tc358767.c | 40 ++++++++++---------------------
 1 file changed, 12 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 8adaac5ca271..7e4607c6907f 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -144,10 +144,10 @@
 #define DP0_AUXWDATA(i)		(0x066c + (i) * 4)
 #define DP0_AUXRDATA(i)		(0x067c + (i) * 4)
 #define DP0_AUXSTATUS		0x068c
-#define AUX_STATUS_MASK			0xf0
-#define AUX_STATUS_SHIFT		4
-#define AUX_TIMEOUT			BIT(1)
-#define AUX_BUSY			BIT(0)
+#define AUX_BYTES		GENMASK(15, 8)
+#define AUX_STATUS		GENMASK(7, 4)
+#define AUX_TIMEOUT		BIT(1)
+#define AUX_BUSY		BIT(0)
 #define DP0_AUXI2CADR		0x0698
 
 /* Link Training */
@@ -284,29 +284,6 @@ static int tc_aux_wait_busy(struct tc_data *tc, unsigned int timeout_ms)
 			       1000, 1000 * timeout_ms);
 }
 
-static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
-{
-	int ret;
-	u32 value;
-
-	ret = regmap_read(tc->regmap, DP0_AUXSTATUS, &value);
-	if (ret < 0)
-		return ret;
-
-	if (value & AUX_BUSY) {
-		dev_err(tc->dev, "aux busy!\n");
-		return -EBUSY;
-	}
-
-	if (value & AUX_TIMEOUT) {
-		dev_err(tc->dev, "aux access timeout!\n");
-		return -ETIMEDOUT;
-	}
-
-	*reply = (value & AUX_STATUS_MASK) >> AUX_STATUS_SHIFT;
-	return 0;
-}
-
 static int tc_aux_write_data(struct tc_data *tc, const void *data,
 			     size_t size)
 {
@@ -360,6 +337,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
 	struct tc_data *tc = aux_to_tc(aux);
 	size_t size = min_t(size_t, DP_AUX_MAX_PAYLOAD_BYTES - 1, msg->size);
 	u8 request = msg->request & ~DP_AUX_I2C_MOT;
+	u32 auxstatus;
 	int ret;
 
 	if (size == 0)
@@ -397,10 +375,16 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
 	if (ret)
 		return ret;
 
-	ret = tc_aux_get_status(tc, &msg->reply);
+	ret = regmap_read(tc->regmap, DP0_AUXSTATUS, &auxstatus);
 	if (ret)
 		return ret;
 
+	if (auxstatus & AUX_TIMEOUT)
+		return -ETIMEDOUT;
+
+	size = FIELD_GET(AUX_BYTES, auxstatus);
+	msg->reply = FIELD_GET(AUX_STATUS, auxstatus);
+
 	switch (request) {
 	case DP_AUX_NATIVE_READ:
 	case DP_AUX_I2C_READ:
-- 
2.20.1

[PATCH v2 10/15] drm/bridge: tc358767: Add support for address-only I2C transfers

[Andrey Smirnov]

Transfer size of zero means a request to do an address-only
transfer. Since the HW support this, we probably shouldn't be just
ignoring such requests. While at it allow DP_AUX_I2C_MOT flag to pass
through, since it is supported by the HW as well.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/tc358767.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 7e4607c6907f..768f01cc2a30 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -137,6 +137,8 @@
 
 /* AUX channel */
 #define DP0_AUXCFG0		0x0660
+#define DP0_AUXCFG0_BSIZE	GENMASK(11, 8)
+#define DP0_AUXCFG0_ADDR_ONLY	BIT(4)
 #define DP0_AUXCFG1		0x0664
 #define AUX_RX_FILTER_EN		BIT(16)
 
@@ -331,6 +333,18 @@ static int tc_aux_read_data(struct tc_data *tc, void *data, size_t size)
 	return size;
 }
 
+static u32 tc_auxcfg0(struct drm_dp_aux_msg *msg, size_t size)
+{
+	u32 auxcfg0 = msg->request;
+
+	if (size)
+		auxcfg0 |= FIELD_PREP(DP0_AUXCFG0_BSIZE, size - 1);
+	else
+		auxcfg0 |= DP0_AUXCFG0_ADDR_ONLY;
+
+	return auxcfg0;
+}
+
 static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
 			       struct drm_dp_aux_msg *msg)
 {
@@ -340,9 +354,6 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
 	u32 auxstatus;
 	int ret;
 
-	if (size == 0)
-		return 0;
-
 	ret = tc_aux_wait_busy(tc, 100);
 	if (ret)
 		return ret;
@@ -366,8 +377,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
 	if (ret)
 		return ret;
 	/* Start transfer */
-	ret = regmap_write(tc->regmap, DP0_AUXCFG0,
-			   ((size - 1) << 8) | request);
+	ret = regmap_write(tc->regmap, DP0_AUXCFG0, tc_auxcfg0(msg, size));
 	if (ret)
 		return ret;
 
@@ -381,8 +391,14 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
 
 	if (auxstatus & AUX_TIMEOUT)
 		return -ETIMEDOUT;
-
-	size = FIELD_GET(AUX_BYTES, auxstatus);
+	/*
+	 * For some reason address-only DP_AUX_I2C_WRITE (MOT), still
+	 * reports 1 byte transferred in its status. To deal we that
+	 * we ignore aux_bytes field if we know that this was an
+	 * address-only transfer
+	 */
+	if (size)
+		size = FIELD_GET(AUX_BYTES, auxstatus);
 	msg->reply = FIELD_GET(AUX_STATUS, auxstatus);
 
 	switch (request) {
-- 
2.20.1

[PATCH v2 11/15] drm/bridge: tc358767: Introduce tc_set_syspllparam()

[Andrey Smirnov]

Move common code converting clock rate to an appropriate constant and
configuring SYS_PLLPARAM register into a separate routine and convert
the rest of the code to use it. No functional change intended.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/tc358767.c | 48 +++++++++++--------------------
 1 file changed, 17 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 768f01cc2a30..976a9861e537 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -585,35 +585,41 @@ static int tc_stream_clock_calc(struct tc_data *tc)
 	return regmap_write(tc->regmap, DP0_VIDMNGEN1, 32768);
 }
 
-static int tc_aux_link_setup(struct tc_data *tc)
+static int tc_set_syspllparam(struct tc_data *tc)
 {
 	unsigned long rate;
-	u32 dp0_auxcfg1;
-	u32 value;
-	int ret;
+	u32 pllparam = SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
 
 	rate = clk_get_rate(tc->refclk);
 	switch (rate) {
 	case 38400000:
-		value = REF_FREQ_38M4;
+		pllparam |= REF_FREQ_38M4;
 		break;
 	case 26000000:
-		value = REF_FREQ_26M;
+		pllparam |= REF_FREQ_26M;
 		break;
 	case 19200000:
-		value = REF_FREQ_19M2;
+		pllparam |= REF_FREQ_19M2;
 		break;
 	case 13000000:
-		value = REF_FREQ_13M;
+		pllparam |= REF_FREQ_13M;
 		break;
 	default:
 		dev_err(tc->dev, "Invalid refclk rate: %lu Hz\n", rate);
 		return -EINVAL;
 	}
 
+	return regmap_write(tc->regmap, SYS_PLLPARAM, pllparam);
+}
+
+static int tc_aux_link_setup(struct tc_data *tc)
+{
+	int ret;
+	u32 dp_phy_ctrl;
+	u32 dp0_auxcfg1;
+
 	/* Setup DP-PHY / PLL */
-	value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
-	ret = regmap_write(tc->regmap, SYS_PLLPARAM, value);
+	ret = tc_set_syspllparam(tc);
 	if (ret)
 		goto err;
 
@@ -817,8 +823,6 @@ static int tc_main_link_enable(struct tc_data *tc)
 {
 	struct drm_dp_aux *aux = &tc->aux;
 	struct device *dev = tc->dev;
-	unsigned int rate;
-	u32 dp_phy_ctrl;
 	u32 value;
 	int ret;
 	u8 tmp[8];
@@ -840,25 +844,7 @@ static int tc_main_link_enable(struct tc_data *tc)
 	if (ret)
 		return ret;
 
-	rate = clk_get_rate(tc->refclk);
-	switch (rate) {
-	case 38400000:
-		value = REF_FREQ_38M4;
-		break;
-	case 26000000:
-		value = REF_FREQ_26M;
-		break;
-	case 19200000:
-		value = REF_FREQ_19M2;
-		break;
-	case 13000000:
-		value = REF_FREQ_13M;
-		break;
-	default:
-		return -EINVAL;
-	}
-	value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
-	ret = regmap_write(tc->regmap, SYS_PLLPARAM, value);
+	ret = tc_set_syspllparam(tc);
 	if (ret)
 		return ret;
 
-- 
2.20.1

[PATCH v2 12/15] drm/bridge: tc358767: Introduce tc_pllupdate_pllen()

[Andrey Smirnov]

tc_wait_pll_lock() is always called as a follow-up for updating
PLLUPDATE and PLLEN bit of a given PLL control register. To simplify
things, merge the two operation into a single helper function
tc_pllupdate_pllen() and convert the rest of the code to use it. No
functional change intended.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/tc358767.c | 32 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 976a9861e537..f66a1c4a2047 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -447,10 +447,18 @@ static u32 tc_srcctrl(struct tc_data *tc)
 	return reg;
 }
 
-static void tc_wait_pll_lock(struct tc_data *tc)
+static int tc_pllupdate_pllen(struct tc_data *tc, unsigned int pllctrl)
 {
+	int ret;
+
+	ret = regmap_write(tc->regmap, pllctrl, PLLUPDATE | PLLEN);
+	if (ret)
+		return ret;
+
 	/* Wait for PLL to lock: up to 2.09 ms, depending on refclk */
 	usleep_range(3000, 6000);
+
+	return 0;
 }
 
 static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
@@ -550,13 +558,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
 		return ret;
 
 	/* Force PLL parameter update and disable bypass */
-	ret = regmap_write(tc->regmap, PXL_PLLCTRL, PLLUPDATE | PLLEN);
-	if (ret)
-		return ret;
-
-	tc_wait_pll_lock(tc);
-
-	return 0;
+	return tc_pllupdate_pllen(tc, PXL_PLLCTRL);
 }
 
 static int tc_pxl_pll_dis(struct tc_data *tc)
@@ -615,7 +617,6 @@ static int tc_set_syspllparam(struct tc_data *tc)
 static int tc_aux_link_setup(struct tc_data *tc)
 {
 	int ret;
-	u32 dp_phy_ctrl;
 	u32 dp0_auxcfg1;
 
 	/* Setup DP-PHY / PLL */
@@ -630,15 +631,13 @@ static int tc_aux_link_setup(struct tc_data *tc)
 	 * Initially PLLs are in bypass. Force PLL parameter update,
 	 * disable PLL bypass, enable PLL
 	 */
-	ret = regmap_write(tc->regmap, DP0_PLLCTRL, PLLUPDATE | PLLEN);
+	ret = tc_pllupdate_pllen(tc, DP0_PLLCTRL);
 	if (ret)
 		goto err;
-	tc_wait_pll_lock(tc);
 
-	ret = regmap_write(tc->regmap, DP1_PLLCTRL, PLLUPDATE | PLLEN);
+	ret = tc_pllupdate_pllen(tc, DP1_PLLCTRL);
 	if (ret)
 		goto err;
-	tc_wait_pll_lock(tc);
 
 	ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1, 1000);
 	if (ret == -ETIMEDOUT) {
@@ -823,6 +822,7 @@ static int tc_main_link_enable(struct tc_data *tc)
 {
 	struct drm_dp_aux *aux = &tc->aux;
 	struct device *dev = tc->dev;
+	u32 dp_phy_ctrl;
 	u32 value;
 	int ret;
 	u8 tmp[8];
@@ -858,15 +858,13 @@ static int tc_main_link_enable(struct tc_data *tc)
 		return ret;
 
 	/* PLL setup */
-	ret = regmap_write(tc->regmap, DP0_PLLCTRL, PLLUPDATE | PLLEN);
+	ret = tc_pllupdate_pllen(tc, DP0_PLLCTRL);
 	if (ret)
 		return ret;
-	tc_wait_pll_lock(tc);
 
-	ret = regmap_write(tc->regmap, DP1_PLLCTRL, PLLUPDATE | PLLEN);
+	ret = tc_pllupdate_pllen(tc, DP1_PLLCTRL);
 	if (ret)
 		return ret;
-	tc_wait_pll_lock(tc);
 
 	/* Reset/Enable Main Links */
 	dp_phy_ctrl |= DP_PHY_RST | PHY_M1_RST | PHY_M0_RST;
-- 
2.20.1

[PATCH v2 13/15] drm/bridge: tc358767: Simplify tc_aux_wait_busy()

[Andrey Smirnov]

We never pass anything but 100 as timeout_ms to tc_aux_wait_busy(), so
we may as well hardcode that value and simplify function's signature.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/tc358767.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index f66a1c4a2047..07d568761463 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -280,10 +280,9 @@ static inline int tc_poll_timeout(struct tc_data *tc, unsigned int addr,
 					sleep_us, timeout_us);
 }
 
-static int tc_aux_wait_busy(struct tc_data *tc, unsigned int timeout_ms)
+static int tc_aux_wait_busy(struct tc_data *tc)
 {
-	return tc_poll_timeout(tc, DP0_AUXSTATUS, AUX_BUSY, 0,
-			       1000, 1000 * timeout_ms);
+	return tc_poll_timeout(tc, DP0_AUXSTATUS, AUX_BUSY, 0, 1000, 100000);
 }
 
 static int tc_aux_write_data(struct tc_data *tc, const void *data,
@@ -354,7 +353,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
 	u32 auxstatus;
 	int ret;
 
-	ret = tc_aux_wait_busy(tc, 100);
+	ret = tc_aux_wait_busy(tc);
 	if (ret)
 		return ret;
 
@@ -381,7 +380,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
 	if (ret)
 		return ret;
 
-	ret = tc_aux_wait_busy(tc, 100);
+	ret = tc_aux_wait_busy(tc);
 	if (ret)
 		return ret;
 
-- 
2.20.1

[PATCH v2 14/15] drm/bridge: tc358767: Drop unnecessary 8 byte buffer

[Andrey Smirnov]

tc_get_display_props() never reads more than a byte via AUX, so
there's no need to reserve 8 for that purpose. No function change
intended.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/tc358767.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 07d568761463..b106e6181a5f 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -664,8 +664,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
 static int tc_get_display_props(struct tc_data *tc)
 {
 	int ret;
-	/* temp buffer */
-	u8 tmp[8];
+	u8 reg;
 
 	/* Read DP Rx Link Capability */
 	ret = drm_dp_link_probe(&tc->aux, &tc->link.base);
@@ -681,21 +680,21 @@ static int tc_get_display_props(struct tc_data *tc)
 		tc->link.base.num_lanes = 2;
 	}
 
-	ret = drm_dp_dpcd_readb(&tc->aux, DP_MAX_DOWNSPREAD, tmp);
+	ret = drm_dp_dpcd_readb(&tc->aux, DP_MAX_DOWNSPREAD, &reg);
 	if (ret < 0)
 		goto err_dpcd_read;
-	tc->link.spread = tmp[0] & DP_MAX_DOWNSPREAD_0_5;
+	tc->link.spread = reg & DP_MAX_DOWNSPREAD_0_5;
 
-	ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
+	ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, &reg);
 	if (ret < 0)
 		goto err_dpcd_read;
 
 	tc->link.scrambler_dis = false;
 	/* read assr */
-	ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
+	ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, &reg);
 	if (ret < 0)
 		goto err_dpcd_read;
-	tc->link.assr = tmp[0] & DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
+	tc->link.assr = reg & DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
 
 	dev_dbg(tc->dev, "DPCD rev: %d.%d, rate: %s, lanes: %d, framing: %s\n",
 		tc->link.base.revision >> 4, tc->link.base.revision & 0x0f,
-- 
2.20.1

[PATCH v2 15/15] drm/bridge: tc358767: Replace magic number in tc_main_link_enable()

[Andrey Smirnov]

We don't need 8 byte array, DP_LINK_STATUS_SIZE (6) should be
enough. This also gets rid of a magic number as a bonus.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/tc358767.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index b106e6181a5f..68af144b8768 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -823,7 +823,7 @@ static int tc_main_link_enable(struct tc_data *tc)
 	u32 dp_phy_ctrl;
 	u32 value;
 	int ret;
-	u8 tmp[8];
+	u8 tmp[DP_LINK_STATUS_SIZE];
 	u32 error;
 
 	dev_dbg(tc->dev, "link enable\n");
-- 
2.20.1

Re: [PATCH v2 00/15] tc358767 driver improvements

[Tomi Valkeinen]

Hi,

On 22/03/2019 05:28, Andrey Smirnov wrote:
> Everyone:
> 
> This series contains various improvements (at least in my mind) and
> fixes that I made to tc358767 while working with the code of the
> driver. Hopefuly each patch is self explanatory.
> 
> Feedback is welcome!

Ah, I hadn't realized there was another series for tc358767 going
around. I should've checked dri-devel list. Sorry about that!

I'm not able to apply your patches, it fails at "Simplify
tc_set_video_mode()", even if I think I've got the exact same base as
you have.

Can you push your branch somewhere?

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 00/15] tc358767 driver improvements

[Andrey Smirnov]

On Fri, Mar 22, 2019 at 1:06 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> Hi,
>
> On 22/03/2019 05:28, Andrey Smirnov wrote:
> > Everyone:
> >
> > This series contains various improvements (at least in my mind) and
> > fixes that I made to tc358767 while working with the code of the
> > driver. Hopefuly each patch is self explanatory.
> >
> > Feedback is welcome!

Ugh, I just realized that I messed up CC list of "drm/bridge:
tc358767: Simplify AUX data read" and it only went to dri-devel. Will
fix in next version.

>
> Ah, I hadn't realized there was another series for tc358767 going
> around. I should've checked dri-devel list. Sorry about that!
>

No worries, lucky for me Lucas Stach noticed your series and pointed
me to it, so it all worked out in the end.

> I'm not able to apply your patches, it fails at "Simplify
> tc_set_video_mode()", even if I think I've got the exact same base as
> you have.
>
> Can you push your branch somewhere?

Sure, it should be available here:
https://github.com/ndreys/linux/commits/rdu1-rdu2 Please ignore
"drm/bridge: tc358767: Add code to control VDCC rail", sitting on top.
I need it on my board, but it's currently purely experimental and not
meant to be upstreamed yet (or maybe ever)

Thanks,
Andrey Smirnov