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. > > > > - 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? -- Kind regards, Sakari Ailus
