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

[Sakari Ailus]

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.

-- 
Regards,

Sakari Ailus