Hi Dan,
On 7/4/23 11:22, Dan Scally wrote:
> Morning Hans
>
> On 27/06/2023 15:18, Hans de Goede wrote:
>> Add read-only link-freq and pixel-rate controls. This is necessary for
>> the sensor to work with the ipu3-cio2 driver and for libcamera.
>>
>> Acked-by: Rui Miguel Silva <rmfrfs@xxxxxxxxx>
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
>> drivers/media/i2c/ov2680.c | 23 +++++++++++++++++++++++
>> 1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
>> index 8bc542df1890..95d3152ddd22 100644
>> --- a/drivers/media/i2c/ov2680.c
>> +++ b/drivers/media/i2c/ov2680.c
>> @@ -75,6 +75,12 @@
>> #define OV2680_MIN_CROP_WIDTH 2
>> #define OV2680_MIN_CROP_HEIGHT 2
>> +/* Fixed pre-div of 1/2 */
>> +#define OV2680_PLL_PREDIV0 2
>> +
>> +/* Pre-div configurable through reg 0x3080, left at its default of 0x02 : 1/2 */
>> +#define OV2680_PLL_PREDIV 2
>> +
>> /* 66MHz pixel clock: 66MHz / 1704 * 1294 = 30fps */
>> #define OV2680_PIXELS_PER_LINE 1704
>> #define OV2680_LINES_PER_FRAME 1294
>> @@ -118,6 +124,8 @@ struct ov2680_ctrls {
>> struct v4l2_ctrl *hflip;
>> struct v4l2_ctrl *vflip;
>> struct v4l2_ctrl *test_pattern;
>> + struct v4l2_ctrl *link_freq;
>> + struct v4l2_ctrl *pixel_rate;
>> };
>> struct ov2680_mode {
>> @@ -145,6 +153,8 @@ struct ov2680_dev {
>> struct clk *xvclk;
>> u32 xvclk_freq;
>> u8 pll_mult;
>> + s64 link_freq[1];
>> + s64 pixel_rate;
>> struct regulator_bulk_data supplies[OV2680_NUM_SUPPLIES];
>> struct gpio_desc *pwdn_gpio;
>> @@ -906,6 +916,12 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
>> ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN,
>> 0, 1023, 1, 250);
>> + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ,
>> + 0, 0, sensor->link_freq);
>> + ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE,
>> + 0, sensor->pixel_rate,
>> + 1, sensor->pixel_rate);
>> +
>> if (hdl->error) {
>> ret = hdl->error;
>> goto cleanup_entity;
>> @@ -913,6 +929,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
>> ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>> ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>> + ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> sensor->sd.ctrl_handler = hdl;
>> @@ -1030,6 +1047,12 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
>> sensor->pll_mult = ov2680_pll_multipliers[i];
>> + sensor->link_freq[0] = sensor->xvclk_freq / OV2680_PLL_PREDIV0 /
>> + OV2680_PLL_PREDIV * sensor->pll_mult;
>> +
>> + /* CSI-2 is double data rate, bus-format is 10 bpp */
>> + sensor->pixel_rate = sensor->link_freq[0] * 2 / 10;
>
>
> I'm a little unsure on this one. My understanding is that the link frequency really ought to come from the endpoint properties (which in our case would be set by the ipu-bridge; though it doesn't for this sensor at the moment because I didn't understand it properly back then) because it's a platform specific thing. What the value should be, I have been determining by reading the PLL settings for the sensor whilst the laptop's running Windows. So whilst this is probably technically fine in supporting the link frequency that the driver already expects to configure for whatever platform this was originally designed for, my guess would be that the Miix expects a different link frequency and ideally we'd support that instead. For example see these commits for the ov7251:
The datasheet is clear that the ov2680 is intended to be used with
a fixed pixelclock of 66 MHz:
"2.2 architecture
The OV2680 sensor core generates streaming pixel data at a constant
frame rate to a pixel clock of 66 MHz."
and the ov2680 always operates in single lane mode. So there really is
not much to configure here.
Also the datasheet only contains a rudimentary description of the PLL,
which is not really enough to write a function to configure the PLL for
arbitrary link-frequencies.
The adjustment to make the sensor work with a 19.2MHz xvclk instead
of the default 24 MHz comes from the atomisp code. How to make other
adjustments would pretty much be guess work.
I guess we could add code to check the link-frequencies and check
that there is only 1 and it matches the expected 330 MHz then the driver
still honors the link-frequencies property while at the same time
sticking with the fixed setup the sensor is intended to be used with.
Would adding a link-frequency check like that work for you ?
And if yes what should the link-frequency control return then,
the actual achieved frequency (this would be better IMHO) or
the one from the property ?
Regards,
Hans
