Skip to content

Conversation

@mjs513
Copy link

@mjs513 mjs513 commented Sep 20, 2025

This PR is a clean PR of video: ov7675: add Omnivision OV7675 sensor support and basic controls PR per your request.


/* OV7670 definitions */
#define OV7670_PROD_ID 0x76
#define OV7670_PROD_ID 0x76

Choose a reason for hiding this comment

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

New spaces added here, please avoid unnecessary modifications.

Copy link
Author

Choose a reason for hiding this comment

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

Weird - ran it through clang and check patch as well.

Copy link
Author

Choose a reason for hiding this comment

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

hopefully fixed - checkpatch did not pick up errors

Choose a reason for hiding this comment

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

There are still new spaces added before 0x76

@avolmat-st
Copy link

@mjs513

Please do not add a commit to fix review comments. You have to update the existing commits in order to fix the issue related to what is in each commit.
Moreover, the code you are providing does not compile and add new structures that are not used within the driver.

Please fix that before requesting again a new review.

@mjs513
Copy link
Author

mjs513 commented Sep 22, 2025

@avolmat-st

Thought you would want it as new commit - going to be a pain to do it in the separate commits. In addition other than the 0x20 comment they seemed to all apply to the combined version

As for not compiling for you - not sure why. I tested with both the OV7675 and OV7670 using zephyr directly and via the arduino ide. Both versions compiled without an issue. But will double check anyway

@avolmat-st
Copy link

Thought you would want it as new commit - going to be a pain to do it in the separate commits. In addition other than the 0x20 comment they seemed to all apply to the combined version

Only commits which have been reviewed and are corrects will be merged into the tree. We do not merge series of commits fixing each others within the same PR.

Cf https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html

Especially this section:https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#pr-requirements, point 3 (Squash Intermediary or Non-Final Development History) and 4 (Ensure Clean History Before Submission)

@mjs513 mjs513 force-pushed the Add-New-OV7675-Driver branch 6 times, most recently from 6874e05 to a1a7b03 Compare September 24, 2025 12:47
@avolmat-st
Copy link

Hi @mjs513,

I will have a look at the code itself. I don't know how to make comment on the commit log itself so I write it here.
This is how it looks like now:

commit a1a7b03c47f93c32a6a947523fe5a017c377c430 (HEAD -> Add-New-OV7675-Driver)
Author: Mike S <[email protected]>
Date:   Sat Sep 20 10:13:23 2025 -0400

    drivers: video: Commit adds the OV7675 driver to the existing OV7670 driver
    
    drivers: video: Commit adds the OV7675 driver to the existing OV7670 driver
    
    Updates ov7670.c to incorporate the OV7675 driver into the OV7670 driver.
    
    Signed-off-by: Michael Smorto <[email protected]>

commit 4a441e429ef5164938660da1587d391791d1d00a
Author: Mike S <[email protected]>
Date:   Sat Sep 20 10:04:29 2025 -0400

    drivers: video: ov7670 driver changes in prep for adding OV7675
    
    drivers: video: ov7670 driver changes in prep for adding OV7675
    
    Updates ov7670.c in prep for incorporation of OV7675. See previous PR.
    
    Signed-off-by: Michael Smorto <[email protected]>

Aka, the short message like (first line) is repeated again within the commit log. Could you fix that ?
Moreover, only commits remains, so it is not possible to have See previous PR in a commit log. You need to describe what the commit does.

@mjs513 mjs513 requested a review from avolmat-st September 24, 2025 12:55
@mjs513
Copy link
Author

mjs513 commented Sep 24, 2025

@avolmat-st - @KurtE

All changes incorporated but can not seem to update the com10 register in prep commit (drivers: video: ov7670 driver changes in prep for adding OV7675) but it is fixed in the full commit.

I did manage to fix dual camera mode so should be ready to try when PRs are incorporated for multiple streams.

Tested on the GIGA - you are getting compile errors let me know what you are seeing.


/* OV7670 definitions */
#define OV7670_PROD_ID 0x76
#define OV7670_PROD_ID 0x76

Choose a reason for hiding this comment

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

There are still new spaces added before 0x76

@avolmat-st
Copy link

@mjs513
I only reviewed the first commit but since it seems like some of the comment I made before are valid again, I didn't proceed with the 2nd patch. I'll do that once this one is fixed.

