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@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? > > 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. -- Kind regards, Sakari Ailus
