Hi Laurent, Thanks for your feedback. I have incorporated your suggestions for the next and hopefully last version of this patch-set, a few followups on your review bellow. On 2018-04-04 01:09:29 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Tuesday, 27 March 2018 00:44:39 EEST Niklas Söderlund wrote: > > With the recent cleanup of the format code to prepare for Gen3 it's > > possible to simplify the Gen2 format code path as well. Clean up the > > process by defining two functions to handle the set format and reset of > > format when the standard is changed. > > > > While at it replace the driver local struct rvin_source_fmt with a > > struct v4l2_rect as all it's used for is keep track of the source > > dimensions. > > I wonder whether we should introduce v4l2_size (or <insert your preferred name > here>) when all we need is width and height, as v4l2_rect stores the top and > left offsets too. This doesn't have to be fixed here though. Yes that would have been useful for this change. I have added this to my ever growing TODO list :-) [snip] > > > - vin->format.bytesperline = rvin_format_bytesperline(&vin->format); > > - vin->format.sizeimage = rvin_format_sizeimage(&vin->format); > > + vin->source.top = vin->crop.top = 0; > > + vin->source.left = vin->crop.left = 0; > > + vin->source.width = vin->crop.width = vin->format.width; > > + vin->source.height = vin->crop.height = vin->format.height; > > I find multiple assignations on the same line hard to read. How about > > vin->source.top = 0; > vin->source.left = 0; > vin->source.width = vin->format.width; > vin->source.height = vin->format.height; > > vin->crop = vin->source; > vin->compose = vin->source; This looks much better and I will use it, thanks ! [snip] > > > + /* > > + * If source is ALTERNATE the driver will use the VIN hardware > > + * to INTERLACE it. The crop height then needs to be doubled. > > + */ > > + if (pix->field == V4L2_FIELD_ALTERNATE) > > + crop->height *= 2; > > You're duplicating this logic in rvin_format_align() so it makes me feel that > there's still room for some simplification, but that can be done in a separate > patch (or I could simply be wrong). I'm sure someone more clever then me can simplify this more. In this particular case rvin_format_align() deals with the video device format while here we deal with the crop rectangle. I will keep this in mind for future work and see if this can be unified in simpler way. [snip] > > - return __rvin_try_format(vin, V4L2_SUBDEV_FORMAT_TRY, > > &f->fmt.pix, > > - &source); > > + return rvin_try_format(vin, V4L2_SUBDEV_FORMAT_TRY, &f->fmt.pix, &crop, > > + &compose); > > How about making crop and compose optional in rvin_try_format() to avoid a > need for the two local variables here ? Great suggestion, I have incorporated this for the next version. > > Apart from these small issues, I think this is a nice simplification, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> Thanks! -- Regards, Niklas Söderlund
