Re: [PATCH RFC 1/2] media: i2c: ov8865: fix horizontal flip control polarity

[Hans de Goede]

Hi,

On 29-Jul-26 00:30, Sakari Ailus wrote:
> Hi Jacob,
> 
> Thanks for the set.
> 
> On Mon, Jul 20, 2026 at 04:28:40PM +0200, Jakob Berg Jespersen wrote:
>> The ov8865's native readout is horizontally mirrored and the
>> FORMAT2 FLIP_HORZ bits (reg 0x3821) un-mirror it: with the bits
>> cleared the image is mirrored, with them set it is not. The driver
>> maps V4L2_CID_HFLIP=1 to setting the bits, so requesting a flip
>> produces an unflipped image and vice versa.
>>
>> This is user-visible on the Surface Pro 7+ rear camera (mounted with a
>> 180 degree rotation, SSDB degree=180): libcamera requests HFLIP=1+VFLIP=1
>> to undo the mount rotation and gets a horizontally flipped image.
>>
>> Verified by a live 4-state flip/image matrix on the streaming sensor:
>>
>>   hflip=0 vflip=0 -> 180 degree rotation (both flips)
>>   hflip=1 vflip=0 -> vertical flip only
>>   hflip=0 vflip=1 -> correct image
>>   hflip=1 vflip=1 -> horizontal flip only
>>
>> (vertical flip = reflection over a horizontal mirror line, as on water;
>> horizontal flip = reflection over a vertical mirror line, as in a mirror)
>>
>> which is only consistent with an inverted HFLIP and a correct VFLIP.
>>
>> Invert the polarity so HFLIP=0 yields the unflipped image.
> 
> Is this just about, looking at the patch, that the driver before this patch
> effectively implements an inverted HFLIP control? I think the description
> could be simplified.

Ack and even more so for the second patch. The text about
"like the ov8865" really does not belong in the commit message
there.

Regards,

Hans


> 
>>
>> Signed-off-by: Jakob Berg Jespersen <dev@xxxxxxx>
>> ---
>>  drivers/media/i2c/ov8865.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
>> index c6d53c3d55ca..901ea7c16395 100644
>> --- a/drivers/media/i2c/ov8865.c
>> +++ b/drivers/media/i2c/ov8865.c
>> @@ -2204,8 +2204,14 @@ static int ov8865_flip_horz_configure(struct ov8865_sensor *sensor, bool enable)
>>  	u8 bits = OV8865_FORMAT2_FLIP_HORZ_ISP_EN |
>>  		  OV8865_FORMAT2_FLIP_HORZ_SENSOR_EN;
>>  
>> +	/*
>> +	 * The sensor's native readout is horizontally mirrored; the
>> +	 * FLIP_HORZ bits un-mirror it. Map the control so that HFLIP=0
>> +	 * yields an unmirrored image (verified on Surface Pro 7+ rear
>> +	 * camera by a live flip-control/image matrix test).
>> +	 */
> 
> Just a comment saying this is inverted in the sensor considering HFLIP
> should be enough here.
> 
>>  	return ov8865_update_bits(sensor, OV8865_FORMAT2_REG, bits,
>> -				  enable ? bits : 0);
>> +				  enable ? 0 : bits);
>>  }
>>  
>>  /* Test Pattern */
>>
>