Hi, On 6/14/23 23:34, 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 ? Besides Sakari mentioning a sensor driver being in the works which needs this. Adding 64 bit support later is troublesome because it will change the prototype of cci_read, specically the return by reference val will change from u32 *val to u64 *val, requiring changing all the callers. So if anything now is the time to change this. As for this being slower on 32 bit archs. We are talking about code here which is ultimately transferring data over 400 KHz i2c. The i2c is always going to be the bottleneck and if we want / need to optimize this we really need to focus on reducing unnecessary i2c transfers first. >>> - Introduce a new cci_reg_sequence struct with 64 bit reg values for 64 bit >>> support and without the delay_us field > > This consumes extra memory too. Is it an issue ? Compared to v1 this does not consume extra memory since this drops the delay_us field. It does consume extra memory to still adding a new cci_reg_sequence struct with 2 32 bit members. But given that any hw using sensor drivers is going to need to have multiple buffers to store the sensor data with each buffer consuming megabytes of RAM I'm not really worried about growing the size of a couple of fixed register init lists by a bit. As for your other minor review comments I agree and I'll fix them all for the next version. Regards, Hans
