From: Tomi Valkeinen <tomi.valkeinen@ti.com>
To: Andrey Smirnov <andrew.smirnov@gmail.com>,
<dri-devel@lists.freedesktop.org>
Cc: Archit Taneja <architt@codeaurora.org>,
Andrzej Hajda <a.hajda@samsung.com>,
Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
Andrey Gusakov <andrey.gusakov@cogentembedded.com>,
Philipp Zabel <p.zabel@pengutronix.de>,
Chris Healy <cphealy@gmail.com>,
Lucas Stach <l.stach@pengutronix.de>,
<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 04/15] drm/bridge: tc358767: Simplify tc_set_video_mode()
Date: Fri, 22 Mar 2019 12:19:41 +0200 [thread overview]
Message-ID: <0d08d4e0-ff1d-ae14-e8f8-696c7448fcb1@ti.com> (raw)
In-Reply-To: <20190322032901.12045-5-andrew.smirnov@gmail.com>
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
WARNING: multiple messages have this Message-ID (diff)
From: Tomi Valkeinen <tomi.valkeinen@ti.com>
To: Andrey Smirnov <andrew.smirnov@gmail.com>,
dri-devel@lists.freedesktop.org
Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>,
linux-kernel@vger.kernel.org,
Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
Chris Healy <cphealy@gmail.com>
Subject: Re: [PATCH v2 04/15] drm/bridge: tc358767: Simplify tc_set_video_mode()
Date: Fri, 22 Mar 2019 12:19:41 +0200 [thread overview]
Message-ID: <0d08d4e0-ff1d-ae14-e8f8-696c7448fcb1@ti.com> (raw)
In-Reply-To: <20190322032901.12045-5-andrew.smirnov@gmail.com>
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2019-03-22 10:19 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-22 3:28 [PATCH v2 00/15] tc358767 driver improvements Andrey Smirnov
2019-03-22 3:28 ` [PATCH v2 01/15] drm/bridge: tc358767: Simplify tc_poll_timeout() Andrey Smirnov
2019-03-22 10:12 ` Tomi Valkeinen
2019-03-22 10:12 ` Tomi Valkeinen
2019-03-22 3:28 ` [PATCH v2 02/15] drm/bridge: tc358767: Simplify polling in tc_main_link_setup() Andrey Smirnov
2019-03-22 10:13 ` Tomi Valkeinen
2019-03-22 10:13 ` Tomi Valkeinen
2019-03-22 3:28 ` [PATCH v2 03/15] drm/bridge: tc358767: Simplify polling in tc_link_training() Andrey Smirnov
2019-03-22 10:14 ` Tomi Valkeinen
2019-03-22 10:14 ` Tomi Valkeinen
2019-03-22 3:28 ` [PATCH v2 04/15] drm/bridge: tc358767: Simplify tc_set_video_mode() Andrey Smirnov
2019-03-22 10:19 ` Tomi Valkeinen [this message]
2019-03-22 10:19 ` Tomi Valkeinen
2019-03-22 17:24 ` Andrey Smirnov
2019-03-22 3:28 ` [PATCH v2 05/15] drm/bridge: tc358767: Drop custom tc_write()/tc_read() accessors Andrey Smirnov
2019-03-22 10:29 ` Tomi Valkeinen
2019-03-22 10:29 ` Tomi Valkeinen
2019-03-22 17:45 ` Andrey Smirnov
2019-03-22 3:28 ` [PATCH v2 06/15] drm/bridge: tc358767: Simplify AUX data read Andrey Smirnov
2019-03-22 3:28 ` [PATCH v2 07/15] drm/bridge: tc358767: Simplify AUX data write Andrey Smirnov
2019-03-22 10:51 ` Tomi Valkeinen
2019-03-22 10:51 ` Tomi Valkeinen
2019-03-22 17:25 ` Andrey Smirnov
2019-03-22 3:28 ` [PATCH v2 08/15] drm/bridge: tc358767: Increase AUX transfer length limit Andrey Smirnov
2019-03-22 13:14 ` Tomi Valkeinen
2019-03-22 13:14 ` Tomi Valkeinen
2019-03-22 19:01 ` Andrey Smirnov
2019-03-22 3:28 ` [PATCH v2 09/15] drm/bridge: tc358767: Use reported AUX transfer size Andrey Smirnov
2019-03-22 3:28 ` [PATCH v2 10/15] drm/bridge: tc358767: Add support for address-only I2C transfers Andrey Smirnov
2019-03-22 3:28 ` [PATCH v2 11/15] drm/bridge: tc358767: Introduce tc_set_syspllparam() Andrey Smirnov
2019-03-22 3:28 ` [PATCH v2 12/15] drm/bridge: tc358767: Introduce tc_pllupdate_pllen() Andrey Smirnov
2019-03-22 3:28 ` [PATCH v2 13/15] drm/bridge: tc358767: Simplify tc_aux_wait_busy() Andrey Smirnov
2019-03-22 3:29 ` [PATCH v2 14/15] drm/bridge: tc358767: Drop unnecessary 8 byte buffer Andrey Smirnov
2019-03-22 3:29 ` [PATCH v2 15/15] drm/bridge: tc358767: Replace magic number in tc_main_link_enable() Andrey Smirnov
2019-03-22 8:05 ` [PATCH v2 00/15] tc358767 driver improvements Tomi Valkeinen
2019-03-22 8:05 ` Tomi Valkeinen
2019-03-22 19:25 ` Andrey Smirnov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0d08d4e0-ff1d-ae14-e8f8-696c7448fcb1@ti.com \
--to=tomi.valkeinen@ti.com \
--cc=Laurent.pinchart@ideasonboard.com \
--cc=a.hajda@samsung.com \
--cc=andrew.smirnov@gmail.com \
--cc=andrey.gusakov@cogentembedded.com \
--cc=architt@codeaurora.org \
--cc=cphealy@gmail.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=l.stach@pengutronix.de \
--cc=linux-kernel@vger.kernel.org \
--cc=p.zabel@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.