Hi Hans, Laurent, Sakari, Can I cherry-pick this patch and use these new functions also for cci regs of the alvium driver? Are on going to be merge? Let me know. Thanks! :) Regards, Tommaso On Thu, Jun 15, 2023 at 12:21:00PM +0300, Laurent Pinchart wrote: > 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
