Hi Sakari, On 9/26/22 11:42, Sakari Ailus wrote: [...] >>>>>> + 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. > Yes this is hardware limited. I'll send v6 soon. Thanks a ton. >> >>> >>> ... >>> >>>>>> + 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. >
