<resend with Alain added to the To: for some reason reply-to-all did not add Alain>
Hi Alain,
On 10/30/23 18:36, Alain Volmat wrote:
> Hi,
>
> Goal of this email is to get first comments prior to posting a patch.
>
> Could we consider enhancements within the v4l2-cci in order to also
> allow regmap_range_cfg usage for paged register access ?
Yes definitely.
Extending v4l2-cci for other use cases was already briefly discussed
between Kieran (Cc-ed) and me:
The CCI part of the MIPI CSI spec says that multi-byte registers are
always in big endian format, but some of the Sony IMX sensors actually
use little-endian format for multi-byte registers.
The main reason why we need v4l2-cci and cannot use regmap directly is
because of the variable register width in CCI, where as regmap only
supports a single width. v4l2 cci uses 8 bits width in the underlying
regmap-config and then takes care of multy-byte registers by e.g.
reading multiple bytes and calling e.g. get_unaligned_be16() on
the read bytes.
For the IMX scenario the plan is to add the notion of v4l2-cci
flags by adding this to include/media/v4l2-cci.h :
struct v4l2_cci {
struct regmap *map;
long flags;
}
And then change the prototype for devm_cci_regmap_init_i2c() to:
struct v4l2_cci *devm_cci_regmap_init_i2c(struct i2c_client *client,
int reg_addr_bits, long flags);
And have devm_cci_regmap_init_i2c():
1. devm_kmalloc() a struct v4l2_cci
2. store the regmap there
3. copy over flags from the function argument
Combined with modifying all the other functions to take
"struct v4l2_cci *cci" as first argument instead of
"struct regmap *map".
This change will require all existing sensor drivers using
v4l2-cci to be converted for the "struct regmap *map" ->
"struct v4l2_cci *cci" change, this all needs to be done
in one single commit adding the new struct + flags argument
to avoid breaking the compilation.
Then once we have this a second patch can add:
/* devm_cci_regmap_init_i2c() flags argument defines */
#define V4L2_CCI_DATA_LE BIT(0)
to include/media/v4l2-cci.h and make v4l2-cci.h honor
this flag solving the IMX scenario.
We need to make this change sooner rather then later,
while we only still have a few sensor drivers using
v4l2-cci.
So back to your question yes extensions are welcome
and we already have one planned. If we are going to do
more extensions though, then I really would want to see
the little-endian data plan get implemented first, having
our own struct v4l2_cci should help with future extensions
were we can then just add more fields to it if necessary.
I'm sorry about asking you to implement this first before
being able to solve your own problem, but this should be
relatively KISS to implement and I can test the patches
for you for at least some of the sensor drivers.
> At least two drivers currently being upstream and using v4l2-cci infrastructure
> could benefit from regmap_range_cfg.
> The GC0308 driver is partially using v4l2-cci and partially regmap (in order to use
> regmap_range_cfg) and the GC2145 driver is using v4l2-cci but doing paging manually.
>
> The function devm_cci_regmap_init_i2c is already taking as parameter one argument
> reg_addr_bits to be used in the regmap_config structure. We could also add
> regmap_range_cfg pointer and size arguments to the function or
> alternatively add another init function with more arguments ?
I think adding a devm_cci_regmap_init_i2c_ex() would make sense here, this
could already be done when adding the flags argument, giving only
devm_cci_regmap_init_i2c_ex() the flags argument. For just the flags argument
having a _ex seems overkill but if we are going to add regmap_range_cfg pointer
and size arguments too then I think an _ex makes sense.
And then in v4l2-cci.c only have the _ex and have a static inline helper
in v4l2-cci-h defining the non _ex version ?
Note this devm_cci_regmap_init_i2c_ex() variant is just an idea /
suggestion I'm open to discussion about that.
To be clear if you plan to implement the devm_cci_regmap_init_i2c_ex()
variant, then this should be done in the first patch adding the:
struct v4l2_cci {
struct regmap *map;
long flags;
};
bits, so that we don't have to add an extra 0 argument for the flags to
all the existing callers of devm_cci_regmap_init_i2c() in that patch.
And then a future IMX driver conversion can use the _ex variant.
Regards,
Hans
