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

Jump to bottom

IMX708 link frequency changes #5381

Merged
merged 8 commits into from
Mar 31, 2023
Merged

IMX708 link frequency changes #5381

merged 8 commits into from
Mar 31, 2023

Conversation

naushir
Copy link
Contributor

@naushir naushir commented Mar 13, 2023 •
edited
Loading

This is to assist with raspberrypi/libcamera#43.

Now ready for review.

@naushir naushir force-pushed the rpi-6.1.y branch 5 times, most recently from 3aa05ac to b6ed68a Compare March 14, 2023 13:00
@naushir naushir changed the title WIP: DNI: TEST PATCH FOR IMX708 CLOCK CHANGES IMx708 clock changes Mar 29, 2023
@naushir
Copy link
Contributor Author

naushir commented Mar 29, 2023

Slight change of plans. Will use a overlay parameter instead of a v4l2 ctrl.

@naushir naushir changed the title IMx708 clock changes IMX708 link frequency changes Mar 29, 2023
6by9
6by9 reviewed Mar 29, 2023
View reviewed changes
drivers/media/i2c/imx708.c Outdated Show resolved Hide resolved
drivers/media/i2c/imx708.c Outdated Show resolved Hide resolved
drivers/media/i2c/imx708.c Outdated Show resolved Hide resolved
drivers/media/i2c/imx708.c Outdated Show resolved Hide resolved
@6by9
Copy link
Contributor

6by9 commented Mar 29, 2023

Unrelated but just noticed in reviewing.

	struct v4l2_ctrl *red_balance;
	struct v4l2_ctrl *blue_balance;

in struct imx708 appear unused.

struct v4l2_ctrl *notify_gains; I think is also redundant, as I believe you can use

		ret = imx708_write_reg(imx708, IMX708_REG_COLOUR_BALANCE_BLUE,
				       IMX708_REG_VALUE_16BIT,
				       ctrl->p_new.p_u32[0]);
		if (ret)
			break;
		ret = imx708_write_reg(imx708, IMX708_REG_COLOUR_BALANCE_RED,
				       IMX708_REG_VALUE_16BIT,
				       ctrl->p_new.p_u32[3]);

in imx708_set_ctrl. It should come back to the same place.

@naushir
Copy link
Contributor Author

naushir commented Mar 31, 2023

Unrelated but just noticed in reviewing.

	struct v4l2_ctrl *red_balance;
	struct v4l2_ctrl *blue_balance;

in struct imx708 appear unused.

struct v4l2_ctrl *notify_gains; I think is also redundant, as I believe you can use

		ret = imx708_write_reg(imx708, IMX708_REG_COLOUR_BALANCE_BLUE,
				       IMX708_REG_VALUE_16BIT,
				       ctrl->p_new.p_u32[0]);
		if (ret)
			break;
		ret = imx708_write_reg(imx708, IMX708_REG_COLOUR_BALANCE_RED,
				       IMX708_REG_VALUE_16BIT,
				       ctrl->p_new.p_u32[3]);

in imx708_set_ctrl. It should come back to the same place.

I'll fix these up in a separate commt.

@naushir
Copy link
Contributor Author

naushir commented Mar 31, 2023

Updated PR with the changes. I've had to remove the 444Mhz link frequency as it does not work correctly, even with a lower fps. The next link frequency available 445.5Mhz doesn't work either. I've added an option for 453Mhz instead, which does seem to be stable.

I've also added a 3rd patch to this PR to address the upstream review feedback comments. I'll send upstream a new revision of the patch including all these changes once they are merged into our tree.

@6by9
Copy link
Contributor

6by9 commented Mar 31, 2023

Patches look good.

Didn't upstream want the regulator names to be in lower case? I seem to recall updating the bindings based on that.

Again for upstream, I thought I'd seen a comment over using a v4l2_ctrl cluster for HFLIP & VFLIP so that the s_ctrl only gets called the once instead of twice. It may have been with regard a different driver, but it does apply here too.

@naushir
Copy link
Contributor Author

naushir commented Mar 31, 2023

Didn't upstream want the regulator names to be in lower case? I seem to recall updating the bindings based on that.

Again for upstream, I thought I'd seen a comment over using a v4l2_ctrl cluster for HFLIP & VFLIP so that the s_ctrl only gets called the once instead of twice. It may have been with regard a different driver, but it does apply here too.

I don't recall those specific comments. I'm following patchworks here. But I can fix them up as well.

@6by9
Copy link
Contributor

6by9 commented Mar 31, 2023

Case of regulator names is on the dtbindings patch - https://patchwork.linuxtv.org/project/linux-media/patch/20230124150546.12876-2-naush@raspberrypi.com/#143941
The driver obviously has to match the binding.

