Re: [PATCH 05/13] media: imx355: Set register LINE_LENGTH_PCK programmatically

[Dave Stevenson]

Hi Jacopo

On Thu, 7 May 2026 at 15:09, Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Dave
>
> On Wed, May 06, 2026 at 07:23:43PM +0100, Dave Stevenson wrote:
> > The driver already has the LLP value stored in the mode structure,
> > but also had the same value set via register writes in the mode's
> > register list. Remove this duplication.
>
> Moving stuff from register data tables to modes is .. better for sure.
> You know.. freely configurable etc etc
> But this is an improvement, so...

AFAIK there is currently no accepted way to convert a driver from
having a load of defined modes to freely configurable.
This driver already has 14 modes advertised, so how do you retain
backwards compatibility with selecting those modes via set_fmt whilst
also allowing freely configurable cropping? AIUI set_fmt is not
allowed to update the value returned from get_selection. That means
we're stuck with these modes.
I also don't currently have a datasheet for this sensor, so I don't
know what restrictions are documented with respect to cropping on this
sensor.

> Reviewed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
>
> >
> > This can't be implemented via a s_ctrl handler for V4L2_CID_HBLANK
> > as __v4l2_ctrl_handler_setup doesn't call s_ctrl for read only
> > controls.
>
> Kind of unrelated, but I wonder if imx355_set_ctrl() doesn't actually
> get called for hblank but the error simply goes ignored. The real
> solution here would be to initialize the hblank control with a NULL
> control handler.

This was partly a comment for future me as well as reviewers. I'd
expected to be able to handle it in s_ctrl and then found it didn't
work.

I believe you are right that the
__v4l2_ctrl_modify_range(imx355->hblank....) call in
imx355_set_pad_format will result in s_ctrl being called for
V4L2_CID_HBLANK, but the return value isn't checked. When min, max,
and def are all the same, it isn't going to fail. Using a NULL control
handler would tidy that up though.

 Dave

> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > ---
> > ---
> >  drivers/media/i2c/imx355.c | 38 ++++++++++----------------------------
> >  1 file changed, 10 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> > index 589bad6c58e4..56a82f37709e 100644
> > --- a/drivers/media/i2c/imx355.c
> > +++ b/drivers/media/i2c/imx355.c
> > @@ -34,6 +34,9 @@
> >  #define IMX355_REG_FLL                       0x0340
> >  #define IMX355_FLL_MAX                       0xffff
> >
> > +#define IMX355_REG_LLP                       0x0342
> > +#define IMX355_LLP_MAX                       0xffff
> > +
> >  #define IMX355_REG_X_ADD_START               0x0344
> >  #define IMX355_REG_Y_ADD_START               0x0346
> >  #define IMX355_REG_X_ADD_END         0x0348
> > @@ -266,8 +269,6 @@ static const struct imx355_reg_list imx355_global_setting = {
> >  };
> >
> >  static const struct imx355_reg mode_3268x2448_regs[] = {
> > -     { 0x0342, 0x0e },
> > -     { 0x0343, 0x58 },
> >       { 0x0900, 0x00 },
> >       { 0x0901, 0x11 },
> >       { 0x0902, 0x00 },
> > @@ -276,8 +277,6 @@ static const struct imx355_reg mode_3268x2448_regs[] = {
> >  };
> >
> >  static const struct imx355_reg mode_3264x2448_regs[] = {
> > -     { 0x0342, 0x0e },
> > -     { 0x0343, 0x58 },
> >       { 0x0900, 0x00 },
> >       { 0x0901, 0x11 },
> >       { 0x0902, 0x00 },
> > @@ -286,8 +285,6 @@ static const struct imx355_reg mode_3264x2448_regs[] = {
> >  };
> >
> >  static const struct imx355_reg mode_3280x2464_regs[] = {
> > -     { 0x0342, 0x0e },
> > -     { 0x0343, 0x58 },
> >       { 0x0900, 0x00 },
> >       { 0x0901, 0x11 },
> >       { 0x0902, 0x00 },
> > @@ -296,8 +293,6 @@ static const struct imx355_reg mode_3280x2464_regs[] = {
> >  };
> >
> >  static const struct imx355_reg mode_1940x1096_regs[] = {
> > -     { 0x0342, 0x0e },
> > -     { 0x0343, 0x58 },
> >       { 0x0900, 0x00 },
> >       { 0x0901, 0x11 },
> >       { 0x0902, 0x00 },
> > @@ -306,8 +301,6 @@ static const struct imx355_reg mode_1940x1096_regs[] = {
> >  };
> >
> >  static const struct imx355_reg mode_1936x1096_regs[] = {
> > -     { 0x0342, 0x0e },
> > -     { 0x0343, 0x58 },
> >       { 0x0900, 0x00 },
> >       { 0x0901, 0x11 },
> >       { 0x0902, 0x00 },
> > @@ -316,8 +309,6 @@ static const struct imx355_reg mode_1936x1096_regs[] = {
> >  };
> >
> >  static const struct imx355_reg mode_1924x1080_regs[] = {
> > -     { 0x0342, 0x0e },
> > -     { 0x0343, 0x58 },
> >       { 0x0900, 0x00 },
> >       { 0x0901, 0x11 },
> >       { 0x0902, 0x00 },
> > @@ -326,8 +317,6 @@ static const struct imx355_reg mode_1924x1080_regs[] = {
> >  };
> >
> >  static const struct imx355_reg mode_1920x1080_regs[] = {
> > -     { 0x0342, 0x0e },
> > -     { 0x0343, 0x58 },
> >       { 0x0900, 0x00 },
> >       { 0x0901, 0x11 },
> >       { 0x0902, 0x00 },
> > @@ -336,8 +325,6 @@ static const struct imx355_reg mode_1920x1080_regs[] = {
> >  };
> >
> >  static const struct imx355_reg mode_1640x1232_regs[] = {
> > -     { 0x0342, 0x07 },
> > -     { 0x0343, 0x2c },
> >       { 0x0900, 0x01 },
> >       { 0x0901, 0x22 },
> >       { 0x0902, 0x00 },
> > @@ -346,8 +333,6 @@ static const struct imx355_reg mode_1640x1232_regs[] = {
> >  };
> >
> >  static const struct imx355_reg mode_1640x922_regs[] = {
> > -     { 0x0342, 0x07 },
> > -     { 0x0343, 0x2c },
> >       { 0x0900, 0x01 },
> >       { 0x0901, 0x22 },
> >       { 0x0902, 0x00 },
> > @@ -356,8 +341,6 @@ static const struct imx355_reg mode_1640x922_regs[] = {
> >  };
> >
> >  static const struct imx355_reg mode_1300x736_regs[] = {
> > -     { 0x0342, 0x07 },
> > -     { 0x0343, 0x2c },
> >       { 0x0900, 0x01 },
> >       { 0x0901, 0x22 },
> >       { 0x0902, 0x00 },
> > @@ -366,8 +349,6 @@ static const struct imx355_reg mode_1300x736_regs[] = {
> >  };
> >
> >  static const struct imx355_reg mode_1296x736_regs[] = {
> > -     { 0x0342, 0x07 },
> > -     { 0x0343, 0x2c },
> >       { 0x0900, 0x01 },
> >       { 0x0901, 0x22 },
> >       { 0x0902, 0x00 },
> > @@ -376,8 +357,6 @@ static const struct imx355_reg mode_1296x736_regs[] = {
> >  };
> >
> >  static const struct imx355_reg mode_1284x720_regs[] = {
> > -     { 0x0342, 0x07 },
> > -     { 0x0343, 0x2c },
> >       { 0x0900, 0x01 },
> >       { 0x0901, 0x22 },
> >       { 0x0902, 0x00 },
> > @@ -386,8 +365,6 @@ static const struct imx355_reg mode_1284x720_regs[] = {
> >  };
> >
> >  static const struct imx355_reg mode_1280x720_regs[] = {
> > -     { 0x0342, 0x07 },
> > -     { 0x0343, 0x2c },
> >       { 0x0900, 0x01 },
> >       { 0x0901, 0x22 },
> >       { 0x0902, 0x00 },
> > @@ -396,8 +373,6 @@ static const struct imx355_reg mode_1280x720_regs[] = {
> >  };
> >
> >  static const struct imx355_reg mode_820x616_regs[] = {
> > -     { 0x0342, 0x0e },
> > -     { 0x0343, 0x58 },
> >       { 0x0900, 0x01 },
> >       { 0x0901, 0x44 },
> >       { 0x0902, 0x00 },
> > @@ -1041,6 +1016,13 @@ static int imx355_start_streaming(struct imx355 *imx355)
> >       if (ret)
> >               return ret;
> >
> > +     /* set line length */
> > +     ret = imx355_write_reg(imx355, IMX355_REG_LLP,
> > +                            imx355->hblank->val + imx355->cur_mode->width,
> > +                            2);
> > +     if (ret)
> > +             return ret;
> > +
> >       /* Apply customized values from user */
> >       ret =  __v4l2_ctrl_handler_setup(imx355->sd.ctrl_handler);
> >       if (ret)
> >
> > --
> > 2.34.1
> >
> >