Re: [PATCH] drm/bridge: analogix_dp: Remove duplicated code

[Yakir Yang]

Tomeu,

o_O

Ignore my previous email, seems I make a mistaken about the email 
format, and mess up the patch format.

I have move my previous comments to this one :-)

On 08/04/2016 02:23 PM, Tomeu Vizoso wrote:
> Remove code for reading the EDID and DPCD fields and use the helpers
> instead.
>
> Besides the obvious code reduction, other helpers are being added to the
> core that could be used in this driver and will be good to be able to
> use them instead of duplicating them.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
> Cc: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
> Cc: Mika Kahola <mika.kahola@xxxxxxxxx>
> Cc: Yakir Yang <ykk@xxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 390 +++++++++++----------
>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  39 +--
>   drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 324 -----------------
>   3 files changed, 201 insertions(+), 552 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 32715daf73cb..c81cb37e56b6 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -31,6 +31,7 @@
>   #include <drm/bridge/analogix_dp.h>
>   
>   #include "analogix_dp_core.h"
> +#include "analogix_dp_reg.h"
>   
>   #define to_dp(nm)	container_of(nm, struct analogix_dp_device, nm)
>   
> @@ -97,150 +98,21 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
>   	return 0;
>   }
>   
> -static unsigned char analogix_dp_calc_edid_check_sum(unsigned char *edid_data)
> -{
> -	int i;
> -	unsigned char sum = 0;
> -
> -	for (i = 0; i < EDID_BLOCK_LENGTH; i++)
> -		sum = sum + edid_data[i];
> -
> -	return sum;
> -}
> -
> -static int analogix_dp_read_edid(struct analogix_dp_device *dp)
> -{
> -	unsigned char *edid = dp->edid;
> -	unsigned int extend_block = 0;
> -	unsigned char sum;
> -	unsigned char test_vector;
> -	int retval;
> -
> -	/*
> -	 * EDID device address is 0x50.
> -	 * However, if necessary, you must have set upper address
> -	 * into E-EDID in I2C device, 0x30.
> -	 */
> -
> -	/* Read Extension Flag, Number of 128-byte EDID extension blocks */
> -	retval = analogix_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
> -						EDID_EXTENSION_FLAG,
> -						&extend_block);
> -	if (retval)
> -		return retval;
> -
> -	if (extend_block > 0) {
> -		dev_dbg(dp->dev, "EDID data includes a single extension!\n");
> -
> -		/* Read EDID data */
> -		retval = analogix_dp_read_bytes_from_i2c(dp,
> -						I2C_EDID_DEVICE_ADDR,
> -						EDID_HEADER_PATTERN,
> -						EDID_BLOCK_LENGTH,
> -						&edid[EDID_HEADER_PATTERN]);
> -		if (retval != 0) {
> -			dev_err(dp->dev, "EDID Read failed!\n");
> -			return -EIO;
> -		}
> -		sum = analogix_dp_calc_edid_check_sum(edid);
> -		if (sum != 0) {
> -			dev_err(dp->dev, "EDID bad checksum!\n");
> -			return -EIO;
> -		}
> -
> -		/* Read additional EDID data */
> -		retval = analogix_dp_read_bytes_from_i2c(dp,
> -				I2C_EDID_DEVICE_ADDR,
> -				EDID_BLOCK_LENGTH,
> -				EDID_BLOCK_LENGTH,
> -				&edid[EDID_BLOCK_LENGTH]);
> -		if (retval != 0) {
> -			dev_err(dp->dev, "EDID Read failed!\n");
> -			return -EIO;
> -		}
> -		sum = analogix_dp_calc_edid_check_sum(&edid[EDID_BLOCK_LENGTH]);
> -		if (sum != 0) {
> -			dev_err(dp->dev, "EDID bad checksum!\n");
> -			return -EIO;
> -		}
> -
> -		analogix_dp_read_byte_from_dpcd(dp, DP_TEST_REQUEST,
> -						&test_vector);
> -		if (test_vector & DP_TEST_LINK_EDID_READ) {
> -			analogix_dp_write_byte_to_dpcd(dp,
> -				DP_TEST_EDID_CHECKSUM,
> -				edid[EDID_BLOCK_LENGTH + EDID_CHECKSUM]);
> -			analogix_dp_write_byte_to_dpcd(dp,
> -				DP_TEST_RESPONSE,
> -				DP_TEST_EDID_CHECKSUM_WRITE);
> -		}
> -	} else {
> -		dev_info(dp->dev, "EDID data does not include any extensions.\n");
> -
> -		/* Read EDID data */
> -		retval = analogix_dp_read_bytes_from_i2c(dp,
> -				I2C_EDID_DEVICE_ADDR, EDID_HEADER_PATTERN,
> -				EDID_BLOCK_LENGTH, &edid[EDID_HEADER_PATTERN]);
> -		if (retval != 0) {
> -			dev_err(dp->dev, "EDID Read failed!\n");
> -			return -EIO;
> -		}
> -		sum = analogix_dp_calc_edid_check_sum(edid);
> -		if (sum != 0) {
> -			dev_err(dp->dev, "EDID bad checksum!\n");
> -			return -EIO;
> -		}
> -
> -		analogix_dp_read_byte_from_dpcd(dp, DP_TEST_REQUEST,
> -						&test_vector);
> -		if (test_vector & DP_TEST_LINK_EDID_READ) {
> -			analogix_dp_write_byte_to_dpcd(dp,
> -				DP_TEST_EDID_CHECKSUM, edid[EDID_CHECKSUM]);
> -			analogix_dp_write_byte_to_dpcd(dp,
> -				DP_TEST_RESPONSE, DP_TEST_EDID_CHECKSUM_WRITE);
> -		}
> -	}
> -
> -	dev_dbg(dp->dev, "EDID Read success!\n");
> -	return 0;
> -}
> -
> -static int analogix_dp_handle_edid(struct analogix_dp_device *dp)
> -{
> -	u8 buf[12];
> -	int i;
> -	int retval;
> -
> -	/* Read DPCD DP_DPCD_REV~RECEIVE_PORT1_CAP_1 */
> -	retval = analogix_dp_read_bytes_from_dpcd(dp, DP_DPCD_REV, 12, buf);
> -	if (retval)
> -		return retval;
> -
> -	/* Read EDID */
> -	for (i = 0; i < 3; i++) {
> -		retval = analogix_dp_read_edid(dp);
> -		if (!retval)
> -			break;
> -	}
> -
> -	return retval;
> -}
> -
>   static void
>   analogix_dp_enable_rx_to_enhanced_mode(struct analogix_dp_device *dp,
>   				       bool enable)
>   {
>   	u8 data;
>   
> -	analogix_dp_read_byte_from_dpcd(dp, DP_LANE_COUNT_SET, &data);
> +	drm_dp_dpcd_readb(&dp->aux, DP_LANE_COUNT_SET, &data);
>   
>   	if (enable)
> -		analogix_dp_write_byte_to_dpcd(dp, DP_LANE_COUNT_SET,
> -					       DP_LANE_COUNT_ENHANCED_FRAME_EN |
> -					       DPCD_LANE_COUNT_SET(data));
> +		drm_dp_dpcd_writeb(&dp->aux, DP_LANE_COUNT_SET,
> +				   DP_LANE_COUNT_ENHANCED_FRAME_EN |
> +					DPCD_LANE_COUNT_SET(data));
>   	else
> -		analogix_dp_write_byte_to_dpcd(dp, DP_LANE_COUNT_SET,
> -					       DPCD_LANE_COUNT_SET(data));
> +		drm_dp_dpcd_writeb(&dp->aux, DP_LANE_COUNT_SET,
> +				   DPCD_LANE_COUNT_SET(data));
>   }
>   
>   static int analogix_dp_is_enhanced_mode_available(struct analogix_dp_device *dp)
> @@ -248,7 +120,7 @@ static int analogix_dp_is_enhanced_mode_available(struct analogix_dp_device *dp)
>   	u8 data;
>   	int retval;
>   
> -	analogix_dp_read_byte_from_dpcd(dp, DP_MAX_LANE_COUNT, &data);
> +	drm_dp_dpcd_readb(&dp->aux, DP_MAX_LANE_COUNT, &data);
>   	retval = DPCD_ENHANCED_FRAME_CAP(data);
>   
>   	return retval;
> @@ -267,8 +139,8 @@ static void analogix_dp_training_pattern_dis(struct analogix_dp_device *dp)
>   {
>   	analogix_dp_set_training_pattern(dp, DP_NONE);
>   
> -	analogix_dp_write_byte_to_dpcd(dp, DP_TRAINING_PATTERN_SET,
> -				       DP_TRAINING_PATTERN_DISABLE);
> +	drm_dp_dpcd_writeb(&dp->aux, DP_TRAINING_PATTERN_SET,
> +			   DP_TRAINING_PATTERN_DISABLE);
>   }
>   
>   static void
> @@ -313,8 +185,8 @@ static int analogix_dp_link_start(struct analogix_dp_device *dp)
>   	/* Setup RX configuration */
>   	buf[0] = dp->link_train.link_rate;
>   	buf[1] = dp->link_train.lane_count;
> -	retval = analogix_dp_write_bytes_to_dpcd(dp, DP_LINK_BW_SET, 2, buf);
> -	if (retval)
> +	retval = drm_dp_dpcd_write(&dp->aux, DP_LINK_BW_SET, buf, 2);
> +	if (retval < 0)
>   		return retval;
>   
>   	/* Set TX pre-emphasis to minimum */
> @@ -338,20 +210,22 @@ static int analogix_dp_link_start(struct analogix_dp_device *dp)
>   	analogix_dp_set_training_pattern(dp, TRAINING_PTN1);
>   
>   	/* Set RX training pattern */
> -	retval = analogix_dp_write_byte_to_dpcd(dp,
> -			DP_TRAINING_PATTERN_SET,
> -			DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_1);
> -	if (retval)
> +	retval = drm_dp_dpcd_writeb(&dp->aux, DP_TRAINING_PATTERN_SET,
> +				    DP_LINK_SCRAMBLING_DISABLE |
> +					DP_TRAINING_PATTERN_1);

'DP_TRAINING_PATTERN_1'  need align with 'DP_LINK_SCRAMBLING_DISABLE'

> +	if (retval < 0)
>   		return retval;
>   
>   	for (lane = 0; lane < lane_count; lane++)
>   		buf[lane] = DP_TRAIN_PRE_EMPH_LEVEL_0 |
>   			    DP_TRAIN_VOLTAGE_SWING_LEVEL_0;
>   
> -	retval = analogix_dp_write_bytes_to_dpcd(dp, DP_TRAINING_LANE0_SET,
> -						 lane_count, buf);
> +	retval = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET, buf,
> +				   lane_count);
> +	if (retval < 0)
> +		return retval;
>   
> -	return retval;
> +	return 0;
>   }
>   
>   static unsigned char analogix_dp_get_lane_status(u8 link_status[2], int lane)
> @@ -503,25 +377,23 @@ static int analogix_dp_process_clock_recovery(struct analogix_dp_device *dp)
>   
>   	lane_count = dp->link_train.lane_count;
>   
> -	retval =  analogix_dp_read_bytes_from_dpcd(dp,
> -			DP_LANE0_1_STATUS, 2, link_status);
> -	if (retval)
> +	retval = drm_dp_dpcd_read(&dp->aux, DP_LANE0_1_STATUS, link_status, 2);
> +	if (retval < 0)
>   		return retval;
>   
> -	retval =  analogix_dp_read_bytes_from_dpcd(dp,
> -			DP_ADJUST_REQUEST_LANE0_1, 2, adjust_request);
> -	if (retval)
> +	retval = drm_dp_dpcd_read(&dp->aux, DP_ADJUST_REQUEST_LANE0_1,
> +				  adjust_request, 2);
> +	if (retval < 0)
>   		return retval;
>   
>   	if (analogix_dp_clock_recovery_ok(link_status, lane_count) == 0) {
>   		/* set training pattern 2 for EQ */
>   		analogix_dp_set_training_pattern(dp, TRAINING_PTN2);
>   
> -		retval = analogix_dp_write_byte_to_dpcd(dp,
> -				DP_TRAINING_PATTERN_SET,
> -				DP_LINK_SCRAMBLING_DISABLE |
> -				DP_TRAINING_PATTERN_2);
> -		if (retval)
> +		retval = drm_dp_dpcd_writeb(&dp->aux, DP_TRAINING_PATTERN_SET,
> +					    DP_LINK_SCRAMBLING_DISABLE |
> +						DP_TRAINING_PATTERN_2);

'DP_TRAINING_PATTERN_2'  need align with 'DP_LINK_SCRAMBLING_DISABLE'

> +		if (retval < 0)
>   			return retval;
>   
>   		dev_info(dp->dev, "Link Training Clock Recovery success\n");
> @@ -559,13 +431,12 @@ static int analogix_dp_process_clock_recovery(struct analogix_dp_device *dp)
>   		analogix_dp_set_lane_link_training(dp,
>   			dp->link_train.training_lane[lane], lane);
>   
> -	retval = analogix_dp_write_bytes_to_dpcd(dp,
> -			DP_TRAINING_LANE0_SET, lane_count,
> -			dp->link_train.training_lane);
> -	if (retval)
> +	retval = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET,
> +				   dp->link_train.training_lane, lane_count);
> +	if (retval < 0)
>   		return retval;
>   
> -	return retval;
> +	return 0;
>   }
>   
>   static int analogix_dp_process_equalizer_training(struct analogix_dp_device *dp)
> @@ -578,9 +449,8 @@ static int analogix_dp_process_equalizer_training(struct analogix_dp_device *dp)
>   
>   	lane_count = dp->link_train.lane_count;
>   
> -	retval = analogix_dp_read_bytes_from_dpcd(dp,
> -			DP_LANE0_1_STATUS, 2, link_status);
> -	if (retval)
> +	retval = drm_dp_dpcd_read(&dp->aux, DP_LANE0_1_STATUS, link_status, 2);
> +	if (retval < 0)
>   		return retval;
>   
>   	if (analogix_dp_clock_recovery_ok(link_status, lane_count)) {
> @@ -588,14 +458,13 @@ static int analogix_dp_process_equalizer_training(struct analogix_dp_device *dp)
>   		return -EIO;
>   	}
>   
> -	retval = analogix_dp_read_bytes_from_dpcd(dp,
> -			DP_ADJUST_REQUEST_LANE0_1, 2, adjust_request);
> -	if (retval)
> +	retval = drm_dp_dpcd_read(&dp->aux, DP_ADJUST_REQUEST_LANE0_1, adjust_request, 2);
> +	if (retval < 0)
>   		return retval;
>   
> -	retval = analogix_dp_read_byte_from_dpcd(dp,
> -			DP_LANE_ALIGN_STATUS_UPDATED, &link_align);
> -	if (retval)
> +	retval = drm_dp_dpcd_readb(&dp->aux, DP_LANE_ALIGN_STATUS_UPDATED,
> +				   &link_align);
> +	if (retval < 0)
>   		return retval;
>   
>   	analogix_dp_get_adjust_training_lane(dp, adjust_request);
> @@ -636,10 +505,12 @@ static int analogix_dp_process_equalizer_training(struct analogix_dp_device *dp)
>   		analogix_dp_set_lane_link_training(dp,
>   			dp->link_train.training_lane[lane], lane);
>   
> -	retval = analogix_dp_write_bytes_to_dpcd(dp, DP_TRAINING_LANE0_SET,
> -			lane_count, dp->link_train.training_lane);
> +	retval = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET,
> +				   dp->link_train.training_lane, lane_count);
> +	if (retval < 0)
> +		return retval;
>   
> -	return retval;
> +	return 0;
>   }
>   
>   static void analogix_dp_get_max_rx_bandwidth(struct analogix_dp_device *dp,
> @@ -653,7 +524,7 @@ static void analogix_dp_get_max_rx_bandwidth(struct analogix_dp_device *dp,
>   	 * For DP rev.1.2, Maximum link rate of Main Link lanes
>   	 * 0x06 = 1.62 Gbps, 0x0a = 2.7 Gbps, 0x14 = 5.4Gbps
>   	 */
> -	analogix_dp_read_byte_from_dpcd(dp, DP_MAX_LINK_RATE, &data);
> +	drm_dp_dpcd_readb(&dp->aux, DP_MAX_LINK_RATE, &data);
>   	*bandwidth = data;
>   }
>   
> @@ -666,7 +537,7 @@ static void analogix_dp_get_max_rx_lane_count(struct analogix_dp_device *dp,
>   	 * For DP rev.1.1, Maximum number of Main Link lanes
>   	 * 0x01 = 1 lane, 0x02 = 2 lanes, 0x04 = 4 lanes
>   	 */
> -	analogix_dp_read_byte_from_dpcd(dp, DP_MAX_LANE_COUNT, &data);
> +	drm_dp_dpcd_readb(&dp->aux, DP_MAX_LANE_COUNT, &data);
>   	*lane_count = DPCD_MAX_LANE_COUNT(data);
>   }
>   
> @@ -835,19 +706,15 @@ static void analogix_dp_enable_scramble(struct analogix_dp_device *dp,
>   	if (enable) {
>   		analogix_dp_enable_scrambling(dp);
>   
> -		analogix_dp_read_byte_from_dpcd(dp, DP_TRAINING_PATTERN_SET,
> -						&data);
> -		analogix_dp_write_byte_to_dpcd(dp,
> -			DP_TRAINING_PATTERN_SET,
> -			(u8)(data & ~DP_LINK_SCRAMBLING_DISABLE));
> +		drm_dp_dpcd_readb(&dp->aux, DP_TRAINING_PATTERN_SET, &data);
> +		drm_dp_dpcd_writeb(&dp->aux, DP_TRAINING_PATTERN_SET,
> +				   (u8)(data & ~DP_LINK_SCRAMBLING_DISABLE));
>   	} else {
>   		analogix_dp_disable_scrambling(dp);
>   
> -		analogix_dp_read_byte_from_dpcd(dp, DP_TRAINING_PATTERN_SET,
> -						&data);
> -		analogix_dp_write_byte_to_dpcd(dp,
> -			DP_TRAINING_PATTERN_SET,
> -			(u8)(data | DP_LINK_SCRAMBLING_DISABLE));
> +		drm_dp_dpcd_readb(&dp->aux, DP_TRAINING_PATTERN_SET, &data);
> +		drm_dp_dpcd_writeb(&dp->aux, DP_TRAINING_PATTERN_SET,
> +				   (u8)(data | DP_LINK_SCRAMBLING_DISABLE));
>   	}
>   }
>   
> @@ -926,12 +793,11 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
>   int analogix_dp_get_modes(struct drm_connector *connector)
>   {
>   	struct analogix_dp_device *dp = to_dp(connector);
> -	struct edid *edid = (struct edid *)dp->edid;
>   	int num_modes = 0;
>   
> -	if (analogix_dp_handle_edid(dp) == 0) {
> -		drm_mode_connector_update_edid_property(&dp->connector, edid);
> -		num_modes += drm_add_edid_modes(&dp->connector, edid);
> +	if ((dp->edid = drm_get_edid(connector, &dp->aux.ddc))) {
> +		drm_mode_connector_update_edid_property(&dp->connector, dp->edid);
> +		num_modes += drm_add_edid_modes(&dp->connector, dp->edid);
>   	}

You could remove the '->edid' variable from struct analogix_dp_device, 
cause we don't need this information at other places. You need to free 
the edid after parsed it, cause each time when you call drm_get_edid() 
helper, it would malloc a new space for 'edid'. Besides I would prefer 
move the statement out of the condition.

	struct edid *edid;

	edid = drm_get_edid(connector, &dp->aux.ddc);
	if (edid) {
		drm_mode_connector_update_edid_property(&dp->connector, dp->edid);
		num_modes += drm_add_edid_modes(&dp->connector, dp->edid);
		kfree(edid);
	}

>   
>   	if (dp->plat_data->panel)
> @@ -984,6 +850,138 @@ static const struct drm_connector_funcs analogix_dp_connector_funcs = {
>   	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>   };
>   
> +static ssize_t analogix_dpaux_transfer(struct drm_dp_aux *aux,
> +				       struct drm_dp_aux_msg *msg)
> +{
> +	struct analogix_dp_device *dp = to_dp(aux);
> +	u32 reg;
> +	u8 *buffer = msg->buffer;
> +	int timeout_loop = 0;
> +	unsigned int i;
> +	int retval = 0;
> +
> +	/* Buffer size of AUX CH is 16 bytes */
> +	if (WARN_ON(msg->size > 16))
> +		return -E2BIG;
> +
> +	/* Clear AUX CH data buffer */
> +	reg = BUF_CLR;
> +	writel(reg, dp->reg_base + ANALOGIX_DP_BUFFER_DATA_CTL);
> +
> +	switch (msg->request & ~DP_AUX_I2C_MOT) {
> +	case DP_AUX_I2C_WRITE:
> +		reg = AUX_TX_COMM_WRITE | AUX_TX_COMM_I2C_TRANSACTION;
> +		if (msg->request & DP_AUX_I2C_MOT)
> +			reg |= AUX_TX_COMM_MOT;
> +
> +		break;
> +
> +	case DP_AUX_I2C_READ:
> +		reg = AUX_TX_COMM_READ | AUX_TX_COMM_I2C_TRANSACTION;
> +		if (msg->request & DP_AUX_I2C_MOT)
> +			reg |= AUX_TX_COMM_MOT;
> +
> +		break;
> +
> +	case DP_AUX_NATIVE_WRITE:
> +		reg = AUX_TX_COMM_WRITE | AUX_TX_COMM_DP_TRANSACTION;
> +		break;
> +
> +	case DP_AUX_NATIVE_READ:
> +		reg = AUX_TX_COMM_READ | AUX_TX_COMM_DP_TRANSACTION;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	reg |= AUX_LENGTH(msg->size);
> +	writel(reg, dp->reg_base + ANALOGIX_DP_AUX_CH_CTL_1);
> +
> +	/* Select DPCD device address */
> +	reg = AUX_ADDR_7_0(msg->address);
> +	writel(reg, dp->reg_base + ANALOGIX_DP_AUX_ADDR_7_0);
> +	reg = AUX_ADDR_15_8(msg->address);
> +	writel(reg, dp->reg_base + ANALOGIX_DP_AUX_ADDR_15_8);
> +	reg = AUX_ADDR_19_16(msg->address);
> +	writel(reg, dp->reg_base + ANALOGIX_DP_AUX_ADDR_19_16);
> +
> +	if ((msg->request & DP_AUX_I2C_READ) == 0) {
> +		for (i = 0; i < msg->size; i++) {
> +			reg = buffer[i];
> +			writel(reg, dp->reg_base + ANALOGIX_DP_BUF_DATA_0 +
> +			       4 * i);
> +			retval++;
> +		}
> +	}
> +
> +	/* Enable AUX CH operation */
> +	reg = 0;
> +
> +	/* Zero-sized messages specify address-only transactions. */
> +	if (msg->size < 1)
> +		reg |= ADDR_ONLY;
> +
> +	reg |= AUX_EN;
> +	writel(reg, dp->reg_base + ANALOGIX_DP_AUX_CH_CTL_2);
> +
> +	/* Is AUX CH command reply received? */
> +	/* TODO: Wait for an interrupt instead of looping? */
> +	reg = readl(dp->reg_base + ANALOGIX_DP_INT_STA);
> +	while (!(reg & RPLY_RECEIV)) {
> +		timeout_loop++;
> +		if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
> +			dev_err(dp->dev, "AUX CH command reply failed!\n");
> +			return -ETIMEDOUT;
> +		}
> +		reg = readl(dp->reg_base + ANALOGIX_DP_INT_STA);
> +		usleep_range(10, 11);
> +	}
> +
> +	/* Clear interrupt source for AUX CH command reply */
> +	writel(RPLY_RECEIV, dp->reg_base + ANALOGIX_DP_INT_STA);
> +
> +	/* Clear interrupt source for AUX CH access error */
> +	reg = readl(dp->reg_base + ANALOGIX_DP_INT_STA);
> +	if (reg & AUX_ERR) {
> +		writel(AUX_ERR, dp->reg_base + ANALOGIX_DP_INT_STA);
> +		return -EREMOTEIO;
> +	}
> +
> +	/* Check AUX CH error access status */
> +	reg = readl(dp->reg_base + ANALOGIX_DP_AUX_CH_STA);
> +	if ((reg & AUX_STATUS_MASK) != 0) {
> +		dev_err(dp->dev, "AUX CH error happens: %d\n\n",
> +			reg & AUX_STATUS_MASK);
> +		return -EREMOTEIO;
> +	}
> +
> +	if (msg->request & DP_AUX_I2C_READ) {
> +		for (i = 0; i < msg->size; i++) {
> +			reg = readl(dp->reg_base + ANALOGIX_DP_BUF_DATA_0 +
> +				    4 * i);
> +			buffer[i] = (unsigned char)reg;
> +			retval++;
> +		}
> +	}
> +
> +	/* Check if Rx sends defer */
> +	reg = readl(dp->reg_base + ANALOGIX_DP_AUX_RX_COMM);
> +	if (reg == AUX_RX_COMM_AUX_DEFER)
> +		msg->reply = DP_AUX_NATIVE_REPLY_DEFER;
> +	else if (reg == AUX_RX_COMM_I2C_DEFER)
> +		msg->reply = DP_AUX_I2C_REPLY_DEFER;
> +	else if ((msg->request & ~DP_AUX_I2C_MOT) == DP_AUX_I2C_WRITE ||
> +		 (msg->request & ~DP_AUX_I2C_MOT) == DP_AUX_I2C_READ)
> +		msg->reply = DP_AUX_I2C_REPLY_ACK;
> +	else if ((msg->request & ~DP_AUX_I2C_MOT) == DP_AUX_NATIVE_WRITE ||
> +		 (msg->request & ~DP_AUX_I2C_MOT) == DP_AUX_NATIVE_READ)
> +		msg->reply = DP_AUX_NATIVE_REPLY_ACK;
> +
> +	return retval;
> +}
> +

Would you like to move this function to 'analogix_dp_reg.c' ?

> +
>   static int analogix_dp_bridge_attach(struct drm_bridge *bridge)
>   {
>   	struct analogix_dp_device *dp = bridge->driver_private;
> @@ -1355,6 +1353,14 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
>   	dp->drm_dev = drm_dev;
>   	dp->encoder = dp->plat_data->encoder;
>   
> +	dp->aux.name = "DP-AUX";
> +	dp->aux.transfer = analogix_dpaux_transfer;
> +	dp->aux.dev = &pdev->dev;
> +
> +	ret = drm_dp_aux_register(&dp->aux);
> +	if (ret)
> +		goto err_disable_pm_runtime;
> +
>   	ret = analogix_dp_create_bridge(drm_dev, dp);
>   	if (ret) {
>   		DRM_ERROR("failed to create bridge (%d)\n", ret);
> @@ -1384,6 +1390,8 @@ void analogix_dp_unbind(struct device *dev, struct device *master,
>   	}
>   
>   	pm_runtime_disable(dev);
> +
> +	kfree(dp->edid);

Could remove this free code.


Thanks,
- Yakir

>   }
>   EXPORT_SYMBOL_GPL(analogix_dp_unbind);
>   
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> index b45638043ec4..27eb27a71802 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> @@ -20,15 +20,6 @@
>   #define MAX_CR_LOOP 5
>   #define MAX_EQ_LOOP 5
>   
> -/* I2C EDID Chip ID, Slave Address */
> -#define I2C_EDID_DEVICE_ADDR			0x50
> -#define I2C_E_EDID_DEVICE_ADDR			0x30
> -
> -#define EDID_BLOCK_LENGTH			0x80
> -#define EDID_HEADER_PATTERN			0x00
> -#define EDID_EXTENSION_FLAG			0x7e
> -#define EDID_CHECKSUM				0x7f
> -
>   /* DP_MAX_LANE_COUNT */
>   #define DPCD_ENHANCED_FRAME_CAP(x)		(((x) >> 7) & 0x1)
>   #define DPCD_MAX_LANE_COUNT(x)			((x) & 0x1f)
> @@ -166,6 +157,7 @@ struct analogix_dp_device {
>   	struct drm_device	*drm_dev;
>   	struct drm_connector	connector;
>   	struct drm_bridge	*bridge;
> +	struct drm_dp_aux       aux;
>   	struct clk		*clock;
>   	unsigned int		irq;
>   	void __iomem		*reg_base;
> @@ -176,7 +168,7 @@ struct analogix_dp_device {
>   	int			dpms_mode;
>   	int			hpd_gpio;
>   	bool                    force_hpd;
> -	unsigned char           edid[EDID_BLOCK_LENGTH * 2];
> +	struct edid		*edid;
>   
>   	struct analogix_dp_plat_data *plat_data;
>   };
> @@ -206,33 +198,6 @@ void analogix_dp_reset_aux(struct analogix_dp_device *dp);
>   void analogix_dp_init_aux(struct analogix_dp_device *dp);
>   int analogix_dp_get_plug_in_status(struct analogix_dp_device *dp);
>   void analogix_dp_enable_sw_function(struct analogix_dp_device *dp);
> -int analogix_dp_start_aux_transaction(struct analogix_dp_device *dp);
> -int analogix_dp_write_byte_to_dpcd(struct analogix_dp_device *dp,
> -				   unsigned int reg_addr,
> -				   unsigned char data);
> -int analogix_dp_read_byte_from_dpcd(struct analogix_dp_device *dp,
> -				    unsigned int reg_addr,
> -				    unsigned char *data);
> -int analogix_dp_write_bytes_to_dpcd(struct analogix_dp_device *dp,
> -				    unsigned int reg_addr,
> -				    unsigned int count,
> -				    unsigned char data[]);
> -int analogix_dp_read_bytes_from_dpcd(struct analogix_dp_device *dp,
> -				     unsigned int reg_addr,
> -				     unsigned int count,
> -				     unsigned char data[]);
> -int analogix_dp_select_i2c_device(struct analogix_dp_device *dp,
> -				  unsigned int device_addr,
> -				  unsigned int reg_addr);
> -int analogix_dp_read_byte_from_i2c(struct analogix_dp_device *dp,
> -				   unsigned int device_addr,
> -				   unsigned int reg_addr,
> -				   unsigned int *data);
> -int analogix_dp_read_bytes_from_i2c(struct analogix_dp_device *dp,
> -				    unsigned int device_addr,
> -				    unsigned int reg_addr,
> -				    unsigned int count,
> -				    unsigned char edid[]);
>   void analogix_dp_set_link_bandwidth(struct analogix_dp_device *dp, u32 bwtype);
>   void analogix_dp_get_link_bandwidth(struct analogix_dp_device *dp, u32 *bwtype);
>   void analogix_dp_set_lane_count(struct analogix_dp_device *dp, u32 count);
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index 48030f0cf497..eac61aa4224f 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -585,330 +585,6 @@ int analogix_dp_write_byte_to_dpcd(struct analogix_dp_device *dp,
>   	return retval;
>   }
>   
> -int analogix_dp_read_byte_from_dpcd(struct analogix_dp_device *dp,
> -				    unsigned int reg_addr,
> -				    unsigned char *data)
> -{
> -	u32 reg;
> -	int i;
> -	int retval;
> -
> -	for (i = 0; i < 3; i++) {
> -		/* Clear AUX CH data buffer */
> -		reg = BUF_CLR;
> -		writel(reg, dp->reg_base + ANALOGIX_DP_BUFFER_DATA_CTL);
> -
> -		/* Select DPCD device address */
> -		reg = AUX_ADDR_7_0(reg_addr);
> -		writel(reg, dp->reg_base + ANALOGIX_DP_AUX_ADDR_7_0);
> -		reg = AUX_ADDR_15_8(reg_addr);
> -		writel(reg, dp->reg_base + ANALOGIX_DP_AUX_ADDR_15_8);
> -		reg = AUX_ADDR_19_16(reg_addr);
> -		writel(reg, dp->reg_base + ANALOGIX_DP_AUX_ADDR_19_16);
> -
> -		/*
> -		 * Set DisplayPort transaction and read 1 byte
> -		 * If bit 3 is 1, DisplayPort transaction.
> -		 * If Bit 3 is 0, I2C transaction.
> -		 */
> -		reg = AUX_TX_COMM_DP_TRANSACTION | AUX_TX_COMM_READ;
> -		writel(reg, dp->reg_base + ANALOGIX_DP_AUX_CH_CTL_1);
> -
> -		/* Start AUX transaction */
> -		retval = analogix_dp_start_aux_transaction(dp);
> -		if (retval == 0)
> -			break;
> -
> -		dev_dbg(dp->dev, "%s: Aux Transaction fail!\n", __func__);
> -	}
> -
> -	/* Read data buffer */
> -	reg = readl(dp->reg_base + ANALOGIX_DP_BUF_DATA_0);
> -	*data = (unsigned char)(reg & 0xff);
> -
> -	return retval;
> -}
> -
> -int analogix_dp_write_bytes_to_dpcd(struct analogix_dp_device *dp,
> -				    unsigned int reg_addr,
> -				    unsigned int count,
> -				    unsigned char data[])
> -{
> -	u32 reg;
> -	unsigned int start_offset;
> -	unsigned int cur_data_count;
> -	unsigned int cur_data_idx;
> -	int i;
> -	int retval = 0;
> -
> -	/* Clear AUX CH data buffer */
> -	reg = BUF_CLR;
> -	writel(reg, dp->reg_base + ANALOGIX_DP_BUFFER_DATA_CTL);
> -
> -	start_offset = 0;
> -	while (start_offset < count) {
> -		/* Buffer size of AUX CH is 16 * 4bytes */
> -		if ((count - start_offset) > 16)
> -			cur_data_count = 16;
> -		else
> -			cur_data_count = count - start_offset;
> -
> -		for (i = 0; i < 3; i++) {
> -			/* Select DPCD device address */
> -			reg = AUX_ADDR_7_0(reg_addr + start_offset);
> -			writel(reg, dp->reg_base + ANALOGIX_DP_AUX_ADDR_7_0);
> -			reg = AUX_ADDR_15_8(reg_addr + start_offset);
> -			writel(reg, dp->reg_base + ANALOGIX_DP_AUX_ADDR_15_8);
> -			reg = AUX_ADDR_19_16(reg_addr + start_offset);
> -			writel(reg, dp->reg_base + ANALOGIX_DP_AUX_ADDR_19_16);
> -
> -			for (cur_data_idx = 0; cur_data_idx < cur_data_count;
> -			     cur_data_idx++) {
> -				reg = data[start_offset + cur_data_idx];
> -				writel(reg, dp->reg_base +
> -				       ANALOGIX_DP_BUF_DATA_0 +
> -				       4 * cur_data_idx);
> -			}
> -
> -			/*
> -			 * Set DisplayPort transaction and write
> -			 * If bit 3 is 1, DisplayPort transaction.
> -			 * If Bit 3 is 0, I2C transaction.
> -			 */
> -			reg = AUX_LENGTH(cur_data_count) |
> -				AUX_TX_COMM_DP_TRANSACTION | AUX_TX_COMM_WRITE;
> -			writel(reg, dp->reg_base + ANALOGIX_DP_AUX_CH_CTL_1);
> -
> -			/* Start AUX transaction */
> -			retval = analogix_dp_start_aux_transaction(dp);
> -			if (retval == 0)
> -				break;
> -
> -			dev_dbg(dp->dev, "%s: Aux Transaction fail!\n",
> -				__func__);
> -		}
> -
> -		start_offset += cur_data_count;
> -	}
> -
> -	return retval;
> -}
> -
> -int analogix_dp_read_bytes_from_dpcd(struct analogix_dp_device *dp,
> -				     unsigned int reg_addr,
> -				     unsigned int count,
> -				     unsigned char data[])
> -{
> -	u32 reg;
> -	unsigned int start_offset;
> -	unsigned int cur_data_count;
> -	unsigned int cur_data_idx;
> -	int i;
> -	int retval = 0;
> -
> -	/* Clear AUX CH data buffer */
> -	reg = BUF_CLR;
> -	writel(reg, dp->reg_base + ANALOGIX_DP_BUFFER_DATA_CTL);
> -
> -	start_offset = 0;
> -	while (start_offset < count) {
> -		/* Buffer size of AUX CH is 16 * 4bytes */
> -		if ((count - start_offset) > 16)
> -			cur_data_count = 16;
> -		else
> -			cur_data_count = count - start_offset;
> -
> -		/* AUX CH Request Transaction process */
> -		for (i = 0; i < 3; i++) {
> -			/* Select DPCD device address */
> -			reg = AUX_ADDR_7_0(reg_addr + start_offset);
> -			writel(reg, dp->reg_base + ANALOGIX_DP_AUX_ADDR_7_0);
> -			reg = AUX_ADDR_15_8(reg_addr + start_offset);
> -			writel(reg, dp->reg_base + ANALOGIX_DP_AUX_ADDR_15_8);
> -			reg = AUX_ADDR_19_16(reg_addr + start_offset);
> -			writel(reg, dp->reg_base + ANALOGIX_DP_AUX_ADDR_19_16);
> -
> -			/*
> -			 * Set DisplayPort transaction and read
> -			 * If bit 3 is 1, DisplayPort transaction.
> -			 * If Bit 3 is 0, I2C transaction.
> -			 */
> -			reg = AUX_LENGTH(cur_data_count) |
> -				AUX_TX_COMM_DP_TRANSACTION | AUX_TX_COMM_READ;
> -			writel(reg, dp->reg_base + ANALOGIX_DP_AUX_CH_CTL_1);
> -
> -			/* Start AUX transaction */
> -			retval = analogix_dp_start_aux_transaction(dp);
> -			if (retval == 0)
> -				break;
> -
> -			dev_dbg(dp->dev, "%s: Aux Transaction fail!\n",
> -				__func__);
> -		}
> -
> -		for (cur_data_idx = 0; cur_data_idx < cur_data_count;
> -		    cur_data_idx++) {
> -			reg = readl(dp->reg_base + ANALOGIX_DP_BUF_DATA_0
> -						 + 4 * cur_data_idx);
> -			data[start_offset + cur_data_idx] =
> -				(unsigned char)reg;
> -		}
> -
> -		start_offset += cur_data_count;
> -	}
> -
> -	return retval;
> -}
> -
> -int analogix_dp_select_i2c_device(struct analogix_dp_device *dp,
> -				  unsigned int device_addr,
> -				  unsigned int reg_addr)
> -{
> -	u32 reg;
> -	int retval;
> -
> -	/* Set EDID device address */
> -	reg = device_addr;
> -	writel(reg, dp->reg_base + ANALOGIX_DP_AUX_ADDR_7_0);
> -	writel(0x0, dp->reg_base + ANALOGIX_DP_AUX_ADDR_15_8);
> -	writel(0x0, dp->reg_base + ANALOGIX_DP_AUX_ADDR_19_16);
> -
> -	/* Set offset from base address of EDID device */
> -	writel(reg_addr, dp->reg_base + ANALOGIX_DP_BUF_DATA_0);
> -
> -	/*
> -	 * Set I2C transaction and write address
> -	 * If bit 3 is 1, DisplayPort transaction.
> -	 * If Bit 3 is 0, I2C transaction.
> -	 */
> -	reg = AUX_TX_COMM_I2C_TRANSACTION | AUX_TX_COMM_MOT |
> -		AUX_TX_COMM_WRITE;
> -	writel(reg, dp->reg_base + ANALOGIX_DP_AUX_CH_CTL_1);
> -
> -	/* Start AUX transaction */
> -	retval = analogix_dp_start_aux_transaction(dp);
> -	if (retval != 0)
> -		dev_dbg(dp->dev, "%s: Aux Transaction fail!\n", __func__);
> -
> -	return retval;
> -}
> -
> -int analogix_dp_read_byte_from_i2c(struct analogix_dp_device *dp,
> -				   unsigned int device_addr,
> -				   unsigned int reg_addr,
> -				   unsigned int *data)
> -{
> -	u32 reg;
> -	int i;
> -	int retval;
> -
> -	for (i = 0; i < 3; i++) {
> -		/* Clear AUX CH data buffer */
> -		reg = BUF_CLR;
> -		writel(reg, dp->reg_base + ANALOGIX_DP_BUFFER_DATA_CTL);
> -
> -		/* Select EDID device */
> -		retval = analogix_dp_select_i2c_device(dp, device_addr,
> -						       reg_addr);
> -		if (retval != 0)
> -			continue;
> -
> -		/*
> -		 * Set I2C transaction and read data
> -		 * If bit 3 is 1, DisplayPort transaction.
> -		 * If Bit 3 is 0, I2C transaction.
> -		 */
> -		reg = AUX_TX_COMM_I2C_TRANSACTION |
> -			AUX_TX_COMM_READ;
> -		writel(reg, dp->reg_base + ANALOGIX_DP_AUX_CH_CTL_1);
> -
> -		/* Start AUX transaction */
> -		retval = analogix_dp_start_aux_transaction(dp);
> -		if (retval == 0)
> -			break;
> -
> -		dev_dbg(dp->dev, "%s: Aux Transaction fail!\n", __func__);
> -	}
> -
> -	/* Read data */
> -	if (retval == 0)
> -		*data = readl(dp->reg_base + ANALOGIX_DP_BUF_DATA_0);
> -
> -	return retval;
> -}
> -
> -int analogix_dp_read_bytes_from_i2c(struct analogix_dp_device *dp,
> -				    unsigned int device_addr,
> -				    unsigned int reg_addr,
> -				    unsigned int count,
> -				    unsigned char edid[])
> -{
> -	u32 reg;
> -	unsigned int i, j;
> -	unsigned int cur_data_idx;
> -	unsigned int defer = 0;
> -	int retval = 0;
> -
> -	for (i = 0; i < count; i += 16) {
> -		for (j = 0; j < 3; j++) {
> -			/* Clear AUX CH data buffer */
> -			reg = BUF_CLR;
> -			writel(reg, dp->reg_base + ANALOGIX_DP_BUFFER_DATA_CTL);
> -
> -			/* Set normal AUX CH command */
> -			reg = readl(dp->reg_base + ANALOGIX_DP_AUX_CH_CTL_2);
> -			reg &= ~ADDR_ONLY;
> -			writel(reg, dp->reg_base + ANALOGIX_DP_AUX_CH_CTL_2);
> -
> -			/*
> -			 * If Rx sends defer, Tx sends only reads
> -			 * request without sending address
> -			 */
> -			if (!defer)
> -				retval = analogix_dp_select_i2c_device(dp,
> -						device_addr, reg_addr + i);
> -			else
> -				defer = 0;
> -
> -			if (retval == 0) {
> -				/*
> -				 * Set I2C transaction and write data
> -				 * If bit 3 is 1, DisplayPort transaction.
> -				 * If Bit 3 is 0, I2C transaction.
> -				 */
> -				reg = AUX_LENGTH(16) |
> -					AUX_TX_COMM_I2C_TRANSACTION |
> -					AUX_TX_COMM_READ;
> -				writel(reg, dp->reg_base +
> -					ANALOGIX_DP_AUX_CH_CTL_1);
> -
> -				/* Start AUX transaction */
> -				retval = analogix_dp_start_aux_transaction(dp);
> -				if (retval == 0)
> -					break;
> -
> -				dev_dbg(dp->dev, "%s: Aux Transaction fail!\n",
> -					__func__);
> -			}
> -			/* Check if Rx sends defer */
> -			reg = readl(dp->reg_base + ANALOGIX_DP_AUX_RX_COMM);
> -			if (reg == AUX_RX_COMM_AUX_DEFER ||
> -			    reg == AUX_RX_COMM_I2C_DEFER) {
> -				dev_err(dp->dev, "Defer: %d\n\n", reg);
> -				defer = 1;
> -			}
> -		}
> -
> -		for (cur_data_idx = 0; cur_data_idx < 16; cur_data_idx++) {
> -			reg = readl(dp->reg_base + ANALOGIX_DP_BUF_DATA_0
> -						 + 4 * cur_data_idx);
> -			edid[i + cur_data_idx] = (unsigned char)reg;
> -		}
> -	}
> -
> -	return retval;
> -}
> -
>   void analogix_dp_set_link_bandwidth(struct analogix_dp_device *dp, u32 bwtype)
>   {
>   	u32 reg;