dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/bridge: it6505: Add range and selector_reg
@ 2023-03-27  4:48 Hsin-Yi Wang
  2023-03-27  8:32 ` Andrzej Hajda
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Hsin-Yi Wang @ 2023-03-27  4:48 UTC (permalink / raw)
  To: Robert Foss, Douglas Anderson
  Cc: xiazhengqiao, Neil Armstrong, Jonas Karlman, dri-devel,
	linux-kernel, Jernej Skrabec, Laurent Pinchart, Andrzej Hajda,
	kenneth.hung

There are 2 banks on it6505, and when writing to different bank,
REG_BANK_SEL needs to be set to the targeted bank. The current code set
this additionally, which causes a race condition when a process is
writing bank 0 registers while another process set the bank to 1. Set
ranges in regmap config so the regmap API would handle the bank changes.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
 drivers/gpu/drm/bridge/ite-it6505.c | 34 +++++++++++++++++++----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index bc451b2a77c28..abaf6e23775eb 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -258,12 +258,12 @@
 #define REG_AUD_INFOFRAM_SUM 0xFB
 
 /* the following six registers are in bank1 */
-#define REG_DRV_0_DB_800_MV 0x7E
-#define REG_PRE_0_DB_800_MV 0x7F
-#define REG_PRE_3P5_DB_800_MV 0x81
-#define REG_SSC_CTRL0 0x88
-#define REG_SSC_CTRL1 0x89
-#define REG_SSC_CTRL2 0x8A
+#define REG_DRV_0_DB_800_MV 0x17E
+#define REG_PRE_0_DB_800_MV 0x17F
+#define REG_PRE_3P5_DB_800_MV 0x181
+#define REG_SSC_CTRL0 0x188
+#define REG_SSC_CTRL1 0x189
+#define REG_SSC_CTRL2 0x18A
 
 #define RBR DP_LINK_BW_1_62
 #define HBR DP_LINK_BW_2_7
@@ -489,7 +489,7 @@ static const struct it6505_audio_sample_rate_map audio_sample_rate_map[] = {
 };
 
 static const struct regmap_range it6505_bridge_volatile_ranges[] = {
-	{ .range_min = 0, .range_max = 0xFF },
+	{ .range_min = 0, .range_max = 0x1FF },
 };
 
 static const struct regmap_access_table it6505_bridge_volatile_table = {
@@ -497,11 +497,27 @@ static const struct regmap_access_table it6505_bridge_volatile_table = {
 	.n_yes_ranges = ARRAY_SIZE(it6505_bridge_volatile_ranges),
 };
 
+static const struct regmap_range_cfg it6505_regmap_banks[] = {
+	{
+		.name = "it6505",
+		.range_min = 0x00,
+		.range_max = 0x1FF,
+		.selector_reg = REG_BANK_SEL,
+		.selector_mask = 0x1,
+		.selector_shift = 0,
+		.window_start = 0x00,
+		.window_len = 0x100,
+	},
+};
+
 static const struct regmap_config it6505_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
 	.volatile_table = &it6505_bridge_volatile_table,
 	.cache_type = REGCACHE_NONE,
+	.ranges = it6505_regmap_banks,
+	.num_ranges = ARRAY_SIZE(it6505_regmap_banks),
+	.max_register = 0x1FF,
 };
 
 static int it6505_read(struct it6505 *it6505, unsigned int reg_addr)
@@ -1267,7 +1283,6 @@ static void it6505_init(struct it6505 *it6505)
 	it6505_write(it6505, REG_TIME_STMP_CTRL,
 		     EN_SSC_GAT | EN_ENHANCE_VID_STMP | EN_ENHANCE_AUD_STMP);
 	it6505_write(it6505, REG_INFOFRAME_CTRL, 0x00);
-	it6505_write(it6505, REG_BANK_SEL, 0x01);
 	it6505_write(it6505, REG_DRV_0_DB_800_MV,
 		     afe_setting_table[it6505->afe_setting][0]);
 	it6505_write(it6505, REG_PRE_0_DB_800_MV,
@@ -1277,7 +1292,6 @@ static void it6505_init(struct it6505 *it6505)
 	it6505_write(it6505, REG_SSC_CTRL0, 0x9E);
 	it6505_write(it6505, REG_SSC_CTRL1, 0x1C);
 	it6505_write(it6505, REG_SSC_CTRL2, 0x42);
-	it6505_write(it6505, REG_BANK_SEL, 0x00);
 }
 
 static void it6505_video_disable(struct it6505 *it6505)
@@ -1506,11 +1520,9 @@ static void it6505_setup_ssc(struct it6505 *it6505)
 	it6505_set_bits(it6505, REG_TRAIN_CTRL0, SPREAD_AMP_5,
 			it6505->enable_ssc ? SPREAD_AMP_5 : 0x00);
 	if (it6505->enable_ssc) {
-		it6505_write(it6505, REG_BANK_SEL, 0x01);
 		it6505_write(it6505, REG_SSC_CTRL0, 0x9E);
 		it6505_write(it6505, REG_SSC_CTRL1, 0x1C);
 		it6505_write(it6505, REG_SSC_CTRL2, 0x42);
-		it6505_write(it6505, REG_BANK_SEL, 0x00);
 		it6505_write(it6505, REG_SP_CTRL0, 0x07);
 		it6505_write(it6505, REG_IP_CTRL1, 0x29);
 		it6505_write(it6505, REG_IP_CTRL2, 0x03);
-- 
2.40.0.348.gf938b09366-goog


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm/bridge: it6505: Add range and selector_reg
  2023-03-27  4:48 [PATCH] drm/bridge: it6505: Add range and selector_reg Hsin-Yi Wang
@ 2023-03-27  8:32 ` Andrzej Hajda
  2023-03-27  9:51 ` Neil Armstrong
  2023-03-27  9:55 ` Neil Armstrong
  2 siblings, 0 replies; 4+ messages in thread
