[PATCH 0/9] tc358767 driver improvements

[PATCH 0/9] tc358767 driver improvements

[Andrey Smirnov]

Everyone:

This series contains various improvements (at least in my mind) 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

Andrey Smirnov (9):
  drm/bridge: tc358767: Simplify tc_poll_timeout()
  drm/bridge: tc358767: Simplify tc_stream_clock_calc()
  drm/bridge: tc358767: Simplify tc_set_video_mode()
  drm/bridge: tc358767: Simplify polling in tc_main_link_setup()
  drm/bridge: tc358767: Simplify polling in tc_link_training()
  drm/bridge: tc358767: Simplify error check in tc_aux_linx_setup()
  drm/bridge: tc358767: Introduce tc_set_syspllparam()
  drm/bridge: tc358767: Introduce tc_pllupdate_pllen()
  drm/bridge: tc358767: Drop tc_read() macro

 drivers/gpu/drm/bridge/tc358767.c | 283 ++++++++++++++----------------
 1 file changed, 129 insertions(+), 154 deletions(-)

-- 
2.20.1

[PATCH 1/9] 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. 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: 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 | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index e6403b9549f1..b0f8264a1285 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -252,24 +252,11 @@ static inline int tc_poll_timeout(struct regmap *map, unsigned int addr,
 				  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(map, 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)
-- 
2.20.1

Re: [PATCH 1/9] drm/bridge: tc358767: Simplify tc_poll_timeout()

[Andrzej Hajda]

On 26.02.2019 20:36, 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. 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: Chris Healy <cphealy@gmail.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index e6403b9549f1..b0f8264a1285 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -252,24 +252,11 @@ static inline int tc_poll_timeout(struct regmap *map, unsigned int addr,
>  				  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(map, 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)

Re: [PATCH 1/9] drm/bridge: tc358767: Simplify tc_poll_timeout()

[Laurent Pinchart]

Hi Andrey,

Thank you for the patch.

On Tue, Feb 26, 2019 at 11:36:01AM -0800, 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. 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: Chris Healy <cphealy@gmail.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/tc358767.c | 19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index e6403b9549f1..b0f8264a1285 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -252,24 +252,11 @@ static inline int tc_poll_timeout(struct regmap *map, unsigned int addr,
>  				  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(map, 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)

-- 
Regards,

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

[PATCH 2/9] drm/bridge: tc358767: Simplify tc_stream_clock_calc()

[Andrey Smirnov]

Drop the use of tc_write() as well as "magicly" used "ret" and "err:"
and replace it with a simple call to regmap_write(). 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: 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 | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index b0f8264a1285..86ebd49194b7 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -504,7 +504,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
@@ -520,11 +519,7 @@ 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)
-- 
2.20.1

Re: [PATCH 2/9] drm/bridge: tc358767: Simplify tc_stream_clock_calc()

[Andrzej Hajda]

On 26.02.2019 20:36, Andrey Smirnov wrote:
> Drop the use of tc_write() as well as "magicly" used "ret" and "err:"
> and replace it with a simple call to regmap_write(). 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: 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 | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index b0f8264a1285..86ebd49194b7 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -504,7 +504,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
> @@ -520,11 +519,7 @@ 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);


The patch looks semantically correct, but you are dropping here custom
accessor (tc_write) in favor of regmap API.

I think the best would be consistent across the whole driver: either use
only  accessors, either drop them entirely and use regmap API.

And it would be good to have comment of the original authors about this
change.


Regards

Andrzej


>  }
>  
>  static int tc_aux_link_setup(struct tc_data *tc)

Re: [PATCH 2/9] drm/bridge: tc358767: Simplify tc_stream_clock_calc()

[Laurent Pinchart]

Hello,

On Mon, Mar 04, 2019 at 10:42:20AM +0100, Andrzej Hajda wrote:
> On 26.02.2019 20:36, Andrey Smirnov wrote:
> > Drop the use of tc_write() as well as "magicly" used "ret" and "err:"
> > and replace it with a simple call to regmap_write(). 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: 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 | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > index b0f8264a1285..86ebd49194b7 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -504,7 +504,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
> > @@ -520,11 +519,7 @@ 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);
> 
> 
> The patch looks semantically correct, but you are dropping here custom
> accessor (tc_write) in favor of regmap API.
> 
> I think the best would be consistent across the whole driver: either use
> only  accessors, either drop them entirely and use regmap API.

I agree with this. The tc_write macro with its goto err is pretty nasty,
and I'd like to see it removed from the whole driver, or at least
replaced with an accessor that wouldn't hide the goto.

> And it would be good to have comment of the original authors about this
> change.
> 
> >  }
> >  
> >  static int tc_aux_link_setup(struct tc_data *tc)

-- 
Regards,

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

Re: [PATCH 2/9] drm/bridge: tc358767: Simplify tc_stream_clock_calc()

[Andrey Smirnov]

