Hi Tommaso, On 6/15/23 12:05, Tommaso Merciai wrote: > Hi Hans, Laurent, Sakari, > > Can I cherry-pick this patch and use these new functions also > for cci regs of the alvium driver? Yes that sounds like a good plan. > Are on going to be merge? Yes this will hopefully get merged upstream soon. Note I'm about to send out a v3 addressing some small remarks on this v2. I'll Cc you on that. Regards, Hans > > 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 >
