Hi Sakari,
Thanks for your input.
> Where's the corresponding DT binding patch? Ideally it would be part of the
> same set.
Well I've sent it a moment before this one. Will make them a set next
time.
>> +#define AR0521_WIDTH_BLANKING_MIN 572u
>> +#define AR0521_HEIGHT_BLANKING_MIN 28u // must be even
>
> Please use /* */ for comments. The SPDX tag is an exception.
As far as I know, this is no longer the case, the C99 comments are now
permitted and maybe even encouraged. Or was I dreaming?
checkpatch doesn't protest either.
> Please wrap your lines at 80 or earlier, unless a sound reason exists to do
> otherwise.
This limitation appears to be lifted as well, after all those years.
Is there a specific reason to still use it here? Yes, lines longer than
80 chars make the code much more readable (for my eyes, at least).
Yes, I know there is some "soft" limit, and I trim lines when it makes
them better in my opinion.
>> +static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> + struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
>> + struct ar0521_dev *sensor = to_ar0521_dev(sd);
>> + int ret;
>> +
>> + // v4l2_ctrl_lock() locks our own mutex
>> +
>> + dev_dbg(&sensor->i2c_client->dev, "%s(0x%X)\n", __func__, ctrl->id);
>
> Please make sure the sensor is powered on before accessing it. See e.g.
> imx219_set_ctrl() how to do this.
I do, the lower-level IO function ar0521_write_regs() checks for this.
However, I identified a case when pm_runtime_* isn't available
(CONFIG_*), so I will post corrected patch.
> Please drop the s_power callback and rely on runtime PM.
Ok
> Please drop *frame_interval() callbacks. See
> https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html for an
> explanation.
>
> Let me know if you have questions.
I already wrote about this, you must have missed it:
...However, it apparently isn't as flexible as *frame_interval() -
I can't control the precise timings:
- the V4L2_CID_PIXEL_RATE is discrete and R/O (i.e. the application
can't control it)
- even if I could somehow control pixel rate, frame interval is
calculated as (width + h_blanking) * (height + v_blanking) /
pixel_rate, which may be a bit coarse for precise video.
With *frame_interval(), I compensate with per-frame "extra" delay
(in single pixels, not whole H or V lines).
If the (userspace) application can control pixel rate and the "extra"
timing (well maybe pixel rate and the total number of pixels including
blanking and "extra") then I would be more than happy dropping
frame_interval().
I guess I could easily do that myself, if there is consensus about it.
E.g. V4L2_CID_PIXEL_RATE would not be forced R/O (and discrete) anymore
and I would invent a V4L2_CID_TOTAL_PIXELS or something.
The V4L2_CID_PIXEL_RATE issue may be somehow offset by the
V4L2_CID_LINK_FREQ, but the latter is "menu" type and thus not very
useful (am I to populate it with 250 values 1 MHz apart?).
Perhaps the receiver could publish it's input frequency range instead,
then the transmitter would set a fixed value? I don't know. And this
doesn't cover a case where the user needs a slower rate than max(tx, rx)
for some reason.
We should decide something about this, though.
I look forward for your comments,
--
Krzysztof "Chris" Hałasa
Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
