Hi Hans, On Thu, Jun 15, 2023 at 01:10:40PM +0200, Hans de Goede wrote: > 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. Thanks for the info! I want to ask you your opinion about this: Into alvium driver actually I'm using the following defines manipulations: #define REG_BCRM_REG_ADDR_R REG_BCRM_CCI_16BIT(0x0014) #define REG_BCRM_FEATURE_INQUIRY_R REG_BCRM_V4L2_64BIT(0x0008) #define REG_BCRM_DEVICE_FIRMWARE_VERSION_R REG_BCRM_V4L2_64BIT(0x0010) My plan is to use your cci API for cci register in this way defines became like: #define REG_BCRM_REG_ADDR_R CCI_REG16(0x0014) And leave v4l2 regs are it are right now: #define REG_BCRM_FEATURE_INQUIRY_R REG_BCRM_V4L2_64BIT(0x0008) #define REG_BCRM_DEVICE_FIRMWARE_VERSION_R REG_BCRM_V4L2_64BIT(0x0010) What do you think about? > > Note I'm about to send out a v3 addressing some small > remarks on this v2. I'll Cc you on that. Thanks, in this way I can test that and let you know my feedback. Regards, Tommaso > > 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 > > >
