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

[Kate Hsuan]

Hi Hans and Sakari,

On Wed, Apr 22, 2026 at 4:05 AM Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> wrote:
>
> Hi Hans, Kate,
>
> On Tue, Apr 21, 2026 at 11:47:12AM +0200, Hans de Goede wrote:
> > Hi Sakari, Kate,
> >
> > On 21-Apr-26 11:02, Sakari Ailus wrote:
> > > Hi Hans,
> > >
> > > On Fri, Apr 17, 2026 at 12:16:11PM +0200, Hans de Goede wrote:
> >
> > ...
> >
> > >>> +
> > >>> +         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.
> > >>
> > >>
> > >>> +
> > >>> + 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:
> > >>
> > >>    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.
> > >
> > > The cropping configuration should reflect the values on the sensor's pixel
> > > array and should not be affected by flipping. At least the crop window
> > > needs to be adjusted accordingly by the driver. Is there a need to change
> > > flipping while streaming?
> >
> > Ah, that is a very valid question, no I don't think we do need to
> > set them while streaming.
> >
> > Kate if you cannot get the start_x / start_y coordinate changes
> > when changing flipping to work to get a stable bayer output
> > pattern, then another way to fix this is to only allow changing
> > the flip controls while not streaming and return -EBUSY otherwise.
> >
> > This can then be combined with reporting a flip-ctrl dependend
> > bayer-order so that userspace sees the right bayer-order after
> > flipping is applied as long as userspace reads the subdev format
> > after setting the controls (which libcamera does I believe).
>
> It's indeed currently a bit annoying to implement this. The common raw
> sensor model will make this easier as the driver just indicates the native
> pattern to userspace. I don't have an estimate currently when that set
> would be in so the wait could be very long. Libcamera will need changes,
> too.
>
> >
> > For an example of an imx driver which reports a different
> > bayer order depending in flipping see: imx214.c and
> > the imx214_get_format_code() helper, a call to which should
> > be used to replace any hardcoded mbus-formats in the driver.
>

After many configuration attempts, tweaking the X, Y coordinates can
not get the right Bayer order for the hflip, and also breaks the
sensor's requirement (multiple by 4). However, changing the Bayer
format for each kind of flipping works, and the colour is correct for
every kind of flip, including h/v flipping. The side-effect is that
the user can't flip the image during runtime.

It also worked with libcamera. Qcam shows the right colour when
rotating the image 180 degrees.
I'll continue to work on this approach, :)

> --
> Regards,
>
> Sakari Ailus
>


-- 
BR,
Kate