Add the effective and active sensor sizes and add functionality to read
rotation and orientation from device trees.
Signed-off-by: André Apitzsch <[email protected]>
---
André Apitzsch (4):
media: i2c: imx214: Explain some magic numbers
media: i2c: imx214: Move controls init to separate function
media: i2c: imx214: Read orientation and rotation from system firmware
media: i2c: imx214: Add sensor's pixel matrix size
drivers/media/i2c/imx214.c | 171 +++++++++++++++++++++++++++++++--------------
1 file changed, 117 insertions(+), 54 deletions(-)
---
base-commit: e8361b005d7c92997d12f2b85a9e4a525738bd9d
change-id: 20231023-imx214-68c438ebfb0c
Best regards,
--
André Apitzsch <[email protected]>
Code refinement, no functional changes.
Signed-off-by: André Apitzsch <[email protected]>
---
drivers/media/i2c/imx214.c | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 4f77ea02cc27..9218c149d4c8 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -19,12 +19,23 @@
#include <media/v4l2-fwnode.h>
#include <media/v4l2-subdev.h>
+#define IMX214_REG_MODE_SELECT 0x0100
+#define IMX214_MODE_STANDBY 0x00
+#define IMX214_MODE_STREAMING 0x01
+
#define IMX214_DEFAULT_CLK_FREQ 24000000
#define IMX214_DEFAULT_LINK_FREQ 480000000
#define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10)
#define IMX214_FPS 30
#define IMX214_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10
+/* Exposure control */
+#define IMX214_REG_EXPOSURE 0x0202
+#define IMX214_EXPOSURE_MIN 0
+#define IMX214_EXPOSURE_MAX 3184
+#define IMX214_EXPOSURE_STEP 1
+#define IMX214_EXPOSURE_DEFAULT 0x0c70
+
static const char * const imx214_supply_name[] = {
"vdda",
"vddd",
@@ -665,7 +676,7 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
case V4L2_CID_EXPOSURE:
vals[1] = ctrl->val;
vals[0] = ctrl->val >> 8;
- ret = regmap_bulk_write(imx214->regmap, 0x202, vals, 2);
+ ret = regmap_bulk_write(imx214->regmap, IMX214_REG_EXPOSURE, vals, 2);
if (ret < 0)
dev_err(imx214->dev, "Error %d\n", ret);
ret = 0;
@@ -743,7 +754,7 @@ static int imx214_start_streaming(struct imx214 *imx214)
dev_err(imx214->dev, "could not sync v4l2 controls\n");
goto error;
}
- ret = regmap_write(imx214->regmap, 0x100, 1);
+ ret = regmap_write(imx214->regmap, IMX214_REG_MODE_SELECT, IMX214_MODE_STREAMING);
if (ret < 0) {
dev_err(imx214->dev, "could not sent start table %d\n", ret);
goto error;
@@ -761,7 +772,7 @@ static int imx214_stop_streaming(struct imx214 *imx214)
{
int ret;
- ret = regmap_write(imx214->regmap, 0x100, 0);
+ ret = regmap_write(imx214->regmap, IMX214_REG_MODE_SELECT, IMX214_MODE_STANDBY);
if (ret < 0)
dev_err(imx214->dev, "could not sent stop table %d\n", ret);
@@ -991,9 +1002,12 @@ static int imx214_probe(struct i2c_client *client)
*
* Yours sincerely, Ricardo.
*/
- imx214->exposure = v4l2_ctrl_new_std(&imx214->ctrls, &imx214_ctrl_ops,
+ imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
V4L2_CID_EXPOSURE,
- 0, 3184, 1, 0x0c70);
+ IMX214_EXPOSURE_MIN,
+ IMX214_EXPOSURE_MAX,
+ IMX214_EXPOSURE_STEP,
+ IMX214_EXPOSURE_DEFAULT);
imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
NULL,
--
2.42.0
Obtain rotation and orientation information from system firmware and
register the appropriate controls.
Signed-off-by: André Apitzsch <[email protected]>
---
drivers/media/i2c/imx214.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 554fc4733128..bef8dc36e2d0 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -704,9 +704,14 @@ static int imx214_ctrls_init(struct imx214 *imx214)
.width = 1120,
.height = 1120,
};
+ struct v4l2_fwnode_device_properties props;
struct v4l2_ctrl_handler *ctrl_hdlr;
int ret;
+ ret = v4l2_fwnode_device_parse(imx214->dev, &props);
+ if (ret < 0)
+ return ret;
+
ctrl_hdlr = &imx214->ctrls;
ret = v4l2_ctrl_handler_init(&imx214->ctrls, 3);
if (ret)
@@ -746,6 +751,8 @@ static int imx214_ctrls_init(struct imx214 *imx214)
V4L2_CID_UNIT_CELL_SIZE,
v4l2_ctrl_ptr_create((void *)&unit_size));
+ v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx214_ctrl_ops, &props);
+
ret = ctrl_hdlr->error;
if (ret) {
v4l2_ctrl_handler_free(ctrl_hdlr);
--
2.42.0
Set effictive and active sensor pixel sizes as shown in product
brief[1].
[1]: https://www.mouser.com/datasheet/2/897/ProductBrief_IMX214_20150428-1289331.pdf
Signed-off-by: André Apitzsch <[email protected]>
---
drivers/media/i2c/imx214.c | 39 ++++++++++++++++++++++++++++++++-------
1 file changed, 32 insertions(+), 7 deletions(-)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index bef8dc36e2d0..a2d441cd8dcd 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -36,6 +36,14 @@
#define IMX214_EXPOSURE_STEP 1
#define IMX214_EXPOSURE_DEFAULT 0x0c70
+/* IMX214 native and active pixel array size */
+#define IMX214_NATIVE_WIDTH 4224U
+#define IMX214_NATIVE_HEIGHT 3136U
+#define IMX214_PIXEL_ARRAY_LEFT 8U
+#define IMX214_PIXEL_ARRAY_TOP 8U
+#define IMX214_PIXEL_ARRAY_WIDTH 4208U
+#define IMX214_PIXEL_ARRAY_HEIGHT 3120U
+
static const char * const imx214_supply_name[] = {
"vdda",
"vddd",
@@ -634,14 +642,31 @@ static int imx214_get_selection(struct v4l2_subdev *sd,
{
struct imx214 *imx214 = to_imx214(sd);
- if (sel->target != V4L2_SEL_TGT_CROP)
- return -EINVAL;
+ switch (sel->target) {
+ case V4L2_SEL_TGT_CROP:
+ mutex_lock(&imx214->mutex);
+ sel->r = *__imx214_get_pad_crop(imx214, sd_state, sel->pad,
+ sel->which);
+ mutex_unlock(&imx214->mutex);
+ return 0;
- mutex_lock(&imx214->mutex);
- sel->r = *__imx214_get_pad_crop(imx214, sd_state, sel->pad,
- sel->which);
- mutex_unlock(&imx214->mutex);
- return 0;
+ case V4L2_SEL_TGT_NATIVE_SIZE:
+ sel->r.top = 0;
+ sel->r.left = 0;
+ sel->r.width = IMX214_NATIVE_WIDTH;
+ sel->r.height = IMX214_NATIVE_HEIGHT;
+ return 0;
+
+ case V4L2_SEL_TGT_CROP_DEFAULT:
+ case V4L2_SEL_TGT_CROP_BOUNDS:
+ sel->r.top = IMX214_PIXEL_ARRAY_TOP;
+ sel->r.left = IMX214_PIXEL_ARRAY_LEFT;
+ sel->r.width = IMX214_PIXEL_ARRAY_WIDTH;
+ sel->r.height = IMX214_PIXEL_ARRAY_HEIGHT;
+ return 0;
+ }
+
+ return -EINVAL;
}
static int imx214_entity_init_cfg(struct v4l2_subdev *subdev,
--
2.42.0
Quoting André Apitzsch (2023-10-23 22:47:50)
> Code refinement, no functional changes.
>
> Signed-off-by: André Apitzsch <[email protected]>
> ---
> drivers/media/i2c/imx214.c | 24 +++++++++++++++++++-----
> 1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 4f77ea02cc27..9218c149d4c8 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -19,12 +19,23 @@
> #include <media/v4l2-fwnode.h>
> #include <media/v4l2-subdev.h>
>
> +#define IMX214_REG_MODE_SELECT 0x0100
> +#define IMX214_MODE_STANDBY 0x00
> +#define IMX214_MODE_STREAMING 0x01
> +
> #define IMX214_DEFAULT_CLK_FREQ 24000000
> #define IMX214_DEFAULT_LINK_FREQ 480000000
> #define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10)
> #define IMX214_FPS 30
> #define IMX214_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10
>
> +/* Exposure control */
> +#define IMX214_REG_EXPOSURE 0x0202
> +#define IMX214_EXPOSURE_MIN 0
> +#define IMX214_EXPOSURE_MAX 3184
> +#define IMX214_EXPOSURE_STEP 1
> +#define IMX214_EXPOSURE_DEFAULT 0x0c70
I like this change, and I see that 0x0c70 was directly moved here from
the code below. But could we replace 0xc70 here with 3184 please so that
it's /far/ clearer that the Exposure Default == Exposure Max which is
otherwise hidden?
--
Regards
Kieran
> +
> static const char * const imx214_supply_name[] = {
> "vdda",
> "vddd",
> @@ -665,7 +676,7 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
> case V4L2_CID_EXPOSURE:
> vals[1] = ctrl->val;
> vals[0] = ctrl->val >> 8;
> - ret = regmap_bulk_write(imx214->regmap, 0x202, vals, 2);
> + ret = regmap_bulk_write(imx214->regmap, IMX214_REG_EXPOSURE, vals, 2);
> if (ret < 0)
> dev_err(imx214->dev, "Error %d\n", ret);
> ret = 0;
> @@ -743,7 +754,7 @@ static int imx214_start_streaming(struct imx214 *imx214)
> dev_err(imx214->dev, "could not sync v4l2 controls\n");
> goto error;
> }
> - ret = regmap_write(imx214->regmap, 0x100, 1);
> + ret = regmap_write(imx214->regmap, IMX214_REG_MODE_SELECT, IMX214_MODE_STREAMING);
> if (ret < 0) {
> dev_err(imx214->dev, "could not sent start table %d\n", ret);
> goto error;
> @@ -761,7 +772,7 @@ static int imx214_stop_streaming(struct imx214 *imx214)
> {
> int ret;
>
> - ret = regmap_write(imx214->regmap, 0x100, 0);
> + ret = regmap_write(imx214->regmap, IMX214_REG_MODE_SELECT, IMX214_MODE_STANDBY);
> if (ret < 0)
> dev_err(imx214->dev, "could not sent stop table %d\n", ret);
>
> @@ -991,9 +1002,12 @@ static int imx214_probe(struct i2c_client *client)
> *
> * Yours sincerely, Ricardo.
> */
> - imx214->exposure = v4l2_ctrl_new_std(&imx214->ctrls, &imx214_ctrl_ops,
> + imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> V4L2_CID_EXPOSURE,
> - 0, 3184, 1, 0x0c70);
> + IMX214_EXPOSURE_MIN,
> + IMX214_EXPOSURE_MAX,
> + IMX214_EXPOSURE_STEP,
> + IMX214_EXPOSURE_DEFAULT);
>
> imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
> NULL,
>
> --
> 2.42.0
>
Hi Andre'
On Mon, Oct 23, 2023 at 11:47:52PM +0200, André Apitzsch wrote:
> Obtain rotation and orientation information from system firmware and
> register the appropriate controls.
>
> Signed-off-by: André Apitzsch <[email protected]>
> ---
> drivers/media/i2c/imx214.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 554fc4733128..bef8dc36e2d0 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -704,9 +704,14 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> .width = 1120,
> .height = 1120,
> };
> + struct v4l2_fwnode_device_properties props;
> struct v4l2_ctrl_handler *ctrl_hdlr;
> int ret;
>
> + ret = v4l2_fwnode_device_parse(imx214->dev, &props);
> + if (ret < 0)
> + return ret;
> +
> ctrl_hdlr = &imx214->ctrls;
> ret = v4l2_ctrl_handler_init(&imx214->ctrls, 3);
> if (ret)
> @@ -746,6 +751,8 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> V4L2_CID_UNIT_CELL_SIZE,
> v4l2_ctrl_ptr_create((void *)&unit_size));
>
> + v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx214_ctrl_ops, &props);
> +
I think it's fine not checking the return value of
v4l2_ctrl_new_fwnode_properties() if you inspect ctrl_hdlr->error just
after
Reviewed-by: Jacopo Mondi <[email protected]>
Thanks
j
> ret = ctrl_hdlr->error;
> if (ret) {
> v4l2_ctrl_handler_free(ctrl_hdlr);
>
> --
> 2.42.0
>
Am Montag, dem 23.10.2023 um 23:44 +0100 schrieb Kieran Bingham:
> Quoting André Apitzsch (2023-10-23 22:47:50)
> > Code refinement, no functional changes.
> >
> > Signed-off-by: André Apitzsch <[email protected]>
> > ---
> > drivers/media/i2c/imx214.c | 24 +++++++++++++++++++-----
> > 1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx214.c
> > b/drivers/media/i2c/imx214.c
> > index 4f77ea02cc27..9218c149d4c8 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -19,12 +19,23 @@
> > #include <media/v4l2-fwnode.h>
> > #include <media/v4l2-subdev.h>
> >
> > +#define IMX214_REG_MODE_SELECT 0x0100
> > +#define IMX214_MODE_STANDBY 0x00
> > +#define IMX214_MODE_STREAMING 0x01
> > +
> > #define IMX214_DEFAULT_CLK_FREQ 24000000
> > #define IMX214_DEFAULT_LINK_FREQ 480000000
> > #define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ *
> > 8LL) / 10)
> > #define IMX214_FPS 30
> > #define IMX214_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10
> >
> > +/* Exposure control */
> > +#define IMX214_REG_EXPOSURE 0x0202
> > +#define IMX214_EXPOSURE_MIN 0
> > +#define IMX214_EXPOSURE_MAX 3184
> > +#define IMX214_EXPOSURE_STEP 1
> > +#define IMX214_EXPOSURE_DEFAULT 0x0c70
>
> I like this change, and I see that 0x0c70 was directly moved here
> from
> the code below. But could we replace 0xc70 here with 3184 please so
> that
> it's /far/ clearer that the Exposure Default == Exposure Max which is
> otherwise hidden?
>
Hi Kieran,
I can do that. But I propose to replace 3184 with 0x0c70 instead.
Because that matches the entries used in the reg_8 lists
mode_4096x2304[] and mode_1920x1080[].
{0x0202, 0x0C},
{0x0203, 0x70},
What do you think?
Regards,
André
> --
> Regards
>
> Kieran
>
Hi Andre'
On Mon, Oct 23, 2023 at 11:47:53PM +0200, André Apitzsch wrote:
> Set effictive and active sensor pixel sizes as shown in product
s/effictive/effective
> brief[1].
>
> [1]: https://www.mouser.com/datasheet/2/897/ProductBrief_IMX214_20150428-1289331.pdf
>
> Signed-off-by: André Apitzsch <[email protected]>
> ---
> drivers/media/i2c/imx214.c | 39 ++++++++++++++++++++++++++++++++-------
> 1 file changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index bef8dc36e2d0..a2d441cd8dcd 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -36,6 +36,14 @@
> #define IMX214_EXPOSURE_STEP 1
> #define IMX214_EXPOSURE_DEFAULT 0x0c70
>
> +/* IMX214 native and active pixel array size */
> +#define IMX214_NATIVE_WIDTH 4224U
> +#define IMX214_NATIVE_HEIGHT 3136U
> +#define IMX214_PIXEL_ARRAY_LEFT 8U
> +#define IMX214_PIXEL_ARRAY_TOP 8U
> +#define IMX214_PIXEL_ARRAY_WIDTH 4208U
> +#define IMX214_PIXEL_ARRAY_HEIGHT 3120U
> +
I do get slightly different numbers from the datasheet version I have
The sensor is said to have 4224x3208 total pixels of which 4208x3120
are active ones.
The pixel array diagram shows 64 "OPB" (optically black ?) lines,
followed by 8 dummy lines, followed by 3120 valid lines. There are 8
dummy columns at each side of the 4208 valid ones.
Now, NATIVE which represents the full pixel array size seems to be
4224x3208 (other parts of the datasheet only report 3200 lines though)
BOUNDS represents the readabale array area, which I presume
corresponds to what is named as 'effective area' by the datasheet. It
excludes the OPB lines at the top of the image and seems to be
represented by (0, 64, 4224, 3160).
CROP_DEFAULT represents the default crop rectangle which covers the
active pixel area, so it excludes 8 more lines of dummy pixels and 8
dummy columns, which gives a rectangle (8, 72, 4208, 3120)
Also note that the driver always reports a TGT_CROP rectangle with
top/left points set to 0. If my understanding is correct, V4L2
selection targets are defined from the most external target
(TGT_NATIVE in this case), and the driver should be corrected to
initialize the crop rectangle with a top-left corner at (8, 72).
Does this make sense ?
Thanks
j
> static const char * const imx214_supply_name[] = {
> "vdda",
> "vddd",
> @@ -634,14 +642,31 @@ static int imx214_get_selection(struct v4l2_subdev *sd,
> {
> struct imx214 *imx214 = to_imx214(sd);
>
> - if (sel->target != V4L2_SEL_TGT_CROP)
> - return -EINVAL;
> + switch (sel->target) {
> + case V4L2_SEL_TGT_CROP:
> + mutex_lock(&imx214->mutex);
> + sel->r = *__imx214_get_pad_crop(imx214, sd_state, sel->pad,
> + sel->which);
> + mutex_unlock(&imx214->mutex);
> + return 0;
>
> - mutex_lock(&imx214->mutex);
> - sel->r = *__imx214_get_pad_crop(imx214, sd_state, sel->pad,
> - sel->which);
> - mutex_unlock(&imx214->mutex);
> - return 0;
> + case V4L2_SEL_TGT_NATIVE_SIZE:
> + sel->r.top = 0;
> + sel->r.left = 0;
> + sel->r.width = IMX214_NATIVE_WIDTH;
> + sel->r.height = IMX214_NATIVE_HEIGHT;
> + return 0;
> +
> + case V4L2_SEL_TGT_CROP_DEFAULT:
> + case V4L2_SEL_TGT_CROP_BOUNDS:
> + sel->r.top = IMX214_PIXEL_ARRAY_TOP;
> + sel->r.left = IMX214_PIXEL_ARRAY_LEFT;
> + sel->r.width = IMX214_PIXEL_ARRAY_WIDTH;
> + sel->r.height = IMX214_PIXEL_ARRAY_HEIGHT;
> + return 0;
> + }
> +
> + return -EINVAL;
> }
>
> static int imx214_entity_init_cfg(struct v4l2_subdev *subdev,
>
> --
> 2.42.0
>
Quoting André Apitzsch (2023-10-24 08:30:01)
> Am Montag, dem 23.10.2023 um 23:44 +0100 schrieb Kieran Bingham:
> > Quoting André Apitzsch (2023-10-23 22:47:50)
> > > Code refinement, no functional changes.
> > >
> > > Signed-off-by: André Apitzsch <[email protected]>
> > > ---
> > > drivers/media/i2c/imx214.c | 24 +++++++++++++++++++-----
> > > 1 file changed, 19 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx214.c
> > > b/drivers/media/i2c/imx214.c
> > > index 4f77ea02cc27..9218c149d4c8 100644
> > > --- a/drivers/media/i2c/imx214.c
> > > +++ b/drivers/media/i2c/imx214.c
> > > @@ -19,12 +19,23 @@
> > > #include <media/v4l2-fwnode.h>
> > > #include <media/v4l2-subdev.h>
> > >
> > > +#define IMX214_REG_MODE_SELECT 0x0100
> > > +#define IMX214_MODE_STANDBY 0x00
> > > +#define IMX214_MODE_STREAMING 0x01
> > > +
> > > #define IMX214_DEFAULT_CLK_FREQ 24000000
> > > #define IMX214_DEFAULT_LINK_FREQ 480000000
> > > #define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ *
> > > 8LL) / 10)
> > > #define IMX214_FPS 30
> > > #define IMX214_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10
> > >
> > > +/* Exposure control */
> > > +#define IMX214_REG_EXPOSURE 0x0202
> > > +#define IMX214_EXPOSURE_MIN 0
> > > +#define IMX214_EXPOSURE_MAX 3184
> > > +#define IMX214_EXPOSURE_STEP 1
> > > +#define IMX214_EXPOSURE_DEFAULT 0x0c70
> >
> > I like this change, and I see that 0x0c70 was directly moved here
> > from
> > the code below. But could we replace 0xc70 here with 3184 please so
> > that
> > it's /far/ clearer that the Exposure Default == Exposure Max which is
> > otherwise hidden?
> >
> Hi Kieran,
>
> I can do that. But I propose to replace 3184 with 0x0c70 instead.
> Because that matches the entries used in the reg_8 lists
> mode_4096x2304[] and mode_1920x1080[].
>
> {0x0202, 0x0C},
> {0x0203, 0x70},
>
> What do you think?
I think exposure values are easier to read as integers than hex values.
This is the 'Coarse Integration Time' register with a unit of 'lines'.
If you have lots of time, or wish to delve deeper - we could talk about
splitting out those register tables to use CCI and/or more readable
functions ;-)
Do you have the datasheet for this sensor - or are you just working from
the information within this driver?
What device are you using to test this driver?
--
Kieran
> Regards,
> André
Hi Jacopo,
Am Dienstag, dem 24.10.2023 um 09:52 +0200 schrieb Jacopo Mondi:
> Hi Andre'
>
> On Mon, Oct 23, 2023 at 11:47:53PM +0200, André Apitzsch wrote:
> > Set effictive and active sensor pixel sizes as shown in product
>
> s/effictive/effective
>
> > brief[1].
> >
> > [1]:
> > https://www.mouser.com/datasheet/2/897/ProductBrief_IMX214_20150428-1289331.pdf
> >
> > Signed-off-by: André Apitzsch <[email protected]>
> > ---
> > drivers/media/i2c/imx214.c | 39 ++++++++++++++++++++++++++++++++--
> > -----
> > 1 file changed, 32 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx214.c
> > b/drivers/media/i2c/imx214.c
> > index bef8dc36e2d0..a2d441cd8dcd 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -36,6 +36,14 @@
> > #define IMX214_EXPOSURE_STEP 1
> > #define IMX214_EXPOSURE_DEFAULT 0x0c70
> >
> > +/* IMX214 native and active pixel array size */
> > +#define IMX214_NATIVE_WIDTH 4224U
> > +#define IMX214_NATIVE_HEIGHT 3136U
> > +#define IMX214_PIXEL_ARRAY_LEFT 8U
> > +#define IMX214_PIXEL_ARRAY_TOP 8U
> > +#define IMX214_PIXEL_ARRAY_WIDTH 4208U
> > +#define IMX214_PIXEL_ARRAY_HEIGHT 3120U
> > +
>
> I do get slightly different numbers from the datasheet version I have
>
> The sensor is said to have 4224x3208 total pixels of which 4208x3120
> are active ones.
>
> The pixel array diagram shows 64 "OPB" (optically black ?) lines,
> followed by 8 dummy lines, followed by 3120 valid lines. There are 8
> dummy columns at each side of the 4208 valid ones.
>
> Now, NATIVE which represents the full pixel array size seems to be
> 4224x3208 (other parts of the datasheet only report 3200 lines
> though)
>
> BOUNDS represents the readabale array area, which I presume
> corresponds to what is named as 'effective area' by the datasheet. It
> excludes the OPB lines at the top of the image and seems to be
> represented by (0, 64, 4224, 3160).
>
> CROP_DEFAULT represents the default crop rectangle which covers the
> active pixel area, so it excludes 8 more lines of dummy pixels and 8
> dummy columns, which gives a rectangle (8, 72, 4208, 3120)
>
> Also note that the driver always reports a TGT_CROP rectangle with
> top/left points set to 0. If my understanding is correct, V4L2
> selection targets are defined from the most external target
> (TGT_NATIVE in this case), and the driver should be corrected to
> initialize the crop rectangle with a top-left corner at (8, 72).
>
> Does this make sense ?
As far as I understood, only the effective and active sizes of three
sizes provided in the datasheet (total, effective and active) matter.
By comparing the values used in imx219.c (and imx415.c) with the ones
in the corresponding datasheets [1,2] I assume, that "effective"
matches "NATIVE_SIZE", "active" matches "CROP_DEFAULT" and "total" is
ignored.
The commit message of 1ed36ecd1459b653cced8929bfb37dba94b64c5d ("media:
i2c: imx219: Selection compliance fixes") seems to support me here:
> The top/left crop coordinates of the TGT_CROP rectangle were set to
> (0, 0) instead of (8, 8) which is the offset from the larger physical
> pixel array rectangle.
This (8, 8) is half the difference between number of effective and
active pixels of imx219[1].
Together with the 8 dummy lines and 8 dummy columns you mentioned, I
still think my values are right. But I've just started working with
V4L2, so I might be wrong.
Could you share the imx214 datasheet with me?
Best regards,
André
[1] https://www.arducam.com/downloads/modules/RaspberryPi_camera/IMX219DS.PDF
[2] https://www.sony-semicon.com/files/62/pdf/p-12_IMX415-AAQR_AAMR_Flyer.pdf
>
> Thanks
> j
>
>
> > static const char * const imx214_supply_name[] = {
> > "vdda",
> > "vddd",
> > @@ -634,14 +642,31 @@ static int imx214_get_selection(struct
> > v4l2_subdev *sd,
> > {
> > struct imx214 *imx214 = to_imx214(sd);
> >
> > - if (sel->target != V4L2_SEL_TGT_CROP)
> > - return -EINVAL;
> > + switch (sel->target) {
> > + case V4L2_SEL_TGT_CROP:
> > + mutex_lock(&imx214->mutex);
> > + sel->r = *__imx214_get_pad_crop(imx214, sd_state,
> > sel->pad,
> > + sel->which);
> > + mutex_unlock(&imx214->mutex);
> > + return 0;
> >
> > - mutex_lock(&imx214->mutex);
> > - sel->r = *__imx214_get_pad_crop(imx214, sd_state, sel-
> > >pad,
> > - sel->which);
> > - mutex_unlock(&imx214->mutex);
> > - return 0;
> > + case V4L2_SEL_TGT_NATIVE_SIZE:
> > + sel->r.top = 0;
> > + sel->r.left = 0;
> > + sel->r.width = IMX214_NATIVE_WIDTH;
> > + sel->r.height = IMX214_NATIVE_HEIGHT;
> > + return 0;
> > +
> > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > + sel->r.top = IMX214_PIXEL_ARRAY_TOP;
> > + sel->r.left = IMX214_PIXEL_ARRAY_LEFT;
> > + sel->r.width = IMX214_PIXEL_ARRAY_WIDTH;
> > + sel->r.height = IMX214_PIXEL_ARRAY_HEIGHT;
> > + return 0;
> > + }
> > +
> > + return -EINVAL;
> > }
> >
> > static int imx214_entity_init_cfg(struct v4l2_subdev *subdev,
> >
> > --
> > 2.42.0
> >
Am Dienstag, dem 24.10.2023 um 13:52 +0100 schrieb Kieran Bingham:
> Quoting André Apitzsch (2023-10-24 08:30:01)
> > Am Montag, dem 23.10.2023 um 23:44 +0100 schrieb Kieran Bingham:
> > > Quoting André Apitzsch (2023-10-23 22:47:50)
> > > > Code refinement, no functional changes.
> > > >
> > > > Signed-off-by: André Apitzsch <[email protected]>
> > > > ---
> > > > drivers/media/i2c/imx214.c | 24 +++++++++++++++++++-----
> > > > 1 file changed, 19 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx214.c
> > > > b/drivers/media/i2c/imx214.c
> > > > index 4f77ea02cc27..9218c149d4c8 100644
> > > > --- a/drivers/media/i2c/imx214.c
> > > > +++ b/drivers/media/i2c/imx214.c
> > > > @@ -19,12 +19,23 @@
> > > > #include <media/v4l2-fwnode.h>
> > > > #include <media/v4l2-subdev.h>
> > > >
> > > > +#define IMX214_REG_MODE_SELECT 0x0100
> > > > +#define IMX214_MODE_STANDBY 0x00
> > > > +#define IMX214_MODE_STREAMING 0x01
> > > > +
> > > > #define IMX214_DEFAULT_CLK_FREQ 24000000
> > > > #define IMX214_DEFAULT_LINK_FREQ 480000000
> > > > #define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ *
> > > > 8LL) / 10)
> > > > #define IMX214_FPS 30
> > > > #define IMX214_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10
> > > >
> > > > +/* Exposure control */
> > > > +#define IMX214_REG_EXPOSURE 0x0202
> > > > +#define IMX214_EXPOSURE_MIN 0
> > > > +#define IMX214_EXPOSURE_MAX 3184
> > > > +#define IMX214_EXPOSURE_STEP 1
> > > > +#define IMX214_EXPOSURE_DEFAULT 0x0c70
> > >
> > > I like this change, and I see that 0x0c70 was directly moved here
> > > from
> > > the code below. But could we replace 0xc70 here with 3184 please
> > > so
> > > that
> > > it's /far/ clearer that the Exposure Default == Exposure Max
> > > which is
> > > otherwise hidden?
> > >
> > Hi Kieran,
> >
> > I can do that. But I propose to replace 3184 with 0x0c70 instead.
> > Because that matches the entries used in the reg_8 lists
> > mode_4096x2304[] and mode_1920x1080[].
> >
> > {0x0202, 0x0C},
> > {0x0203, 0x70},
> >
> > What do you think?
>
> I think exposure values are easier to read as integers than hex
> values.
Okay, integer it is.
>
> This is the 'Coarse Integration Time' register with a unit of
> 'lines'.
>
> If you have lots of time, or wish to delve deeper - we could talk
> about splitting out those register tables to use CCI and/or more
> readable functions ;-)
I will consider it.
>
> Do you have the datasheet for this sensor - or are you just working
> from the information within this driver?
I don't have the datasheet, yet (maybe somebody could provide it), so I
have to work with the information within this driver and downstream
implementations, like the one by nvidia [1].
>
> What device are you using to test this driver?
>
The sensor is part of my mobile, BQ Aquaris M5 (Longcheer L9100).
Best regards,
André
[1] https://github.com/InES-HPMM/linux-l4t/blob/9150157d2b1f73aefe01e9a07e3b062a45d57247/drivers/media/i2c/imx214_mode_tbls.h
> --
> Kieran
>
> > Regards,
> > André
Hi Andre'
On Wed, Oct 25, 2023 at 11:26:00PM +0200, André Apitzsch wrote:
> Hi Jacopo,
>
> Am Dienstag, dem 24.10.2023 um 09:52 +0200 schrieb Jacopo Mondi:
> > Hi Andre'
> >
> > On Mon, Oct 23, 2023 at 11:47:53PM +0200, André Apitzsch wrote:
> > > Set effictive and active sensor pixel sizes as shown in product
> >
> > s/effictive/effective
> >
> > > brief[1].
> > >
> > > [1]:
> > > https://www.mouser.com/datasheet/2/897/ProductBrief_IMX214_20150428-1289331.pdf
> > >
> > > Signed-off-by: André Apitzsch <[email protected]>
> > > ---
> > > drivers/media/i2c/imx214.c | 39 ++++++++++++++++++++++++++++++++--
> > > -----
> > > 1 file changed, 32 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx214.c
> > > b/drivers/media/i2c/imx214.c
> > > index bef8dc36e2d0..a2d441cd8dcd 100644
> > > --- a/drivers/media/i2c/imx214.c
> > > +++ b/drivers/media/i2c/imx214.c
> > > @@ -36,6 +36,14 @@
> > > #define IMX214_EXPOSURE_STEP 1
> > > #define IMX214_EXPOSURE_DEFAULT 0x0c70
> > >
> > > +/* IMX214 native and active pixel array size */
> > > +#define IMX214_NATIVE_WIDTH 4224U
> > > +#define IMX214_NATIVE_HEIGHT 3136U
> > > +#define IMX214_PIXEL_ARRAY_LEFT 8U
> > > +#define IMX214_PIXEL_ARRAY_TOP 8U
> > > +#define IMX214_PIXEL_ARRAY_WIDTH 4208U
> > > +#define IMX214_PIXEL_ARRAY_HEIGHT 3120U
> > > +
> >
> > I do get slightly different numbers from the datasheet version I have
> >
> > The sensor is said to have 4224x3208 total pixels of which 4208x3120
> > are active ones.
> >
> > The pixel array diagram shows 64 "OPB" (optically black ?) lines,
> > followed by 8 dummy lines, followed by 3120 valid lines. There are 8
> > dummy columns at each side of the 4208 valid ones.
> >
> > Now, NATIVE which represents the full pixel array size seems to be
> > 4224x3208 (other parts of the datasheet only report 3200 lines
> > though)
> >
> > BOUNDS represents the readabale array area, which I presume
> > corresponds to what is named as 'effective area' by the datasheet. It
> > excludes the OPB lines at the top of the image and seems to be
> > represented by (0, 64, 4224, 3160).
> >
> > CROP_DEFAULT represents the default crop rectangle which covers the
> > active pixel area, so it excludes 8 more lines of dummy pixels and 8
> > dummy columns, which gives a rectangle (8, 72, 4208, 3120)
> >
> > Also note that the driver always reports a TGT_CROP rectangle with
> > top/left points set to 0. If my understanding is correct, V4L2
> > selection targets are defined from the most external target
> > (TGT_NATIVE in this case), and the driver should be corrected to
> > initialize the crop rectangle with a top-left corner at (8, 72).
> >
> > Does this make sense ?
>
> As far as I understood, only the effective and active sizes of three
> sizes provided in the datasheet (total, effective and active) matter.
> By comparing the values used in imx219.c (and imx415.c) with the ones
> in the corresponding datasheets [1,2] I assume, that "effective"
> matches "NATIVE_SIZE", "active" matches "CROP_DEFAULT" and "total" is
> ignored.
imx219 driver indeed does not consider the OPB areas in the definition
of the rectangles...
Also looking at the X/Y_ADDR_START value assigned in the register tables
for full resolution mode (3280x2462) they have value of 0, indeed
meaning the active area is the only readable one.
Then yes, you're right, for imx219
NATIVE = effective
CROP_DEFAULT = BOUND = active
> The commit message of 1ed36ecd1459b653cced8929bfb37dba94b64c5d ("media:
> i2c: imx219: Selection compliance fixes") seems to support me here:
>
> > The top/left crop coordinates of the TGT_CROP rectangle were set to
> > (0, 0) instead of (8, 8) which is the offset from the larger physical
> > pixel array rectangle.
>
> This (8, 8) is half the difference between number of effective and
> active pixels of imx219[1].
>
> Together with the 8 dummy lines and 8 dummy columns you mentioned, I
> still think my values are right. But I've just started working with
> V4L2, so I might be wrong.
To actually verify if the 'effective area' is readable or not, we
should know what register controls the X/Y_ADDR_START value, and
that's an information I don't have in my version of the datasheet.
It's however plausible that it behaves the same as imx219, as the
driver's register sequences seems to program the crop sizes in
register 0x034c and 0x034e and there's not programmed top-left corner
there.
Ok then, let's be consistent and do the same as imx219 as you're doing
here.
Reviewed-by: Jacopo Mondi <[email protected]>
>
> Could you share the imx214 datasheet with me?
>
> Best regards,
> André
>
> [1] https://www.arducam.com/downloads/modules/RaspberryPi_camera/IMX219DS.PDF
> [2] https://www.sony-semicon.com/files/62/pdf/p-12_IMX415-AAQR_AAMR_Flyer.pdf
> >
> > Thanks
> > j
> >
> >
> > > static const char * const imx214_supply_name[] = {
> > > "vdda",
> > > "vddd",
> > > @@ -634,14 +642,31 @@ static int imx214_get_selection(struct
> > > v4l2_subdev *sd,
> > > {
> > > struct imx214 *imx214 = to_imx214(sd);
> > >
> > > - if (sel->target != V4L2_SEL_TGT_CROP)
> > > - return -EINVAL;
> > > + switch (sel->target) {
> > > + case V4L2_SEL_TGT_CROP:
> > > + mutex_lock(&imx214->mutex);
> > > + sel->r = *__imx214_get_pad_crop(imx214, sd_state,
> > > sel->pad,
> > > + sel->which);
> > > + mutex_unlock(&imx214->mutex);
> > > + return 0;
> > >
> > > - mutex_lock(&imx214->mutex);
> > > - sel->r = *__imx214_get_pad_crop(imx214, sd_state, sel-
> > > >pad,
> > > - sel->which);
> > > - mutex_unlock(&imx214->mutex);
> > > - return 0;
> > > + case V4L2_SEL_TGT_NATIVE_SIZE:
> > > + sel->r.top = 0;
> > > + sel->r.left = 0;
> > > + sel->r.width = IMX214_NATIVE_WIDTH;
> > > + sel->r.height = IMX214_NATIVE_HEIGHT;
> > > + return 0;
> > > +
> > > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > > + sel->r.top = IMX214_PIXEL_ARRAY_TOP;
> > > + sel->r.left = IMX214_PIXEL_ARRAY_LEFT;
> > > + sel->r.width = IMX214_PIXEL_ARRAY_WIDTH;
> > > + sel->r.height = IMX214_PIXEL_ARRAY_HEIGHT;
> > > + return 0;
> > > + }
> > > +
> > > + return -EINVAL;
> > > }
> > >
> > > static int imx214_entity_init_cfg(struct v4l2_subdev *subdev,
> > >
> > > --
> > > 2.42.0
> > >
>
Hi Jacopo
On Fri, 27 Oct 2023 at 09:57, Jacopo Mondi
<[email protected]> wrote:
>
> Hi Andre'
>
> On Wed, Oct 25, 2023 at 11:26:00PM +0200, André Apitzsch wrote:
> > Hi Jacopo,
> >
> > Am Dienstag, dem 24.10.2023 um 09:52 +0200 schrieb Jacopo Mondi:
> > > Hi Andre'
> > >
> > > On Mon, Oct 23, 2023 at 11:47:53PM +0200, André Apitzsch wrote:
> > > > Set effictive and active sensor pixel sizes as shown in product
> > >
> > > s/effictive/effective
> > >
> > > > brief[1].
> > > >
> > > > [1]:
> > > > https://www.mouser.com/datasheet/2/897/ProductBrief_IMX214_20150428-1289331.pdf
> > > >
> > > > Signed-off-by: André Apitzsch <[email protected]>
> > > > ---
> > > > drivers/media/i2c/imx214.c | 39 ++++++++++++++++++++++++++++++++--
> > > > -----
> > > > 1 file changed, 32 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx214.c
> > > > b/drivers/media/i2c/imx214.c
> > > > index bef8dc36e2d0..a2d441cd8dcd 100644
> > > > --- a/drivers/media/i2c/imx214.c
> > > > +++ b/drivers/media/i2c/imx214.c
> > > > @@ -36,6 +36,14 @@
> > > > #define IMX214_EXPOSURE_STEP 1
> > > > #define IMX214_EXPOSURE_DEFAULT 0x0c70
> > > >
> > > > +/* IMX214 native and active pixel array size */
> > > > +#define IMX214_NATIVE_WIDTH 4224U
> > > > +#define IMX214_NATIVE_HEIGHT 3136U
> > > > +#define IMX214_PIXEL_ARRAY_LEFT 8U
> > > > +#define IMX214_PIXEL_ARRAY_TOP 8U
> > > > +#define IMX214_PIXEL_ARRAY_WIDTH 4208U
> > > > +#define IMX214_PIXEL_ARRAY_HEIGHT 3120U
> > > > +
> > >
> > > I do get slightly different numbers from the datasheet version I have
> > >
> > > The sensor is said to have 4224x3208 total pixels of which 4208x3120
> > > are active ones.
> > >
> > > The pixel array diagram shows 64 "OPB" (optically black ?) lines,
> > > followed by 8 dummy lines, followed by 3120 valid lines. There are 8
> > > dummy columns at each side of the 4208 valid ones.
> > >
> > > Now, NATIVE which represents the full pixel array size seems to be
> > > 4224x3208 (other parts of the datasheet only report 3200 lines
> > > though)
> > >
> > > BOUNDS represents the readabale array area, which I presume
> > > corresponds to what is named as 'effective area' by the datasheet. It
> > > excludes the OPB lines at the top of the image and seems to be
> > > represented by (0, 64, 4224, 3160).
> > >
> > > CROP_DEFAULT represents the default crop rectangle which covers the
> > > active pixel area, so it excludes 8 more lines of dummy pixels and 8
> > > dummy columns, which gives a rectangle (8, 72, 4208, 3120)
> > >
> > > Also note that the driver always reports a TGT_CROP rectangle with
> > > top/left points set to 0. If my understanding is correct, V4L2
> > > selection targets are defined from the most external target
> > > (TGT_NATIVE in this case), and the driver should be corrected to
> > > initialize the crop rectangle with a top-left corner at (8, 72).
> > >
> > > Does this make sense ?
> >
> > As far as I understood, only the effective and active sizes of three
> > sizes provided in the datasheet (total, effective and active) matter.
> > By comparing the values used in imx219.c (and imx415.c) with the ones
> > in the corresponding datasheets [1,2] I assume, that "effective"
> > matches "NATIVE_SIZE", "active" matches "CROP_DEFAULT" and "total" is
> > ignored.
>
> imx219 driver indeed does not consider the OPB areas in the definition
> of the rectangles...
>
> Also looking at the X/Y_ADDR_START value assigned in the register tables
> for full resolution mode (3280x2462) they have value of 0, indeed
> meaning the active area is the only readable one.
>
> Then yes, you're right, for imx219
> NATIVE = effective
> CROP_DEFAULT = BOUND = active
>
> > The commit message of 1ed36ecd1459b653cced8929bfb37dba94b64c5d ("media:
> > i2c: imx219: Selection compliance fixes") seems to support me here:
>
> >
> > > The top/left crop coordinates of the TGT_CROP rectangle were set to
> > > (0, 0) instead of (8, 8) which is the offset from the larger physical
> > > pixel array rectangle.
> >
> > This (8, 8) is half the difference between number of effective and
> > active pixels of imx219[1].
> >
> > Together with the 8 dummy lines and 8 dummy columns you mentioned, I
> > still think my values are right. But I've just started working with
> > V4L2, so I might be wrong.
>
> To actually verify if the 'effective area' is readable or not, we
> should know what register controls the X/Y_ADDR_START value, and
> that's an information I don't have in my version of the datasheet.
I happen to have an IMX214 datasheet.
X_ADDR_START is 0x0344/5 (set in multiples of 2)
Y_ADDR_START is 0x0346/7 (set in multiples of 4)
X_ADDR_END is 0x0348/9 (set in multiples of 2)
Y_ADDR_END is 0x034a/b (set in multiples of 4)
X_OUTPUT_SIZE 0x034c/d
Y_OUTPUT_SIZE 0x034e/f
X direction are 13bit values, Y direction are 12 bit.
[12:8] or [11:8] in the low bits of the first register, [7:0] in the
second register.
Dave
> It's however plausible that it behaves the same as imx219, as the
> driver's register sequences seems to program the crop sizes in
> register 0x034c and 0x034e and there's not programmed top-left corner
> there.
>
> Ok then, let's be consistent and do the same as imx219 as you're doing
> here.
>
> Reviewed-by: Jacopo Mondi <[email protected]>
>
> >
> > Could you share the imx214 datasheet with me?
> >
> > Best regards,
> > André
> >
> > [1] https://www.arducam.com/downloads/modules/RaspberryPi_camera/IMX219DS.PDF
> > [2] https://www.sony-semicon.com/files/62/pdf/p-12_IMX415-AAQR_AAMR_Flyer.pdf
> > >
> > > Thanks
> > > j
> > >
> > >
> > > > static const char * const imx214_supply_name[] = {
> > > > "vdda",
> > > > "vddd",
> > > > @@ -634,14 +642,31 @@ static int imx214_get_selection(struct
> > > > v4l2_subdev *sd,
> > > > {
> > > > struct imx214 *imx214 = to_imx214(sd);
> > > >
> > > > - if (sel->target != V4L2_SEL_TGT_CROP)
> > > > - return -EINVAL;
> > > > + switch (sel->target) {
> > > > + case V4L2_SEL_TGT_CROP:
> > > > + mutex_lock(&imx214->mutex);
> > > > + sel->r = *__imx214_get_pad_crop(imx214, sd_state,
> > > > sel->pad,
> > > > + sel->which);
> > > > + mutex_unlock(&imx214->mutex);
> > > > + return 0;
> > > >
> > > > - mutex_lock(&imx214->mutex);
> > > > - sel->r = *__imx214_get_pad_crop(imx214, sd_state, sel-
> > > > >pad,
> > > > - sel->which);
> > > > - mutex_unlock(&imx214->mutex);
> > > > - return 0;
> > > > + case V4L2_SEL_TGT_NATIVE_SIZE:
> > > > + sel->r.top = 0;
> > > > + sel->r.left = 0;
> > > > + sel->r.width = IMX214_NATIVE_WIDTH;
> > > > + sel->r.height = IMX214_NATIVE_HEIGHT;
> > > > + return 0;
> > > > +
> > > > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > > > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > > > + sel->r.top = IMX214_PIXEL_ARRAY_TOP;
> > > > + sel->r.left = IMX214_PIXEL_ARRAY_LEFT;
> > > > + sel->r.width = IMX214_PIXEL_ARRAY_WIDTH;
> > > > + sel->r.height = IMX214_PIXEL_ARRAY_HEIGHT;
> > > > + return 0;
> > > > + }
> > > > +
> > > > + return -EINVAL;
> > > > }
> > > >
> > > > static int imx214_entity_init_cfg(struct v4l2_subdev *subdev,
> > > >
> > > > --
> > > > 2.42.0
> > > >
> >
Hi Dave
On Fri, Oct 27, 2023 at 11:29:11AM +0100, Dave Stevenson wrote:
> Hi Jacopo
>
> On Fri, 27 Oct 2023 at 09:57, Jacopo Mondi
> <[email protected]> wrote:
> >
> > Hi Andre'
> >
> > On Wed, Oct 25, 2023 at 11:26:00PM +0200, André Apitzsch wrote:
> > > Hi Jacopo,
> > >
> > > Am Dienstag, dem 24.10.2023 um 09:52 +0200 schrieb Jacopo Mondi:
> > > > Hi Andre'
> > > >
> > > > On Mon, Oct 23, 2023 at 11:47:53PM +0200, André Apitzsch wrote:
> > > > > Set effictive and active sensor pixel sizes as shown in product
> > > >
> > > > s/effictive/effective
> > > >
> > > > > brief[1].
> > > > >
> > > > > [1]:
> > > > > https://www.mouser.com/datasheet/2/897/ProductBrief_IMX214_20150428-1289331.pdf
> > > > >
> > > > > Signed-off-by: André Apitzsch <[email protected]>
> > > > > ---
> > > > > drivers/media/i2c/imx214.c | 39 ++++++++++++++++++++++++++++++++--
> > > > > -----
> > > > > 1 file changed, 32 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/imx214.c
> > > > > b/drivers/media/i2c/imx214.c
> > > > > index bef8dc36e2d0..a2d441cd8dcd 100644
> > > > > --- a/drivers/media/i2c/imx214.c
> > > > > +++ b/drivers/media/i2c/imx214.c
> > > > > @@ -36,6 +36,14 @@
> > > > > #define IMX214_EXPOSURE_STEP 1
> > > > > #define IMX214_EXPOSURE_DEFAULT 0x0c70
> > > > >
> > > > > +/* IMX214 native and active pixel array size */
> > > > > +#define IMX214_NATIVE_WIDTH 4224U
> > > > > +#define IMX214_NATIVE_HEIGHT 3136U
> > > > > +#define IMX214_PIXEL_ARRAY_LEFT 8U
> > > > > +#define IMX214_PIXEL_ARRAY_TOP 8U
> > > > > +#define IMX214_PIXEL_ARRAY_WIDTH 4208U
> > > > > +#define IMX214_PIXEL_ARRAY_HEIGHT 3120U
> > > > > +
> > > >
> > > > I do get slightly different numbers from the datasheet version I have
> > > >
> > > > The sensor is said to have 4224x3208 total pixels of which 4208x3120
> > > > are active ones.
> > > >
> > > > The pixel array diagram shows 64 "OPB" (optically black ?) lines,
> > > > followed by 8 dummy lines, followed by 3120 valid lines. There are 8
> > > > dummy columns at each side of the 4208 valid ones.
> > > >
> > > > Now, NATIVE which represents the full pixel array size seems to be
> > > > 4224x3208 (other parts of the datasheet only report 3200 lines
> > > > though)
> > > >
> > > > BOUNDS represents the readabale array area, which I presume
> > > > corresponds to what is named as 'effective area' by the datasheet. It
> > > > excludes the OPB lines at the top of the image and seems to be
> > > > represented by (0, 64, 4224, 3160).
> > > >
> > > > CROP_DEFAULT represents the default crop rectangle which covers the
> > > > active pixel area, so it excludes 8 more lines of dummy pixels and 8
> > > > dummy columns, which gives a rectangle (8, 72, 4208, 3120)
> > > >
> > > > Also note that the driver always reports a TGT_CROP rectangle with
> > > > top/left points set to 0. If my understanding is correct, V4L2
> > > > selection targets are defined from the most external target
> > > > (TGT_NATIVE in this case), and the driver should be corrected to
> > > > initialize the crop rectangle with a top-left corner at (8, 72).
> > > >
> > > > Does this make sense ?
> > >
> > > As far as I understood, only the effective and active sizes of three
> > > sizes provided in the datasheet (total, effective and active) matter.
> > > By comparing the values used in imx219.c (and imx415.c) with the ones
> > > in the corresponding datasheets [1,2] I assume, that "effective"
> > > matches "NATIVE_SIZE", "active" matches "CROP_DEFAULT" and "total" is
> > > ignored.
> >
> > imx219 driver indeed does not consider the OPB areas in the definition
> > of the rectangles...
> >
I know it sounds ridiculous as I've been the one adding selection
support to imx219, but I presume we discussed it somewhen in the past:
do you happen to remember why we left the OPB area out from the native
sizes ? (Does OPB stand for "Optically black" ? )
> > Also looking at the X/Y_ADDR_START value assigned in the register tables
> > for full resolution mode (3280x2462) they have value of 0, indeed
> > meaning the active area is the only readable one.
> >
> > Then yes, you're right, for imx219
> > NATIVE = effective
> > CROP_DEFAULT = BOUND = active
> >
I presume you can confirm this, right ?
> > > The commit message of 1ed36ecd1459b653cced8929bfb37dba94b64c5d ("media:
> > > i2c: imx219: Selection compliance fixes") seems to support me here:
> >
> > >
> > > > The top/left crop coordinates of the TGT_CROP rectangle were set to
> > > > (0, 0) instead of (8, 8) which is the offset from the larger physical
> > > > pixel array rectangle.
> > >
> > > This (8, 8) is half the difference between number of effective and
> > > active pixels of imx219[1].
> > >
> > > Together with the 8 dummy lines and 8 dummy columns you mentioned, I
> > > still think my values are right. But I've just started working with
> > > V4L2, so I might be wrong.
> >
> > To actually verify if the 'effective area' is readable or not, we
> > should know what register controls the X/Y_ADDR_START value, and
> > that's an information I don't have in my version of the datasheet.
>
> I happen to have an IMX214 datasheet.
> X_ADDR_START is 0x0344/5 (set in multiples of 2)
> Y_ADDR_START is 0x0346/7 (set in multiples of 4)
> X_ADDR_END is 0x0348/9 (set in multiples of 2)
> Y_ADDR_END is 0x034a/b (set in multiples of 4)
> X_OUTPUT_SIZE 0x034c/d
> Y_OUTPUT_SIZE 0x034e/f
>
> X direction are 13bit values, Y direction are 12 bit.
> [12:8] or [11:8] in the low bits of the first register, [7:0] in the
> second register.
AH thanks! Unfortunately the largest imx214 mode is cropped from full
pixel array it seems, so not that helpful :(
>
> Dave
>
> > It's however plausible that it behaves the same as imx219, as the
> > driver's register sequences seems to program the crop sizes in
> > register 0x034c and 0x034e and there's not programmed top-left corner
> > there.
> >
> > Ok then, let's be consistent and do the same as imx219 as you're doing
> > here.
> >
> > Reviewed-by: Jacopo Mondi <[email protected]>
> >
> > >
> > > Could you share the imx214 datasheet with me?
> > >
> > > Best regards,
> > > André
> > >
> > > [1] https://www.arducam.com/downloads/modules/RaspberryPi_camera/IMX219DS.PDF
> > > [2] https://www.sony-semicon.com/files/62/pdf/p-12_IMX415-AAQR_AAMR_Flyer.pdf
> > > >
> > > > Thanks
> > > > j
> > > >
> > > >
> > > > > static const char * const imx214_supply_name[] = {
> > > > > "vdda",
> > > > > "vddd",
> > > > > @@ -634,14 +642,31 @@ static int imx214_get_selection(struct
> > > > > v4l2_subdev *sd,
> > > > > {
> > > > > struct imx214 *imx214 = to_imx214(sd);
> > > > >
> > > > > - if (sel->target != V4L2_SEL_TGT_CROP)
> > > > > - return -EINVAL;
> > > > > + switch (sel->target) {
> > > > > + case V4L2_SEL_TGT_CROP:
> > > > > + mutex_lock(&imx214->mutex);
> > > > > + sel->r = *__imx214_get_pad_crop(imx214, sd_state,
> > > > > sel->pad,
> > > > > + sel->which);
> > > > > + mutex_unlock(&imx214->mutex);
> > > > > + return 0;
> > > > >
> > > > > - mutex_lock(&imx214->mutex);
> > > > > - sel->r = *__imx214_get_pad_crop(imx214, sd_state, sel-
> > > > > >pad,
> > > > > - sel->which);
> > > > > - mutex_unlock(&imx214->mutex);
> > > > > - return 0;
> > > > > + case V4L2_SEL_TGT_NATIVE_SIZE:
> > > > > + sel->r.top = 0;
> > > > > + sel->r.left = 0;
> > > > > + sel->r.width = IMX214_NATIVE_WIDTH;
> > > > > + sel->r.height = IMX214_NATIVE_HEIGHT;
> > > > > + return 0;
> > > > > +
> > > > > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > > > > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > > > > + sel->r.top = IMX214_PIXEL_ARRAY_TOP;
> > > > > + sel->r.left = IMX214_PIXEL_ARRAY_LEFT;
> > > > > + sel->r.width = IMX214_PIXEL_ARRAY_WIDTH;
> > > > > + sel->r.height = IMX214_PIXEL_ARRAY_HEIGHT;
> > > > > + return 0;
> > > > > + }
> > > > > +
> > > > > + return -EINVAL;
> > > > > }
> > > > >
> > > > > static int imx214_entity_init_cfg(struct v4l2_subdev *subdev,
> > > > >
> > > > > --
> > > > > 2.42.0
> > > > >
> > >
Hi André
Thanks for your patch!
I usually do not care about creating a define for something that I use
only once... but if you think this is more clear, let it be :)
On Mon, Oct 23, 2023 at 11:49 PM André Apitzsch <[email protected]> wrote:
>
> Code refinement, no functional changes.
>
> Signed-off-by: André Apitzsch <[email protected]>
Reviewed-by: Ricardo Ribalda <[email protected]>
> ---
> drivers/media/i2c/imx214.c | 24 +++++++++++++++++++-----
> 1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 4f77ea02cc27..9218c149d4c8 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -19,12 +19,23 @@
> #include <media/v4l2-fwnode.h>
> #include <media/v4l2-subdev.h>
>
> +#define IMX214_REG_MODE_SELECT 0x0100
> +#define IMX214_MODE_STANDBY 0x00
> +#define IMX214_MODE_STREAMING 0x01
> +
> #define IMX214_DEFAULT_CLK_FREQ 24000000
> #define IMX214_DEFAULT_LINK_FREQ 480000000
> #define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10)
> #define IMX214_FPS 30
> #define IMX214_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10
>
> +/* Exposure control */
> +#define IMX214_REG_EXPOSURE 0x0202
> +#define IMX214_EXPOSURE_MIN 0
> +#define IMX214_EXPOSURE_MAX 3184
> +#define IMX214_EXPOSURE_STEP 1
> +#define IMX214_EXPOSURE_DEFAULT 0x0c70
> +
> static const char * const imx214_supply_name[] = {
> "vdda",
> "vddd",
> @@ -665,7 +676,7 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
> case V4L2_CID_EXPOSURE:
> vals[1] = ctrl->val;
> vals[0] = ctrl->val >> 8;
> - ret = regmap_bulk_write(imx214->regmap, 0x202, vals, 2);
> + ret = regmap_bulk_write(imx214->regmap, IMX214_REG_EXPOSURE, vals, 2);
> if (ret < 0)
> dev_err(imx214->dev, "Error %d\n", ret);
> ret = 0;
> @@ -743,7 +754,7 @@ static int imx214_start_streaming(struct imx214 *imx214)
> dev_err(imx214->dev, "could not sync v4l2 controls\n");
> goto error;
> }
> - ret = regmap_write(imx214->regmap, 0x100, 1);
> + ret = regmap_write(imx214->regmap, IMX214_REG_MODE_SELECT, IMX214_MODE_STREAMING);
> if (ret < 0) {
> dev_err(imx214->dev, "could not sent start table %d\n", ret);
> goto error;
> @@ -761,7 +772,7 @@ static int imx214_stop_streaming(struct imx214 *imx214)
> {
> int ret;
>
> - ret = regmap_write(imx214->regmap, 0x100, 0);
> + ret = regmap_write(imx214->regmap, IMX214_REG_MODE_SELECT, IMX214_MODE_STANDBY);
> if (ret < 0)
> dev_err(imx214->dev, "could not sent stop table %d\n", ret);
>
> @@ -991,9 +1002,12 @@ static int imx214_probe(struct i2c_client *client)
> *
> * Yours sincerely, Ricardo.
> */
> - imx214->exposure = v4l2_ctrl_new_std(&imx214->ctrls, &imx214_ctrl_ops,
> + imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> V4L2_CID_EXPOSURE,
> - 0, 3184, 1, 0x0c70);
> + IMX214_EXPOSURE_MIN,
> + IMX214_EXPOSURE_MAX,
> + IMX214_EXPOSURE_STEP,
> + IMX214_EXPOSURE_DEFAULT);
>
> imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
> NULL,
>
> --
> 2.42.0
>
On Mon, Oct 23, 2023 at 11:49 PM André Apitzsch <[email protected]> wrote:
>
> Obtain rotation and orientation information from system firmware and
> register the appropriate controls.
>
> Signed-off-by: André Apitzsch <[email protected]>
Reviewed-by: Ricardo Ribalda <[email protected]>
Hi Jacopo
On Fri, 27 Oct 2023 at 12:12, Jacopo Mondi
<[email protected]> wrote:
>
> Hi Dave
>
> On Fri, Oct 27, 2023 at 11:29:11AM +0100, Dave Stevenson wrote:
> > Hi Jacopo
> >
> > On Fri, 27 Oct 2023 at 09:57, Jacopo Mondi
> > <[email protected]> wrote:
> > >
> > > Hi Andre'
> > >
> > > On Wed, Oct 25, 2023 at 11:26:00PM +0200, André Apitzsch wrote:
> > > > Hi Jacopo,
> > > >
> > > > Am Dienstag, dem 24.10.2023 um 09:52 +0200 schrieb Jacopo Mondi:
> > > > > Hi Andre'
> > > > >
> > > > > On Mon, Oct 23, 2023 at 11:47:53PM +0200, André Apitzsch wrote:
> > > > > > Set effictive and active sensor pixel sizes as shown in product
> > > > >
> > > > > s/effictive/effective
> > > > >
> > > > > > brief[1].
> > > > > >
> > > > > > [1]:
> > > > > > https://www.mouser.com/datasheet/2/897/ProductBrief_IMX214_20150428-1289331.pdf
> > > > > >
> > > > > > Signed-off-by: André Apitzsch <[email protected]>
> > > > > > ---
> > > > > > drivers/media/i2c/imx214.c | 39 ++++++++++++++++++++++++++++++++--
> > > > > > -----
> > > > > > 1 file changed, 32 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/imx214.c
> > > > > > b/drivers/media/i2c/imx214.c
> > > > > > index bef8dc36e2d0..a2d441cd8dcd 100644
> > > > > > --- a/drivers/media/i2c/imx214.c
> > > > > > +++ b/drivers/media/i2c/imx214.c
> > > > > > @@ -36,6 +36,14 @@
> > > > > > #define IMX214_EXPOSURE_STEP 1
> > > > > > #define IMX214_EXPOSURE_DEFAULT 0x0c70
> > > > > >
> > > > > > +/* IMX214 native and active pixel array size */
> > > > > > +#define IMX214_NATIVE_WIDTH 4224U
> > > > > > +#define IMX214_NATIVE_HEIGHT 3136U
> > > > > > +#define IMX214_PIXEL_ARRAY_LEFT 8U
> > > > > > +#define IMX214_PIXEL_ARRAY_TOP 8U
> > > > > > +#define IMX214_PIXEL_ARRAY_WIDTH 4208U
> > > > > > +#define IMX214_PIXEL_ARRAY_HEIGHT 3120U
> > > > > > +
> > > > >
> > > > > I do get slightly different numbers from the datasheet version I have
> > > > >
> > > > > The sensor is said to have 4224x3208 total pixels of which 4208x3120
> > > > > are active ones.
> > > > >
> > > > > The pixel array diagram shows 64 "OPB" (optically black ?) lines,
> > > > > followed by 8 dummy lines, followed by 3120 valid lines. There are 8
> > > > > dummy columns at each side of the 4208 valid ones.
> > > > >
> > > > > Now, NATIVE which represents the full pixel array size seems to be
> > > > > 4224x3208 (other parts of the datasheet only report 3200 lines
> > > > > though)
> > > > >
> > > > > BOUNDS represents the readabale array area, which I presume
> > > > > corresponds to what is named as 'effective area' by the datasheet. It
> > > > > excludes the OPB lines at the top of the image and seems to be
> > > > > represented by (0, 64, 4224, 3160).
> > > > >
> > > > > CROP_DEFAULT represents the default crop rectangle which covers the
> > > > > active pixel area, so it excludes 8 more lines of dummy pixels and 8
> > > > > dummy columns, which gives a rectangle (8, 72, 4208, 3120)
> > > > >
> > > > > Also note that the driver always reports a TGT_CROP rectangle with
> > > > > top/left points set to 0. If my understanding is correct, V4L2
> > > > > selection targets are defined from the most external target
> > > > > (TGT_NATIVE in this case), and the driver should be corrected to
> > > > > initialize the crop rectangle with a top-left corner at (8, 72).
> > > > >
> > > > > Does this make sense ?
> > > >
> > > > As far as I understood, only the effective and active sizes of three
> > > > sizes provided in the datasheet (total, effective and active) matter.
> > > > By comparing the values used in imx219.c (and imx415.c) with the ones
> > > > in the corresponding datasheets [1,2] I assume, that "effective"
> > > > matches "NATIVE_SIZE", "active" matches "CROP_DEFAULT" and "total" is
> > > > ignored.
> > >
> > > imx219 driver indeed does not consider the OPB areas in the definition
> > > of the rectangles...
> > >
>
> I know it sounds ridiculous as I've been the one adding selection
> support to imx219, but I presume we discussed it somewhen in the past:
> do you happen to remember why we left the OPB area out from the native
> sizes ? (Does OPB stand for "Optically black" ? )
Yes, OPB is Optical Black. Optically black pixels can be used for
subtracting dark noise from pixels.
OPB lines are often sent on a totally separate CSI-2 data type, so
they will generally not apply to the image buffer that is read out.
See IMX290/462 for an example of that (it uses data type 0x37)
I believe some other sensors use the OPB internally to perform
compensation on-sensor.
I've never fully followed what the full selection API is meant to be
used for beyond determining which parts of the array are cropped out
in particular modes. It's numbers that I extract from the datasheet,
stick in the driver, and the user can do something with them if they
wish.
> > > Also looking at the X/Y_ADDR_START value assigned in the register tables
> > > for full resolution mode (3280x2462) they have value of 0, indeed
> > > meaning the active area is the only readable one.
> > >
> > > Then yes, you're right, for imx219
> > > NATIVE = effective
> > > CROP_DEFAULT = BOUND = active
> > >
>
> I presume you can confirm this, right ?
Sounds about right.
> > > > The commit message of 1ed36ecd1459b653cced8929bfb37dba94b64c5d ("media:
> > > > i2c: imx219: Selection compliance fixes") seems to support me here:
> > >
> > > >
> > > > > The top/left crop coordinates of the TGT_CROP rectangle were set to
> > > > > (0, 0) instead of (8, 8) which is the offset from the larger physical
> > > > > pixel array rectangle.
> > > >
> > > > This (8, 8) is half the difference between number of effective and
> > > > active pixels of imx219[1].
> > > >
> > > > Together with the 8 dummy lines and 8 dummy columns you mentioned, I
> > > > still think my values are right. But I've just started working with
> > > > V4L2, so I might be wrong.
> > >
> > > To actually verify if the 'effective area' is readable or not, we
> > > should know what register controls the X/Y_ADDR_START value, and
> > > that's an information I don't have in my version of the datasheet.
> >
> > I happen to have an IMX214 datasheet.
> > X_ADDR_START is 0x0344/5 (set in multiples of 2)
> > Y_ADDR_START is 0x0346/7 (set in multiples of 4)
> > X_ADDR_END is 0x0348/9 (set in multiples of 2)
> > Y_ADDR_END is 0x034a/b (set in multiples of 4)
> > X_OUTPUT_SIZE 0x034c/d
> > Y_OUTPUT_SIZE 0x034e/f
> >
> > X direction are 13bit values, Y direction are 12 bit.
> > [12:8] or [11:8] in the low bits of the first register, [7:0] in the
> > second register.
>
> AH thanks! Unfortunately the largest imx214 mode is cropped from full
> pixel array it seems, so not that helpful :(
Yup, I noticed that too. Make the selection reflect the register set -
it's all you can do.
I don't know why it got cropped down so much to make a 16:9 full res
mode. Possibly a limit of 4096 on an ISP or receiver somewhere, or
potentially just the use case.
I guess there's nothing stopping an extra mode being added which uses
the full sensor array.
Dave
> >
> > Dave
> >
> > > It's however plausible that it behaves the same as imx219, as the
> > > driver's register sequences seems to program the crop sizes in
> > > register 0x034c and 0x034e and there's not programmed top-left corner
> > > there.
> > >
> > > Ok then, let's be consistent and do the same as imx219 as you're doing
> > > here.
> > >
> > > Reviewed-by: Jacopo Mondi <[email protected]>
> > >
> > > >
> > > > Could you share the imx214 datasheet with me?
> > > >
> > > > Best regards,
> > > > André
> > > >
> > > > [1] https://www.arducam.com/downloads/modules/RaspberryPi_camera/IMX219DS.PDF
> > > > [2] https://www.sony-semicon.com/files/62/pdf/p-12_IMX415-AAQR_AAMR_Flyer.pdf
> > > > >
> > > > > Thanks
> > > > > j
> > > > >
> > > > >
> > > > > > static const char * const imx214_supply_name[] = {
> > > > > > "vdda",
> > > > > > "vddd",
> > > > > > @@ -634,14 +642,31 @@ static int imx214_get_selection(struct
> > > > > > v4l2_subdev *sd,
> > > > > > {
> > > > > > struct imx214 *imx214 = to_imx214(sd);
> > > > > >
> > > > > > - if (sel->target != V4L2_SEL_TGT_CROP)
> > > > > > - return -EINVAL;
> > > > > > + switch (sel->target) {
> > > > > > + case V4L2_SEL_TGT_CROP:
> > > > > > + mutex_lock(&imx214->mutex);
> > > > > > + sel->r = *__imx214_get_pad_crop(imx214, sd_state,
> > > > > > sel->pad,
> > > > > > + sel->which);
> > > > > > + mutex_unlock(&imx214->mutex);
> > > > > > + return 0;
> > > > > >
> > > > > > - mutex_lock(&imx214->mutex);
> > > > > > - sel->r = *__imx214_get_pad_crop(imx214, sd_state, sel-
> > > > > > >pad,
> > > > > > - sel->which);
> > > > > > - mutex_unlock(&imx214->mutex);
> > > > > > - return 0;
> > > > > > + case V4L2_SEL_TGT_NATIVE_SIZE:
> > > > > > + sel->r.top = 0;
> > > > > > + sel->r.left = 0;
> > > > > > + sel->r.width = IMX214_NATIVE_WIDTH;
> > > > > > + sel->r.height = IMX214_NATIVE_HEIGHT;
> > > > > > + return 0;
> > > > > > +
> > > > > > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > > > > > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > > > > > + sel->r.top = IMX214_PIXEL_ARRAY_TOP;
> > > > > > + sel->r.left = IMX214_PIXEL_ARRAY_LEFT;
> > > > > > + sel->r.width = IMX214_PIXEL_ARRAY_WIDTH;
> > > > > > + sel->r.height = IMX214_PIXEL_ARRAY_HEIGHT;
> > > > > > + return 0;
> > > > > > + }
> > > > > > +
> > > > > > + return -EINVAL;
> > > > > > }
> > > > > >
> > > > > > static int imx214_entity_init_cfg(struct v4l2_subdev *subdev,
> > > > > >
> > > > > > --
> > > > > > 2.42.0
> > > > > >
> > > >