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

  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>
---
 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
  

On Tue, Jun 6, 2023 at 7:58 PM Hans de Goede <hdegoede@redhat.com> 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.

...

> +#include <linux/delay.h>
> +#include <linux/dev_printk.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>

+ types.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];
> +       }

I really prefer to see put_unaligned() here depending on the length.
Note, that on some CPUs it might be one assembly instruction or even
none, depending on how the result is going to be used.

> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(cci_read);

Can we have it namespaced?

> +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);

Same comments as per above function.

...

> +               if (regs[i].delay_us)

I'm wondering why fsleep() doesn't have this check? Or does it?

> +                       fsleep(regs[i].delay_us);

...

> +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,

Is the lock required?
If so, how is it helpful?

Can we move this outside as static const?

> +       };
> +
> +       return devm_regmap_init_i2c(client, &config);
> +}

...

> +#ifndef _V4L2_CCI_H
> +#define _V4L2_CCI_H

+ bits.h

> +#include <linux/regmap.h>

Not used, rather requires forward declarations of

struct regmap
struct reg_sequence

Also note missing i2c_client forward declaration.

> +#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.
> + */
> +enum cci_reg_type {
> +       cci_reg_8 = 0,

But this is guaranteed by the C standard... See also below.

> +       cci_reg_16,

But this one becomes 1, so the above comment doesn't clarify why it's
okay to have it 1 and not 2.

> +       cci_reg_24,
> +       cci_reg_32,
> +};
  

Hi Hans,

Thank you for the patchset.

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.
> 
> 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;

Could i and len be unsigned?

> +	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;

Same here.

> +	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);
> +}
> +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);
> +	}
> +
> +	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);

Bulk write functions would be nice, too: CCI does not limit access to
register-like targets.

> +
> +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.
> + */
> +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.
> + */
> +#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))

I'd drop enum ccsi_reg_type and just define the values here using plain
numbers. How you test which width you have changes a little but not
necessarily for worse. Up to you.

> +
> +/**
> + * cci_read() - Read a value from a single CCI register
> + *
> + * @map: Register map to write to
> + * @reg: Register address to write, use CCI_REG#() macros to encode reg width
> + * @val: Pointer to store read value
> + * @err: optional pointer to store errors, if a previous error is set the write will be skipped
> + *
> + * Return: %0 on success or a negative error code on failure.
> + */
> +int cci_read(struct regmap *map, u32 reg, u32 *val, int *err);
> +
> +/**
> + * cci_write() - Write a value to a single CCI register
> + *
> + * @map: Register map to write to
> + * @reg: Register address to write, use CCI_REG#() macros to encode reg width
> + * @val: Value to be written
> + * @err: optional pointer to store errors, if a previous error is set the write will be skipped
> + *
> + * Return: %0 on success or a negative error code on failure.
> + */
> +int cci_write(struct regmap *map, u32 reg, u32 val, int *err);
> +
> +/**
> + * cci_update_bits() - Perform a read/modify/write cycle on a single CCI register
> + *
> + * @map: Register map to write to
> + * @reg: Register address to write, use CCI_REG#() macros to encode reg width
> + * @mask: Bitmask to change
> + * @val: New value for bitmask
> + * @err: optional pointer to store errors, if a previous error is set the update will be skipped
> + *
> + * For 8 bit width registers this is guaranteed to be atomic wrt other
> + * cci_*() register access functions. For multi-byte width registers
> + * atomicity is NOT guaranteed.
> + *
> + * Return: %0 on success or a negative error code on failure.
> + */
> +int cci_update_bits(struct regmap *map, u32 reg, u32 mask, u32 val, int *err);
> +
> +/**
> + * cci_multi_reg_write() - Write multiple registers to the device
> + *
> + * @map: Register map to write to
> + * @regs: Array of structures containing register-address, value pairs to be written
> + *        register-addresses use CCI_REG#() macros to encode reg width
> + * @num_regs: Number of registers to write
> + * @err: optional pointer to store errors, if a previous error is set the update will be skipped
> + *
> + * Write multiple registers to the device where the set of register, value
> + * pairs are supplied in any order, possibly not all in a single range.
> + *
> + * Return: %0 on success or a negative error code on failure.
> + */
> +int cci_multi_reg_write(struct regmap *map, const struct reg_sequence *regs, int num_regs, int *err);
> +
> +/**
> + * cci_regmap_init_i2c() - Create regmap to use with cci_*() register access functions
> + *
> + * @client: i2c_client to create the regmap for
> + * @reg_addr_bits: register address width to use (8 or 16)
> + *
> + * Note the memory for the created regmap is devm() managed, tied to the client.
> + *
> + * Return: %0 on success or a negative error code on failure.
> + */
> +struct regmap *cci_regmap_init_i2c(struct i2c_client *client, int reg_addr_bits);
> +
> +#endif
  

Hi,

On 6/6/23 22:43, Andy Shevchenko wrote:
> On Tue, Jun 6, 2023 at 7:58 PM Hans de Goede <hdegoede@redhat.com> 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.
> 
> ...
> 
>> +#include <linux/delay.h>
>> +#include <linux/dev_printk.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
> 
> + types.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];
>> +       }
> 
> I really prefer to see put_unaligned() here depending on the length.
> Note, that on some CPUs it might be one assembly instruction or even
> none, depending on how the result is going to be used.

Ok, so you mean changing it to something like this:

	switch (len)
	case 1:
		*val = buf[0];
		break;
	case 2:
		*val = get_unaligned_be16(buf);
		break;
	case 3:
		*val = __get_unaligned_be24(buf);
		break;
	case 4:
		*val = get_unaligned_be32(buf);
		break;
	}

?

		

> 
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(cci_read);
> 
> Can we have it namespaced?

I'm not sure if having just these 5 symbols in their own namespace is worth it. SO far the media subsystem is not using module/symbol namespacing at all.

Sakari, Laurent, any opinions on this ?



>> +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);
> 
> Same comments as per above function.
> 
> ...
> 
>> +               if (regs[i].delay_us)
> 
> I'm wondering why fsleep() doesn't have this check? Or does it?
> 
>> +                       fsleep(regs[i].delay_us);
> 
> ...
> 
>> +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,
> 
> Is the lock required?
> If so, how is it helpful?

Interesting questions sensor drivers typically already do
their own locking.

