Hi Tomasz,
Please see my comments.
-----Original Message-----
>Hi Ping-chung,
>On Wed, Aug 1, 2018 at 11:31 AM Ping-chung Chen <ping-chung.chen@xxxxxxxxx> wrote:
>
> From: "Chen, Ping-chung" <ping-chung.chen@xxxxxxxxx>
>
> Add a V4L2 sub-device driver for the Sony IMX208 image sensor.
> This is a camera sensor using the I2C bus for control and the
> CSI-2 bus for data.
>
> Signed-off-by: Ping-Chung Chen <ping-chung.chen@xxxxxxxxx>
> ---
> since v1:
> -- Update the function media_entity_pads_init for upstreaming.
> -- Change the structure name mutex as imx208_mx.
> -- Refine the control flow of test pattern function.
> -- vflip/hflip control support (will impact the output bayer order)
> -- support 4 bayer orders output (via change v/hflip)
> - SRGGB10(default), SGRBG10, SGBRG10, SBGGR10
> -- Simplify error handling in the set_stream function.
> since v2:
> -- Refine coding style.
> -- Fix the if statement to use pm_runtime_get_if_in_use().
> -- Print more error log during error handling.
> -- Remove mutex_destroy() from imx208_free_controls().
> -- Add more comments.
>Thanks for addressing the comments. There are still few remaining, though. Please see inline.
[snip]
> +enum {
> + IMX208_LINK_FREQ_384MHZ_INDEX,
> + IMX208_LINK_FREQ_96MHZ_INDEX,
> +};
> +
> +/*
> + * pixel_rate = link_freq * data-rate * nr_of_lanes / bits_per_sample
> + * data rate => double data rate; number of lanes => 2; bits per
> +pixel => 10 */ static u64 link_freq_to_pixel_rate(u64 f) {
> + f *= IMX208_DATA_RATE_DOUBLE * IMX208_NUM_OF_LANES;
> + do_div(f, IMX208_PIXEL_BITS);
> +
> + return f;
> +}
> +
> +/* Menu items for LINK_FREQ V4L2 control */ static const s64
> +link_freq_menu_items[] = {
> + IMX208_LINK_FREQ_384MHZ,
> + IMX208_LINK_FREQ_96MHZ,
>I asked for having explicit indices using IMX208_LINK_FREQ_*_INDEX enum used here too.
Sorry I forgot this one, fixed.
> +};
> +
> +/* Link frequency configs */
> +static const struct imx208_link_freq_config link_freq_configs[] = {
> + [IMX208_LINK_FREQ_384MHZ_INDEX] = {
> + .pixels_per_line = IMX208_PPL_384MHZ,
> + .reg_list = {
> + .num_of_regs = ARRAY_SIZE(pll_ctrl_reg),
> + .regs = pll_ctrl_reg,
> + }
> + },
> + [IMX208_LINK_FREQ_96MHZ_INDEX] = {
> + .pixels_per_line = IMX208_PPL_96MHZ,
> + .reg_list = {
> + .num_of_regs = ARRAY_SIZE(pll_ctrl_reg),
> + .regs = pll_ctrl_reg,
> + }
> + },
> +};
> +
> +/* Mode configs */
> +static const struct imx208_mode supported_modes[] = {
> + [IMX208_LINK_FREQ_384MHZ_INDEX] = {
>This is not the right index for this array (even if numerically the same. This array is just a list of available modes (resolutions) and there is no other data structure referring to particular entries of it, so it doesn't need explicit indexing.
Done.
> + .width = 1936,
> + .height = 1096,
> + .vts_def = IMX208_VTS_60FPS,
> + .vts_min = IMX208_VTS_60FPS_MIN,
> + .reg_list = {
> + .num_of_regs = ARRAY_SIZE(mode_1936x1096_60fps_regs),
> + .regs = mode_1936x1096_60fps_regs,
> + },
> + .link_freq_index = IMX208_LINK_FREQ_384MHZ_INDEX,
> + },
> + [IMX208_LINK_FREQ_96MHZ_INDEX] = {
>Ditto.
Best regards,
Tomasz
