On Thu, Jun 15, 2023 at 11:11:20AM +0200, Hans de Goede wrote: > Hi Sakari, > > On 6/14/23 23:48, Sakari Ailus wrote: > > Hi Laurent, > > > > On Thu, Jun 15, 2023 at 12:34:29AM +0300, Laurent Pinchart wrote: > >> Hello, > >> > >> On Wed, Jun 14, 2023 at 08:39:56PM +0000, Sakari Ailus wrote: > >>> On Wed, Jun 14, 2023 at 09:23:39PM +0200, Hans de Goede wrote: > >>>> The CSI2 specification specifies a standard method to access camera sensor > >>>> registers called "Camera Control Interface (CCI)". > >>>> > >>>> This uses either 8 or 16 bit (big-endian wire order) register addresses > >>>> and supports 8, 16, 24 or 32 bit (big-endian wire order) register widths. > >>>> > >>>> Currently a lot of Linux camera sensor drivers all have their own custom > >>>> helpers for this, often copy and pasted from other drivers. > >>>> > >>>> Add a set of generic helpers for this so that all sensor drivers can > >>>> switch to a single common implementation. > >>>> > >>>> These helpers take an extra optional "int *err" function parameter, > >>>> this can be used to chain a bunch of register accesses together with > >>>> only a single error check at the end, rather then needing to error > >>>> check each individual register access. The first failing call will > >>>> set the contents of err to a non 0 value and all other calls will > >>>> then become no-ops. > >>>> > >>>> Link: https://lore.kernel.org/linux-media/59aefa7f-7bf9-6736-6040-39551329cd0a@xxxxxxxxxx/ > >>>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > >>>> --- > >>>> Changes in v2: > >>>> - Drop cci_reg_type enum > >>>> - Make having an encoded reg-width mandatory rather then using 0 to encode > >>>> 8 bit width making reg-addresses without an encoded width default to > >>>> a width of 8 > >>>> - Add support for 64 bit wide registers > >> > >> I'm in two minds about this. This means that the read and write > >> functions take a u64 argument, which will be less efficient on 32-bit > >> platforms. I think it would be possible, with some macro magic, to > >> accept different argument sizes, but maybe that's a micro-optimization > >> that would actually result in worse code. > >> > >> 64-bit support could be useful, but as far as I can tell, it's not used > >> in this series, so maybe we could leave this for later ? > > > > I prefer to have it now, I just told Tommaso working on the Alvium driver > > to use this, and he needs 64-bit access. :-) > > > > You could also easily have 32-bit and 64-bit variant of the functions, with > > C11 _Generic(). Introducing it now would be easier than later. > > I took a quick look at C11 _Generic() and that looks at the type > of "things" so in this case it would look at type of the val argument. > > Problem is that that can still be e.g. an int when doing a > read/write from a 64 bit registers. > > So we would then need to handle the 64 bit width case in the 32 > bit versions of the functions too. > > And likewise I can see someone passing a long on a 64 bit > arch while doing a cci_write() to a non 64 bit register. > > So this would basically mean copy and pasting cci_read() > + cci_write() 2x with the only difference being one > variant taking a 32 bit val argument and the other a > 64 bit val argument. > > This seems like premature optimization to me. > > As mentioned in my reply to Laurent if we want to > optimize things we really should look at avoiding > unnecessary i2c transfers, or packing multiple > writes into a single i2c transfer for writes to > subsequent registers. That is where significant > speedups can be made. This is something I'd really like to see, but it's way more work. There's an important need of applying changes atomically, which is often not possible to strictly guarantee over I2C. Userspace ends up writing V4L2 controls as quickly as it can after the start of a frame, hoping they will all reach the sensor before the end of the frame. Some platforms have camera-specific I2C controllers that have the ability to buffer I2C transfers and issue them based on a hardware trigger. How to fit this in thé kernel I2C API will be an interesting exercise. -- Regards, Laurent Pinchart
