Messages in this thread | | | Date | Wed, 27 Jan 2021 11:18:14 +0100 | From | Wolfram Sang <> | Subject | Re: [PATCH 2/3] media: i2c: adv7511: remove open coded version of SMBus block read |
| |
Hi Hans,
> > - adv7511_edid_rd(sd, 256, &state->edid.data[segment * 256]); > > + err = adv7511_edid_rd(sd, 256, &state->edid.data[segment * 256]); > > adv7511_dbg_dump_edid(2, debug, sd, segment, &state->edid.data[segment * 256]); > > Only call adv7511_dbg_dump_edid if err >= 0.
Yes.
> > > if (segment == 0) { > > Change condition to: err >= 0 && segment == 0
Yes.
> > > state->edid.blocks = state->edid.data[0x7e] + 1; > > v4l2_dbg(1, debug, sd, "%s: %d blocks in total\n", __func__, state->edid.blocks); > > }
So I guarded the above block with an 'if (!err)' clause. adv7511_edid_rd() returns either 0 or errno, so we can take the above simpler condition.
> > - if (!edid_verify_crc(sd, segment) || > > - !edid_verify_header(sd, segment)) { > > + if (err < 0 || !edid_verify_crc(sd, segment) || !edid_verify_header(sd, segment)) { > > /* edid crc error, force reread of edid segment */ > > Hmm, this comment is a bit out of date. Change to: > > /* > * Couldn't read EDID or EDID has invalid content. > * Force reread of EDID segment. > */
I updated the comment but kept it a oneliner.
> > > v4l2_err(sd, "%s: edid crc or header error\n", __func__); > > Only show this message if err >= 0. For err < 0 the adv7511_edid_rd() already > logs an error.
Yes. I used 'if (!err)' again here.
Will resend in a minute, thanks for the review!
All the best,
Wolfram
[unhandled content-type:application/pgp-signature] | |