Hello On Thu, Jan 26, 2023 at 06:02:07PM +0200, Sakari Ailus wrote: > Hi Dave, > > On Thu, Jan 26, 2023 at 03:48:04PM +0000, Dave Stevenson wrote: > > Hi Jacopo and Sakari > > > > On Thu, 26 Jan 2023 at 14:52, Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> wrote: > > > > > > Hi Jacopo, > > > > > > On Tue, Jan 17, 2023 at 11:06:00AM +0100, Jacopo Mondi wrote: > > > > Currently the imx258 driver requires to have the 'rotation' device node > > > > property specified in DTS with a fixed value of 180 degrees. > > > > > > > > The "rotation" fwnode device property is intended to allow specify the > > > > sensor's physical mounting rotation, so that it can be exposed through > > > > the read-only V4L2_CID_CAMERA_SENSOR_ROTATION control and applications > > > > can decide how to compensate for that. > > > > > > > > The imx258 driver has read-only VFLIP and HFLIP enabled, resulting in > > > > a 180 degrees image rotation being produced by the sensor. But this > > > > doesn't imply that the physical mounting rotation should match the > > > > driver's implementation. > > > > > > > > I took into the series Robert's patch that register device node properties and > > > > on top of that register flips controls, in order to remove the hard requirement > > > > of the 180 degrees rotation property presence. > > > > > > Reconsidering these patches after the flipping vs. rotation discussion, > > > they seem fine. The only thing I'd like to see, after removing the rotation > > > property check, would be to add support for the actual flipping controls. > > > I'm pretty sure they can be found in the same registers as on CCS --- the > > > rest of the registers look very much like that. Would you like to send a > > > patch? :-) > > > > Yes it is register 0x0101, bits 0 (H) & 1 (V). > > > > Do watch out as there are register errors in the driver. Currently > > Y_ADD_STA is set to 0, and Y_ADD_END to 3118, giving 3119 lines total. > > Yes, this is the problem with register list based drivers. Well spotted. > > I remember one driver for a Toshiba sensor using value of 5 for a register > the range of which was 2--10, but only even values were allowed. It worked > nonetheless... oh well. > > I wonder if this sensor would work better with the CCS driver > > > That means that when you initially implement flips the Bayer order > > won't change, but you change the field of view by one line. > > The start and end values also break the requirements listed in the > > datasheets for STA/END values being multiples of X (table 4-2 of the > > datasheet). Correcting that will change the Bayer order when inverted. > > Does that count as a regression to userspace? I hope not. Memory says > > that if you don't correct Y_ADD_END then some of the binned modes > > misbehave. > > Most sensors also require even values for the ?_ADDR_START registers (and > odd for the _ADDR_END registers). Using an invalid value sometimes might > work, too, but only testing will tell. > > > > > I have been through this loop before as Soho Enterprise [1] make an > > IMX258 board for the Pi. I haven't upstreamed the patches [2] though > > (sorry). > > It'd be nice if both worked with the same driver. > There are a lot of interesting changes in here that would be worth upstreaming https://github.com/raspberrypi/linux/commits/rpi-5.15.y/drivers/media/i2c/imx258.c I would prefer if we can get these three easy patches of minr in and then start shoveling the good stuff from the rpi repo ? Otherwise I can plumb flip support in with the current wrong totals which, if I understand you right, doesn't require changing the bayer patter order ? > -- > Kind regards, > > Sakari Ailus
