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