dts: ov5647 Fix link frequency and media: i2c: ov5647: Add checks for link frequency

[Bajec]

dts: ov5647:
Updated the link frequency from 297000000 to the correct values of 218500000 and 208333000
Link frequency 297000000 was not used in the driver and was incorrect.

media: i2c: ov5647:
Added validation for the link-frequency property in the driver. The new checks ensure that:

• The link-frequency property is present in the device tree
• The number of link frequencies matches the ov5647_link_freqs array size
• The link frequencies in the device tree match the supported values in ov5647_link_freqs array

These checks help avoid misconfigurations and prevent runtime errors due to incorrect declarations

[pelwell]

This is not my area of expertise, so this may be a dumb question, but:

What is the point of having the same table of frequencies in the code and in DT? If it's an inherent property of all instances of the device then isn't it better to just put it in the code?

[Bajec]

This is not my area of expertise, so this may be a dumb question, but:

What is the point of having the same table of frequencies in the code and in DT? If it's an inherent property of all instances of the device then isn't it better to just put it in the code?

Device Tree is used to select the sensor driver and configure parameters such as number of CSI-2 lanes, continuous clock lane operation, and link frequency.

Link frequency can vary due to the selected mode.

This is actually a follow-up to #6423 (comment)

[pelwell]

That doesn't answer the question - what is the value of having the same information in two places, and the extra burden of code to make sure the two are the same? If the driver had a list of allowable frequencies and DT selected one, that would make sense, but that's not what you're doing here.

[Bajec]

That doesn't answer the question - what is the value of having the same information in two places, and the extra burden of code to make sure the two are the same? If the driver had a list of allowable frequencies and DT selected one, that would make sense, but that's not what you're doing here.

The same approach is used in IMX* drivers; I don't really see a reason for the concern over doing this.
I don't mean to be rude, but isn't a wrongly defined link frequency in the driver/Device Tree (that is actually released as stable) worse than adding a few extra checks?

[pelwell]

The same approach is used in IMX* drivers

Are you sure? The ones I looked at report an error if more than one frequency is found in the DT, i.e. "driver had a list of allowable frequencies and DT selected one".

Plus, your checking will fail if the frequencies are listed in a different order.

[Bajec]

Are you sure?

Here you go:
driver
dtree

Plus, your checking will fail if the frequencies are listed in a different order.

You're looking for a needle in a haystack now. User has no contact with the link-frequency selection to produce error. Why would you check and complicate things even more for an unordered case?

Then we can say what if you incorrectly calculate the link frequency as it was in the first place? What part of code will check that? (I will repeat - stable release was wrong and that was ok, now fix is the problem)

And you are contradicting yourself:

extra burden of code to make sure the two are the same?

First I "complicate" code, then you want to complicate it more to check for unordered case?

I mean, if this is too difficult to understand, feel free to reject the pull request.

[pelwell]

From the PR that prompted this:

As I've said, there will be a follow up patch that will read the link frequencies and configure the clock setup accordingly.

I'll wait and see what @6by9 comes up with.

[Bajec]

Sure, maybe we will both learn something new.

[6by9]

I agree with the patch to the dtoverlay - the numbers in there are wrong and should be corrected.

The driver patch is trickier.
DT is considered ABI, and you've now broken existing DT configurations that used to work. link-frequencies is not mandatory according to the bindings for ov5647.
When there are no options on link frequency then there is minimal point in validating it anyway. Many drivers (including this one) don't even validate the number of CSI2 data lanes, and that is more critical to driver configuration.

I was looking at adding the long exposure mode to the driver yesterday which will be selected via a new link frequency value. The DT parsing needs to fall back to defaults if the link frequency is missing, has the incorrect number of entries, or has unsupported values.

(We've also got 5 patches when we only want 2. Fixups should be squashed into the original patch and the branch force-pushed)

[Bajec]

I don't want to spend time on this anymore. If you are happy with current state of the driver then I'm too.

[6by9]

Seeing as you'll find https://github.com/torvalds/linux/blob/master/MAINTAINERS#L17026-L17033 reads

OMNIVISION OV5647 SENSOR DRIVER
M:	Dave Stevenson <[email protected]> <<<<<<<<<<<<<<<<
M:	Jacopo Mondi <[email protected]>
L:	[email protected]
S:	Maintained
T:	git git://linuxtv.org/media_tree.git
F:	Documentation/devicetree/bindings/media/i2c/ovti,ov5647.yaml
F:	drivers/media/i2c/ov5647.c

I have a significant interest in this driver, but partly in ensuring that we're doing everything in a way that is compatible with upstream.

Then again it looks like I've been a bit amiss in not upstreaming our own patches based on https://github.com/raspberrypi/linux/commits/rpi-6.12.y/drivers/media/i2c/ov5647.c - I'll be finding a few minutes to do that as soon as I can.