So I guess we could indeed tell regmap to skip locking here.

Sakari, Laurent any opinion on this ?

> Can we move this outside as static const?

No, because reg_bits is not const.



>> +       };
>> +
>> +       return devm_regmap_init_i2c(client, &config);
>> +}
> 
> ...
> 
>> +#ifndef _V4L2_CCI_H
>> +#define _V4L2_CCI_H
> 
> + bits.h
> 
>> +#include <linux/regmap.h>
> 
> Not used, rather requires forward declarations of
> 
> struct regmap
> struct reg_sequence

Ack, I'll change this for the next version.

> Also note missing i2c_client forward declaration.

That was also taken care of by 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.
>> + */
>> +enum cci_reg_type {
>> +       cci_reg_8 = 0,
> 
> But this is guaranteed by the C standard... See also below.
> 
>> +       cci_reg_16,
> 
> But this one becomes 1, so the above comment doesn't clarify why it's
> okay to have it 1 and not 2.

Basically the idea is that the enum value is the reg-width in bytes - 1
where the - 1 is there so that cci_reg_8 = 0 .

Regards,

Hans
  

Hi Sakari,

On 6/7/23 09:50, Sakari Ailus wrote:
> Hi Hans,
> 
> Thank you for the patchset.
> 
> 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.
>>
>> 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;
> 
> Could i and len be unsigned?

Andy suggested replacing the for-loop below with:

	switch (len)
	case 1:
		*val = buf[0];
		break;
	case 2:
		*val = get_unaligned_be16(buf);
		break;
	case 3:
		*val = __get_unaligned_be24(buf);
		break;
	case 4:
		*val = get_unaligned_be32(buf);
		break;
	}

Then i goes away. What do you think about doing it like
this instead ?

> 
>> +	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;
> 
> Same here.
> 
>> +	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);
>> +}
>> +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);
>> +	}
>> +
>> +	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);
> 
> Bulk write functions would be nice, too: CCI does not limit access to
> register-like targets.

For bulk writing encoding the register width into the address
makes no sense, so we would need to specify in the documentation
that only raw register addresses are accepted and that the write
is always done in bytes.

At which point we are basically adding a 1:1 wrapper around
regmap_bulk_write(). So I think it would be better for sensor
drivers which need this to just use regmap_bulk_write()
directly.

Regards,

Hans
  

Hi Hans,

On Wed, Jun 07, 2023 at 10:46:58AM +0200, Hans de Goede wrote:
> Hi Sakari,
> 
> On 6/7/23 09:50, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > Thank you for the patchset.
> > 
> > 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.
> >>
> >> 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;
> > 
> > Could i and len be unsigned?
> 
> Andy suggested replacing the for-loop below with:
> 
> 	switch (len)
> 	case 1:
> 		*val = buf[0];
> 		break;
> 	case 2:
> 		*val = get_unaligned_be16(buf);
> 		break;
> 	case 3:
> 		*val = __get_unaligned_be24(buf);
> 		break;
> 	case 4:
> 		*val = get_unaligned_be32(buf);
> 		break;
> 	}
> 
> Then i goes away. What do you think about doing it like
> this instead ?

I'll reply to discussion there.

> 
> > 
> >> +	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;
> > 
> > Same here.
> > 
> >> +	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);
> >> +}
> >> +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;

	    ^

I should be unsigned here. As well as num_regs.

> >> +
> >> +	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);
> >> +	}
> >> +
> >> +	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);
> > 
> > Bulk write functions would be nice, too: CCI does not limit access to
> > register-like targets.
> 
> For bulk writing encoding the register width into the address
> makes no sense, so we would need to specify in the documentation
> that only raw register addresses are accepted and that the write
> is always done in bytes.
> 
> At which point we are basically adding a 1:1 wrapper around
> regmap_bulk_write(). So I think it would be better for sensor
> drivers which need this to just use regmap_bulk_write()
> directly.

Ah, good point. The first argument is indeed the regmap map.
  

Hi Hans,

On Wed, Jun 07, 2023 at 10:40:34AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 6/6/23 22:43, Andy Shevchenko wrote:
> > On Tue, Jun 6, 2023 at 7:58 PM Hans de Goede <hdegoede@redhat.com> 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.
> > 
> > ...
> > 
> >> +#include <linux/delay.h>
> >> +#include <linux/dev_printk.h>
> >> +#include <linux/module.h>
> >> +#include <linux/regmap.h>
> > 
> > + types.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];
> >> +       }
> > 
> > I really prefer to see put_unaligned() here depending on the length.
> > Note, that on some CPUs it might be one assembly instruction or even
> > none, depending on how the result is going to be used.
> 
> Ok, so you mean changing it to something like this:
> 
> 	switch (len)
> 	case 1:
> 		*val = buf[0];
> 		break;
> 	case 2:
> 		*val = get_unaligned_be16(buf);
> 		break;
> 	case 3:
> 		*val = __get_unaligned_be24(buf);
> 		break;
> 	case 4:
> 		*val = get_unaligned_be32(buf);
> 		break;
> 	}

I think the loop looks nicer but I'm fine with this as well.

> 
> ?
> 
> 		
> 
> > 
> >> +       return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(cci_read);
> > 
> > Can we have it namespaced?
> 
> I'm not sure if having just these 5 symbols in their own namespace is worth it. SO far the media subsystem is not using module/symbol namespacing at all.
> 
> Sakari, Laurent, any opinions on this ?

Regmap nor V4L2 use it so I wouldn't use it here either.

> 
> 
> 
> >> +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);
> > 
> > Same comments as per above function.
> > 
> > ...
> > 
> >> +               if (regs[i].delay_us)
> > 
> > I'm wondering why fsleep() doesn't have this check? Or does it?
> > 
> >> +                       fsleep(regs[i].delay_us);
> > 
> > ...
> > 
> >> +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,
> > 
> > Is the lock required?
> > If so, how is it helpful?
> 
> Interesting questions sensor drivers typically already do
> their own locking.
> 
> So I guess we could indeed tell regmap to skip locking here.
> 
> Sakari, Laurent any opinion on this ?

There are loops here so it won't be atomic in any case.

Generally drivers indeed already take care of this. I don't think we need
locking on this level.

