Re: [PATCH 2/4] media: i2c: Add driver for Sony IMX728

["Sebastian LaVine" <slavine@d3embedded.com>]

On Thu Feb 13, 2025 at 5:19 AM EST, Laurent Pinchart wrote:
>
> ...
>
>> +static const struct cci_reg_sequence imx728_wdr_12bit_3856x2176[] = {
>
> This table is way too big, with over 8000 entries. Some are even
> duplicated, with identical or different values for the same register. It
> will take more than a second at 400kHz to program this.
>
> At the very least I would expect a way to compact the table and make use
> of I2C register address auto-increment. Default power-up values should
> also likely be just dropped.
>
> I haven't checked in details, but doesn't this table also contain tuning
> data for your specific camera ?
>

In my testing, it takes around two seconds to write this table to the sensor.

I can investigate how to condense the table further, though the
registers for this sensor are more complex than just writing values to
addresses. The meaning of certain address writes depend on previous
writes -- thus the "duplicated" writes you mentioned.

I do not believe this table contains tuning information for our camera
module in particular.

> [snip]
>
>> +};
>
> [snip]
>
>> +static int imx728_get_frame_interval(struct v4l2_subdev *sd,
>> +                                    struct v4l2_subdev_state *sd_state,
>> +                                    struct v4l2_subdev_frame_interval *fi)
>> +{
>> +       struct imx728 *imx728 = to_imx728(sd);
>> +
>> +       fi->interval.numerator = 1;
>> +       fi->interval.denominator = imx728->fps;
>> +       return 0;
>> +}
>> +
>> +static int imx728_set_frame_interval(struct v4l2_subdev *sd,
>> +                                    struct v4l2_subdev_state *sd_state,
>> +                                    struct v4l2_subdev_frame_interval *fi)
>> +{
>> +       struct imx728 *imx728 = to_imx728(sd);
>> +       u32 req_fps;
>> +
>> +       mutex_lock(&imx728->lock);
>> +
>> +       if (fi->interval.numerator == 0 || fi->interval.denominator == 0) {
>> +               fi->interval.denominator = IMX728_FRAMERATE_DEFAULT;
>> +               fi->interval.numerator = 1;
>> +       }
>> +
>> +       req_fps = clamp_val(DIV_ROUND_CLOSEST(fi->interval.denominator,
>> +                                             fi->interval.numerator),
>> +                           IMX728_FRAMERATE_MIN, IMX728_FRAMERATE_MAX);
>> +
>> +       fi->interval.numerator = 1;
>> +       fi->interval.denominator = req_fps;
>> +
>> +       imx728->fps = req_fps;
>> +
>> +       mutex_unlock(&imx728->lock);
>> +       dev_dbg(imx728->dev, "%s frame rate = %d\n", __func__, imx728->fps);
>> +
>> +       return 0;
>> +}
>
> The frame rate on raw sensors is controlled through h/v blanking. You
> can drop thse functions, especially given that imx728->fps isn't used
> anywhere else.

Okay, I will drop these functions in v4. Thanks.

--
Sebastian

Please be aware that this email includes email addresses outside of the organization.