Hi Sakari, > > + /* stream off */ > > + ret = imx258_write_reg(imx258, IMX258_REG_MODE_SELECT, > > + IMX258_REG_VALUE_08BIT, > > +IMX258_MODE_STANDBY); >> Yes, this is true. But the sensor is already in software standby mode here, >> isn't it?
No, it's not. 0x0100 is not set to 0 in default setting. Power on sequence requests sw-standby to be set at T6 (reset + 0.4ms), and streaming on to be set at T7 (reset + 12ms). > Fair enough; to be on the safe side, you indeed need the 12 ms delay here. OK. I will set usleep_range(1000, 2000), which is enough for test on DUT. Regards, Andy -----Original Message----- From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com] Sent: Friday, January 12, 2018 4:46 PM To: Yeh, Andy <andy....@intel.com> Cc: linux-media@vger.kernel.org; Chiang, AlanX <alanx.chi...@intel.com>; Chen, JasonX Z <jasonx.z.c...@intel.com> Subject: Re: [PATCH] media: imx258: Add imx258 camera sensor driver Hi Andy, On Fri, Jan 12, 2018 at 08:18:50AM +0000, Yeh, Andy wrote: > 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. Please wrap the lines at 80 characters. > > Regards, Andy > > From: Yeh, Andy > Sent: Friday, January 12, 2018 3:31 PM > To: 'Sakari Ailus' <sakari.ai...@linux.intel.com> > Cc: 'linux-media@vger.kernel.org' <linux-media@vger.kernel.org>; AlanX > Chiang (alanx.chi...@intel.com) <alanx.chi...@intel.com>; JasonX Z > Chen (jasonx.z.c...@intel.com) <jasonx.z.c...@intel.com> > 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/core > boot/+/826746/5/src/mainboard/google/poppy/variants/nautilus/include/v > ariant/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. > ) Ack. > > > + /* 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 Yes, this is true. But the sensor is already in software standby mode here, isn't it? ... > 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. Fair enough; to be on the safe side, you indeed need the 12 ms delay here. > > > +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? If it turns out the sensor needs this, then you can re-introduce the function. Please remove it now. > > > + 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. Ack. -- Kind regards, Sakari Ailus sakari.ai...@linux.intel.com
