Re: [PATCH 2/2] media: i2c: imx219: Simplify imx219_get_binning() function

[Jai Luthra]

Hi Jacopo

Thanks for the review,

Quoting Jacopo Mondi (2025-10-17 15:19:37)
> On Fri, Oct 17, 2025 at 01:43:50PM +0530, Jai Luthra wrote:
> > In imx219_set_pad_format() there is now a constraint to enforce hbin ==
> > vbin. So, simplify the logic in imx219_get_binning() function by
> > removing dead code that handles the case where hbin != vbin.
> >
> > Signed-off-by: Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/media/i2c/imx219.c | 16 +++-------------
> >  1 file changed, 3 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 300935b1ef2497050fe2808e4ceedda389a75b50..48efdcd2a8f96b678f9819223e0f9895fb4025ea 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -409,24 +409,14 @@ static void imx219_get_binning(struct v4l2_subdev_state *state, u8 *bin_h,
> >       u32 hbin = crop->width / format->width;
> >       u32 vbin = crop->height / format->height;
> >
> > -     *bin_h = IMX219_BINNING_NONE;
> > -     *bin_v = IMX219_BINNING_NONE;
> > -
> > -     /*
> > -      * Use analog binning only if both dimensions are binned, as it crops
> > -      * the other dimension.
> > -      */
> >       if (hbin == 2 && vbin == 2) {
> >               *bin_h = IMX219_BINNING_X2_ANALOG;
> >               *bin_v = IMX219_BINNING_X2_ANALOG;
> 
> So we're always going for BINNING_ANALOG_X2 whenever we bin now
> 
> I tested the binned mode 1640x1232 and 640x480 (which should bin then
> crop) and both works fine.
> 
> I was wondering if we should then just rename ANALOG_X2 to X2 but the
> datasheet actually defines that mode as "x2 analog (special) binning"
> so I would keep the current name.
> 
> I didn't know, but the sensor can also x4 bin!

I actually tested the x4 binning with 640x480 sometime ago, it's available
here:
https://github.com/jailuthra/linux/tree/imx219_binning_4x

Will hopefully be upstream support for both x4 and x2 modes once Sakari's
series with binning controls is merged :-)

> 
> Anyway, for this patch, I would keep a comment around that says we
> always use the special analog binning mode which is now the default.
> 

Indeed, will add this in v2.

> This little nit apart
> Reviewed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> Tested-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> 
> Thanks
>   j
> 
> > -
> > -             return;
> > +     } else {
> > +             *bin_h = IMX219_BINNING_NONE;
> > +             *bin_v = IMX219_BINNING_NONE;
> >       }
> >
> > -     if (hbin == 2)
> > -             *bin_h = IMX219_BINNING_X2;
> > -     if (vbin == 2)
> > -             *bin_v = IMX219_BINNING_X2;
> >  }
> >
> >  static inline u32 imx219_get_rate_factor(struct v4l2_subdev_state *state)
> >
> > --
> > 2.51.0
> >

Thanks,
    Jai