Hi Tomasz, On Fri, Mar 23, 2018 at 08:43:50PM +0900, Tomasz Figa wrote: > Hi Andy, > > Some issues found when reviewing cherry pick of this patch to Chrome > OS kernel. Please see inline. > > On Sat, Mar 17, 2018 at 1:38 AM, Andy Yeh <andy.yeh@xxxxxxxxx> wrote: > > [snip] > > > + case V4L2_CID_VBLANK: > > + /* > > + * Auto Frame Length Line Control is enabled by default. > > + * Not need control Vblank Register. > > + */ > > What is the meaning of this control then? Should it be read-only? The read-only flag is for the uAPI; the control framework still passes through changes to the control value done using kAPI to the driver. > > > + break; > > + default: > > + dev_info(&client->dev, > > + "ctrl(id:0x%x,val:0x%x) is not handled\n", > > + ctrl->id, ctrl->val); > > + ret = -EINVAL; > > + break; > > + } > > + > > + pm_runtime_put(&client->dev); > > + > > + return ret; > > + > > [snip] > > > + v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &imx258_ctrl_ops, > > + V4L2_CID_TEST_PATTERN, > > + ARRAY_SIZE(imx258_test_pattern_menu) - 1, > > + 0, 0, imx258_test_pattern_menu); > > There is no code for handling this control in imx258_s_ctrl(). It's > not a correct behavior to register a control, which isn't handled by > the driver. Please either implement the control completely or remove > it. Indeed. -- Kind regards, Sakari Ailus sakari.ailus@xxxxxxxxxxxxxxx