> 
> > Can we move this outside as static const?
> 
> No, because reg_bits is not const.
> 
> 
> 
> >> +       };
> >> +
> >> +       return devm_regmap_init_i2c(client, &config);
> >> +}
> > 
> > ...
> > 
> >> +#ifndef _V4L2_CCI_H
> >> +#define _V4L2_CCI_H
> > 
> > + bits.h
> > 
> >> +#include <linux/regmap.h>
> > 
> > Not used, rather requires forward declarations of
> > 
> > struct regmap
> > struct reg_sequence
> 
> Ack, I'll change this for the next version.
> 
> > Also note missing i2c_client forward declaration.
> 
> That was also taken care of by 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.
> >> + */
> >> +enum cci_reg_type {
> >> +       cci_reg_8 = 0,
> > 
> > But this is guaranteed by the C standard... See also below.
> > 
> >> +       cci_reg_16,
> > 
> > But this one becomes 1, so the above comment doesn't clarify why it's
> > okay to have it 1 and not 2.
> 
> Basically the idea is that the enum value is the reg-width in bytes - 1
> where the - 1 is there so that cci_reg_8 = 0 .

I'm fine with the comment.
  

Hello,

On Wed, Jun 07, 2023 at 12:01:10PM +0000, Sakari Ailus wrote:
> On Wed, Jun 07, 2023 at 10:40:34AM +0200, Hans de Goede wrote:
> > On 6/6/23 22:43, Andy Shevchenko wrote:
> > > On Tue, Jun 6, 2023 at 7:58 PM 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.
> > > 
> > > ...
> > > 
> > >> +#include <linux/delay.h>
> > >> +#include <linux/dev_printk.h>
> > >> +#include <linux/module.h>
> > >> +#include <linux/regmap.h>
> > > 
> > > + types.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];
> > >> +       }
> > > 
> > > I really prefer to see put_unaligned() here depending on the length.
> > > Note, that on some CPUs it might be one assembly instruction or even
> > > none, depending on how the result is going to be used.
> > 
> > Ok, so you mean changing it to something like this:
> > 
> > 	switch (len)
> > 	case 1:
> > 		*val = buf[0];
> > 		break;
> > 	case 2:
> > 		*val = get_unaligned_be16(buf);
> > 		break;
> > 	case 3:
> > 		*val = __get_unaligned_be24(buf);
> > 		break;
> > 	case 4:
> > 		*val = get_unaligned_be32(buf);
> > 		break;
> > 	}
> 
> I think the loop looks nicer but I'm fine with this as well.
> 
> > ?
> > 
> > >> +       return 0;
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(cci_read);
> > > 
> > > Can we have it namespaced?
> > 
> > I'm not sure if having just these 5 symbols in their own namespace
> > is worth it. SO far the media subsystem is not using module/symbol
> > namespacing at all.
> > 
> > Sakari, Laurent, any opinions on this ?
> 
> Regmap nor V4L2 use it so I wouldn't use it here either.

Ditto.

> > >> +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);
> > > 
> > > Same comments as per above function.
> > > 
> > > ...
> > > 
> > >> +               if (regs[i].delay_us)
> > > 
> > > I'm wondering why fsleep() doesn't have this check? Or does it?
> > > 
> > >> +                       fsleep(regs[i].delay_us);
> > > 
> > > ...
> > > 
> > >> +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,
> > > 
> > > Is the lock required?
> > > If so, how is it helpful?
> > 
> > Interesting questions sensor drivers typically already do
> > their own locking.
> > 
> > So I guess we could indeed tell regmap to skip locking here.
> > 
> > Sakari, Laurent any opinion on this ?
> 
> There are loops here so it won't be atomic in any case.
> 
> Generally drivers indeed already take care of this. I don't think we need
> locking on this level.

Agreed.

> > > Can we move this outside as static const?
> > 
> > No, because reg_bits is not const.
> > 
> > >> +       };
> > >> +
> > >> +       return devm_regmap_init_i2c(client, &config);
> > >> +}
> > > 
> > > ...
> > > 
> > >> +#ifndef _V4L2_CCI_H
> > >> +#define _V4L2_CCI_H
> > > 
> > > + bits.h
> > > 
> > >> +#include <linux/regmap.h>
> > > 
> > > Not used, rather requires forward declarations of
> > > 
> > > struct regmap
> > > struct reg_sequence
> > 
> > Ack, I'll change this for the next version.
> > 
> > > Also note missing i2c_client forward declaration.
> > 
> > That was also taken care of by 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.
> > >> + */
> > >> +enum cci_reg_type {
> > >> +       cci_reg_8 = 0,
> > > 
> > > But this is guaranteed by the C standard... See also below.
> > > 
> > >> +       cci_reg_16,
> > > 
> > > But this one becomes 1, so the above comment doesn't clarify why it's
> > > okay to have it 1 and not 2.
> > 
> > Basically the idea is that the enum value is the reg-width in bytes - 1
> > where the - 1 is there so that cci_reg_8 = 0 .
> 
> I'm fine with the comment.
  

On Wed, Jun 7, 2023 at 3:01 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> On Wed, Jun 07, 2023 at 10:40:34AM +0200, Hans de Goede wrote:
> > On 6/6/23 22:43, Andy Shevchenko wrote:
> > > On Tue, Jun 6, 2023 at 7:58 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> > >> +int cci_read(struct regmap *map, u32 reg, u32 *val, int *err)
> > >> +{
> > >> +       int i, len, ret;
> > >> +       u8 buf[4];

> > >> +       *val = 0;
> > >> +       for (i = 0; i < len; i++) {
> > >> +               *val <<= 8;
> > >> +               *val |= buf[i];
> > >> +       }
> > >
> > > I really prefer to see put_unaligned() here depending on the length.
> > > Note, that on some CPUs it might be one assembly instruction or even
> > > none, depending on how the result is going to be used.
> >
> > Ok, so you mean changing it to something like this:
> >
> >       switch (len)
> >       case 1:
> >               *val = buf[0];
> >               break;
> >       case 2:
> >               *val = get_unaligned_be16(buf);
> >               break;
> >       case 3:
> >               *val = __get_unaligned_be24(buf);

__without double underscore prefix

> >               break;
> >       case 4:
> >               *val = get_unaligned_be32(buf);
> >               break;
> >       }
>
> I think the loop looks nicer but I'm fine with this as well.
>
> > ?

But the loop hides what's going on there. And I believe code
generation would be worse with a loop.
Also note, that in case of switch-case we don't write to the pointed
memory several times, which I think is also the win.

> > >> +       return 0;
> > >> +}

