Hello, On Wed, Jun 14, 2023 at 09:48:00PM +0000, Sakari Ailus wrote: > On Thu, Jun 15, 2023 at 12:34:29AM +0300, Laurent Pinchart wrote: > > 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. > > > > > > > - 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 ? > > Most of the registers are 8-bit, 64-bit ones are exceedingly rare and will > probably remain so for CCI (small register space and slow bus!). Maybe the > 64-bit support could be separate from the rest, using C11 _Generic() as > suggested above? For the read and write functions, that's an interesting idea, but I'm not sure if it will be a useful optimization. I suppose we could try and see. For register sequences, I think it would become cumbersome and error prone to have to call different functions because a 64-bit register is present in the list. I wonder if we could somehow pack the array instead, turning the array of cci_reg_sequence into a u8 array, with 32-bit register addresses followed by a number of bytes corresponding to the register width. It should be doable with macros I think. I'm not asking for this to be implemented right now, but I agree that it's likely best to do so earlier than later. -- Regards, Laurent Pinchart
