Thanks Sakari to remind. Since I would like to attach some snips from the datasheets. But blocked, I will re-edit to plain table and hope it could be understood. Regards, Andy From: Yeh, Andy Sent: Friday, January 12, 2018 3:31 PM To: 'Sakari Ailus' <sakari.ailus@xxxxxxxxxxxxxxx> Cc: 'linux-media@xxxxxxxxxxxxxxx' <linux-media@xxxxxxxxxxxxxxx>; AlanX Chiang (alanx.chiang@xxxxxxxxx) <alanx.chiang@xxxxxxxxx>; JasonX Z Chen (jasonx.z.chen@xxxxxxxxx) <jasonx.z.chen@xxxxxxxxx> Subject: RE: [PATCH] media: imx258: Add imx258 camera sensor driver Hi Sakari, Thanks for the review comments. Please check first then I will update patch accordingly. > + usleep_range(1000, 2000); You are right. This should be removed since delayed in ACPI power on sequence. https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+/826746/5/src/mainboard/google/poppy/variants/nautilus/include/variant/acpi/mipi_camera.asl#402 \_SB.PCI0.I2C2.CAM0.CRST(1) Sleep(5) (could be less so it could be adjusted in coreboot then for shorter launch time. ) > + /* stream off */ > + ret = imx258_write_reg(imx258, IMX258_REG_MODE_SELECT, > + IMX258_REG_VALUE_08BIT, IMX258_MODE_STANDBY); > I don't think it should be possible that the sensor was streaming here. If > it was, something must have been really wrong. Sensor datasheet claimed before updating register and streaming, need do SW standby. However Sony implements the function with single bit. (different from OVT) So I will change the comment to "software standby" to replace "stream off" to clarify the purpose. Address: 0x0100 Value: 0: software standby 1: streaming > + if (ret) { > + dev_err(&client->dev, "%s failed to set stream off registers\n", > + __func__); > + return ret; > + } > + usleep_range(13000, 14000); > That's a very long delay. Will preciously adjust the delay here, and let me explain the delay based on sensor datasheet. (under Intel NDA) Also please help review if any comments. Above delay (13~14ms) is the time from XCLR (reset) to streaming. So I was programming it as 13~14ms. (numbering from spec) T6 = 0.4ms: MCLK start to reset rising till CCI Read version ID register wait time. T7 = 12ms: MCLK start and reset rising till Send streaming command wait time (to complete reading all parameters) Since in coreboot, there are 5ms delay after reset, so assume T6" is 5ms. And from T6~T7, per the profiling data on DUT, the duration is always longer than 25ms (removed the manual delay) while loading all required register lists. I would still prefer to keep a 1~2ms delay range before streaming on for safety purpose. > +static int imx258_get_skip_frames(struct v4l2_subdev *sd, u32 *frames) > +{ > + /* > + * Sony: The 1st frame is ok when set from SW standby to streaming. > + */ > + *frames = 0; > If that's the case, then you can drop g_skip_frames callback altogether.. If remove g_skip_frames callback, it implies always 0 when user queries. I would like to keep the flexibly if we will need change the skip_frames value. How do you think? > + case V4L2_CID_GAIN: > + /* Todo */ > + break; > + case V4L2_CID_TEST_PATTERN: > + /* Todo */ > If the control isn't implemented, I suggest to remove it from the driver altogether, or alternatively implement it. > The GAIN control should likely be DIGITAL_GAIN instead, right? You are right. Will remove them first. We are almost done (DIGITAL_GAIN) and will submit separately. Regards, Andy -----Original Message----- From: Sakari Ailus [mailto:sakari.ailus@xxxxxxxxxxxxxxx] Sent: Friday, January 12, 2018 5:25 AM To: Yeh, Andy <andy.yeh@xxxxxxxxx> Cc: linux-media@xxxxxxxxxxxxxxx Subject: Re: [PATCH] media: imx258: Add imx258 camera sensor driver Hi Andy, Thanks for the patch. Please see my comments below. On Thu, Jan 11, 2018 at 10:47:39PM +0800, Andy Yeh 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: Andy Yeh <andy.yeh@xxxxxxxxx> > --- > MAINTAINERS | 7 + > drivers/media/i2c/Kconfig | 11 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/imx258.c | 1184 > ++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 1203 insertions(+) > create mode 100644 drivers/media/i2c/imx258.c > > diff --git a/MAINTAINERS b/MAINTAINERS index d76af75..806aa46 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12640,6 +12640,13 @@ S: Maintained > F: drivers/ssb/ > F: include/linux/ssb/ > > +SONY IMX258 SENSOR DRIVER > +M: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > +L: linux-media@xxxxxxxxxxxxxxx > +T: git git://linuxtv.org/media_tree.git > +S: Maintained > +F: drivers/media/i2c/imx258.c > + > SONY IMX274 SENSOR DRIVER > M: Leon Luo <leonl@xxxxxxxxxxxxxxxxxx> > L: linux-media@xxxxxxxxxxxxxxx > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index cb5d7ff..cabde37 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -555,6 +555,17 @@ config VIDEO_APTINA_PLL config VIDEO_SMIAPP_PLL > tristate > > +config VIDEO_IMX258 > + tristate "Sony IMX258 sensor support" > + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API > + depends on MEDIA_CAMERA_SUPPORT > + ---help--- > + This is a Video4Linux2 sensor-level driver for the Sony > + IMX258 camera. > + > + To compile this driver as a module, choose M here: the > + module will be called imx258. > + > config VIDEO_IMX274 > tristate "Sony IMX274 sensor support" > depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API diff --git > a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index > 548a9ef..cf1e0f1 100644 > --- a/drivers/media/i2c/Makefile > +++ b/drivers/media/i2c/Makefile > @@ -93,6 +93,7 @@ obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o > obj-$(CONFIG_VIDEO_ML86V7667) += ml86v7667.o > obj-$(CONFIG_VIDEO_OV2659) += ov2659.o > obj-$(CONFIG_VIDEO_TC358743) += tc358743.o > +obj-$(CONFIG_VIDEO_IMX258) += imx258.o > obj-$(CONFIG_VIDEO_IMX274) += imx274.o > > obj-$(CONFIG_SDR_MAX2175) += max2175.o diff --git > a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c new file > mode 100644 index 0000000..e4a44d6 > --- /dev/null > +++ b/drivers/media/i2c/imx258.c > @@ -0,0 +1,1184 @@ > +// Copyright (C) 2018 Intel Corporation // SPDX-License-Identifier: > +GPL-2.0 > + > +#include <linux/acpi.h> > +#include <linux/delay.h> > +#include <linux/i2c.h> > +#include <linux/module.h> > +#include <linux/pm_runtime.h> > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-device.h> > + > +#define IMX258_REG_VALUE_08BIT 1 > +#define IMX258_REG_VALUE_16BIT 2 > +#define IMX258_REG_VALUE_24BIT 3 > + > +#define IMX258_REG_MODE_SELECT 0x0100 > +#define IMX258_MODE_STANDBY 0x00 > +#define IMX258_MODE_STREAMING 0x01 > + > +/* Chip ID */ > +#define IMX258_REG_CHIP_ID 0x0016 > +#define IMX258_CHIP_ID 0x0258 > + > +/* V_TIMING internal */ > +#define IMX258_REG_VTS 0x0340 > +#define IMX258_VTS_30FPS 0x0c98 > +#define IMX258_VTS_MAX 0x7fff > + > +/* HBLANK control - read only */ > +#define IMX258_PPL_650MHZ 5352 > +#define IMX258_PPL_325MHZ 2676 > + > +/* Exposure control */ > +#define IMX258_REG_EXPOSURE 0x0202 > +#define IMX258_EXPOSURE_MIN 4 > +#define IMX258_EXPOSURE_STEP 1 > +#define IMX258_EXPOSURE_DEFAULT 0x640 > + > +/* Analog gain control */ > +#define IMX258_REG_ANALOG_GAIN 0x0204 > +#define IMX258_ANA_GAIN_MIN 0 > +#define IMX258_ANA_GAIN_MAX 0x1fff > +#define IMX258_ANA_GAIN_STEP 1 > +#define IMX258_ANA_GAIN_DEFAULT 0x0 > + > +/* Orientation */ > +#define REG_MIRROR_FLIP_CONTROL 0x0101 > +#define REG_CONFIG_MIRROR_FLIP 0x03 > + > +struct imx258_reg { > + u16 address; > + u8 val; > +}; > + > +struct imx258_reg_list { > + u32 num_of_regs; > + const struct imx258_reg *regs; > +}; > + > +/* Link frequency config */ > +struct imx258_link_freq_config { > + u32 pixels_per_line; > + > + /* PLL registers for this link frequency */ > + struct imx258_reg_list reg_list; > +}; > + > +/* Mode : resolution and related config&values */ struct imx258_mode > +{ > + /* Frame width */ > + u32 width; > + /* Frame height */ > + u32 height; > + > + /* V-timing */ > + u32 vts_def; > + u32 vts_min; > + > + /* Index of Link frequency config to be used */ > + u32 link_freq_index; > + /* Default register values */ > + struct imx258_reg_list reg_list; > +}; > + > +/* 4208x3118 needs 1300Mbps/lane, 4 lanes */ static const struct > +imx258_reg mipi_data_rate_1300mbps[] = { > + {0x0301, 0x05}, > + {0x0303, 0x02}, > + {0x0305, 0x03}, > + {0x0306, 0x00}, > + {0x0307, 0xCB}, > + {0x0309, 0x0A}, > + {0x030B, 0x01}, > + {0x030D, 0x02}, > + {0x030E, 0x00}, > + {0x030F, 0xD8}, > + {0x0310, 0x00}, > + {0x0820, 0x14}, > + {0x0821, 0x4C}, > + {0x0822, 0xCC}, > + {0x0823, 0xCC}, > +}; > + > +static const struct imx258_reg mipi_data_rate_650mbps[] = { > + {0x0301, 0x05}, > + {0x0303, 0x02}, > + {0x0305, 0x03}, > + {0x0306, 0x00}, > + {0x0307, 0x64}, > + {0x0309, 0x0A}, > + {0x030B, 0x01}, > + {0x030D, 0x02}, > + {0x030E, 0x00}, > + {0x030F, 0xD8}, > + {0x0310, 0x00}, > + {0x0820, 0x14}, > + {0x0821, 0x50}, > + {0x0822, 0x00}, > + {0x0823, 0x00}, > +}; > + > +static const struct imx258_reg mode_4208x3118_regs[] = { > + {0x0136, 0x13}, > + {0x0137, 0x33}, > + {0x3051, 0x00}, > + {0x3052, 0x00}, > + {0x4E21, 0x14}, > + {0x6B11, 0xCF}, > + {0x7FF0, 0x08}, > + {0x7FF1, 0x0F}, > + {0x7FF2, 0x08}, > + {0x7FF3, 0x1B}, > + {0x7FF4, 0x23}, > + {0x7FF5, 0x60}, > + {0x7FF6, 0x00}, > + {0x7FF7, 0x01}, > + {0x7FF8, 0x00}, > + {0x7FF9, 0x78}, > + {0x7FFA, 0x00}, > + {0x7FFB, 0x00}, > + {0x7FFC, 0x00}, > + {0x7FFD, 0x00}, > + {0x7FFE, 0x00}, > + {0x7FFF, 0x03}, > + {0x7F76, 0x03}, > + {0x7F77, 0xFE}, > + {0x7FA8, 0x03}, > + {0x7FA9, 0xFE}, > + {0x7B24, 0x81}, > + {0x7B25, 0x00}, > + {0x6564, 0x07}, > + {0x6B0D, 0x41}, > + {0x653D, 0x04}, > + {0x6B05, 0x8C}, > + {0x6B06, 0xF9}, > + {0x6B08, 0x65}, > + {0x6B09, 0xFC}, > + {0x6B0A, 0xCF}, > + {0x6B0B, 0xD2}, > + {0x6700, 0x0E}, > + {0x6707, 0x0E}, > + {0x9104, 0x00}, > + {0x7421, 0x1C}, > + {0x7423, 0xD7}, > + {0x5F04, 0x00}, > + {0x5F05, 0xED}, > + {0x0112, 0x0A}, > + {0x0113, 0x0A}, > + {0x0114, 0x03}, > + {0x0342, 0x14}, > + {0x0343, 0xE8}, > + {0x0340, 0x0C}, > + {0x0341, 0x98}, > + {0x0344, 0x00}, > + {0x0345, 0x00}, > + {0x0346, 0x00}, > + {0x0347, 0x00}, > + {0x0348, 0x10}, > + {0x0349, 0x6F}, > + {0x034A, 0x0C}, > + {0x034B, 0x2E}, > + {0x0381, 0x01}, > + {0x0383, 0x01}, > + {0x0385, 0x01}, > + {0x0387, 0x01}, > + {0x0900, 0x00}, > + {0x0901, 0x11}, > + {0x0401, 0x00}, > + {0x0404, 0x00}, > + {0x0405, 0x10}, > + {0x0408, 0x00}, > + {0x0409, 0x00}, > + {0x040A, 0x00}, > + {0x040B, 0x00}, > + {0x040C, 0x10}, > + {0x040D, 0x70}, > + {0x040E, 0x0C}, > + {0x040F, 0x30}, > + {0x3038, 0x00}, > + {0x303A, 0x00}, > + {0x303B, 0x10}, > + {0x300D, 0x00}, > + {0x034C, 0x10}, > + {0x034D, 0x70}, > + {0x034E, 0x0C}, > + {0x034F, 0x30}, > + {0x0202, 0x0C}, > + {0x0203, 0x8E}, > + {0x0204, 0x00}, > + {0x0205, 0x00}, > + {0x020E, 0x01}, > + {0x020F, 0x00}, > + {0x7BCD, 0x00}, > + {0x94DC, 0x20}, > + {0x94DD, 0x20}, > + {0x94DE, 0x20}, > + {0x95DC, 0x20}, > + {0x95DD, 0x20}, > + {0x95DE, 0x20}, > + {0x7FB0, 0x00}, > + {0x9010, 0x3E}, > + {0x9419, 0x50}, > + {0x941B, 0x50}, > + {0x9519, 0x50}, > + {0x951B, 0x50}, > + {0x3030, 0x00}, > + {0x3032, 0x00}, > + {0x0220, 0x00}, > +}; > + > +static const struct imx258_reg mode_2104_1560_regs[] = { > + {0x0136, 0x1A}, > + {0x0137, 0x00}, > + {0x3051, 0x00}, > + {0x3052, 0x00}, > + {0x4E21, 0x14}, > + {0x6B11, 0xCF}, > + {0x7FF0, 0x08}, > + {0x7FF1, 0x0F}, > + {0x7FF2, 0x08}, > + {0x7FF3, 0x1B}, > + {0x7FF4, 0x23}, > + {0x7FF5, 0x60}, > + {0x7FF6, 0x00}, > + {0x7FF7, 0x01}, > + {0x7FF8, 0x00}, > + {0x7FF9, 0x78}, > + {0x7FFA, 0x00}, > + {0x7FFB, 0x00}, > + {0x7FFC, 0x00}, > + {0x7FFD, 0x00}, > + {0x7FFE, 0x00}, > + {0x7FFF, 0x03}, > + {0x7F76, 0x03}, > + {0x7F77, 0xFE}, > + {0x7FA8, 0x03}, > + {0x7FA9, 0xFE}, > + {0x7B24, 0x81}, > + {0x7B25, 0x00}, > + {0x6564, 0x07}, > + {0x6B0D, 0x41}, > + {0x653D, 0x04}, > + {0x6B05, 0x8C}, > + {0x6B06, 0xF9}, > + {0x6B08, 0x65}, > + {0x6B09, 0xFC}, > + {0x6B0A, 0xCF}, > + {0x6B0B, 0xD2}, > + {0x6700, 0x0E}, > + {0x6707, 0x0E}, > + {0x9104, 0x00}, > + {0x7421, 0x1C}, > + {0x7423, 0xD7}, > + {0x5F04, 0x00}, > + {0x5F05, 0xED}, > + {0x0112, 0x0A}, > + {0x0113, 0x0A}, > + {0x0114, 0x03}, > + {0x0342, 0x14}, > + {0x0343, 0xE8}, > + {0x0340, 0x06}, > + {0x0341, 0x40}, > + {0x0344, 0x00}, > + {0x0345, 0x00}, > + {0x0346, 0x00}, > + {0x0347, 0x00}, > + {0x0348, 0x10}, > + {0x0349, 0x6F}, > + {0x034A, 0x0C}, > + {0x034B, 0x2E}, > + {0x0381, 0x01}, > + {0x0383, 0x01}, > + {0x0385, 0x01}, > + {0x0387, 0x01}, > + {0x0900, 0x01}, > + {0x0901, 0x12}, > + {0x0401, 0x01}, > + {0x0404, 0x00}, > + {0x0405, 0x1F}, > + {0x0408, 0x00}, > + {0x0409, 0x3C}, > + {0x040A, 0x00}, > + {0x040B, 0x00}, > + {0x040C, 0x0F}, > + {0x040D, 0xF6}, > + {0x040E, 0x06}, > + {0x040F, 0x18}, > + {0x3038, 0x00}, > + {0x303A, 0x00}, > + {0x303B, 0x10}, > + {0x300D, 0x00}, > + {0x034C, 0x08}, > + {0x034D, 0x38}, > + {0x034E, 0x06}, > + {0x034F, 0x18}, > + {0x0202, 0x06}, > + {0x0203, 0x36}, > + {0x0204, 0x00}, > + {0x0205, 0x00}, > + {0x020E, 0x01}, > + {0x020F, 0x00}, > + {0x7BCD, 0x01}, > + {0x94DC, 0x20}, > + {0x94DD, 0x20}, > + {0x94DE, 0x20}, > + {0x95DC, 0x20}, > + {0x95DD, 0x20}, > + {0x95DE, 0x20}, > + {0x7FB0, 0x00}, > + {0x9010, 0x3E}, > + {0x9419, 0x50}, > + {0x941B, 0x50}, > + {0x9519, 0x50}, > + {0x951B, 0x50}, > + {0x3030, 0x00}, > + {0x3032, 0x00}, > + {0x0220, 0x00}, > +}; > + > +static const char * const imx258_test_pattern_menu[] = { > + "Disabled", > + "Vertical Color Bar Type 1", > + "Vertical Color Bar Type 2", > + "Vertical Color Bar Type 3", > + "Vertical Color Bar Type 4" > +}; > + > +/* Configurations for supported link frequencies */ > +#define IMX258_LINK_FREQ_650MHZ 649600000ULL > +#define IMX258_LINK_FREQ_325MHZ 324800000ULL > + > +/* > + * pixel_rate = link_freq * data-rate * nr_of_lanes / bits_per_sample > + * data rate => double data rate; number of lanes => 4; bits per > +pixel => 10 */ static u64 link_freq_to_pixel_rate(u64 f) { > + f *= 2 * 4; > + do_div(f, 10); > + > + return f; > +} > + > +/* Menu items for LINK_FREQ V4L2 control */ static const s64 > +link_freq_menu_items[] = { > + IMX258_LINK_FREQ_650MHZ, > + IMX258_LINK_FREQ_325MHZ, > +}; > + > +/* Link frequency configs */ > +static const struct imx258_link_freq_config > + link_freq_configs[] = { Fits on previous line. > + { > + .pixels_per_line = IMX258_PPL_650MHZ, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(mipi_data_rate_1300mbps), > + .regs = mipi_data_rate_1300mbps, > + } > + }, > + { > + .pixels_per_line = IMX258_PPL_325MHZ, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(mipi_data_rate_650mbps), > + .regs = mipi_data_rate_650mbps, > + } > + }, > +}; > + > +/* Mode configs */ > +static const struct imx258_mode supported_modes[] = { > + { > + .width = 4208, > + .height = 3118, > + .vts_def = IMX258_VTS_30FPS, > + .vts_min = IMX258_VTS_30FPS, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(mode_4208x3118_regs), > + .regs = mode_4208x3118_regs, > + }, > + .link_freq_index = 0, > + }, > + { > + .width = 2104, > + .height = 1560, > + .vts_def = IMX258_VTS_30FPS, > + .vts_min = 1608, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(mode_2104_1560_regs), > + .regs = mode_2104_1560_regs, > + }, > + .link_freq_index = 1, > + }, > +}; > + > +struct imx258 { > + struct v4l2_subdev sd; > + struct media_pad pad; > + > + struct v4l2_ctrl_handler ctrl_handler; > + /* V4L2 Controls */ > + struct v4l2_ctrl *link_freq; > + struct v4l2_ctrl *pixel_rate; > + struct v4l2_ctrl *vblank; > + struct v4l2_ctrl *hblank; > + struct v4l2_ctrl *exposure; > + > + /* Current mode */ > + const struct imx258_mode *cur_mode; > + > + /* Mutex for serialized access */ > + struct mutex mutex; > + > + /* Streaming on/off */ > + bool streaming; > +}; > + > +#define to_imx258(_sd) container_of(_sd, struct imx258, sd) > + > +/* Read registers up to 4 at a time */ static int > +imx258_read_reg(struct imx258 *imx258, u16 reg, u32 len, u32 *val) { > + struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); > + struct i2c_msg msgs[2]; > + u8 *data_be_p; > + int ret; > + u32 data_be = 0; > + u16 reg_addr_be = cpu_to_be16(reg); > + > + if (len > 4) > + return -EINVAL; > + > + data_be_p = (u8 *)&data_be; > + /* Write register address */ > + msgs[0].addr = client->addr; > + msgs[0].flags = 0; > + msgs[0].len = 2; > + msgs[0].buf = (u8 *)®_addr_be; > + > + /* Read data from register */ > + msgs[1].addr = client->addr; > + msgs[1].flags = I2C_M_RD; > + msgs[1].len = len; > + msgs[1].buf = &data_be_p[4 - len]; > + > + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > + if (ret != ARRAY_SIZE(msgs)) > + return -EIO; > + > + *val = be32_to_cpu(data_be); > + > + return 0; > +} > + > +/* Write registers up to 4 at a time */ static int > +imx258_write_reg(struct imx258 *imx258, u16 reg, u32 len, u32 val) { > + struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); > + int buf_i, val_i; > + u8 buf[6], *val_p; > + > + if (len > 4) > + return -EINVAL; > + > + buf[0] = reg >> 8; > + buf[1] = reg & 0xff; > + > + val = cpu_to_be32(val); > + val_p = (u8 *)&val; > + buf_i = 2; > + val_i = 4 - len; > + > + while (val_i < 4) > + buf[buf_i++] = val_p[val_i++]; > + > + if (i2c_master_send(client, buf, len + 2) != len + 2) > + return -EIO; > + > + return 0; > +} > + > +/* Write a list of registers */ > +static int imx258_write_regs(struct imx258 *imx258, > + const struct imx258_reg *regs, u32 len) { > + struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); > + int ret; > + u32 i; > + > + for (i = 0; i < len; i++) { > + ret = imx258_write_reg(imx258, regs[i].address, 1, > + regs[i].val); > + if (ret) { > + dev_err_ratelimited( > + &client->dev, > + "Failed to write reg 0x%4.4x. error = %d\n", > + regs[i].address, ret); > + > + return ret; > + } > + } > + > + return 0; > +} > + > +static int imx258_write_reg_list(struct imx258 *imx258, > + const struct imx258_reg_list *r_list) { > + return imx258_write_regs(imx258, r_list->regs, r_list->num_of_regs); > +} > + > +/* Open sub-device */ > +static int imx258_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh > +*fh) { > + struct v4l2_mbus_framefmt *try_fmt = > + v4l2_subdev_get_try_format(sd, fh->pad, 0); > + > + /* Initialize try_fmt */ > + try_fmt->width = supported_modes[0].width; > + try_fmt->height = supported_modes[0].height; > + try_fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10; > + try_fmt->field = V4L2_FIELD_NONE; > + > + return 0; > +} > + > +static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) { > + struct imx258 *imx258 = > + container_of(ctrl->handler, struct imx258, ctrl_handler); > + struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); > + s64 max; > + int ret = 0; > + > + /* Propagate change of current control to all related controls */ > + if (ctrl->id == V4L2_CID_VBLANK) { > + /* Update max exposure while meeting expected vblanking */ > + max = imx258->cur_mode->height + ctrl->val - 8; > + __v4l2_ctrl_modify_range(imx258->exposure, > + imx258->exposure->minimum, > + max, imx258->exposure->step, max); > + } > + > + /* > + * Applying V4L2 control value only happens > + * when power is up for streaming > + */ > + if (pm_runtime_get_if_in_use(&client->dev) <= 0) > + return 0; > + > + switch (ctrl->id) { > + case V4L2_CID_ANALOGUE_GAIN: > + ret = imx258_write_reg(imx258, IMX258_REG_ANALOG_GAIN, > + IMX258_REG_VALUE_16BIT, ctrl->val); > + break; > + case V4L2_CID_EXPOSURE: > + ret = imx258_write_reg(imx258, IMX258_REG_EXPOSURE, > + IMX258_REG_VALUE_24BIT, > + ctrl->val << 4); > + break; > + case V4L2_CID_VBLANK: > + /* Update VTS that meets expected vertical blanking */ > + ret = imx258_write_reg(imx258, IMX258_REG_VTS, > + IMX258_REG_VALUE_16BIT, > + imx258->cur_mode->height > + + ctrl->val); > + break; > + case V4L2_CID_GAIN: > + /* Todo */ > + break; > + case V4L2_CID_TEST_PATTERN: > + /* Todo */ If the control isn't implemented, I suggest to remove it from the driver altogether, or alternatively implement it. The GAIN control should likely be DIGITAL_GAIN instead, right? > + break; > + default: > + dev_info(&client->dev, > + "ctrl(id:0x%x,val:0x%x) is not handled\n", > + ctrl->id, ctrl->val); > + break; > + } > + > + pm_runtime_put(&client->dev); > + > + return ret; > +} > + > +static const struct v4l2_ctrl_ops imx258_ctrl_ops = { > + .s_ctrl = imx258_set_ctrl, > +}; > + > +static int imx258_enum_mbus_code(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_mbus_code_enum *code) { > + /* Only one bayer order(GRBG) is supported */ > + if (code->index > 0) > + return -EINVAL; > + > + code->code = MEDIA_BUS_FMT_SGRBG10_1X10; > + > + return 0; > +} > + > +static int imx258_enum_frame_size(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_frame_size_enum *fse) { > + if (fse->index >= ARRAY_SIZE(supported_modes)) > + return -EINVAL; > + > + if (fse->code != MEDIA_BUS_FMT_SGRBG10_1X10) > + return -EINVAL; > + > + fse->min_width = supported_modes[fse->index].width; > + fse->max_width = fse->min_width; > + fse->min_height = supported_modes[fse->index].height; > + fse->max_height = fse->min_height; > + > + return 0; > +} > + > +static void imx258_update_pad_format(const struct imx258_mode *mode, > + struct v4l2_subdev_format *fmt) { > + fmt->format.width = mode->width; > + fmt->format.height = mode->height; > + fmt->format.code = MEDIA_BUS_FMT_SGRBG10_1X10; > + fmt->format.field = V4L2_FIELD_NONE; } > + > +static int __imx258_get_pad_format(struct imx258 *imx258, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt) { > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) > + fmt->format = *v4l2_subdev_get_try_format(&imx258->sd, cfg, > + fmt->pad); > + else > + imx258_update_pad_format(imx258->cur_mode, fmt); > + > + return 0; > +} > + > +static int imx258_get_pad_format(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt) { > + struct imx258 *imx258 = to_imx258(sd); > + int ret; > + > + mutex_lock(&imx258->mutex); > + ret = __imx258_get_pad_format(imx258, cfg, fmt); > + mutex_unlock(&imx258->mutex); > + > + return ret; > +} > + > +/* > + * Calculate resolution distance > + */ > +static int imx258_get_resolution_dist(const struct imx258_mode *mode, > + struct v4l2_mbus_framefmt *framefmt) { > + return abs(mode->width - framefmt->width) + > + abs(mode->height - framefmt->height); } > + > +/* > + * Find the closest supported resolution to the requested resolution > + */ > +static const struct imx258_mode * > +imx258_find_best_fit(struct imx258 *imx258, > + struct v4l2_subdev_format *fmt) > +{ > + int i, dist, cur_best_fit = 0, cur_best_fit_dist = -1; > + struct v4l2_mbus_framefmt *framefmt = &fmt->format; > + > + for (i = 0; i < ARRAY_SIZE(supported_modes); i++) { > + dist = imx258_get_resolution_dist(&supported_modes[i], > + framefmt); > + if (cur_best_fit_dist == -1 || dist < cur_best_fit_dist) { > + cur_best_fit_dist = dist; > + cur_best_fit = i; > + } > + } > + > + return &supported_modes[cur_best_fit]; > +} > + > +static int imx258_set_pad_format(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt) > +{ > + struct imx258 *imx258 = to_imx258(sd); > + const struct imx258_mode *mode; > + struct v4l2_mbus_framefmt *framefmt; > + s32 vblank_def; > + s32 vblank_min; > + s64 h_blank; > + s64 pixel_rate; > + s64 link_freq; > + > + mutex_lock(&imx258->mutex); > + > + /* Only one raw bayer(GBRG) order is supported */ > + fmt->format.code = MEDIA_BUS_FMT_SGRBG10_1X10; > + > + mode = imx258_find_best_fit(imx258, fmt); > + imx258_update_pad_format(mode, fmt); > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { > + framefmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad); > + *framefmt = fmt->format; > + } else { > + imx258->cur_mode = mode; > + __v4l2_ctrl_s_ctrl(imx258->link_freq, mode->link_freq_index); > + > + link_freq = link_freq_menu_items[mode->link_freq_index]; > + pixel_rate = link_freq_to_pixel_rate(link_freq); > + __v4l2_ctrl_s_ctrl_int64(imx258->pixel_rate, pixel_rate); > + /* Update limits and set FPS to default */ > + vblank_def = imx258->cur_mode->vts_def - > + imx258->cur_mode->height; > + vblank_min = imx258->cur_mode->vts_min - > + imx258->cur_mode->height; > + __v4l2_ctrl_modify_range( > + imx258->vblank, vblank_min, > + IMX258_VTS_MAX - imx258->cur_mode->height, 1, > + vblank_def); > + __v4l2_ctrl_s_ctrl(imx258->vblank, vblank_def); > + h_blank = > + link_freq_configs[mode->link_freq_index].pixels_per_line > + - imx258->cur_mode->width; > + __v4l2_ctrl_modify_range(imx258->hblank, h_blank, > + h_blank, 1, h_blank); > + } > + > + mutex_unlock(&imx258->mutex); > + > + return 0; > +} > + > +static int imx258_get_skip_frames(struct v4l2_subdev *sd, u32 *frames) > +{ > + /* > + * Sony: The 1st frame is ok when set from SW standby to streaming. > + */ > + *frames = 0; If that's the case, then you can drop g_skip_frames callback altogether. > + > + return 0; > +} > + > +/* Start streaming */ > +static int imx258_start_streaming(struct imx258 *imx258) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); > + const struct imx258_reg_list *reg_list; > + int ret, link_freq_index; > + > + usleep_range(1000, 2000); Any particular reason for the delay here? The ACPI power on sequence does not contain the necessary delay itself? > + /* stream off */ > + ret = imx258_write_reg(imx258, IMX258_REG_MODE_SELECT, > + IMX258_REG_VALUE_08BIT, IMX258_MODE_STANDBY); I don't think it should be possible that the sensor was streaming here. If it was, something must have been really wrong. > + if (ret) { > + dev_err(&client->dev, "%s failed to set stream off registers\n", > + __func__); > + return ret; > + } > + > + usleep_range(13000, 14000); That's a very long delay. > + > + /* Setup PLL */ > + link_freq_index = imx258->cur_mode->link_freq_index; > + reg_list = &link_freq_configs[link_freq_index].reg_list; > + ret = imx258_write_reg_list(imx258, reg_list); > + if (ret) { > + dev_err(&client->dev, "%s failed to set plls\n", __func__); > + return ret; > + } > + > + /* Apply default values of current mode */ > + reg_list = &imx258->cur_mode->reg_list; > + ret = imx258_write_reg_list(imx258, reg_list); > + if (ret) { > + dev_err(&client->dev, "%s failed to set mode\n", __func__); > + return ret; > + } > + > + /* 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; > + > + return imx258_write_reg(imx258, IMX258_REG_MODE_SELECT, > + IMX258_REG_VALUE_08BIT, > + IMX258_MODE_STREAMING); > +} > + > +/* Stop streaming */ > +static int imx258_stop_streaming(struct imx258 *imx258) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); > + int ret; > + > + ret = imx258_write_reg(imx258, IMX258_REG_MODE_SELECT, > + IMX258_REG_VALUE_08BIT, IMX258_MODE_STANDBY); > + if (ret) > + dev_err(&client->dev, "%s failed to set stream\n", __func__); > + > + /* Return success even if it was an error, as there is nothing the > + * caller can do about it. > + */ > + return 0; > +} > + > +static int imx258_set_stream(struct v4l2_subdev *sd, int enable) > +{ > + struct imx258 *imx258 = to_imx258(sd); > + struct i2c_client *client = v4l2_get_subdevdata(sd); > + int ret = 0; > + > + mutex_lock(&imx258->mutex); > + if (imx258->streaming == enable) { > + mutex_unlock(&imx258->mutex); > + return 0; > + } > + > + if (enable) { > + ret = pm_runtime_get_sync(&client->dev); > + if (ret < 0) { > + pm_runtime_put_noidle(&client->dev); > + goto err_unlock; > + } > + > + /* > + * Apply default & customized values > + * and then start streaming. > + */ > + ret = imx258_start_streaming(imx258); > + if (ret) > + goto err_rpm_put; > + } else { > + imx258_stop_streaming(imx258); > + pm_runtime_put(&client->dev); > + } > + > + imx258->streaming = enable; > + mutex_unlock(&imx258->mutex); > + > + return ret; > + > +err_rpm_put: > + pm_runtime_put(&client->dev); > +err_unlock: > + mutex_unlock(&imx258->mutex); > + > + return ret; > +} > + > +static int __maybe_unused imx258_suspend(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx258 *imx258 = to_imx258(sd); > + > + if (imx258->streaming) > + imx258_stop_streaming(imx258); > + > + return 0; > +} > + > +static int __maybe_unused imx258_resume(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx258 *imx258 = to_imx258(sd); > + int ret; > + > + if (imx258->streaming) { > + ret = imx258_start_streaming(imx258); > + if (ret) > + goto error; > + } > + > + return 0; > + > +error: > + imx258_stop_streaming(imx258); > + imx258->streaming = 0; > + return ret; > +} > + > +/* Verify chip ID */ > +static int imx258_identify_module(struct imx258 *imx258) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); > + int ret; > + u32 val; > + > + ret = imx258_read_reg(imx258, IMX258_REG_CHIP_ID, > + IMX258_REG_VALUE_16BIT, &val); > + if (ret) { > + dev_err(&client->dev, "failed to read chip id %x\n", > + IMX258_CHIP_ID); > + return ret; > + } > + > + if (val != IMX258_CHIP_ID) { > + dev_err(&client->dev, "chip id mismatch: %x!=%x\n", > + IMX258_CHIP_ID, val); > + return -EIO; > + } > + > + return 0; > +} > + > +static const struct v4l2_subdev_video_ops imx258_video_ops = { > + .s_stream = imx258_set_stream, > +}; > + > +static const struct v4l2_subdev_pad_ops imx258_pad_ops = { > + .enum_mbus_code = imx258_enum_mbus_code, > + .get_fmt = imx258_get_pad_format, > + .set_fmt = imx258_set_pad_format, > + .enum_frame_size = imx258_enum_frame_size, > +}; > + > +static const struct v4l2_subdev_sensor_ops imx258_sensor_ops = { > + .g_skip_frames = imx258_get_skip_frames, > +}; > + > +static const struct v4l2_subdev_ops imx258_subdev_ops = { > + .video = &imx258_video_ops, > + .pad = &imx258_pad_ops, > + .sensor = &imx258_sensor_ops, > +}; > + > +static const struct media_entity_operations imx258_subdev_entity_ops = { > + .link_validate = v4l2_subdev_link_validate, The sensor has a single entity that has no sink pads. Therefore you have no need for link_validate op, nor entity_ops. Same for sensor_ops above. > +}; > + > +static const struct v4l2_subdev_internal_ops imx258_internal_ops = { > + .open = imx258_open, > +}; > + > +/* 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; > + > + pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]); > + pixel_rate_min = link_freq_to_pixel_rate(link_freq_menu_items[1]); > + /* By default, PIXEL_RATE is read only */ > + imx258->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops, > + V4L2_CID_PIXEL_RATE, > + pixel_rate_min, pixel_rate_max, > + 1, pixel_rate_max); > + > + > + vblank_def = imx258->cur_mode->vts_def - imx258->cur_mode->height; > + vblank_min = imx258->cur_mode->vts_min - imx258->cur_mode->height; > + imx258->vblank = v4l2_ctrl_new_std( > + ctrl_hdlr, &imx258_ctrl_ops, V4L2_CID_VBLANK, > + vblank_min, > + IMX258_VTS_MAX - imx258->cur_mode->height, 1, > + vblank_def); > + > + imx258->hblank = v4l2_ctrl_new_std( > + ctrl_hdlr, &imx258_ctrl_ops, V4L2_CID_HBLANK, > + IMX258_PPL_650MHZ - imx258->cur_mode->width, > + IMX258_PPL_650MHZ - imx258->cur_mode->width, > + 1, > + IMX258_PPL_650MHZ - imx258->cur_mode->width); > + > + if (!imx258->hblank) { > + ret = -EINVAL; > + goto error; > + } > + > + imx258->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + exposure_max = imx258->cur_mode->vts_def - 8; > + imx258->exposure = v4l2_ctrl_new_std( > + ctrl_hdlr, &imx258_ctrl_ops, > + V4L2_CID_EXPOSURE, IMX258_EXPOSURE_MIN, > + exposure_max, IMX258_EXPOSURE_STEP, > + IMX258_EXPOSURE_DEFAULT); > + > + v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops, V4L2_CID_ANALOGUE_GAIN, > + IMX258_ANA_GAIN_MIN, IMX258_ANA_GAIN_MAX, > + IMX258_ANA_GAIN_STEP, IMX258_ANA_GAIN_DEFAULT); > + > + 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); > + > + if (ctrl_hdlr->error) { > + ret = ctrl_hdlr->error; > + dev_err(&client->dev, "%s control init failed (%d)\n", > + __func__, ret); > + goto error; > + } > + > + imx258->sd.ctrl_handler = ctrl_hdlr; > + > + return 0; > + > +error: > + v4l2_ctrl_handler_free(ctrl_hdlr); > + mutex_destroy(&imx258->mutex); > + > + return ret; > +} > + > +static void imx258_free_controls(struct imx258 *imx258) > +{ > + v4l2_ctrl_handler_free(imx258->sd.ctrl_handler); > + mutex_destroy(&imx258->mutex); > +} > + > +static int imx258_probe(struct i2c_client *client) > +{ > + struct imx258 *imx258; > + int ret; > + u32 val = 0; > + > + device_property_read_u32(&client->dev, "clock-frequency", &val); > + if (val != 19200000) > + return -EINVAL; > + > + imx258 = devm_kzalloc(&client->dev, sizeof(*imx258), GFP_KERNEL); > + if (!imx258) > + return -ENOMEM; > + > + /* Initialize subdev */ > + v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops); > + > + /* Check module identity */ > + ret = imx258_identify_module(imx258); > + if (ret) > + return ret; > + > + /* Set default mode to max resolution */ > + imx258->cur_mode = &supported_modes[0]; > + > + ret = imx258_init_controls(imx258); > + if (ret) > + return ret; > + > + /* Initialize subdev */ > + imx258->sd.internal_ops = &imx258_internal_ops; > + imx258->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > + imx258->sd.entity.ops = &imx258_subdev_entity_ops; > + imx258->sd.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR; > + > + /* Initialize source pad */ > + imx258->pad.flags = MEDIA_PAD_FL_SOURCE; > + > + ret = media_entity_pads_init(&imx258->sd.entity, 1, &imx258->pad); > + Extra newline. > + if (ret) { > + dev_err(&client->dev, "%s failed:%d\n", __func__, ret); > + goto error_handler_free; > + } > + > + ret = v4l2_async_register_subdev_sensor_common(&imx258->sd); > + if (ret < 0) > + goto error_media_entity; > + > + pm_runtime_set_active(&client->dev); > + pm_runtime_enable(&client->dev); > + pm_runtime_idle(&client->dev); > + > + return 0; > + > +error_media_entity: > + media_entity_cleanup(&imx258->sd.entity); > + > +error_handler_free: > + imx258_free_controls(imx258); > + > + return ret; > +} > + > +static int imx258_remove(struct i2c_client *client) > +{ > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx258 *imx258 = to_imx258(sd); > + > + v4l2_async_unregister_subdev(sd); > + media_entity_cleanup(&sd->entity); > + imx258_free_controls(imx258); > + > + pm_runtime_disable(&client->dev); > + pm_runtime_set_suspended(&client->dev); > + > + return 0; > +} > + > +static const struct dev_pm_ops imx258_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(imx258_suspend, imx258_resume) > +}; > + > +#ifdef CONFIG_ACPI > +static const struct acpi_device_id imx258_acpi_ids[] = { > + { "SONY258A" }, > + { /* sentinel */ } > +}; > + > +MODULE_DEVICE_TABLE(acpi, imx258_acpi_ids); > +#endif > + > +static struct i2c_driver imx258_i2c_driver = { > + .driver = { > + .name = "imx258", > + .pm = &imx258_pm_ops, > + .acpi_match_table = ACPI_PTR(imx258_acpi_ids), > + }, > + .probe_new = imx258_probe, > + .remove = imx258_remove, > +}; > + > +module_i2c_driver(imx258_i2c_driver); > + > +MODULE_AUTHOR("Yeh, Andy <andy.yeh@xxxxxxxxx>"); > +MODULE_AUTHOR("Chiang, Alan <alanx.chiang@xxxxxxxxx>"); > +MODULE_AUTHOR("Chen, Jason <jasonx.z.chen@xxxxxxxxx>"); > +MODULE_DESCRIPTION("Sony IMX258 sensor driver"); > +MODULE_LICENSE("GPL v2"); -- Kind regards, Sakari Ailus sakari.ailus@xxxxxxxxxxxxxxx