...

> > >> +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;
> > >> +       }

Similar way here.

> > >> +
> > >> +       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;
> > >> +}
  

Hi,

On 6/7/23 17:40, Andy Shevchenko wrote:
> On Wed, Jun 7, 2023 at 3:01 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
>> On Wed, Jun 07, 2023 at 10:40:34AM +0200, Hans de Goede wrote:
>>> On 6/6/23 22:43, Andy Shevchenko wrote:
>>>> On Tue, Jun 6, 2023 at 7:58 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> ...
> 
>>>>> +int cci_read(struct regmap *map, u32 reg, u32 *val, int *err)
>>>>> +{
>>>>> +       int i, len, ret;
>>>>> +       u8 buf[4];
> 
>>>>> +       *val = 0;
>>>>> +       for (i = 0; i < len; i++) {
>>>>> +               *val <<= 8;
>>>>> +               *val |= buf[i];
>>>>> +       }
>>>>
>>>> I really prefer to see put_unaligned() here depending on the length.
>>>> Note, that on some CPUs it might be one assembly instruction or even
>>>> none, depending on how the result is going to be used.
>>>
>>> Ok, so you mean changing it to something like this:
>>>
>>>       switch (len)
>>>       case 1:
>>>               *val = buf[0];
>>>               break;
>>>       case 2:
>>>               *val = get_unaligned_be16(buf);
>>>               break;
>>>       case 3:
>>>               *val = __get_unaligned_be24(buf);
> 
> __without double underscore prefix

include/asm-generic/unaligned.h

defines __get_unaligned_be24() and not get_unaligned_be24(), I guess because 24bit is not a standard register width.



> 
>>>               break;
>>>       case 4:
>>>               *val = get_unaligned_be32(buf);
>>>               break;
>>>       }
>>
>> I think the loop looks nicer but I'm fine with this as well.
>>
>>> ?
> 
> But the loop hides what's going on there. And I believe code
> generation would be worse with a loop.
> Also note, that in case of switch-case we don't write to the pointed
> memory several times, which I think is also the win.

I understand, unless someone objects I'll move to the switch-case
approach for v2.

Regards,

Hans



> 
>>>>> +       return 0;
>>>>> +}
> 
> ...
> 
>>>>> +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;
>>>>> +       }
> 
> Similar way here.
> 
>>>>> +
>>>>> +       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;
>>>>> +}
>
  

On Wed, Jun 7, 2023 at 6:58 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 6/7/23 17:40, Andy Shevchenko wrote:
> > On Wed, Jun 7, 2023 at 3:01 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> >> On Wed, Jun 07, 2023 at 10:40:34AM +0200, Hans de Goede wrote:
> >>> On 6/6/23 22:43, Andy Shevchenko wrote:

...

> >>>               *val = __get_unaligned_be24(buf);
> >
> > __without double underscore prefix
>
> include/asm-generic/unaligned.h
>
> defines __get_unaligned_be24() and not get_unaligned_be24(), I guess because 24bit is not a standard register width.

Strange. Do you have some custom patches in the area?

https://elixir.bootlin.com/linux/v6.4-rc5/source/include/asm-generic/unaligned.h#L112
https://elixir.bootlin.com/linux/v6.4-rc5/source/include/asm-generic/unaligned.h#L90
  

Hi,

On 6/7/23 18:14, Andy Shevchenko wrote:
> On Wed, Jun 7, 2023 at 6:58 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 6/7/23 17:40, Andy Shevchenko wrote:
>>> On Wed, Jun 7, 2023 at 3:01 PM Sakari Ailus
>>> <sakari.ailus@linux.intel.com> wrote:
>>>> On Wed, Jun 07, 2023 at 10:40:34AM +0200, Hans de Goede wrote:
>>>>> On 6/6/23 22:43, Andy Shevchenko wrote:
> 
> ...
> 
>>>>>               *val = __get_unaligned_be24(buf);
>>>
>>> __without double underscore prefix
>>
>> include/asm-generic/unaligned.h
>>
>> defines __get_unaligned_be24() and not get_unaligned_be24(), I guess because 24bit is not a standard register width.
> 
> Strange. Do you have some custom patches in the area?
> 
> https://elixir.bootlin.com/linux/v6.4-rc5/source/include/asm-generic/unaligned.h#L112
> https://elixir.bootlin.com/linux/v6.4-rc5/source/include/asm-generic/unaligned.h#L90

No I do not have any custom patches in that area; and the wrapper you
point to is right there...

I somehow missed it, sorry.

So I will drop the __ as requested when adding the switch-case implementation for v2.

Regards,

Hans
  

On Wed, Jun 07, 2023 at 06:20:08PM +0200, Hans de Goede wrote:
> On 6/7/23 18:14, Andy Shevchenko wrote:
> > On Wed, Jun 7, 2023 at 6:58 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >> On 6/7/23 17:40, Andy Shevchenko wrote:
> >>> On Wed, Jun 7, 2023 at 3:01 PM Sakari Ailus
> >>> <sakari.ailus@linux.intel.com> wrote:
> >>>> On Wed, Jun 07, 2023 at 10:40:34AM +0200, Hans de Goede wrote:
> >>>>> On 6/6/23 22:43, Andy Shevchenko wrote:

...

> >>>>>               *val = __get_unaligned_be24(buf);
> >>>
> >>> __without double underscore prefix
> >>
> >> include/asm-generic/unaligned.h
> >>
> >> defines __get_unaligned_be24() and not get_unaligned_be24(), I guess because 24bit is not a standard register width.
> > 
> > Strange. Do you have some custom patches in the area?
> > 
> > https://elixir.bootlin.com/linux/v6.4-rc5/source/include/asm-generic/unaligned.h#L112
> > https://elixir.bootlin.com/linux/v6.4-rc5/source/include/asm-generic/unaligned.h#L90
> 
> No I do not have any custom patches in that area; and the wrapper you
> point to is right there...
> 
> I somehow missed it, sorry.

No problem.

> So I will drop the __ as requested when adding the switch-case implementation for v2.

Thank you!
  

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 ?

> +}
> +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.

> +	}
> +
> +	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.

> + */
> +#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/ ?

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

