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

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

Hi Hans,

On Mon, Jun 12, 2023 at 05:33:40PM +0200, Hans de Goede wrote:
> On 6/12/23 17:16, Laurent Pinchart wrote:
> > On Mon, Jun 12, 2023 at 03:48:01PM +0200, Hans de Goede wrote:
> >> On 6/8/23 12:27, Laurent Pinchart wrote:
> >>> On Wed, Jun 07, 2023 at 09:01:40PM +0200, Hans de Goede wrote:
> >>>> On 6/7/23 20:18, Laurent Pinchart wrote:
> >>
> >> <snip>
> >>
> >>>>>> +int cci_update_bits(struct regmap *map, u32 reg, u32 mask, u32 val, int *err)
> >>>>>> +{
> >>>>>> +	int width, ret;
> >>>>>> +	u32 readval;
> >>>>>> +
> >>>>>> +	if (err && *err)
> >>>>>> +		return *err;
> >>>>>> +
> >>>>>> +	/*
> >>>>>> +	 * For single byte updates use regmap_update_bits(), this uses
> >>>>>> +	 * the regmap-lock to protect against other read-modify-writes racing.
> >>>>>> +	 */
> >>>>>> +	width = (reg & CCI_REG_WIDTH_MASK) >> CCI_REG_WIDTH_SHIFT;
> >>>>>> +	if (width == cci_reg_8) {
> >>>>>> +		reg &= CCI_REG_ADDR_MASK;
> >>>>>> +		ret = regmap_update_bits(map, reg, mask, val);
> >>>>>> +		if (ret) {
> >>>>>> +			dev_err(regmap_get_device(map), "Error updating reg 0x%4x: %d\n", reg, ret);
> >>>>>> +			if (err)
> >>>>>> +				*err = ret;
> >>>>>> +		}
> >>>>>> +
> >>>>>> +		return ret;
> >>>>>> +	}
> >>>>>> +
> >>>>>> +	ret = cci_read(map, reg, &readval, err);
> >>>>>> +	if (ret)
> >>>>>> +		return ret;
> >>>>>> +
> >>>>>> +	val = (readval & ~mask) | (val & mask);
> >>>>>> +
> >>>>>> +	return cci_write(map, reg, val, err);
> >>>>>
> >>>>> Unless I'm mistaken, the regmap cache isn't used. This makes update
> >>>>> operations fairly costly due to the read. Could that be improved ?
> >>>>
> >>>> The problem is that some registers may be volatile,
> >>>> think e.g. expsoure on a sensor where auto-exposure is supported.
> >>>>
> >>>> So normally drivers which want to use regmap caching, also
> >>>> provide a whole bunch of tables describing the registers
> >>>> (lists of volatile + list of writable + list of readable
> >>>> registers).
> >>>>
> >>>> So enabling caching is not trivial. I think that it would be best
> >>>> for drivers which want that to supply their own regmap_config config
> >>>> and directly call devm_regmap_init_i2c() if they then use
> >>>> the resulting regmaps with the existing cci_* helpers then caching
> >>>> will be used automatically.
> >>>
> >>> Would there be a way to use the cache for update operations (as I think
> >>> we can consider that registers used in those operations won't be
> >>> volatile), and bypass it for standalone reads ?
> >>
> >> There is not really a nice way to only use the cache for update operations.
> >>
> >> I guess we could do something hacky like tell regmap to create a cache,
> >> then set the cache-bypass flag and drop the cache-bypass flag during
> >> update ops. But that really is abusing the regmap API.
> >>
> >> Generally speaking update operations don't happen that often though,
> >> so IMHO hacking to get this cached is not worth it.
> > 
> > I2C reads are slow, so even if they're not very common, it would be nice
> > to avoid them. We can start without any caching and improve it later,
> > I'm fine with that.
> > 
> >>>>>> +}
> >>>>>> +EXPORT_SYMBOL_GPL(cci_update_bits);
> >>>>>> +
> >>>>>> +int cci_multi_reg_write(struct regmap *map, const struct reg_sequence *regs, int num_regs, int *err)
> >>>>>> +{
> >>>>>> +	int i, ret;
> >>>>>> +
> >>>>>> +	if (err && *err)
> >>>>>> +		return *err;
> >>>>>> +
> >>>>>> +	for (i = 0; i < num_regs; i++) {
> >>>>>> +		ret = cci_write(map, regs[i].reg, regs[i].def, err);
> >>>>>> +		if (ret)
> >>>>>> +			return ret;
> >>>>>> +
> >>>>>> +		if (regs[i].delay_us)
> >>>>>> +			fsleep(regs[i].delay_us);
> >>>>>
> >>>>> Do you have an immediate need for this ? If not, I'd drop support for
> >>>>> the delay, and add it later when and if needed. It will be easier to
> >>>>> discuss the API and use cases with a real user.
> >>>>
> >>>> This is a 1:1 mirror of regmap_multi_reg_write() note this uses
> >>>> the existing struct reg_sequence delay_us field and the:
> >>>>
> >>>> 		if (regs[i].delay_us)
> >>>> 			fsleep(regs[i].delay_us);
> >>>>
> >>>> is copied from the implementation of regmap_multi_reg_write()
> >>>
> >>> The reason why I don't like it much as that such delays are often hacks
> >>> hidden in the middle of register arrays that should in many cases be
> >>> handled differently. I was hoping that, by not supporting them yet,
> >>> we'll have an easier time to get drivers right. Maybe I'm wrong.
> >>
> >> I understand, but having this is more or less a downside of
> >> the choice to mirror the regmap API as close as possible.
> >>
> >> As Sakari said, having the field there just to ignore it
> >> seems like a bad idea.
> > 
> > I'm not sure to agree here, if we see no valid use case for that field,
> > why would it be a bad idea to ignore it ? That shouldn't affect anyone.
> 
> I'm fine with dropping the fsleep() here, but IMHO it should
> be replaced with a warning if it is missing do we want dev_warn
> or WARN_ON() here ?  Since setting it would be considered
> a driver bug I guess WARN_ON() ?

Sounds good to me.

> >> I think that this being abused is something to watch for during
> >> review, rather then enforcing it not being used in the CCI code.
> >>
> >>>>>> +	}
> >>>>>> +
> >>>>>> +	return 0;
> >>>>>> +}
> >>>>>> +EXPORT_SYMBOL_GPL(cci_multi_reg_write);
> >>>>>> +
> >>>>>> +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,
> >>>>>> +	};
> >>>>>> +
> >>>>>> +	return devm_regmap_init_i2c(client, &config);
> >>>>>> +}
> >>>>>> +EXPORT_SYMBOL_GPL(cci_regmap_init_i2c);
> >>>>>> +
> >>>>>> +MODULE_LICENSE("GPL");
> >>>>>> +MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
> >>>>>> diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..69b8a7c4a013
> >>>>>> --- /dev/null
> >>>>>> +++ b/include/media/v4l2-cci.h
> >>>>>> @@ -0,0 +1,109 @@
> >>>>>> +/* SPDX-License-Identifier: GPL-2.0 */
> >>>>>> +/*
> >>>>>> + * MIPI Camera Control Interface (CCI) register access helpers.
> >>>>>> + *
> >>>>>> + * Copyright (C) 2023 Hans de Goede <hansg@kernel.org>
> >>>>>> + */
> >>>>>> +#ifndef _V4L2_CCI_H
> >>>>>> +#define _V4L2_CCI_H
> >>>>>> +
> >>>>>> +#include <linux/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.
> >>>>>
> >>>>> It does, but at the same time, it prevents catching errors caused by
> >>>>> incorrect register macros. I'm tempted to consider that catching those
> >>>>> errors is more important.
> >>>>>
> >>>>>> + */
> >>>>>> +enum cci_reg_type {
> >>>>>> +	cci_reg_8 = 0,
> >>>>>> +	cci_reg_16,
> >>>>>> +	cci_reg_24,
> >>>>>> +	cci_reg_32,
> >>>>>> +};
> >>>>>> +
> >>>>>> +/*
> >>>>>> + * 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.
> >>>>>
> >>>>> Even if it's a no-op I'd prefer making its use mandatory. It makes
> >>>>> driver code more explicit, and eases catching issues during review.
> >>>>
> >>>> The problem is that almost all sensor drivers contain long list
> >>>> of register-address, -val pairs which they send to their own custom
> >>>> regmap_multi_reg_write()
> >>>>
> >>>> See e.g. the drivers/media/i2c/imx219.c (to stick with the imx
> >>>> theme from your imx290 request) this has a lot of quite long
> >>>> struct imx219_reg arrays with raw initializers.
> >>>>
> >>>> Often some or all of these registers in such list are
> >>>> undocumented (if we have access to a datasheet at all),
> >>>> so we simply don't know the register width.
> >>>>
> >>>> So arguably adding CCI_REG8(x) around all the addresses
> >>>> here is wrong, since this suggests we know the register
> >>>> width.
> >>>>
> >>>> With the current proposal to have 0 mean both unset and 8bit
> >>>> width this kinda register lists just work and converting
> >>>> the driver becomes just a matter of replacing e.g.
> >>>> imx219_write_regs() with cci_multi_reg_write().
> >>>>
> >>>> Where as otherwise we would need to add CCI_REG8(x)
> >>>> around the addresses which:
> >>>>
> >>>> a) Suggests we actually know the register width which
> >>>>    we often do not know at all
> >>>>
> >>>> b) causes a ton of needless churn
> >>>>
> >>>> so I would very much prefer to keep this as as and
> >>>> allow unmarked register addresses.
> >>>>
> >>>> As for the CCI_REG8(x) being useful as an annotation
> >>>> during review you are of course free to enforce its
> >>>> use during review. And note that I did use it for
> >>>> all the OV2680_REG_FOO defines in both ov2680 conversions.
> >>>>
> >>>> I do agree enforcing its use makes sense for individual
> >>>> register address defines. The reason to make it optional
> >>>> and the place where I want it to be optional is for
> >>>> the array of raw register-addr + initializer-val pairs
> >>>> case.
> >>>
> >>> For register arrays, I'm fine with that. For register macros, I don't
> >>> want to see
> >>>
> >>> #define MY_WELL_DEFINED_8B_REG		0x1234
> >>>
> >>> For those I want drivers to use CCI_REG8(). It seems we're on the same
> >>> page :-)
> >>
> >> Right, but if we want cci_multi_reg_write() to work with register
> >> arrays without the CCI_REG8() macros then the code needs to stay
> >> as is and we cannot enforce use of the macro by erroring out
> >> if it is not used.
> > 
> > Could we have an internal __cci_reg_write() that doesn't check the size,
> > and a cci_reg_write() wrapper that does ?
> 
> And then make cci_multi_reg_write() use the non checking version I presume?
> Yes that is possible / should work.
> 
> Or alternatively we could make drivers which use raw register init lists
> (with the reg-width macros) directly call regmap_multi_reg_write() since
> all register writes are expected to be 8 bit wide in that case directly
> calling regmap_multi_reg_write() should work fine too.
> 
> And then we can have all the cci_ prefixed versions always check
> that a reg-width has been specified, which would be more consistent.

Sounds good to me too :-)

-- 
Regards,

Laurent Pinchart