On Mon, Mar 4, 2019 at 4:20 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hello,
>
> On Mon, Mar 04, 2019 at 10:42:20AM +0100, Andrzej Hajda wrote:
> > On 26.02.2019 20:36, Andrey Smirnov wrote:
> > > Drop the use of tc_write() as well as "magicly" used "ret" and "err:"
> > > and replace it with a simple call to regmap_write(). 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: 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 | 7 +------
> > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > > index b0f8264a1285..86ebd49194b7 100644
> > > --- a/drivers/gpu/drm/bridge/tc358767.c
> > > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > > @@ -504,7 +504,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
> > > @@ -520,11 +519,7 @@ 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);
> >
> >
> > The patch looks semantically correct, but you are dropping here custom
> > accessor (tc_write) in favor of regmap API.
> >
> > I think the best would be consistent across the whole driver: either use
> > only  accessors, either drop them entirely and use regmap API.
>
> I agree with this. The tc_write macro with its goto err is pretty nasty,
> and I'd like to see it removed from the whole driver, or at least
> replaced with an accessor that wouldn't hide the goto.
>
> > And it would be good to have comment of the original authors about this
> > change.
> >

OK, I'll create a patch to remove tc_write/read in v2 and add original
authors to CC.

Thanks,
Andrey Smirnov

[PATCH 3/9] 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 regmap_multi_reg_write().
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: 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 | 125 ++++++++++++++++--------------
 1 file changed, 65 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 86ebd49194b7..c85468fcc157 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -641,10 +641,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;
@@ -653,76 +649,85 @@ 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 */
-
+	u32 vpctrl0 = (0 << 20) /* VSDELAY */ |
+		      OPXLFMT_RGB888 | FRMSYNC_DISABLED |
+		      MSF_DISABLED;
+	u32 htim01 = (ALIGN(left_margin, 2) << 16) | /* H back porch */
+		     (ALIGN(hsync_len, 2) << 0); /* Hsync */
+	u32 htim02 = (ALIGN(right_margin, 2) << 16) | /* H front porch */
+		     (ALIGN(mode->hdisplay, 2) << 0); /* width */
+	u32 vtim01 = (upper_margin << 16) | /* V back porch */
+		     (vsync_len << 0); /* Vsync */
+	u32 vtim02 = (lower_margin << 16) | /* V front porch */
+		     (mode->vdisplay << 0); /* height */
 	/* 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));
+	u32 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 */
+	u32 dp0_vidsyncdelay = (max_tu_symbol << 16) |	/* thresh_dly */
+			       (vid_sync_dly << 0);
+	u32 dp0_totalval = (mode->vtotal << 16) | (mode->htotal);
+	u32 dp0_startval = ((upper_margin + vsync_len) << 16) |
+			   ((left_margin + hsync_len) << 0);
+	u32 dp0_activeval = (mode->vdisplay << 16) | (mode->hdisplay);
+	u32 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;
+	u32 dpipxlfmt = VS_POL_ACTIVE_LOW | HS_POL_ACTIVE_LOW |
+			DE_POL_ACTIVE_HIGH | SUB_CFG_TYPE_CONFIG1 |
+			DPI_BPP_RGB888;
+	u32 dp0_misc = (max_tu_symbol << 23) |
+		       (TU_SIZE_RECOMMENDED << 16) |
+		       BPC_8;
+
+	const struct reg_sequence video_mode_settings[] = {
+		{ VPCTRL0, vpctrl0 },
+		{ HTIM01, htim01 },
+		{ HTIM02, htim02 },
+		{ VTIM01, vtim01 },
+		{ VTIM02, vtim02 },
+		{ VFUEN0, VFUEN },  /* update settings */
+		{ TSTCTL, tstctl },
+		{ DP0_VIDSYNCDELAY, dp0_vidsyncdelay },
+		{ DP0_TOTALVAL, dp0_totalval },
+		{ DP0_STARTVAL, dp0_startval },
+		{ DP0_ACTIVEVAL, dp0_activeval },
+		{ DP0_SYNCVAL, dp0_syncval },
+		{ DPIPXLFMT, dpipxlfmt },
+		{ DP0_MISC, dp0_misc },
+	};
 
-	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_link_training(struct tc_data *tc, int pattern)
-- 
2.20.1

Re: [PATCH 3/9] drm/bridge: tc358767: Simplify tc_set_video_mode()

[Laurent Pinchart]

Hi Andrey,

Thank you for the patch.

