Re: [PATCH v2 1/5] media: Add MIPI CCI register access helper functions

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@redhat.com/
> > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > ---
> > > > 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