Hi Sakari, Thanks a lot for your answer. On 9/25/22 13:32, Sakari Ailus wrote: > Hi Benjamin, > > On Fri, Aug 26, 2022 at 04:12:22PM +0200, Benjamin MUGNIER wrote: >>>> + int rot_term; >>> >>> rot_term value is always the same. No need for a field here for that. >>> >> >> It changes according to the sensor's model id and could be either >> VGX761_SHORT_ROT_TERM or VGX661_SHORT_ROT_TERM. See the >> vgxy61_fill_sensor_param function. > > Ah, thanks for the explanation. I somehow had missed that earlier. > >>>> + struct i2c_client *client = sensor->i2c_client; >>>> + /* Correction factor for time required between 2 lines */ >>>> + const int mipi_margin = 1056; >>> >>> What does this value signify? Is it really constant or somehow dependent by >>> the configuration set by the driver? >> >> It is a simplification of time needed to send short packets and for the MIPI >> to add transition times (EoT, LPS, and SoT packet delimiters) needed by the >> protocol to go in low power between 2 packets of data. This is a mipi IP >> constant for the sensor. > > Ok. A comment elaborating this as above would be nice. > No problem, queued for v6. >>>> + ctrl = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, ARRAY_SIZE(link_freq) - 1, 0, >>>> + link_freq); >>>> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; >>>> + /* Custom controls */ >>>> + v4l2_ctrl_new_custom(hdl, &vgxy61_hdr_ctrl, NULL); >>>> + /* Keep a pointer to these controls as we need to update them when setting the format */ >>>> + sensor->pixel_rate_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE, 1, INT_MAX, 1, >>>> + get_pixel_rate(sensor)); >>>> + sensor->pixel_rate_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; >>>> + sensor->expo_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, sensor->expo_min, >>>> + sensor->expo_max, 1, sensor->expo_long); >>>> + sensor->vblank_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, sensor->vblank_min, >>>> + 0xffff - sensor->current_mode->crop.height, 1, >>>> + sensor->vblank); >>>> + sensor->vflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, sensor->vflip); >>>> + sensor->hflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, sensor->hflip); >>> >>> You don't seem to have the link frequency control. I suppose you should, as >>> well as have it in DT. >>> >>> <URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html#handling-clocks> >>> >> >> Are you referring to V4L2_CID_LINK_FREQ? I defined it above as a read only >> control as it is fixed for this sensor. > > Ah, it was of course the first control. > > Anyway, in general case, the frequencies that can be used on the board > should come from DT, as using other frequencies may interfere with other > devices in the same system. Ah I just remembered we talked about this with Laurent on v2: https://lore.kernel.org/linux-media/YmA5eTOoGNEbMyKB@xxxxxxxxxxxxxxxxxxxxxxxxxx/ Here is the relevant part: >>> The link frequencies should come from DT. >> >> This is a hard requirement for this sensor. This value can not be >> modified and must be 402MHz. >> >> I see the ov9734.c does something similar. Is adding this value to the >> device tree really mandatory? > > I suppose it's fine then. Sakari, what do you think ? With this extra information, should I move the link frequency to the device tree or is it fine this way? > > ... > >>>> + sensor->slave_mode = of_property_read_bool(dev->of_node, "slave-mode"); >>> >>> What does this actually do? >>> >>> On parallel bus the slave mode tells the synchronisation signals originate >>> from the receiver. On CSI-2 there are no specific synchronisation signals. >>> >> >> We discussed about this on v1 a while ago: >> >> https://lore.kernel.org/all/c610a2c9-31b1-1950-00fa-a6b3fd3517a1@xxxxxxxxxxx/ >> >> Tell me if there is any changes to be made, or maybe some documentation to >> clarify? > > Ah, yes, I hadn't had time to read that reply yet. :-( > > I think we should have a generic solution for this. You may not have two > similar kinds of sensors you're synchronising this way. One option could be > to drop this functionality from the driver now to get it in sooner. > > Something to consider: > > - how to convey where the signal comes from (phandle), > > - capture start signal polarity, > > - whether it's input/output and > > - pull-down / pull-up configuration? > > These are similar to what GPIOs have. Hardware support may vary of course. > Yes, we discussed this in the media summit. I'll drop this for now. I'll come back to this once the first version of the driver is accepted, and if Dave is not faster than me on this topic ;)
