[PATCH 1/3] media: add pixel_size control

[PATCH 1/3] media: add pixel_size control

[Ricardo Ribalda Delgado]

This control returns the pixel size in nanometres. The struct provides
the width and the height in separated fields to take into consideration
asymmetric pixels and/or hardware binning.
This control is required for automatic calibration of the sensor.

Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 11 +++++++++++
 include/media/v4l2-ctrls.h           |  2 ++
 include/uapi/linux/v4l2-controls.h   |  3 +++
 include/uapi/linux/videodev2.h       | 11 +++++++++++
 4 files changed, 27 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index cd1ae016706f..a3a0086c96ff 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -978,6 +978,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_AUTO_FOCUS_RANGE:		return "Auto Focus, Range";
 	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
 	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
+	case V4L2_CID_PIXEL_SIZE:		return "Pixel Size";
 
 	/* FM Radio Modulator controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1357,6 +1358,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER:
 		*type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER;
 		break;
+	case V4L2_CID_PIXEL_SIZE:
+		*type = V4L2_CTRL_TYPE_PIXEL_SIZE;
+		break;
 	default:
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		break;
@@ -1423,6 +1427,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_RDS_RX_TRAFFIC_ANNOUNCEMENT:
 	case V4L2_CID_RDS_RX_TRAFFIC_PROGRAM:
 	case V4L2_CID_RDS_RX_MUSIC_SPEECH:
+	case V4L2_CID_PIXEL_SIZE:
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
 		break;
 	case V4L2_CID_RF_TUNER_PLL_LOCK:
@@ -1705,6 +1710,9 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 	case V4L2_CTRL_TYPE_FWHT_PARAMS:
 		break;
 
+	case V4L2_CTRL_TYPE_PIXEL_SIZE:
+		break;
+
 	case V4L2_CTRL_TYPE_H264_SPS:
 	case V4L2_CTRL_TYPE_H264_PPS:
 	case V4L2_CTRL_TYPE_H264_SCALING_MATRIX:
@@ -2403,6 +2411,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
 		elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header);
 		break;
+	case V4L2_CTRL_TYPE_PIXEL_SIZE:
+		elem_size = sizeof(struct v4l2_pixel_size);
+		break;
 	default:
 		if (type < V4L2_CTRL_COMPOUND_TYPES)
 			elem_size = sizeof(s32);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 570ff4b0205a..63de780398b8 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -50,6 +50,7 @@ struct poll_table_struct;
  * @p_h264_slice_params:	Pointer to a struct v4l2_ctrl_h264_slice_params.
  * @p_h264_decode_params:	Pointer to a struct v4l2_ctrl_h264_decode_params.
  * @p_vp8_frame_header:		Pointer to a VP8 frame header structure.
+ * @p_pixel_size:		Pointer to a pixel_size value.
  * @p:				Pointer to a compound value.
  */
 union v4l2_ctrl_ptr {
@@ -68,6 +69,7 @@ union v4l2_ctrl_ptr {
 	struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
 	struct v4l2_ctrl_h264_decode_params *p_h264_decode_params;
 	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
+	struct v4l2_pixel_size *p_pixel_size;
 	void *p;
 };
 
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index a2669b79b294..13f0410df4c6 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -912,6 +912,9 @@ enum v4l2_auto_focus_range {
 #define V4L2_CID_PAN_SPEED			(V4L2_CID_CAMERA_CLASS_BASE+32)
 #define V4L2_CID_TILT_SPEED			(V4L2_CID_CAMERA_CLASS_BASE+33)
 
+#define V4L2_CID_PIXEL_SIZE			(V4L2_CID_CAMERA_CLASS_BASE+34)
+
+
 /* FM Modulator class control IDs */
 
 #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 2427bc4d8eba..21f4846dca0b 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -422,6 +422,11 @@ struct v4l2_fract {
 	__u32   denominator;
 };
 
+struct v4l2_pixel_size {
+	__u32   width;
+	__u32   height;
+};
+
 /**
   * struct v4l2_capability - Describes V4L2 device caps returned by VIDIOC_QUERYCAP
   *
@@ -1718,6 +1723,12 @@ enum v4l2_ctrl_type {
 	V4L2_CTRL_TYPE_U8	     = 0x0100,
 	V4L2_CTRL_TYPE_U16	     = 0x0101,
 	V4L2_CTRL_TYPE_U32	     = 0x0102,
+	/*
+	 * V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS = 0x0103,
+	 * V4L2_CTRL_TYPE_MPEG2_QUANTIZATION = 0x0104,
+	 * V4L2_CTRL_TYPE_FWHT_PARAMS = 0x0105,
+	 */
+	V4L2_CTRL_TYPE_PIXEL_SIZE    = 0x0106,
 };
 
 /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
-- 
2.23.0.rc1

[PATCH 2/3] Documentation: Describe V4L2_CID_PIXEL_SIZE

[Ricardo Ribalda Delgado]

New control to pass to userspace the width/height of a pixel. Which is
needed for 3D calibration and lens selection.

Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
---
 Documentation/media/uapi/v4l/ext-ctrls-camera.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
index 51c1d5c9eb00..670c57a6f622 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
@@ -510,6 +510,12 @@ enum v4l2_scene_mode -
     value down. A value of zero stops the motion if one is in progress
     and has no effect otherwise.
 
