Re: [PATCH] drm/bridge: it6505: Add lock for it6505 i2c bank

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 24.03.2023 08:29, Hsin-Yi Wang wrote:
From: xiazhengqiao <xiazhengqiao@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>

When the i2c bank register (REG_BANK_SEL) is set to 1,
only the registers belong to bank 1 can be written.
There will be a race condition when a process is writing
bank 0 registers while another process set the bank to 1.
Add a mutex to handle regmap read/write locking for
registers in multiple i2c bank. Since the driver now
owns the lock, there's no need to use regmap API's lock.

Signed-off-by: xiazhengqiao <xiazhengqiao@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx>
---
  drivers/gpu/drm/bridge/ite-it6505.c | 72 ++++++++++++++++++++++-------
  1 file changed, 55 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index bc451b2a77c28..1a8dcc49fc1ee 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
@@ -414,12 +414,14 @@ struct it6505 {
  	struct mutex extcon_lock;
  	struct mutex mode_lock; /* used to bridge_detect */
  	struct mutex aux_lock; /* used to aux data transfers */
+	struct mutex bank_lock; /* used to protect i2c bank access */
  	struct regmap *regmap;
  	struct drm_display_mode source_output_mode;
  	struct drm_display_mode video_info;
  	struct notifier_block event_nb;
  	struct extcon_dev *extcon;
  	struct work_struct extcon_wq;
+	int bank_state;   /* 1 indicates bank 1, 0 indicates bank 0 */
  	int extcon_state;
  	enum drm_connector_status connector_status;
  	enum link_train_status link_state;
@@ -502,8 +504,22 @@ static const struct regmap_config it6505_regmap_config = {
  	.val_bits = 8,
  	.volatile_table = &it6505_bridge_volatile_table,
  	.cache_type = REGCACHE_NONE,
+	.disable_locking = true,
+	.can_sleep = true,
  };
+static int it6505_config_bank(struct it6505 *it6505, unsigned int reg_addr)
+{
+	int err = 0, target = !!(reg_addr > 0xff);
+
+	if (target != it6505->bank_state) {

It would be better to return if equal, this way you can avoid indentation.

+		err = regmap_write(it6505->regmap, REG_BANK_SEL, target);
+		if (!err)
+			it6505->bank_state = target;
+	}
+	return err;
+}
+
  static int it6505_read(struct it6505 *it6505, unsigned int reg_addr)
  {
  	unsigned int value;
@@ -513,7 +529,10 @@ static int it6505_read(struct it6505 *it6505, unsigned int reg_addr)
  	if (!it6505->powered)
  		return -ENODEV;
- err = regmap_read(it6505->regmap, reg_addr, &value);
+	mutex_lock(&it6505->bank_lock);
+	err = it6505_config_bank(it6505, reg_addr);
+	err |= regmap_read(it6505->regmap, reg_addr & 0xff, &value);

Shoudn't be rather if (!err) err = regmap_read(...) ?

+	mutex_unlock(&it6505->bank_lock);
  	if (err < 0) {
  		dev_err(dev, "read failed reg[0x%x] err: %d", reg_addr, err);
  		return err;
@@ -531,8 +550,10 @@ static int it6505_write(struct it6505 *it6505, unsigned int reg_addr,
  	if (!it6505->powered)
  		return -ENODEV;
- err = regmap_write(it6505->regmap, reg_addr, reg_val);
-
+	mutex_lock(&it6505->bank_lock);
+	err = it6505_config_bank(it6505, reg_addr);
+	err |= regmap_write(it6505->regmap, reg_addr & 0xff, reg_val);
+	mutex_unlock(&it6505->bank_lock);
  	if (err < 0) {
  		dev_err(dev, "write failed reg[0x%x] = 0x%x err = %d",
  			reg_addr, reg_val, err);
@@ -551,7 +572,10 @@ static int it6505_set_bits(struct it6505 *it6505, unsigned int reg,
  	if (!it6505->powered)
  		return -ENODEV;
- err = regmap_update_bits(it6505->regmap, reg, mask, value);
+	mutex_lock(&it6505->bank_lock);
+	err = it6505_config_bank(it6505, reg);
+	err |= regmap_update_bits(it6505->regmap, reg & 0xff, mask, value);
+	mutex_unlock(&it6505->bank_lock);
  	if (err < 0) {
  		dev_err(dev, "write reg[0x%x] = 0x%x mask = 0x%x failed err %d",
  			reg, value, mask, err);
@@ -892,7 +916,10 @@ static void it6505_aux_reset(struct it6505 *it6505)
static void it6505_reset_logic(struct it6505 *it6505)
  {
+	mutex_lock(&it6505->bank_lock);
+	it6505_config_bank(it6505, REG_RESET_CTRL);
  	regmap_write(it6505->regmap, REG_RESET_CTRL, ALL_LOGIC_RESET);
+	mutex_unlock(&it6505->bank_lock);

Why not call it6505_write ?
  	usleep_range(1000, 1500);
  }
@@ -972,9 +999,14 @@ static ssize_t it6505_aux_operation(struct it6505 *it6505,
  	it6505_write(it6505, REG_AUX_ADR_16_19,
  		     ((address >> 16) & 0x0F) | ((size - 1) << 4));
- if (cmd == CMD_AUX_NATIVE_WRITE)
+	if (cmd == CMD_AUX_NATIVE_WRITE) {
+		mutex_lock(&it6505->bank_lock);
+		it6505_config_bank(it6505, REG_AUX_OUT_DATA0);
  		regmap_bulk_write(it6505->regmap, REG_AUX_OUT_DATA0, buffer,
  				  size);
+		mutex_unlock(&it6505->bank_lock);
+	}
+
/* Aux Fire */
  	it6505_write(it6505, REG_AUX_CMD_REQ, cmd);
@@ -1197,9 +1229,12 @@ static int it6505_send_video_infoframe(struct it6505 *it6505,
  	if (err)
  		return err;
- err = regmap_bulk_write(it6505->regmap, REG_AVI_INFO_DB1,
+	mutex_lock(&it6505->bank_lock);
+	err = it6505_config_bank(it6505, REG_AVI_INFO_DB1);
+	err |= regmap_bulk_write(it6505->regmap, REG_AVI_INFO_DB1,
  				buffer + HDMI_INFOFRAME_HEADER_SIZE,
  				frame->length);
+	mutex_unlock(&it6505->bank_lock);

Common code with it6505_aux_operation, maybe it6505_bulk_write ?

Have you checked if regmap does not support banking? IMO it should be implemented there.

Regards
Andrzej

  	if (err)
  		return err;
@@ -1267,7 +1302,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 +1311,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 +1539,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);
@@ -1983,8 +2014,11 @@ static int it6505_setup_sha1_input(struct it6505 *it6505, u8 *sha1_input)
  	it6505_set_bits(it6505, REG_HDCP_CTRL2, HDCP_EN_M0_READ,
  			HDCP_EN_M0_READ);
- err = regmap_bulk_read(it6505->regmap, REG_M0_0_7,
+	mutex_lock(&it6505->bank_lock);
+	err = it6505_config_bank(it6505, REG_M0_0_7);
+	err |= regmap_bulk_read(it6505->regmap, REG_M0_0_7,
  			       sha1_input + msg_count, 8);
+	mutex_unlock(&it6505->bank_lock);
it6505_set_bits(it6505, REG_HDCP_CTRL2, HDCP_EN_M0_READ, 0x00); @@ -2577,6 +2611,9 @@ static int it6505_poweron(struct it6505 *it6505)
  	}
it6505->powered = true;
+	mutex_lock(&it6505->bank_lock);
+	it6505->bank_state = 0;
+	mutex_unlock(&it6505->bank_lock);
  	it6505_reset_logic(it6505);
  	it6505_int_mask_enable(it6505);
  	it6505_init(it6505);
@@ -3359,6 +3396,7 @@ static int it6505_i2c_probe(struct i2c_client *client)
  	mutex_init(&it6505->extcon_lock);
  	mutex_init(&it6505->mode_lock);
  	mutex_init(&it6505->aux_lock);
+	mutex_init(&it6505->bank_lock);
it6505->bridge.of_node = client->dev.of_node;
  	it6505->connector_status = connector_status_disconnected;




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux