Re: [PATCH v2 2/2] media: i2c: Add driver for Sony IMX219 sensor

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux