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@xxxxxxxxxx/ > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > --- > 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@xxxxxxxxxx> > + */ > + > +#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@xxxxxxxxxx>"); > 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@xxxxxxxxxx> > + */ > +#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 -- Kind regards, Sakari Ailus
