Hi Dan,
Many thanks for your comments.
2015-12-07 9:09 GMT+01:00 Dan Carpenter <dan.carpenter@xxxxxxxxxx>:
> 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);
>> + else
>> + sp_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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
