9 Dec
2015
9 Dec
'15
11:55 a.m.
Hi Dan,
Many thanks for your comments.
2015-12-07 9:09 GMT+01:00 Dan Carpenter dan.carpenter@oracle.com:
On Fri, Dec 04, 2015 at 09:35:07AM +0100, Enric Balletbo i Serra wrote:
+static int sp_wait_aux_op_finish(struct anx78xx *anx78xx) +{
u8 errcnt;u8 val;struct device *dev = &anx78xx->client->dev;errcnt = 150;while (errcnt--) {sp_reg_read(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG, &val);if (!(val & SP_AUX_EN))break;usleep_range(2000, 4000);}if (!errcnt) {This is off by one. It should be:
while (--errcnt) { ... } if (errcnt == 0) return -EWHATEVER;Or:
while (errcnt--) { ... } if (errcnt == -1) return -EWHATEVER;Also "errcnt" is a bad name, it should be retry_cnt or something (or maybe it actually is counting errors?). Also -1 is never a correct error code, please change all the -1 returns to something better.
Ok, I renamed to retry_cnt and changed all the -1 values to something better.
/* Buffer size of AUX CH is 16 */if (count > 16)return -1;Just make a define so that you don't need to add comments about why 16 is correct.
if (count > SIZE_AUX_CH) return -EINVAL;
Added a define
errcnt = 10;while (errcnt--) {sp_reg_read(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG, &val);if (!(val & SP_AUX_EN))break;usleep_range(1000, 2000);}if (!errcnt) {dev_err(dev,"failed to read DP AUX Channel Control Register 2\n");sp_reset_aux(anx78xx);return -1;}Off by one again.
Ack
sp_reg_write(anx78xx, TX_P0, SP_AUX_ADDR_7_0_REG, SP_I2C_EXTRA_ADDR);sp_tx_aux_wr(anx78xx, offset);/* read 16 bytes (MOT = 1) */sp_tx_aux_rd(anx78xx, 0xf0 | DP_AUX_I2C_MOT | DP_AUX_I2C_READ);for (i = 0; i < 16; i++) {errcnt = 10;while (errcnt--) {sp_reg_read(anx78xx, TX_P0, SP_BUF_DATA_COUNT_REG,&val);if (val & SP_BUF_DATA_COUNT_MASK)break;usleep_range(2000, 4000);}if (!errcnt) {dev_err(dev,"failed to read DP Buffer Data Count Register\n");sp_reset_aux(anx78xx);return -1;}And here.
Ack
errcnt = 10;while (errcnt--) {sp_reg_read(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG, &val);if (!(val & SP_AUX_EN))break;usleep_range(1000, 2000);}if (!errcnt) {dev_err(dev,"failed to read DP AUX Channel Control Register 2\n");sp_reset_aux(anx78xx);return -1;}Here.
Ack
return 0;+}
+static int sp_edid_block_checksum(const u8 *raw_edid) +{
int i;u8 csum = 0;for (i = 0; i < EDID_LENGTH; i++)csum += raw_edid[i];return csum;+}
+static int sp_tx_edid_read(struct anx78xx *anx78xx) +{
struct device *dev = &anx78xx->client->dev;u8 val, last_block, offset = 0;u8 buf[16];int i, j, count;sp_tx_edid_read_initial(anx78xx);sp_reg_write(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL1_REG, 0x04);sp_reg_set_bits(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG,SP_AUX_EN | SP_ADDR_ONLY);if (sp_wait_aux_op_finish(anx78xx))return -1;sp_addronly_set(anx78xx, false);/* Read the number of blocks */sp_tx_aux_wr(anx78xx, 0x7e);sp_tx_aux_rd(anx78xx, DP_AUX_I2C_READ);sp_reg_read(anx78xx, TX_P0, SP_DP_BUF_DATA0_REG, &last_block);dev_dbg(dev, "last EDID block is %d\n", last_block);/* FIXME: Why not just cap to 3 if the reported value is >3 */if (last_block > 3)last_block = 1;/* for every block */for (count = 0; count <= last_block; count++) {switch (count) {case 0:case 1:for (i = 0; i < 8; i++) {offset = (i + count * 8) * 16;if (sp_edid_read(anx78xx, offset, buf))return -1;for (j = 0; j < 16; j++)sp.edid_blocks[offset + j] = buf[j];}break;case 2:case 3:offset = (count == 2) ? 0x00 : 0x80;for (j = 0; j < 8; j++) {if (sp_seg_edid_read(anx78xx, count / 2,offset))return -1;offset = offset + 0x10;}break;default:break;Is there something which complains if you leave out the default case statement? It's not reachable.
I left out the default case as is not reachable.
}}sp_reset_aux(anx78xx);if (!drm_edid_block_valid(sp.edid_blocks, 0, true, NULL)) {dev_err(dev, "EDID block is invalid\n");return -1;}sp_dp_read_bytes_from_dpcd(anx78xx, DP_TEST_REQUEST, 1, &val);if (val & DP_TEST_LINK_EDID_READ) {dev_dbg(dev, "EDID test requested\n");val = sp_edid_block_checksum(sp.edid_blocks);dev_dbg(dev, "EDID checksum is %d\n", val);sp_dp_write_bytes_to_dpcd(anx78xx, DP_TEST_EDID_CHECKSUM, 1,&val);sp.tx_test_edid = true;val = DP_TEST_EDID_CHECKSUM_WRITE;sp_dp_write_bytes_to_dpcd(anx78xx, DP_TEST_RESPONSE, 1, &val);}return 0;+}
+static bool sp_check_with_pre_edid(struct anx78xx *anx78xx) +{
struct device *dev = &anx78xx->client->dev;u8 i;u8 buf[16];bool ret = false;sp_tx_edid_read_initial(anx78xx);sp_reg_write(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL1_REG, 0x04);sp_reg_set_bits(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG, 0x03);if (sp_wait_aux_op_finish(anx78xx))goto return_point;sp_addronly_set(anx78xx, false);if (sp_edid_read(anx78xx, 0x70, buf))goto return_point;for (i = 0; i < 16; i++) {if (sp.edid_blocks[0x70 + i] != buf[i]) {dev_dbg(dev, "%s\n","different checksum and blocks num\n");goto return_point;}}Can you just say:
if (memcmp(&sp.edid_blocks[0x70], buf, 16) != 0) { dev_dbg(dev, "different checksum and blocks num\n"); goto return_point; }
I will change
if (sp_edid_read(anx78xx, 0x08, buf))goto return_point;for (i = 0; i < 16; i++) {if (sp.edid_blocks[i + 8] != buf[i]) {dev_dbg(dev, "different edid information\n");goto return_point;}}ret = true;+return_point:
sp_reset_aux(anx78xx);return ret;+}
[ snip ]
+static bool sp_config_video_output(struct anx78xx *anx78xx) +{
struct device *dev = &anx78xx->client->dev;u8 val;switch (sp.tx_vo_state) {default:case VO_WAIT_VIDEO_STABLE:sp_reg_read(anx78xx, RX_P0, SP_SYSTEM_STATUS_REG, &val);if ((val & SP_TMDS_DE_DET) && (val & SP_TMDS_CLOCK_DET)) {if (sp_tx_bw_lc_sel(anx78xx))return false;sp_enable_video_input(anx78xx, false);sp_hdmi_new_avi_int(anx78xx);sp_reg_read(anx78xx, RX_P0,SP_PACKET_RECEIVING_STATUS_REG, &val);if (val & SP_VSI_RCVD)sp_hdmi_new_vsi_int(anx78xx);sp_enable_video_input(anx78xx, true);sp.tx_vo_state = VO_WAIT_TX_VIDEO_STABLE;} else {dev_dbg(dev, "HDMI input video not stable!\n");break;}/* fallthrough */case VO_WAIT_TX_VIDEO_STABLE:/** The flag is write clear and can be latched from last* status. So the first read and write is to clear the* previous status.*/sp_reg_read(anx78xx, TX_P0, SP_DP_SYSTEM_CTRL_BASE + 2, &val);sp_reg_write(anx78xx, TX_P0, SP_DP_SYSTEM_CTRL_BASE + 2, val);sp_reg_read(anx78xx, TX_P0, SP_DP_SYSTEM_CTRL_BASE + 2, &val);if (val & SP_CHA_STA) {dev_dbg(dev, "stream clock not stable!\n");break;} else {No need for the else statement. Pull it in one indent level.
Ack
/** The flag is write clear and can be latched from* last status. So the first read and write is to* clear the previous status.*/sp_reg_read(anx78xx, TX_P0,SP_DP_SYSTEM_CTRL_BASE + 3,&val);sp_reg_write(anx78xx, TX_P0,SP_DP_SYSTEM_CTRL_BASE + 3,val);sp_reg_read(anx78xx, TX_P0,SP_DP_SYSTEM_CTRL_BASE + 3,&val);if (val & SP_STRM_VALID) {if (sp.tx_test_lt)sp.tx_test_lt = false;sp.tx_vo_state = VO_FINISH;} else {dev_err(dev, "video stream not valid!\n");break;}}/* fallthrough */case VO_FINISH:sp_block_power_ctrl(anx78xx, SP_TX_PWR_AUDIO, false);sp_hdmi_mute_video(anx78xx, false);sp_video_mute(anx78xx, false);sp_show_information(anx78xx);return true;}
Changed
return false;+}
[ snip ]
+static void sp_config_audio(struct anx78xx *anx78xx) +{
int i;u8 val;sp_block_power_ctrl(anx78xx, SP_TX_PWR_AUDIO, true);sp_reg_read(anx78xx, TX_P0, SP_DP_MAIN_LINK_BW_SET_REG, &val);if (val & SP_INITIAL_SLIM_M_AUD_SEL)if (sp_calculate_audio_m_value(anx78xx))return;Combine these:
if ((val & SP_INITIAL_SLIM_M_AUD_SEL) && sp_calculate_audio_m_value(anx78xx)) return;
Ok
sp_reg_clear_bits(anx78xx, TX_P1, SP_AUD_INTERFACE_CTRL0_REG,SP_AUD_INTERFACE_DISABLE);sp_reg_set_bits(anx78xx, TX_P1, SP_AUD_INTERFACE_CTRL2_REG,SP_M_AUD_ADJUST_ST);sp_reg_read(anx78xx, RX_P0, SP_HDMI_STATUS_REG, &val);if (val & SP_HDMI_AUD_LAYOUT)sp_reg_set_bits(anx78xx, TX_P2, SP_AUD_CH_STATUS_BASE + 5,SP_I2S_CH_NUM_8 | SP_AUDIO_LAYOUT);elsesp_reg_clear_bits(anx78xx, TX_P2, SP_AUD_CH_STATUS_BASE + 5,SP_I2S_CHANNEL_NUM_MASK | SP_AUDIO_LAYOUT);/* transfer audio channel status from HDMI Rx to Slimport Tx */for (i = 1; i <= SP_AUD_CH_STATUS_REG_NUM; i++) {sp_reg_read(anx78xx, RX_P0, SP_AUD_SPDIF_CH_STATUS_BASE + i,&val);sp_reg_write(anx78xx, TX_P2, SP_AUD_CH_STATUS_BASE + i,val);}Either this loop is off by one or the loop in sp_hdmi_audio_samplechg_int() is off by one. Also just call that function instead of re-implimenting it here.
Right, I'll fix that
/* enable audio */sp_enable_audio_output(anx78xx, true);+}
+static bool sp_config_audio_output(struct anx78xx *anx78xx) +{
u8 val;switch (sp.tx_ao_state) {default:case AO_INIT:case AO_CTS_RCV_INT:case AO_AUDIO_RCV_INT:sp_reg_read(anx78xx, RX_P0, SP_HDMI_STATUS_REG, &val);if (!val & SP_HDMI_MODE) {This is a precendence error. It should be:
Ack
if (!(val & SP_HDMI_MODE)) {
sp.tx_ao_state = AO_INIT;return true;}break;regards, dan carpenter
I'll wait a bit more to see if anyone else does more comments and then I'll send another version with the changes you suggested. Thanks.
Regards, Enric