+``V4L2_CID_PIXEL_SIZE (struct)``
+    This control returns the pixel size in nanometres. The struct provides
+    the width and the height in separated fields to take into consideration
+    asymmetric pixels and/or hardware binning.
+    This control is required for automatic calibration of the sensor.
+
 .. [#f1]
    This control may be changed to a menu control in the future, if more
    options are required.
-- 
2.23.0.rc1

[PATCH 3/3] media: imx214: Add new control with V4L2_CID_PIXEL_SIZE

[Ricardo Ribalda Delgado]

According to the product brief, the unit cell size is 1120 nanometers^2.

https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf

Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
---
 drivers/media/i2c/imx214.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 159a3a604f0e..b2f6bd2d8d7d 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -47,6 +47,7 @@ struct imx214 {
 	struct v4l2_ctrl *pixel_rate;
 	struct v4l2_ctrl *link_freq;
 	struct v4l2_ctrl *exposure;
+	struct v4l2_ctrl *pixel_size;
 
 	struct regulator_bulk_data	supplies[IMX214_NUM_SUPPLIES];
 
@@ -941,6 +942,26 @@ static int __maybe_unused imx214_resume(struct device *dev)
 	return ret;
 }
 
+static void pixel_size_init(const struct v4l2_ctrl *ctrl, u32 idx,
+		     union v4l2_ctrl_ptr ptr)
+{
+	ptr.p_pixel_size->width = 1120;
+	ptr.p_pixel_size->height = 1120;
+}
+
+static const struct v4l2_ctrl_type_ops pixel_size_ops = {
+	.init = pixel_size_init,
+};
+
+static struct v4l2_ctrl *new_pixel_size_ctrl(struct v4l2_ctrl_handler *handler)
+{
+	static struct v4l2_ctrl_config ctrl = {
+		.id = V4L2_CID_PIXEL_SIZE,
+		.type_ops = &pixel_size_ops,
+	};
+
+	return v4l2_ctrl_new_custom(handler, &ctrl, NULL);
+}
 static int imx214_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -1029,6 +1050,8 @@ static int imx214_probe(struct i2c_client *client)
 					     V4L2_CID_EXPOSURE,
 					     0, 3184, 1, 0x0c70);
 
