Hi Laurent,
On Fri, Nov 10, 2023 at 05:21:15PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Fri, Nov 10, 2023 at 11:47:05AM +0200, Sakari Ailus wrote:
> > Use V4L2 CCI for accessing device's registers. The 8-bit compatibility
> > read option is removed but this is supported by regmap through other
> > means.
> >
> > Also the CCS register definitions are re-generated with V4L2 CCI
> > definitions. The older SMIA++ register definitions have been manually
> > converted.
>
> Unrelated to this series, could ccs-regs.h be generated as part of the
> kernel build process ?
I was thinking of it but it requires Perl. That would be a new addition to
the minimum requirements and as much as people like Perl, it might not be
trivially available in every system where the kernel (including the CCS
driver) needs to be built.
>
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/ccs/ccs-core.c | 80 +-
> > drivers/media/i2c/ccs/ccs-reg-access.c | 191 +----
> > drivers/media/i2c/ccs/ccs-regs.h | 904 +++++++++++-----------
> > drivers/media/i2c/ccs/ccs.h | 2 +
> > drivers/media/i2c/ccs/smiapp-reg-defs.h | 948 ++++++++++++------------
> > 5 files changed, 988 insertions(+), 1137 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > index d210b6c87db4..471bb30ab298 100644
> > --- a/drivers/media/i2c/ccs/ccs-core.c
> > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > @@ -25,8 +25,9 @@
> > #include <linux/slab.h>
> > #include <linux/smiapp.h>
> > #include <linux/v4l2-mediabus.h>
> > -#include <media/v4l2-fwnode.h>
> > +#include <media/v4l2-cci.h>
> > #include <media/v4l2-device.h>
> > +#include <media/v4l2-fwnode.h>
>
> As you're also sorting includes, mediabus goes last.
It's under include/linux directory, not include/media.
>
> > #include <uapi/linux/ccs.h>
> >
> > #include "ccs.h"
> > @@ -98,7 +99,7 @@ static int ccs_limit_ptr(struct ccs_sensor *sensor, unsigned int limit,
> > linfo = &ccs_limits[ccs_limit_offsets[limit].info];
> >
> > if (WARN_ON(!sensor->ccs_limits) ||
> > - WARN_ON(offset + ccs_reg_width(linfo->reg) >
> > + WARN_ON(offset + CCI_REG_WIDTH_BYTES(linfo->reg) >
> > ccs_limit_offsets[limit + 1].lim))
> > return -EINVAL;
> >
> > @@ -124,7 +125,7 @@ void ccs_replace_limit(struct ccs_sensor *sensor,
> > dev_dbg(&client->dev, "quirk: 0x%8.8x \"%s\" %u = %u, 0x%x\n",
> > linfo->reg, linfo->name, offset, val, val);
> >
> > - ccs_assign_limit(ptr, ccs_reg_width(linfo->reg), val);
> > + ccs_assign_limit(ptr, CCI_REG_WIDTH_BYTES(linfo->reg), val);
> > }
> >
> > u32 ccs_get_limit(struct ccs_sensor *sensor, unsigned int limit,
> > @@ -138,7 +139,7 @@ u32 ccs_get_limit(struct ccs_sensor *sensor, unsigned int limit,
> > if (ret)
> > return 0;
> >
> > - switch (ccs_reg_width(ccs_limits[ccs_limit_offsets[limit].info].reg)) {
> > + switch (CCI_REG_WIDTH_BYTES(ccs_limits[ccs_limit_offsets[limit].info].reg)) {
> > case sizeof(u8):
> > val = *(u8 *)ptr;
> > break;
> > @@ -176,7 +177,7 @@ static int ccs_read_all_limits(struct ccs_sensor *sensor)
> >
> > for (i = 0, l = 0, ptr = alloc; ccs_limits[i].size; i++) {
> > u32 reg = ccs_limits[i].reg;
> > - unsigned int width = ccs_reg_width(reg);
> > + unsigned int width = CCI_REG_WIDTH_BYTES(reg);
> > unsigned int j;
> >
> > if (l == CCS_L_LAST) {
> > @@ -2725,66 +2726,54 @@ static int ccs_identify_module(struct ccs_sensor *sensor)
> > rval = ccs_read(sensor, MODULE_MANUFACTURER_ID,
> > &minfo->mipi_manufacturer_id);
> > if (!rval && !minfo->mipi_manufacturer_id)
> > - rval = ccs_read_addr_8only(sensor,
> > - SMIAPP_REG_U8_MANUFACTURER_ID,
> > - &minfo->smia_manufacturer_id);
> > + rval = ccs_read_addr(sensor, SMIAPP_REG_U8_MANUFACTURER_ID,
> > + &minfo->smia_manufacturer_id);
> > if (!rval)
> > - rval = ccs_read_addr_8only(sensor, CCS_R_MODULE_MODEL_ID,
> > - &minfo->model_id);
> > + rval = ccs_read(sensor, MODULE_MODEL_ID, &minfo->model_id);
> > if (!rval)
> > - rval = ccs_read_addr_8only(sensor,
> > - CCS_R_MODULE_REVISION_NUMBER_MAJOR,
> > - &rev);
> > + rval = ccs_read(sensor, MODULE_REVISION_NUMBER_MAJOR, &rev);
> > if (!rval) {
> > - rval = ccs_read_addr_8only(sensor,
> > - CCS_R_MODULE_REVISION_NUMBER_MINOR,
> > - &minfo->revision_number);
> > + rval = ccs_read(sensor, MODULE_REVISION_NUMBER_MINOR,
> > + &minfo->revision_number);
> > minfo->revision_number |= rev << 8;
> > }
> > if (!rval)
> > - rval = ccs_read_addr_8only(sensor, CCS_R_MODULE_DATE_YEAR,
> > - &minfo->module_year);
> > + rval = ccs_read(sensor, MODULE_DATE_YEAR, &minfo->module_year);
> > if (!rval)
> > - rval = ccs_read_addr_8only(sensor, CCS_R_MODULE_DATE_MONTH,
> > - &minfo->module_month);
> > + rval = ccs_read(sensor, MODULE_DATE_MONTH,
> > + &minfo->module_month);
> > if (!rval)
> > - rval = ccs_read_addr_8only(sensor, CCS_R_MODULE_DATE_DAY,
> > - &minfo->module_day);
> > + rval = ccs_read(sensor, MODULE_DATE_DAY, &minfo->module_day);
> >
> > /* Sensor info */
> > if (!rval)
> > rval = ccs_read(sensor, SENSOR_MANUFACTURER_ID,
> > &minfo->sensor_mipi_manufacturer_id);
> > if (!rval && !minfo->sensor_mipi_manufacturer_id)
> > - rval = ccs_read_addr_8only(sensor,
> > - CCS_R_SENSOR_MANUFACTURER_ID,
> > - &minfo->sensor_smia_manufacturer_id);
> > + rval = ccs_read(sensor, SENSOR_MANUFACTURER_ID,
> > + &minfo->sensor_smia_manufacturer_id);
> > if (!rval)
> > - rval = ccs_read_addr_8only(sensor,
> > - CCS_R_SENSOR_MODEL_ID,
> > - &minfo->sensor_model_id);
> > + rval = ccs_read(sensor, SENSOR_MODEL_ID,
> > + &minfo->sensor_model_id);
> > if (!rval)
> > - rval = ccs_read_addr_8only(sensor,
> > - CCS_R_SENSOR_REVISION_NUMBER,
> > - &minfo->sensor_revision_number);
> > + rval = ccs_read(sensor, SENSOR_REVISION_NUMBER,
> > + &minfo->sensor_revision_number);
> > if (!rval && !minfo->sensor_revision_number)
> > - rval = ccs_read_addr_8only(sensor,
> > - CCS_R_SENSOR_REVISION_NUMBER_16,
> > - &minfo->sensor_revision_number);
> > + rval = ccs_read(sensor, SENSOR_REVISION_NUMBER_16,
> > + &minfo->sensor_revision_number);
> > if (!rval)
> > - rval = ccs_read_addr_8only(sensor,
> > - CCS_R_SENSOR_FIRMWARE_VERSION,
> > - &minfo->sensor_firmware_version);
> > + rval = ccs_read(sensor, SENSOR_FIRMWARE_VERSION,
> > + &minfo->sensor_firmware_version);
> >
> > /* SMIA */
> > if (!rval)
> > rval = ccs_read(sensor, MIPI_CCS_VERSION, &minfo->ccs_version);
> > if (!rval && !minfo->ccs_version)
> > - rval = ccs_read_addr_8only(sensor, SMIAPP_REG_U8_SMIA_VERSION,
> > - &minfo->smia_version);
> > + rval = ccs_read_addr(sensor, SMIAPP_REG_U8_SMIA_VERSION,
> > + &minfo->smia_version);
> > if (!rval && !minfo->ccs_version)
> > - rval = ccs_read_addr_8only(sensor, SMIAPP_REG_U8_SMIAPP_VERSION,
> > - &minfo->smiapp_version);
> > + rval = ccs_read_addr(sensor, SMIAPP_REG_U8_SMIAPP_VERSION,
> > + &minfo->smiapp_version);
> >
> > if (rval) {
> > dev_err(&client->dev, "sensor detection failed\n");
> > @@ -3318,6 +3307,13 @@ static int ccs_probe(struct i2c_client *client)
> > if (IS_ERR(sensor->xshutdown))
> > return PTR_ERR(sensor->xshutdown);
> >
> > + sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> > + if (IS_ERR(sensor->regmap)) {
> > + dev_err(&client->dev, "can't initialise CCI (%ld)\n",
> > + PTR_ERR(sensor->regmap));
> > + return PTR_ERR(sensor->regmap);
> > + }
> > +
> > rval = ccs_power_on(&client->dev);
> > if (rval < 0)
> > return rval;
> > @@ -3653,7 +3649,7 @@ static int ccs_module_init(void)
> > ccs_limit_offsets[l + 1].lim =
> > ALIGN(ccs_limit_offsets[l].lim +
> > ccs_limits[i].size,
> > - ccs_reg_width(ccs_limits[i + 1].reg));
> > + max(CCI_REG_WIDTH_BYTES(ccs_limits[i + 1].reg), 1UL));
>
> What's the reason for the max() here ?
The guardian entry of the ccs_limits array contains zero in the reg field,
as one would expect, and CCI_REG_WIDTH_BYTES() returns, as one would
expect, zero. And aligning by zero is generally shunned by experienced
kernel developers.
ccs_reg_width() used flags for 16- and 32-bit registers so a register that
had neither of the flags was considered an 8-bit one.
>
> > ccs_limit_offsets[l].info = i;
> > l++;
> > } else {
> > diff --git a/drivers/media/i2c/ccs/ccs-reg-access.c b/drivers/media/i2c/ccs/ccs-reg-access.c
> > index 652d705a2ef5..81d3e2db38f1 100644
> > --- a/drivers/media/i2c/ccs/ccs-reg-access.c
> > +++ b/drivers/media/i2c/ccs/ccs-reg-access.c
> > @@ -62,87 +62,6 @@ static u32 float_to_u32_mul_1000000(struct i2c_client *client, u32 phloat)
> > }
> >
> >
> > -/*
> > - * Read a 8/16/32-bit i2c register. The value is returned in 'val'.
> > - * Returns zero if successful, or non-zero otherwise.
> > - */
> > -static int ____ccs_read_addr(struct ccs_sensor *sensor, u16 reg, u16 len,
> > - u32 *val)
> > -{
> > - struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > - struct i2c_msg msg;
> > - unsigned char data_buf[sizeof(u32)] = { 0 };
> > - unsigned char offset_buf[sizeof(u16)];
> > - int r;
> > -
> > - if (len > sizeof(data_buf))
> > - return -EINVAL;
> > -
> > - msg.addr = client->addr;
> > - msg.flags = 0;
> > - msg.len = sizeof(offset_buf);
> > - msg.buf = offset_buf;
> > - put_unaligned_be16(reg, offset_buf);
> > -
> > - r = i2c_transfer(client->adapter, &msg, 1);
> > - if (r != 1) {
> > - if (r >= 0)
> > - r = -EBUSY;
> > - goto err;
> > - }
> > -
> > - msg.len = len;
> > - msg.flags = I2C_M_RD;
> > - msg.buf = &data_buf[sizeof(data_buf) - len];
> > -
> > - r = i2c_transfer(client->adapter, &msg, 1);
> > - if (r != 1) {
> > - if (r >= 0)
> > - r = -EBUSY;
> > - goto err;
> > - }
> > -
> > - *val = get_unaligned_be32(data_buf);
> > -
> > - return 0;
> > -
> > -err:
> > - dev_err(&client->dev, "read from offset 0x%x error %d\n", reg, r);
> > -
> > - return r;
> > -}
> > -
> > -/* Read a register using 8-bit access only. */
> > -static int ____ccs_read_addr_8only(struct ccs_sensor *sensor, u16 reg,
> > - u16 len, u32 *val)
> > -{
> > - unsigned int i;
> > - int rval;
> > -
> > - *val = 0;
> > -
> > - for (i = 0; i < len; i++) {
> > - u32 val8;
> > -
> > - rval = ____ccs_read_addr(sensor, reg + i, 1, &val8);
> > - if (rval < 0)
> > - return rval;
> > - *val |= val8 << ((len - i - 1) << 3);
> > - }
> > -
> > - return 0;
> > -}
> > -
> > -unsigned int ccs_reg_width(u32 reg)
> > -{
> > - if (reg & CCS_FL_16BIT)
> > - return sizeof(u16);
> > - if (reg & CCS_FL_32BIT)
> > - return sizeof(u32);
> > -
> > - return sizeof(u8);
> > -}
> > -
> > static u32 ireal32_to_u32_mul_1000000(struct i2c_client *client, u32 val)
> > {
> > if (val >> 10 > U32_MAX / 15625) {
> > @@ -178,21 +97,14 @@ u32 ccs_reg_conv(struct ccs_sensor *sensor, u32 reg, u32 val)
> > static int __ccs_read_addr(struct ccs_sensor *sensor, u32 reg, u32 *val,
> > bool only8, bool conv)
> > {
> > - unsigned int len = ccs_reg_width(reg);
> > + u64 __val;
> > int rval;
> >
> > - if (!only8)
> > - rval = ____ccs_read_addr(sensor, CCS_REG_ADDR(reg), len, val);
> > - else
> > - rval = ____ccs_read_addr_8only(sensor, CCS_REG_ADDR(reg), len,
> > - val);
> > + rval = cci_read(sensor->regmap, reg, &__val, NULL);
> > if (rval < 0)
> > return rval;
> >
> > - if (!conv)
> > - return 0;
> > -
> > - *val = ccs_reg_conv(sensor, reg, *val);
> > + *val = conv ? ccs_reg_conv(sensor, reg, __val) : __val;
> >
> > return 0;
> > }
> > @@ -200,7 +112,7 @@ static int __ccs_read_addr(struct ccs_sensor *sensor, u32 reg, u32 *val,
> > static int __ccs_static_read_only(struct ccs_reg *regs, size_t num_regs,
> > u32 reg, u32 *val)
> > {
> > - unsigned int width = ccs_reg_width(reg);
> > + unsigned int width = CCI_REG_WIDTH_BYTES(reg);
> > size_t i;
> >
> > for (i = 0; i < num_regs; i++, regs++) {
> > @@ -291,71 +203,13 @@ int ccs_read_addr_noconv(struct ccs_sensor *sensor, u32 reg, u32 *val)
> > return ccs_read_addr_raw(sensor, reg, val, false, true, false, true);
> > }
> >
> > -static int ccs_write_retry(struct i2c_client *client, struct i2c_msg *msg)
> > -{
> > - unsigned int retries;
> > - int r;
> > -
> > - for (retries = 0; retries < 10; retries++) {
> > - /*
> > - * Due to unknown reason sensor stops responding. This
> > - * loop is a temporaty solution until the root cause
> > - * is found.
> > - */
> > - r = i2c_transfer(client->adapter, msg, 1);
> > - if (r != 1) {
> > - usleep_range(1000, 2000);
> > - continue;
> > - }
> > -
> > - if (retries)
> > - dev_err(&client->dev,
> > - "sensor i2c stall encountered. retries: %d\n",
> > - retries);
> > - return 0;
> > - }
> > -
> > - return r;
> > -}
> > -
> > -int ccs_write_addr_no_quirk(struct ccs_sensor *sensor, u32 reg, u32 val)
> > -{
> > - struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > - struct i2c_msg msg;
> > - unsigned char data[6];
> > - unsigned int len = ccs_reg_width(reg);
> > - int r;
> > -
> > - if (len > sizeof(data) - 2)
> > - return -EINVAL;
> > -
> > - msg.addr = client->addr;
> > - msg.flags = 0; /* Write */
> > - msg.len = 2 + len;
> > - msg.buf = data;
> > -
> > - put_unaligned_be16(CCS_REG_ADDR(reg), data);
> > - put_unaligned_be32(val << (8 * (sizeof(val) - len)), data + 2);
> > -
> > - dev_dbg(&client->dev, "writing reg 0x%4.4x value 0x%*.*x (%u)\n",
> > - CCS_REG_ADDR(reg), ccs_reg_width(reg) << 1,
> > - ccs_reg_width(reg) << 1, val, val);
> > -
> > - r = ccs_write_retry(client, &msg);
> > - if (r)
> > - dev_err(&client->dev,
> > - "wrote 0x%x to offset 0x%x error %d\n", val,
> > - CCS_REG_ADDR(reg), r);
> > -
> > - return r;
> > -}
> > -
> > /*
> > * Write to a 8/16-bit register.
> > * Returns zero if successful, or non-zero otherwise.
> > */
> > int ccs_write_addr(struct ccs_sensor *sensor, u32 reg, u32 val)
> > {
> > + unsigned int retries = 10;
>
> This is really not nice, but unrelated to this patch.
>
> > int rval;
> >
> > rval = ccs_call_quirk(sensor, reg_access, true, ®, &val);
> > @@ -364,7 +218,12 @@ int ccs_write_addr(struct ccs_sensor *sensor, u32 reg, u32 val)
> > if (rval < 0)
> > return rval;
>
> Here you test rval < 0, but below cci_write() will consider any positive
> value as an error too. There may not be an actual bug if the function
> call above doesn't return positive values, but it's error-prone
> nonetheless.
Good point. The quirk function is supposed to return either an error or
zero, but I'll assign it to zero here just in case.
I'd hope in the future we would have few quirks and would do most of this
using CCS static data instead.
>
> >
> > - return ccs_write_addr_no_quirk(sensor, reg, val);
> > + do {
> > + if (cci_write(sensor->regmap, reg, val, &rval))
> > + fsleep(1000);
> > + } while (rval && --retries);
> > +
> > + return rval;
> > }
> >
> > #define MAX_WRITE_LEN 32U
> > @@ -373,40 +232,36 @@ int ccs_write_data_regs(struct ccs_sensor *sensor, struct ccs_reg *regs,
> > size_t num_regs)
> > {
> > struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > - unsigned char buf[2 + MAX_WRITE_LEN];
> > - struct i2c_msg msg = {
> > - .addr = client->addr,
> > - .buf = buf,
> > - };
> > size_t i;
> >
> > for (i = 0; i < num_regs; i++, regs++) {
> > unsigned char *regdata = regs->value;
> > unsigned int j;
> > + int len;
> >
> > - for (j = 0; j < regs->len;
> > - j += msg.len - 2, regdata += msg.len - 2) {
> > + for (j = 0; j < regs->len; j += len, regdata += len) {
> > char printbuf[(MAX_WRITE_LEN << 1) +
> > 1 /* \0 */] = { 0 };
> > + unsigned int retries = 10;
> > int rval;
> >
> > - msg.len = min(regs->len - j, MAX_WRITE_LEN);
> > + len = min(regs->len - j, MAX_WRITE_LEN);
> >
> > - bin2hex(printbuf, regdata, msg.len);
> > + bin2hex(printbuf, regdata, len);
> > dev_dbg(&client->dev,
> > "writing msr reg 0x%4.4x value 0x%s\n",
> > regs->addr + j, printbuf);
> >
> > - put_unaligned_be16(regs->addr + j, buf);
> > - memcpy(buf + 2, regdata, msg.len);
> > -
> > - msg.len += 2;
> > -
> > - rval = ccs_write_retry(client, &msg);
> > + do {
> > + rval = regmap_bulk_write(sensor->regmap,
> > + regs->addr + j, regdata, len);
>
> I'm surprised by the line length ;-)
Yes, indeed, there seems to be an issue here. I'll fix it for v2.
>
> > + if (rval)
> > + fsleep(1000);
> > + } while (rval && --retries);
>
> A blank line would be nice.
Yes.
>
> > if (rval) {
> > dev_err(&client->dev,
> > "error writing %u octets to address 0x%4.4x\n",
> > - msg.len, regs->addr + j);
> > + len, regs->addr + j);
> > return rval;
> > }
> > }
> > diff --git a/drivers/media/i2c/ccs/ccs-regs.h b/drivers/media/i2c/ccs/ccs-regs.h
> > index 6ce84c5ecf20..45f933cbe478 100644
> > --- a/drivers/media/i2c/ccs/ccs-regs.h
> > +++ b/drivers/media/i2c/ccs/ccs-regs.h
> > @@ -10,59 +10,57 @@
> >
> > #include <linux/bits.h>
> >
> > -#define CCS_FL_BASE 16
> > -#define CCS_FL_16BIT BIT(CCS_FL_BASE)
> > -#define CCS_FL_32BIT BIT(CCS_FL_BASE + 1)
> > -#define CCS_FL_FLOAT_IREAL BIT(CCS_FL_BASE + 2)
> > -#define CCS_FL_IREAL BIT(CCS_FL_BASE + 3)
> > -#define CCS_R_ADDR(r) ((r) & 0xffff)
> > +#include <media/v4l2-cci.h>
> >
> > -#define CCS_R_MODULE_MODEL_ID (0x0000 | CCS_FL_16BIT)
> > -#define CCS_R_MODULE_REVISION_NUMBER_MAJOR 0x0002
> > -#define CCS_R_FRAME_COUNT 0x0005
> > -#define CCS_R_PIXEL_ORDER 0x0006
> > +#define CCS_FL_BASE CCI_REG_FLAG_PRIVATE_START
>
> Unless CCS_FL_BASE is used in the driver, I would drop it and use
> CCI_REG_FLAG_PRIVATE_START in the two macros below. Up to you.
>
> > +#define CCS_FL_FLOAT_IREAL BIT(CCS_FL_BASE)
> > +#define CCS_FL_IREAL BIT(CCS_FL_BASE + 1)
> > +#define CCS_R_MODULE_MODEL_ID CCI_REG16(0x0000)
> > +#define CCS_R_MODULE_REVISION_NUMBER_MAJOR CCI_REG8(0x0002)
> > +#define CCS_R_FRAME_COUNT CCI_REG8(0x0005)
> > +#define CCS_R_PIXEL_ORDER CCI_REG8(0x0006)
> > #define CCS_PIXEL_ORDER_GRBG 0U
> > #define CCS_PIXEL_ORDER_RGGB 1U
> > #define CCS_PIXEL_ORDER_BGGR 2U
> > #define CCS_PIXEL_ORDER_GBRG 3U
> > -#define CCS_R_MIPI_CCS_VERSION 0x0007
> > +#define CCS_R_MIPI_CCS_VERSION CCI_REG8(0x0007)
> > #define CCS_MIPI_CCS_VERSION_V1_0 0x10
> > #define CCS_MIPI_CCS_VERSION_V1_1 0x11
> > #define CCS_MIPI_CCS_VERSION_MAJOR_SHIFT 4U
> > #define CCS_MIPI_CCS_VERSION_MAJOR_MASK 0xf0
> > #define CCS_MIPI_CCS_VERSION_MINOR_SHIFT 0U
> > #define CCS_MIPI_CCS_VERSION_MINOR_MASK 0xf
> > -#define CCS_R_DATA_PEDESTAL (0x0008 | CCS_FL_16BIT)
> > -#define CCS_R_MODULE_MANUFACTURER_ID (0x000e | CCS_FL_16BIT)
> > -#define CCS_R_MODULE_REVISION_NUMBER_MINOR 0x0010
> > -#define CCS_R_MODULE_DATE_YEAR 0x0012
> > -#define CCS_R_MODULE_DATE_MONTH 0x0013
> > -#define CCS_R_MODULE_DATE_DAY 0x0014
> > -#define CCS_R_MODULE_DATE_PHASE 0x0015
> > +#define CCS_R_DATA_PEDESTAL CCI_REG16(0x0008)
> > +#define CCS_R_MODULE_MANUFACTURER_ID CCI_REG16(0x000e)
> > +#define CCS_R_MODULE_REVISION_NUMBER_MINOR CCI_REG8(0x0010)
> > +#define CCS_R_MODULE_DATE_YEAR CCI_REG8(0x0012)
> > +#define CCS_R_MODULE_DATE_MONTH CCI_REG8(0x0013)
> > +#define CCS_R_MODULE_DATE_DAY CCI_REG8(0x0014)
> > +#define CCS_R_MODULE_DATE_PHASE CCI_REG8(0x0015)
> > #define CCS_MODULE_DATE_PHASE_SHIFT 0U
> > #define CCS_MODULE_DATE_PHASE_MASK 0x7
> > #define CCS_MODULE_DATE_PHASE_TS 0U
> > #define CCS_MODULE_DATE_PHASE_ES 1U
> > #define CCS_MODULE_DATE_PHASE_CS 2U
> > #define CCS_MODULE_DATE_PHASE_MP 3U
> > -#define CCS_R_SENSOR_MODEL_ID (0x0016 | CCS_FL_16BIT)
> > -#define CCS_R_SENSOR_REVISION_NUMBER 0x0018
> > -#define CCS_R_SENSOR_FIRMWARE_VERSION 0x001a
> > -#define CCS_R_SERIAL_NUMBER (0x001c | CCS_FL_32BIT)
> > -#define CCS_R_SENSOR_MANUFACTURER_ID (0x0020 | CCS_FL_16BIT)
> > -#define CCS_R_SENSOR_REVISION_NUMBER_16 (0x0022 | CCS_FL_16BIT)
> > -#define CCS_R_FRAME_FORMAT_MODEL_TYPE 0x0040
> > +#define CCS_R_SENSOR_MODEL_ID CCI_REG16(0x0016)
> > +#define CCS_R_SENSOR_REVISION_NUMBER CCI_REG8(0x0018)
> > +#define CCS_R_SENSOR_FIRMWARE_VERSION CCI_REG8(0x001a)
> > +#define CCS_R_SERIAL_NUMBER CCI_REG32(0x001c)
> > +#define CCS_R_SENSOR_MANUFACTURER_ID CCI_REG16(0x0020)
> > +#define CCS_R_SENSOR_REVISION_NUMBER_16 CCI_REG16(0x0022)
> > +#define CCS_R_FRAME_FORMAT_MODEL_TYPE CCI_REG8(0x0040)
> > #define CCS_FRAME_FORMAT_MODEL_TYPE_2_BYTE 1U
> > #define CCS_FRAME_FORMAT_MODEL_TYPE_4_BYTE 2U
> > -#define CCS_R_FRAME_FORMAT_MODEL_SUBTYPE 0x0041
> > +#define CCS_R_FRAME_FORMAT_MODEL_SUBTYPE CCI_REG8(0x0041)
> > #define CCS_FRAME_FORMAT_MODEL_SUBTYPE_ROWS_SHIFT 0U
> > #define CCS_FRAME_FORMAT_MODEL_SUBTYPE_ROWS_MASK 0xf
> > #define CCS_FRAME_FORMAT_MODEL_SUBTYPE_COLUMNS_SHIFT 4U
> > #define CCS_FRAME_FORMAT_MODEL_SUBTYPE_COLUMNS_MASK 0xf0
> > -#define CCS_R_FRAME_FORMAT_DESCRIPTOR(n) ((0x0042 | CCS_FL_16BIT) + (n) * 2)
> > +#define CCS_R_FRAME_FORMAT_DESCRIPTOR(n) (CCI_REG16(0x0042) + (n) * 2)
>
> This assumes that the address is encoded in the LSBs. I think it would
> be better to write
>
> #define CCS_R_FRAME_FORMAT_DESCRIPTOR(n) CCI_REG16(0x0042 + (n) * 2)
Oops. I missed checking this one. I'll fix the script for v2...
>
> > #define CCS_LIM_FRAME_FORMAT_DESCRIPTOR_MIN_N 0U
> > #define CCS_LIM_FRAME_FORMAT_DESCRIPTOR_MAX_N 14U
> > -#define CCS_R_FRAME_FORMAT_DESCRIPTOR_4(n) ((0x0060 | CCS_FL_32BIT) + (n) * 4)
> > +#define CCS_R_FRAME_FORMAT_DESCRIPTOR_4(n) (CCI_REG32(0x0060) + (n) * 4)
> > #define CCS_FRAME_FORMAT_DESCRIPTOR_PIXELS_SHIFT 0U
> > #define CCS_FRAME_FORMAT_DESCRIPTOR_PIXELS_MASK 0xfff
> > #define CCS_FRAME_FORMAT_DESCRIPTOR_PCODE_SHIFT 12U
>
> [snip]
>
> > diff --git a/drivers/media/i2c/ccs/ccs.h b/drivers/media/i2c/ccs/ccs.h
> > index 2c013d96adcc..096573845a10 100644
> > --- a/drivers/media/i2c/ccs/ccs.h
> > +++ b/drivers/media/i2c/ccs/ccs.h
> > @@ -13,6 +13,7 @@
> > #define __CCS_H__
> >
> > #include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > #include <media/v4l2-ctrls.h>
> > #include <media/v4l2-subdev.h>
> >
> > @@ -211,6 +212,7 @@ struct ccs_sensor {
> > struct clk *ext_clk;
> > struct gpio_desc *xshutdown;
> > struct gpio_desc *reset;
> > + struct regmap *regmap;
> > void *ccs_limits;
> > u8 nbinning_subtypes;
> > struct ccs_binning_subtype binning_subtypes[CCS_LIM_BINNING_SUB_TYPE_MAX_N + 1];
> > diff --git a/drivers/media/i2c/ccs/smiapp-reg-defs.h b/drivers/media/i2c/ccs/smiapp-reg-defs.h
> > index 177e3e51207a..72b1af2a9824 100644
> > --- a/drivers/media/i2c/ccs/smiapp-reg-defs.h
> > +++ b/drivers/media/i2c/ccs/smiapp-reg-defs.h
> > @@ -13,480 +13,480 @@
> > #define __SMIAPP_REG_DEFS_H__
>
> Shouldn't you include v4l2-cci.h ?
I can add this for completeness. linux/bits.h is needed, too.
>
> >
> > /* Register addresses */
>
> I haven't reviewed the manual changes here.
That's fine. I'm not worried about those as I used a small Perl script
(that is however bygone now) to do the conversion. :-)
>
> [snip]
> >
> > /* Register bit definitions */
> > #define SMIAPP_IMAGE_ORIENTATION_HFLIP BIT(0)
>
--
Kind regards,
Sakari Ailus
