Re: [PATCH 2/2] media: i2c: imx471: Add Sony IMX471 image sensor driver

[Kate Hsuan]

Hi Hans,

On Fri, Apr 17, 2026 at 6:16 PM Hans de Goede
<johannes.goede@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Kate,
>
> On 17-Apr-26 10:32, Kate Hsuan wrote:
> > Add a new driver for Sony imx471 camera sensor. It is based on
> > Jimmy Su <jimmy.su@xxxxxxxxx> implementation and the driver can be found
> > in the following URL.
> > https://github.com/intel/ipu6-drivers/commits/master/drivers/media/i2c/imx471.c
> >
> > This sensor can be found on Lenovo X9-14 and X9-15 laptop and it is a part
> > of IPU7 solution. The driver was tested on Lenovo X9-14 and X9-15 laptops.
> >
> > Link: https://github.com/intel/ipu6-drivers/blob/master/drivers/media/i2c/imx471.c
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2454119
> > Signed-off-by: Kate Hsuan <hpa@xxxxxxxxxx>
> <snip>
>
> > diff --git a/drivers/media/i2c/imx471.c b/drivers/media/i2c/imx471.c
> > new file mode 100644
> > index 000000000000..32a105a60731
> > --- /dev/null
> > +++ b/drivers/media/i2c/imx471.c
> > @@ -0,0 +1,1047 @@
>
> <snip>
>
> > +static int imx471_update_flip(struct imx471_data *sensor, u32 value,
> > +                           u8 flip_bit)
> > +{
> > +     int ret;
> > +     u64 val = value ? flip_bit : 0;
> > +
> > +     if (sensor->streaming)
> > +             return -EBUSY;
>
> I see no reason why this could not be updated while streaming,
> since the h/y offsets get adjusted the bayer pattern stays
> the same so changing while streaming should be fine.
>
> > +
> > +     /* hflip */
> > +     /*
> > +      * Some manufacturers mount the sensor upside-down (rotation == 180).
> > +      * V4L2 sets both h/vflip to 1 for 180-degree rotation, but only the
> > +      * vflip should actually be applied. Skip the initial hflip write to
> > +      * preserve correct orientation.
> > +      */
>
> I was answering your off-list email about this, but now I see that you've
> added this workaround here. I believe that this workaround is wrong, so
> let me move answer things here instead of off-list:
>
> > I filled in the DMI information in the table and I found v4l2 sets up
> > both hflip=1 and vflip=1 when the rotation is 180.
>
> Yes that is correct, note this is actually done by libcamera, in response
> to the rotation property reporting 180 degrees rotation after adding the
> laptop to the DMI table.
>
> > In my case, I only
> > need to set vflip then I can get a correct image.
>
> First of all are you sure that you only need to set vflip? A camera is not
> a mirror! If you say raise your right hand in front of the camera then on
> the screen you should be seen raising the hand which is on the left for
> "the you" looking at the screen because if you were to look at you from
> the pov of the camera your right hand is on the left.
>
> The easiest way to check this is to have something with some written text
> on it. In a mirror you cannot (easily) read e.g. the text printed on
> a T-shirt but with a camera you should be able to read this without
> problems.
>
> Also make sure you use qcam to test because qcam does not mirror/hflip.
> Some apps hflip the image for you (esp. things like google meet) because
> people are so used to seeing themselves in a mirror that they adjust
> the view for you. Note e.g. google meet only mirrors your own preview
> it sends out an unmirrored image to the people on the call (IIRC).

Thank you for answering this in detail.
I was confused by the Camera app that hflip the image from a UVC
camera and the default hflip register setting.
Now I know how I can fix my v1 and recalibrate my 3D space recognition.

>
> If after this long mansplaining (sorry) writeup about the difference
> between a mirror and a camera you still think you only need vflip,
> then that means that either the hflip ot the vflip control of
> the sensor is inverted and the driver needs to invert it.
>
> Are we sure the camera module is upside down? Maybe vflip is the one
> which we need to invert and the module is not upside-down at all ?
>
> Hmm, looking at other imx sensor drivers, unlike ov sensors where
> sometimes hflip is inverted it seems the 2 flip controls are sofar
> always straight forward on imx. Although some drivers only implement
> vflip and have no hflip at all.
>
> As you mention in the cover letter this is a cleaned up version of:
> https://github.com/intel/ipu6-drivers/tree/master/drivers/media/i2c/imx471.c
>
> Note that we've seen issues with mirroring / flipping from various
> other drivers originating from Intel, they have not always got this
> correct, especially when it comes to mirroring by default (when
> the hflip control's value is 0) but also with vflipping by default
> when the driver was developed on a laptop which had the module
> upside-down, see e.g. :
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/media/i2c/ov02c10.c

Yes, ov02c10 is my example.

>
> where we needed to do quite a few flipping related fixes.
>
> > +     if (flip_bit == IMX471_HFLIP_BIT) {
> > +             if (sensor->rotation == 180 && !sensor->hflip_initialized) {
> > +                     sensor->hflip_initialized = true;
> > +                     return 0;
> > +             }
>
> This looks like you skip writing the hflip on the first start stream,
> but what about subsequent streams ?

I'll drop this. It is unnecessary.

>
> Also see my next comment below, I think this skipping only once
> does point us in the right direction.
>
> > +
> > +             cci_update_bits(sensor->regmap, IMX471_REG_ORIENTATION,
> > +                             flip_bit, val, &ret);
> > +
> > +             return ret;
> > +     }
> > +
> > +     /* vflip */
> > +     cci_update_bits(sensor->regmap, IMX471_REG_ORIENTATION,
> > +                     flip_bit, val, &ret);
> > +     if (ret)
> > +             return ret;
>
> Hmm, I wonder if the problem here is you doing 2 subsequent
> cci_update_bits(). If the flip control registered is double-buffered
> and the new value is latched as the actual value on the start
> of the next frame; and this is combined with reading back
> reading the active value, not the last written value then
> the first time you do this the setting of the hflip bit will
> be overwritten by the second cci_update_bits.
>
> I think it would be better to do something similar to what
> imx219.c and replace these 2 cci_update_bits() calls with:
>
>         cci_write(imx471->regmap, IMX471_REG_ORIENTATION,
>                   imx471->hflip->val | imx471->vflip->val << 1, &ret);
>
> I believe this should work here too.

Okay.

>
>
> > +
> > +     cci_write(sensor->regmap, IMX471_REG_V_WIN_OFFSET,
> > +               value ? 0xe0 : 0xeb, &ret);
> > +     if (ret)
> > +             return ret;
> > +
> > +     cci_update_bits(sensor->regmap, IMX471_REG_H_WIN_OFFSET, 1,
> > +                     value ? 0x01 : 0x00, &ret);
>
> No need for cci_update_bits() here, the register is always
> initialized to 0xc8 so this can just use hardcoded values
> like the V_WIN_OFFSET path:

Sorry for my test code. I'll drop it.

>
>         cci_write(sensor->regmap, IMX471_REG_H_WIN_OFFSET,
>                   value ? 0xc9 : 0xc8, &ret);
>
> > +     return ret;
>
> Updating both offsets here is wrong when hflip != vflip, you
> should only update V_WIN_OFFSET when changing vflip and
> H_WIN_OFFSET when changing hflip.
>
> I suggest dropping this function and instead in set_ctrl()
> do this:
>
>         case V4L2_CID_HFLIP:
>                 cci_write(imx471->regmap, IMX471_REG_ORIENTATION,
>                           imx471->hflip->val | imx471->vflip->val << 1, &ret);
>                 cci_write(sensor->regmap, IMX471_REG_H_WIN_OFFSET,
>                           value ? 0xc9 : 0xc8, &ret);
>                 break;
>         case V4L2_CID_VFLIP:
>                 cci_write(imx471->regmap, IMX471_REG_ORIENTATION,
>                           imx471->hflip->val | imx471->vflip->val << 1, &ret);
>                 cci_write(sensor->regmap, IMX471_REG_V_WIN_OFFSET,
>                           value ? 0xe0 : 0xeb, &ret);
>                 break;

That is better. I'll move them to the set_ctrl().

>
> Regards,
>
> Hans
>


-- 
BR,
Kate