Quoting Hans de Goede (2023-10-31 17:26:58)
> Hi,
>
> On 10/31/23 18:05, Alain Volmat wrote:
> > 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 ?
>
> Yes I believe so, Laurent, Kieran ?
I'm not 100% sure here, I think there are some IMX sensors with Little
Endian - and some with Big Endian ... because all the same would be too
easy.
I haven't seen a single device with mixed big and little endian yet
though.
> > 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(...).
>
> Hmm, that (CCI_REG16_LE etc) is an interesting proposal, that
> would avoid the need to add a struct with flags and if I understand
> things correctly then you would also not need any extra data
> on top of the regmap, right ?
>
> I did not take the mixed endian case for data registers into
> account yet. Since that apparently is a thing I think that
> your CCI_REG16_LE etc proposal is better then adding a struct
> with flags.
>
> Laurent, Kieran what do you think ?
Especially given there are some devices with multiple encodings, I like
this.
I think it can simplify things if we don't need a separate
struct v4l2_cci allocated too.
> > 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.
>
> If we are going to deal with mixed endianess with a flag encoded
> in the high bits of the register then I greatly favor just
> putting the encoding in the high bits and not having
> a default endianness + a flag for reverse endianess, that
> just feels wrong and the code to implement this will also
> be less then ideal.
>
> > Are you aware of other sensors having "mixed" endianness registers ?
>
> Nope this is all new to me.
I haven't seen any others yet - but I haven't been looking out for them
yet either. Most of the time all the writes are done as 8-bit writes
from tables. ... so this is all coming up new in a lot of cases I think.
--
Kieran
