Hi Dave, On Tue, Jan 14, 2020 at 11:34:46AM +0000, Dave Stevenson wrote: ... > > >> + msleep(IMX219_XCLR_DELAY_MS); > > > > > > I guess 10 ms is ok albeit it'd be nicer if you calculated the required > > > delay instead. > > > > I think this 10 ms delay actually serves two purposes here. It is > > not only the delay after XCLR pin is set high (reset de-asserted), > > but it also lets the camera power supplies voltages to settle after > > regulator_bulk_enable() called few lines above. > > > > So I would make the delay a sum of 1) the value which depends on > > input clock frequency (the driver currently supports 24MHz only) > > and 2) a fixed value of 8 ms or so to account for the power supplies > > settle time. So that the sum would be the same 10 ms for 24MHz input > > clock. > > Does it makes sense? > > Regulator settling times shouldn't really be included here - that > should be taken care of via the regulator framework (in the case of DT > for regulator-fixed you define the startup-delay-us property). > > What level do you end up computing it to? > > This delay covers t4, t5 and t6 from figure 38 on page 77 of the datasheet[1] > t4 is max 200usecs. > t5 is 6ms. > t6 is 32000 cycles of INCK. As you say INCK=24MHz is the only > supported clock frequency at present, so 1.3ms. Minimum clock is 6MHz > which will make this 5.3ms. It's nicer to calculate that still. Someone may well add support for other frequencies and it's easy to miss changing this. > > t6 is in parallel with t5, but it is smaller than t5 even at the > minimum clock frequency. > Yes we can be programming the sensor over I2C after t5 but before t6, > but you'd now want to be computing the number of I2C writes required, > the speed of the I2C bus, and probably a few other parameters to > ensure you don't violate t5. The sensor supports 1MHz CCI if INCK is > >11.4MHz. A quick check says we have around 60 register writes to > initialise the sensor. 38 clocks (4 * (8 data bits + ACK) + start + > end) to write each register, which I make 2.4ms. It is therefore > possible to violate t5. > > Is it worth that level of computation, or do you just take t4+t5 at 6.2ms? I'd just ignore the I²C part and wait for long enough. This is not *that* much time after all. Of course, if someone feels like optimising this, why not, but it looks like something that doesn't really pay off. > > I have been a bit naughty up until now in not setting startup-delay-us > on the regulator definition and relying on this delay instead. The > driver ought to do the right thing though and I'll fix my > configuration. Agreed. -- Sakari Ailus
