Re: [PATCH v5] media: add imx319 camera sensor driver
On 09/21/2018 03:41 PM, Sakari Ailus wrote:
> Hi Bingbu,
>
> On Fri, Sep 21, 2018 at 03:20:19PM +0800, Bing Bu Cao wrote:
> ...
+ try_fmt->field = V4L2_FIELD_NONE;
+
+ mutex_unlock(>mutex);
+
+ return 0;
+}
+
+static int imx319_update_digital_gain(struct imx319 *imx319, u32 d_gain)
+{
+ int ret;
+
+ ret = imx319_write_reg(imx319, IMX319_REG_DPGA_USE_GLOBAL_GAIN, 1, 1);
+ if (ret)
+ return ret;
>>> You could do this write right after powering the sensor on, couldn't you?
>> Sakari,
>> Use IMX319_REG_DPGA_USE_GLOBAL_GAIN to do digital gain control, 1 for all
>> color and 0 for by color. For all color, use register
>> IMX319_REG_DIG_GAIN_GLOBAL to set the digital gain value, otherwise use
>> other registers below. By default the digital gain select is not set and
>> the gain will be set by color via setting register 0x0210, 0x0212,
>> 0x0213, these registers were set during powering on. So I prefer to only
>> change the default digital gain control if any digital gain request.
>>
>> Does this make sense?
> Why are per-component gains used at all when they always have the same
> value and the sensor supports global gain?
I see that both the per-color and all-color gains are set during powering on.
I think they are from the reference settings from vendor, let me try to move
this control selection into streaming ops.
Thanks!
>
Re: [PATCH v5] media: add imx319 camera sensor driver
On Fri, Sep 21, 2018 at 11:28 AM Bing Bu Cao wrote:
>
>
> On 09/18/2018 05:49 PM, Tomasz Figa wrote:
> > Hi Bingbu,
> >
> > On Mon, Sep 17, 2018 at 2:53 PM wrote:
> >> From: Bingbu Cao
> >>
> >> Add a v4l2 sub-device driver for the Sony imx319 image sensor.
> >> This is a camera sensor using the i2c bus for control and the
> >> csi-2 bus for data.
> > Please see my comments inline. Also, I'd appreciate being CCed on
> > related work in the future.
> Ack.
> Sorry, will add you into the cc-list.
> >
> > [snip]
> >> +
> >> +static const char * const imx319_test_pattern_menu[] = {
> >> + "Disabled",
> >> + "100% color bars",
> >> + "Solid color",
> >> + "Fade to gray color bars",
> >> + "PN9"
> >> +};
> >> +
> >> +static const int imx319_test_pattern_val[] = {
> >> + IMX319_TEST_PATTERN_DISABLED,
> >> + IMX319_TEST_PATTERN_COLOR_BARS,
> >> + IMX319_TEST_PATTERN_SOLID_COLOR,
> >> + IMX319_TEST_PATTERN_GRAY_COLOR_BARS,
> >> + IMX319_TEST_PATTERN_PN9,
> >> +};
> > This array is not needed. All the entries are equal to corresponding
> > indices, i.e. the array is equivalent to { 0, 1, 2, 3, 4 }. We can use
> > ctrl->val directly.
> Ack.
> > [snip]
> >
> >> +/* Write a list of registers */
> >> +static int imx319_write_regs(struct imx319 *imx319,
> >> + const struct imx319_reg *regs, u32 len)
> >> +{
> >> + struct i2c_client *client = v4l2_get_subdevdata(>sd);
> >> + int ret;
> >> + u32 i;
> >> +
> >> + for (i = 0; i < len; i++) {
> >> + ret = imx319_write_reg(imx319, regs[i].address, 1,
> >> regs[i].val);
> >> + if (ret) {
> >> + dev_err_ratelimited(>dev,
> >> +
> > Hmm, the message is clipped here. Let me see if it's something with my
> > email client...
> The code here:
>
> 1827 for (i = 0; i < len; i++) {
> 1828 ret = imx319_write_reg(imx319, regs[i].address, 1, regs[i].val);
> 1829 if (ret) {
> 1830 dev_err_ratelimited(>dev,
> 1831 "write reg 0x%4.4x return err %d",
> 1832 regs[i].address, ret);
> 1833 return ret;
> 1834 }
> 1835 } Same as the code shown on your client?
That was an issue with my email client, which showed only lines until
1831. I've worked around it and reviewed rest of the code in next
reply. Sorry for the noise.
Best regards,
Tomasz
Re: [PATCH v5] media: add imx319 camera sensor driver
Hi Bingbu,
On Fri, Sep 21, 2018 at 03:20:19PM +0800, Bing Bu Cao wrote:
...
> >> + try_fmt->field = V4L2_FIELD_NONE;
> >> +
> >> + mutex_unlock(>mutex);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int imx319_update_digital_gain(struct imx319 *imx319, u32 d_gain)
> >> +{
> >> + int ret;
> >> +
> >> + ret = imx319_write_reg(imx319, IMX319_REG_DPGA_USE_GLOBAL_GAIN, 1, 1);
> >> + if (ret)
> >> + return ret;
> > You could do this write right after powering the sensor on, couldn't you?
> Sakari,
> Use IMX319_REG_DPGA_USE_GLOBAL_GAIN to do digital gain control, 1 for all
> color and 0 for by color. For all color, use register
> IMX319_REG_DIG_GAIN_GLOBAL to set the digital gain value, otherwise use
> other registers below. By default the digital gain select is not set and
> the gain will be set by color via setting register 0x0210, 0x0212,
> 0x0213, these registers were set during powering on. So I prefer to only
> change the default digital gain control if any digital gain request.
>
> Does this make sense?
Why are per-component gains used at all when they always have the same
value and the sensor supports global gain?
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v5] media: add imx319 camera sensor driver
On 09/17/2018 07:34 PM, Sakari Ailus wrote: > Hi Bingbu, > > Thanks for the update! A few more small comments, I think we're done after > these. > > On Mon, Sep 17, 2018 at 01:57:52PM +0800, bingbu@intel.com wrote: >> From: Bingbu Cao >> >> Add a v4l2 sub-device driver for the Sony imx319 image sensor. >> This is a camera sensor using the i2c bus for control and the >> csi-2 bus for data. >> >> This driver supports following features: >> - manual exposure and analog/digital gain control support >> - vblank/hblank control support >> - 4 test patterns control support >> - vflip/hflip control support (will impact the output bayer order) >> - support following resolutions: >> - 3264x2448, 3280x2464 @ 30fps >> - 1936x1096, 1920x1080 @ 60fps >> - 1640x1232, 1640x922, 1296x736, 1280x720 @ 120fps >> - support 4 bayer orders output (via change v/hflip) >> - SRGGB10(default), SGRBG10, SGBRG10, SBGGR10 >> >> Signed-off-by: Bingbu Cao >> Signed-off-by: Tianshu Qiu >> >> --- >> >> This patch is based on sakari's media-tree git: >> https://git.linuxtv.org/sailus/media_tree.git/log/?h=for-4.20-1 >> >> Changes from v4 to v5: >> - use single PLL for all internal clocks >> - change link frequency to 482.4MHz >> - adjust frame timing for 2x2 binning modes >>and enlarge frame readout time >> - get CSI-2 link frequencies and external clock >>from firmware >> - use unlocked __v4l2_ctrl_grab() with change from: >>https://git.linuxtv.org/sailus/media_tree.git/commit/?h=unlocked-ctrl-grab >> >> Changes since v1: >> - fix some coding style issues - line breaks >> - add v4l2_ctrl_grab() to prevent v/hflip change >>during streaming >> - add v4l2 ctrl event (un)subscribe support >> - add more info into commit message >> >> --- >> --- >> MAINTAINERS|7 + >> drivers/media/i2c/Kconfig | 11 + >> drivers/media/i2c/Makefile |1 + >> drivers/media/i2c/imx319.c | 2524 >> >> 4 files changed, 2543 insertions(+) >> create mode 100644 drivers/media/i2c/imx319.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index a5b256b25905..abc4abb6f83c 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -13530,6 +13530,13 @@ S: Maintained >> F: drivers/media/i2c/imx274.c >> F: Documentation/devicetree/bindings/media/i2c/imx274.txt >> >> +SONY IMX319 SENSOR DRIVER >> +M: Bingbu Cao >> +L: linux-media@vger.kernel.org >> +T: git git://linuxtv.org/media_tree.git >> +S: Maintained >> +F: drivers/media/i2c/imx319.c >> + >> SONY MEMORYSTICK CARD SUPPORT >> M: Alex Dubov >> W: http://tifmxx.berlios.de/ >> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig >> index bfdb494686bf..603ac087975b 100644 >> --- a/drivers/media/i2c/Kconfig >> +++ b/drivers/media/i2c/Kconfig >> @@ -614,6 +614,17 @@ config VIDEO_IMX274 >>This is a V4L2 sensor driver for the Sony IMX274 >>CMOS image sensor. >> >> +config VIDEO_IMX319 >> +tristate "Sony IMX319 sensor support" >> +depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API >> +depends on MEDIA_CAMERA_SUPPORT >> +help >> + This is a Video4Linux2 sensor driver for the Sony >> + IMX319 camera. >> + >> + To compile this driver as a module, choose M here: the >> + module will be called imx319. >> + >> config VIDEO_OV2640 >> tristate "OmniVision OV2640 sensor support" >> depends on VIDEO_V4L2 && I2C >> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile >> index a94eb03d10d4..d10b438577be 100644 >> --- a/drivers/media/i2c/Makefile >> +++ b/drivers/media/i2c/Makefile >> @@ -108,5 +108,6 @@ 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_VIDEO_IMX319) += imx319.o >> >> obj-$(CONFIG_SDR_MAX2175) += max2175.o >> diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c >> new file mode 100644 >> index ..43c28c701431 >> --- /dev/null >> +++ b/drivers/media/i2c/imx319.c >> @@ -0,0 +1,2524 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// Copyright (C) 2018 Intel Corporation >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#define IMX319_REG_MODE_SELECT 0x0100 >> +#define IMX319_MODE_STANDBY 0x00 >> +#define IMX319_MODE_STREAMING 0x01 >> + >> +/* Chip ID */ >> +#define IMX319_REG_CHIP_ID 0x0016 >> +#define IMX319_CHIP_ID 0x0319 >> + >> +/* V_TIMING internal */ >> +#define IMX319_REG_FLL 0x0340 >> +#define IMX319_FLL_MAX 0x >> + >> +/* Exposure control */ >> +#define IMX319_REG_EXPOSURE 0x0202 >> +#define IMX319_EXPOSURE_MIN 1 >> +#define IMX319_EXPOSURE_STEP1 >> +#define
Re: [PATCH v5] media: add imx319 camera sensor driver
Ack, I will add more explanation into the code.
On 09/19/2018 12:11 PM, Tomasz Figa wrote:
> Hi Bingbu,
>
> On Mon, Sep 17, 2018 at 2:53 PM wrote:
> [snip]
>> +static int imx319_update_digital_gain(struct imx319 *imx319, u32 d_gain)
>> +{
>> + int ret;
>> +
>> + ret = imx319_write_reg(imx319, IMX319_REG_DPGA_USE_GLOBAL_GAIN, 1,
>> 1);
>> + if (ret)
>> + return ret;
>> +
>> + /* Digital gain = (d_gain & 0xFF00) + (d_gain & 0xFF)/256 times */
> What's the unit here?
>
> Is the equation above really correct? The range, besides ~0, would be
> from 256.0 to 65280 + 255/256, which sounds strange.
>
>> + return imx319_write_reg(imx319, IMX319_REG_DIG_GAIN_GLOBAL, 2,
>> d_gain);
>> +}
>> +
>> +static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> + struct imx319 *imx319 = container_of(ctrl->handler,
>> +struct imx319, ctrl_handler);
>> + struct i2c_client *client = v4l2_get_subdevdata(>sd);
>> + s64 max;
>> + int ret;
>> +
>> + /* Propagate change of current control to all related controls */
>> + switch (ctrl->id) {
>> + case V4L2_CID_VBLANK:
>> + /* Update max exposure while meeting expected vblanking */
>> + max = imx319->cur_mode->height + ctrl->val - 18;
>> + __v4l2_ctrl_modify_range(imx319->exposure,
>> +imx319->exposure->minimum,
>> +max, imx319->exposure->step, max);
>> + break;
>> + }
>> +
>> + /*
>> +* Applying V4L2 control value only happens
>> +* when power is up for streaming
>> +*/
>> + if (pm_runtime_get_if_in_use(>dev) == 0)
> nit: if (!pm_runtime_get_if_in_use(>dev)
>
>> + return 0;
>> +
> [snip]
>> +/* Initialize control handlers */
>> +static int imx319_init_controls(struct imx319 *imx319)
>> +{
>> + struct i2c_client *client = v4l2_get_subdevdata(>sd);
>> + struct v4l2_ctrl_handler *ctrl_hdlr;
>> + s64 exposure_max;
>> + s64 vblank_def;
>> + s64 vblank_min;
>> + s64 hblank;
>> + s64 pixel_rate;
>> + const struct imx319_mode *mode;
>> + int ret;
>> +
>> + ctrl_hdlr = >ctrl_handler;
>> + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
>> + if (ret)
>> + return ret;
>> +
>> + ctrl_hdlr->lock = >mutex;
>> + imx319->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>> _ctrl_ops,
>> + V4L2_CID_LINK_FREQ, 0, 0,
>> +
>> imx319->pdata->link_freqs);
>> + if (imx319->link_freq)
>> + imx319->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> +
>> + /* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
>> + pixel_rate = (imx319->link_def_freq * 2 * 4) / 10;
>> + /* By default, PIXEL_RATE is read only */
>> + imx319->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
>> + V4L2_CID_PIXEL_RATE,
>> pixel_rate,
>> + pixel_rate, 1, pixel_rate);
>> +
>> + /* Initialze vblank/hblank/exposure parameters based on current mode
>> */
>> + mode = imx319->cur_mode;
>> + vblank_def = mode->fll_def - mode->height;
>> + vblank_min = mode->fll_min - mode->height;
>> + imx319->vblank = v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
>> + V4L2_CID_VBLANK, vblank_min,
>> + IMX319_FLL_MAX - mode->height,
>> + 1, vblank_def);
>> +
>> + hblank = mode->llp - mode->width;
>> + imx319->hblank = v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
>> + V4L2_CID_HBLANK, hblank, hblank,
>> + 1, hblank);
>> + if (imx319->hblank)
>> + imx319->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> +
>> + exposure_max = mode->fll_def - 18;
>> + imx319->exposure = v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
>> +V4L2_CID_EXPOSURE,
>> +IMX319_EXPOSURE_MIN,
>> exposure_max,
>> +IMX319_EXPOSURE_STEP,
>> +IMX319_EXPOSURE_DEFAULT);
> Please explain how to interpret the exposure value in a comment.
>
>> +
>> + imx319->hflip = v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
>> + V4L2_CID_HFLIP, 0, 1, 1, 0);
>> + imx319->vflip = v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
>> + V4L2_CID_VFLIP, 0, 1, 1, 0);
>> +
>> + v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
>> V4L2_CID_ANALOGUE_GAIN,
>> +
Re: [PATCH v5] media: add imx319 camera sensor driver
On 09/18/2018 05:49 PM, Tomasz Figa wrote:
> Hi Bingbu,
>
> On Mon, Sep 17, 2018 at 2:53 PM wrote:
>> From: Bingbu Cao
>>
>> Add a v4l2 sub-device driver for the Sony imx319 image sensor.
>> This is a camera sensor using the i2c bus for control and the
>> csi-2 bus for data.
> Please see my comments inline. Also, I'd appreciate being CCed on
> related work in the future.
Ack.
Sorry, will add you into the cc-list.
>
> [snip]
>> +
>> +static const char * const imx319_test_pattern_menu[] = {
>> + "Disabled",
>> + "100% color bars",
>> + "Solid color",
>> + "Fade to gray color bars",
>> + "PN9"
>> +};
>> +
>> +static const int imx319_test_pattern_val[] = {
>> + IMX319_TEST_PATTERN_DISABLED,
>> + IMX319_TEST_PATTERN_COLOR_BARS,
>> + IMX319_TEST_PATTERN_SOLID_COLOR,
>> + IMX319_TEST_PATTERN_GRAY_COLOR_BARS,
>> + IMX319_TEST_PATTERN_PN9,
>> +};
> This array is not needed. All the entries are equal to corresponding
> indices, i.e. the array is equivalent to { 0, 1, 2, 3, 4 }. We can use
> ctrl->val directly.
Ack.
> [snip]
>
>> +/* Write a list of registers */
>> +static int imx319_write_regs(struct imx319 *imx319,
>> + const struct imx319_reg *regs, u32 len)
>> +{
>> + struct i2c_client *client = v4l2_get_subdevdata(>sd);
>> + int ret;
>> + u32 i;
>> +
>> + for (i = 0; i < len; i++) {
>> + ret = imx319_write_reg(imx319, regs[i].address, 1,
>> regs[i].val);
>> + if (ret) {
>> + dev_err_ratelimited(>dev,
>> +
> Hmm, the message is clipped here. Let me see if it's something with my
> email client...
The code here:
1827 for (i = 0; i < len; i++) {
1828 ret = imx319_write_reg(imx319, regs[i].address, 1, regs[i].val);
1829 if (ret) {
1830 dev_err_ratelimited(>dev,
1831 "write reg 0x%4.4x return err %d",
1832 regs[i].address, ret);
1833 return ret;
1834 }
1835 } Same as the code shown on your client?
>
> Best regards,
> Tomasz
>
Re: [PATCH v5] media: add imx319 camera sensor driver
Ack. On 09/17/2018 07:34 PM, Sakari Ailus wrote: > Hi Bingbu, > > Thanks for the update! A few more small comments, I think we're done after > these. > > On Mon, Sep 17, 2018 at 01:57:52PM +0800, bingbu@intel.com wrote: >> From: Bingbu Cao >> >> Add a v4l2 sub-device driver for the Sony imx319 image sensor. >> This is a camera sensor using the i2c bus for control and the >> csi-2 bus for data. >> >> This driver supports following features: >> - manual exposure and analog/digital gain control support >> - vblank/hblank control support >> - 4 test patterns control support >> - vflip/hflip control support (will impact the output bayer order) >> - support following resolutions: >> - 3264x2448, 3280x2464 @ 30fps >> - 1936x1096, 1920x1080 @ 60fps >> - 1640x1232, 1640x922, 1296x736, 1280x720 @ 120fps >> - support 4 bayer orders output (via change v/hflip) >> - SRGGB10(default), SGRBG10, SGBRG10, SBGGR10 >> >> Signed-off-by: Bingbu Cao >> Signed-off-by: Tianshu Qiu >> >> --- >> >> This patch is based on sakari's media-tree git: >> https://git.linuxtv.org/sailus/media_tree.git/log/?h=for-4.20-1 >> >> Changes from v4 to v5: >> - use single PLL for all internal clocks >> - change link frequency to 482.4MHz >> - adjust frame timing for 2x2 binning modes >>and enlarge frame readout time >> - get CSI-2 link frequencies and external clock >>from firmware >> - use unlocked __v4l2_ctrl_grab() with change from: >>https://git.linuxtv.org/sailus/media_tree.git/commit/?h=unlocked-ctrl-grab >> >> Changes since v1: >> - fix some coding style issues - line breaks >> - add v4l2_ctrl_grab() to prevent v/hflip change >>during streaming >> - add v4l2 ctrl event (un)subscribe support >> - add more info into commit message >> >> --- >> --- >> MAINTAINERS|7 + >> drivers/media/i2c/Kconfig | 11 + >> drivers/media/i2c/Makefile |1 + >> drivers/media/i2c/imx319.c | 2524 >> >> 4 files changed, 2543 insertions(+) >> create mode 100644 drivers/media/i2c/imx319.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index a5b256b25905..abc4abb6f83c 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -13530,6 +13530,13 @@ S: Maintained >> F: drivers/media/i2c/imx274.c >> F: Documentation/devicetree/bindings/media/i2c/imx274.txt >> >> +SONY IMX319 SENSOR DRIVER >> +M: Bingbu Cao >> +L: linux-media@vger.kernel.org >> +T: git git://linuxtv.org/media_tree.git >> +S: Maintained >> +F: drivers/media/i2c/imx319.c >> + >> SONY MEMORYSTICK CARD SUPPORT >> M: Alex Dubov >> W: http://tifmxx.berlios.de/ >> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig >> index bfdb494686bf..603ac087975b 100644 >> --- a/drivers/media/i2c/Kconfig >> +++ b/drivers/media/i2c/Kconfig >> @@ -614,6 +614,17 @@ config VIDEO_IMX274 >>This is a V4L2 sensor driver for the Sony IMX274 >>CMOS image sensor. >> >> +config VIDEO_IMX319 >> +tristate "Sony IMX319 sensor support" >> +depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API >> +depends on MEDIA_CAMERA_SUPPORT >> +help >> + This is a Video4Linux2 sensor driver for the Sony >> + IMX319 camera. >> + >> + To compile this driver as a module, choose M here: the >> + module will be called imx319. >> + >> config VIDEO_OV2640 >> tristate "OmniVision OV2640 sensor support" >> depends on VIDEO_V4L2 && I2C >> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile >> index a94eb03d10d4..d10b438577be 100644 >> --- a/drivers/media/i2c/Makefile >> +++ b/drivers/media/i2c/Makefile >> @@ -108,5 +108,6 @@ 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_VIDEO_IMX319) += imx319.o >> >> obj-$(CONFIG_SDR_MAX2175) += max2175.o >> diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c >> new file mode 100644 >> index ..43c28c701431 >> --- /dev/null >> +++ b/drivers/media/i2c/imx319.c >> @@ -0,0 +1,2524 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// Copyright (C) 2018 Intel Corporation >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#define IMX319_REG_MODE_SELECT 0x0100 >> +#define IMX319_MODE_STANDBY 0x00 >> +#define IMX319_MODE_STREAMING 0x01 >> + >> +/* Chip ID */ >> +#define IMX319_REG_CHIP_ID 0x0016 >> +#define IMX319_CHIP_ID 0x0319 >> + >> +/* V_TIMING internal */ >> +#define IMX319_REG_FLL 0x0340 >> +#define IMX319_FLL_MAX 0x >> + >> +/* Exposure control */ >> +#define IMX319_REG_EXPOSURE 0x0202 >> +#define IMX319_EXPOSURE_MIN 1 >> +#define IMX319_EXPOSURE_STEP1 >> +#define
Re: [PATCH v5] media: add imx319 camera sensor driver
Hi Bingbu,
On Mon, Sep 17, 2018 at 2:53 PM wrote:
[snip]
> +static int imx319_update_digital_gain(struct imx319 *imx319, u32 d_gain)
> +{
> + int ret;
> +
> + ret = imx319_write_reg(imx319, IMX319_REG_DPGA_USE_GLOBAL_GAIN, 1, 1);
> + if (ret)
> + return ret;
> +
> + /* Digital gain = (d_gain & 0xFF00) + (d_gain & 0xFF)/256 times */
What's the unit here?
Is the equation above really correct? The range, besides ~0, would be
from 256.0 to 65280 + 255/256, which sounds strange.
> + return imx319_write_reg(imx319, IMX319_REG_DIG_GAIN_GLOBAL, 2,
> d_gain);
> +}
> +
> +static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct imx319 *imx319 = container_of(ctrl->handler,
> +struct imx319, ctrl_handler);
> + struct i2c_client *client = v4l2_get_subdevdata(>sd);
> + s64 max;
> + int ret;
> +
> + /* Propagate change of current control to all related controls */
> + switch (ctrl->id) {
> + case V4L2_CID_VBLANK:
> + /* Update max exposure while meeting expected vblanking */
> + max = imx319->cur_mode->height + ctrl->val - 18;
> + __v4l2_ctrl_modify_range(imx319->exposure,
> +imx319->exposure->minimum,
> +max, imx319->exposure->step, max);
> + break;
> + }
> +
> + /*
> +* Applying V4L2 control value only happens
> +* when power is up for streaming
> +*/
> + if (pm_runtime_get_if_in_use(>dev) == 0)
nit: if (!pm_runtime_get_if_in_use(>dev)
> + return 0;
> +
[snip]
> +/* Initialize control handlers */
> +static int imx319_init_controls(struct imx319 *imx319)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(>sd);
> + struct v4l2_ctrl_handler *ctrl_hdlr;
> + s64 exposure_max;
> + s64 vblank_def;
> + s64 vblank_min;
> + s64 hblank;
> + s64 pixel_rate;
> + const struct imx319_mode *mode;
> + int ret;
> +
> + ctrl_hdlr = >ctrl_handler;
> + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
> + if (ret)
> + return ret;
> +
> + ctrl_hdlr->lock = >mutex;
> + imx319->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> _ctrl_ops,
> + V4L2_CID_LINK_FREQ, 0, 0,
> + imx319->pdata->link_freqs);
> + if (imx319->link_freq)
> + imx319->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + /* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> + pixel_rate = (imx319->link_def_freq * 2 * 4) / 10;
> + /* By default, PIXEL_RATE is read only */
> + imx319->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
> + V4L2_CID_PIXEL_RATE,
> pixel_rate,
> + pixel_rate, 1, pixel_rate);
> +
> + /* Initialze vblank/hblank/exposure parameters based on current mode
> */
> + mode = imx319->cur_mode;
> + vblank_def = mode->fll_def - mode->height;
> + vblank_min = mode->fll_min - mode->height;
> + imx319->vblank = v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
> + V4L2_CID_VBLANK, vblank_min,
> + IMX319_FLL_MAX - mode->height,
> + 1, vblank_def);
> +
> + hblank = mode->llp - mode->width;
> + imx319->hblank = v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
> + V4L2_CID_HBLANK, hblank, hblank,
> + 1, hblank);
> + if (imx319->hblank)
> + imx319->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + exposure_max = mode->fll_def - 18;
> + imx319->exposure = v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
> +V4L2_CID_EXPOSURE,
> +IMX319_EXPOSURE_MIN,
> exposure_max,
> +IMX319_EXPOSURE_STEP,
> +IMX319_EXPOSURE_DEFAULT);
Please explain how to interpret the exposure value in a comment.
> +
> + imx319->hflip = v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
> + V4L2_CID_HFLIP, 0, 1, 1, 0);
> + imx319->vflip = v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops,
> + V4L2_CID_VFLIP, 0, 1, 1, 0);
> +
> + v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
> + IMX319_ANA_GAIN_MIN, IMX319_ANA_GAIN_MAX,
> + IMX319_ANA_GAIN_STEP, IMX319_ANA_GAIN_DEFAULT);
Please explain how the gain value and in what units is calculated in a comment.
> +
> +
Re: [PATCH v5] media: add imx319 camera sensor driver
Hi Bingbu,
On Mon, Sep 17, 2018 at 2:53 PM wrote:
>
> From: Bingbu Cao
>
> Add a v4l2 sub-device driver for the Sony imx319 image sensor.
> This is a camera sensor using the i2c bus for control and the
> csi-2 bus for data.
Please see my comments inline. Also, I'd appreciate being CCed on
related work in the future.
[snip]
> +
> +static const char * const imx319_test_pattern_menu[] = {
> + "Disabled",
> + "100% color bars",
> + "Solid color",
> + "Fade to gray color bars",
> + "PN9"
> +};
> +
> +static const int imx319_test_pattern_val[] = {
> + IMX319_TEST_PATTERN_DISABLED,
> + IMX319_TEST_PATTERN_COLOR_BARS,
> + IMX319_TEST_PATTERN_SOLID_COLOR,
> + IMX319_TEST_PATTERN_GRAY_COLOR_BARS,
> + IMX319_TEST_PATTERN_PN9,
> +};
This array is not needed. All the entries are equal to corresponding
indices, i.e. the array is equivalent to { 0, 1, 2, 3, 4 }. We can use
ctrl->val directly.
[snip]
> +/* Write a list of registers */
> +static int imx319_write_regs(struct imx319 *imx319,
> + const struct imx319_reg *regs, u32 len)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(>sd);
> + int ret;
> + u32 i;
> +
> + for (i = 0; i < len; i++) {
> + ret = imx319_write_reg(imx319, regs[i].address, 1,
> regs[i].val);
> + if (ret) {
> + dev_err_ratelimited(>dev,
> +
Hmm, the message is clipped here. Let me see if it's something with my
email client...
Best regards,
Tomasz
Re: [PATCH v5] media: add imx319 camera sensor driver
Hi Bingbu, Thanks for the update! A few more small comments, I think we're done after these. On Mon, Sep 17, 2018 at 01:57:52PM +0800, bingbu@intel.com wrote: > From: Bingbu Cao > > Add a v4l2 sub-device driver for the Sony imx319 image sensor. > This is a camera sensor using the i2c bus for control and the > csi-2 bus for data. > > This driver supports following features: > - manual exposure and analog/digital gain control support > - vblank/hblank control support > - 4 test patterns control support > - vflip/hflip control support (will impact the output bayer order) > - support following resolutions: > - 3264x2448, 3280x2464 @ 30fps > - 1936x1096, 1920x1080 @ 60fps > - 1640x1232, 1640x922, 1296x736, 1280x720 @ 120fps > - support 4 bayer orders output (via change v/hflip) > - SRGGB10(default), SGRBG10, SGBRG10, SBGGR10 > > Signed-off-by: Bingbu Cao > Signed-off-by: Tianshu Qiu > > --- > > This patch is based on sakari's media-tree git: > https://git.linuxtv.org/sailus/media_tree.git/log/?h=for-4.20-1 > > Changes from v4 to v5: > - use single PLL for all internal clocks > - change link frequency to 482.4MHz > - adjust frame timing for 2x2 binning modes >and enlarge frame readout time > - get CSI-2 link frequencies and external clock >from firmware > - use unlocked __v4l2_ctrl_grab() with change from: >https://git.linuxtv.org/sailus/media_tree.git/commit/?h=unlocked-ctrl-grab > > Changes since v1: > - fix some coding style issues - line breaks > - add v4l2_ctrl_grab() to prevent v/hflip change >during streaming > - add v4l2 ctrl event (un)subscribe support > - add more info into commit message > > --- > --- > MAINTAINERS|7 + > drivers/media/i2c/Kconfig | 11 + > drivers/media/i2c/Makefile |1 + > drivers/media/i2c/imx319.c | 2524 > > 4 files changed, 2543 insertions(+) > create mode 100644 drivers/media/i2c/imx319.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index a5b256b25905..abc4abb6f83c 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -13530,6 +13530,13 @@ S: Maintained > F: drivers/media/i2c/imx274.c > F: Documentation/devicetree/bindings/media/i2c/imx274.txt > > +SONY IMX319 SENSOR DRIVER > +M: Bingbu Cao > +L: linux-media@vger.kernel.org > +T: git git://linuxtv.org/media_tree.git > +S: Maintained > +F: drivers/media/i2c/imx319.c > + > SONY MEMORYSTICK CARD SUPPORT > M: Alex Dubov > W: http://tifmxx.berlios.de/ > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index bfdb494686bf..603ac087975b 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -614,6 +614,17 @@ config VIDEO_IMX274 > This is a V4L2 sensor driver for the Sony IMX274 > CMOS image sensor. > > +config VIDEO_IMX319 > + tristate "Sony IMX319 sensor support" > + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API > + depends on MEDIA_CAMERA_SUPPORT > + help > + This is a Video4Linux2 sensor driver for the Sony > + IMX319 camera. > + > + To compile this driver as a module, choose M here: the > + module will be called imx319. > + > config VIDEO_OV2640 > tristate "OmniVision OV2640 sensor support" > depends on VIDEO_V4L2 && I2C > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > index a94eb03d10d4..d10b438577be 100644 > --- a/drivers/media/i2c/Makefile > +++ b/drivers/media/i2c/Makefile > @@ -108,5 +108,6 @@ 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_VIDEO_IMX319) += imx319.o > > obj-$(CONFIG_SDR_MAX2175) += max2175.o > diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c > new file mode 100644 > index ..43c28c701431 > --- /dev/null > +++ b/drivers/media/i2c/imx319.c > @@ -0,0 +1,2524 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (C) 2018 Intel Corporation > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define IMX319_REG_MODE_SELECT 0x0100 > +#define IMX319_MODE_STANDBY 0x00 > +#define IMX319_MODE_STREAMING0x01 > + > +/* Chip ID */ > +#define IMX319_REG_CHIP_ID 0x0016 > +#define IMX319_CHIP_ID 0x0319 > + > +/* V_TIMING internal */ > +#define IMX319_REG_FLL 0x0340 > +#define IMX319_FLL_MAX 0x > + > +/* Exposure control */ > +#define IMX319_REG_EXPOSURE 0x0202 > +#define IMX319_EXPOSURE_MIN 1 > +#define IMX319_EXPOSURE_STEP 1 > +#define IMX319_EXPOSURE_DEFAULT 0x04f6 > + > +/* Analog gain control */ > +#define IMX319_REG_ANALOG_GAIN 0x0204 > +#define IMX319_ANA_GAIN_MIN 0 > +#define
Re: [PATCH v5] media: add imx319 camera sensor driver
Hi Bingbu,
I love your patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.19-rc4 next-20180913]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/bingbu-cao-intel-com/media-add-imx319-camera-sensor-driver/20180917-135744
base: git://linuxtv.org/media_tree.git master
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/media/i2c/imx319.c: In function 'imx319_set_stream':
>> drivers/media/i2c/imx319.c:2146:2: error: implicit declaration of function
>> '__v4l2_ctrl_grab'; did you mean 'v4l2_ctrl_grab'?
>> [-Werror=implicit-function-declaration]
__v4l2_ctrl_grab(imx319->vflip, enable);
^~~~
v4l2_ctrl_grab
cc1: some warnings being treated as errors
vim +2146 drivers/media/i2c/imx319.c
2111
2112 static int imx319_set_stream(struct v4l2_subdev *sd, int enable)
2113 {
2114 struct imx319 *imx319 = to_imx319(sd);
2115 struct i2c_client *client = v4l2_get_subdevdata(sd);
2116 int ret = 0;
2117
2118 mutex_lock(>mutex);
2119 if (imx319->streaming == enable) {
2120 mutex_unlock(>mutex);
2121 return 0;
2122 }
2123
2124 if (enable) {
2125 ret = pm_runtime_get_sync(>dev);
2126 if (ret < 0) {
2127 pm_runtime_put_noidle(>dev);
2128 goto err_unlock;
2129 }
2130
2131 /*
2132 * Apply default & customized values
2133 * and then start streaming.
2134 */
2135 ret = imx319_start_streaming(imx319);
2136 if (ret)
2137 goto err_rpm_put;
2138 } else {
2139 imx319_stop_streaming(imx319);
2140 pm_runtime_put(>dev);
2141 }
2142
2143 imx319->streaming = enable;
2144
2145 /* vflip and hflip cannot change during streaming */
> 2146 __v4l2_ctrl_grab(imx319->vflip, enable);
2147 __v4l2_ctrl_grab(imx319->hflip, enable);
2148
2149 mutex_unlock(>mutex);
2150
2151 return ret;
2152
2153 err_rpm_put:
2154 pm_runtime_put(>dev);
2155 err_unlock:
2156 mutex_unlock(>mutex);
2157
2158 return ret;
2159 }
2160
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