Clusters was a comment from Laurent on imx290 - https://patchwork.linuxtv.org/project/linux-media/patch/20230131192016.3476937-12-dave.stevenson@raspberrypi.com/#144338
It is trivial to add - see https://git.linuxtv.org/sailus/media_tree.git/commit/drivers/media/i2c/imx290.c?id=924350ae03259b613a29a38c1e6f5b439eb9ce66

@naushir
Copy link
Contributor Author

naushir commented Mar 31, 2023 •
edited
Loading

Added 3 new patches on top for the dtb changes and h/v flip cluster control change.

@6by9
Copy link
Contributor

6by9 commented Mar 31, 2023

LGTM, and hopefully in reasonable shape for a v2 to mainline.
If only we had the multiplexed stream support to get rid of the multiple pads bit then we could actually use mainline!

@pelwell
Copy link
Contributor

pelwell commented Mar 31, 2023

  1. There are a number of typos in the commit messages - "dtbinding"->"dt-bindings", "documenation"->"documentation", "cluser"->"cluster".
  2. The first commit ("drivers: media: imx708: Increase usable link frequencies") and fifth commit ("drivers: media: imx708: Follow the documented devicetree labels") include driver and overlay changes, which makes the usual DT squashing more awkward.
  3. Checkpatch is complaining about the fourth commit changing both yaml and headers, but I can't see it (unless the MAINTAINERS file counts as a header).

Let me know when you've made whatever changes you are going to and I'll merge it.

naushir added 7 commits March 31, 2023 14:56
@naushir
drivers: media: imx708: Increase usable link frequencies
7b47514 
Add support for three different usable link frequencies (default 450Mhz,
447Mhz, and 453MHz) for the IMX708 camera sensor. The choice of
frequency is handled thorugh the "link-frequency" overlay parameter.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
@naushir
dtoverlays: Add link-frequency parameter for the Sony IMX708 sensor
dc08e77 
Add a parameter to change the sensor device CSI-2 link frequency to
one of the following values: 450Mhz (default), 447Mhz, or 453Mhz.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
@naushir
drivers: media: imx708: Remove unused control fields
5e935c6 
Remove unused and redundant control fields from the state structure.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
@naushir
drivers: media: imx708: Tidy-ups to address upstream review comments
73d6ede 
This commit addresses vaious tidy-ups requesed for upstreaming the
IMX708 driver. Notably:

- Remove #define IMX708_NUM_SUPPLIES and use ARRAY_SIZE() directly
- Use dev_err_probe where possible in imx708_probe()
- Fix error handling paths in imx708_probe()

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
@naushir
dt-bindings: media: i2c: Replace IMX708 sensor binding documentation …
c656fbc 
…file

Replace the existing imx708.yaml file with sony,imx708.yaml that follows
the latest devicetree conventions for camera sensors.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
@naushir
dtoverlays: Follow the standard devicetree labels for IMX708
ef74ecc 
Switch the system clock name from "xclk" to "inclk".
Use lower case lables for all regulator names.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
@naushir
drivers: media: imx708: Follow the standard devicetree labels
fefeb7b 
Switch the system clock name from "xclk" to "inclk".
Use lower case lables for all regulator names.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
@naushir
drives: media: imx708: Put HFLIP and VFLIP controls in a cluster
e7f3238 
Create a cluster for the HVLIP and VFLIP controls so they are treated
as a single composite control.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
@naushir
Copy link
Contributor Author

naushir commented Mar 31, 2023

I think that should cover it all. Let me know if I've missed anything!

@pelwell pelwell merged commit aacdf1d into raspberrypi:rpi-6.1.y Mar 31, 2023
@naushir naushir mentioned this pull request Apr 3, 2023
Closed
popcornmix added a commit to raspberrypi/firmware that referenced this pull request Apr 3, 2023
@popcornmix
kernel: gpio-fsm: Avoid truncation of delay jiffies
add7273 
See: raspberrypi/linux#5416

kernel: media: bcm2835-unicam: Start and stop media_pipeline with same node
See: raspberrypi/linux#5409

kernel: IMX708 link frequency changes
See: raspberrypi/linux#5381
popcornmix added a commit to raspberrypi/rpi-firmware that referenced this pull request Apr 3, 2023
@popcornmix
kernel: gpio-fsm: Avoid truncation of delay jiffies
0b15297 
See: raspberrypi/linux#5416

kernel: media: bcm2835-unicam: Start and stop media_pipeline with same node
See: raspberrypi/linux#5409

kernel: IMX708 link frequency changes
See: raspberrypi/linux#5381
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@6by9 6by9 6by9 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@naushir @6by9 @pelwell