-
Notifications
You must be signed in to change notification settings - Fork 7.6k
video: drivers misc fixes #87366
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
video: drivers misc fixes #87366
Conversation
@josuah : The video emul imager exposes 3 controls : VIDEO_CID_EXPOSURE, VIDEO_CID_GAIN and VIDEO_CID_TEST_PATTERN. However, it does nothing specific to each control type, just a "fake" register write (?). I know this is just for testing purpose but even if this is just an emulation, we need to emul something "real". If not, it will be very difficult to rework the video control framework as each control type needs a specific implementation. I am going to remove these controls in the driver and just keep 1 control of type "vendor-specific" as an emulated read/write. Is it ok for you to do that ? |
I added that commit here: db5abeb Also available as a stand-alone commit on top of this Whichever is most convenient! Tested with: |
00869da
to
1b501e5
Compare
Oh, sorry ... I did it as per usual to always test the PR on the latest main (forgot to push the rebase) to see if there is a problem and pushed it unconsciously. Will pay attention. Sorry again ! |
drivers/video/ov5640.c
Outdated
{SDE_CTRL2_REG, abs(sin_coef) & 0xFF}}; | ||
|
||
ret = ov5640_modify_reg(&cfg->i2c, SDE_CTRL8_REG, 0x7F, sign); | ||
if (ret) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recently we had a discussion with @josuah in which we discussed about whether or not we should enforce MISRA style. Normally this is apparently a Zephyr requirement but is not enforced so hard to decide actually. In doubt I update by ST-MIPID02 / GC2145 PR few days ago for all the code I modified (I didn't modify the whole driver when the driver already existed) to if in such similar situation if (ret < 0). I tend to actually not being a big fan of it but that was more in preparation of this style being enforced.
I honestly don't really know if we should enforce it now or not. An opinion ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recently we had a discussion with @josuah in which we discussed about whether or not we should enforce MISRA style.
To me, it's the same. Without MISRA rule, it's a bit shorten. Personally, I am not fan of it as well, e.g. when it comes to if else
the style is a bit ugly to me:
But well, if it's enforced by Zephyr one day, we need to follow. Currently, to put it simple, I will change this in the next push ... personnaly, I tend to avoid any debate about things which are less technical :-).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pdgendt mentioned:
It's a required rule, but it's not enforced (yet) in CI
There is a doc page that tells the roadmap of for different level of coding style enforcement (including MISRA, and listing of areas) but I cannot find it again. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am ok with the PR, with only the point about MISRA or not ;) just have to be decided maybe. Also the PR will have to be rebased since now the endpoint removal as been merged as far as I know.
The MIPI CSI-2 Rx needs to be initialized after the camera sensor which is generally initialized with CONFIG_VIDEO_INIT_PRIORITY. This is currently true "by chance" due to the order the linker links the object files. This linker order is not easily controlled, so use an explicit priority value to ensure this requirement. Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
Fix some type range related to pixel rate which can cause overflow. Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
Fix coding style in a variable naming. Signed-off-by: Farah Fliss <farah.fliss@nxp.com> Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
The mt9m114 camera driver used to be single-instance. Improve it to multi-instance. Signed-off-by: Farah Fliss <farah.fliss@nxp.com> Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
Update the control value directly. No need for an internal variable. Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
@avolmat-st : To make sure I will push the branch correctly this time:
|
f4ffe98
to
c42c5f0
Compare
Simply rebased and fixed conflict because #87070 was merged |
SDE_CTRL8_REG's value must be modified using modify_register. Signed-off-by: Trung Hieu Le <trunghieu.le@nxp.com> Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
Fix the sign register for brightness control Signed-off-by: Trung Hieu Le <trunghieu.le@nxp.com> Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
Fix sign's register for constrast value. Signed-off-by: Trung Hieu Le <trunghieu.le@nxp.com> Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
Compute bits per pixel according to the pixel format instead of hardcoding it. Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
Drop video_stm32_dcmi_is_fmt_valid() as it is not needed. In this function, (i) checking against a format based on another utility function video_bits_per_pixel() is not robust, this check is done in the sensor driver, (ii) checking against the heap size is not appropriate because this should be done when allocating buffers, not in get/set format. Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
The format pitch (bytesperline) field is typically set by the bridge drivers, i.e. DMA, ISP drivers who actually handle the memory as they know exactly the memory layout constraints. Application just set the pixel format and resolution and must always read back this field to see what the driver actually sets (to allocate buffers for example). Also, drop format pitch setting in sensor drivers as this is not needed. Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
Do not set_format() when doing get_format(). This design seems initially to simplify the sample (just get_format() and everything works out of the box) but it makes thing incomprehensive and error prone. Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
Do not set_format() when doing get_format(). This design seems initially to simplify the sample (just get_format() and everything works out of the box) but it makes thing incomprehensive and error prone. Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>
c42c5f0
to
1159950
Compare
Forced push:
|
|
Sorry for the late reply. Yes thanks a lot !! |
@@ -67,7 +67,9 @@ static int emul_rx_set_fmt(const struct device *const dev, struct video_format * | |||
} | |||
|
|||
/* Cache the format selected locally to use it for getting the size of the buffer */ | |||
fmt->pitch = fmt->width * video_bits_per_pixel(fmt->pixelformat) / BITS_PER_BYTE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a small semantic change and I proposed a migration-guide entry here to avoid getting in the way of this PR:
This is not a breaking change, and a migration guide entry is only a hint for how to simplify applications after 4.2.
@mmahadevan108, @dleach02 PTL |
https://elixir.bootlin.com/linux/v6.11.1/source/drivers/media/platform/qcom/venus/vdec.c#L231
https://elixir.bootlin.com/linux/v6.11.1/source/drivers/media/platform/ti/omap3isp/ispvideo.c#L143
https://elixir.bootlin.com/linux/v6.11.1/source/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c#L404