Hi Hans,
On Fri, Feb 10, 2023 at 01:59:07PM +0100, Hans de Goede wrote:
> > Who will now write the patches for this? :-)
>
> I have already more or less volunteered / I opened this can of worms,
> so that would be me...
:-)
Thank you, this is much appreciated, and will allow factoring out ~ 50
lines of code from almost all sensor (as well as a few other) drivers.
>
> I see in your other reply that you are fine with going without
> wrappers for the fixed width accesses for now, good. TBH I don't
> think these will add much value.
We can get back to this topic when converting drivers. These are
convenience wrappers after all and they're easy to add if needed, there's
no connection to the underlying implementation which is the important part.
>
> I'll try to make some time to work on this somewhere the next
> couple of weeks.
>
> Here is a rough sketch of the API for initial discussion:
>
> /*
> * 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.
> */
> enum cci_reg_type {
> cci_reg_8 = 0,
> cci_reg_16,
> cci_reg_24,
> cci_reg_32,
I'd use capital letters for these as they are macro-like. Or define them as
macros.
> };
>
> /*
> * Macros to define register address with the register width encoded
> * into the higher bits. CCI_REG8() is a no-op so its use is optional.
> */
> #define CCI_REG_SIZE_SHIFT 16
> #define CCI_REG_ADDR_MASK GENMASK(15, 0)
>
> #define CCI_REG8(x) ((cci_reg_8 << CCI_REG_SIZE_SHIFT) | (x))
> #define CCI_REG16(x) ((cci_reg_16 << CCI_REG_SIZE_SHIFT) | (x))
> #define CCI_REG24(x) ((cci_reg_24 << CCI_REG_SIZE_SHIFT) | (x))
> #define CCI_REG32(x) ((cci_reg_32 << CCI_REG_SIZE_SHIFT) | (x))
The top 8 bits could be used for flags in the future, for driver's use.
The CCS driver stores register's properties (value format) there.
>
> int cci_read(struct regmap *regmap, u32 reg, u32 *val, int *err);
> int cci_write(struct regmap *regmap, u32 reg, u32 val, int *err);
> int cci_update_bits(struct regmap *map, u32 reg, u32 mask, u32 val, int *err);
> int cci_multi_reg_write(struct regmap *map, const struct reg_sequence *regs, int num_regs, int *err);
We should also have a bulk data write function, although drivers could use
regmap directly to do this. Not needed now, nor this is a common need. Just
a note.
>
> Note the regmap here is intended to be a regmap with 16 bit register-address
> width and 8 bit register-data width. I'll add a helper to get the regmap
> from an i2c_client to the initial implementation.
CCI also supports 8-bit register addresses (virtually all lens VCM and
flash drivers as well as some sensor drivers) so this should be a
parameter.
I expect some drivers to support CCI via both I²C and I3C. We don't need
I3C now as all sensors are I²C, but I²C should be visible in the API only
when it's about making the connection to an I²C client.
>
> Also note that all the function names have been chosen to be 1:1 mirrors
> of the matching regmap functions with the addition of the *err argument.
--
Kind regards,
Sakari Ailus
