Hello,
On Tue, Oct 31, 2023 at 06:26:58PM +0100, Hans de Goede wrote:
> On 10/31/23 18:05, Alain Volmat wrote:
> > 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 ?
>
> > 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 ?
>
> > 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.
I'm in two minds about that. It's annoying to have to mark every single
register with _LE, but at the same time, these are CCI helpers, so
making life more difficult for LE sensors could be considered
reasonable. Except, of course, that it will make life more difficult for
us, not for the sensor vendors. We need a way to share the pain, but
that's another discussion.
We could also consider that the few GC2145 registers that use a
different endianness should be handled as 8-bit registers by CCI, and
managed by the gc2145 driver. If this situation is very rare, it may not
be worth it trying to handle it in v4l2-cci if it makes life more
complicated for everybody.
> > Are you aware of other sensors having "mixed" endianness registers ?
>
> Nope this is all new to me.
--
Regards,
Laurent Pinchart