On Tue, Feb 26, 2019 at 11:36:03AM -0800, Andrey Smirnov wrote:
> Simplify tc_set_video_mode() by replacing repreated calls to
> tc_write()/regmap_write() with a single call regmap_multi_reg_write().
> 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: 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 | 125 ++++++++++++++++--------------
>  1 file changed, 65 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 86ebd49194b7..c85468fcc157 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -641,10 +641,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;
> @@ -653,76 +649,85 @@ 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 */
> -
> +	u32 vpctrl0 = (0 << 20) /* VSDELAY */ |
> +		      OPXLFMT_RGB888 | FRMSYNC_DISABLED |
> +		      MSF_DISABLED;
> +	u32 htim01 = (ALIGN(left_margin, 2) << 16) | /* H back porch */
> +		     (ALIGN(hsync_len, 2) << 0); /* Hsync */
> +	u32 htim02 = (ALIGN(right_margin, 2) << 16) | /* H front porch */
> +		     (ALIGN(mode->hdisplay, 2) << 0); /* width */
> +	u32 vtim01 = (upper_margin << 16) | /* V back porch */
> +		     (vsync_len << 0); /* Vsync */
> +	u32 vtim02 = (lower_margin << 16) | /* V front porch */
> +		     (mode->vdisplay << 0); /* height */
>  	/* 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));
> +	u32 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 */
> +	u32 dp0_vidsyncdelay = (max_tu_symbol << 16) |	/* thresh_dly */
> +			       (vid_sync_dly << 0);
> +	u32 dp0_totalval = (mode->vtotal << 16) | (mode->htotal);
> +	u32 dp0_startval = ((upper_margin + vsync_len) << 16) |
> +			   ((left_margin + hsync_len) << 0);
> +	u32 dp0_activeval = (mode->vdisplay << 16) | (mode->hdisplay);
> +	u32 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;
> +	u32 dpipxlfmt = VS_POL_ACTIVE_LOW | HS_POL_ACTIVE_LOW |
> +			DE_POL_ACTIVE_HIGH | SUB_CFG_TYPE_CONFIG1 |
> +			DPI_BPP_RGB888;
> +	u32 dp0_misc = (max_tu_symbol << 23) |
> +		       (TU_SIZE_RECOMMENDED << 16) |
> +		       BPC_8;
> +
> +	const struct reg_sequence video_mode_settings[] = {
> +		{ VPCTRL0, vpctrl0 },
> +		{ HTIM01, htim01 },
> +		{ HTIM02, htim02 },
> +		{ VTIM01, vtim01 },
> +		{ VTIM02, vtim02 },
> +		{ VFUEN0, VFUEN },  /* update settings */
> +		{ TSTCTL, tstctl },
> +		{ DP0_VIDSYNCDELAY, dp0_vidsyncdelay },
> +		{ DP0_TOTALVAL, dp0_totalval },
> +		{ DP0_STARTVAL, dp0_startval },
> +		{ DP0_ACTIVEVAL, dp0_activeval },
> +		{ DP0_SYNCVAL, dp0_syncval },
> +		{ DPIPXLFMT, dpipxlfmt },
> +		{ DP0_MISC, dp0_misc },
> +	};

I like the removal of the tc_write() macro, but slightly dislike the
need for this array. Another possible option to handle errors in a nicer
way would be

static int tc_write(struct tc_data *tc, unsigned int reg, unsigned int val,
		    int *status)
{
	if (!*status)
		*status = regmap_write(tc->regmap, reg, val);
	return *status;
}

...
	int err = 0;

	tc_write(tc, ..., ..., &err);
	tc_write(tc, ..., ..., &err);
	tc_write(tc, ..., ..., &err);
	tc_write(tc, ..., ..., &err);
	tc_write(tc, ..., ..., &err);

	return err;

I will let you and the driver author(s) device what would be best.

>  
> -	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_link_training(struct tc_data *tc, int pattern)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/9] drm/bridge: tc358767: Simplify tc_set_video_mode()

[Andrey Smirnov]

On Mon, Mar 4, 2019 at 4:26 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Andrey,
>
> Thank you for the patch.
>
> On Tue, Feb 26, 2019 at 11:36:03AM -0800, Andrey Smirnov wrote:
> > Simplify tc_set_video_mode() by replacing repreated calls to
> > tc_write()/regmap_write() with a single call regmap_multi_reg_write().
> > 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: 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 | 125 ++++++++++++++++--------------
> >  1 file changed, 65 insertions(+), 60 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > index 86ebd49194b7..c85468fcc157 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -641,10 +641,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;
> > @@ -653,76 +649,85 @@ 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 */
> > -
> > +     u32 vpctrl0 = (0 << 20) /* VSDELAY */ |
> > +                   OPXLFMT_RGB888 | FRMSYNC_DISABLED |
> > +                   MSF_DISABLED;
> > +     u32 htim01 = (ALIGN(left_margin, 2) << 16) | /* H back porch */
> > +                  (ALIGN(hsync_len, 2) << 0); /* Hsync */
> > +     u32 htim02 = (ALIGN(right_margin, 2) << 16) | /* H front porch */
> > +                  (ALIGN(mode->hdisplay, 2) << 0); /* width */
> > +     u32 vtim01 = (upper_margin << 16) | /* V back porch */
> > +                  (vsync_len << 0); /* Vsync */
> > +     u32 vtim02 = (lower_margin << 16) | /* V front porch */
> > +                  (mode->vdisplay << 0); /* height */
> >       /* 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));
> > +     u32 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 */
> > +     u32 dp0_vidsyncdelay = (max_tu_symbol << 16) |  /* thresh_dly */
> > +                            (vid_sync_dly << 0);
> > +     u32 dp0_totalval = (mode->vtotal << 16) | (mode->htotal);
> > +     u32 dp0_startval = ((upper_margin + vsync_len) << 16) |
> > +                        ((left_margin + hsync_len) << 0);
> > +     u32 dp0_activeval = (mode->vdisplay << 16) | (mode->hdisplay);
> > +     u32 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;
> > +     u32 dpipxlfmt = VS_POL_ACTIVE_LOW | HS_POL_ACTIVE_LOW |
> > +                     DE_POL_ACTIVE_HIGH | SUB_CFG_TYPE_CONFIG1 |
> > +                     DPI_BPP_RGB888;
> > +     u32 dp0_misc = (max_tu_symbol << 23) |
> > +                    (TU_SIZE_RECOMMENDED << 16) |
> > +                    BPC_8;
> > +
> > +     const struct reg_sequence video_mode_settings[] = {
> > +             { VPCTRL0, vpctrl0 },
> > +             { HTIM01, htim01 },
> > +             { HTIM02, htim02 },
> > +             { VTIM01, vtim01 },
> > +             { VTIM02, vtim02 },
> > +             { VFUEN0, VFUEN },  /* update settings */
> > +             { TSTCTL, tstctl },
> > +             { DP0_VIDSYNCDELAY, dp0_vidsyncdelay },
> > +             { DP0_TOTALVAL, dp0_totalval },
> > +             { DP0_STARTVAL, dp0_startval },
> > +             { DP0_ACTIVEVAL, dp0_activeval },
> > +             { DP0_SYNCVAL, dp0_syncval },
> > +             { DPIPXLFMT, dpipxlfmt },
> > +             { DP0_MISC, dp0_misc },
> > +     };
>
> I like the removal of the tc_write() macro, but slightly dislike the
> need for this array. Another possible option to handle errors in a nicer
> way would be
>
> static int tc_write(struct tc_data *tc, unsigned int reg, unsigned int val,
>                     int *status)
> {
>         if (!*status)
>                 *status = regmap_write(tc->regmap, reg, val);
>         return *status;
> }
>
> ...
>         int err = 0;
>
>         tc_write(tc, ..., ..., &err);
>         tc_write(tc, ..., ..., &err);
>         tc_write(tc, ..., ..., &err);
>         tc_write(tc, ..., ..., &err);
>         tc_write(tc, ..., ..., &err);
>
>         return err;
>

