On Tue, Jun 6, 2023 at 7:58 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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.
...
> +#include <linux/delay.h>
> +#include <linux/dev_printk.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
+ types.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];
> + }
I really prefer to see put_unaligned() here depending on the length.
Note, that on some CPUs it might be one assembly instruction or even
none, depending on how the result is going to be used.
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(cci_read);
Can we have it namespaced?
> +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);
Same comments as per above function.
...
> + if (regs[i].delay_us)
I'm wondering why fsleep() doesn't have this check? Or does it?
> + fsleep(regs[i].delay_us);
...
> +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,
Is the lock required?
If so, how is it helpful?
Can we move this outside as static const?
> + };
> +
> + return devm_regmap_init_i2c(client, &config);
> +}
...
> +#ifndef _V4L2_CCI_H
> +#define _V4L2_CCI_H
+ bits.h
> +#include <linux/regmap.h>
Not used, rather requires forward declarations of
struct regmap
struct reg_sequence
Also note missing i2c_client forward declaration.
> +#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,
But this is guaranteed by the C standard... See also below.
> + cci_reg_16,
But this one becomes 1, so the above comment doesn't clarify why it's
okay to have it 1 and not 2.
> + cci_reg_24,
> + cci_reg_32,
> +};
--
With Best Regards,
Andy Shevchenko
