[PATCH 1/1] scripts/kernel-doc: Fix struct and struct field attribute processing
The kernel-doc attempts to clear the struct and struct member attributes
from the API documentation it produces. It falls short of the job in the
following respects:
- extra whitespaces are left where __attribute__((...)) was removed,
- only a single attribute is removed per struct,
- attributes (such as aligned) containing numbers were not removed,
- attributes are only cleared from struct fields, not structs themselves.
This patch addresses these issues by removing the attributes.
Signed-off-by: Sakari Ailus
---
scripts/kernel-doc | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index ffbe901a37b51..5a3945dc50358 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -1062,7 +1062,7 @@ sub dump_struct($$) {
my $x = shift;
my $file = shift;
-if ($x =~ /(struct|union)\s+(\w+)\s*\{(.*)\}/) {
+if ($x =~
/(struct|union)\s+(\w+)\s*\{(.*)\}(\s*(__packed|__aligned|__attribute__\s*\(\([a-z0-9,_\s\(\)]*\)\)))*/)
{
my $decl_type = $1;
$declaration_name = $2;
my $members = $3;
@@ -1073,8 +1073,9 @@ sub dump_struct($$) {
# strip comments:
$members =~ s/\/\*.*?\*\///gos;
# strip attributes
- $members =~ s/__attribute__\s*\(\([a-z,_\*\s\(\)]*\)\)//i;
- $members =~ s/__aligned\s*\([^;]*\)//gos;
+ $members =~ s/\s*__attribute__\s*\(\([a-z0-9,_\*\s\(\)]*\)\)//gi;
+ $members =~ s/\s*__aligned\s*\([^;]*\)//gos;
+ $members =~ s/\s*__packed\s*//gos;
$members =~ s/\s*CRYPTO_MINALIGN_ATTR//gos;
# replace DECLARE_BITMAP
$members =~ s/DECLARE_BITMAP\s*\(([^,)]+),\s*([^,)]+)\)/unsigned long
$1\[BITS_TO_LONGS($2)\]/gos;
--
2.11.0
Re: [PATCH 15/24] media: v4l2-subdev: get rid of __V4L2_SUBDEV_MK_GET_TRY() macro
On Tue, Dec 19, 2017 at 09:03:55AM -0200, Mauro Carvalho Chehab wrote: > [PATCH] media: v4l2-subdev: get rid of __V4L2_SUBDEV_MK_GET_TRY() macro > > The __V4L2_SUBDEV_MK_GET_TRY() macro is used to define > 3 functions that have the same arguments. The code of those > functions is simple enough to just declare them, de-obfuscating > the code. > > While here, replace BUG_ON() by WARN_ON() as there's no reason > why to panic the Kernel if this fails. > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> Thanks! Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com> -- Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/24] media: v4l2-subdev: get rid of __V4L2_SUBDEV_MK_GET_TRY() macro
On Mon, Dec 18, 2017 at 05:27:04PM -0200, Mauro Carvalho Chehab wrote:
> Em Mon, 9 Oct 2017 23:23:56 +0300
> Sakari Ailus <sakari.ai...@iki.fi> escreveu:
>
> > Hi Mauro,
> >
> > On Mon, Oct 09, 2017 at 07:19:21AM -0300, Mauro Carvalho Chehab wrote:
> > > The __V4L2_SUBDEV_MK_GET_TRY() macro is used to define
> > > 3 functions that have the same arguments. The code of those
> > > functions is simple enough to just declare them, de-obfuscating
> > > the code.
> > >
> > > While here, replace BUG_ON() by WARN_ON() as there's no reason
> > > why to panic the Kernel if this fails.
> >
> > BUG_ON() might actually be a better idea as this will lead to memory
> > corruption. I presume it's not been hit often.
>
> Well, let's then change the code to:
>
> if (WARN_ON(pad >= sd->entity.num_pads))
> return -EINVAL;
>
> This way, it won't try to use an invalid value. As those are default
> handlers for ioctls, userspace should be able to handle it.
Another approach would be to return the entry for a valid pad. Few if any
drivers perform error handling on the value returned for the simple reason
that they know how many pads their own entities have.
>
> >
> > That said, I, too, favour WARN_ON() in this case. In case pad exceeds the
> > number of pads, then zero could be used, for instance. The only real
> > problem comes if there were no pads to begin with. The callers of these
> > functions also don't expect to receive NULL. Another option would be to
> > define a static, dummy variable for the purpose that would be at least safe
> > to access. Or we could just use the dummy entry whenever the pad isn't
> > valid.
> >
> > This will make the functions more complex and I might just keep the
> > original macro. Even grep works on it nowadays.
> >
> > >
> > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> > > ---
> > > include/media/v4l2-subdev.h | 37 +
> > > 1 file changed, 25 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index 1f34045f07ce..35c4476c56ee 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -897,19 +897,32 @@ struct v4l2_subdev_fh {
> > > container_of(fh, struct v4l2_subdev_fh, vfh)
> > >
> > > #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > > -#define __V4L2_SUBDEV_MK_GET_TRY(rtype, fun_name, field_name)
> > > \
> > > - static inline struct rtype *\
> > > - fun_name(struct v4l2_subdev *sd,\
> > > - struct v4l2_subdev_pad_config *cfg,\
> > > - unsigned int pad) \
> > > - { \
> > > - BUG_ON(pad >= sd->entity.num_pads); \
> > > - return [pad].field_name;\
> > > - }
> > > +static inline struct v4l2_mbus_framefmt
> > > +*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> > > + struct v4l2_subdev_pad_config *cfg,
> > > + unsigned int pad)
> > > +{
> > > + WARN_ON(pad >= sd->entity.num_pads);
> > > + return [pad].try_fmt;
> > > +}
> > >
> > > -__V4L2_SUBDEV_MK_GET_TRY(v4l2_mbus_framefmt, v4l2_subdev_get_try_format,
> > > try_fmt)
> > > -__V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, v4l2_subdev_get_try_crop, try_crop)
> > > -__V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, v4l2_subdev_get_try_compose,
> > > try_compose)
> > > +static inline struct v4l2_rect
> > > +*v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
> > > + struct v4l2_subdev_pad_config *cfg,
> > > + unsigned int pad)
> > > +{
> > > + WARN_ON(pad >= sd->entity.num_pads);
> > > + return [pad].try_crop;
> > > +}
> > > +
> > > +static inline struct v4l2_rect
> > > +*v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
> > > + struct v4l2_subdev_pad_config *cfg,
> > > + unsigned int pad)
> > > +{
> > > + WARN_ON(pad >= sd->entity.num_pads);
> > > + return [pad].try_compose;
> > > +}
> > > #endif
> > >
> > > extern const struct v4l2_file_operations v4l2_subdev_fops;
> >
>
>
>
> Thanks,
> Mauro
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 5/7] media: open.rst: Adjust some terms to match the glossary
Hi Mauro, On Tue, Oct 10, 2017 at 08:37:43AM -0300, Mauro Carvalho Chehab wrote: > Em Fri, 6 Oct 2017 15:48:22 +0300 > Sakari Ailus <sakari.ai...@iki.fi> escreveu: > > > Hi Mauro, > > > > On Wed, Sep 27, 2017 at 07:23:47PM -0300, Mauro Carvalho Chehab wrote: > > > As we now have a glossary, some terms used on open.rst > > > require adjustments. > > > > > > Acked-by: Hans Verkuil <hans.verk...@cisco.com> > > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> > > > --- > > > Documentation/media/uapi/v4l/open.rst | 12 ++-- > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > diff --git a/Documentation/media/uapi/v4l/open.rst > > > b/Documentation/media/uapi/v4l/open.rst > > > index f603bc9b49a0..0daf0c122c19 100644 > > > --- a/Documentation/media/uapi/v4l/open.rst > > > +++ b/Documentation/media/uapi/v4l/open.rst > > > @@ -143,7 +143,7 @@ Related Devices > > > Devices can support several functions. For example video capturing, VBI > > > capturing and radio support. > > > > > > -The V4L2 API creates different nodes for each of these functions. > > > +The V4L2 API creates different V4L2 device nodes for each of these > > > functions. > > > > A V4L2 device node is an instance of the V4L2 API. At the very least we > > should call them "V4L2 device node types", not device nodes only. This > > simply would suggests they're separate. > > OK, I added "types" there. > > > > > s/creates/defines/ ? > > It is meant to say create. > > A device that supports both radio, video and VBI for the same V4L2 > input will create three device nodes: > /dev/video0 > /dev/radio0 > /dev/vbi0 > > As all are associated to the same video input, and an ioctl send > to one device may affect the other devices too, as they all associated > with the same hardware. Right. In this case I'd change the sentence. What would you think of this? "Each of these functions is available via separate V4L2 device node." For it's not the V4L2 API that creates them. I failed to grasp what the original sentence meant. Was it about API, or framework, or were the devices nodes just separate or unlike as well? -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/7] media: add glossary.rst with a glossary of terms used at V4L2 spec
Hi Mauro, On Tue, Oct 10, 2017 at 09:49:38AM -0300, Mauro Carvalho Chehab wrote: > Em Tue, 10 Oct 2017 14:54:35 +0300 > Sakari Ailus <sakari.ai...@iki.fi> escreveu: > > > On Tue, Oct 10, 2017 at 06:15:03AM -0300, Mauro Carvalho Chehab wrote: > > > Em Fri, 6 Oct 2017 14:51:06 +0300 > > > Sakari Ailus <sakari.ai...@iki.fi> escreveu: > > > > > > > Hi Mauro, > > > > > > > > On Fri, Oct 06, 2017 at 01:22:29PM +0300, Sakari Ailus wrote: > > > > > > +V4L2 device node > > > > > > + A device node that is associated to a V4L2 main driver, > > > > > > + as specified in :ref:`v4l2_device_naming`. > > > > > > > > I think we need to name the interface, not so much their instances. > > > > > > > > How about adding: > > > > > > > > V4L2 > > > > Video4Linux 2 interface. The interface implemented by **V4L2 > > > > device > > > > nodes**. > > > > > > > > and: > > > > > > > > V4L2 device node > > > > A device node implementing the **V4L2** interface. > > > > > > Not sure if I answered it already. subdev API is part of V4L2. > > > So, a change like that would cause more harm than good ;-) > > > > Hmm. There seems to be a gap here. It'd be much easier to maintain > > consistency in naming and definitions if V4L2 sub-device nodes were also > > documented to be V4L2 device nodes, just as any other device nodes exposed > > by drivers through the V4L2 framework. > > > > > > > > The definition should let it clear that only the devnodes > > > implemented by the V4L2 main driver are considered as > > > V4L2 device nodes. > > > > Why? I don't think we should make assumptions on which driver exposes a > > device node; this is not visible to the user space after all. > > Because the V4L2 spec documents, with the exception of the subdev.rst > (and where otherwise noticed), assumes that a V4L2 device node doesn't > include subdevs. > > So, if you loo, for example, at the chapter 1 name: > "common API elements" > > it implies that every single V4L2 device node supports what's there. > But that's not the case, for example, for what's described at > Documentation/media/uapi/v4l/querycap.rst (with is part of > chapter 1). Ah. I see what you mean. > > There are a couple of possible alternatives: > > 1) define V4L2 device nodes excluding /dev/subdev, with is the >current approach; > > 2) rewrite the entire V4L2 uAPI spec to explicitly talk, on each >section, if it applies or not to sub-devices; There are exceptions in the common API elements section even now. For instance, it mentions that radio devices don't support video streaming related IOCTLs. Under common API elements, also the first section (opening and closing devices) refers to the interfaces section which, as we know, contains sub-device documentation. Do you see that something else is needed than telling which common API elements aren't relevant for sub-devices? (I didn't find explicit information in other sections, by a quick glance at least, telling which interfaces they apply to.) Should we make such a change, this would be an additional argument for supporting VIDIOC_QUERYCAP on sub-devices. Further on, section 8, "Common definitions for V4L2 and V4L2 subdev interfaces", should probably be merged with the "common API elements" section. Just my thougts. Anyway... let's continue tomorrow. > > 3) "promote" subdev API to a separate part of the media spec, >just like what it was done for media controller, e. g. adding >a /Documentation/media/uapi/subdev directory and add there >descriptions for all syscalls that apply to subdevs >(open, close, ioctl). That would be weird from kAPI point of >view, as splitting it from V4L2 won't make sense there. So, >we'll likely need to add some notes at both kAPI and uAPI to >explain that the subdev API userspace API is just a different >way to expose V4L2 hardware control, but, internally, both >are implemented by the same V4L2 core. > > This patchset assumes (1). I'm ok if someone wants to do either > (2) or (3), but I won't have the required time to do such > changes. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 18/24] media: vb2-core: use bitops for bits
On Mon, Oct 09, 2017 at 07:19:24AM -0300, Mauro Carvalho Chehab wrote: > Use the existing macros to identify vb2_io_modes bits. > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com> -- Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/24] media: vb2: add cross references at memops and v4l2 kernel-doc markups
On Mon, Oct 09, 2017 at 07:19:28AM -0300, Mauro Carvalho Chehab wrote: > Add cross-references where needed and add periods at the end of > each kernel-doc paragraph, in order to make it coherent with other > VB2 descriptions. > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com> -- Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/24] media: vb2-core: Improve kernel-doc markups
Hi Mauro, On Mon, Oct 09, 2017 at 07:19:25AM -0300, Mauro Carvalho Chehab wrote: > There are several issues on the current markups: > - lack of cross-references; > - wrong cross-references; > - lack of a period of the end of several phrases; > - Some descriptions can be enhanced. > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> > --- > include/media/videobuf2-core.h | 376 > ++--- > 1 file changed, 204 insertions(+), 172 deletions(-) > > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 0308d8439049..e145f1475ffe 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h ... > /** > * vb2_core_queue_init() - initialize a videobuf2 queue > - * @q: videobuf2 queue; this structure should be allocated in > driver > + * @q: pointer to vb2_queue with videobuf2 queue. > + * This structure should be allocated in driver > * > * The _queue structure should be allocated by the driver. The driver is > * responsible of clearing it's content and setting initial values for some > * required entries before calling this function. > - * q->ops, q->mem_ops, q->type and q->io_modes are mandatory. Please refer > - * to the struct vb2_queue description in include/media/videobuf2-core.h > - * for more information. > + * > + * .. note:: > + * > + *The following fields at @q should be set before calling this function: should -> shall I bet there's a lot of that in the documentation elsewhere, including this patch. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/7] media: add glossary.rst with a glossary of terms used at V4L2 spec
On Tue, Oct 10, 2017 at 06:15:03AM -0300, Mauro Carvalho Chehab wrote: > Em Fri, 6 Oct 2017 14:51:06 +0300 > Sakari Ailus <sakari.ai...@iki.fi> escreveu: > > > Hi Mauro, > > > > On Fri, Oct 06, 2017 at 01:22:29PM +0300, Sakari Ailus wrote: > > > > +V4L2 device node > > > > + A device node that is associated to a V4L2 main driver, > > > > + as specified in :ref:`v4l2_device_naming`. > > > > I think we need to name the interface, not so much their instances. > > > > How about adding: > > > > V4L2 > > Video4Linux 2 interface. The interface implemented by **V4L2 device > > nodes**. > > > > and: > > > > V4L2 device node > > A device node implementing the **V4L2** interface. > > Not sure if I answered it already. subdev API is part of V4L2. > So, a change like that would cause more harm than good ;-) Hmm. There seems to be a gap here. It'd be much easier to maintain consistency in naming and definitions if V4L2 sub-device nodes were also documented to be V4L2 device nodes, just as any other device nodes exposed by drivers through the V4L2 framework. > > The definition should let it clear that only the devnodes > implemented by the V4L2 main driver are considered as > V4L2 device nodes. Why? I don't think we should make assumptions on which driver exposes a device node; this is not visible to the user space after all. -- Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/7] media: add glossary.rst with a glossary of terms used at V4L2 spec
On Tue, Oct 10, 2017 at 05:30:04AM -0300, Mauro Carvalho Chehab wrote: > Em Fri, 6 Oct 2017 13:22:29 +0300 > Sakari Ailus <sakari.ai...@iki.fi> escreveu: > > > > +Bridge driver > > > + The same as V4L2 main driver. > > > > Not all V4L2 main drivers can be bridge drivers. Mem-to-mem devices, for > > instance. How about: > > > > A driver for a device receiving image data from another device (or > > transmitting it to a sub-device) controlled by a sub-device driver. Bridge > > drivers typically act as V4L2 main drivers. > > That is not true for some device drivers we have. > > The GSPCA drivers are bridge drivers, but they don't use any sub-device > (well, it should, but nobody will redesign it, as the efforts would > be huge, for a very little gain). Also uvcdriver doesn't need sub-device > drivers, as the camera's internal firmware does the interface with the > sensors. > > We could, instead define it as: > > Bridge driver > A driver that provides a bridge between the CPU's bus to the > data and control buses of a media hardware. Often, the > bridge driver is the same as V4L2 main driver. Looks good to me. -- Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/24] media: v4l2-subdev: document remaining undocumented functions
each element at v4l2_subdev_ops. > + * @args...: arguments for @f. > * > * Example: err = v4l2_subdev_call(sd, video, s_std, norm); > */ > @@ -1048,6 +1100,14 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, > __result; \ > }) > > +/** > + * v4l2_subdev_has_op - Checks if a subdev defines a certain ops. s/ops/operation/ (or "op"). > + * > + * @sd: pointer to the v4l2_subdev > + * @o: name of the element at v4l2_subdev_ops that contains @f. > + * Each element there groups a set of callbacks functions. How about: @o: The group of callback functions in v4l2_subdev_ops which @f is a part of. > + * @f: callback function to be checked for its existence. > + */ > #define v4l2_subdev_has_op(sd, o, f) \ > ((sd)->ops->o && (sd)->ops->o->f) > With these considered, Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com> -- Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 17/24] media: v4l2-subdev: fix a typo
On Mon, Oct 09, 2017 at 07:19:23AM -0300, Mauro Carvalho Chehab wrote:
> ownner -> owner
>
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> ---
> include/media/v4l2-subdev.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index e215732eed45..345da052618c 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -814,7 +814,7 @@ struct v4l2_subdev_platform_data {
> * @list: List of sub-devices
> * @owner: The owner is the same as the driver's device owner.
> * @owner_v4l2_dev: true if the >owner matches the owner of
> @v4l2_dev->dev
> - * ownner. Initialized by v4l2_device_register_subdev().
> + * owner. Initialized by v4l2_device_register_subdev().
> * @flags: subdev flags, as defined by v4l2_subdev_flags.
> *
> * @v4l2_dev: pointer to struct _device
Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com>
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/24] media: v4l2-subdev: use kernel-doc markups to document subdev flags
Hi Mauro, On Mon, Oct 09, 2017 at 07:19:16AM -0300, Mauro Carvalho Chehab wrote: > Right now, those are documented together with the subdev struct, > instead of together with the definitions. > > Convert the definitions to an enum, use BIT() macros and document > it at its right place. > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> For patches 10--14: Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com> -- Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/24] media: v4l2-subdev: get rid of __V4L2_SUBDEV_MK_GET_TRY() macro
Hi Mauro,
On Mon, Oct 09, 2017 at 07:19:21AM -0300, Mauro Carvalho Chehab wrote:
> The __V4L2_SUBDEV_MK_GET_TRY() macro is used to define
> 3 functions that have the same arguments. The code of those
> functions is simple enough to just declare them, de-obfuscating
> the code.
>
> While here, replace BUG_ON() by WARN_ON() as there's no reason
> why to panic the Kernel if this fails.
BUG_ON() might actually be a better idea as this will lead to memory
corruption. I presume it's not been hit often.
That said, I, too, favour WARN_ON() in this case. In case pad exceeds the
number of pads, then zero could be used, for instance. The only real
problem comes if there were no pads to begin with. The callers of these
functions also don't expect to receive NULL. Another option would be to
define a static, dummy variable for the purpose that would be at least safe
to access. Or we could just use the dummy entry whenever the pad isn't
valid.
This will make the functions more complex and I might just keep the
original macro. Even grep works on it nowadays.
>
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> ---
> include/media/v4l2-subdev.h | 37 +
> 1 file changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 1f34045f07ce..35c4476c56ee 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -897,19 +897,32 @@ struct v4l2_subdev_fh {
> container_of(fh, struct v4l2_subdev_fh, vfh)
>
> #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> -#define __V4L2_SUBDEV_MK_GET_TRY(rtype, fun_name, field_name)
> \
> - static inline struct rtype *\
> - fun_name(struct v4l2_subdev *sd,\
> - struct v4l2_subdev_pad_config *cfg,\
> - unsigned int pad) \
> - { \
> - BUG_ON(pad >= sd->entity.num_pads); \
> - return [pad].field_name;\
> - }
> +static inline struct v4l2_mbus_framefmt
> +*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + unsigned int pad)
> +{
> + WARN_ON(pad >= sd->entity.num_pads);
> + return [pad].try_fmt;
> +}
>
> -__V4L2_SUBDEV_MK_GET_TRY(v4l2_mbus_framefmt, v4l2_subdev_get_try_format,
> try_fmt)
> -__V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, v4l2_subdev_get_try_crop, try_crop)
> -__V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, v4l2_subdev_get_try_compose, try_compose)
> +static inline struct v4l2_rect
> +*v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + unsigned int pad)
> +{
> + WARN_ON(pad >= sd->entity.num_pads);
> + return [pad].try_crop;
> +}
> +
> +static inline struct v4l2_rect
> +*v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + unsigned int pad)
> +{
> + WARN_ON(pad >= sd->entity.num_pads);
> + return [pad].try_compose;
> +}
> #endif
>
> extern const struct v4l2_file_operations v4l2_subdev_fops;
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/24] media: v4l2-mediabus: use BIT() macro for flags
On Mon, Oct 09, 2017 at 07:19:09AM -0300, Mauro Carvalho Chehab wrote: > Instead of using (1 << n) for bits, use the BIT() macro, > as it makes them better documented. I wouldn't say this makes a difference from documentation point of view. Anyway, Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com> > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> -- Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/24] media: v4l2-flash-led-class.h: add kernel-doc to two ancillary funcs
Hi Mauro,
Thanks for the patch.
On Mon, Oct 09, 2017 at 07:19:08AM -0300, Mauro Carvalho Chehab wrote:
> There are two ancillary functions at v4l2-flash-led-class.h
> that aren't documented.
>
> Document them.
>
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> ---
> include/media/v4l2-flash-led-class.h | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/include/media/v4l2-flash-led-class.h
> b/include/media/v4l2-flash-led-class.h
> index 5c1d50f78e12..39a5daa977aa 100644
> --- a/include/media/v4l2-flash-led-class.h
> +++ b/include/media/v4l2-flash-led-class.h
> @@ -91,12 +91,24 @@ struct v4l2_flash {
> struct v4l2_ctrl **ctrls;
> };
>
> +/**
> + * v4l2_subdev_to_v4l2_flash - Returns a _flash from the
v4l2_flash -> struct v4l2_flash ?
Same below. With these,
Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> + * v4l2_subdev embedded on it.
> + *
> + * @sd: pointer to v4l2_subdev
> + */
> static inline struct v4l2_flash *v4l2_subdev_to_v4l2_flash(
> struct v4l2_subdev *sd)
> {
> return container_of(sd, struct v4l2_flash, sd);
> }
>
> +/**
> + * v4l2_ctrl_to_v4l2_flash - Returns a _flash from the
> + * v4l2_ctrl embedded on it.
> + *
> + * @c: pointer to v4l2_ctrl
> + */
> static inline struct v4l2_flash *v4l2_ctrl_to_v4l2_flash(struct v4l2_ctrl *c)
> {
> return container_of(c->handler, struct v4l2_flash, hdl);
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/24] media: v4l2-dev.h: add kernel-doc to two macros
Hi Mauro, On Mon, Oct 09, 2017 at 07:19:07AM -0300, Mauro Carvalho Chehab wrote: > There are two macros at v4l2-dev.h that aren't documented. > > Document them, for completeness. > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> > --- > include/media/v4l2-dev.h | 18 +++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h > index e657614521e3..de1a1453cfd9 100644 > --- a/include/media/v4l2-dev.h > +++ b/include/media/v4l2-dev.h > @@ -260,9 +260,21 @@ struct video_device > struct mutex *lock; > }; > > -#define media_entity_to_video_device(__e) \ > - container_of(__e, struct video_device, entity) > -/* dev to video-device */ > +/** > + * media_entity_to_video_device - Returns a video_device from > + * the media_entity embedded on it. > + * > + * @entity: pointer to media_entity > + */ > +#define media_entity_to_video_device(entity) \ > + container_of(entity, struct video_device, entity) > + > +/** > + * media_entity_to_video_device - Returns a video_device from -> to_video_device With that, Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com> > + * the device embedded on it. > + * > + * @cd: pointer to device > + */ > #define to_video_device(cd) container_of(cd, struct video_device, dev) > > /** -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 4/7] media: open.rst: document devnode-centric and mc-centric types
Hi Mauro, On Wed, Sep 27, 2017 at 07:23:46PM -0300, Mauro Carvalho Chehab wrote: > When we added support for omap3, back in 2010, we added a new > type of V4L2 devices that aren't fully controlled via the V4L2 > device node. > > Yet, we have never clearly documented in the V4L2 specification > the differences between the two types. > > Let's document them based on the the current implementation. > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> > --- > Documentation/media/uapi/v4l/open.rst | 40 > +++ > 1 file changed, 40 insertions(+) > > diff --git a/Documentation/media/uapi/v4l/open.rst > b/Documentation/media/uapi/v4l/open.rst > index 18030131ef77..f603bc9b49a0 100644 > --- a/Documentation/media/uapi/v4l/open.rst > +++ b/Documentation/media/uapi/v4l/open.rst > @@ -7,6 +7,46 @@ Opening and Closing Devices > *** > > > +.. _v4l2_hardware_control: > + > + > +Types of V4L2 media hardware control > + > + > +V4L2 hardware periferal is usually complex: support for it is > +implemented via a V4L2 main driver and often by several additional drivers. > +The main driver always exposes one or more **V4L2 device nodes** > +(see :ref:`v4l2_device_naming`) with are responsible for implementing > +data streaming, if applicable. > + > +The other drivers are called **V4L2 sub-devices** and provide control to s/drivers/devices/ > +other hardware components usually connected via a serial bus (like > +I²C, SMBus or SPI). Depending on the main driver, they can be implicitly > +controlled directly by the main driver or explicitly via > +the **V4L2 sub-device API** (see :ref:`subdev`). > + > +When V4L2 was originally designed, there was only one type of > +media hardware control: via the **V4L2 device nodes**. We refer to this kind > +of control as **V4L2 device node centric** (or, simply, > "**vdevnode-centric**"). I think this could be easier understood if we start with the differences instead of what we call the types. I'd also refer to interfaces rather than their instances. How about (pending finalising discussion on naming): When **V4L2** was originally designed, the **V4L2 device** served the purpose of both control and data interfaces and there was no separation between the two interface-wise. V4L2 controls, setting inputs and outputs, format configuration and buffer related operations were all performed through the same **V4L2 device nodes**. Devices offering such interface are called **V4L2 device node centric**. Later on, support for the **Media controller** interface was added to V4L2 in order to better support complex **V4L2 aggregate devices** where the **V4L2** interface alone could no longer meaningfully serve as both a control and a data interface. On such aggregate devices, **V4L2** interface remains a data interface whereas control takes place through the **Media controller** and **V4L2 sub-device** interfaces. Stream control is an exception to this: streaming is enabled and disabled through the **V4L2** interface. These devices are called **Media controller centric**. **MC-centric** aggregate devices provide more versatile control of the hardware than **V4L2 device node centric** devices. On **MC-centric** aggregate devices the **V4L2 sub-device nodes** represent specific parts of the **V4L2 aggregate device**, to which they enable control. Also, the additional versatility of **MC-centric** aggregate devices comes with additional responsibilities, the main one of which is the requirements of the user configuring the device pipeline before starting streaming. This typically involves configuring the links using the **Media controller** interface and the media bus formats on pads (at both ends of the links) using the **V4L2 sub-device** interface. > + > +Later (kernel 2.6.39), a new type of periferal control was > +added in order to support complex media hardware that are common for embedded > +systems. This type of periferal is controlled mainly via the media > +controller and V4L2 sub-devices. So, it is called > +**Media controller centric** (or, simply, "**MC-centric**") control. > + > +For **vdevnode-centric** media hardware control, the media hardware is > +controlled via the **V4L2 device nodes**. They may optionally support the > +:ref:`media controller API ` as well, > +in order to inform the application which device nodes are available > +(see :ref:`related`). > + > +For **MC-centric** media hardware control it is required to configure > +the pipelines via the :ref:`media controller API ` before > +the periferal can be used. For such devices, the sub-devices' configuration > +can be controlled via the :ref:`sub-device API `, which creates one > +dev
Re: [PATCH v7 1/7] media: add glossary.rst with a glossary of terms used at V4L2 spec
Hi Mauro, On Fri, Oct 06, 2017 at 01:22:29PM +0300, Sakari Ailus wrote: > > +V4L2 device node > > + A device node that is associated to a V4L2 main driver, > > + as specified in :ref:`v4l2_device_naming`. I think we need to name the interface, not so much their instances. How about adding: V4L2 Video4Linux 2 interface. The interface implemented by **V4L2 device nodes**. and: V4L2 device node A device node implementing the **V4L2** interface. -- Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/7] media: open.rst: better document device node naming
On Tue, Aug 29, 2017 at 10:17:51AM -0300, Mauro Carvalho Chehab wrote: > Right now, only kAPI documentation describes the device naming. > However, such description is needed at the uAPI too. Add it, > and describe how to get an unique identify for a given device. > > Acked-by: Hans Verkuil <hans.verk...@cisco.com> > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com> -- Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/7] media: add glossary.rst with a glossary of terms used at V4L2 spec
Hi Mauro, On Thu, Oct 05, 2017 at 09:26:51AM -0300, Mauro Carvalho Chehab wrote: > > > > > + > > > > > + See :ref:`media_controller`. > > > > > + > > > > > +MC-centric > > > > > + V4L2 hardware that requires a Media controller. > > > > > + > > > > > + See :ref:`v4l2_hardware_control`. > > > > > + > > > > > +Microprocessor > > > > > + An electronic circuitry that carries out the instructions > > > > > + of a computer program by performing the basic arithmetic, > > > > > logical, > > > > > + control and input/output (I/O) operations specified by the > > > > > + instructions on a single integrated circuit. > > > > > + > > > > > +SMBus > > > > > + A subset of I²C, with defines a stricter usage of the bus. > > > > > + > > > > > +Serial Peripheral Interface Bus - SPI > > > > > > > > We don't have "Bus" in I²C, I'd leave it out here, too. > > > > > > I2C is a serial bus (and it is implemented as a bus inside the Kernel). > > > Take a look at Documentation/i2c/summary. > > > > I don't disagree with that, but at the same time this is not related to my > > suggestion. > > > > "Bus" is not part of the abbreviation SPI, therefore we should not suggest > > that here. > > Ah, so you proposal here is just to replace: > > Serial Peripheral Interface Bus - SPI > > To > Serial Peripheral Interface - SPI > > Right? If so, it sounds OK. Yes, please. That's exactly what I had in mind. ... > > > > > +V4L2 hardware > > > > > + A hardware used to on a media device supported by the V4L2 > > > > > + subsystem. > > > > > + > > > > > +V4L2 hardware control > > > > > + The type of hardware control that a device supports. > > > > > + > > > > > + See :ref:`v4l2_hardware_control`. > > > > > + > > > > > +V4L2 main driver > > > > > + The V4L2 device driver that implements the main logic to talk > > > > > with > > > > > + the V4L2 hardware. > > > > > + > > > > > + Also known as bridge driver. > > > > > > > > Is UVC driver a bridge driver? How about instead: > > > > > > Yes, sure: UVC driver is a bridge driver/main driver. It is the UVC driver > > > that sends/receives data from the USB bus and send to the sensors. > > > It also sends data via URB to the USB host driver, with, in turn send it > > > to send to CPU (usually via DMA - although some USB drivers actually > > > implement direct I/O for short messages). > > > > > > > Bridge and ISP drivers typically are V4L2 main drivers. > > > > > > We don't have a concept of an "ISP driver". Adding it sounds very > > > > I think we do have that roughly as much as we do have bridge driver. We > > definitely also support devices that are called ISPs, therefore we do have > > ISP drivers. > > We have drivers for things implemented via ISP. However, right now, > there's no distinction at the driver if the functionality is implemented > on software (ISP) or in hardware. > > > > > > confusing, as an ISP hardware may actually implement different > > > functions - so it ends by being supported by multiple drivers. > > > > Typically ISPs are controlled by a single driver as the sub-blocks in an > > ISP usually can only be found in that very ISP. > > I'm almost sure that this is not true for Exynos drivers. There are > m2m drivers and normal drivers for the same ISP (doing different things, > like format conversion, scaling, etc). I don't know the Exynos hardware. There are exceptions but they tend to be increasingly rare as extra memory hops hurt performance and increase power consumption in common use cases. If there is a split to multiple devices, then usually the first device is CSI-2 receiver plus DMA (bridge) and the second is the ISP (i.e. where the processing happens). -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/7] media: add glossary.rst with a glossary of terms used at V4L2 spec
On Thu, Oct 05, 2017 at 03:39:29PM -0300, Mauro Carvalho Chehab wrote: > Em Thu, 5 Oct 2017 11:21:07 +0300 > Sakari Ailus <sakari.ai...@iki.fi> escreveu: > > > Hi Mauro, > > > > My apologies for the late reply. > > > > On Tue, Aug 29, 2017 at 10:07:50AM -0300, Mauro Carvalho Chehab wrote: > > > Em Tue, 29 Aug 2017 10:47:48 +0300 > > > Sakari Ailus <sakari.ai...@iki.fi> escreveu: > > > > > > > Hi Mauro, > > > > > > > > Thanks for the update. A few comments below. > > > > > > > > On Mon, Aug 28, 2017 at 09:53:55AM -0300, Mauro Carvalho Chehab wrote: > > > > > Add a glossary of terms for V4L2, as several concepts are complex > > > > > enough to cause misunderstandings. > > > > > > > > > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> > > > > > --- > > > > > Documentation/media/uapi/v4l/glossary.rst | 147 > > > > > ++ > > > > > Documentation/media/uapi/v4l/v4l2.rst | 1 + > > > > > 2 files changed, 148 insertions(+) > > > > > create mode 100644 Documentation/media/uapi/v4l/glossary.rst > > > > > > > > > > diff --git a/Documentation/media/uapi/v4l/glossary.rst > > > > > b/Documentation/media/uapi/v4l/glossary.rst > > > > > new file mode 100644 > > > > > index ..0b6ab5adec81 > > > > > --- /dev/null > > > > > +++ b/Documentation/media/uapi/v4l/glossary.rst > > > > > @@ -0,0 +1,147 @@ > > > > > + > > > > > +Glossary > > > > > + > > > > > + > > > > > +.. note:: > > > > > + > > > > > + This goal of section is to standardize the terms used within the > > > > > V4L2 > > > > > + documentation. It is written incrementally as they are > > > > > standardized in > > > > > + the V4L2 documentation. So, it is a Work In Progress. > > > > > > > > I'd leave the WiP part out. > > > > > > IMO, it is important to mention it, as the glossary, right now, covers > > > only what's used on the first two sections of the API book. There are > > > a lot more to be covered. > > > > Works for me. > > > > > > > > > > > > > > + > > > > > +.. Please keep the glossary entries in alphabetical order > > > > > + > > > > > +.. glossary:: > > > > > + > > > > > +Bridge driver > > > > > + The same as V4L2 main driver. > > > > > > > > I've understood bridges being essentially a bus receiver + DMA. Most > > > > ISPs > > > > contain both but have more than that. How about: > > > > > > > > A driver for a bus (e.g. parallel, CSI-2) receiver and DMA. Bridge > > > > drivers > > > > typically act as V4L2 main drivers. > > > > > > No, only on some drivers the bridge driver has DMA. A vast amount of > > > drivers (USB ones) don't implement any DMA inside the driver, as it is > > > up to the USB host driver to implement support for DMA. > > > > > > There are even some USB host drivers that don't always use DMA for I/O > > > transfers, using direct I/O if the message is smaller than a threshold > > > or not multiple of the bus word. This is pretty common on SoC USB host > > > drivers. > > > > > > In any case, for the effect of this spec, and for all discussions we > > > ever had about it, bridge driver == V4L2 main driver. I don't > > > see any reason why to distinguish between them. > > > > I think you should precisely define what a bridge driver means. Generally > > ISP drivers aren't referred to as bridge drivers albeit they, too, function > > as V4L2 main drivers. > > Btw, this is already defined, currently, at v4l2-subdev.h: > > * Sub-devices are devices that are connected somehow to the main bridge > * device. These devices are usually audio/video muxers/encoders/decoders or > * sensors and webcam controllers. > * > * Usually these devices are controlled through an i2c bus, but other busses > * may also be used. > > Please notice that there it says: "main bridge" :-) > > Such definition was added since the beginning of the subdev concept, back in > 2008 and was reviewed by several V4L core developers: A lot has happened since 2008. :-) Anyway, I'll review the latest set. -- Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/7] media: add glossary.rst with a glossary of terms used at V4L2 spec
Hi Mauro, My apologies for the late reply. On Tue, Aug 29, 2017 at 10:07:50AM -0300, Mauro Carvalho Chehab wrote: > Em Tue, 29 Aug 2017 10:47:48 +0300 > Sakari Ailus <sakari.ai...@iki.fi> escreveu: > > > Hi Mauro, > > > > Thanks for the update. A few comments below. > > > > On Mon, Aug 28, 2017 at 09:53:55AM -0300, Mauro Carvalho Chehab wrote: > > > Add a glossary of terms for V4L2, as several concepts are complex > > > enough to cause misunderstandings. > > > > > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> > > > --- > > > Documentation/media/uapi/v4l/glossary.rst | 147 > > > ++ > > > Documentation/media/uapi/v4l/v4l2.rst | 1 + > > > 2 files changed, 148 insertions(+) > > > create mode 100644 Documentation/media/uapi/v4l/glossary.rst > > > > > > diff --git a/Documentation/media/uapi/v4l/glossary.rst > > > b/Documentation/media/uapi/v4l/glossary.rst > > > new file mode 100644 > > > index ..0b6ab5adec81 > > > --- /dev/null > > > +++ b/Documentation/media/uapi/v4l/glossary.rst > > > @@ -0,0 +1,147 @@ > > > + > > > +Glossary > > > + > > > + > > > +.. note:: > > > + > > > + This goal of section is to standardize the terms used within the V4L2 > > > + documentation. It is written incrementally as they are standardized in > > > + the V4L2 documentation. So, it is a Work In Progress. > > > > I'd leave the WiP part out. > > IMO, it is important to mention it, as the glossary, right now, covers > only what's used on the first two sections of the API book. There are > a lot more to be covered. Works for me. > > > > > > + > > > +.. Please keep the glossary entries in alphabetical order > > > + > > > +.. glossary:: > > > + > > > +Bridge driver > > > + The same as V4L2 main driver. > > > > I've understood bridges being essentially a bus receiver + DMA. Most ISPs > > contain both but have more than that. How about: > > > > A driver for a bus (e.g. parallel, CSI-2) receiver and DMA. Bridge drivers > > typically act as V4L2 main drivers. > > No, only on some drivers the bridge driver has DMA. A vast amount of > drivers (USB ones) don't implement any DMA inside the driver, as it is > up to the USB host driver to implement support for DMA. > > There are even some USB host drivers that don't always use DMA for I/O > transfers, using direct I/O if the message is smaller than a threshold > or not multiple of the bus word. This is pretty common on SoC USB host > drivers. > > In any case, for the effect of this spec, and for all discussions we > ever had about it, bridge driver == V4L2 main driver. I don't > see any reason why to distinguish between them. I think you should precisely define what a bridge driver means. Generally ISP drivers aren't referred to as bridge drivers albeit they, too, function as V4L2 main drivers. > > > > > > + > > > +Device Node > > > + A character device node in the file system used to control and do > > > + input/output data transfers from/to a Kernel driver. > > > + > > > +Digital Signal Processor - DSP > > > + A specialized microprocessor, with its architecture optimized for > > > + the operational needs of digital signal processing. > > > + > > > +Driver > > > + The part of the Linux Kernel that implements support > > > + for a hardware component. > > > + > > > +Field-programmable Gate Array - FPGA > > > + A field-programmable gate array (FPGA) is an integrated circuit > > > + designed to be configured by a customer or a designer after > > > + manufacturing. > > > + > > > + See https://en.wikipedia.org/wiki/Field-programmable_gate_array. > > > + > > > +Hardware component > > > + A subset of the media hardware. For example an I²C or SPI device, > > > + or an IP block inside an SoC or FPGA. > > > + > > > +Hardware peripheral > > > + A group of hardware components that together make a larger > > > + user-facing functional peripheral. For instance the SoC ISP IP > > > + cores and external camera sensors together make a > > > + camera hardware peripheral. > > > + > > > + Also known as peripheral. > > > > Aren't peripherals usually understood to be devices that you can plug into > > a c
Re: [PATCH v2 13/17] media: v4l2-async: simplify v4l2_async_subdev structure
Hi Mauro,
(Removing the non-list recipients.)
On Fri, Sep 29, 2017 at 06:27:13AM -0300, Mauro Carvalho Chehab wrote:
> Em Thu, 28 Sep 2017 15:09:21 +0300
> Sakari Ailus <sakari.ai...@iki.fi> escreveu:
>
> > Hi Mauro,
> >
> > On Wed, Sep 27, 2017 at 06:46:56PM -0300, Mauro Carvalho Chehab wrote:
> > > The V4L2_ASYNC_MATCH_FWNODE match criteria requires just one
> > > struct to be filled (struct fwnode_handle). The V4L2_ASYNC_MATCH_DEVNAME
> > > match criteria requires just a device name.
> > >
> > > So, it doesn't make sense to enclose those into structs,
> > > as the criteria can go directly into the union.
> > >
> > > That makes easier to document it, as we don't need to document
> > > weird senseless structs.
> >
> > The idea is that in the union, there's a struct which is specific to the
> > match_type field. I wouldn't call it senseless.
>
> Why a struct for each specific match_type is **needed**? It it is not
> needed, then it is senseless per definition :-)
>
> In the specific case of fwnode, there's already a named struct
> for fwnode_handle. The only thing is that it is declared outside
> enum v4l2_async_match_type. So, I don't see any reason to do things
> like:
>
> struct {
> struct fwnode_handle *fwnode;
> } fwnode;
>
> If you're in doubt about that, think on how would you document
> both fwnode structs. Both fwnode structs specify the match
> criteria if %V4L2_ASYNC_MATCH_FWNODE.
>
> The same applies to this:
>
> struct {
> const char *name;
> } device_name;
>
> Both device_name and name specifies the match criteria if
> %V4L2_ASYNC_MATCH_DEVNAME.
>
> >
> > In the two cases there's just a single field in the containing struct. You
> > could remove the struct in that case as you do in this patch, and just use
> > the field. But I think the result is less clean and so I wouldn't make this
> > change.
>
> It is actually cleaner without the stucts.
>
> Without the useless struct, if one wants to match a firmware node, it
> should be doing:
>
> pdata->asd[i]->match_type = V4L2_ASYNC_MATCH_FWNODE;
> pdata->asd[i]->match.fwnode = of_fwnode_handle(rem);
This code should be and will be moved out of drivers. See:
<URL:http://www.spinics.net/lists/linux-media/msg122688.html>
So there are going to be quite a bit fewer instances of it, and none should
remain in drivers. I frankly don't have a strong opinion on this; there are
arguments for and against. I just don't see a reason to change it.
It'd be nice to have Hans's and Laurent's opinion though.
>
> And that' it. For anyone that reads the above code, even not knowing
> all details of "match", is clear that the match criteria is whatever
> of_fwnode_handle() returns.
>
> Now, on this:
>
> pdata->asd[i]->match_type = V4L2_ASYNC_MATCH_FWNODE;
> pdata->asd[i]->match.fwnode.fwnode = of_fwnode_handle(rem);
>
> It sounds that something is missing, as only one field of match.fwnode
> was specified. Anyone not familiar with that non-conventional usage of
> a struct with just one struct field inside would need to seek for the
> header file declaring the struct. That would consume a lot of time for
> code reviewers for no good reason.
>
> The same apply for devname search:
>
> In this case:
> asd->match_type = V4L2_ASYNC_MATCH_DEVNAME;
> asd->match.device_name.name = imxsd->devname;
>
> I would be expecting something else to be filled at device_name's
> struct, for example to specify a case sensitive or case insensitive
> match criteria, to allow seeking for a device's substring, or to
> allow using other struct device fields to narrow the seek.
>
> With this:
>
> asd->match_type = V4L2_ASYNC_MATCH_DEVNAME;
> asd->match.device_name = imxsd->devname;
>
> It is clear that the match criteria is fully specified.
>
> > The confusion comes possibly from the fact that the struct is named the
> > same as the field in the struct. These used to be called of and node, but
> > with the fwnode property framework the references to the fwnode are, well,
> > typically similarly called "fwnode". There's no underlying firmware
> > interface with that name, fwnode property API is just an API.
>
> The duplicated "fwnode" name only made it more evident that we don't
> need to enclose a single match criteria field inside a struct.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 6/7] media: videodev2: add a flag for MC-centric devices
On Wed, Sep 27, 2017 at 07:23:48PM -0300, Mauro Carvalho Chehab wrote: > As both vdev-centric and MC-centric devices may implement the > same APIs, we need a flag to allow userspace to distinguish > between them. > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com> -- Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 7/7] media: open.rst: add a notice about subdev-API on vdev-centric
On Wed, Sep 27, 2017 at 07:23:49PM -0300, Mauro Carvalho Chehab wrote: > The documentation doesn't mention if vdev-centric hardware > control would have subdev API or not. > > Add a notice about that, reflecting the current status, where > three drivers use it, in order to support some subdev-specific > controls. > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> Thanks! Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com> -- Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v2 13/17] media: v4l2-async: simplify v4l2_async_subdev structure
Hi Mauro, On Wed, Sep 27, 2017 at 06:46:56PM -0300, Mauro Carvalho Chehab wrote: > The V4L2_ASYNC_MATCH_FWNODE match criteria requires just one > struct to be filled (struct fwnode_handle). The V4L2_ASYNC_MATCH_DEVNAME > match criteria requires just a device name. > > So, it doesn't make sense to enclose those into structs, > as the criteria can go directly into the union. > > That makes easier to document it, as we don't need to document > weird senseless structs. The idea is that in the union, there's a struct which is specific to the match_type field. I wouldn't call it senseless. In the two cases there's just a single field in the containing struct. You could remove the struct in that case as you do in this patch, and just use the field. But I think the result is less clean and so I wouldn't make this change. The confusion comes possibly from the fact that the struct is named the same as the field in the struct. These used to be called of and node, but with the fwnode property framework the references to the fwnode are, well, typically similarly called "fwnode". There's no underlying firmware interface with that name, fwnode property API is just an API. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/17] V4L cleanups and documentation improvements
Hi Mauro, On Wed, Sep 27, 2017 at 06:46:43PM -0300, Mauro Carvalho Chehab wrote: > This patch series is meant to improve V4L documentation. It touches > some files at the tree doing some cleanup, in order to simplify > the source code. > > Mauro Carvalho Chehab (17): > media: tuner-types: add kernel-doc markups for struct tunertype > media: v4l2-common: get rid of v4l2_routing dead struct > media: v4l2-common: get rid of struct v4l2_discrete_probe > media: v4l2-common.h: document ancillary functions > media: v4l2-device.h: document ancillary macros > media: v4l2-dv-timings.h: convert comment into kernel-doc markup > media: v4l2-event.rst: improve events description > media: v4l2-ioctl.h: convert debug macros into enum and document > media: cec-pin.h: convert comments for cec_pin_state into kernel-doc > media: rc-core.rst: add an introduction for RC core > media: rc-core.h: minor adjustments at rc_driver_type doc > media: v4l2-fwnode.h: better describe bus union at fwnode endpoint > struct > media: v4l2-async: simplify v4l2_async_subdev structure > media: v4l2-async: better describe match union at async match struct > media: v4l2-ctrls: document nested members of structs > media: videobuf2-core: improve kernel-doc markups > media: media-entity.h: add kernel-doc markups for nested structs For patches apart form 7 and 13 (see my comments to them): Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com> -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 07/17] media: v4l2-event.rst: improve events description
Hi Mauro, On Wed, Sep 27, 2017 at 06:46:50PM -0300, Mauro Carvalho Chehab wrote: > Both v4l2-event.rst and v4l2-event.h have an overview of > events, but there are some inconsistencies there: > > - at v4l2-event, the event's ringbuffer is called kevent. Its s/ringbuffer/ring buffer/g With that, Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com> > name is, instead, v4l2_kevent; > > - Some things are mentioned on both places (with different words), > others are either on one of the files. > > In order to cleanup this mess, put everything at v4l2-event.rst > and improve it to be a little more coherent and to have cross > references. > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> > --- > Documentation/media/kapi/v4l2-event.rst | 64 > ++--- > include/media/v4l2-event.h | 34 -- > 2 files changed, 52 insertions(+), 46 deletions(-) > > diff --git a/Documentation/media/kapi/v4l2-event.rst > b/Documentation/media/kapi/v4l2-event.rst > index 9938d21ef4d1..7831a503e2aa 100644 > --- a/Documentation/media/kapi/v4l2-event.rst > +++ b/Documentation/media/kapi/v4l2-event.rst > @@ -5,27 +5,67 @@ V4L2 events > The V4L2 events provide a generic way to pass events to user space. > The driver must use :c:type:`v4l2_fh` to be able to support V4L2 events. > > -Events are defined by a type and an optional ID. The ID may refer to a V4L2 > -object such as a control ID. If unused, then the ID is 0. > +Events are subscribed per-filehandle. An event specification consists of a > +``type`` and is optionally associated with an object identified through the > +``id`` field. If unused, then the ``id`` is 0. So an event is uniquely > +identified by the ``(type, id)`` tuple. > > -When the user subscribes to an event the driver will allocate a number of > -kevent structs for that event. So every (type, ID) event tuple will have > -its own set of kevent structs. This guarantees that if a driver is generating > -lots of events of one type in a short time, then that will not overwrite > -events of another type. > +The :c:type:`v4l2_fh` struct has a list of subscribed events on its > +``subscribed`` field. > > -But if you get more events of one type than the number of kevents that were > -reserved, then the oldest event will be dropped and the new one added. > +When the user subscribes to an event, a :c:type:`v4l2_subscribed_event` > +struct is added to :c:type:`v4l2_fh`\ ``.subscribed``, one for every > +subscribed event. > + > +Each :c:type:`v4l2_subscribed_event` struct ends with a > +:c:type:`v4l2_kevent` ringbuffer, with the size given by the caller > +of :c:func:`v4l2_event_subscribe`. Such ringbuffer is used to store any > events > +raised by the driver. > + > +So every ``(type, ID)`` event tuple will have its own set of > +:c:type:`v4l2_kevent` ringbuffer. This guarantees that if a driver is > +generating lots of events of one type in a short time, then that will > +not overwrite events of another type. > + > +But if you get more events of one type than the size of the > +:c:type:`v4l2_kevent` ringbuffer, then the oldest event will be dropped > +and the new one added. > + > +The :c:type:`v4l2_kevent` struct links into the ``available`` > +list of the :c:type:`v4l2_fh` struct so :ref:`VIDIOC_DQEVENT` will > +know which event to dequeue first. > + > +Finally, if the event subscription is associated with a particular object > +such as a V4L2 control, then that object needs to know about that as well > +so that an event can be raised by that object. So the ``node`` field can > +be used to link the :c:type:`v4l2_subscribed_event` struct into a list of > +such object. > + > +So to summarize: > + > +- struct :c:type:`v4l2_fh` has two lists: one of the ``subscribed`` events, > + and one of the ``available`` events. > + > +- struct :c:type:`v4l2_subscribed_event` has a ringbuffer of raised > + (pending) events of that particular type. > + > +- If struct :c:type:`v4l2_subscribed_event` is associated with a specific > + object, then that object will have an internal list of > + struct :c:type:`v4l2_subscribed_event` so it knows who subscribed an > + event to that object. > > Furthermore, the internal struct :c:type:`v4l2_subscribed_event` has > ``merge()`` and ``replace()`` callbacks which drivers can set. These > callbacks are called when a new event is raised and there is no more room. > + > The ``replace()`` callback allows you to replace the payload of the old event > with that of the new event, merging any relevant data from the old payload > into the new payload that replaces it. It is called when this event type has > -only one
Re: [PATCH v2 12/17] media: v4l2-fwnode.h: better describe bus union at fwnode endpoint struct
Hi Mauro,
Thanks for the patch.
On Wed, Sep 27, 2017 at 06:46:55PM -0300, Mauro Carvalho Chehab wrote:
> Now that kernel-doc handles nested unions, better document the
> bus union at struct v4l2_fwnode_endpoint.
>
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> ---
> include/media/v4l2-fwnode.h | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 7adec9851d9e..5f4716f967d0 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -79,7 +79,18 @@ struct v4l2_fwnode_bus_mipi_csi1 {
> * struct v4l2_fwnode_endpoint - the endpoint data structure
> * @base: fwnode endpoint of the v4l2_fwnode
> * @bus_type: bus type
> - * @bus: bus configuration data structure
> + * @bus: union with bus configuration data structure
> + * @bus.parallel: pointer for v4l2_fwnode_bus_parallel.
> + * Used if the bus is parallel.
> + * @bus.mipi_csi1: pointer for v4l2_fwnode_bus_mipi_csi1.
> + * Used if the bus is Mobile Industry Processor
s/Mobile.*Interface/MIPI Alliance/
Same below.
Please see:
<URL:https://mipi.org/content/what-does-mipi-stand>
With that,
Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> + * Interface's Camera Serial Interface version 1
> + * (MIPI CSI1) or Standard Mobile Imaging Architecture's
> + * Compact Camera Port 2 (SMIA CCP2).
> + * @bus.mipi_csi2: pointer for v4l2_fwnode_bus_mipi_csi2.
> + * Used if the bus is Mobile Industry Processor
> + * Interface's Camera Serial Interface version 2
> + * (MIPI CSI2).
> * @link_frequencies: array of supported link frequencies
> * @nr_of_link_frequencies: number of elements in link_frequenccies array
> */
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] docs-rst: media: Don't use \small for V4L2_PIX_FMT_SRGGB10 documentation
There appears to be an issue in using \small in certain cases on Sphinx
1.4 and 1.5. Other format documents don't use \small either, remove it
from here as well.
Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
---
Hi Mauro,
What would you think of this as an alternative approach? No hacks needed.
Just a recognition \small could have issues. For what it's worth, I
couldn't reproduce the issue on Sphinx 1.4.9.
Regards,
Sakari
Documentation/media/uapi/v4l/pixfmt-srggb10p.rst | 11 ---
1 file changed, 11 deletions(-)
diff --git a/Documentation/media/uapi/v4l/pixfmt-srggb10p.rst
b/Documentation/media/uapi/v4l/pixfmt-srggb10p.rst
index 86cd07e5bfa3..368ee61ab209 100644
--- a/Documentation/media/uapi/v4l/pixfmt-srggb10p.rst
+++ b/Documentation/media/uapi/v4l/pixfmt-srggb10p.rst
@@ -33,13 +33,6 @@ of a small V4L2_PIX_FMT_SBGGR10P image:
**Byte Order.**
Each cell is one byte.
-
-.. raw:: latex
-
-\small
-
-.. tabularcolumns:: |p{2.0cm}|p{1.0cm}|p{1.0cm}|p{1.0cm}|p{1.0cm}|p{10.0cm}|
-
.. flat-table::
:header-rows: 0
:stub-columns: 0
@@ -73,7 +66,3 @@ Each cell is one byte.
- R\ :sub:`33high`
- R\ :sub:`33low`\ (bits 7--6) G\ :sub:`32low`\ (bits 5--4)
R\ :sub:`31low`\ (bits 3--2) G\ :sub:`30low`\ (bits 1--0)
-
-.. raw:: latex
-
-\normalsize
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/7] media: open.rst: remove the minor number range
On Mon, Aug 28, 2017 at 09:53:57AM -0300, Mauro Carvalho Chehab wrote: > minor numbers use to range between 0 to 255, but that > was changed a long time ago. While it still applies when > CONFIG_VIDEO_FIXED_MINOR_RANGES, when the minor number is > dynamically allocated, this may not be true. In any case, > this is not relevant, as udev will take care of it. > > So, remove this useless misinformation. > > Acked-by: Hans Verkuil <hans.verk...@cisco.com> > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com> -- Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/7] media: open.rst: better document device node naming
Hi Mauro, On Mon, Aug 28, 2017 at 09:53:56AM -0300, Mauro Carvalho Chehab wrote: > Right now, only kAPI documentation describes the device naming. > However, such description is needed at the uAPI too. Add it, > and describe how to get an unique identify for a given device. > > Acked-by: Hans Verkuil <hans.verk...@cisco.com> > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> > --- > Documentation/media/uapi/v4l/open.rst | 39 > --- > 1 file changed, 36 insertions(+), 3 deletions(-) > > diff --git a/Documentation/media/uapi/v4l/open.rst > b/Documentation/media/uapi/v4l/open.rst > index afd116edb40d..fc0037091814 100644 > --- a/Documentation/media/uapi/v4l/open.rst > +++ b/Documentation/media/uapi/v4l/open.rst > @@ -7,12 +7,14 @@ Opening and Closing Devices > *** > > > -Device Naming > -= > +.. _v4l2_device_naming: > + > +V4L2 Device Node Naming > +=== > > V4L2 drivers are implemented as kernel modules, loaded manually by the > system administrator or automatically when a device is first discovered. > -The driver modules plug into the "videodev" kernel module. It provides > +The driver modules plug into the ``videodev`` kernel module. It provides > helper functions and a common application interface specified in this > document. > > @@ -23,6 +25,37 @@ option CONFIG_VIDEO_FIXED_MINOR_RANGES. In that case minor > numbers > are allocated in ranges depending on the device node type (video, radio, > etc.). > > +The existing V4L2 device node types are: > + > + > == > +Default device node name Usage > + > == > +``/dev/videoX`` Video input/output devices > +``/dev/vbiX`` Vertical blank data (i.e. closed captions, > teletext) > +``/dev/radioX`` Radio tuners and modulators > +``/dev/swradioX`` Software Defined Radio tuners and modulators > +``/dev/v4l-touchX`` Touch sensors Should we document V4L2 sub-device nodes here as well? They are implemented by the V4L2 core as well as the other device node types. Their purpose is somewhat different, though, and I think we'll need to make that explicit somehow. > + > == > + > +Where ``X`` is a non-negative number. > + > +.. note:: > + > + 1. The actual device node name is system-dependent, as udev rules may > apply. > + 2. There is no warranty that ``X`` will remain the same for the same s/warranty/guarantee/ > + device, as the number depends on the device driver's probe order. > + If you need an unique name, udev default rules produce > + ``/dev/v4l/by-id/`` and ``/dev/v4l/by-path/`` directoiries containing "directories" > + links that can be used uniquely to identify a V4L2 device node:: > + > + $ tree /dev/v4l > + /dev/v4l > + ├── by-id > + │ └── usb-OmniVision._USB_Camera-B4.04.27.1-video-index0 -> > ../../video0 > + └── by-path > + └── pci-:00:14.0-usb-0:2:1.0-video-index0 -> ../../video0 > + > + > Many drivers support "video_nr", "radio_nr" or "vbi_nr" module > options to select specific video/radio/vbi node numbers. This allows the > user to request that the device node is named e.g. /dev/video5 instead -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] media: open.rst: document devnode-centric and mc-centric types
Hi Mauro, Thanks for the update. A few more small comments below. On Fri, Aug 25, 2017 at 06:40:05AM -0300, Mauro Carvalho Chehab wrote: > From: "mche...@s-opensource.com" <mche...@s-opensource.com> > > When we added support for omap3, back in 2010, we added a new > type of V4L2 devices that aren't fully controlled via the V4L2 > device node. Yet, we never made it clear, at the V4L2 spec, > about the differences between both types. > > Let's document them with the current implementation. > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> > --- > Documentation/media/uapi/v4l/open.rst | 50 > +++ > 1 file changed, 50 insertions(+) > > diff --git a/Documentation/media/uapi/v4l/open.rst > b/Documentation/media/uapi/v4l/open.rst > index afd116edb40d..a72d142897c0 100644 > --- a/Documentation/media/uapi/v4l/open.rst > +++ b/Documentation/media/uapi/v4l/open.rst > @@ -6,6 +6,56 @@ > Opening and Closing Devices > *** > > +Types of V4L2 hardware control > +== > + > +V4L2 devices are usually complex: they are implemented via a main driver and > +often several additional drivers. The main driver always exposes one or > +more **V4L2 device** devnodes (see :ref:`v4l2_device_naming`). s/V4L2 device\*\* devnodes/V4L2 device nodes\*\*/ (As you also have a few paragraphs below.) > + > +The other drivers are called **V4L2 sub-devices** and provide control to > +other parts of the hardware usually connected via a serial bus (like > +I²C, SMBus or SPI). They can be implicitly controlled directly by the s/They/Depending on the main driver, they/ > +main driver or explicitly through via the **V4L2 sub-device API** interface. Through or via, but not both. " interface" is redundant. > + > +When V4L2 was originally designed, there was only one type of device control. > +The entire device was controlled via the **V4L2 device nodes**. We refer to > +this kind of control as **V4L2 device node centric** (or, simply, > +**vdev-centric**). s/vdev-centric/V4L2-centric/ ? > + > +Since the end of 2010, a new type of V4L2 device control was added in order > +to support complex devices that are common for embedded systems. Those > +devices are controlled mainly via the media controller and sub-devices. > +So, they're called: **Media controller centric** (or, simply, > +"**MC-centric**"). > + > +For **vdev-centric** control, the device and their corresponding hardware > +pipelines are controlled via the **V4L2 device** node. They may optionally I'd add highlighting to " node" as well. > +expose via the :ref:`media controller API `. > + > +For **MC-centric** control, before using the V4L2 device, it is required to > +set the hardware pipelines via the > +:ref:`media controller API `. For those devices, the > +sub-devices' configuration can be controlled via the > +:ref:`sub-device API `, with creates one device node per sub device. > + > +In summary, for **MC-centric** devices: > + > +- The **V4L2 device** node is responsible for controlling the streaming > + features; Same here. Or, just remove node. The other device nodes are just as much nodes as this one. > +- The **media controller device** is responsible to setup the pipelines; > +- The **V4L2 sub-devices** are responsible for sub-device > + specific settings. > + > +.. note:: > + > + A **vdev-centric** may optionally expose V4L2 sub-devices via > + :ref:`sub-device API `. In that case, it has to implement > + the :ref:`media controller API ` as well. Looks good! > + > + > + > +.. _v4l2_device_naming: > > Device Naming > = -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2] media: open.rst: document devnode-centric and mc-centric types
Hi Mauro, Thanks for the update! A few comments below. (Cc Hans and Laurent.) On Thu, Aug 24, 2017 at 09:07:35AM -0300, Mauro Carvalho Chehab wrote: > From: "mche...@s-opensource.com" <mche...@s-opensource.com> > > When we added support for omap3, back in 2010, we added a new > type of V4L2 devices that aren't fully controlled via the V4L2 > device node. Yet, we never made it clear, at the V4L2 spec, > about the differences between both types. > > Let's document them with the current implementation. > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> > Signed-off-by: Mauro Carvalho Chehab <mche...@osg.samsung.com> Pick one. :-) > --- > Documentation/media/uapi/v4l/open.rst | 47 > +++ > 1 file changed, 47 insertions(+) > > diff --git a/Documentation/media/uapi/v4l/open.rst > b/Documentation/media/uapi/v4l/open.rst > index afd116edb40d..cf522d9bb53c 100644 > --- a/Documentation/media/uapi/v4l/open.rst > +++ b/Documentation/media/uapi/v4l/open.rst > @@ -6,6 +6,53 @@ > Opening and Closing Devices > *** > > +Types of V4L2 device control > + > + > +V4L2 devices are usually complex: they're implemented via a main driver and > +often several additional drivers. The main driver always exposes one or > +more **V4L2 device** devnodes (see :ref:`v4l2_device_naming`). The other > +devices are called **V4L2 sub-devices**. They are usually controlled via a > +serial bus (I2C or SMBus). Reading this paragraph again, I think this is meant as an introduction to the reader. In this level of documentation (user space), it's fair to describe hardware. As the V4L2 sub-device concept in the kernel and the V4L2 sub-device API which is visible to the user space are not exactly the same things, I think I'd avoid using V4L2 sub-device concept when referring to hardware and how the concept is used in the kernel. How about, to replace the two last sentences: The other devices are typically I²C or SPI devices. Depending on the main driver, these devices are controlled either implicitly through the main driver or explicitly through the **V4L2 sub-device** interface. (I'm not sure the second sentence is even needed here.) > + > +When V4L2 started, there was only one type of device control. The entire > +device was controlled via the **V4L2 device nodes**. We refer to this > +kind of control as **V4L2 device-centric** (or, simply, **device-centric**). I'm still not quite happy with the naming. "Device" is too generic. How about "V4L2-centric"? SDR, radio and video nodes are part of V4L2, so I think that should convey the message as well as is factually correct. > + > +Since the end of 2010, a new type of V4L2 device control was added in order > +to support complex devices that are common on embedded systems. Those > +devices are controlled mainly via the media controller and sub-devices. > +So, they're called: **media controller centric** (or, simply, > +"**mc-centric**"). Looks good. I'd use capital "M" in Media controller. > + > +On **device-centric** control, the device and their corresponding hardware > +pipelines are controlled via the **V4L2 device** node. They may optionally > +expose the hardware pipelines via the > +:ref:`media controller API `. s/the hardware pipelines// It could be that there is a media device, but still no pipeline. Think of lens and flash devices, for instance. > + > +On a **mc-centric**, before using the V4L2 device, it is required to In English, abbreviations are capitalised, i.e. "MC-centric". > +set the hardware pipelines via the > +:ref:`media controller API `. On those devices, the > +sub-devices' configuration can be controlled via the > +:ref:`sub-device API `, with creates one device node per sub device. > + > +In summary, on **mc-centric** devices: > + > +- The **V4L2 device** node is mainly responsible for controlling the > + streaming features; > +- The **media controller device** is responsible to setup the pipelines > + and image settings (like size and format); "image settings (like size and format)" go to the sub-device bullet below. > +- The **V4L2 sub-devices** are responsible for sub-device > + specific settings. > + > +.. note:: > + > + It is forbidden for **device-centric** devices to expose V4L2 > + sub-devices via :ref:`sub-device API `, although this > + might change in the future. I believe we agree on the subject matter but we can still argue. :-D Could you drop ", although this might change in the future" part? We've established that there is no use case currently and we could well allow things in the API that were not allowed before withou
Re: [PATCH RFC] media: open.rst: document devnode-centric and mc-centric types
Hi Mauro, On Wed, Aug 23, 2017 at 10:42:28AM -0300, Mauro Carvalho Chehab wrote: > Em Wed, 23 Aug 2017 12:37:30 +0300 > Sakari Ailus <sakari.ai...@iki.fi> escreveu: > > > Hi Mauro, > > > > Thanks for the patch! Something like this was long due. > > > > I cc'd Hans and Laurent to get their attention, too. > > > > On Sat, Aug 19, 2017 at 07:04:40AM -0300, Mauro Carvalho Chehab wrote: > > > When we added support for omap3, back in 2010, we added a new > > > type of V4L2 devices that aren't fully controlled via the V4L2 > > > device node. Yet, we never made it clear, at the V4L2 spec, > > > about the differences between both types. > > > > > > Let's document them with the current implementation. > > > > > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> > > > --- > > > > > > This patch is a result of this week's discussion we had with regards to > > > merging > > > a patch series from Sakari documenting the need of propagating streaming > > > control between sub-devices on some complex mc-centric devices. > > > > > > One big gap on our documentation popped up: while we have distinct > > > behavior > > > for mc-centric and devnode-centric types of V4L2 devices, we never clearly > > > documented about that. > > > > > > This RFC patch is a first attempt to have a definition of those types, > > > and to > > > standardize the names we use to distinguish between those types. > > > > > > Comments are welcome. > > > > > > > > > Documentation/media/uapi/v4l/open.rst | 37 > > > +++ > > > 1 file changed, 37 insertions(+) > > > > > > diff --git a/Documentation/media/uapi/v4l/open.rst > > > b/Documentation/media/uapi/v4l/open.rst > > > index afd116edb40d..9cf4f74c466a 100644 > > > --- a/Documentation/media/uapi/v4l/open.rst > > > +++ b/Documentation/media/uapi/v4l/open.rst > > > @@ -6,6 +6,43 @@ > > > Opening and Closing Devices > > > *** > > > > > > +Types of V4L2 devices > > > += > > > + > > > +V4L2 devices are usually complex: they're implemented via a main driver > > > and > > > > Not all such as UVC webcams. How about: > > > > s/implemented/often implemented/ > > > > > +several other drivers. > > True. uvcvideo and gspca drivers has only a main driver. > > What about, instead: > > V4L2 devices are usually complex: they're implemented via a main driver > and often by several other drivers. How about s/other/additional/? That I think would better convey the suggestion the main driver's role stays even if there are other drivers. > > > The main driver always exposes a V4L2 device node This should actually say that there's "one or more V4L2 device nodes". > > > +(see :ref:`v4l2_device_naming`). The other devices are called > > > **sub-devices**. > > > +They are usually controlled via a serial bus (I2C or SMBus). > > > > The main driver also often exposes sub-devices. > > What about: > > (see :ref:`v4l2_device_naming`). The other devices, when present, > are called **sub-devices**. I might use "V4L2 sub-devices" instead of plain "sub-devices". I don't have strong opinion either way. > > > The practice has been that the video nodes are registered by the driver > > that generally orchestrates things for the complete device but I don't > > think there's anything that would exclude doing this otherwise if there are > > two struct devices involved that do DMA. > > Yeah. Well, I avoided the discussion if a mc-centric device with just > enable a single DMA engine for camera output should orchestrate things > for the complete device, as this is something that the current drivers > don't usually do (yet, IMHO, they should, at least for those cheap SoC > devices with just one camera connector). > > Anyway, this is a separate issue. For now, let's focus on documenting > the current situation. > > > > > > + > > > +When V4L2 started, there was only one type of device, fully controlled > > > via > > > +V4L2 device nodes. We call those devices as **devnode-centric devices**. > > > > > > > As device nodes can refer to any types of device nodes (such as sub-device > > nodes), how about calling these "video node centric"? To make it
Re: [PATCH RFC] media: open.rst: document devnode-centric and mc-centric types
Hi Mauro, Thanks for the patch! Something like this was long due. I cc'd Hans and Laurent to get their attention, too. On Sat, Aug 19, 2017 at 07:04:40AM -0300, Mauro Carvalho Chehab wrote: > When we added support for omap3, back in 2010, we added a new > type of V4L2 devices that aren't fully controlled via the V4L2 > device node. Yet, we never made it clear, at the V4L2 spec, > about the differences between both types. > > Let's document them with the current implementation. > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> > --- > > This patch is a result of this week's discussion we had with regards to > merging > a patch series from Sakari documenting the need of propagating streaming > control between sub-devices on some complex mc-centric devices. > > One big gap on our documentation popped up: while we have distinct behavior > for mc-centric and devnode-centric types of V4L2 devices, we never clearly > documented about that. > > This RFC patch is a first attempt to have a definition of those types, and to > standardize the names we use to distinguish between those types. > > Comments are welcome. > > > Documentation/media/uapi/v4l/open.rst | 37 > +++ > 1 file changed, 37 insertions(+) > > diff --git a/Documentation/media/uapi/v4l/open.rst > b/Documentation/media/uapi/v4l/open.rst > index afd116edb40d..9cf4f74c466a 100644 > --- a/Documentation/media/uapi/v4l/open.rst > +++ b/Documentation/media/uapi/v4l/open.rst > @@ -6,6 +6,43 @@ > Opening and Closing Devices > *** > > +Types of V4L2 devices > += > + > +V4L2 devices are usually complex: they're implemented via a main driver and Not all such as UVC webcams. How about: s/implemented/often implemented/ > +several other drivers. The main driver always exposes a V4L2 device node > +(see :ref:`v4l2_device_naming`). The other devices are called > **sub-devices**. > +They are usually controlled via a serial bus (I2C or SMBus). The main driver also often exposes sub-devices. The practice has been that the video nodes are registered by the driver that generally orchestrates things for the complete device but I don't think there's anything that would exclude doing this otherwise if there are two struct devices involved that do DMA. > + > +When V4L2 started, there was only one type of device, fully controlled via > +V4L2 device nodes. We call those devices as **devnode-centric devices**. As device nodes can refer to any types of device nodes (such as sub-device nodes), how about calling these "video node centric"? To make it more explicit, I'd use "V4L2 video node centric" here. Perhaps "video node centric" later to make it shorter. > +Since the end of 2010, a new type of V4L2 device was added, in order to > +support complex devices that are common on embedded systems. Those devices > +are controlled mainly via the media controller. So, they're called: s/:// > +**mc-centric devices**. "Media controller (MC) centric devices" and "MC-centric devices" later? > + > +On a **devnode-centric device**, the device and their corresponding hardware > +pipelines are controlled via the V4L2 device node. They may optionally > +expose the hardware pipelines via the > +:ref:`media controller API `. > + > +On a **mc-centric device**, before using the V4L2 device, it is required to > +set the hardware pipelines via the > +:ref:`media controller API `. On those devices, the > +sub-devices' configuration can be controlled via the > +:ref:`sub-device API `, with creates one device node per sub device. > +On such devices, the V4L2 device node is mainly responsible for controlling > +the streaming features, while the media controller and the subdevices device > +nodes are responsible for configuring the hardware. What would you think of using wording that conveys the message that in this case V4L2 video device nodes essentially are a data interface only whilst V4L2 sub-device nodes and MC are control interfaces? This is the grounding difference between the two in my opinion, and makes it easy to understand for the reader. > + > +.. note:: > + > + Currently, it is forbidden for a **devnode-centric device** to expose > + subdevices via :ref:`sub-device API `, although this might > + change in the future. I'd leave out this note. One of the purposes of MC was to find device nodes, and finding a subdev node related to, say, a video node is a pain without MC. Less variance in interface availability is better for the user space, and unlike between video node centric vs. MC-centric, there are hardly technical reasons (or at least I can't remember one) for doing this. I remember we
