Re: [PATCH 1/3] media: Add MIPI CCI register access helper functions

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 6/6/23 22:43, Andy Shevchenko wrote:
> On Tue, Jun 6, 2023 at 7:58 PM Hans de Goede <hdegoede@redhat.com> 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.

Ok, so you mean changing it to something like this:

	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;
	}

?

		

> 
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(cci_read);
> 
> Can we have it namespaced?

I'm not sure if having just these 5 symbols in their own namespace is worth it. SO far the media subsystem is not using module/symbol namespacing at all.

Sakari, Laurent, any opinions on this ?



>> +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?

Interesting questions sensor drivers typically already do
their own locking.

So I guess we could indeed tell regmap to skip locking here.

Sakari, Laurent any opinion on this ?

> Can we move this outside as static const?

No, because reg_bits is not 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

Ack, I'll change this for the next version.

> Also note missing i2c_client forward declaration.

That was also taken care of by 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.
>> + */
>> +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.

Basically the idea is that the enum value is the reg-width in bytes - 1
where the - 1 is there so that cci_reg_8 = 0 .

Regards,

Hans