Hi, On 6/15/23 13:54, Tommaso Merciai wrote: > 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 I'm not sure what you mean with "V4L2" registers ? I guess you mean registers which cannot be accessed through the CCI helper functions, but starting with v2 this is no longer true. There now is a CCI_REG64() so you can simply use that. Regards, Hans > > ? > >> >>> >>> 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 >>>> >>> >
