Re: [PATCH v3 2/2] media: i2c: Add Samsung S5K3T2 image sensor driver

[armandas.kvietkus]

Hi Sakari,

Thanks for the review. It is really good to get feedback from a person
after the bot reviews, so I appreciate you taking the time.

On Thu, Oct 01, 2026 at 10:19:21AM +0300, Sakari Ailus wrote:
> Is Documentation/process/coding-assistants.rst relevant for this
> contribution?

No, it is not relevant. No coding assistants were used for this driver.

> But HBLANK should be handled below, shouldn't it? The fewer of the
> configuration that could be dynamic is set through register lists, the
> better.

Yes. The line length register 0x0342 holds the same value as hts in the
mode table. I'll write width + HBLANK there from s_ctrl and take it
out of the mode tables.

> Hmm. The link frequency is supposed to be set by the user, not by the
> driver. How do you change mode in that case?

The mode decides it. Each mode's register list sets the PLL, so the
link frequency comes with the mode. set_fmt picks the mode and then
sets the control to match, like imx258 does.

The modes are also tied to their frequency by bandwidth. 2592x1940 at
60 fps needs about 3.0 gbit/s, and four lanes at 300 MHz carry 2.4
gbit/s. So of the three modes, only 1280x720 fits at 300 MHz.

If you prefer a writable control, I can do that and only offer the
modes that fit the selected frequency. Which one do you want?

> Can you set these using the control handler's set callback? They should be
> clustered, too.

Yes. I'll write the flips from s_ctrl and cluster them.

> How about calling this s5k3t2_format_from_mode()?

Sure.

I'll fix the rest in v4.

Armandas