On 3/7/22 08:16, Tomi Valkeinen wrote: > Hi Hans, > > On 04/03/2022 15:34, Hans Verkuil wrote: >> Hi Tomi, >> >> On 3/1/22 11:55, Tomi Valkeinen wrote: >>> Add v4l2_subdev_call_state_active() macro to help calling subdev ops >>> that take a subdev state as a parameter. Normally the v4l2 framework >>> will lock and pass the correct subdev state to the subdev ops, but there >>> are legacy situations where this is not the case (e.g. non-MC video >>> device driver calling set_fmt in a source subdev). >>> >>> As this macro is only needed for legacy use cases, the macro is added in >>> a new header file, v4l2-subdev-legacy.h. >>> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> >>> --- >>> include/media/v4l2-subdev-legacy.h | 42 ++++++++++++++++++++++++++++++ >>> 1 file changed, 42 insertions(+) >>> create mode 100644 include/media/v4l2-subdev-legacy.h >>> >>> diff --git a/include/media/v4l2-subdev-legacy.h b/include/media/v4l2-subdev-legacy.h >>> new file mode 100644 >>> index 000000000000..6a61e579b629 >>> --- /dev/null >>> +++ b/include/media/v4l2-subdev-legacy.h >>> @@ -0,0 +1,42 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >>> +/* >>> + * V4L2 sub-device legacy support header. >>> + * >>> + * Copyright (C) 2022 Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> >>> + */ >>> + >>> +#ifndef _V4L2_SUBDEV_LEGACY_H >>> +#define _V4L2_SUBDEV_LEGACY_H >>> + >>> +/** >>> + * v4l2_subdev_call_state_active - call an operation of a v4l2_subdev which >>> + * takes state as a parameter, passing the >>> + * subdev its active state. >>> + * >>> + * @sd: pointer to the &struct v4l2_subdev >>> + * @o: name of the element at &struct v4l2_subdev_ops that contains @f. >>> + * Each element there groups a set of callbacks functions. >>> + * @f: callback function to be called. >>> + * The callback functions are defined in groups, according to >>> + * each element at &struct v4l2_subdev_ops. >>> + * @args: arguments for @f. >>> + * >>> + * This is similar to v4l2_subdev_call(), except that this version can only be >>> + * used for ops that take a subdev state as a parameter. The macro will get the >>> + * active state and lock it before calling the op, and unlock it after the >>> + * call. >>> + */ >> >> You should explain why this is a legacy macro and, ideally, what would need to >> be done to get rid of it. The first is in the commit log, but nobody reads that :-) >> >> But if just using it in a non-MC video device driver constitutes 'legacy' use, >> then I disagree with that. There are many non-MC video device drivers, nothing >> legacy about that. > > It's difficult to define all the scenarios where this can be used, but the ones I can imagine fall under legacy (depending on how you define that, though). > > I use this in CAL driver, which supports non-MC (legacy) and MC. CAL has a bunch of video devices (one for each DMA engine) and two CSI-2 PHY devices (v4l2 subdevs). > > When operating in MC mode, the userspace will call, e.g., set_fmt in the PHY subdev, and so forth. > > But in non-MC case the userspace calls VIDIOC_S_FMT in the video dev, and the video dev has to propagate that to the PHY subdev. I do this propagation using the v4l2_subdev_call_state_active macro. > > I don't know if there are other drivers that support both non-MC and MC modes. I could also just move this macro to the CAL driver, and we could add this to the v4l2 framework if we see other drivers using similar constructs. It is common to have non-MC drivers that call set_fmt of a subdev. Wouldn't they all need to use this helper macro? If so, then this is NOT a legacy use, it's just a non-MC driver use. Am I missing something? Regards, Hans > > Tomi