+	imx214->pixel_size = new_pixel_size_ctrl(&imx214->ctrls);
+
 	ret = imx214->ctrls.error;
 	if (ret) {
 		dev_err(&client->dev, "%s control init failed (%d)\n",
-- 
2.23.0.rc1

Re: [PATCH 2/3] Documentation: Describe V4L2_CID_PIXEL_SIZE

[Philipp Zabel]

On Mon, 2019-08-19 at 14:17 +0200, Ricardo Ribalda Delgado wrote:
> New control to pass to userspace the width/height of a pixel. Which is
> needed for 3D calibration and lens selection.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> ---
>  Documentation/media/uapi/v4l/ext-ctrls-camera.rst | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> index 51c1d5c9eb00..670c57a6f622 100644
> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> @@ -510,6 +510,12 @@ enum v4l2_scene_mode -
>      value down. A value of zero stops the motion if one is in progress
>      and has no effect otherwise.
>  
> +``V4L2_CID_PIXEL_SIZE (struct)``
> +    This control returns the pixel size in nanometres. The struct provides
> +    the width and the height in separated fields to take into consideration
> +    asymmetric pixels and/or hardware binning.
> +    This control is required for automatic calibration of the sensor.
> +
>  .. [#f1]
>     This control may be changed to a menu control in the future, if more
>     options are required.

I suppose this is a common term, but should it be mentioned that pixel
size is the same as unit cell size, and not necessarily the size of the
light sensitive area? Just in case the effective fill-factor is < 100%.

regards
Philipp

Re: [PATCH 2/3] Documentation: Describe V4L2_CID_PIXEL_SIZE

[Ricardo Ribalda Delgado]

Completely agree. Maybe we should call the control unit cell size?, in
case in the future we need a control for the light sensitive area.

Best regards

On Mon, Aug 19, 2019 at 3:42 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Mon, 2019-08-19 at 14:17 +0200, Ricardo Ribalda Delgado wrote:
> > New control to pass to userspace the width/height of a pixel. Which is
> > needed for 3D calibration and lens selection.
> >
> > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> > ---
> >  Documentation/media/uapi/v4l/ext-ctrls-camera.rst | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > index 51c1d5c9eb00..670c57a6f622 100644
> > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > @@ -510,6 +510,12 @@ enum v4l2_scene_mode -
> >      value down. A value of zero stops the motion if one is in progress
> >      and has no effect otherwise.
> >
> > +``V4L2_CID_PIXEL_SIZE (struct)``
> > +    This control returns the pixel size in nanometres. The struct provides
> > +    the width and the height in separated fields to take into consideration
> > +    asymmetric pixels and/or hardware binning.
> > +    This control is required for automatic calibration of the sensor.
> > +
> >  .. [#f1]
> >     This control may be changed to a menu control in the future, if more
> >     options are required.
>
> I suppose this is a common term, but should it be mentioned that pixel
> size is the same as unit cell size, and not necessarily the size of the
> light sensitive area? Just in case the effective fill-factor is < 100%.
>
> regards
> Philipp

Re: [PATCH 1/3] media: add pixel_size control

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 5711 bytes --]

Hi Ricardo,

On Mon, Aug 19, 2019 at 02:17:18PM +0200, Ricardo Ribalda Delgado wrote:
> This control returns the pixel size in nanometres. The struct provides
> the width and the height in separated fields to take into consideration
> asymmetric pixels and/or hardware binning.
> This control is required for automatic calibration of the sensor.
>
> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c | 11 +++++++++++
>  include/media/v4l2-ctrls.h           |  2 ++
>  include/uapi/linux/v4l2-controls.h   |  3 +++
>  include/uapi/linux/videodev2.h       | 11 +++++++++++
>  4 files changed, 27 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index cd1ae016706f..a3a0086c96ff 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -978,6 +978,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_AUTO_FOCUS_RANGE:		return "Auto Focus, Range";
>  	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
>  	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
> +	case V4L2_CID_PIXEL_SIZE:		return "Pixel Size";

Is this a camera class control or an image source one ?
Also, isn't pixel size a bit too generic? I would somehow specify this is
the physical pixel size

>
>  	/* FM Radio Modulator controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1357,6 +1358,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER:
>  		*type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER;
>  		break;
> +	case V4L2_CID_PIXEL_SIZE:
> +		*type = V4L2_CTRL_TYPE_PIXEL_SIZE;

Isn't this a read-only control?

> +		break;
>  	default:
>  		*type = V4L2_CTRL_TYPE_INTEGER;
>  		break;
> @@ -1423,6 +1427,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_RDS_RX_TRAFFIC_ANNOUNCEMENT:
>  	case V4L2_CID_RDS_RX_TRAFFIC_PROGRAM:
>  	case V4L2_CID_RDS_RX_MUSIC_SPEECH:
> +	case V4L2_CID_PIXEL_SIZE:
>  		*flags |= V4L2_CTRL_FLAG_READ_ONLY;

Ah yes, you set flags here... I don't get why we have two switches
here.. Anyway, I would set both type and flags in a single case,
otherwise one should jump back and forth...

>  		break;
>  	case V4L2_CID_RF_TUNER_PLL_LOCK:
> @@ -1705,6 +1710,9 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  	case V4L2_CTRL_TYPE_FWHT_PARAMS:
>  		break;
>
> +	case V4L2_CTRL_TYPE_PIXEL_SIZE:
> +		break;
> +
>  	case V4L2_CTRL_TYPE_H264_SPS:
>  	case V4L2_CTRL_TYPE_H264_PPS:
>  	case V4L2_CTRL_TYPE_H264_SCALING_MATRIX:
> @@ -2403,6 +2411,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
>  		elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header);
>  		break;
> +	case V4L2_CTRL_TYPE_PIXEL_SIZE:
> +		elem_size = sizeof(struct v4l2_pixel_size);
> +		break;
>  	default:
>  		if (type < V4L2_CTRL_COMPOUND_TYPES)
>  			elem_size = sizeof(s32);
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 570ff4b0205a..63de780398b8 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -50,6 +50,7 @@ struct poll_table_struct;
>   * @p_h264_slice_params:	Pointer to a struct v4l2_ctrl_h264_slice_params.
>   * @p_h264_decode_params:	Pointer to a struct v4l2_ctrl_h264_decode_params.
>   * @p_vp8_frame_header:		Pointer to a VP8 frame header structure.
> + * @p_pixel_size:		Pointer to a pixel_size value.
>   * @p:				Pointer to a compound value.
>   */
>  union v4l2_ctrl_ptr {
> @@ -68,6 +69,7 @@ union v4l2_ctrl_ptr {
>  	struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
>  	struct v4l2_ctrl_h264_decode_params *p_h264_decode_params;
>  	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
> +	struct v4l2_pixel_size *p_pixel_size;
>  	void *p;
>  };
>
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index a2669b79b294..13f0410df4c6 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -912,6 +912,9 @@ enum v4l2_auto_focus_range {
>  #define V4L2_CID_PAN_SPEED			(V4L2_CID_CAMERA_CLASS_BASE+32)
>  #define V4L2_CID_TILT_SPEED			(V4L2_CID_CAMERA_CLASS_BASE+33)
>
> +#define V4L2_CID_PIXEL_SIZE			(V4L2_CID_CAMERA_CLASS_BASE+34)
> +
> +

Double empty line

>  /* FM Modulator class control IDs */
>
>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 2427bc4d8eba..21f4846dca0b 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -422,6 +422,11 @@ struct v4l2_fract {
>  	__u32   denominator;
>  };
>
> +struct v4l2_pixel_size {

I wonder if instead of defining a 'pixel size' this shouldn't be a
more generic v4l2_size or something similar. Or even a v4l2_rect with
left=top=0.

> +	__u32   width;
> +	__u32   height;
> +};
> +
>  /**
>    * struct v4l2_capability - Describes V4L2 device caps returned by VIDIOC_QUERYCAP
>    *
> @@ -1718,6 +1723,12 @@ enum v4l2_ctrl_type {
>  	V4L2_CTRL_TYPE_U8	     = 0x0100,
>  	V4L2_CTRL_TYPE_U16	     = 0x0101,
>  	V4L2_CTRL_TYPE_U32	     = 0x0102,
> +	/*
> +	 * V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS = 0x0103,
> +	 * V4L2_CTRL_TYPE_MPEG2_QUANTIZATION = 0x0104,
> +	 * V4L2_CTRL_TYPE_FWHT_PARAMS = 0x0105,
> +	 */

leftovers ?

> +	V4L2_CTRL_TYPE_PIXEL_SIZE    = 0x0106,

I don't see other compound controls with a custom payload adding their
types here. What am I missing?

Thanks
  j

>  };
>
>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
> --
> 2.23.0.rc1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] media: imx214: Add new control with V4L2_CID_PIXEL_SIZE

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 2483 bytes --]

Hi Ricardo,

On Mon, Aug 19, 2019 at 02:17:20PM +0200, Ricardo Ribalda Delgado wrote:
> According to the product brief, the unit cell size is 1120 nanometers^2.

Should this information come from DT ?

I'm asking as I've a series in review that adds an helper that
collectes dt properties and register controls for them. It currently
only supports the newly proposed camera location control, but there
might be others like the rotation, for which we already have a DT
property.

https://patchwork.kernel.org/project/linux-media/list/?series=160901

This new one is indeed an HW property of the sensor, I wonder if
having it in the firmware interface would make any sense or not...

Thanks
  j

>
> https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf
>
> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> ---
>  drivers/media/i2c/imx214.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 159a3a604f0e..b2f6bd2d8d7d 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -47,6 +47,7 @@ struct imx214 {
>  	struct v4l2_ctrl *pixel_rate;
>  	struct v4l2_ctrl *link_freq;
>  	struct v4l2_ctrl *exposure;
> +	struct v4l2_ctrl *pixel_size;
>
>  	struct regulator_bulk_data	supplies[IMX214_NUM_SUPPLIES];
>
> @@ -941,6 +942,26 @@ static int __maybe_unused imx214_resume(struct device *dev)
>  	return ret;
>  }
>
> +static void pixel_size_init(const struct v4l2_ctrl *ctrl, u32 idx,
> +		     union v4l2_ctrl_ptr ptr)
> +{
> +	ptr.p_pixel_size->width = 1120;
> +	ptr.p_pixel_size->height = 1120;
> +}
> +
> +static const struct v4l2_ctrl_type_ops pixel_size_ops = {
> +	.init = pixel_size_init,
> +};
> +
> +static struct v4l2_ctrl *new_pixel_size_ctrl(struct v4l2_ctrl_handler *handler)
> +{
> +	static struct v4l2_ctrl_config ctrl = {
> +		.id = V4L2_CID_PIXEL_SIZE,
> +		.type_ops = &pixel_size_ops,
> +	};
> +
> +	return v4l2_ctrl_new_custom(handler, &ctrl, NULL);
> +}
>  static int imx214_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
> @@ -1029,6 +1050,8 @@ static int imx214_probe(struct i2c_client *client)
>  					     V4L2_CID_EXPOSURE,
>  					     0, 3184, 1, 0x0c70);
>
> +	imx214->pixel_size = new_pixel_size_ctrl(&imx214->ctrls);
> +
>  	ret = imx214->ctrls.error;
>  	if (ret) {
>  		dev_err(&client->dev, "%s control init failed (%d)\n",
> --
> 2.23.0.rc1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] media: imx214: Add new control with V4L2_CID_PIXEL_SIZE

[Ricardo Ribalda Delgado]

Hi Jacopo


On Wed, Aug 21, 2019 at 6:14 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Ricardo,
>
> On Mon, Aug 19, 2019 at 02:17:20PM +0200, Ricardo Ribalda Delgado wrote:
> > According to the product brief, the unit cell size is 1120 nanometers^2.
>
> Should this information come from DT ?

I do not think so. You cannot change this value and it needs to be
defined also in sensors/cameras that might not have a DT, like a usb
webcam.

It would be like adding to the DT the min/max exposure time...

But of course we can discuss it ;)

Best regards

>
> I'm asking as I've a series in review that adds an helper that
> collectes dt properties and register controls for them. It currently
> only supports the newly proposed camera location control, but there
> might be others like the rotation, for which we already have a DT
> property.
>
> https://patchwork.kernel.org/project/linux-media/list/?series=160901
>
> This new one is indeed an HW property of the sensor, I wonder if
> having it in the firmware interface would make any sense or not...
>
> Thanks
>   j
>
> >
> > https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf
> >
> > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> > ---
> >  drivers/media/i2c/imx214.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > index 159a3a604f0e..b2f6bd2d8d7d 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -47,6 +47,7 @@ struct imx214 {
> >       struct v4l2_ctrl *pixel_rate;
> >       struct v4l2_ctrl *link_freq;
> >       struct v4l2_ctrl *exposure;
> > +     struct v4l2_ctrl *pixel_size;
> >
> >       struct regulator_bulk_data      supplies[IMX214_NUM_SUPPLIES];
> >
> > @@ -941,6 +942,26 @@ static int __maybe_unused imx214_resume(struct device *dev)
> >       return ret;
> >  }
> >
> > +static void pixel_size_init(const struct v4l2_ctrl *ctrl, u32 idx,
> > +                  union v4l2_ctrl_ptr ptr)
> > +{
> > +     ptr.p_pixel_size->width = 1120;
> > +     ptr.p_pixel_size->height = 1120;
> > +}
> > +
> > +static const struct v4l2_ctrl_type_ops pixel_size_ops = {
> > +     .init = pixel_size_init,
> > +};
> > +
> > +static struct v4l2_ctrl *new_pixel_size_ctrl(struct v4l2_ctrl_handler *handler)
> > +{
> > +     static struct v4l2_ctrl_config ctrl = {
> > +             .id = V4L2_CID_PIXEL_SIZE,
> > +             .type_ops = &pixel_size_ops,
> > +     };
> > +
> > +     return v4l2_ctrl_new_custom(handler, &ctrl, NULL);
> > +}
> >  static int imx214_probe(struct i2c_client *client)
> >  {
> >       struct device *dev = &client->dev;
> > @@ -1029,6 +1050,8 @@ static int imx214_probe(struct i2c_client *client)
> >                                            V4L2_CID_EXPOSURE,
> >                                            0, 3184, 1, 0x0c70);
> >
> > +     imx214->pixel_size = new_pixel_size_ctrl(&imx214->ctrls);
> > +
> >       ret = imx214->ctrls.error;
> >       if (ret) {
> >               dev_err(&client->dev, "%s control init failed (%d)\n",
> > --
> > 2.23.0.rc1
> >

Re: [PATCH 1/3] media: add pixel_size control

[Ricardo Ribalda Delgado]

[-- Attachment #1: Type: text/plain, Size: 13739 bytes --]

Hi Jacopo

[sorry for the multiple send, working on a train and fighting with the
mail client and the 4g connection :S]

On Wed, 21 Aug 2019, 18:10 Jacopo Mondi, <jacopo@jmondi.org> wrote:
>
> Hi Ricardo,
>
> On Mon, Aug 19, 2019 at 02:17:18PM +0200, Ricardo Ribalda Delgado wrote:
> > This control returns the pixel size in nanometres. The struct provides
> > the width and the height in separated fields to take into consideration
> > asymmetric pixels and/or hardware binning.
> > This control is required for automatic calibration of the sensor.
> >
> > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-ctrls.c | 11 +++++++++++
> >  include/media/v4l2-ctrls.h           |  2 ++
> >  include/uapi/linux/v4l2-controls.h   |  3 +++
> >  include/uapi/linux/videodev2.h       | 11 +++++++++++
> >  4 files changed, 27 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index cd1ae016706f..a3a0086c96ff 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -978,6 +978,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >       case V4L2_CID_AUTO_FOCUS_RANGE:         return "Auto Focus, Range";
> >       case V4L2_CID_PAN_SPEED:                return "Pan, Speed";
> >       case V4L2_CID_TILT_SPEED:               return "Tilt, Speed";
> > +     case V4L2_CID_PIXEL_SIZE:               return "Pixel Size";
>
> Is this a camera class control or an image source one ?
> Also, isn't pixel size a bit too generic? I would somehow specify this is
> the physical pixel size

Please review v2. It has changed from pixel_size to unit size, which
is more clear.

>
>
> >
> >       /* FM Radio Modulator controls */
> >       /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> > @@ -1357,6 +1358,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >       case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER:
> >               *type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER;
> >               break;
> > +     case V4L2_CID_PIXEL_SIZE:
> > +             *type = V4L2_CTRL_TYPE_PIXEL_SIZE;
>
> Isn't this a read-only control?
>
> > +             break;
> >       default:
> >               *type = V4L2_CTRL_TYPE_INTEGER;
> >               break;
> > @@ -1423,6 +1427,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >       case V4L2_CID_RDS_RX_TRAFFIC_ANNOUNCEMENT:
> >       case V4L2_CID_RDS_RX_TRAFFIC_PROGRAM:
> >       case V4L2_CID_RDS_RX_MUSIC_SPEECH:
> > +     case V4L2_CID_PIXEL_SIZE:
> >               *flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
> Ah yes, you set flags here... I don't get why we have two switches
> here.. Anyway, I would set both type and flags in a single case,
> otherwise one should jump back and forth...

Sure. will do on v3

>
> >               break;
> >       case V4L2_CID_RF_TUNER_PLL_LOCK:
> > @@ -1705,6 +1710,9 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >       case V4L2_CTRL_TYPE_FWHT_PARAMS:
> >               break;
> >
> > +     case V4L2_CTRL_TYPE_PIXEL_SIZE:
> > +             break;
> > +
> >       case V4L2_CTRL_TYPE_H264_SPS:
> >       case V4L2_CTRL_TYPE_H264_PPS:
> >       case V4L2_CTRL_TYPE_H264_SCALING_MATRIX:
> > @@ -2403,6 +2411,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> >       case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
> >               elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header);
> >               break;
> > +     case V4L2_CTRL_TYPE_PIXEL_SIZE:
> > +             elem_size = sizeof(struct v4l2_pixel_size);
> > +             break;
> >       default:
> >               if (type < V4L2_CTRL_COMPOUND_TYPES)
> >                       elem_size = sizeof(s32);
> > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> > index 570ff4b0205a..63de780398b8 100644
> > --- a/include/media/v4l2-ctrls.h
> > +++ b/include/media/v4l2-ctrls.h
> > @@ -50,6 +50,7 @@ struct poll_table_struct;
> >   * @p_h264_slice_params:     Pointer to a struct v4l2_ctrl_h264_slice_params.
> >   * @p_h264_decode_params:    Pointer to a struct v4l2_ctrl_h264_decode_params.
> >   * @p_vp8_frame_header:              Pointer to a VP8 frame header structure.
> > + * @p_pixel_size:            Pointer to a pixel_size value.
> >   * @p:                               Pointer to a compound value.
> >   */
> >  union v4l2_ctrl_ptr {
> > @@ -68,6 +69,7 @@ union v4l2_ctrl_ptr {
> >       struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
> >       struct v4l2_ctrl_h264_decode_params *p_h264_decode_params;
> >       struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
> > +     struct v4l2_pixel_size *p_pixel_size;
> >       void *p;
> >  };
> >
> > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > index a2669b79b294..13f0410df4c6 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -912,6 +912,9 @@ enum v4l2_auto_focus_range {
> >  #define V4L2_CID_PAN_SPEED                   (V4L2_CID_CAMERA_CLASS_BASE+32)
> >  #define V4L2_CID_TILT_SPEED                  (V4L2_CID_CAMERA_CLASS_BASE+33)
> >
> > +#define V4L2_CID_PIXEL_SIZE                  (V4L2_CID_CAMERA_CLASS_BASE+34)
> > +
> > +
>
> Double empty line

Thanks , will fix in v3
>
> >  /* FM Modulator class control IDs */
> >
> >  #define V4L2_CID_FM_TX_CLASS_BASE            (V4L2_CTRL_CLASS_FM_TX | 0x900)
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 2427bc4d8eba..21f4846dca0b 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -422,6 +422,11 @@ struct v4l2_fract {
> >       __u32   denominator;
> >  };
> >
> > +struct v4l2_pixel_size {
>
> I wonder if instead of defining a 'pixel size' this shouldn't be a
> more generic v4l2_size or something similar. Or even a v4l2_rect with
> left=top=0.

Fixed on v2

>
> > +     __u32   width;
> > +     __u32   height;
> > +};
> > +
> >  /**
> >    * struct v4l2_capability - Describes V4L2 device caps returned by VIDIOC_QUERYCAP
> >    *
> > @@ -1718,6 +1723,12 @@ enum v4l2_ctrl_type {
> >       V4L2_CTRL_TYPE_U8            = 0x0100,
> >       V4L2_CTRL_TYPE_U16           = 0x0101,
> >       V4L2_CTRL_TYPE_U32           = 0x0102,
> > +     /*
> > +      * V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS = 0x0103,
> > +      * V4L2_CTRL_TYPE_MPEG2_QUANTIZATION = 0x0104,
> > +      * V4L2_CTRL_TYPE_FWHT_PARAMS = 0x0105,
> > +      */
>
> leftovers ?
They are defined on other .h. I thought it was nice to know why I did
not use 0x0103

include/media/mpeg2-ctrls.h:#define V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS 0x0103
include/media/mpeg2-ctrls.h:#define V4L2_CTRL_TYPE_MPEG2_QUANTIZATION 0x0104
include/media/fwht-ctrls.h:#define V4L2_CTRL_TYPE_FWHT_PARAMS 0x0105



>
> > +     V4L2_CTRL_TYPE_PIXEL_SIZE    = 0x0106,
>
> I don't see other compound controls with a custom payload adding their
> types here. What am I missing?

@Hans could you clarify here what is the best practice?

Thanks!!


On Wed, Aug 21, 2019 at 6:10 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Ricardo,
>
> On Mon, Aug 19, 2019 at 02:17:18PM +0200, Ricardo Ribalda Delgado wrote:
> > This control returns the pixel size in nanometres. The struct provides
> > the width and the height in separated fields to take into consideration
> > asymmetric pixels and/or hardware binning.
> > This control is required for automatic calibration of the sensor.
> >
> > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-ctrls.c | 11 +++++++++++
> >  include/media/v4l2-ctrls.h           |  2 ++
> >  include/uapi/linux/v4l2-controls.h   |  3 +++
> >  include/uapi/linux/videodev2.h       | 11 +++++++++++
> >  4 files changed, 27 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index cd1ae016706f..a3a0086c96ff 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -978,6 +978,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >       case V4L2_CID_AUTO_FOCUS_RANGE:         return "Auto Focus, Range";
> >       case V4L2_CID_PAN_SPEED:                return "Pan, Speed";
> >       case V4L2_CID_TILT_SPEED:               return "Tilt, Speed";
> > +     case V4L2_CID_PIXEL_SIZE:               return "Pixel Size";
>
> Is this a camera class control or an image source one ?
> Also, isn't pixel size a bit too generic? I would somehow specify this is
> the physical pixel size
>
> >
> >       /* FM Radio Modulator controls */
> >       /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> > @@ -1357,6 +1358,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >       case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER:
> >               *type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER;
> >               break;
> > +     case V4L2_CID_PIXEL_SIZE:
> > +             *type = V4L2_CTRL_TYPE_PIXEL_SIZE;
>
> Isn't this a read-only control?
>
> > +             break;
> >       default:
> >               *type = V4L2_CTRL_TYPE_INTEGER;
> >               break;
> > @@ -1423,6 +1427,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >       case V4L2_CID_RDS_RX_TRAFFIC_ANNOUNCEMENT:
> >       case V4L2_CID_RDS_RX_TRAFFIC_PROGRAM:
> >       case V4L2_CID_RDS_RX_MUSIC_SPEECH:
> > +     case V4L2_CID_PIXEL_SIZE:
> >               *flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
> Ah yes, you set flags here... I don't get why we have two switches
> here.. Anyway, I would set both type and flags in a single case,
> otherwise one should jump back and forth...
>
> >               break;
> >       case V4L2_CID_RF_TUNER_PLL_LOCK:
> > @@ -1705,6 +1710,9 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >       case V4L2_CTRL_TYPE_FWHT_PARAMS:
> >               break;
> >
> > +     case V4L2_CTRL_TYPE_PIXEL_SIZE:
> > +             break;
> > +
> >       case V4L2_CTRL_TYPE_H264_SPS:
> >       case V4L2_CTRL_TYPE_H264_PPS:
> >       case V4L2_CTRL_TYPE_H264_SCALING_MATRIX:
> > @@ -2403,6 +2411,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> >       case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
> >               elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header);
> >               break;
> > +     case V4L2_CTRL_TYPE_PIXEL_SIZE:
> > +             elem_size = sizeof(struct v4l2_pixel_size);
> > +             break;
> >       default:
> >               if (type < V4L2_CTRL_COMPOUND_TYPES)
> >                       elem_size = sizeof(s32);
> > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> > index 570ff4b0205a..63de780398b8 100644
> > --- a/include/media/v4l2-ctrls.h
> > +++ b/include/media/v4l2-ctrls.h
> > @@ -50,6 +50,7 @@ struct poll_table_struct;
> >   * @p_h264_slice_params:     Pointer to a struct v4l2_ctrl_h264_slice_params.
> >   * @p_h264_decode_params:    Pointer to a struct v4l2_ctrl_h264_decode_params.
> >   * @p_vp8_frame_header:              Pointer to a VP8 frame header structure.
> > + * @p_pixel_size:            Pointer to a pixel_size value.
> >   * @p:                               Pointer to a compound value.
> >   */
> >  union v4l2_ctrl_ptr {
> > @@ -68,6 +69,7 @@ union v4l2_ctrl_ptr {
> >       struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
> >       struct v4l2_ctrl_h264_decode_params *p_h264_decode_params;
> >       struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
> > +     struct v4l2_pixel_size *p_pixel_size;
> >       void *p;
> >  };
> >
> > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > index a2669b79b294..13f0410df4c6 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -912,6 +912,9 @@ enum v4l2_auto_focus_range {
> >  #define V4L2_CID_PAN_SPEED                   (V4L2_CID_CAMERA_CLASS_BASE+32)
> >  #define V4L2_CID_TILT_SPEED                  (V4L2_CID_CAMERA_CLASS_BASE+33)
> >
> > +#define V4L2_CID_PIXEL_SIZE                  (V4L2_CID_CAMERA_CLASS_BASE+34)
> > +
> > +
>
> Double empty line
>
> >  /* FM Modulator class control IDs */
> >
> >  #define V4L2_CID_FM_TX_CLASS_BASE            (V4L2_CTRL_CLASS_FM_TX | 0x900)
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 2427bc4d8eba..21f4846dca0b 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -422,6 +422,11 @@ struct v4l2_fract {
> >       __u32   denominator;
> >  };
> >
> > +struct v4l2_pixel_size {
>
> I wonder if instead of defining a 'pixel size' this shouldn't be a
> more generic v4l2_size or something similar. Or even a v4l2_rect with
> left=top=0.
>
> > +     __u32   width;
> > +     __u32   height;
> > +};
> > +
> >  /**
> >    * struct v4l2_capability - Describes V4L2 device caps returned by VIDIOC_QUERYCAP
> >    *
> > @@ -1718,6 +1723,12 @@ enum v4l2_ctrl_type {
> >       V4L2_CTRL_TYPE_U8            = 0x0100,
> >       V4L2_CTRL_TYPE_U16           = 0x0101,
> >       V4L2_CTRL_TYPE_U32           = 0x0102,
> > +     /*
> > +      * V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS = 0x0103,
> > +      * V4L2_CTRL_TYPE_MPEG2_QUANTIZATION = 0x0104,
> > +      * V4L2_CTRL_TYPE_FWHT_PARAMS = 0x0105,
> > +      */
>
> leftovers ?
>
> > +     V4L2_CTRL_TYPE_PIXEL_SIZE    = 0x0106,
>
> I don't see other compound controls with a custom payload adding their
> types here. What am I missing?
>
> Thanks
>   j
>
> >  };
> >
> >  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
> > --
> > 2.23.0.rc1
> >

[-- Attachment #2: signature.asc --]
[-- Type: text/plain, Size: 849 bytes --]

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEtcQ9SICaIIqPWDjAcjQGjxahVjwFAl1dbUYACgkQcjQGjxah
VjzMSg//T3Z0Pn/MM4rl0d9E08+UhovwymM92g/k2giBnTTDmLyZSanh4wJ5z95z
CcySqDqSdsgk4oe7BSMR3+kaK/O0M9o7wkzNUIzpgT0H2zPIkhLo4rkYN97W2XCj
eVIcUkdK0YSRpJLUYXP4PyTYMNhwwmP8ugCtfnhdMnVP5ly5H/jHcH9PVlBUVpaq
qrZVRmfzX7N6jt5L3QE7EMXnH0OouQuZRE7c0RqcRYkFwtsyDtu7Wc9cfAxQGKW2
Udkx387YTsLGTvh3TgVyiR28fonYxfo48lj8YPM49O53stwsqgMeg667UJ6HPzko
RY6QUwbz9Yo5ThnMhfIRqIP88wPTYyd+gls8Hs6PJ+LZx5AGYavfBvtWI15NuRSc
+VKrN9s2pqrO4SJsZvtxR0P4he6SeGPNDjozLv2rgtrJDlWstPKe97fwoRWBhFwd
xb8dHGD6LD5X6bDiJNWFNs5gMti3lMH92CfXC36S2OktV40EVAXWQCryQWohH3cm
hvFbS8odj2AvXP+1ogErP5QgZe4iI+kDct1kUxt1TjE3iWjbvIENpjgTU9WUvM70
VY6YcW58J1vk+DZHORiEbB5lA/8cDeoWYSVTROKq9FPrfGxOybwzLzcVfo5N37QO
JaPhMfm1TLiecZV2w8KdpdfHsEJF3QEGDy9pBztLV/B8FCw7xA0=
=+EdP
-----END PGP SIGNATURE-----

Re: [PATCH 3/3] media: imx214: Add new control with V4L2_CID_PIXEL_SIZE

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 3788 bytes --]

Hi Ricardo,

On Wed, Aug 21, 2019 at 06:31:05PM +0200, Ricardo Ribalda Delgado wrote:
> Hi Jacopo
>
>
> On Wed, Aug 21, 2019 at 6:14 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi Ricardo,
> >
> > On Mon, Aug 19, 2019 at 02:17:20PM +0200, Ricardo Ribalda Delgado wrote:
> > > According to the product brief, the unit cell size is 1120 nanometers^2.
> >
> > Should this information come from DT ?
>
> I do not think so. You cannot change this value and it needs to be
> defined also in sensors/cameras that might not have a DT, like a usb
> webcam.

You're probably right. I got this thinking because the camera
location/orientation are two read only parameters that come from DT,
but their value depends on the design of the device where the camera
is installed on, so they're configurable, while this and other
physical properties are not, and it doesn't make much sense to have
them in DT.

Thanks
   j

>
> It would be like adding to the DT the min/max exposure time...
>
> But of course we can discuss it ;)
>
> Best regards
>
> >
> > I'm asking as I've a series in review that adds an helper that
> > collectes dt properties and register controls for them. It currently
> > only supports the newly proposed camera location control, but there
> > might be others like the rotation, for which we already have a DT
> > property.
> >
> > https://patchwork.kernel.org/project/linux-media/list/?series=160901
> >
> > This new one is indeed an HW property of the sensor, I wonder if
> > having it in the firmware interface would make any sense or not...
> >
> > Thanks
> >   j
> >
> > >
> > > https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf
> > >
> > > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> > > ---
> > >  drivers/media/i2c/imx214.c | 23 +++++++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > > index 159a3a604f0e..b2f6bd2d8d7d 100644
> > > --- a/drivers/media/i2c/imx214.c
> > > +++ b/drivers/media/i2c/imx214.c
> > > @@ -47,6 +47,7 @@ struct imx214 {
> > >       struct v4l2_ctrl *pixel_rate;
> > >       struct v4l2_ctrl *link_freq;
> > >       struct v4l2_ctrl *exposure;
> > > +     struct v4l2_ctrl *pixel_size;
> > >
> > >       struct regulator_bulk_data      supplies[IMX214_NUM_SUPPLIES];
> > >
> > > @@ -941,6 +942,26 @@ static int __maybe_unused imx214_resume(struct device *dev)
> > >       return ret;
> > >  }
> > >
> > > +static void pixel_size_init(const struct v4l2_ctrl *ctrl, u32 idx,
> > > +                  union v4l2_ctrl_ptr ptr)
> > > +{
> > > +     ptr.p_pixel_size->width = 1120;
> > > +     ptr.p_pixel_size->height = 1120;
> > > +}
> > > +
> > > +static const struct v4l2_ctrl_type_ops pixel_size_ops = {
> > > +     .init = pixel_size_init,
> > > +};
> > > +
> > > +static struct v4l2_ctrl *new_pixel_size_ctrl(struct v4l2_ctrl_handler *handler)
> > > +{
> > > +     static struct v4l2_ctrl_config ctrl = {
> > > +             .id = V4L2_CID_PIXEL_SIZE,
> > > +             .type_ops = &pixel_size_ops,
> > > +     };
> > > +
> > > +     return v4l2_ctrl_new_custom(handler, &ctrl, NULL);
> > > +}
> > >  static int imx214_probe(struct i2c_client *client)
> > >  {
> > >       struct device *dev = &client->dev;
> > > @@ -1029,6 +1050,8 @@ static int imx214_probe(struct i2c_client *client)
> > >                                            V4L2_CID_EXPOSURE,
> > >                                            0, 3184, 1, 0x0c70);
> > >
> > > +     imx214->pixel_size = new_pixel_size_ctrl(&imx214->ctrls);
> > > +
> > >       ret = imx214->ctrls.error;
> > >       if (ret) {
> > >               dev_err(&client->dev, "%s control init failed (%d)\n",
> > > --
> > > 2.23.0.rc1
> > >

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]