Changes to the ADV7180 driver. Added parameters to allow the user to set; input method, pattern sele…

[SamDudley95]

…ct, and free run. Added a generic i2c write parameter to give the user more functionality

[pelwell]

@6by9 Setting aside the lack of adherence to coding standards and fact that this is a large change to mostly upstream file, what is your opinion of this PR?

[6by9]

Sorry, most of this is the wrong way to do things.

The dbg_input parameter was originally mine as there is an upstream hole in V4L2 for being able to select the input through the API. The V4L2 [GS]_INPUT ioctls don't quite match the V4L2 subdev API s_routing.
There have been patches proposed that expose [GS]_ROUTING subdev ioctls, but the series has stalled - https://patchwork.kernel.org/project/linux-media/list/?series=98277 patches 18&19.

Just rereading the 5.9 TI am437x vpfe driver (which parts of Unicam were based on), it does appear to manage to do a mapping from s_input to s_routing. I need to do a bit more digging to see if that is generic enough to be adapted or not. The davinci vpif_capture driver also has a mechanism to call s_routing, so it's one to investigate.

pattern_select should be done via control V4L2_CID_TEST_PATTERN. See imx219.c or similar for how to do that. No hacking with module parameters required for that one.

free_run would presumably be used in tandem with setting a test pattern, in which case it should be set automatically set.
From the user guide (UG-637)

Free-run mode provides the user with a stable clock and
predictable data if the input signal cannot be decoded, for
example, if input video is not present.

The ADV728x automatically enters free-run mode if the input
signal cannot be decoded. The user can prevent this operation
by setting the DEF_VAL_AUTO_EN to 0. When the DEF_VAL_
AUTO_EN bit is set to 0, the ADV728x outputs noise if it
cannot decode the input video. It is recommended that the user
keep DEF_VAL_AUTO_EN set to 1.

The user can force free-run mode by setting the DEF_VAL_EN
bit to 1. This can be a useful tool in debugging system level issues.

I can only see this being useful as an independent control if there is a valid use for test pattern synchronised to the incoming video. That's a small enough test case that I wouldn't consider it useful.

Directly prodding I2C registers from a module parameter is a definite no-no. Implement g_register and s_register which are under CONFIG_VIDEO_ADV_DEBUG. See adv7183.c

This is a maintained upstream driver. We have 2 small patches over and above that which I really ought to get around to upstreaming. Certainly adding V4L2_CID_TEST_PATTERN correctly should be acceptable to mainline, as I would expect would be adding g_register and s_register.
They do need to be submitted as two independent patches though, and with Signed-off-by lines to allow others to upstream it should you choose not to.

[6by9]

This is a partial revert of 6457b62
Why?
The chip produces 2 independent frames rather than line by line interleaved.
https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/field-order.html#c.v4l2_field

[6by9]

No need to change the text here.

[6by9]

Nor add debug messages here.

[SamDudley95]

Sorry these three were for debug I forgot to revert them. I'm happy for this to not be pulled as I've clearly implemented it incorrectly.

[6by9]

Thanks for trying - it's the way to learn.
It's only though years of watching upstream and working with the APIs that I know the "proper way" to implement these things - they aren't always obvious. Hacks are reserved for where there isn't a "proper way" (eg dbg_input and I'm now looking to fix that properly).

Where drivers have the potential to be upstreamed, or are upstream drivers in the first place, we do try to keep to the coding standards and play by upstream's rules. It's a bit of a slog to start with, and you'll be swearing at checkpatch for a while, but it allows others to make use of your work and try upstreaming it without ambiguity.

[pelwell]

Thanks, @6by9.

[SamDudley95]

Thank you both for the feedback, @6by9 @pelwell. If I implement these changes as you suggested should I start from the upstream driver or is it OK to continue from here?

[6by9]

Starting from here is fine. It's not a driver that gets a huge amount of upstream development effort so the likelihood of conflicts is low, and it's easier to test on these branches compared to mainline.

If they're done cleanly then you may persuade me to do battle with upstream on your behalf when I come to do my couple of patches. That's why I need you to have followed the mainline processes so that your contributions can be taken.

[6by9]

g_register and s_register have just been discussed on the linux-media list for a different driver
https://www.spinics.net/lists/linux-media/msg177929.html

As the name of the operations imply, this is meant for debug purpose only. They are however prone to be abused to configure the sensor from userspace in production, which isn't a direction we want to take. What's your use case for this ? I'd rather drop this patch and see the driver extended with support for more controls if needed

My personally view is that as it's hidden behind CONFIG_VIDEO_ADV_DEBUG (which isn't normally defined) it's not really a big deal, but mainline seem to be taking a different view.
Is there a specific register you were needing to tweak?

[SamDudley95]

Nothing specifically, it was just to avoid rebuilding the driver if I wanted to tweak a register later on.