Hi Tomasz, Thanks for the comments. OKAY as inline. Please check the Patch V8. Regards, Andy -----Original Message----- From: Tomasz Figa [mailto:tfiga@xxxxxxxxxxxx] Sent: Monday, March 12, 2018 3:33 PM To: Yeh, Andy <andy.yeh@xxxxxxxxx> Cc: Linux Media Mailing List <linux-media@xxxxxxxxxxxxxxx>; Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>; Chen, JasonX Z <jasonx.z.chen@xxxxxxxxx>; Chiang, AlanX <alanx.chiang@xxxxxxxxx>; Lai, Jim <jim.lai@xxxxxxxxx> Subject: Re: [PATCH v7] media: imx258: Add imx258 camera sensor driver Hi Andy, On Mon, Mar 12, 2018 at 1:01 AM, Andy Yeh <andy.yeh@xxxxxxxxx> wrote: >> Add a V4L2 sub-device driver for the Sony IMX258 image sensor. >> This is a camera sensor using the I2C bus for control and the >> CSI-2 bus for data. >> >> Signed-off-by: Jason Chen <jasonx.z.chen@xxxxxxxxx> >> Signed-off-by: Alan Chiang <alanx.chiang@xxxxxxxxx> >> --- >> since v2: >> -- Update the streaming function to remove SW_STANDBY in the beginning. >> -- Adjust the delay time from 1ms to 12ms before set stream-on register. >> since v3: >> -- fix the sd.entity to make code be compiled on the mainline kernel. >> since v4: >> -- Enabled AG, DG, and Exposure time control correctly. >> since v5: >> -- Sensor vendor provided a new setting to fix different CLK issue >> -- Add one more resolution for 1048x780, used for VGA streaming since >> v6: >> -- improved i2c read/write function to support writing 2 registers >> -- modified i2c reg read/write function with a more portable way >> -- utilized v4l2_find_nearest_size instead of the local find_best_fit >> function >> -- defined an enum for the link freq entries for explicit indexing > >Thanks for the patch. Looks almost good now. Just two new comments inline. > >[snip] >> + /* Set Orientation be 180 degree */ >> + ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL, >> + IMX258_REG_VALUE_08BIT, REG_CONFIG_MIRROR_FLIP); >> + if (ret) { >> + dev_err(&client->dev, "%s failed to set orientation\n", >> + __func__); >> + return ret; >> + } >> + >> + /* Apply customized values from user */ >> + ret = __v4l2_ctrl_handler_setup(imx258->sd.ctrl_handler); >> + if (ret) >> + return ret; >> + >> + /* >> + * Per sensor datasheet: >> + * These are the minimum 12ms delays for accessing the sensor through >> + * I2C and enabling streaming after lifting the device from reset. >> + */ >> + usleep_range(12000, 13000); > >Hmm, the code above already accesses the sensor through I2C. If this works, is this sleep perhaps already done by ACPI code or it is only needed for streaming? > Okay. usleep removed since confirmed sufficient delay implemented in coreboot. Stress test passed. >[snip] >> +/* Initialize control handlers */ >> +static int imx258_init_controls(struct imx258 *imx258) { >> + struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); >> + struct v4l2_ctrl_handler *ctrl_hdlr; >> + s64 exposure_max; >> + s64 vblank_def; >> + s64 vblank_min; >> + s64 pixel_rate_min; >> + s64 pixel_rate_max; >> + int ret; >> + >> + ctrl_hdlr = &imx258->ctrl_handler; >> + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8); >> + if (ret) >> + return ret; >> + >> + mutex_init(&imx258->mutex); >> + ctrl_hdlr->lock = &imx258->mutex; >> + imx258->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, >> + &imx258_ctrl_ops, >> + V4L2_CID_LINK_FREQ, >> + ARRAY_SIZE(link_freq_menu_items) - 1, >> + 0, >> + link_freq_menu_items); >> + >> + if (!imx258->link_freq) { >> + ret = -EINVAL; >> + goto error; >> + } >> + >> + imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > >nit: As discussed earlier with Sakari, I think we agreed on making this, and other controls that need dereferencing here, as follows: > > imx258->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, > &imx258_ctrl_ops, > V4L2_CID_LINK_FREQ, > ARRAY_SIZE(link_freq_menu_items) - 1, > 0, > link_freq_menu_items); > if (imx258->link_freq) > imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > >The error will be reported at the end of this function, through the check for ctrl_hdlr->error. > > OKAY Best regards, Tomasz
