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@ideasonboard.com>
---
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;
-
- 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.0On 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@ideasonboard.com>
> ---
> 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!
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.
This little nit apart
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Tested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
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
>
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@ideasonboard.com>
> > ---
> > 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@ideasonboard.com>
> Tested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> 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
© 2016 - 2026 Red Hat, Inc.