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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hello,

On Wed, Jun 07, 2023 at 12:01:10PM +0000, Sakari Ailus wrote:
> On Wed, Jun 07, 2023 at 10:40:34AM +0200, Hans de Goede wrote:
> > On 6/6/23 22:43, Andy Shevchenko wrote:
> > > On Tue, Jun 6, 2023 at 7:58 PM 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.
> > > 
> > > ...
> > > 
> > >> +#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;
> > 	}
> 
> I think the loop looks nicer but I'm fine with this as well.
> 
> > ?
> > 
> > >> +       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 ?
> 
> Regmap nor V4L2 use it so I wouldn't use it here either.

Ditto.

> > >> +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 ?
> 
> There are loops here so it won't be atomic in any case.
> 
> Generally drivers indeed already take care of this. I don't think we need
> locking on this level.

Agreed.

> > > 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 .
> 
> I'm fine with the comment.

-- 
Regards,

Laurent Pinchart