Re: [RFC] regmap_range_cfg usage with v4l2-cci

[Hans de Goede <hdegoede@redhat.com>]

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 ?

> 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.

> Are you aware of other sensors having "mixed" endianness registers ?

Nope this is all new to me. 

Regards,

Hans