On Thu, Jun 15, 2023 at 01:26:25PM +0200, Tommaso Merciai wrote: > 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? Or maybe is worth don't use v4l2 bit for v4l2 regs, I think is implicit that what regs that are not CCI are v4l2, then we return wit the following defines: #define REG_BCRM_REG_ADDR_R CCI_REG16(0x0014) ^CCI regs #define REG_BCRM_FEATURE_INQUIRY_R 0x0008 #define REG_BCRM_DEVICE_FIRMWARE_VERSION_R 0x0010 ^v4l2 regs ? > > > > > 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 > > > > >
