Hi Janusz, On Fri, May 31, 2019 at 07:56:33PM +0200, Janusz Krzysztofik wrote: > Hi Sakari, > > On Friday, May 31, 2019 1:42:58 PM CEST Sakari Ailus wrote: > > Hi Janusz, > > > > On Sun, May 26, 2019 at 10:47:57PM +0200, Janusz Krzysztofik wrote: > > > According to V4L2 subdevice interface specification, frame scaling > > > factors should be reset to default values on modification of input frame > > > format. Assuming that requirement also applies in case of crop > > > rectangle modification unless V4L2_SEL_FLAG_KEEP_CONFIG is requested, > > > the driver now does not respect it. > > > > > > The KEEP_CONFIG case is already implemented, fix the other path. > > > > > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@xxxxxxxxx> > > > --- > > > drivers/media/i2c/ov6650.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c > > > index 47590cd51190..cc70d8952999 100644 > > > --- a/drivers/media/i2c/ov6650.c > > > +++ b/drivers/media/i2c/ov6650.c > > > @@ -472,6 +472,8 @@ static int ov6650_get_selection(struct v4l2_subdev > *sd, > > > } > > > } > > > > > > +static int ov6650_s_fmt(struct v4l2_subdev *sd, u32 code, bool > half_scale); > > > + > > > > Would it be possible to rearrange the functions in the file so you wouldn't > > need extra prototypes? Preferrably that'd be a new patch. > > Sure. I've intentionally done it like that for better readability of the > patches, but I have a task in my queue for rearrangement of functions order as > soon as other modifications are in place. I'm totally fine with doing that after this set on as well. Up to you. > > > > static int ov6650_set_selection(struct v4l2_subdev *sd, > > > struct v4l2_subdev_pad_config *cfg, > > > struct v4l2_subdev_selection *sel) > > > @@ -515,7 +517,13 @@ static int ov6650_set_selection(struct v4l2_subdev > *sd, > > > } > > > if (!ret) > > > priv->rect.height = sel->r.height; > > > + else > > > + return ret; > > > > if (ret) > > return ret; > > OK > > Perhaps you will have more comments on other patches so I'll wait a bit and > then resubmit the series as v2. Not so much on this set BUT I realised that the subtle effect of "media: ov6650: Register with asynchronous subdevice framework" is that the driver is now responsible for serialising the access to its own data structures now. And it doesn't do that. Could you submit a fix, please? It'd be good to get that to 5.2 through the fixes branch. -- Kind regards, Sakari Ailus sakari.ailus@xxxxxxxxxxxxxxx