Same.

> + * @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 ?

> + *
> + * Return: %0 on success or a negative error code on failure.
> + */
> +int cci_read(struct regmap *map, u32 reg, u32 *val, int *err);
> +
> +/**
> + * cci_write() - Write a value to a single CCI register
> + *
> + * @map: Register map to write to
> + * @reg: Register address to write, use CCI_REG#() macros to encode reg width
> + * @val: Value to be written
> + * @err: optional pointer to store errors, if a previous error is set the write will be skipped
> + *
> + * Return: %0 on success or a negative error code on failure.
> + */
> +int cci_write(struct regmap *map, u32 reg, u32 val, int *err);
> +
> +/**
> + * cci_update_bits() - Perform a read/modify/write cycle on a single CCI register
> + *
> + * @map: Register map to write to
> + * @reg: Register address to write, use CCI_REG#() macros to encode reg width
> + * @mask: Bitmask to change
> + * @val: New value for bitmask
> + * @err: optional pointer to store errors, if a previous error is set the update will be skipped
> + *
> + * For 8 bit width registers this is guaranteed to be atomic wrt other
> + * cci_*() register access functions. For multi-byte width registers
> + * atomicity is NOT guaranteed.
> + *
> + * Return: %0 on success or a negative error code on failure.
> + */
> +int cci_update_bits(struct regmap *map, u32 reg, u32 mask, u32 val, int *err);
> +
> +/**
> + * cci_multi_reg_write() - Write multiple registers to the device
> + *
> + * @map: Register map to write to
> + * @regs: Array of structures containing register-address, value pairs to be written
> + *        register-addresses use CCI_REG#() macros to encode reg width
> + * @num_regs: Number of registers to write
> + * @err: optional pointer to store errors, if a previous error is set the update will be skipped
> + *
> + * Write multiple registers to the device where the set of register, value
> + * pairs are supplied in any order, possibly not all in a single range.
> + *
> + * Return: %0 on success or a negative error code on failure.
> + */
> +int cci_multi_reg_write(struct regmap *map, const struct reg_sequence *regs, int num_regs, int *err);
> +
> +/**
> + * cci_regmap_init_i2c() - Create regmap to use with cci_*() register access functions
> + *
> + * @client: i2c_client to create the regmap for
> + * @reg_addr_bits: register address width to use (8 or 16)
> + *
> + * Note the memory for the created regmap is devm() managed, tied to the client.
> + *
> + * Return: %0 on success or a negative error code on failure.
> + */
> +struct regmap *cci_regmap_init_i2c(struct i2c_client *client, int reg_addr_bits);
> +
> +#endif
  

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
  

On Wed, Jun 07, 2023 at 09:01:40PM +0200, Hans de Goede wrote:
> On 6/7/23 20:18, Laurent Pinchart wrote:
> > On Tue, Jun 06, 2023 at 06:58:06PM +0200, Hans de Goede wrote:

...

> >> +		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()

Reading this I'm wondering if we can actually implement a regmap-cci inside
drivers/base/regmap and use it. It might be that this is impossible, but can
save us from repeating existing code I think.
  

Hi Andy,

On 6/7/23 22:07, Andy Shevchenko wrote:
> On Wed, Jun 07, 2023 at 09:01:40PM +0200, Hans de Goede wrote:
>> On 6/7/23 20:18, Laurent Pinchart wrote:
>>> On Tue, Jun 06, 2023 at 06:58:06PM +0200, Hans de Goede wrote:
> 
> ...
> 
>>>> +		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()
> 
> Reading this I'm wondering if we can actually implement a regmap-cci inside
> drivers/base/regmap and use it. It might be that this is impossible, but can
> save us from repeating existing code I think.

Someone (you I believe?) already suggested this when discussing replacing
the ov_16bit_addr_reg_helpers.h . This is not possible because regmap
assumes a fixed register width for the device and this assumption is
all over the place in regmap.

The whole purpose of the CCI helpers is to provide a thin layer on top
to deal with different register widths while delegating everything else
to regmap.

Regards,

Hans
  

Hi Andy,

On Wed, Jun 07, 2023 at 11:07:28PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 07, 2023 at 09:01:40PM +0200, Hans de Goede wrote:
> > On 6/7/23 20:18, Laurent Pinchart wrote:
> > > On Tue, Jun 06, 2023 at 06:58:06PM +0200, Hans de Goede wrote:
> 
> ...
> 
> > >> +		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()
> 
> Reading this I'm wondering if we can actually implement a regmap-cci inside
> drivers/base/regmap and use it. It might be that this is impossible, but can
> save us from repeating existing code I think.

I very much prefer this set over trying to bury equivalent functionality
in regmap. CCI is quite unlike what regmap is intended for, due to
its variable width registers and that CCI isn't a bus itself (but on top of
the bus specification itself).
  

On Thu, Jun 8, 2023 at 11:33 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> On Wed, Jun 07, 2023 at 11:07:28PM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 07, 2023 at 09:01:40PM +0200, Hans de Goede wrote:
> > > On 6/7/23 20:18, Laurent Pinchart wrote:
> > > > On Tue, Jun 06, 2023 at 06:58:06PM +0200, Hans de Goede wrote:

...

> > >             if (regs[i].delay_us)
> > >                     fsleep(regs[i].delay_us);
> > >
> > > is copied from the implementation of regmap_multi_reg_write()
> >
> > Reading this I'm wondering if we can actually implement a regmap-cci inside
> > drivers/base/regmap and use it. It might be that this is impossible, but can
> > save us from repeating existing code I think.
>
> I very much prefer this set over trying to bury equivalent functionality
> in regmap. CCI is quite unlike what regmap is intended for, due to
> its variable width registers and that CCI isn't a bus itself (but on top of
> the bus specification itself).

Oh, okay, no objections!
  

Hi Hans,

On Wed, Jun 07, 2023 at 09:01:40PM +0200, Hans de Goede wrote:
> On 6/7/23 20:18, Laurent Pinchart wrote:
> > 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.

Would there be a way to use the cache for update operations (as I think
we can consider that registers used in those operations won't be
volatile), and bypass it for standalone reads ?

> >> +}
> >> +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()

The reason why I don't like it much as that such delays are often hacks
hidden in the middle of register arrays that should in many cases be
handled differently. I was hoping that, by not supporting them yet,
we'll have an easier time to get drivers right. Maybe I'm wrong.