I'd rather not define any new helper functions just for the sake of
avoiding using regmap_multi_reg_write(). I updated this patch in v2 to
combine register value initialization and array declaration, so maybe
you'll find it less displeasing.

Thanks,
Andrey Smirnov

[PATCH 4/9] drm/bridge: tc358767: Simplify polling in tc_main_link_setup()

[Andrey Smirnov]

Replace explicit polling loop with equivalent call to
regmap_read_poll_timeout() for simplicity. 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: 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 | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index c85468fcc157..6455e6484722 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -882,15 +882,11 @@ static int tc_main_link_setup(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) {
+	ret = regmap_read_poll_timeout(tc->regmap, DP_PHY_CTRL, value,
+				       value & PHY_RDY, 1, 1000);
+	if (ret) {
 		dev_err(dev, "timeout waiting for phy become ready");
-		return -ETIMEDOUT;
+		return ret;
 	}
 
 	/* Set misc: 8 bits per color */
-- 
2.20.1

Re: [PATCH 4/9] drm/bridge: tc358767: Simplify polling in tc_main_link_setup()

[Laurent Pinchart]

On Tue, Feb 26, 2019 at 11:36:04AM -0800, Andrey Smirnov wrote:
> Replace explicit polling loop with equivalent call to
> regmap_read_poll_timeout() for simplicity. 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: Chris Healy <cphealy@gmail.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/tc358767.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index c85468fcc157..6455e6484722 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -882,15 +882,11 @@ static int tc_main_link_setup(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) {
> +	ret = regmap_read_poll_timeout(tc->regmap, DP_PHY_CTRL, value,
> +				       value & PHY_RDY, 1, 1000);
> +	if (ret) {
>  		dev_err(dev, "timeout waiting for phy become ready");
> -		return -ETIMEDOUT;
> +		return ret;
>  	}
>  
>  	/* Set misc: 8 bits per color */

-- 
Regards,

Laurent Pinchart

[PATCH 5/9] drm/bridge: tc358767: Simplify polling in tc_link_training()

[Andrey Smirnov]

Replace explicit polling in tc_link_training() with equivalent call to
regmap_read_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: 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 | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 6455e6484722..ea30cec4a0c3 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -735,7 +735,6 @@ static int tc_link_training(struct tc_data *tc, int pattern)
 	const char * const *errors;
 	u32 srcctrl = tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
 		      DP0_SRCCTRL_AUTOCORRECT;
-	int timeout;
 	int retry;
 	u32 value;
 	int ret;
@@ -765,20 +764,17 @@ static int tc_link_training(struct tc_data *tc, int pattern)
 		tc_write(DP0CTL, DP_EN);
 
 		/* wait */
-		timeout = 1000;
-		do {
-			tc_read(DP0_LTSTAT, &value);
-			udelay(1);
-		} while ((!(value & LT_LOOPDONE)) && (--timeout));
-		if (timeout == 0) {
+		ret = regmap_read_poll_timeout(tc->regmap, DP0_LTSTAT, value,
+					       value & LT_LOOPDONE, 1, 1000);
+		if (ret) {
 			dev_err(tc->dev, "Link training timeout!\n");
 		} else {
 			int pattern = (value >> 11) & 0x3;
 			int error = (value >> 8) & 0x7;
 
 			dev_dbg(tc->dev,
-				"Link training phase %d done after %d uS: %s\n",
-				pattern, 1000 - timeout, errors[error]);
+				"Link training phase %d done: %s\n",
+				pattern, errors[error]);
 			if (pattern == DP_TRAINING_PATTERN_1 && error == 0)
 				break;
 			if (pattern == DP_TRAINING_PATTERN_2) {
-- 
2.20.1

Re: [PATCH 5/9] drm/bridge: tc358767: Simplify polling in tc_link_training()

[Laurent Pinchart]

Hi Andrey,

Thank you for the patch.

On Tue, Feb 26, 2019 at 11:36:05AM -0800, Andrey Smirnov wrote:
> Replace explicit polling in tc_link_training() with equivalent call to
> regmap_read_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: 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 | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 6455e6484722..ea30cec4a0c3 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -735,7 +735,6 @@ static int tc_link_training(struct tc_data *tc, int pattern)
>  	const char * const *errors;
>  	u32 srcctrl = tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
>  		      DP0_SRCCTRL_AUTOCORRECT;
> -	int timeout;
>  	int retry;
>  	u32 value;
>  	int ret;
> @@ -765,20 +764,17 @@ static int tc_link_training(struct tc_data *tc, int pattern)
>  		tc_write(DP0CTL, DP_EN);
>  
>  		/* wait */
> -		timeout = 1000;
> -		do {
> -			tc_read(DP0_LTSTAT, &value);
> -			udelay(1);
> -		} while ((!(value & LT_LOOPDONE)) && (--timeout));
> -		if (timeout == 0) {
> +		ret = regmap_read_poll_timeout(tc->regmap, DP0_LTSTAT, value,
> +					       value & LT_LOOPDONE, 1, 1000);
> +		if (ret) {
>  			dev_err(tc->dev, "Link training timeout!\n");
>  		} else {
>  			int pattern = (value >> 11) & 0x3;
>  			int error = (value >> 8) & 0x7;
>  
>  			dev_dbg(tc->dev,
> -				"Link training phase %d done after %d uS: %s\n",
> -				pattern, 1000 - timeout, errors[error]);
> +				"Link training phase %d done: %s\n",
> +				pattern, errors[error]);

It's probably not a big deal in this specific case, but in general it
can be useful to know how long the poll took. Any hope to enhance
regmap_read_poll_timeout() to return either the elapsed time or the
remaining timeout instead of 0 on success ?

>  			if (pattern == DP_TRAINING_PATTERN_1 && error == 0)
>  				break;
>  			if (pattern == DP_TRAINING_PATTERN_2) {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/9] drm/bridge: tc358767: Simplify polling in tc_link_training()

[Andrey Smirnov]

On Mon, Mar 4, 2019 at 4:30 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Andrey,
>
> Thank you for the patch.
>
> On Tue, Feb 26, 2019 at 11:36:05AM -0800, Andrey Smirnov wrote:
> > Replace explicit polling in tc_link_training() with equivalent call to
> > regmap_read_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: 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 | 14 +++++---------
> >  1 file changed, 5 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > index 6455e6484722..ea30cec4a0c3 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -735,7 +735,6 @@ static int tc_link_training(struct tc_data *tc, int pattern)
> >       const char * const *errors;
> >       u32 srcctrl = tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
> >                     DP0_SRCCTRL_AUTOCORRECT;
> > -     int timeout;
> >       int retry;
> >       u32 value;
> >       int ret;
> > @@ -765,20 +764,17 @@ static int tc_link_training(struct tc_data *tc, int pattern)
> >               tc_write(DP0CTL, DP_EN);
> >
> >               /* wait */
> > -             timeout = 1000;
> > -             do {
> > -                     tc_read(DP0_LTSTAT, &value);
> > -                     udelay(1);
> > -             } while ((!(value & LT_LOOPDONE)) && (--timeout));
> > -             if (timeout == 0) {
> > +             ret = regmap_read_poll_timeout(tc->regmap, DP0_LTSTAT, value,
> > +                                            value & LT_LOOPDONE, 1, 1000);
> > +             if (ret) {
> >                       dev_err(tc->dev, "Link training timeout!\n");
> >               } else {
> >                       int pattern = (value >> 11) & 0x3;
> >                       int error = (value >> 8) & 0x7;
> >
> >                       dev_dbg(tc->dev,
> > -                             "Link training phase %d done after %d uS: %s\n",
> > -                             pattern, 1000 - timeout, errors[error]);
> > +                             "Link training phase %d done: %s\n",
> > +                             pattern, errors[error]);
>
> It's probably not a big deal in this specific case, but in general it
> can be useful to know how long the poll took.

I don't disagree, but bear in mind that the way time is measured in
original loop assumes that tc_read, an I2C transaction over 100KHz
bus, takes insignificant amount of time compared to 1 uS delay. I
think original debug statement does a bit of a false advertising when
it presents a number of polling loop iterations as if it is time it
took to establish a link in microseconds.

> Any hope to enhance regmap_read_poll_timeout() to return either the elapsed time or the
> remaining timeout instead of 0 on success ?
>

I'd rather not go there. That'll take convincing Mark Brown to accept
that semantics change, then fixing all of the callers across the tree
via a separate patch series.

What if instead we just add an extra debug statement before link
training starts, so that duration of the process can be discerned from
logging timestamps? This does require user doing a bit of math by
hand, but it's actually more accurate timing info compared to original
and it doesn't require any API modification.

Thanks,
Andrey Smirnov

Re: [PATCH 5/9] drm/bridge: tc358767: Simplify polling in tc_link_training()

[Laurent Pinchart]

Hi Andrey,

On Mon, Mar 11, 2019 at 11:26:20AM -0700, Andrey Smirnov wrote:
> On Mon, Mar 4, 2019 at 4:30 AM Laurent Pinchart wrote:
> > On Tue, Feb 26, 2019 at 11:36:05AM -0800, Andrey Smirnov wrote:
> >> Replace explicit polling in tc_link_training() with equivalent call to
> >> regmap_read_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: 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 | 14 +++++---------
> >>  1 file changed, 5 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> >> index 6455e6484722..ea30cec4a0c3 100644
> >> --- a/drivers/gpu/drm/bridge/tc358767.c
> >> +++ b/drivers/gpu/drm/bridge/tc358767.c
> >> @@ -735,7 +735,6 @@ static int tc_link_training(struct tc_data *tc, int pattern)
> >>       const char * const *errors;
> >>       u32 srcctrl = tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
> >>                     DP0_SRCCTRL_AUTOCORRECT;
> >> -     int timeout;
> >>       int retry;
> >>       u32 value;
> >>       int ret;
> >> @@ -765,20 +764,17 @@ static int tc_link_training(struct tc_data *tc, int pattern)
> >>               tc_write(DP0CTL, DP_EN);
> >>
> >>               /* wait */
> >> -             timeout = 1000;
> >> -             do {
> >> -                     tc_read(DP0_LTSTAT, &value);
> >> -                     udelay(1);
> >> -             } while ((!(value & LT_LOOPDONE)) && (--timeout));
> >> -             if (timeout == 0) {
> >> +             ret = regmap_read_poll_timeout(tc->regmap, DP0_LTSTAT, value,
> >> +                                            value & LT_LOOPDONE, 1, 1000);
> >> +             if (ret) {
> >>                       dev_err(tc->dev, "Link training timeout!\n");
> >>               } else {
> >>                       int pattern = (value >> 11) & 0x3;
> >>                       int error = (value >> 8) & 0x7;
> >>
> >>                       dev_dbg(tc->dev,
> >> -                             "Link training phase %d done after %d uS: %s\n",
> >> -                             pattern, 1000 - timeout, errors[error]);
> >> +                             "Link training phase %d done: %s\n",
> >> +                             pattern, errors[error]);
> >
> > It's probably not a big deal in this specific case, but in general it
> > can be useful to know how long the poll took.
> 
> I don't disagree, but bear in mind that the way time is measured in
> original loop assumes that tc_read, an I2C transaction over 100KHz
> bus, takes insignificant amount of time compared to 1 uS delay. I
> think original debug statement does a bit of a false advertising when
> it presents a number of polling loop iterations as if it is time it
> took to establish a link in microseconds.
> 
> > Any hope to enhance regmap_read_poll_timeout() to return either the elapsed time or the
> > remaining timeout instead of 0 on success ?
> 
> I'd rather not go there. That'll take convincing Mark Brown to accept
> that semantics change, then fixing all of the callers across the tree
> via a separate patch series.
> 
> What if instead we just add an extra debug statement before link
> training starts, so that duration of the process can be discerned from
> logging timestamps? This does require user doing a bit of math by
> hand, but it's actually more accurate timing info compared to original
> and it doesn't require any API modification.

That works for me.

-- 
Regards,

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

[PATCH 6/9] drm/bridge: tc358767: Simplify error check in tc_aux_linx_setup()

[Andrey Smirnov]

Tc_poll_timeout() can only return -ETIMEDOUT, so checking for other
errors is not necessary. Drop it. 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: 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 | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index ea30cec4a0c3..54ff95f66e30 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -569,11 +569,10 @@ static int tc_aux_link_setup(struct tc_data *tc)
 
 	ret = tc_poll_timeout(tc->regmap, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1,
 			      1000);
-	if (ret == -ETIMEDOUT) {
+	if (ret) {
 		dev_err(tc->dev, "Timeout waiting for PHY to become ready");
 		return ret;
-	} else if (ret)
-		goto err;
+	}
 
 	/* Setup AUX link */
 	tc_write(DP0_AUXCFG1, AUX_RX_FILTER_EN |
-- 
2.20.1

Re: [PATCH 6/9] drm/bridge: tc358767: Simplify error check in tc_aux_linx_setup()

[Laurent Pinchart]

Hi Andrey,

Thank you for the patch.

On Tue, Feb 26, 2019 at 11:36:06AM -0800, Andrey Smirnov wrote:
> Tc_poll_timeout() can only return -ETIMEDOUT, so checking for other
> errors is not necessary. Drop it. No functional change intended.

Is that true given patch 1/9 in this series ? regmap_read_poll_timeout()
can also return an error from regmap_read().

> 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: 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 | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index ea30cec4a0c3..54ff95f66e30 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -569,11 +569,10 @@ static int tc_aux_link_setup(struct tc_data *tc)
>  
>  	ret = tc_poll_timeout(tc->regmap, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1,
>  			      1000);
> -	if (ret == -ETIMEDOUT) {
> +	if (ret) {
>  		dev_err(tc->dev, "Timeout waiting for PHY to become ready");
>  		return ret;
> -	} else if (ret)
> -		goto err;
> +	}
>  
>  	/* Setup AUX link */
>  	tc_write(DP0_AUXCFG1, AUX_RX_FILTER_EN |

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/9] drm/bridge: tc358767: Simplify error check in tc_aux_linx_setup()

[Andrey Smirnov]

On Mon, Mar 4, 2019 at 4:33 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Andrey,
>
> Thank you for the patch.
>
> On Tue, Feb 26, 2019 at 11:36:06AM -0800, Andrey Smirnov wrote:
> > Tc_poll_timeout() can only return -ETIMEDOUT, so checking for other
> > errors is not necessary. Drop it. No functional change intended.
>
> Is that true given patch 1/9 in this series ? regmap_read_poll_timeout()
> can also return an error from regmap_read().
>

Ugh, I misread "?:" in regmap_read_poll_timeout() code as "?", this
patch is incorrect. Sorry about that, will drop in v2.

Thanks,
Andrey Smirnov

[PATCH 7/9] 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>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
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 | 50 +++++++++++++------------------
 1 file changed, 20 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 54ff95f66e30..227f14cd2d3d 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -522,35 +522,42 @@ 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 value;
-	int ret;
-	u32 dp_phy_ctrl;
+	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;
+
 	/* Setup DP-PHY / PLL */
-	value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
-	tc_write(SYS_PLLPARAM, value);
+	ret = tc_set_syspllparam(tc);
+	if (ret)
+		return ret;
 
 	dp_phy_ctrl = BGREN | PWR_SW_EN | PHY_A0_EN;
 	if (tc->link.base.num_lanes == 2)
@@ -811,7 +818,6 @@ static int tc_main_link_setup(struct tc_data *tc)
 {
 	struct drm_dp_aux *aux = &tc->aux;
 	struct device *dev = tc->dev;
-	unsigned int rate;
 	u32 dp_phy_ctrl;
 	int timeout;
 	u32 value;
@@ -828,25 +834,9 @@ static int tc_main_link_setup(struct tc_data *tc)
 		 (tc->link.spread ? DP0_SRCCTRL_SSCG : 0) |
 		 ((tc->link.base.rate != 162000) ? DP0_SRCCTRL_BW27 : 0));
 
-	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;
-	tc_write(SYS_PLLPARAM, value);
+	ret = tc_set_syspllparam(tc);
+	if (ret)
+		return ret;
 
 	/* Setup Main Link */
 	dp_phy_ctrl = BGREN | PWR_SW_EN | PHY_A0_EN | PHY_M0_EN;
-- 
2.20.1

Re: [PATCH 7/9] drm/bridge: tc358767: Introduce tc_set_syspllparam()

[Laurent Pinchart]

Hi Andrey,

Thank you for the patch.

On Tue, Feb 26, 2019 at 11:36:07AM -0800, Andrey Smirnov wrote:
> 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>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> 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

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/tc358767.c | 50 +++++++++++++------------------
>  1 file changed, 20 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 54ff95f66e30..227f14cd2d3d 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -522,35 +522,42 @@ 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 value;
> -	int ret;
> -	u32 dp_phy_ctrl;
> +	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;
> +
>  	/* Setup DP-PHY / PLL */
> -	value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
> -	tc_write(SYS_PLLPARAM, value);
> +	ret = tc_set_syspllparam(tc);
> +	if (ret)
> +		return ret;
>  
>  	dp_phy_ctrl = BGREN | PWR_SW_EN | PHY_A0_EN;
>  	if (tc->link.base.num_lanes == 2)
> @@ -811,7 +818,6 @@ static int tc_main_link_setup(struct tc_data *tc)
>  {
>  	struct drm_dp_aux *aux = &tc->aux;
>  	struct device *dev = tc->dev;
> -	unsigned int rate;
>  	u32 dp_phy_ctrl;
>  	int timeout;
>  	u32 value;
> @@ -828,25 +834,9 @@ static int tc_main_link_setup(struct tc_data *tc)
>  		 (tc->link.spread ? DP0_SRCCTRL_SSCG : 0) |
>  		 ((tc->link.base.rate != 162000) ? DP0_SRCCTRL_BW27 : 0));
>  
> -	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;
> -	tc_write(SYS_PLLPARAM, value);
> +	ret = tc_set_syspllparam(tc);
> +	if (ret)
> +		return ret;
>  
>  	/* Setup Main Link */
>  	dp_phy_ctrl = BGREN | PWR_SW_EN | PHY_A0_EN | PHY_M0_EN;
> -- 
> 2.20.1
> 

-- 
Regards,

Laurent Pinchart

[PATCH 8/9] 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>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
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 | 36 +++++++++++++++++++------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 227f14cd2d3d..239b3aaa255d 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -390,10 +390,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)
@@ -487,11 +495,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
 		 (best_mul << 0));		/* Multiplier for PLL */
 
 	/* Force PLL parameter update and disable bypass */
-	tc_write(PXL_PLLCTRL, PLLUPDATE | PLLEN);
-
-	tc_wait_pll_lock(tc);
-
-	return 0;
+	return tc_pllupdate_pllen(tc, PXL_PLLCTRL);
 err:
 	return ret;
 }
@@ -568,11 +572,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
 	 */
-	tc_write(DP0_PLLCTRL, PLLUPDATE | PLLEN);
-	tc_wait_pll_lock(tc);
+	ret = tc_pllupdate_pllen(tc, DP0_PLLCTRL);
+	if (ret)
+		return ret;
 
-	tc_write(DP1_PLLCTRL, PLLUPDATE | PLLEN);
-	tc_wait_pll_lock(tc);
+	ret = tc_pllupdate_pllen(tc, DP1_PLLCTRL);
+	if (ret)
+		return ret;
 
 	ret = tc_poll_timeout(tc->regmap, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1,
 			      1000);
@@ -846,11 +852,13 @@ static int tc_main_link_setup(struct tc_data *tc)
 	msleep(100);
 
 	/* PLL setup */
-	tc_write(DP0_PLLCTRL, PLLUPDATE | PLLEN);
-	tc_wait_pll_lock(tc);
+	ret = tc_pllupdate_pllen(tc, DP0_PLLCTRL);
+	if (ret)
+		return ret;
 
-	tc_write(DP1_PLLCTRL, PLLUPDATE | PLLEN);
-	tc_wait_pll_lock(tc);
+	ret = tc_pllupdate_pllen(tc, DP1_PLLCTRL);
+	if (ret)
+		return ret;
 
 	/* PXL PLL setup */
 	if (tc_test_pattern) {
-- 
2.20.1

Re: [PATCH 8/9] drm/bridge: tc358767: Introduce tc_pllupdate_pllen()

[Laurent Pinchart]

Hi Andrey,

Thank you for the patch.

On Tue, Feb 26, 2019 at 11:36:08AM -0800, Andrey Smirnov wrote:
> Tc_wait_pll_lock() is always called as a follow-up for updating

s/Tc/tc/

> 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>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> 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

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/tc358767.c | 36 +++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 227f14cd2d3d..239b3aaa255d 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -390,10 +390,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)
> @@ -487,11 +495,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
>  		 (best_mul << 0));		/* Multiplier for PLL */
>  
>  	/* Force PLL parameter update and disable bypass */
> -	tc_write(PXL_PLLCTRL, PLLUPDATE | PLLEN);
> -
> -	tc_wait_pll_lock(tc);
> -
> -	return 0;
> +	return tc_pllupdate_pllen(tc, PXL_PLLCTRL);
>  err:
>  	return ret;
>  }
> @@ -568,11 +572,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
>  	 */
> -	tc_write(DP0_PLLCTRL, PLLUPDATE | PLLEN);
> -	tc_wait_pll_lock(tc);
> +	ret = tc_pllupdate_pllen(tc, DP0_PLLCTRL);
> +	if (ret)
> +		return ret;
>  
> -	tc_write(DP1_PLLCTRL, PLLUPDATE | PLLEN);
> -	tc_wait_pll_lock(tc);
> +	ret = tc_pllupdate_pllen(tc, DP1_PLLCTRL);
> +	if (ret)
> +		return ret;
>  
>  	ret = tc_poll_timeout(tc->regmap, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1,
>  			      1000);
> @@ -846,11 +852,13 @@ static int tc_main_link_setup(struct tc_data *tc)
>  	msleep(100);
>  
>  	/* PLL setup */
> -	tc_write(DP0_PLLCTRL, PLLUPDATE | PLLEN);
> -	tc_wait_pll_lock(tc);
> +	ret = tc_pllupdate_pllen(tc, DP0_PLLCTRL);
> +	if (ret)
> +		return ret;
>  
> -	tc_write(DP1_PLLCTRL, PLLUPDATE | PLLEN);
> -	tc_wait_pll_lock(tc);
> +	ret = tc_pllupdate_pllen(tc, DP1_PLLCTRL);
> +	if (ret)
> +		return ret;
>  
>  	/* PXL PLL setup */
>  	if (tc_test_pattern) {

-- 
Regards,

Laurent Pinchart

[PATCH 9/9] drm/bridge: tc358767: Drop tc_read() macro

[Andrey Smirnov]

There's only one place where tc_read() is used, so it doesn't save us
much. Drop it. 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: 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, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 239b3aaa255d..3c574f1569aa 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -240,12 +240,6 @@ static inline struct tc_data *connector_to_tc(struct drm_connector *c)
 		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 regmap *map, unsigned int addr,
 				  unsigned int cond_mask,
@@ -337,8 +331,13 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
 	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)
+					goto err;
+			}
 			buf[i] = tmp & 0xff;
 			tmp = tmp >> 8;
 			i++;
-- 
2.20.1

Re: [PATCH 9/9] drm/bridge: tc358767: Drop tc_read() macro

[Laurent Pinchart]

Hi Andrey,

Thank you for the patch.

On Tue, Feb 26, 2019 at 11:36:09AM -0800, Andrey Smirnov wrote:
> There's only one place where tc_read() is used, so it doesn't save us
> much. Drop it. 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: 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, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 239b3aaa255d..3c574f1569aa 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -240,12 +240,6 @@ static inline struct tc_data *connector_to_tc(struct drm_connector *c)
>  		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)

While I really like removing the goto from the macro, I think we should
either have accessors for both read and write, or remove them
completely. How about just dropping the goto in this patch, and decide
separately whether to keep accessors or remove them ?

>  static inline int tc_poll_timeout(struct regmap *map, unsigned int addr,
>  				  unsigned int cond_mask,
> @@ -337,8 +331,13 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
>  	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)
> +					goto err;

You can return ret directly here.

> +			}
>  			buf[i] = tmp & 0xff;
>  			tmp = tmp >> 8;
>  			i++;

-- 
Regards,

Laurent Pinchart