On Wed, 2017-05-03 at 22:28 +0300, Sakari Ailus wrote: > Hi Philipp, > > Thanks for continuing working on this! > > I have some minor comments below... Thank you for the comments. [...] > Could you rebase this on the V4L2 fwnode patchset here, please? > > <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=v4l2-acpi> > > The conversion is rather simple, as shown here: > > <URL:https://git.linuxtv.org/sailus/media_tree.git/commit/?h=v4l2-acpi&id=679035e11bfdbea146fed5d52fb794b34dc9cea6> What is the status of this patchset? Will this be merged soon? [...] > > +static inline bool is_source_pad(struct video_mux *vmux, unsigned int pad) > > It's a common practice to test pad flags rather than the pad number. > Although the pad number here implicitly tells this, too, testing pad flags > is cleaner. > > The matter was discussed in the past and it was decided not to add helper > functions to the framework for the purpose as testing the flags is trivial. Ok, I'll drop is_source_pad and check (pad->flags & MEDIA_PAD_FL_SOURCE) instead in the next version. [...] > > +static int video_mux_set_format(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *sdformat) > > +{ > > + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd); > > + struct v4l2_mbus_framefmt *mbusformat; > > + > > + mbusformat = __video_mux_get_pad_format(sd, cfg, sdformat->pad, > > + sdformat->which); > > + if (!mbusformat) > > + return -EINVAL; > > + > > + mutex_lock(&vmux->lock); > > + > > + /* Source pad mirrors active sink pad, no limitations on sink pads */ > > + if (is_source_pad(vmux, sdformat->pad) && vmux->active >= 0) > > + sdformat->format = vmux->format_mbus[vmux->active]; > > + > > + mutex_unlock(&vmux->lock); > > + > > + *mbusformat = sdformat->format; > > Shouldn't you do this before releasing the mutex? The assignment won't be > an atomic operation. Same for get_format; you should take the mutex. Yes, I'll extend the mutex to cover the mbus formats. [...] > > +static struct v4l2_subdev_pad_ops video_mux_pad_ops = { > > + .get_fmt = video_mux_get_format, > > + .set_fmt = video_mux_set_format, > > +}; > > + > > +static struct v4l2_subdev_ops video_mux_subdev_ops = { > > Const for both of the structs? Will do, thanks. regards Philipp