@mjs513 mjs513 force-pushed the Add-New-OV7675-Driver branch from a1a7b03 to d2f5e51 Compare September 24, 2025 13:09
@mjs513
Copy link
Author

mjs513 commented Sep 24, 2025

Morning @avolmat-st

Thanks really strange. I am using Github desktop to push changes but I am not seeing that and using amend to make the change. So for

commit a1a7b03c47f93c32a6a947523fe5a017c377c430 (HEAD -> Add-New-OV7675-Driver)
Author: Mike S <[email protected]>
Date:   Sat Sep 20 10:13:23 2025 -0400

    drivers: video: Commit adds the OV7675 driver to the existing OV7670 driver
    
    drivers: video: Commit adds the OV7675 driver to the existing OV7670 driver
    
    Updates ov7670.c to incorporate the OV7675 driver into the OV7670 driver.
    
    Signed-off-by: Michael Smorto <[email protected]>

the only thing I am seeing after I do the amend (I am als updating for changes here)

…
drivers: video: Commit adds the OV7675 driver to the existing OV7670 driver

Updates ov7670.c to incorporate the OV7675 driver into the OV7670 driver + resolve initial comments + fixes for dual camera support.

Signed-off-by: Michael Smorto <[email protected]>

This is all I am seeing for the other commit

drivers: video: ov7670 driver changes in prep for adding OV7675

Updates ov7670.c in prep for incorporation of OV7675. See previous PR.

Signed-off-by: Michael Smorto <[email protected]>

So not sure how to fix that one

UPDATE: NOW FIXED

@avolmat-st
Copy link

Morning @avolmat-st

Thanks really strange. I am using Github desktop to push changes but I am not seeing that and using amend to make the change. So for

So not sure how to fix that one

I can't help with Github desktop, never used that.

@mjs513
Copy link
Author

mjs513 commented Sep 24, 2025

I can't help with Github desktop, never used that.
Playing with just using command line - any hints

Think I figured it out now to see about making changes

@mjs513
Copy link
Author

mjs513 commented Sep 25, 2025

@avolmat-st

Tried to incorporate changes to the 1st commit, i.e.,

drivers: video: ov7670 driver changes in prep for adding OV7675
4a441e429ef5164938660da1587d391791d1d00a

but things were getting so corrupted with updating the file had to stop the trying to do the updates.

Wondering if it would be better to temporarily delete the second commit

drivers: video: Commit adds the OV7675 driver to the existing OV7670 driver
d2f5e51bf3ecd07c3d301a76ad7208a0703cfb0e

and finish off any changes to the first and then resubmit the second commit?

UPDATE: Sorry went ahead and did this so should be easier to make the changes

@mjs513 mjs513 force-pushed the Add-New-OV7675-Driver branch 2 times, most recently from 2770d45 to d34d077 Compare September 25, 2025 15:39
@mjs513 mjs513 force-pushed the Add-New-OV7675-Driver branch 2 times, most recently from c27c3e3 to 226040f Compare October 21, 2025 11:58
@mjs513 mjs513 requested a review from avolmat-st October 21, 2025 11:59
@avolmat-st
Copy link

Haven't reviewed everything, however it seems to me that the memcpy on set_fmt is still missing. (already mentioned #96319 (comment) and #96319 (comment))

Was sure that the extra memcpy was needed. Anyways added it back in:

	if (!memcmp(&data->fmt, fmt, sizeof(data->fmt))) {
		/* nothing to do */
		return 0;
	}

	memcpy(&data->fmt, fmt, sizeof(data->fmt));

This only appears in OV767x_set_fmt in commit Add OV7675 changes to OV767x.

Ok just checked looks like I need to add it to as well for completeness: Update OV7670 to use generic ov767x functions

EDIT: All updated

This memcpy was part of the original code and without it data->fmt is never set so yes it is mandatory.

@mjs513
Copy link
Author

mjs513 commented Oct 21, 2025

This memcpy was part of the original code and without it data->fmt is never set so yes it is mandatory.

What I meant by mandatory is that since its already in the 'if' statement its already be set. Guess my C++ understanding is not quite there. Now you have me curious. Anyway its back in.

Copy link

@avolmat-st avolmat-st left a comment

Choose a reason for hiding this comment

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

Few small / minor points left, but apart from that (and Copyright point to be discussed), looks ok.

