Hello, On Wed, Jun 14, 2023 at 08:39:56PM +0000, Sakari Ailus wrote: > On Wed, Jun 14, 2023 at 09:23:39PM +0200, Hans de Goede wrote: > > The CSI2 specification specifies a standard method to access camera sensor > > registers called "Camera Control Interface (CCI)". > > > > This uses either 8 or 16 bit (big-endian wire order) register addresses > > and supports 8, 16, 24 or 32 bit (big-endian wire order) register widths. > > > > Currently a lot of Linux camera sensor drivers all have their own custom > > helpers for this, often copy and pasted from other drivers. > > > > Add a set of generic helpers for this so that all sensor drivers can > > switch to a single common implementation. > > > > These helpers take an extra optional "int *err" function parameter, > > this can be used to chain a bunch of register accesses together with > > only a single error check at the end, rather then needing to error > > check each individual register access. The first failing call will > > set the contents of err to a non 0 value and all other calls will > > then become no-ops. > > > > Link: https://lore.kernel.org/linux-media/59aefa7f-7bf9-6736-6040-39551329cd0a@xxxxxxxxxx/ > > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > > --- > > Changes in v2: > > - Drop cci_reg_type enum > > - Make having an encoded reg-width mandatory rather then using 0 to encode > > 8 bit width making reg-addresses without an encoded width default to > > a width of 8 > > - Add support for 64 bit wide registers I'm in two minds about this. This means that the read and write functions take a u64 argument, which will be less efficient on 32-bit platforms. I think it would be possible, with some macro magic, to accept different argument sizes, but maybe that's a micro-optimization that would actually result in worse code. 64-bit support could be useful, but as far as I can tell, it's not used in this series, so maybe we could leave this for later ? > > - Introduce a new cci_reg_sequence struct with 64 bit reg values for 64 bit > > support and without the delay_us field This consumes extra memory too. Is it an issue ? > > - Various kerneldoc updates > > - Stop supporting delays in cci_multi_reg_write() > > - Some includes cleanups > > - Disable regmap locking > > --- > > 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 | 157 +++++++++++++++++++ > > include/media/v4l2-cci.h | 121 ++++++++++++++ > > 6 files changed, 290 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 > > This won't do anything if the dependent driver will select V4L2_CCI, will > it? I'd let the sensor driver depend on I2C instead. CCI is also supported > on I3C, for instance. > > > + select REGMAP_I2C > > This is a good question. > > How about adding V4L2_CCI_I2C that would 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..94764f3ebc6c > > --- /dev/null > > +++ b/drivers/media/v4l2-core/v4l2-cci.c > > @@ -0,0 +1,157 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * MIPI Camera Control Interface (CCI) register access helpers. > > + * > > + * Copyright (C) 2023 Hans de Goede <hansg@xxxxxxxxxx> > > + */ > > + > > +#include <linux/bitfield.h> > > +#include <linux/delay.h> > > +#include <linux/dev_printk.h> > > +#include <linux/module.h> > > +#include <linux/regmap.h> > > +#include <linux/types.h> > > + > > +#include <asm/unaligned.h> > > + > > +#include <media/v4l2-cci.h> > > + > > +int cci_read(struct regmap *map, u32 reg, u64 *val, int *err) > > +{ > > + int len, ret; > > len would look better unsigned. > > > + u8 buf[8]; > > + > > + if (err && *err) > > + return *err; > > + > > + len = FIELD_GET(CCI_REG_WIDTH_MASK, reg); > > + reg = FIELD_GET(CCI_REG_ADDR_MASK, reg); > > + > > + 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); > > + goto out; > > + } > > + > > + 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; > > + case 8: > > + *val = get_unaligned_be64(buf); > > + break; > > + default: > > + dev_err(regmap_get_device(map), "Error invalid reg-width %d for reg 0x%04x\n", This should use %u with an unsigned len. Same below. > > + len, reg); > > + ret = -EINVAL; > > + break; > > + } > > + > > +out: > > + if (ret && err) > > + *err = ret; > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(cci_read); > > + > > +int cci_write(struct regmap *map, u32 reg, u64 val, int *err) > > +{ > > + int len, ret; > > Same here. > > > + u8 buf[8]; > > + > > + if (err && *err) > > + return *err; > > + > > + len = FIELD_GET(CCI_REG_WIDTH_MASK, reg); > > + reg = FIELD_GET(CCI_REG_ADDR_MASK, reg); > > + > > + switch (len) { > > + case 1: > > + buf[0] = val; > > + break; > > + case 2: > > + put_unaligned_be16(val, buf); > > + break; > > + case 3: > > + put_unaligned_be24(val, buf); > > + break; > > + case 4: > > + put_unaligned_be32(val, buf); > > + break; > > + case 8: > > + put_unaligned_be64(val, buf); > > + break; > > + default: > > + dev_err(regmap_get_device(map), "Error invalid reg-width %d for reg 0x%04x\n", > > + len, reg); > > + ret = -EINVAL; > > + goto out; > > + } > > + > > + 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); > > + > > +out: > > + if (ret && err) > > + *err = ret; > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(cci_write); > > + > > +int cci_update_bits(struct regmap *map, u32 reg, u64 mask, u64 val, int *err) > > +{ > > + u64 readval; > > + int 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 cci_reg_sequence *regs, > > + int num_regs, int *err) > > +{ > > + int i, ret; > > unsigned int i? > > Same for num_regs. > > > + > > + for (i = 0; i < num_regs; i++) { > > + ret = cci_write(map, regs[i].reg, regs[i].val, err); > > + if (ret) > > + return ret; > > + } > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(cci_multi_reg_write); > > + > > +struct regmap *cci_regmap_init_i2c(struct i2c_client *client, int reg_addr_bits) Should this be called devm_cci_regmap_init_i2c() ? > > +{ > > + struct regmap_config config = { > > + .reg_bits = reg_addr_bits, > > + .val_bits = 8, > > + .reg_format_endian = REGMAP_ENDIAN_BIG, > > + .disable_locking = true, > > + }; > > + > > + return devm_regmap_init_i2c(client, &config); > > +} > > +EXPORT_SYMBOL_GPL(cci_regmap_init_i2c); > > + > > +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..5d8fdff086db > > --- /dev/null > > +++ b/include/media/v4l2-cci.h > > @@ -0,0 +1,121 @@ > > +/* 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/types.h> > > + > > +struct i2c_client; > > +struct reg_sequence; Not needed anymore. > > +struct regmap; > > + > > +/** > > + * struct cci_reg_sequence - An individual write from a sequence of CCI writes > > + * > > + * @reg: Register address, use CCI_REG#() macros to encode reg width > > + * @val: Register value > > + * > > + * Register/value pairs for sequences of writes. > > + */ > > +struct cci_reg_sequence { > > + u32 reg; > > + u64 val; > > +}; > > + > > +/* > > + * Macros to define register address with the register width encoded > > + * into the higher bits. > > + */ > > +#define CCI_REG_ADDR_MASK GENMASK(15, 0) > > +#define CCI_REG_WIDTH_SHIFT 16 > > +#define CCI_REG_WIDTH_MASK GENMASK(19, 16) > > + > > +#define CCI_REG8(x) ((1 << CCI_REG_WIDTH_SHIFT) | (x)) > > +#define CCI_REG16(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x)) > > +#define CCI_REG24(x) ((3 << CCI_REG_WIDTH_SHIFT) | (x)) > > +#define CCI_REG32(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x)) > > +#define CCI_REG64(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x)) > > + > > +/** > > + * cci_read() - Read a value from a single CCI register > > + * > > + * @map: Register map to read from > > + * @reg: Register address to read, 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 Nitpicking, s/optional/Optional/ to match the other arguments. Same below. > > + * then the read will be skipped > > + * > > + * Return: %0 on success or a negative error code on failure. > > + */ > > +int cci_read(struct regmap *map, u32 reg, u64 *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 > > + * then the write will be skipped > > + * > > + * Return: %0 on success or a negative error code on failure. > > + */ > > +int cci_write(struct regmap *map, u32 reg, u64 val, int *err); > > + > > +/** > > + * cci_update_bits() - Perform a read/modify/write cycle on a single CCI register > > + * > > + * @map: Register map to update > > + * @reg: Register address to update, 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 > > + * then the update will be skipped > > + * > > + * Note this uses read-modify-write to update the bits, atomicity wrt other s/wrt/with regard to/ > > + * cci_*() register access functions is NOT guaranteed. > > + * > > + * Return: %0 on success or a negative error code on failure. > > + */ > > +int cci_update_bits(struct regmap *map, u32 reg, u64 mask, u64 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 > > + * then the write 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. > > + * > > + * Use of the CCI_REG#() macros to encode reg width is mandatory. > > + * > > + * For raw lists of register-address, -value pairs with only 8 bit > > + * wide writes regmap_multi_reg_write() can be used instead. > > + * > > + * Return: %0 on success or a negative error code on failure. > > + */ > > +int cci_multi_reg_write(struct regmap *map, const struct cci_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 > > Could you run > > ./scripts/checkpatch.pl --strict --max-line-length=80 > > on this? -- Regards, Laurent Pinchart
