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.

Sign up for GitHub

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
+927 −166
Open

Implement video control framework #82158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6fc1d3e
drivers: video: Add endpoint DT helpers
ngphibang Oct 30, 2024
8b3c3c5
drivers: video: mipi_csi2rx: Use endpoint DT helpers
ngphibang Oct 30, 2024
b98753e
drivers: video: csi: Use endpoint DT helpers
ngphibang Oct 30, 2024
4080680
drivers: video: Introduce video device and its surroundings
ngphibang Nov 25, 2024
0fe00c7
drivers: video: Introduce video async mechanism
ngphibang Nov 26, 2024
a7ea463
include: video-controls: Add video_control struct
ngphibang Nov 26, 2024
81be3e1
drivers: video: Implement video control framework
ngphibang Nov 26, 2024
356e98b
drivers: video: ov5640: Fix brightness control
ngphibang Nov 27, 2024
1583d64
drivers: video: csi: Move instance-independant stuffs to common init
ngphibang Nov 27, 2024
62695e3
drivers: video: Use the new video control framework
ngphibang Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions drivers/video/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@

zephyr_library()

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

zephyr_library_sources_ifdef(CONFIG_VIDEO_MCUX_CSI video_mcux_csi.c)
zephyr_library_sources_ifdef(CONFIG_VIDEO_MCUX_MIPI_CSI2RX video_mcux_mipi_csi2rx.c)
Expand Down
160 changes: 112 additions & 48 deletions drivers/video/ov5640.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
#include <zephyr/drivers/video.h>
#include <zephyr/drivers/video-controls.h>

#include "video_async.h"
#include "video_ctrls.h"

LOG_MODULE_REGISTER(video_ov5640, CONFIG_VIDEO_LOG_LEVEL);

#define CHIP_ID_REG 0x300a
Expand Down Expand Up @@ -137,7 +140,22 @@ struct ov5640_mode_config {
uint16_t def_frmrate;
};

struct ov5640_ctrls {
struct video_ctrl_handler handler;
struct video_ctrl brightness;
struct video_ctrl contrast;
struct video_ctrl hue;
struct video_ctrl saturation;
struct video_ctrl hflip;
struct video_ctrl vflip;
struct video_ctrl light_freq;
struct video_ctrl test_pattern;
struct video_ctrl pixel_rate;
};