@mjs513 mjs513 force-pushed the Add-New-OV7675-Driver branch from 226040f to 240534b Compare October 21, 2025 12:58
@mjs513
Copy link
Author

mjs513 commented Oct 21, 2025

Few small / minor points left, but apart from that (and Copyright point to be discussed), looks ok.

As for the copyright did make one change - found that for openmv can use something simplier which I added for now until we get to this.

@avolmat-st
Copy link

This memcpy was part of the original code and without it data->fmt is never set so yes it is mandatory.

What I meant by mandatory is that since its already in the 'if' statement its already be set. Guess my C++ understanding is not quite there. Now you have me curious. Anyway its back in.

If you refer to the line just above. One is memcmp (compare) and the other one is memcpy (copy).

Anyway now that it is back in it is ok.

Copy link

@avolmat-st avolmat-st left a comment

Choose a reason for hiding this comment

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

Could you also simplify the commit messages ? It is not necessary to mention what has been discussed or steps to achieve this. The commit message should give information about what is done in this commit.

@mjs513 mjs513 force-pushed the Add-New-OV7675-Driver branch 2 times, most recently from 6b64183 to d886b3b Compare October 21, 2025 13:56
@mjs513
Copy link
Author

mjs513 commented Oct 21, 2025

Could you also simplify the commit messages ? It is not necessary to mention what has been discussed or steps to achieve this. The commit message should give information about what is done in this commit.

Was trying to keep a running tally of what changes were being made and would simplify at the end.

@mjs513 mjs513 requested a review from avolmat-st October 21, 2025 14:03
@mjs513 mjs513 force-pushed the Add-New-OV7675-Driver branch 4 times, most recently from 68915ae to db7475b Compare October 23, 2025 17:54
@josuah
Copy link
Contributor

josuah commented Oct 26, 2025

Hello @mjs513. It looks like you needed to import the latest main branch into your work. This is good idea but you will need to do this with a git rebase rather than a git merge. This is to avoid the "merge commit" as seen at the end:

screenshot_2025_1026_1001_35

Could you try the operation again? You might need a git reset --hard <commit_before_merge> to cancel the "git merge", then try again with git rebase origin/main to use "rebase" instead of "merge".

@mjs513
Copy link
Author

mjs513 commented Oct 26, 2025

Hello @mjs513. It looks like you needed to import the latest main branch into your work. This is good idea but you will need to do this with a git rebase rather than a git merge. This is to avoid the "merge commit" as seen at the end:

Could you try the operation again? You might need a git reset --hard <commit_before_merge> to cancel the "git merge", then try again with git rebase origin/main to use "rebase" instead of "merge".

Thats weird didnt notice that will double check\

EDIT:
@josuah
Looks like my for of zephyr pulled in changes to main - but I dont remember ever telling it to synch the changes. The last commit I see in github main is from oct 25 "release of 4.3-rc1?? weird..... trying to fix

Think if I do push pull of squashed changes it always going to want to update to latest changes!

drivers: video: Restructure OV760.c driver before adding OV7675 support

Modifications for use of video cci helpers in video_common.h.

Signed-off-by: Mike S <[email protected]>
drivers: video: Update OV7670 to use generic ov767x functions

Minimal changes to OV760 drive to use generic ov767x structures.
- This includes changes to function names.
-  Includes changes to defines as necessary.
-  Deleted camera_id as no longer required

Signed-off-by: Mike S <[email protected]>
drivers: video Changes ov7670 driver name to ov767x.c

Changes driver name from ov7670.c to ov767x.c.

Signed-off-by: Mike S <[email protected]>
@josuah josuah marked this pull request as draft October 26, 2025 14:58
@mjs513 mjs513 force-pushed the Add-New-OV7675-Driver branch from d919eff to 1172625 Compare October 26, 2025 15:02
@josuah josuah marked this pull request as ready for review October 26, 2025 15:12
drivers: video Add OV7675 changes to OV767x

This commit adds in support for the OV7675 camera.

Signed-off-by: Mike S <[email protected]>
@mjs513 mjs513 force-pushed the Add-New-OV7675-Driver branch from 1172625 to 022c38b Compare October 26, 2025 17:09
@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Boards/SoCs area: Devicetree Bindings area: Tests Issues related to a particular existing or missing test area: Video Video subsystem

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants