Re: [PATCH 2/3] media: i2c: adv7511: remove open coded version of SMBus block read

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 1489 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]