Hi Hans,
On Tue, Oct 31, 2023 at 10:53:16AM +0100, Hans de Goede wrote:
> <resend with Alain added to the To: for some reason reply-to-all did not add Alain>
No pb, I also received it via the mailing-list ;-)
>
> 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.
I understand that in case of IMX sensors, ALL the multi-registers
value are encoded in little-endian right ? In case of the GalaxyCore
GC2145, most of the registers (page 0 / 1 and 2) are correctly
encoded in big-endian, however page 3 (MIPI configuration) are
2 or 3 registers in little-endian. So far maybe this is minor
case, but the approach of having the endianness part of the v4l2_cci
struct wouldn't allow to address such case ?
Originally I thought we could have CCI_REG macros for little endian
as well, such as CCI_REG16_LE etc etc since we anyway still have spare
space I guess on top of the width part. Drawback is that in drivers
for IMX we would end-up with longer macros CCI_REG16_LE(...) instead
of CCI_REG16(...).
Or maybe as you proposed we can have the "default" encoding described
in the flags variable and have a CCI_REG16_REV or any other naming
just to indicate that for THAT precise register the endianess is not
the default one.
Are you aware of other sensors having "mixed" endianness registers ?
>
> 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.
Yep, agreed, devm_cci_regmap_init_i2c_ex sounds like a good name to me.
Let me prepare this and I'll post that in few days.
Regards,
Alain
>
> Regards,
>
> Hans
>