> >> +	}
> >> +
> >> +	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.

For register arrays, I'm fine with that. For register macros, I don't
want to see

#define MY_WELL_DEFINED_8B_REG		0x1234

For those I want drivers to use CCI_REG8(). It seems we're on the same
page :-)

> >> + */
> >> +#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.
  

Hi Laurent,

On Thu, Jun 08, 2023 at 01:27:25PM +0300, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Wed, Jun 07, 2023 at 09:01:40PM +0200, Hans de Goede wrote:
> > On 6/7/23 20:18, Laurent Pinchart wrote:
> > > 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.
> 
> Would there be a way to use the cache for update operations (as I think
> we can consider that registers used in those operations won't be
> volatile), and bypass it for standalone reads ?

Could we rely on regmap on this? It provides a way to tell which registers
are volatile. Very few of these drivers would get any benefit from caching
anyway (or even use update_bits()).

> 
> > >> +}
> > >> +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()
> 
> The reason why I don't like it much as that such delays are often hacks
> hidden in the middle of register arrays that should in many cases be
> handled differently. I was hoping that, by not supporting them yet,
> we'll have an easier time to get drivers right. Maybe I'm wrong.

It's not uncommon for a sensor to require e.g. a given amount of time to
recover from software reset. Then again, embedding software in a register
list can hardly be described as a good practice. There maybe other such
cases, too.

But the field already exists in the struct. I don't object acting based on
its contents as such.

> 
> > >> +	}
> > >> +
> > >> +	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.
> 
> For register arrays, I'm fine with that. For register macros, I don't
> want to see
> 
> #define MY_WELL_DEFINED_8B_REG		0x1234
> 
> For those I want drivers to use CCI_REG8(). It seems we're on the same
> page :-)

For a register list based sensor driver, I don't really mind even missing
this in the register lists. Using that macro doesn't help when the problem
really is a very long list of random-looking numbers.

Better drivers should of course use the macro.
  

Hi Laurent,

On 6/8/23 12:27, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Wed, Jun 07, 2023 at 09:01:40PM +0200, Hans de Goede wrote:
>> On 6/7/23 20:18, Laurent Pinchart wrote:

<snip>

>>>> +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.
> 
> Would there be a way to use the cache for update operations (as I think
> we can consider that registers used in those operations won't be
> volatile), and bypass it for standalone reads ?

There is not really a nice way to only use the cache for update operations.

I guess we could do something hacky like tell regmap to create a cache,
then set the cache-bypass flag and drop the cache-bypass flag during
update ops. But that really is abusing the regmap API.

Generally speaking update operations don't happen that often though,
so IMHO hacking to get this cached is not worth it.

> 
>>>> +}
>>>> +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()
> 
> The reason why I don't like it much as that such delays are often hacks
> hidden in the middle of register arrays that should in many cases be
> handled differently. I was hoping that, by not supporting them yet,
> we'll have an easier time to get drivers right. Maybe I'm wrong.

I understand, but having this is more or less a downside of
the choice to mirror the regmap API as close as possible.

As Sakari said, having the field there just to ignore it
seems like a bad idea.

I think that this being abused is something to watch for during
review, rather then enforcing it not being used in the CCI code.

> 
>>>> +	}
>>>> +
>>>> +	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.
> 
> For register arrays, I'm fine with that. For register macros, I don't
> want to see
> 
> #define MY_WELL_DEFINED_8B_REG		0x1234
> 
> For those I want drivers to use CCI_REG8(). It seems we're on the same
> page :-)

Right, but if we want cci_multi_reg_write() to work with register
arrays without the CCI_REG8() macros then the code needs to stay
as is and we cannot enforce use of the macro by erroring out
if it is not used.

Regards,

Hans
  

Hi Sakari,

On Thu, Jun 08, 2023 at 11:01:29AM +0000, Sakari Ailus wrote:
> On Thu, Jun 08, 2023 at 01:27:25PM +0300, Laurent Pinchart wrote:
> > On Wed, Jun 07, 2023 at 09:01:40PM +0200, Hans de Goede wrote:
> > > On 6/7/23 20:18, Laurent Pinchart wrote:
> > > > 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.
> > 
> > Would there be a way to use the cache for update operations (as I think
> > we can consider that registers used in those operations won't be
> > volatile), and bypass it for standalone reads ?
> 
> Could we rely on regmap on this? It provides a way to tell which registers
> are volatile. Very few of these drivers would get any benefit from caching
> anyway (or even use update_bits()).

Yes we could I suppose. I've never been a big fan of that part of the
regmap API as it's cumbersome to use when dealing with devices that have
lots of registers, like camera sensors usually do. Volatile registers
tend to be scattered around the address space, making the volatile_table
lookup inefficient, and the volatile_reg function wouldn't be great
either. I could live with that I suppose, but given that I think we can
expect read-modify-write operations to never operate on a volatile
register, and plain read operations to nearly always do (with the
exception of read-only version registers that are read once at probe
time only), it would be a bit of a shame to rely on the less efficient
regmap volatile support.

> > > >> +}
> > > >> +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()
> > 
> > The reason why I don't like it much as that such delays are often hacks
> > hidden in the middle of register arrays that should in many cases be
> > handled differently. I was hoping that, by not supporting them yet,
> > we'll have an easier time to get drivers right. Maybe I'm wrong.
> 
> It's not uncommon for a sensor to require e.g. a given amount of time to
> recover from software reset. Then again, embedding software in a register
> list can hardly be described as a good practice. There maybe other such
> cases, too.

That's exactly my concern, I'd like to avoid giving an easy option for
people to embed reset actions in a large registers table :-) I don't
object to the feature if we find valid use cases for it.

> But the field already exists in the struct. I don't object acting based on
> its contents as such.
> 
> > > >> +	}
> > > >> +
> > > >> +	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.
> > 
> > For register arrays, I'm fine with that. For register macros, I don't
> > want to see
> > 
> > #define MY_WELL_DEFINED_8B_REG		0x1234
> > 
> > For those I want drivers to use CCI_REG8(). It seems we're on the same
> > page :-)
> 
> For a register list based sensor driver, I don't really mind even missing
> this in the register lists. Using that macro doesn't help when the problem
> really is a very long list of random-looking numbers.

