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

media: i2c: imx477: Add vsync trigger_mode parameter #4656

Merged

Conversation

neocortex-vision
Copy link

trigger_mode == 0 (default) => no effect / no registers written
trigger_mode == 1 => source
trigger_mode == 2 => sink

This can be set e.g. in /boot/cmdline.txt as imx477.trigger_mode=N
Setting it to 0 or not setting it at all has the same effect.

Tested on HQ cameras (XVS <-> XVS, GND <-> GND) with libcamera.

Credits to 6by9.

@6by9
Copy link
Contributor

6by9 commented Oct 28, 2021

The content looks OK, although we get a couple of warnings from scripts/checkpatch.pl --strict --codespell

No codespell typos will be found - file '/usr/share/codespell/dictionary.txt': No such file or directory
ERROR: do not initialise statics to 0
#25: FILE: drivers/media/i2c/imx477.c:28:
+static int trigger_mode = 0;

CHECK: Alignment should match open parenthesis
#55: FILE: drivers/media/i2c/imx477.c:1738:
+		imx477_write_reg(imx477, IMX477_REG_MC_MODE,
+			IMX477_REG_VALUE_08BIT, 1);

CHECK: Alignment should match open parenthesis
#57: FILE: drivers/media/i2c/imx477.c:1740:
+		imx477_write_reg(imx477, IMX477_REG_MS_SEL,
+			IMX477_REG_VALUE_08BIT, val);

CHECK: Alignment should match open parenthesis
#59: FILE: drivers/media/i2c/imx477.c:1742:
+		imx477_write_reg(imx477, IMX477_REG_XVS_IO_CTRL,
+			IMX477_REG_VALUE_08BIT, val);

CHECK: Alignment should match open parenthesis
#61: FILE: drivers/media/i2c/imx477.c:1744:
+		imx477_write_reg(imx477, IMX477_REG_EXTOUT_EN,
+			IMX477_REG_VALUE_08BIT, val);

ERROR: Missing Signed-off-by: line(s)

total: 2 errors, 0 warnings, 4 checks, 43 lines checked

As currently imx477.c is totally clean of checkpatch warnings, it would be nice to fix all of those up, even though --strict is not strictly required (those on linux-media seem to expect it though).

"Signed-off By" is required as we are going to upstream this driver, so all contributions need to be appropriately tagged - see https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin. Please note "sorry, no pseudonyms or anonymous contributions".

@naushir We were talking about this change being needed earlier in the week - one fewer job for you then.

@naushir
Copy link
Contributor

naushir commented Oct 28, 2021

Once the warning are cleared, LGTM.

@neocortex-vision
Copy link
Author

Thanks, done. Regarding initialisation, default (ineffective) value is now -1.

Copy link
Contributor

@6by9 6by9 left a comment

Choose a reason for hiding this comment

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

Thanks for the update.

Could you also squash your commits together. We apply them exactly as presented in the PR, so don't need your development history.

@@ -25,6 +25,10 @@ static int dpc_enable = 1;
module_param(dpc_enable, int, 0644);
MODULE_PARM_DESC(dpc_enable, "Enable on-sensor DPC");

static int trigger_mode = -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

It was fine using a value of 0, but a static variable is automatically zeroed and so didn't need it explicitly.

static int trigger_mode;
would have been fine.

@pelwell
Copy link
Contributor

pelwell commented Oct 28, 2021

If the aim is to squash everything into one commit, especially if the commit message on the first commit doesn't require modification, then we can squash during the merge, but for more complicated combinations it's necessary for the submitter to squash before pushing.

trigger_mode == 0 (default) => no effect / no registers written
trigger_mode == 1           => source
trigger_mode == 2           => sink

This can be set e.g. in /boot/cmdline.txt as imx477.trigger_mode=N

Signed-off-by: Jonas Jacob <[email protected]>
@6by9
Copy link
Contributor

6by9 commented Oct 28, 2021

Upstream may point out that
if (trigger_mode != 0)
can be simplified to
if (trigger_mode)
but otherwise that looks fine to me. Thanks for your efforts.

@pelwell pelwell merged commit 3728690 into raspberrypi:rpi-5.10.y Oct 28, 2021
@neocortex-vision
Copy link
Author

Thanks for the feedback and excuse the explicit initialisation/checking (it's habitual, coming from a MISRA background).
I was planning to add hsync, vsync and frstobe signal parameters but I see this may have already been planned @naushir.

@neocortex-vision neocortex-vision deleted the imx477_trigger_mode branch October 28, 2021 17:51
popcornmix added a commit to raspberrypi/firmware that referenced this pull request Nov 1, 2021
See: raspberrypi/linux#4406

kernel: media: i2c: imx477: Add vsync trigger_mode parameter
See: raspberrypi/linux#4656

kernel: bcm2835-v4l2-codec: Remove advertised support of VP8
See: raspberrypi/linux#4661

firmware: platform: Remove licence on VP6, VP8, Theora, and FLAC
See: raspberrypi/linux#4661
popcornmix added a commit to raspberrypi/rpi-firmware that referenced this pull request Nov 1, 2021
See: raspberrypi/linux#4406

kernel: media: i2c: imx477: Add vsync trigger_mode parameter
See: raspberrypi/linux#4656

kernel: bcm2835-v4l2-codec: Remove advertised support of VP8
See: raspberrypi/linux#4661

firmware: platform: Remove licence on VP6, VP8, Theora, and FLAC
See: raspberrypi/linux#4661
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants