Hi Hans,
Thanks for the review!
On 09/05/17 09:49, Hans Verkuil wrote:
> On 09/03/2017 07:49 PM, Sakari Ailus wrote:
>> Registering a notifier has required the knowledge of struct v4l2_device
>> for the reason that sub-devices generally are registered to the
>> v4l2_device (as well as the media device, also available through
>> v4l2_device).
>>
>> This information is not available for sub-device drivers at probe time.
>>
>> What this patch does is that it allows registering notifiers without
>> having v4l2_device around. Instead the sub-device pointer is stored to the
>> notifier. Once the sub-device of the driver that registered the notifier
>> is registered, the notifier will gain the knowledge of the v4l2_device,
>> and the binding of async sub-devices from the sub-device driver's notifier
>> may proceed.
>>
>> The master notifier's complete callback is only called when all sub-device
>> notifiers are completed.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>> ---
>> drivers/media/v4l2-core/v4l2-async.c | 153 +++++++++++++++++++++++++++++------
>> include/media/v4l2-async.h | 19 ++++-
>> 2 files changed, 146 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>> index 70d02378b48f..55d7886103d2 100644
>> --- a/drivers/media/v4l2-core/v4l2-async.c
>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>> @@ -25,6 +25,10 @@
>> #include <media/v4l2-fwnode.h>
>> #include <media/v4l2-subdev.h>
>>
>> +static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
>> + struct v4l2_subdev *sd,
>> + struct v4l2_async_subdev *asd);
>> +
>> static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
>> {
>> #if IS_ENABLED(CONFIG_I2C)
>> @@ -101,14 +105,69 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *
>> return NULL;
>> }
>>
>> +static bool v4l2_async_subdev_notifiers_complete(
>> + struct v4l2_async_notifier *notifier)
>> +{
>> + struct v4l2_async_notifier *n;
>> +
>> + list_for_each_entry(n, ¬ifier->notifiers, notifiers) {
>> + if (!n->master)
>> + return false;
>> + }
>> +
>> + return true;
>> +}
>> +
>> +#define notifier_v4l2_dev(n) \
>> + (!!(n)->v4l2_dev ? (n)->v4l2_dev : \
>> + !!(n)->master ? (n)->master->v4l2_dev : NULL)
>> +
>> +static struct v4l2_async_notifier *v4l2_async_get_subdev_notifier(
>> + struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
>
> Why pass the notifier argument when it is not actually used in the function?
>
> Is this function needed at all? As far as I can see the sd always belongs to
> the given notifier, otherwise the v4l2_async_belongs() call would fail.
> And v4l2_async_belongs() is always called before v4l2_async_test_notify().
The function gets a notifier which the sub-device may have registered,
it's not the same notifier that was used in registering the sub-device
itself.
I'll remove the other argument as well.
>
> This could all do with some more code comments. I'm having a difficult time
> understanding it all.
Yes, I'm adding comments to v8.
--
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
