Re: [PATCH 2/3] media: atomisp: ov2680: Convert to new CCI register access helpers

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

Hi,

On 6/7/23 18:05, Laurent Pinchart wrote:

<snip>

>> -static struct ov2680_reg const ov2680_global_setting[] = {
>> +static const struct reg_sequence ov2680_global_setting[] = {
> 
> Maybe you could move this to the .c file and also move the v4l2_cci.h
> inclusion to the .c file ? Or this can be done on top, as I assume
> you'll merge the .h and .c files at some point anyway.

Actually since there already is a (quite old, somewhat broken)
driver/media/i2c/ov2680.c and since the atomisp code can work
with standard v4l2 sensor drivers now I have been porting all
the recent work don on atomisp-ov2680.c to driver/media/i2c/ov2680.c
(to avoid causing regressions for potential users of that).

As I mentioned elsewhere in the thread:

"""
So I'm not even sure if this patch should be merged, as I mentioned in the cover letter this one is mostly here to illustrate use of the new helpers.

I also wrote this patch to make porting recent atomisp-ov2680.c changes over to drivers/media/i2c/ov2680.c easier. Part of the series to get drivers/media/i2c/ov2680.c into shape is converting it to the new CCI helpers so that I could then easily copy over bits from the also converted atomisp-ov2680.c.

So it might be interesting to still merge this so that the latest state of atomisp-ov2680.c is easier to compare to drivers/media/i2c/ov2680.c if the need arises.
"""

So rather then merging the .h into .c the next patch I expect to
write for the atomisp-ov2680.c driver is a patch to drop it from
the kernel since it will be superseded by a proper standard
v4l2 sensor driver then :)

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thank you.

Regards,

Hans





> 
>>  	/* MIPI PHY, 0x10 -> 0x1c enable bp_c_hs_en_lat and bp_d_hs_en_lat */
>>  	{0x3016, 0x1c},
>>  
>> @@ -242,8 +181,6 @@ static struct ov2680_reg const ov2680_global_setting[] = {
>>  
>>  	/* DPC THRE RATIO 0x04 (4) -> 0x00 (0) */
>>  	{0x5792, 0x00},
>> -
>> -	{}
>>  };
>>  
>>  #endif
>