Hi Bingbu,
On Tue, Sep 25, 2018 at 05:10:59PM +0800, Bing Bu Cao wrote:
...
> >>>>> +/* Initialize control handlers */
> >>>>> +static int imx319_init_controls(struct imx319 *imx319)
> >>>>> +{
> >>>>> + struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
> >>>>> + struct v4l2_ctrl_handler *ctrl_hdlr;
> >>>>> + s64 exposure_max;
> >>>>> + s64 vblank_def;
> >>>>> + s64 vblank_min;
> >>>>> + s64 hblank;
> >>>>> + s64 pixel_rate;
> >>>>> + const struct imx319_mode *mode;
> >>>>> + int ret;
> >>>>> +
> >>>>> + ctrl_hdlr = &imx319->ctrl_handler;
> >>>>> + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
> >>>>> + if (ret)
> >>>>> + return ret;
> >>>>> +
> >>>>> + ctrl_hdlr->lock = &imx319->mutex;
> >>>>> + imx319->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx319_ctrl_ops,
> >>>>> + V4L2_CID_LINK_FREQ, 0, 0,
> >>>>> + imx319->hwcfg->link_freqs);
> >>>> Could you check that the link frequency matches with what the register
> >>>> lists assume?
> >>> Sakari, do you mean associate link frequency index with register list?
> > The driver should only allow using link frequencies that are explicitly
> > allowed for the system.
> Sakari, as current driver only support one link frequency, so I think once getting the link frequencies from firmware,
> driver can simply check and match the values with link_freq_menu_items[0] and only keep 1 item in the menu:
Please wrap the lines at around 76 characters, please.
>
> max = ARRAY_SIZE(link_freq_menu_items) - 1;
> imx319->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx319_ctrl_ops,
> V4L2_CID_LINK_FREQ, max, 0,
> link_freq_menu_items);
> Is it OK?
"max" would be 0 in this case, I presume. Seems fine to me.
--
Regards,
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