I think we agree here, the place where I want to see the macros being
used without exceptions is when accessing individual registers.

> Better drivers should of course use the macro.
  

Hi Hans,

On Mon, Jun 12, 2023 at 03:48:01PM +0200, Hans de Goede wrote:
> On 6/8/23 12:27, Laurent Pinchart wrote:
> > On Wed, Jun 07, 2023 at 09:01:40PM +0200, Hans de Goede wrote:
> >> On 6/7/23 20:18, Laurent Pinchart wrote:
> 
> <snip>
> 
> >>>> +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.
> > 
> > Would there be a way to use the cache for update operations (as I think
> > we can consider that registers used in those operations won't be
> > volatile), and bypass it for standalone reads ?
> 
> There is not really a nice way to only use the cache for update operations.
> 
> I guess we could do something hacky like tell regmap to create a cache,
> then set the cache-bypass flag and drop the cache-bypass flag during
> update ops. But that really is abusing the regmap API.
> 
> Generally speaking update operations don't happen that often though,
> so IMHO hacking to get this cached is not worth it.

I2C reads are slow, so even if they're not very common, it would be nice
to avoid them. We can start without any caching and improve it later,
I'm fine with that.

> >>>> +}
> >>>> +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()
> > 
> > The reason why I don't like it much as that such delays are often hacks
> > hidden in the middle of register arrays that should in many cases be
> > handled differently. I was hoping that, by not supporting them yet,
> > we'll have an easier time to get drivers right. Maybe I'm wrong.
> 
> I understand, but having this is more or less a downside of
> the choice to mirror the regmap API as close as possible.
> 
> As Sakari said, having the field there just to ignore it
> seems like a bad idea.

I'm not sure to agree here, if we see no valid use case for that field,
why would it be a bad idea to ignore it ? That shouldn't affect anyone.

> I think that this being abused is something to watch for during
> review, rather then enforcing it not being used in the CCI code.
>
> >>>> +	}
> >>>> +
> >>>> +	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.
> > 
> > For register arrays, I'm fine with that. For register macros, I don't
> > want to see
> > 
> > #define MY_WELL_DEFINED_8B_REG		0x1234
> > 
> > For those I want drivers to use CCI_REG8(). It seems we're on the same
> > page :-)
> 
> Right, but if we want cci_multi_reg_write() to work with register
> arrays without the CCI_REG8() macros then the code needs to stay
> as is and we cannot enforce use of the macro by erroring out
> if it is not used.

Could we have an internal __cci_reg_write() that doesn't check the size,
and a cci_reg_write() wrapper that does ?
  

Hi,

On 6/12/23 17:16, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Mon, Jun 12, 2023 at 03:48:01PM +0200, Hans de Goede wrote:
>> On 6/8/23 12:27, Laurent Pinchart wrote:
>>> On Wed, Jun 07, 2023 at 09:01:40PM +0200, Hans de Goede wrote:
>>>> On 6/7/23 20:18, Laurent Pinchart wrote:
>>
>> <snip>
>>
>>>>>> +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.
>>>
>>> Would there be a way to use the cache for update operations (as I think
>>> we can consider that registers used in those operations won't be
>>> volatile), and bypass it for standalone reads ?
>>
>> There is not really a nice way to only use the cache for update operations.
>>
>> I guess we could do something hacky like tell regmap to create a cache,
>> then set the cache-bypass flag and drop the cache-bypass flag during
>> update ops. But that really is abusing the regmap API.
>>
>> Generally speaking update operations don't happen that often though,
>> so IMHO hacking to get this cached is not worth it.
> 
> I2C reads are slow, so even if they're not very common, it would be nice
> to avoid them. We can start without any caching and improve it later,
> I'm fine with that.
> 
>>>>>> +}
>>>>>> +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()
>>>
>>> The reason why I don't like it much as that such delays are often hacks
>>> hidden in the middle of register arrays that should in many cases be
>>> handled differently. I was hoping that, by not supporting them yet,
>>> we'll have an easier time to get drivers right. Maybe I'm wrong.
>>
>> I understand, but having this is more or less a downside of
>> the choice to mirror the regmap API as close as possible.
>>
>> As Sakari said, having the field there just to ignore it
>> seems like a bad idea.
> 
> I'm not sure to agree here, if we see no valid use case for that field,
> why would it be a bad idea to ignore it ? That shouldn't affect anyone.

I'm fine with dropping the fsleep() here, but IMHO it should
be replaced with a warning if it is missing do we want dev_warn
or WARN_ON() here ?  Since setting it would be considered
a driver bug I guess WARN_ON() ?

> 
>> I think that this being abused is something to watch for during
>> review, rather then enforcing it not being used in the CCI code.
>>
>>>>>> +	}
>>>>>> +
>>>>>> +	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.
>>>
>>> For register arrays, I'm fine with that. For register macros, I don't
>>> want to see
>>>
>>> #define MY_WELL_DEFINED_8B_REG		0x1234
>>>
>>> For those I want drivers to use CCI_REG8(). It seems we're on the same
>>> page :-)
>>
>> Right, but if we want cci_multi_reg_write() to work with register
>> arrays without the CCI_REG8() macros then the code needs to stay
>> as is and we cannot enforce use of the macro by erroring out
>> if it is not used.
> 
> Could we have an internal __cci_reg_write() that doesn't check the size,
> and a cci_reg_write() wrapper that does ?

And then make cci_multi_reg_write() use the non checking version I presume?
Yes that is possible / should work.

Or alternatively we could make drivers which use raw register init lists
(with the reg-width macros) directly call regmap_multi_reg_write() since
all register writes are expected to be 8 bit wide in that case directly
calling regmap_multi_reg_write() should work fine too.

And then we can have all the cci_ prefixed versions always check
that a reg-width has been specified, which would be more consistent.

Regards,

Hans
  

Hi Hans,

