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. -- Kind regards, Sakari Ailus
