On 04/10/2018 07:35 AM, Tomasz Figa wrote:
> Hi Hans,
>
> On Mon, Apr 9, 2018 at 11:20 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> [snip]
>> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
>> new file mode 100644
>> index 000000000000..ead78613fdbe
>> --- /dev/null
>> +++ b/drivers/media/media-request.c
>> @@ -0,0 +1,23 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Media device request objects
>> + *
>> + * Copyright (C) 2018 Intel Corporation
>> + * Copyright (C) 2018, The Chromium OS Authors. All rights reserved.
>
> I'm not sure about the origin of this line, but it's not a correct
> copyright for kernel code produced as a part of Chrome OS project. It would
> normally be something like
>
> Copyright (C) 2018 Google, Inc.
Fixed.
>
>> + *
>> + * Author: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>> + */
>> +
>> +#include <linux/anon_inodes.h>
>> +#include <linux/file.h>
>> +#include <linux/mm.h>
>> +#include <linux/string.h>
>> +
>> +#include <media/media-device.h>
>> +#include <media/media-request.h>
>> +
>> +int media_request_alloc(struct media_device *mdev,
>> + struct media_request_alloc *alloc)
>> +{
>> + return -ENOMEM;
>> +}
>> diff --git a/include/media/media-device.h b/include/media/media-device.h
>> index bcc6ec434f1f..07e323c57202 100644
>> --- a/include/media/media-device.h
>> +++ b/include/media/media-device.h
>> @@ -19,6 +19,7 @@
>> #ifndef _MEDIA_DEVICE_H
>> #define _MEDIA_DEVICE_H
>
>> +#include <linux/anon_inodes.h>
>
> What is the need for anon_inodes in this header?
>
>> #include <linux/list.h>
>> #include <linux/mutex.h>
>
>> @@ -27,6 +28,7 @@
>
>> struct ida;
>> struct device;
>> +struct media_device;
>
>> /**
>> * struct media_entity_notify - Media Entity Notify
>> @@ -50,10 +52,16 @@ struct media_entity_notify {
>> * struct media_device_ops - Media device operations
>> * @link_notify: Link state change notification callback. This callback
> is
>> * called with the graph_mutex held.
>> + * @req_alloc: Allocate a request
>> + * @req_free: Free a request
>> + * @req_queue: Queue a request
>> */
>> struct media_device_ops {
>> int (*link_notify)(struct media_link *link, u32 flags,
>> unsigned int notification);
>> + struct media_request *(*req_alloc)(struct media_device *mdev);
>> + void (*req_free)(struct media_request *req);
>> + int (*req_queue)(struct media_request *req);
>> };
>
>> /**
>> @@ -88,6 +96,8 @@ struct media_device_ops {
>> * @disable_source: Disable Source Handler function pointer
>> *
>> * @ops: Operation handler callbacks
>> + * @req_lock: Serialise access to requests
>> + * @req_queue_mutex: Serialise validating and queueing requests
>
> Let's bikeshed a bit! "access" sounds like a superset of "validating and
> queuing" to me. Perhaps it could make sense to be a bit more specific on
> what type of access the spinlock is used for?
req_lock is unused and is now deleted. It's a left-over from older code.
Regards,
Hans
>
> Best regards,
> Tomasz
>