On Mon, Jun 12, 2023 at 05:33:40PM +0200, Hans de Goede wrote:
> On 6/12/23 17:16, Laurent Pinchart wrote:
> > On Mon, Jun 12, 2023 at 03:48:01PM +0200, Hans de Goede wrote:
> >> On 6/8/23 12:27, Laurent Pinchart wrote:
> >>> On Wed, Jun 07, 2023 at 09:01:40PM +0200, Hans de Goede wrote:
> >>>> On 6/7/23 20:18, Laurent Pinchart wrote:
> >>
> >> <snip>
> >>
> >>>>>> +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.
> >>>
> >>> Would there be a way to use the cache for update operations (as I think
> >>> we can consider that registers used in those operations won't be
> >>> volatile), and bypass it for standalone reads ?
> >>
> >> There is not really a nice way to only use the cache for update operations.
> >>
> >> I guess we could do something hacky like tell regmap to create a cache,
> >> then set the cache-bypass flag and drop the cache-bypass flag during
> >> update ops. But that really is abusing the regmap API.
> >>
> >> Generally speaking update operations don't happen that often though,
> >> so IMHO hacking to get this cached is not worth it.
> > 
> > I2C reads are slow, so even if they're not very common, it would be nice
> > to avoid them. We can start without any caching and improve it later,
> > I'm fine with that.
> > 
> >>>>>> +}
> >>>>>> +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()
> >>>
> >>> The reason why I don't like it much as that such delays are often hacks
> >>> hidden in the middle of register arrays that should in many cases be
> >>> handled differently. I was hoping that, by not supporting them yet,
> >>> we'll have an easier time to get drivers right. Maybe I'm wrong.
> >>
> >> I understand, but having this is more or less a downside of
> >> the choice to mirror the regmap API as close as possible.
> >>
> >> As Sakari said, having the field there just to ignore it
> >> seems like a bad idea.
> > 
> > I'm not sure to agree here, if we see no valid use case for that field,
> > why would it be a bad idea to ignore it ? That shouldn't affect anyone.
> 
> I'm fine with dropping the fsleep() here, but IMHO it should
> be replaced with a warning if it is missing do we want dev_warn
> or WARN_ON() here ?  Since setting it would be considered
> a driver bug I guess WARN_ON() ?

Sounds good to me.

> >> I think that this being abused is something to watch for during
> >> review, rather then enforcing it not being used in the CCI code.
> >>
> >>>>>> +	}
> >>>>>> +
> >>>>>> +	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.
> >>>
> >>> For register arrays, I'm fine with that. For register macros, I don't
> >>> want to see
> >>>
> >>> #define MY_WELL_DEFINED_8B_REG		0x1234
> >>>
> >>> For those I want drivers to use CCI_REG8(). It seems we're on the same
> >>> page :-)
> >>
> >> Right, but if we want cci_multi_reg_write() to work with register
> >> arrays without the CCI_REG8() macros then the code needs to stay
> >> as is and we cannot enforce use of the macro by erroring out
> >> if it is not used.
> > 
> > Could we have an internal __cci_reg_write() that doesn't check the size,
> > and a cci_reg_write() wrapper that does ?
> 
> And then make cci_multi_reg_write() use the non checking version I presume?
> Yes that is possible / should work.
> 
> Or alternatively we could make drivers which use raw register init lists
> (with the reg-width macros) directly call regmap_multi_reg_write() since
> all register writes are expected to be 8 bit wide in that case directly
> calling regmap_multi_reg_write() should work fine too.
> 
> And then we can have all the cci_ prefixed versions always check
> that a reg-width has been specified, which would be more consistent.

Sounds good to me too :-)
  

Hi Laurent,

On Mon, Jun 12, 2023 at 06:03:15PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Thu, Jun 08, 2023 at 11:01:29AM +0000, Sakari Ailus wrote:
> > On Thu, Jun 08, 2023 at 01:27:25PM +0300, Laurent Pinchart wrote:
> > > On Wed, Jun 07, 2023 at 09:01:40PM +0200, Hans de Goede wrote:
> > > > On 6/7/23 20:18, Laurent Pinchart wrote:
> > > > > 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.
> > > 
> > > Would there be a way to use the cache for update operations (as I think
> > > we can consider that registers used in those operations won't be
> > > volatile), and bypass it for standalone reads ?
> > 
> > Could we rely on regmap on this? It provides a way to tell which registers
> > are volatile. Very few of these drivers would get any benefit from caching
> > anyway (or even use update_bits()).
> 
> Yes we could I suppose. I've never been a big fan of that part of the
> regmap API as it's cumbersome to use when dealing with devices that have
> lots of registers, like camera sensors usually do. Volatile registers
> tend to be scattered around the address space, making the volatile_table
> lookup inefficient, and the volatile_reg function wouldn't be great
> either. I could live with that I suppose, but given that I think we can

You can still have a switch there. And for larger registers all octets need
to be included.

> expect read-modify-write operations to never operate on a volatile
> register, and plain read operations to nearly always do (with the
> exception of read-only version registers that are read once at probe
> time only), it would be a bit of a shame to rely on the less efficient
> regmap volatile support.

The driver would need to enable register cache and set up the volatile
registers. It is nearly always easier to just write the full value of that
register instead. So almost always if someone feels they need this, they're
doing something wrong. That's why I'm not too concerned that this is
difficult to set up.

> 
> > > > >> +}
> > > > >> +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()
> > > 
> > > The reason why I don't like it much as that such delays are often hacks
> > > hidden in the middle of register arrays that should in many cases be
> > > handled differently. I was hoping that, by not supporting them yet,
> > > we'll have an easier time to get drivers right. Maybe I'm wrong.
> > 
> > It's not uncommon for a sensor to require e.g. a given amount of time to
> > recover from software reset. Then again, embedding software in a register
> > list can hardly be described as a good practice. There maybe other such
> > cases, too.
> 
> That's exactly my concern, I'd like to avoid giving an easy option for
> people to embed reset actions in a large registers table :-) I don't
> object to the feature if we find valid use cases for it.
> 
> > But the field already exists in the struct. I don't object acting based on
> > its contents as such.
> > 
> > > > >> +	}
> > > > >> +
> > > > >> +	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.
> > > 
> > > For register arrays, I'm fine with that. For register macros, I don't
> > > want to see
> > > 
> > > #define MY_WELL_DEFINED_8B_REG		0x1234
> > > 
> > > For those I want drivers to use CCI_REG8(). It seems we're on the same
> > > page :-)
> > 
> > For a register list based sensor driver, I don't really mind even missing
> > this in the register lists. Using that macro doesn't help when the problem
> > really is a very long list of random-looking numbers.
> 
> I think we agree here, the place where I want to see the macros being
> used without exceptions is when accessing individual registers.

I believe we do, yes.