Re: [PATCH v2 2/2] [media] platform: add video-multiplexer subdevice driver

[Philipp Zabel <p.zabel@pengutronix.de>]

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