Hi Benjamin, On Mon, Sep 26, 2022 at 11:28:59AM +0200, Benjamin MUGNIER wrote: > 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? If the hardware can only use a single frequency (i.e. it's not a driver limitation), then sure, no need to put this in DT. > > > > > ... > > > >>>> + 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 ;) Sounds good. -- Kind regards, Sakari Ailus
