Re: [RFC/PATCH 02/10] media: Media device
Hi Murali, On Sunday 18 July 2010 17:32:24 Muralidharan Karicheri wrote: > Hi Laurent, > > > +++ b/Documentation/media-framework.txt > > @@ -0,0 +1,68 @@ > > +Linux kernel media framework > > + > > + > > > > I felt more details needed in this media-framework.txt for information such > as which driver call this register() /unregister() function, details on link > management etc. I have not seen other patches yet. If it is discussed > elsewhere, please ignore this. I've split the documentation among the patches, adding sections that describe the code as new code was added. The final documentation is much more complete than this. > For the first part of the question, will the v4l2 core calls this for video > devices drivers? For other drivers such as audio, IR etc which are related > to the video devices, how this is handled. I think such details are required > in this documentation. The last patches describe the relationship between the V4L2 and media frameworks in Documentation/video4linux/v4l2-framework.txt. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 02/10] media: Media device
Hi Laurent,
> +++ b/Documentation/media-framework.txt
> @@ -0,0 +1,68 @@
> +Linux kernel media framework
> +
> +
I felt more details needed in this media-framework.txt for information
such as which driver call this
register() /unregister() function, details on link management etc. I
have not seen other patches yet.
If it is discussed elsewhere, please ignore this. For the first part
of the question, will the v4l2 core
calls this for video devices drivers? For other drivers such as audio,
IR etc which are related to
the video devices, how this is handled. I think such details are
required in this documentation.
> + /* If dev == NULL, then name must be filled in by the caller */
> + if (mdev->dev == NULL && WARN_ON(!mdev->name[0]))
> + return 0;
> +
> + /* Set name to driver name + device name if it is empty. */
> + if (!mdev->name[0])
> + snprintf(mdev->name, sizeof(mdev->name), "%s %s",
> + mdev->dev->driver->name, dev_name(mdev->dev));
> +
> + /* Register the device node. */
> + mdev->devnode.fops = &media_device_fops;
> + mdev->devnode.parent = mdev->dev;
> + strlcpy(mdev->devnode.name, mdev->name, sizeof(mdev->devnode.name));
> + mdev->devnode.release = media_device_release;
> + return media_devnode_register(&mdev->devnode, MEDIA_TYPE_DEVICE);
> +}
> +EXPORT_SYMBOL_GPL(media_device_register);
> +
> +/**
> + * media_device_unregister - unregister a media device
> + * @mdev: The media device
> + *
> + */
> +void media_device_unregister(struct media_device *mdev)
> +{
> + media_devnode_unregister(&mdev->devnode);
> +}
> +EXPORT_SYMBOL_GPL(media_device_unregister);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> new file mode 100644
> index 000..6c1fc4a
> --- /dev/null
> +++ b/include/media/media-device.h
> @@ -0,0 +1,53 @@
> +/*
> + * Media device support header.
> + *
> + * Copyright (C) 2010 Laurent Pinchart
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +#ifndef _MEDIA_DEVICE_H
> +#define _MEDIA_DEVICE_H
> +
> +#include
> +#include
> +
> +#include
> +
> +/* Each instance of a media device should create the media_device struct,
> + * either stand-alone or embedded in a larger struct.
> + *
> + * It allows easy access to sub-devices (see v4l2-subdev.h) and provides
> + * basic media device-level support.
> + */
> +
> +#define MEDIA_DEVICE_NAME_SIZE (20 + 16)
> +
> +struct media_device {
> + /* dev->driver_data points to this struct.
> + * Note: dev might be NULL if there is no parent device
> + * as is the case with e.g. ISA devices.
> + */
> + struct device *dev;
> + struct media_devnode devnode;
> +
> + /* unique device name, by default the driver name + bus ID */
> + char name[MEDIA_DEVICE_NAME_SIZE];
> +};
> +
> +int __must_check media_device_register(struct media_device *mdev);
> +void media_device_unregister(struct media_device *mdev);
> +
> +#endif
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Murali Karicheri
mkarich...@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC/PATCH 02/10] media: Media device
> -Original Message- > From: Laurent Pinchart [mailto:laurent.pinch...@ideasonboard.com] > Sent: Friday, July 16, 2010 3:53 AM > To: Aguirre, Sergio > Cc: linux-media@vger.kernel.org; sakari.ai...@maxwell.research.nokia.com > Subject: Re: [RFC/PATCH 02/10] media: Media device > > Hi Sergio, > > Thanks for the review. > > On Thursday 15 July 2010 16:16:44 Aguirre, Sergio wrote: > > > On Wednesday 14 July 2010 08:30:00 Laurent Pinchart wrote: > > > > > > diff --git a/include/media/media-device.h b/include/media/media- > device.h > > > new file mode 100644 > > > index 000..6c1fc4a > > > --- /dev/null > > > +++ b/include/media/media-device.h > > > @@ -0,0 +1,53 @@ > > [snip] > > > > +#ifndef _MEDIA_DEVICE_H > > > +#define _MEDIA_DEVICE_H > > > + > > > +#include > > > +#include > > > + > > > +#include > > > + > > > +/* Each instance of a media device should create the media_device > struct, > > > + * either stand-alone or embedded in a larger struct. > > > + * > > > + * It allows easy access to sub-devices (see v4l2-subdev.h) and > provides > > > + * basic media device-level support. > > > + */ > > > + > > > +#define MEDIA_DEVICE_NAME_SIZE (20 + 16) > > > > Where does above numbers come from ?? > > Copied from v4l2-device.h. It was originally BUS_ID_SIZE (the constant is > now > gone) + 16. > > I can replace that by a hardcoded 32. Ok. Regards, Sergio > > -- > Regards, > > Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC/PATCH 02/10] media: Media device
Hi Laurent,
> -Original Message-
> From: Laurent Pinchart [mailto:laurent.pinch...@ideasonboard.com]
> Sent: Friday, July 16, 2010 3:56 AM
> To: Aguirre, Sergio
> Cc: linux-media@vger.kernel.org; sakari.ai...@maxwell.research.nokia.com
> Subject: Re: [RFC/PATCH 02/10] media: Media device
>
> Hi Sergio,
>
> On Thursday 15 July 2010 16:22:06 Aguirre, Sergio wrote:
> > > On Wednesday 14 July 2010 08:30:00 Laurent Pinchart wrote:
>
>
>
> > > diff --git a/drivers/media/media-device.c b/drivers/media/media-
> device.c
> > > new file mode 100644
> > > index 000..a4d3db5
> > > --- /dev/null
> > > +++ b/drivers/media/media-device.c
> > > @@ -0,0 +1,77 @@
>
>
>
> > > +/**
> > > + * media_device_register - register a media device
> > > + * @mdev:The media device
> > > + *
> > > + * The caller is responsible for initializing the media device before
> > > + * registration. The following fields must be set:
> > > + *
> > > + * - dev should point to the parent device. The field can be NULL
> when no
> > > + * parent device is available (for instance with ISA devices).
> > > + * - name should be set to the device name. If the name is empty a
> parent
> > > + * device must be set. In that case the name will be set to the
> parent
> > > + * device driver name followed by a space and the parent device
> name.
> > > + */
> > > +int __must_check media_device_register(struct media_device *mdev)
> > > +{
> > > + /* If dev == NULL, then name must be filled in by the caller */
> > > + if (mdev->dev == NULL && WARN_ON(!mdev->name[0]))
> >
> > If mdev == NULL, you'll have a kernel panic here.
>
> That's why drivers must not call media_device_register with a NULL pointer
> :-)
> It's not a valid use case, unlike kfree(NULL) for instance.
Right. I know.
I guess I was thinking more in terms of not compromising the system because
of a buggy driver, and exit gracefully instead... But I guess it's also ok,
as a driver developer is usually updating the full kernel anyways.
Regards,
Sergio
>
> --
> Regards,
>
> Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 02/10] media: Media device
Hi Sergio,
On Thursday 15 July 2010 16:22:06 Aguirre, Sergio wrote:
> > On Wednesday 14 July 2010 08:30:00 Laurent Pinchart wrote:
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > new file mode 100644
> > index 000..a4d3db5
> > --- /dev/null
> > +++ b/drivers/media/media-device.c
> > @@ -0,0 +1,77 @@
> > +/**
> > + * media_device_register - register a media device
> > + * @mdev: The media device
> > + *
> > + * The caller is responsible for initializing the media device before
> > + * registration. The following fields must be set:
> > + *
> > + * - dev should point to the parent device. The field can be NULL when no
> > + * parent device is available (for instance with ISA devices).
> > + * - name should be set to the device name. If the name is empty a parent
> > + * device must be set. In that case the name will be set to the parent
> > + * device driver name followed by a space and the parent device name.
> > + */
> > +int __must_check media_device_register(struct media_device *mdev)
> > +{
> > + /* If dev == NULL, then name must be filled in by the caller */
> > + if (mdev->dev == NULL && WARN_ON(!mdev->name[0]))
>
> If mdev == NULL, you'll have a kernel panic here.
That's why drivers must not call media_device_register with a NULL pointer :-)
It's not a valid use case, unlike kfree(NULL) for instance.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 02/10] media: Media device
Hi Sergio, Thanks for the review. On Thursday 15 July 2010 16:16:44 Aguirre, Sergio wrote: > > On Wednesday 14 July 2010 08:30:00 Laurent Pinchart wrote: > > diff --git a/include/media/media-device.h b/include/media/media-device.h > > new file mode 100644 > > index 000..6c1fc4a > > --- /dev/null > > +++ b/include/media/media-device.h > > @@ -0,0 +1,53 @@ [snip] > > +#ifndef _MEDIA_DEVICE_H > > +#define _MEDIA_DEVICE_H > > + > > +#include > > +#include > > + > > +#include > > + > > +/* Each instance of a media device should create the media_device struct, > > + * either stand-alone or embedded in a larger struct. > > + * > > + * It allows easy access to sub-devices (see v4l2-subdev.h) and provides > > + * basic media device-level support. > > + */ > > + > > +#define MEDIA_DEVICE_NAME_SIZE (20 + 16) > > Where does above numbers come from ?? Copied from v4l2-device.h. It was originally BUS_ID_SIZE (the constant is now gone) + 16. I can replace that by a hardcoded 32. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC/PATCH 02/10] media: Media device
Hi Laurent,
Other comment I missed to mention...
> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Laurent Pinchart
> Sent: Wednesday, July 14, 2010 8:30 AM
> To: linux-media@vger.kernel.org
> Cc: sakari.ai...@maxwell.research.nokia.com
> Subject: [RFC/PATCH 02/10] media: Media device
>
> The media_device structure abstracts functions common to all kind of
> media devices (v4l2, dvb, alsa, ...). It manages media entities and
> offers a userspace API to discover and configure the media device
> internal topology.
>
> Signed-off-by: Laurent Pinchart
> ---
> Documentation/media-framework.txt | 68
> drivers/media/Makefile|2 +-
> drivers/media/media-device.c | 77
> +
> include/media/media-device.h | 53 +
> 4 files changed, 199 insertions(+), 1 deletions(-)
> create mode 100644 Documentation/media-framework.txt
> create mode 100644 drivers/media/media-device.c
> create mode 100644 include/media/media-device.h
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> new file mode 100644
> index 000..a4d3db5
> --- /dev/null
> +++ b/drivers/media/media-device.c
> @@ -0,0 +1,77 @@
> +/*
> + * Media device support.
> + *
> + * Copyright (C) 2010 Laurent Pinchart
>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> USA
> + */
> +
> +#include
> +#include
> +
> +#include
> +#include
> +
> +static const struct media_file_operations media_device_fops = {
> + .owner = THIS_MODULE,
> +};
> +
> +static void media_device_release(struct media_devnode *mdev)
> +{
> +}
> +
> +/**
> + * media_device_register - register a media device
> + * @mdev:The media device
> + *
> + * The caller is responsible for initializing the media device before
> + * registration. The following fields must be set:
> + *
> + * - dev should point to the parent device. The field can be NULL when no
> + * parent device is available (for instance with ISA devices).
> + * - name should be set to the device name. If the name is empty a parent
> + * device must be set. In that case the name will be set to the parent
> + * device driver name followed by a space and the parent device name.
> + */
> +int __must_check media_device_register(struct media_device *mdev)
> +{
> + /* If dev == NULL, then name must be filled in by the caller */
> + if (mdev->dev == NULL && WARN_ON(!mdev->name[0]))
If mdev == NULL, you'll have a kernel panic here.
Regards,
Sergio
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC/PATCH 02/10] media: Media device
Hi Laurent,
Very minor comment below.
> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Laurent Pinchart
> Sent: Wednesday, July 14, 2010 8:30 AM
> To: linux-media@vger.kernel.org
> Cc: sakari.ai...@maxwell.research.nokia.com
> Subject: [RFC/PATCH 02/10] media: Media device
>
> The media_device structure abstracts functions common to all kind of
> media devices (v4l2, dvb, alsa, ...). It manages media entities and
> offers a userspace API to discover and configure the media device
> internal topology.
>
> Signed-off-by: Laurent Pinchart
> ---
> Documentation/media-framework.txt | 68
> drivers/media/Makefile|2 +-
> drivers/media/media-device.c | 77
> +
> include/media/media-device.h | 53 +
> 4 files changed, 199 insertions(+), 1 deletions(-)
> create mode 100644 Documentation/media-framework.txt
> create mode 100644 drivers/media/media-device.c
> create mode 100644 include/media/media-device.h
>
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> new file mode 100644
> index 000..6c1fc4a
> --- /dev/null
> +++ b/include/media/media-device.h
> @@ -0,0 +1,53 @@
> +/*
> + * Media device support header.
> + *
> + * Copyright (C) 2010 Laurent Pinchart
>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> USA
> + */
> +
> +#ifndef _MEDIA_DEVICE_H
> +#define _MEDIA_DEVICE_H
> +
> +#include
> +#include
> +
> +#include
> +
> +/* Each instance of a media device should create the media_device struct,
> + * either stand-alone or embedded in a larger struct.
> + *
> + * It allows easy access to sub-devices (see v4l2-subdev.h) and provides
> + * basic media device-level support.
> + */
> +
> +#define MEDIA_DEVICE_NAME_SIZE (20 + 16)
Where does above numbers come from ??
Regards,
Sergio
> +
> +struct media_device {
> + /* dev->driver_data points to this struct.
> + * Note: dev might be NULL if there is no parent device
> + * as is the case with e.g. ISA devices.
> + */
> + struct device *dev;
> + struct media_devnode devnode;
> +
> + /* unique device name, by default the driver name + bus ID */
> + char name[MEDIA_DEVICE_NAME_SIZE];
> +};
> +
> +int __must_check media_device_register(struct media_device *mdev);
> +void media_device_unregister(struct media_device *mdev);
> +
> +#endif
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
