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

[Hans de Goede <johannes.goede@oss.qualcomm.com>]

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).

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,

Hans