Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement video control framework #82158

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions drivers/video/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

zephyr_library()

zephyr_library_sources(video_async.c)
zephyr_library_sources(video_common.c)
zephyr_library_sources(video_device.c)

Expand Down
118 changes: 118 additions & 0 deletions drivers/video/video_async.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
/*
* Copyright 2024 NXP
*
* SPDX-License-Identifier: Apache-2.0
*/

#include "video_async.h"
#include "video_device.h"

#include <zephyr/device.h>
#include <zephyr/logging/log.h>
#include <zephyr/sys/slist.h>

LOG_MODULE_REGISTER(video_async, CONFIG_VIDEO_LOG_LEVEL);

static sys_slist_t notifiers_list;
Copy link
Collaborator

@josuah josuah Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some of this can be replaced by this new device API.

Each of Zephyr struct device *dev is on a static array, and it is possible to get that array at runtime with z_device_get_all_static().

Combined with this new API that allows to check which API a device belongs to, it is already possible to loop through all video devices.

Would that allow to remove some/all looping mechanisms and list to maintain?


/* Find a registered notifier associated with this device */
struct video_notifier *video_async_find_notifier(const struct device *dev)
{
struct video_notifier *nf;

SYS_SLIST_FOR_EACH_CONTAINER(&notifiers_list, nf, node) {
if (nf->dev == dev) {
return nf;
}
}

return NULL;
}
Comment on lines +19 to +31
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be in favor of a struct video_device_data to avoid any notifiers_list:

struct video_device_data *vdata = dev->data;
struct video_notifier *nf = &vdata->notifier;
struct thisdriver_data *data = vdata->data;

Is there anywhere where this would not work?


/* Find the video device of the notifier tree that this notifier is part of */
static struct video_device *video_async_find_vdev(struct video_notifier *notifier)
{
if (!notifier) {
return NULL;
}

while (notifier->parent) {
notifier = notifier->parent;
}

return notifier->vdev;
}

void video_async_init(struct video_notifier *notifier, const struct device *dev,
struct video_device *vdev, const struct device **children_devs,
uint8_t children_num)
{
if (!notifier) {
return;
}

notifier->dev = dev;
notifier->vdev = vdev;
notifier->children_devs = (const struct device **)children_devs;
notifier->children_num = children_num;
}

int video_async_register(struct video_notifier *notifier)
{
uint8_t i = 0;
bool found_parent = false;
struct video_notifier *nf;
struct video_device *vdev = NULL;

if (!notifier) {
return -EINVAL;
}

if (sys_slist_find(&notifiers_list, &notifier->node, NULL)) {
LOG_ERR("Notifier of device %s is already registered", notifier->dev->name);
return -EALREADY;
}

SYS_SLIST_FOR_EACH_CONTAINER(&notifiers_list, nf, node) {
/* Find the parent notifier in the global notifiers registry */
if (!found_parent) {
for (i = 0; i < nf->children_num; ++i) {
if (nf->children_devs[i] == notifier->dev) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to reduce nested ifs :)

Suggested change
if (nf->children_devs[i] == notifier->dev) {
if (nf->children_devs[i] != notifier->dev) {
continue;
}

notifier->parent = nf;

vdev = video_async_find_vdev(notifier);
if (vdev) {
notifier->vdev = vdev;
}

found_parent = true;
break;
}
}
}

/* Find all children notifiers in the notifiers registry */
for (i = 0; i < notifier->children_num; ++i) {
if (notifier->children_devs[i] == nf->dev) {
nf->parent = notifier;
break;
}
}
}

/* Add the notifier to the notifiers registry */
sys_slist_append(&notifiers_list, &notifier->node);

LOG_DBG("Notifier of device %s is registered", notifier->dev->name);

return 0;
}

void video_async_unregister(struct video_notifier *notifier)
{
if (!notifier) {
return;
}

sys_slist_find_and_remove(&notifiers_list, &notifier->node);
}
34 changes: 34 additions & 0 deletions drivers/video/video_async.h
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this supposed to be a public header ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, there are no public header in this PR. They are all internal, for driver usage only.

Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Copyright 2024 NXP
*
* SPDX-License-Identifier: Apache-2.0
*/

#ifndef ZEPHYR_INCLUDE_VIDEO_ASYNC_H_
#define ZEPHYR_INCLUDE_VIDEO_ASYNC_H_

#include <zephyr/device.h>
#include <zephyr/sys/slist.h>

struct video_device;

struct video_notifier {
struct video_device *vdev;
const struct device *dev;
struct video_notifier *parent;
const struct device **children_devs;
uint8_t children_num;
sys_snode_t node;
};

void video_async_init(struct video_notifier *notifier, const struct device *dev,
struct video_device *vdev, const struct device **children_devs,
uint8_t children_num);

int video_async_register(struct video_notifier *notifier);

void video_async_unregister(struct video_notifier *notifier);

struct video_notifier *video_async_find_notifier(const struct device *dev);

#endif /* ZEPHYR_INCLUDE_VIDEO_ASYNC_H_ */