[RFC] regmap_range_cfg usage with v4l2-cci

[RFC] regmap_range_cfg usage with v4l2-cci

[Alain Volmat]

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 ?

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 ?

Regards
Alain

Re: [RFC] regmap_range_cfg usage with v4l2-cci

[Hans de Goede]

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.

Regards,

Hans

Re: [RFC] regmap_range_cfg usage with v4l2-cci

[Hans de Goede]

On 10/31/23 10:46, Hans de Goede wrote:
> 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 ?

p.s.

To be clear if you like / 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 the IMX driver conversion can use the _ex variant.

Regards,

Hans

Re: [RFC] regmap_range_cfg usage with v4l2-cci

[Hans de Goede]

<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

Re: [RFC] regmap_range_cfg usage with v4l2-cci

[Alain Volmat]

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
> 

Re: [RFC] regmap_range_cfg usage with v4l2-cci

[Hans de Goede]

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

Re: [RFC] regmap_range_cfg usage with v4l2-cci

[Kieran Bingham]

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

Re: [RFC] regmap_range_cfg usage with v4l2-cci

[Hans de Goede]

Hi,

On 10/31/23 19:07, Kieran Bingham wrote:
> 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.

Ok, lets go with (CCI_REG16_LE etc) then for the IMX sensors
and for the mixed endianess in the GalaxyCore sensors

Regards,

Hans

Re: [RFC] regmap_range_cfg usage with v4l2-cci

[Laurent Pinchart]

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

Re: [RFC] regmap_range_cfg usage with v4l2-cci

[Hans de Goede]

Hi Laurent,

On 11/1/23 13:41, Laurent Pinchart wrote:
> 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.

I actually like this approach, it allows for mixed endianness for
the data registers and it means we don't need to have a struct v4l2_cci
to store a global (whole sensor) endianness lfag.

I don't think adding CCI_REGXX_LE() macro usage in drivers which
have LE data registers is too much of a hassle. This is a one
time thing and it is only 3 extra chars per register definition.

Regards,

Hans