Hi,
On 8/2/23 21:37, Andy Shevchenko wrote:
> On Wed, Aug 2, 2023 at 8:31 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>> Instead of using a long fixed register settings list for each resolution,
>> calculate the register settings based on the requested width + height.
>>
>> This is based on atomisp-ov2680 commit 0611888592df ("media: atomisp:
>> ov2680: Make setting the modes algorithm based").
>>
>> This will allow future enhancements like adding hblank and vblank controls
>> and adding selection support.
>>
>> This also adds properly programming the ISP window and setting
>> the manual ISP window control bit in register 0x5708, this is
>> necessary for the hflip and vflip conrols to work properly.
>
> ...
>
>> struct ov2680_mode {
>> struct v4l2_mbus_framefmt fmt;
>> struct v4l2_fract frame_interval;
>
>> + bool binning;
>
> You might save a (few) byte(s) by moving this to be the last one.
I prefer to keep it here, it is not like there are going to be 100-s of these struct in memory (in reality there will be max 2 of these in memory) .
>> + u16 h_start;
>> + u16 v_start;
>> + u16 h_end;
>> + u16 v_end;
>> + u16 h_output_size;
>> + u16 v_output_size;
>> + u16 hts;
>> + u16 vts;
>> };
>
>
>> + sensor->mode.h_start = ((OV2680_NATIVE_WIDTH - width) / 2) & ~1;
>> + sensor->mode.v_start = ((OV2680_NATIVE_HEIGHT - height) / 2) & ~1;
>
> ~BIT(0) in both cases?
The start coordinates need to be a multiple of 2 to not change
the bayer order so this is rounding down to a multiple of 2.
Using ~BIT(0) makes that less clear IMHO, so I'm going to keep
this as is.
>
> ...
>
>> + width = min_t(unsigned int, ALIGN(format->format.width, 2),
>> + OV2680_NATIVE_WIDTH);
>> + height = min_t(unsigned int, ALIGN(format->format.height, 2),
>> + OV2680_NATIVE_HEIGHT);
>
> Wondering if you can switch these to use min() (with a strict type checking).
> It might require adding U/UL to the respective constants.
I'll try to switch to regular min() use here for v5.
Regards,
Hans