From: Andrzej Hajda @ 2023-03-27  8:32 UTC (permalink / raw)
  To: Hsin-Yi Wang, Robert Foss, Douglas Anderson
  Cc: xiazhengqiao, Neil Armstrong, Jonas Karlman, kenneth.hung,
	linux-kernel, Jernej Skrabec, dri-devel, Laurent Pinchart

On 27.03.2023 06:48, Hsin-Yi Wang wrote:
> There are 2 banks on it6505, and when writing to different bank,
> REG_BANK_SEL needs to be set to the targeted bank. The current code set
> this additionally, which causes a race condition when a process is
> writing bank 0 registers while another process set the bank to 1. Set
> ranges in regmap config so the regmap API would handle the bank changes.
> 
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>

Looks cool, especially comparing to locking patches.

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej

> ---
>   drivers/gpu/drm/bridge/ite-it6505.c | 34 +++++++++++++++++++----------
>   1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index bc451b2a77c28..abaf6e23775eb 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -258,12 +258,12 @@
>   #define REG_AUD_INFOFRAM_SUM 0xFB
>   
>   /* the following six registers are in bank1 */
> -#define REG_DRV_0_DB_800_MV 0x7E
> -#define REG_PRE_0_DB_800_MV 0x7F
> -#define REG_PRE_3P5_DB_800_MV 0x81
> -#define REG_SSC_CTRL0 0x88
> -#define REG_SSC_CTRL1 0x89
> -#define REG_SSC_CTRL2 0x8A
> +#define REG_DRV_0_DB_800_MV 0x17E
> +#define REG_PRE_0_DB_800_MV 0x17F
> +#define REG_PRE_3P5_DB_800_MV 0x181
> +#define REG_SSC_CTRL0 0x188
> +#define REG_SSC_CTRL1 0x189
> +#define REG_SSC_CTRL2 0x18A
>   
>   #define RBR DP_LINK_BW_1_62
>   #define HBR DP_LINK_BW_2_7
> @@ -489,7 +489,7 @@ static const struct it6505_audio_sample_rate_map audio_sample_rate_map[] = {
>   };
>   
>   static const struct regmap_range it6505_bridge_volatile_ranges[] = {
> -	{ .range_min = 0, .range_max = 0xFF },
> +	{ .range_min = 0, .range_max = 0x1FF },
>   };
>   
>   static const struct regmap_access_table it6505_bridge_volatile_table = {
> @@ -497,11 +497,27 @@ static const struct regmap_access_table it6505_bridge_volatile_table = {
>   	.n_yes_ranges = ARRAY_SIZE(it6505_bridge_volatile_ranges),
>   };
>   
> +static const struct regmap_range_cfg it6505_regmap_banks[] = {
> +	{
> +		.name = "it6505",
> +		.range_min = 0x00,
> +		.range_max = 0x1FF,
> +		.selector_reg = REG_BANK_SEL,
> +		.selector_mask = 0x1,
> +		.selector_shift = 0,
> +		.window_start = 0x00,
> +		.window_len = 0x100,
> +	},
> +};
> +
>   static const struct regmap_config it6505_regmap_config = {
>   	.reg_bits = 8,
>   	.val_bits = 8,
>   	.volatile_table = &it6505_bridge_volatile_table,
>   	.cache_type = REGCACHE_NONE,
> +	.ranges = it6505_regmap_banks,
> +	.num_ranges = ARRAY_SIZE(it6505_regmap_banks),
> +	.max_register = 0x1FF,
>   };
>   
>   static int it6505_read(struct it6505 *it6505, unsigned int reg_addr)
> @@ -1267,7 +1283,6 @@ static void it6505_init(struct it6505 *it6505)
>   	it6505_write(it6505, REG_TIME_STMP_CTRL,
>   		     EN_SSC_GAT | EN_ENHANCE_VID_STMP | EN_ENHANCE_AUD_STMP);
>   	it6505_write(it6505, REG_INFOFRAME_CTRL, 0x00);
> -	it6505_write(it6505, REG_BANK_SEL, 0x01);
>   	it6505_write(it6505, REG_DRV_0_DB_800_MV,
>   		     afe_setting_table[it6505->afe_setting][0]);
>   	it6505_write(it6505, REG_PRE_0_DB_800_MV,
> @@ -1277,7 +1292,6 @@ static void it6505_init(struct it6505 *it6505)
>   	it6505_write(it6505, REG_SSC_CTRL0, 0x9E);
>   	it6505_write(it6505, REG_SSC_CTRL1, 0x1C);
>   	it6505_write(it6505, REG_SSC_CTRL2, 0x42);
> -	it6505_write(it6505, REG_BANK_SEL, 0x00);
>   }
>   
>   static void it6505_video_disable(struct it6505 *it6505)
> @@ -1506,11 +1520,9 @@ static void it6505_setup_ssc(struct it6505 *it6505)
>   	it6505_set_bits(it6505, REG_TRAIN_CTRL0, SPREAD_AMP_5,
>   			it6505->enable_ssc ? SPREAD_AMP_5 : 0x00);
>   	if (it6505->enable_ssc) {
> -		it6505_write(it6505, REG_BANK_SEL, 0x01);
>   		it6505_write(it6505, REG_SSC_CTRL0, 0x9E);
>   		it6505_write(it6505, REG_SSC_CTRL1, 0x1C);
>   		it6505_write(it6505, REG_SSC_CTRL2, 0x42);
> -		it6505_write(it6505, REG_BANK_SEL, 0x00);
>   		it6505_write(it6505, REG_SP_CTRL0, 0x07);
>   		it6505_write(it6505, REG_IP_CTRL1, 0x29);
>   		it6505_write(it6505, REG_IP_CTRL2, 0x03);


^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm/bridge: it6505: Add range and selector_reg
  2023-03-27  4:48 [PATCH] drm/bridge: it6505: Add range and selector_reg Hsin-Yi Wang
  2023-03-27  8:32 ` Andrzej Hajda
@ 2023-03-27  9:51 ` Neil Armstrong
  2023-03-27  9:55 ` Neil Armstrong
  2 siblings, 0 replies; 4+ messages in thread
From: Neil Armstrong @ 2023-03-27  9:51 UTC (permalink / raw)
  To: Hsin-Yi Wang, Robert Foss, Douglas Anderson
  Cc: xiazhengqiao, Andrzej Hajda, Jonas Karlman, dri-devel,
	linux-kernel, Jernej Skrabec, Laurent Pinchart, kenneth.hung

On 27/03/2023 06:48, Hsin-Yi Wang wrote:
> There are 2 banks on it6505, and when writing to different bank,
> REG_BANK_SEL needs to be set to the targeted bank. The current code set
> this additionally, which causes a race condition when a process is
> writing bank 0 registers while another process set the bank to 1. Set
> ranges in regmap config so the regmap API would handle the bank changes.
> 
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>

It's aligned on how we did on it66121


Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

> ---
>   drivers/gpu/drm/bridge/ite-it6505.c | 34 +++++++++++++++++++----------
>   1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index bc451b2a77c28..abaf6e23775eb 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -258,12 +258,12 @@
>   #define REG_AUD_INFOFRAM_SUM 0xFB
>   
>   /* the following six registers are in bank1 */
> -#define REG_DRV_0_DB_800_MV 0x7E
> -#define REG_PRE_0_DB_800_MV 0x7F
> -#define REG_PRE_3P5_DB_800_MV 0x81
> -#define REG_SSC_CTRL0 0x88
> -#define REG_SSC_CTRL1 0x89
> -#define REG_SSC_CTRL2 0x8A
> +#define REG_DRV_0_DB_800_MV 0x17E
> +#define REG_PRE_0_DB_800_MV 0x17F
> +#define REG_PRE_3P5_DB_800_MV 0x181
> +#define REG_SSC_CTRL0 0x188
> +#define REG_SSC_CTRL1 0x189
> +#define REG_SSC_CTRL2 0x18A
>   
>   #define RBR DP_LINK_BW_1_62
>   #define HBR DP_LINK_BW_2_7
> @@ -489,7 +489,7 @@ static const struct it6505_audio_sample_rate_map audio_sample_rate_map[] = {
>   };
>   
>   static const struct regmap_range it6505_bridge_volatile_ranges[] = {
> -	{ .range_min = 0, .range_max = 0xFF },
> +	{ .range_min = 0, .range_max = 0x1FF },
>   };
>   
>   static const struct regmap_access_table it6505_bridge_volatile_table = {
> @@ -497,11 +497,27 @@ static const struct regmap_access_table it6505_bridge_volatile_table = {
>   	.n_yes_ranges = ARRAY_SIZE(it6505_bridge_volatile_ranges),
>   };
>   
> +static const struct regmap_range_cfg it6505_regmap_banks[] = {
> +	{
> +		.name = "it6505",
> +		.range_min = 0x00,
> +		.range_max = 0x1FF,
> +		.selector_reg = REG_BANK_SEL,
> +		.selector_mask = 0x1,
> +		.selector_shift = 0,
> +		.window_start = 0x00,
> +		.window_len = 0x100,
> +	},
> +};
> +
>   static const struct regmap_config it6505_regmap_config = {
>   	.reg_bits = 8,
>   	.val_bits = 8,
>   	.volatile_table = &it6505_bridge_volatile_table,
>   	.cache_type = REGCACHE_NONE,
> +	.ranges = it6505_regmap_banks,
> +	.num_ranges = ARRAY_SIZE(it6505_regmap_banks),
> +	.max_register = 0x1FF,
>   };
>   
>   static int it6505_read(struct it6505 *it6505, unsigned int reg_addr)
> @@ -1267,7 +1283,6 @@ static void it6505_init(struct it6505 *it6505)
>   	it6505_write(it6505, REG_TIME_STMP_CTRL,
>   		     EN_SSC_GAT | EN_ENHANCE_VID_STMP | EN_ENHANCE_AUD_STMP);
>   	it6505_write(it6505, REG_INFOFRAME_CTRL, 0x00);
> -	it6505_write(it6505, REG_BANK_SEL, 0x01);
>   	it6505_write(it6505, REG_DRV_0_DB_800_MV,
>   		     afe_setting_table[it6505->afe_setting][0]);
>   	it6505_write(it6505, REG_PRE_0_DB_800_MV,
> @@ -1277,7 +1292,6 @@ static void it6505_init(struct it6505 *it6505)
>   	it6505_write(it6505, REG_SSC_CTRL0, 0x9E);
>   	it6505_write(it6505, REG_SSC_CTRL1, 0x1C);
>   	it6505_write(it6505, REG_SSC_CTRL2, 0x42);
> -	it6505_write(it6505, REG_BANK_SEL, 0x00);
>   }
>   
>   static void it6505_video_disable(struct it6505 *it6505)
> @@ -1506,11 +1520,9 @@ static void it6505_setup_ssc(struct it6505 *it6505)
>   	it6505_set_bits(it6505, REG_TRAIN_CTRL0, SPREAD_AMP_5,
>   			it6505->enable_ssc ? SPREAD_AMP_5 : 0x00);
>   	if (it6505->enable_ssc) {
> -		it6505_write(it6505, REG_BANK_SEL, 0x01);
>   		it6505_write(it6505, REG_SSC_CTRL0, 0x9E);
>   		it6505_write(it6505, REG_SSC_CTRL1, 0x1C);
>   		it6505_write(it6505, REG_SSC_CTRL2, 0x42);
> -		it6505_write(it6505, REG_BANK_SEL, 0x00);
>   		it6505_write(it6505, REG_SP_CTRL0, 0x07);
>   		it6505_write(it6505, REG_IP_CTRL1, 0x29);
>   		it6505_write(it6505, REG_IP_CTRL2, 0x03);


^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm/bridge: it6505: Add range and selector_reg
  2023-03-27  4:48 [PATCH] drm/bridge: it6505: Add range and selector_reg Hsin-Yi Wang
  2023-03-27  8:32 ` Andrzej Hajda
  2023-03-27  9:51 ` Neil Armstrong
@ 2023-03-27  9:55 ` Neil Armstrong
  2 siblings, 0 replies; 4+ messages in thread
From: Neil Armstrong @ 2023-03-27  9:55 UTC (permalink / raw)
  To: Robert Foss, Douglas Anderson, Hsin-Yi Wang
  Cc: xiazhengqiao, Andrzej Hajda, Jonas Karlman, dri-devel,
	linux-kernel, Jernej Skrabec, Laurent Pinchart, kenneth.hung

Hi,

On Mon, 27 Mar 2023 12:48:05 +0800, Hsin-Yi Wang wrote:
> There are 2 banks on it6505, and when writing to different bank,
> REG_BANK_SEL needs to be set to the targeted bank. The current code set
> this additionally, which causes a race condition when a process is
> writing bank 0 registers while another process set the bank to 1. Set
> ranges in regmap config so the regmap API would handle the bank changes.
> 
> 
> [...]

Thanks, Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next)

[1/1] drm/bridge: it6505: Add range and selector_reg
      https://cgit.freedesktop.org/drm/drm-misc/commit/?id=1a2dbf0303e8793444a57a2eec0c6b29523657d9

-- 
Neil


^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-03-27  9:55 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-27  4:48 [PATCH] drm/bridge: it6505: Add range and selector_reg Hsin-Yi Wang
2023-03-27  8:32 ` Andrzej Hajda
2023-03-27  9:51 ` Neil Armstrong
2023-03-27  9:55 ` Neil Armstrong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).