Hi Laurent, On Wed, Feb 07, 2024 at 04:43:04PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Wed, Dec 20, 2023 at 12:37:08PM +0200, Sakari Ailus wrote: > > Document that after unregistering, Media device memory resources are > > released by the release() callback rather than by calling > > media_device_cleanup(). > > > > Also add that driver memory resources should be bound to the Media device, > > not V4L2 device. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > > --- > > Documentation/driver-api/media/mc-core.rst | 18 ++++++++++++++++-- > > include/media/media-device.h | 6 ++++-- > > 2 files changed, 20 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > > index 2456950ce8ff..346f67760671 100644 > > --- a/Documentation/driver-api/media/mc-core.rst > > +++ b/Documentation/driver-api/media/mc-core.rst > > @@ -46,13 +46,27 @@ Drivers initialise media device instances by calling > > :c:func:`media_device_init()`. After initialising a media device instance, it is > > registered by calling :c:func:`__media_device_register()` via the macro > > ``media_device_register()`` and unregistered by calling > > -:c:func:`media_device_unregister()`. An initialised media device must be > > -eventually cleaned up by calling :c:func:`media_device_cleanup()`. > > +:c:func:`media_device_unregister()`. The resources of an unregistered media > > "of an unregistered media device" sounds weird here, I interpret it as > applying only to media devices that have never been registered. How about "newly unregistered"? > > > +device will be released by the ``release()`` callback of :c:type:`media_device` > > +ops, which will be called when the last user of the media device has released it > > +calling :c:func:`media_device_put()`. > > + > > +The ``release()`` callback is the way all the resources of the media device are > > +released once :c:func:`media_device_init()` has been called. This is also > > +relevant during device driver's probe function as the ``release()`` callback > > +will also have to be able to safely release the resources related to a partially > > +initialised media device. > > > > Note that it is not allowed to unregister a media device instance that was not > > previously registered, or clean up a media device instance that was not > > previously initialised. > > Does this need an update, as we don't cleanup explicitly instead ? I think this should remain unchanged, at least as long as there are drivers using the old API. > > > > > +Media device and driver's per-device context > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > + > > +Drivers should use the struct media_device_ops ``release()`` callback to release > > +their own resources and not e.g. that of the struct v4l2_device. > > + > > Entities > > ^^^^^^^^ > > > > diff --git a/include/media/media-device.h b/include/media/media-device.h > > index c6816be0eee8..98e1892f1b51 100644 > > --- a/include/media/media-device.h > > +++ b/include/media/media-device.h > > @@ -250,8 +250,10 @@ void media_device_init(struct media_device *mdev); > > * > > * @mdev: pointer to struct &media_device > > * > > - * This function that will destroy the graph_mutex that is > > - * initialized in media_device_init(). > > + * This function that will destroy the graph_mutex that is initialized in > > While at it, s/that will/will/ Yes. > > > + * media_device_init(). Note that *only* drivers that do not manage releasing > > + * the memory of th media device itself call this function. This function is > > s/of th/of the/ > > "that do not manage releasing the memory of the media device itself" is > hard to understand for someone who hasn't paid close attention to the > development of this series. This text needs improvements. Hmm. How about: Note that only drivers that do not have a proper release callback of the struct media_device call this function. > > > + * thus effectively DEPRECATED. > > */ > > void media_device_cleanup(struct media_device *mdev); > > > -- Regards, Sakari Ailus