struct ov5640_data {
struct video_notifier notifier;
struct ov5640_ctrls ctrls;
struct video_format fmt;
uint64_t cur_pixrate;
uint16_t cur_frmrate;
Expand Down Expand Up @@ -543,6 +561,9 @@ static int ov5640_set_frmival(const struct device *dev, enum video_endpoint_id e
drv_data->cur_frmrate = best_match;
drv_data->cur_pixrate = drv_data->cur_mode->mipi_frmrate_config[ind].pixelrate;

/* Update pixerate control */
drv_data->ctrls.pixel_rate.val = drv_data->cur_pixrate;

frmival->numerator = 1;
frmival->denominator = best_match;

Expand Down Expand Up @@ -677,10 +698,6 @@ static int ov5640_set_ctrl_test_pattern(const struct device *dev, int value)
{
const struct ov5640_config *cfg = dev->config;

if (!IN_RANGE(value, 0, ARRAY_SIZE(test_pattern_val) - 1)) {
return -EINVAL;
}

return ov5640_write_reg(&cfg->i2c, PRE_ISP_TEST_SET1, test_pattern_val[value]);
}

Expand All @@ -689,10 +706,6 @@ static int ov5640_set_ctrl_hue(const struct device *dev, int value)
const struct ov5640_config *cfg = dev->config;
int cos_coef, sin_coef, sign = 0;

if (!IN_RANGE(value, 0, 360)) {
return -EINVAL;
}

double rad_val = value;
int ret = ov5640_modify_reg(&cfg->i2c, SDE_CTRL0_REG, BIT(0), BIT(0));

Expand Down Expand Up @@ -725,10 +738,6 @@ static int ov5640_set_ctrl_saturation(const struct device *dev, int value)
{
const struct ov5640_config *cfg = dev->config;

if (!IN_RANGE(value, 0, UINT8_MAX)) {
return -EINVAL;
}

struct ov5640_reg saturation_params[] = {{SDE_CTRL3_REG, value}, {SDE_CTRL4_REG, value}};
int ret = ov5640_modify_reg(&cfg->i2c, SDE_CTRL8_REG, BIT(6) | BIT(0), BIT(6) | BIT(0));

Expand All @@ -743,12 +752,8 @@ static int ov5640_set_ctrl_brightness(const struct device *dev, int value)
{
const struct ov5640_config *cfg = dev->config;

if (!IN_RANGE(value, -UINT8_MAX, UINT8_MAX)) {
return -EINVAL;
}

struct ov5640_reg brightness_params[] = {{SDE_CTRL8_REG, value >= 0 ? 0x01 : 0x09},
{SDE_CTRL7_REG, abs(value) & 0xff}};
{SDE_CTRL7_REG, (abs(value) << 4) & 0xf0}};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{SDE_CTRL7_REG, (abs(value) << 4) & 0xf0}};
{SDE_CTRL7_REG, (abs(value) << BIT(2)) & GENMASK(15,8)}};

Copy link
Contributor Author

@ngphibang ngphibang Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I believe you mean (abs(value) << BIT(4)) & GENMASK(7,4).

Could you explain the benefit of using these macros here ?

IMO, GENMASK() will help if we need to generate something dynamically that is only known at runtime like GENMASK(x, y) or in case it is difficult to do the calculation, e.g. GENMASK(63, 51). Here, the mask is very clear and simple, we just put 0xf0 why have to pass by this macro ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you mean (abs(value) << BIT(4)) & GENMASK(7,4).

yep i meant GENMAST(7,4). Would it not be more readable, to replace (abs(value) << BIT(4)) & GENMASK(7,4) with some #define? Yes you are right 0xf0 is pretty straightforward,

int ret = ov5640_modify_reg(&cfg->i2c, SDE_CTRL0_REG, BIT(2), BIT(2));

if (ret) {
Expand All @@ -762,10 +767,6 @@ static int ov5640_set_ctrl_contrast(const struct device *dev, int value)
{
const struct ov5640_config *cfg = dev->config;

if (!IN_RANGE(value, 0, UINT8_MAX)) {
return -EINVAL;
}

int ret = ov5640_modify_reg(&cfg->i2c, SDE_CTRL0_REG, BIT(2), BIT(2));

if (ret) {
Expand Down Expand Up @@ -841,41 +842,27 @@ static int ov5640_set_ctrl_power_line_freq(const struct device *dev, int value)
return ov5640_modify_reg(&cfg->i2c, HZ5060_CTRL01_REG, BIT(7), BIT(7));
}

static int ov5640_set_ctrl(const struct device *dev, unsigned int cid, void *value)
static int ov5640_set_ctrl(const struct device *dev, struct video_control *ctrl)
{
switch (cid) {
switch (ctrl->id) {
case VIDEO_CID_TEST_PATTERN:
return ov5640_set_ctrl_test_pattern(dev, (int)value);
return ov5640_set_ctrl_test_pattern(dev, ctrl->val);
case VIDEO_CID_HUE:
return ov5640_set_ctrl_hue(dev, (int)value);
return ov5640_set_ctrl_hue(dev, ctrl->val);
case VIDEO_CID_SATURATION:
return ov5640_set_ctrl_saturation(dev, (int)(value));
return ov5640_set_ctrl_saturation(dev, ctrl->val);
case VIDEO_CID_BRIGHTNESS:
return ov5640_set_ctrl_brightness(dev, (int)(value));
return ov5640_set_ctrl_brightness(dev, ctrl->val);
case VIDEO_CID_CONTRAST:
return ov5640_set_ctrl_contrast(dev, (int)value);
return ov5640_set_ctrl_contrast(dev, ctrl->val);
case VIDEO_CID_GAIN:
return ov5640_set_ctrl_gain(dev, (int)(value));
return ov5640_set_ctrl_gain(dev, ctrl->val);
case VIDEO_CID_HFLIP:
return ov5640_set_ctrl_hflip(dev, (int)(value));
return ov5640_set_ctrl_hflip(dev, ctrl->val);
case VIDEO_CID_VFLIP:
return ov5640_set_ctrl_vflip(dev, (int)(value));
return ov5640_set_ctrl_vflip(dev, ctrl->val);
case VIDEO_CID_POWER_LINE_FREQUENCY:
return ov5640_set_ctrl_power_line_freq(dev, (int)(value));
default:
return -ENOTSUP;
}
}

static inline int ov5640_get_ctrl(const struct device *dev, unsigned int cid, void *value)
{
struct ov5640_data *drv_data = dev->data;

switch (cid) {
case VIDEO_CID_PIXEL_RATE:
*((uint64_t *)value) = drv_data->cur_pixrate;

return 0;
return ov5640_set_ctrl_power_line_freq(dev, ctrl->val);
default:
return -ENOTSUP;
}
Expand Down Expand Up @@ -916,19 +903,88 @@ static int ov5640_enum_frmival(const struct device *dev, enum video_endpoint_id
return 0;
}

static int ov5640_init_controls(const struct device *dev)
{
int ret;
struct ov5640_data *drv_data = dev->data;
struct ov5640_ctrls *ctrls = &drv_data->ctrls;

video_ctrl_handler_init(&ctrls->handler, dev);

ret = video_ctrl_init(&ctrls->brightness, &ctrls->handler, VIDEO_CID_BRIGHTNESS, -15, 15, 1,
0);
if (ret) {
return ret;
}

ret = video_ctrl_init(&ctrls->contrast, &ctrls->handler, VIDEO_CID_CONTRAST, 0, 255, 1, 0);
if (ret) {
return ret;
}

ret = video_ctrl_init(&ctrls->hue, &ctrls->handler, VIDEO_CID_HUE, 0, 359, 1, 0);
if (ret) {
return ret;
}

ret = video_ctrl_init(&ctrls->saturation, &ctrls->handler, VIDEO_CID_SATURATION, 0, 255, 1,
64);
if (ret) {
return ret;
}

ret = video_ctrl_init(&ctrls->hflip, &ctrls->handler, VIDEO_CID_HFLIP, 0, 1, 1, 0);
if (ret) {
return ret;
}

ret = video_ctrl_init(&ctrls->vflip, &ctrls->handler, VIDEO_CID_VFLIP, 0, 1, 1, 0);
if (ret) {
return ret;
}

ret = video_ctrl_init(&ctrls->light_freq, &ctrls->handler, VIDEO_CID_POWER_LINE_FREQUENCY,
VIDEO_CID_POWER_LINE_FREQUENCY_DISABLED,
VIDEO_CID_POWER_LINE_FREQUENCY_AUTO, 1,
VIDEO_CID_POWER_LINE_FREQUENCY_50HZ);
if (ret) {
return ret;
}

ret = video_ctrl_init(&ctrls->test_pattern, &ctrls->handler, VIDEO_CID_TEST_PATTERN, 0,
ARRAY_SIZE(test_pattern_val) - 1, 1, 0);
if (ret) {
return ret;
}

return video_ctrl_init(
&ctrls->pixel_rate, &ctrls->handler, VIDEO_CID_PIXEL_RATE,
mipi_vga_frmrate_params[0].pixelrate,
mipi_hd_frmrate_params[ARRAY_SIZE(mipi_hd_frmrate_params) - 1].pixelrate, 1,
drv_data->cur_pixrate);
josuah marked this conversation as resolved.
Show resolved Hide resolved
}

static const struct video_driver_api ov5640_driver_api = {
.set_format = ov5640_set_fmt,
.get_format = ov5640_get_fmt,
.get_caps = ov5640_get_caps,
.stream_start = ov5640_stream_start,
.stream_stop = ov5640_stream_stop,
.set_ctrl = ov5640_set_ctrl,
.get_ctrl = ov5640_get_ctrl,
.set_frmival = ov5640_set_frmival,
.get_frmival = ov5640_get_frmival,
.enum_frmival = ov5640_enum_frmival,
};

static int ov5640_async_register(const struct device *dev)
{
struct ov5640_data *drv_data = dev->data;

video_async_init(&drv_data->notifier, dev, NULL, &drv_data->ctrls.handler, NULL, 0);

return video_async_register(&drv_data->notifier);
}

static int ov5640_init(const struct device *dev)
{
const struct ov5640_config *cfg = dev->config;
Expand Down Expand Up @@ -1026,7 +1082,15 @@ static int ov5640_init(const struct device *dev)
return -EIO;
}

return 0;
/* Initialize controls */
ret = ov5640_init_controls(dev);
if (ret) {
LOG_ERR("Unable to init controls");
return ret;
}

/* Register async */
return ov5640_async_register(dev);
}

#define OV5640_INIT(n) \
Expand Down
Loading
Loading