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

[Hans de Goede <hdegoede@redhat.com>]

Hi Laurent,

On 6/7/23 20:18, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Tue, Jun 06, 2023 at 06:58:06PM +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.
> 
> I think there are some sensors that also have 64-bit registers, but we
> can deal with that later.
> 
>> 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>
>> ---
>>  Documentation/driver-api/media/v4l2-cci.rst  |   5 +
>>  Documentation/driver-api/media/v4l2-core.rst |   1 +
>>  drivers/media/v4l2-core/Kconfig              |   5 +
>>  drivers/media/v4l2-core/Makefile             |   1 +
>>  drivers/media/v4l2-core/v4l2-cci.c           | 142 +++++++++++++++++++
>>  include/media/v4l2-cci.h                     | 109 ++++++++++++++
>>  6 files changed, 263 insertions(+)
>>  create mode 100644 Documentation/driver-api/media/v4l2-cci.rst
>>  create mode 100644 drivers/media/v4l2-core/v4l2-cci.c
>>  create mode 100644 include/media/v4l2-cci.h
>>
>> diff --git a/Documentation/driver-api/media/v4l2-cci.rst b/Documentation/driver-api/media/v4l2-cci.rst
>> new file mode 100644
>> index 000000000000..dd297a40ed20
>> --- /dev/null
>> +++ b/Documentation/driver-api/media/v4l2-cci.rst
>> @@ -0,0 +1,5 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +V4L2 CCI kAPI
>> +^^^^^^^^^^^^^
>> +.. kernel-doc:: include/media/v4l2-cci.h
>> diff --git a/Documentation/driver-api/media/v4l2-core.rst b/Documentation/driver-api/media/v4l2-core.rst
>> index 1a8c4a5f256b..239045ecc8f4 100644
>> --- a/Documentation/driver-api/media/v4l2-core.rst
>> +++ b/Documentation/driver-api/media/v4l2-core.rst
>> @@ -22,6 +22,7 @@ Video4Linux devices
>>      v4l2-mem2mem
>>      v4l2-async
>>      v4l2-fwnode
>> +    v4l2-cci
>>      v4l2-rect
>>      v4l2-tuner
>>      v4l2-common
>> diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
>> index 348559bc2468..523ba243261d 100644
>> --- a/drivers/media/v4l2-core/Kconfig
>> +++ b/drivers/media/v4l2-core/Kconfig
>> @@ -74,6 +74,11 @@ config V4L2_FWNODE
>>  config V4L2_ASYNC
>>  	tristate
>>  
>> +config V4L2_CCI
>> +	tristate
>> +	depends on I2C
>> +	select REGMAP_I2C
>> +
>>  # Used by drivers that need Videobuf modules
>>  config VIDEOBUF_GEN
>>  	tristate
>> diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
>> index 41d91bd10cf2..be2551705755 100644
>> --- a/drivers/media/v4l2-core/Makefile
>> +++ b/drivers/media/v4l2-core/Makefile
>> @@ -25,6 +25,7 @@ videodev-$(CONFIG_VIDEO_V4L2_I2C) += v4l2-i2c.o
>>  # (e. g. LC_ALL=C sort Makefile)
>>  
>>  obj-$(CONFIG_V4L2_ASYNC) += v4l2-async.o
>> +obj-$(CONFIG_V4L2_CCI) += v4l2-cci.o
>>  obj-$(CONFIG_V4L2_FLASH_LED_CLASS) += v4l2-flash-led-class.o
>>  obj-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
>>  obj-$(CONFIG_V4L2_H264) += v4l2-h264.o
>> diff --git a/drivers/media/v4l2-core/v4l2-cci.c b/drivers/media/v4l2-core/v4l2-cci.c
>> new file mode 100644
>> index 000000000000..21207d137dbe
>> --- /dev/null
>> +++ b/drivers/media/v4l2-core/v4l2-cci.c
>> @@ -0,0 +1,142 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * MIPI Camera Control Interface (CCI) register access helpers.
>> + *
>> + * Copyright (C) 2023 Hans de Goede <hansg@kernel.org>
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/dev_printk.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +
>> +#include <media/v4l2-cci.h>
>> +
>> +int cci_read(struct regmap *map, u32 reg, u32 *val, int *err)
>> +{
>> +	int i, len, ret;
>> +	u8 buf[4];
>> +
>> +	if (err && *err)
>> +		return *err;
>> +
>> +	/* Set len to register width in bytes */
>> +	len = ((reg & CCI_REG_WIDTH_MASK) >> CCI_REG_WIDTH_SHIFT) + 1;
>> +	reg &= CCI_REG_ADDR_MASK;
>> +
>> +	ret = regmap_bulk_read(map, reg, buf, len);
>> +	if (ret) {
>> +		dev_err(regmap_get_device(map), "Error reading reg 0x%4x: %d\n", reg, ret);
>> +		if (err)
>> +			*err = ret;
>> +
>> +		return ret;
>> +	}
>> +
>> +	*val = 0;
>> +	for (i = 0; i < len; i++) {
>> +		*val <<= 8;
>> +		*val |= buf[i];
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(cci_read);
>> +
>> +int cci_write(struct regmap *map, u32 reg, u32 val, int *err)
>> +{
>> +	int i, len, ret;
>> +	u8 buf[4];
>> +
>> +	if (err && *err)
>> +		return *err;
>> +
>> +	/* Set len to register width in bytes */
>> +	len = ((reg & CCI_REG_WIDTH_MASK) >> CCI_REG_WIDTH_SHIFT) + 1;
>> +	reg &= CCI_REG_ADDR_MASK;
>> +
>> +	for (i = 0; i < len; i++) {
>> +		buf[len - i - 1] = val & 0xff;
>> +		val >>= 8;
>> +	}
>> +
>> +	ret = regmap_bulk_write(map, reg, buf, len);
>> +	if (ret) {
>> +		dev_err(regmap_get_device(map), "Error writing reg 0x%4x: %d\n", reg, ret);
>> +		if (err)
>> +			*err = ret;
>> +	}
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(cci_write);
>> +
>> +int cci_update_bits(struct regmap *map, u32 reg, u32 mask, u32 val, int *err)
>> +{
>> +	int width, ret;
>> +	u32 readval;
>> +
>> +	if (err && *err)
>> +		return *err;
>> +
>> +	/*
>> +	 * For single byte updates use regmap_update_bits(), this uses
>> +	 * the regmap-lock to protect against other read-modify-writes racing.
>> +	 */
>> +	width = (reg & CCI_REG_WIDTH_MASK) >> CCI_REG_WIDTH_SHIFT;
>> +	if (width == cci_reg_8) {
>> +		reg &= CCI_REG_ADDR_MASK;
>> +		ret = regmap_update_bits(map, reg, mask, val);
>> +		if (ret) {
>> +			dev_err(regmap_get_device(map), "Error updating reg 0x%4x: %d\n", reg, ret);
>> +			if (err)
>> +				*err = ret;
>> +		}
>> +
>> +		return ret;
>> +	}
>> +
>> +	ret = cci_read(map, reg, &readval, err);
>> +	if (ret)
>> +		return ret;
>> +
>> +	val = (readval & ~mask) | (val & mask);
>> +
>> +	return cci_write(map, reg, val, err);
> 
> Unless I'm mistaken, the regmap cache isn't used. This makes update
> operations fairly costly due to the read. Could that be improved ?

The problem is that some registers may be volatile,
think e.g. expsoure on a sensor where auto-exposure is supported.

So normally drivers which want to use regmap caching, also
provide a whole bunch of tables describing the registers
(lists of volatile + list of writable + list of readable
registers).

So enabling caching is not trivial. I think that it would be best
for drivers which want that to supply their own regmap_config config
and directly call devm_regmap_init_i2c() if they then use
the resulting regmaps with the existing cci_* helpers then caching
will be used automatically.





> 
>> +}
>> +EXPORT_SYMBOL_GPL(cci_update_bits);
>> +
>> +int cci_multi_reg_write(struct regmap *map, const struct reg_sequence *regs, int num_regs, int *err)
>> +{
>> +	int i, ret;
>> +
>> +	if (err && *err)
>> +		return *err;
>> +
>> +	for (i = 0; i < num_regs; i++) {
>> +		ret = cci_write(map, regs[i].reg, regs[i].def, err);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (regs[i].delay_us)
>> +			fsleep(regs[i].delay_us);
> 
> Do you have an immediate need for this ? If not, I'd drop support for
> the delay, and add it later when and if needed. It will be easier to
> discuss the API and use cases with a real user.

This is a 1:1 mirror of regmap_multi_reg_write() note this uses
the existing struct reg_sequence delay_us field and the:

		if (regs[i].delay_us)
			fsleep(regs[i].delay_us);

is copied from the implementation of regmap_multi_reg_write()

> 
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(cci_multi_reg_write);
>> +
>> +struct regmap *cci_regmap_init_i2c(struct i2c_client *client, int reg_addr_bits)
>> +{
>> +	struct regmap_config config = {
>> +		.reg_bits = reg_addr_bits,
>> +		.val_bits = 8,
>> +		.reg_format_endian = REGMAP_ENDIAN_BIG,
>> +	};
>> +
>> +	return devm_regmap_init_i2c(client, &config);
>> +}
>> +EXPORT_SYMBOL_GPL(cci_regmap_init_i2c);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
>> diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h
>> new file mode 100644
>> index 000000000000..69b8a7c4a013
>> --- /dev/null
>> +++ b/include/media/v4l2-cci.h
>> @@ -0,0 +1,109 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * MIPI Camera Control Interface (CCI) register access helpers.
>> + *
>> + * Copyright (C) 2023 Hans de Goede <hansg@kernel.org>
>> + */
>> +#ifndef _V4L2_CCI_H
>> +#define _V4L2_CCI_H
>> +
>> +#include <linux/regmap.h>
>> +#include <linux/types.h>
>> +
>> +/*
>> + * Note cci_reg_8 deliberately is 0, not 1, so that raw
>> + * (not wrapped in a CCI_REG*() macro) register addresses
>> + * do 8 bit wide accesses. This allows unchanged use of register
>> + * initialization lists of raw address, value pairs which only
>> + * do 8 bit width accesses. Which makes porting drivers easier.
> 
> It does, but at the same time, it prevents catching errors caused by
> incorrect register macros. I'm tempted to consider that catching those
> errors is more important.
> 
>> + */
>> +enum cci_reg_type {
>> +	cci_reg_8 = 0,
>> +	cci_reg_16,
>> +	cci_reg_24,
>> +	cci_reg_32,
>> +};
>> +
>> +/*
>> + * Macros to define register address with the register width encoded
>> + * into the higher bits. CCI_REG8() is a no-op so its use is optional.
> 
> Even if it's a no-op I'd prefer making its use mandatory. It makes
> driver code more explicit, and eases catching issues during review.

The problem is that almost all sensor drivers contain long list
of register-address, -val pairs which they send to their own custom
regmap_multi_reg_write()

See e.g. the drivers/media/i2c/imx219.c (to stick with the imx
theme from your imx290 request) this has a lot of quite long
struct imx219_reg arrays with raw initializers.

Often some or all of these registers in such list are
undocumented (if we have access to a datasheet at all),
so we simply don't know the register width.

So arguably adding CCI_REG8(x) around all the addresses
here is wrong, since this suggests we know the register
width.

With the current proposal to have 0 mean both unset and 8bit
width this kinda register lists just work and converting
the driver becomes just a matter of replacing e.g.
imx219_write_regs() with cci_multi_reg_write().

Where as otherwise we would need to add CCI_REG8(x)
around the addresses which:

a) Suggests we actually know the register width which
   we often do not know at all

b) causes a ton of needless churn

so I would very much prefer to keep this as as and
allow unmarked register addresses.

As for the CCI_REG8(x) being useful as an annotation
during review you are of course free to enforce its
use during review. And note that I did use it for
all the OV2680_REG_FOO defines in both ov2680 conversions.

I do agree enforcing its use makes sense for individual
register address defines. The reason to make it optional
and the place where I want it to be optional is for
the array of raw register-addr + initializer-val pairs
case.

>> + */
>> +#define CCI_REG_ADDR_MASK		GENMASK(15, 0)
>> +#define CCI_REG_WIDTH_SHIFT		16
>> +#define CCI_REG_WIDTH_MASK		GENMASK(17, 16)
>> +
>> +#define CCI_REG8(x)			((cci_reg_8 << CCI_REG_WIDTH_SHIFT) | (x))
>> +#define CCI_REG16(x)			((cci_reg_16 << CCI_REG_WIDTH_SHIFT) | (x))
>> +#define CCI_REG24(x)			((cci_reg_24 << CCI_REG_WIDTH_SHIFT) | (x))
>> +#define CCI_REG32(x)			((cci_reg_32 << CCI_REG_WIDTH_SHIFT) | (x))
>> +
>> +/**
>> + * cci_read() - Read a value from a single CCI register
>> + *
>> + * @map: Register map to write to
> 
> s/write to/read from/ ?

Ack, will fix for v2.

>> + * @reg: Register address to write, use CCI_REG#() macros to encode reg width
> 
> Same.

Ack.

>> + * @val: Pointer to store read value
>> + * @err: optional pointer to store errors, if a previous error is set the write will be skipped
> 
> Line wrap ?

Ack.

Regards,

Hans